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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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