Re: [PATCH v2 1/2] hw/riscv/virt.c: fix non-KVM --enable-debug build

[Andrew Jones <ajones@ventanamicro.com>]

On Thu, Aug 31, 2023 at 02:47:54PM +0200, Philippe Mathieu-Daudé wrote:
> On 31/8/23 10:42, Andrew Jones wrote:
> > On Wed, Aug 30, 2023 at 10:35:02AM -0300, Daniel Henrique Barboza wrote:
> > > A build with --enable-debug and without KVM will fail as follows:
> > > 
> > > /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_riscv_virt.c.o: in function `virt_machine_init':
> > > ./qemu/build/../hw/riscv/virt.c:1465: undefined reference to `kvm_riscv_aia_create'
> > > 
> > > This happens because the code block with "if virt_use_kvm_aia(s)" isn't
> > > being ignored by the debug build, resulting in an undefined reference to
> > > a KVM only function.
> > > 
> > > Add a 'kvm_enabled()' conditional together with virt_use_kvm_aia() will
> > > make the compiler crop the kvm_riscv_aia_create() call entirely from a
> > > non-KVM build. Note that adding the 'kvm_enabled()' conditional inside
> > > virt_use_kvm_aia() won't fix the build because this function would need
> > > to be inlined multiple times to make the compiler zero out the entire
> > > block.
> > > 
> > > While we're at it, use kvm_enabled() in all instances where
> > > virt_use_kvm_aia() is checked to allow the compiler to elide these other
> > > kvm-only instances as well.
> > > 
> > > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > > Fixes: dbdb99948e ("target/riscv: select KVM AIA in riscv virt machine")
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   hw/riscv/virt.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> 
> > I think I'd prefer
> > 
> >   /* We need this inlined for debug (-O0) builds */
> >   static inline QEMU_ALWAYS_INLINE bool virt_use_kvm_aia(RISCVVirtState *s)
> >   {
> >      return kvm_enabled() && kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
> 
> Generally we should know whether KVM is enabled or not _before_
> calling any foo_kvm() code, not after.

That's reasonable and makes me want to suggest squashing the diff below
into the second patch.


diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 592c3ce76828..f712699a4040 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -816,7 +816,7 @@ static void riscv_aplic_realize(DeviceState *dev, Error **errp)
     uint32_t i;
     RISCVAPLICState *aplic = RISCV_APLIC(dev);
 
-    if (!is_kvm_aia(aplic->msimode)) {
+    if (!kvm_enabled() || !is_kvm_aia(aplic->msimode)) {
         aplic->bitfield_words = (aplic->num_irqs + 31) >> 5;
         aplic->sourcecfg = g_new0(uint32_t, aplic->num_irqs);
         aplic->state = g_new0(uint32_t, aplic->num_irqs);
@@ -980,7 +980,7 @@ DeviceState *riscv_aplic_create(hwaddr addr, hwaddr size,
 
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
-    if (!is_kvm_aia(msimode)) {
+    if (!kvm_enabled() || !is_kvm_aia(msimode)) {
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
     }