Re: [libvirt] [PATCH 4/9] util: add RISC-V support
On Tue, 2018-05-15 at 12:53 +0200, Lubomir Rintel wrote:
What works, with images from [1]:
virt-install \
--import --name riscv64 \
--arch riscv64 --machine virt \
--memory 2048 \
--rng /dev/urandom \
--disk /var/lib/libvirt/images/stage4-disk.img,bus=virtio \
--boot kernel=/var/lib/libvirt/images/bbl,kernel_args="console=ttyS0 ro
root=/dev/vda"
[1] https://fedorapeople.org/groups/risc-v/disk-images/
Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk>
---
docs/schemas/basictypes.rng | 2 ++
src/qemu/qemu_command.c | 15 ++++++++-------
src/qemu/qemu_domain.c | 18 +++++++++++++++---
src/qemu/qemu_domain_address.c | 21 +++++++++++----------
src/util/virarch.c | 3 +++
src/util/virarch.h | 6 ++++++
6 files changed, 45 insertions(+), 20 deletions(-)
This patch breaks 'make check'. Please make sure that both 'make
check' and 'make syntax-check' pass after every single patch in
the series - it's a requirement for getting it merged.
I'll wait for a respin that addresses that before moving forward
with the review, but I'd like to make a few comments on the series
as a whole.
The way you've organized your changes could use some improvements:
for example, in this patch you're both teaching libvirt about the
RISC-V architecture and changing guest behavior, which is basically
two or more changes squashed into a single patch; moreover, I would
expect the former to happen fairly early in the series, certainly
before introducing a RISC-V specific serial device.
Squashing patches together is easier than disentangling unrelated
changes, so when in doubt always err on the side of small patches ;)
Overall, I would expect the series to be organized along the
lines of
* preparatory work, such as creating a wrapper around
qemuDomainAssignARMVirtioMMIOAddresses();
* teaching libvirt about the RISC-V architecture;
* teaching libvirt about the new serial console model (and
potentially target type);
* putting it all together;
* updating the test suite.
I'm not saying you have to follow this to the letter, and more
specifically some changes to the test suite might need to happen
at the same time as you add RISC-V support, but overall it should
end up looking similar to that.
Looking forward to v2 :)
--
Andrea Bolognani / Red Hat / Virtualization