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

[Daniel Henrique Barboza]

On 8/29/23 22:26, Richard Henderson wrote:
> On 8/29/23 16:51, Daniel Henrique Barboza wrote:
> > > The compiler certainly does eliminate 0 && foo(), even at -O0.
> > >
> > > There must be something else going on.
> > > Pointer to your tree?
> >
> > It's this tree:
> >
> > https://github.com/alistair23/qemu/tree/riscv-to-apply.next
>
>
> Ok, so while -O0 will eliminate 0 && foo(), it doesn't eliminate with bar() && 
> foo(), where bar must be inlined (multiple times in this case) to find the 0.
>
> Moreover in the case of
>
> > /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_intc_riscv_aplic.c.o: in function 
> > `riscv_kvm_aplic_request':
> > /home/danielhb/work/qemu/build/../hw/intc/riscv_aplic.c:486: undefined 
> > reference to `kvm_set_irq'
>
> this one, where foo (aka riscv_kvm_aplic_request) would have to be eliminated 
> as well. But the compiler won't eliminate entire unused functions with -O0.
>
> This seems to do the trick.  Whether it is aesthetically better than what you 
> had with your patches, I will leave to someone else.
>
>
> r~
>
>
> diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> index 592c3ce768..0e22dcaf8a 100644
> --- a/hw/intc/riscv_aplic.c
> +++ b/hw/intc/riscv_aplic.c
> @@ -481,10 +481,14 @@ static uint32_t riscv_aplic_idc_claimi(RISCVAPLICState 
> *aplic, uint32_t idc)
>       return topi;
>   }
>
> +#ifdef CONFIG_KVM
>   static void riscv_kvm_aplic_request(void *opaque, int irq, int level)
>   {
>       kvm_set_irq(kvm_state, irq, !!level);
>   }
> +#else
> +#define riscv_kvm_aplic_request  ({ qemu_build_not_reached(); NULL; })
> +#endif
>
>   static void riscv_aplic_request(void *opaque, int irq, int level)
>   {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 388e52a294..b787ae38c2 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -782,7 +782,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>       }
>
>       /* KVM AIA only has one APLIC instance */
> -    if (virt_use_kvm_aia(s)) {
> +    if (kvm_enabled() && virt_use_kvm_aia(s)) {
>           create_fdt_socket_aplic(s, memmap, 0,
>                                   msi_m_phandle, msi_s_phandle, phandle,
>                                   &intc_phandles[0], xplic_phandles,
> @@ -1461,7 +1461,7 @@ static void virt_machine_init(MachineState *machine)
>           }
>       }
>
> -    if (virt_use_kvm_aia(s)) {
> +    if (kvm_enabled() && virt_use_kvm_aia(s)) {
>           kvm_riscv_aia_create(machine, IMSIC_MMIO_GROUP_MIN_SHIFT,
>                                VIRT_IRQCHIP_NUM_SOURCES, VIRT_IRQCHIP_NUM_MSIS,
>                                memmap[VIRT_APLIC_S].base,

I'll leave to Alistair to decide, both seems good to me.


TBH I'm bothered why this doesn't work:


diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 592c3ce768..251e08ddc4 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -839,12 +839,16 @@ static void riscv_aplic_realize(DeviceState *dev, Error 
**errp)
      * Only root APLICs have hardware IRQ lines. All non-root APLICs
      * have IRQ lines delegated by their parent APLIC.
      */
-    if (!aplic->parent) {
-        if (is_kvm_aia(aplic->msimode)) {
-            qdev_init_gpio_in(dev, riscv_kvm_aplic_request, aplic->num_irqs);
-        } else {
-            qdev_init_gpio_in(dev, riscv_aplic_request, aplic->num_irqs);
+    if (kvm_enabled()) {
+        if (!aplic->parent) {
+            if (is_kvm_aia(aplic->msimode)) {
+                qdev_init_gpio_in(dev, riscv_kvm_aplic_request, 
aplic->num_irqs);
+            } else {
+                qdev_init_gpio_in(dev, riscv_aplic_request, aplic->num_irqs);
+            }
         }
+    } else if (!aplic->parent) {
+        qdev_init_gpio_in(dev, riscv_aplic_request, aplic->num_irqs);
     }

Why is the compiler refusing to crop an "if kvm_enabled()" block? There's no 
other
conditionals to handle, and it is able to crop "if (kvm_enabled() && 
virt_use_kvm_aia(s))".

Is this solely because riscv_kvm_aplic_request() will be an unused function if 
the crop
happens and, as you said above, "the compiler won't eliminate entire unused 
functions with
-O0"?



Thanks,

Daniel