[PATCH 0/2] riscv: fix --enable-debug in riscv-to-apply.next

[PATCH 0/2] riscv: fix --enable-debug in riscv-to-apply.next

[Daniel Henrique Barboza]

Hi,

There is a couple of build problems that sneaked in riscv-to-apply.next
in the KVM AIA patches. Here's a potential solution for them.

Alistair, feel free to squash the fixes in the original commits since
they're not yet on master.

Cc: Yong-Xuan Wang <yongxuan.wang@sifive.com>

Daniel Henrique Barboza (2):
  hw/intc/riscv_aplic.c fix non-KVM --enable-debug build
  hw/riscv/virt.c: fix non-KVM --enable-debug build

 hw/intc/riscv_aplic.c    |  2 ++
 target/riscv/kvm_riscv.h | 10 ++++++++++
 2 files changed, 12 insertions(+)

-- 
2.41.0

[PATCH 1/2] hw/intc/riscv_aplic.c fix non-KVM --enable-debug build

[Daniel Henrique Barboza]

Commit 6df0b37e2ab breaks a --enable-debug build in a non-KVM
environment with the following error:

/usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_intc_riscv_aplic.c.o: in function `riscv_kvm_aplic_request':
./qemu/build/../hw/intc/riscv_aplic.c:486: undefined reference to `kvm_set_irq'
collect2: error: ld returned 1 exit status

This happens because the debug build will poke into the
'if (is_kvm_aia(aplic->msimode))' block and fail to find a reference to
the KVM only function riscv_kvm_aplic_request().

Make riscv_kvm_aplic_request() a no-op if we're not building KVM.

Fixes: 6df0b37e2ab ("target/riscv: update APLIC and IMSIC to support KVM AIA")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/intc/riscv_aplic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 592c3ce768..b634738bda 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -483,7 +483,9 @@ static uint32_t riscv_aplic_idc_claimi(RISCVAPLICState *aplic, uint32_t idc)
 
 static void riscv_kvm_aplic_request(void *opaque, int irq, int level)
 {
+#ifdef CONFIG_KVM
     kvm_set_irq(kvm_state, irq, !!level);
+#endif
 }
 
 static void riscv_aplic_request(void *opaque, int irq, int level)
-- 
2.41.0

Re: [PATCH 1/2] hw/intc/riscv_aplic.c fix non-KVM --enable-debug build

[Philippe Mathieu-Daudé]

On 29/8/23 14:21, Daniel Henrique Barboza wrote:
> Commit 6df0b37e2ab breaks a --enable-debug build in a non-KVM
> environment with the following error:
> 
> /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_intc_riscv_aplic.c.o: in function `riscv_kvm_aplic_request':
> ./qemu/build/../hw/intc/riscv_aplic.c:486: undefined reference to `kvm_set_irq'
> collect2: error: ld returned 1 exit status
> 
> This happens because the debug build will poke into the
> 'if (is_kvm_aia(aplic->msimode))' block and fail to find a reference to
> the KVM only function riscv_kvm_aplic_request().
> 
> Make riscv_kvm_aplic_request() a no-op if we're not building KVM.
> 
> Fixes: 6df0b37e2ab ("target/riscv: update APLIC and IMSIC to support KVM AIA")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/intc/riscv_aplic.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> index 592c3ce768..b634738bda 100644
> --- a/hw/intc/riscv_aplic.c
> +++ b/hw/intc/riscv_aplic.c
> @@ -483,7 +483,9 @@ static uint32_t riscv_aplic_idc_claimi(RISCVAPLICState *aplic, uint32_t idc)
>   
>   static void riscv_kvm_aplic_request(void *opaque, int irq, int level)
>   {
> +#ifdef CONFIG_KVM
>       kvm_set_irq(kvm_state, irq, !!level);
> +#endif
>   }

Can we use kvm_enabled() to let the compiler elide KVM code
when !CONFIG_KVM?

-- >8 --
diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 592c3ce768..f28d0d3237 100644
@@ -155,7 +155,7 @@
   */
  static bool is_kvm_aia(bool msimode)
  {
-    return kvm_irqchip_in_kernel() && msimode;
+    return kvm_enabled() && kvm_irqchip_in_kernel() && msimode;
  }
---

Re: [PATCH 1/2] hw/intc/riscv_aplic.c fix non-KVM --enable-debug build

[Daniel Henrique Barboza]

On 8/29/23 11:17, Philippe Mathieu-Daudé wrote:
> On 29/8/23 14:21, Daniel Henrique Barboza wrote:
>> Commit 6df0b37e2ab breaks a --enable-debug build in a non-KVM
>> environment with the following error:
>>
>> /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_intc_riscv_aplic.c.o: in function `riscv_kvm_aplic_request':
>> ./qemu/build/../hw/intc/riscv_aplic.c:486: undefined reference to `kvm_set_irq'
>> collect2: error: ld returned 1 exit status
>>
>> This happens because the debug build will poke into the
>> 'if (is_kvm_aia(aplic->msimode))' block and fail to find a reference to
>> the KVM only function riscv_kvm_aplic_request().
>>
>> Make riscv_kvm_aplic_request() a no-op if we're not building KVM.
>>
>> Fixes: 6df0b37e2ab ("target/riscv: update APLIC and IMSIC to support KVM AIA")
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/intc/riscv_aplic.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
>> index 592c3ce768..b634738bda 100644
>> --- a/hw/intc/riscv_aplic.c
>> +++ b/hw/intc/riscv_aplic.c
>> @@ -483,7 +483,9 @@ static uint32_t riscv_aplic_idc_claimi(RISCVAPLICState *aplic, uint32_t idc)
>>   static void riscv_kvm_aplic_request(void *opaque, int irq, int level)
>>   {
>> +#ifdef CONFIG_KVM
>>       kvm_set_irq(kvm_state, irq, !!level);
>> +#endif
>>   }
> 
> Can we use kvm_enabled() to let the compiler elide KVM code
> when !CONFIG_KVM?
> 
> -- >8 --
> diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> index 592c3ce768..f28d0d3237 100644
> @@ -155,7 +155,7 @@
>    */
>   static bool is_kvm_aia(bool msimode)
>   {
> -    return kvm_irqchip_in_kernel() && msimode;
> +    return kvm_enabled() && kvm_irqchip_in_kernel() && msimode;

Apparently we can't:


$ git diff
diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index b634738bda..f28d0d3237 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -155,7 +155,7 @@
   */
  static bool is_kvm_aia(bool msimode)
  {
-    return kvm_irqchip_in_kernel() && msimode;
+    return kvm_enabled() && kvm_irqchip_in_kernel() && msimode;
  }
  
  static uint32_t riscv_aplic_read_input_word(RISCVAPLICState *aplic,
@@ -483,9 +483,7 @@ static uint32_t riscv_aplic_idc_claimi(RISCVAPLICState *aplic, uint32_t idc)
  
  static void riscv_kvm_aplic_request(void *opaque, int irq, int level)
  {
-#ifdef CONFIG_KVM
      kvm_set_irq(kvm_state, irq, !!level);
-#endif
  }

Same error appears:


/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'
collect2: error: ld returned 1 exit status


Thanks,


Daniel



>   }
> ---
> 

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

[Daniel Henrique Barboza]

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 stub for kvm_riscv_aia_create() in kvm_riscv.h when CONFIG_KVM is
false. Adding it as an inline instead of using kvm-stubs.c will make it
easier in the future to remember to add stubs for kvm functions that are
used in multiple accelerator code.

Fixes: dbdb99948e ("target/riscv: select KVM AIA in riscv virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm_riscv.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index 7d4b7c60e2..01be45cc69 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -22,9 +22,19 @@
 void kvm_riscv_init_user_properties(Object *cpu_obj);
 void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
+
+#ifdef CONFIG_KVM
 void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
                           uint64_t aia_irq_num, uint64_t aia_msi_num,
                           uint64_t aplic_base, uint64_t imsic_base,
                           uint64_t guest_num);
+#else
+static inline void kvm_riscv_aia_create(MachineState *machine,
+                                uint64_t group_shift, uint64_t aia_irq_num,
+                                uint64_t aia_msi_num, uint64_t aplic_base,
+                                uint64_t imsic_base, uint64_t guest_num) {
+    g_assert_not_reached();
+}
+#endif
 
 #endif
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

On 29/8/23 14:21, 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 stub for kvm_riscv_aia_create() in kvm_riscv.h when CONFIG_KVM is
> false. Adding it as an inline instead of using kvm-stubs.c will make it
> easier in the future to remember to add stubs for kvm functions that are
> used in multiple accelerator code.
> 
> Fixes: dbdb99948e ("target/riscv: select KVM AIA in riscv virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/kvm_riscv.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)

Similarly:

-- >8 --
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
@@ -79,7 +79,9 @@
  /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by 
QEMU. */
  static bool virt_use_kvm_aia(RISCVVirtState *s)
  {
-    return kvm_irqchip_in_kernel() && s->aia_type == 
VIRT_AIA_TYPE_APLIC_IMSIC;
+    return kvm_enabled()
+        && kvm_irqchip_in_kernel()
+        && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
  }
---

?

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

[Daniel Henrique Barboza]

On 8/29/23 11:18, Philippe Mathieu-Daudé wrote:
> On 29/8/23 14:21, 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 stub for kvm_riscv_aia_create() in kvm_riscv.h when CONFIG_KVM is
>> false. Adding it as an inline instead of using kvm-stubs.c will make it
>> easier in the future to remember to add stubs for kvm functions that are
>> used in multiple accelerator code.
>>
>> Fixes: dbdb99948e ("target/riscv: select KVM AIA in riscv virt machine")
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm_riscv.h | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
> 
> Similarly:
> 
> -- >8 --
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> @@ -79,7 +79,9 @@
>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>   static bool virt_use_kvm_aia(RISCVVirtState *s)
>   {
> -    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
> +    return kvm_enabled()
> +        && kvm_irqchip_in_kernel()
> +        && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>   }
> ---

It doesn't work. Same error:

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 388e52a294..ac710006e7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -79,7 +79,8 @@
  /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
  static bool virt_use_kvm_aia(RISCVVirtState *s)
  {
-    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
+    return kvm_enabled() &&
+           kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
  }
  
  static const MemMapEntry virt_memmap[] = {
diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index 01be45cc69..7d4b7c60e2 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -22,19 +22,9 @@
  void kvm_riscv_init_user_properties(Object *cpu_obj);
  void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
-
-#ifdef CONFIG_KVM
  void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
                            uint64_t aia_irq_num, uint64_t aia_msi_num,
                            uint64_t aplic_base, uint64_t imsic_base,
                            uint64_t guest_num);
-#else
-static inline void kvm_riscv_aia_create(MachineState *machine,
-                                uint64_t group_shift, uint64_t aia_irq_num,
-                                uint64_t aia_msi_num, uint64_t aplic_base,
-                                uint64_t imsic_base, uint64_t guest_num) {
-    g_assert_not_reached();
-}
-#endif



/usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_riscv_virt.c.o: in function `virt_machine_init':
/home/danielhb/work/qemu/build/../hw/riscv/virt.c:1466: undefined reference to `kvm_riscv_aia_create'
collect2: error: ld returned 1 exit status


I'm no compiler expert by any means but it seems that the --enable-debug build does not strip things
out like the usual build does, e.g. it won't elide a 'if kvm_enabled()' block out by checking that
kvm_enabled() is always false.


Thanks,

Daniel


> 
> ?

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

[Richard Henderson]

On 8/29/23 16:09, Daniel Henrique Barboza wrote:
>> -- >8 --
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> @@ -79,7 +79,9 @@
>>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>>   static bool virt_use_kvm_aia(RISCVVirtState *s)
>>   {
>> -    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>> +    return kvm_enabled()
>> +        && kvm_irqchip_in_kernel()
>> +        && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>>   }
>> ---
> 
> It doesn't work. Same error:
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 388e52a294..ac710006e7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -79,7 +79,8 @@
>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>   static bool virt_use_kvm_aia(RISCVVirtState *s)
>   {
> -    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
> +    return kvm_enabled() &&
> +           kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>   }
> 
>   static const MemMapEntry virt_memmap[] = {
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index 01be45cc69..7d4b7c60e2 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -22,19 +22,9 @@
>   void kvm_riscv_init_user_properties(Object *cpu_obj);
>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
> -
> -#ifdef CONFIG_KVM
>   void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>                             uint64_t aia_irq_num, uint64_t aia_msi_num,
>                             uint64_t aplic_base, uint64_t imsic_base,
>                             uint64_t guest_num);
> -#else
> -static inline void kvm_riscv_aia_create(MachineState *machine,
> -                                uint64_t group_shift, uint64_t aia_irq_num,
> -                                uint64_t aia_msi_num, uint64_t aplic_base,
> -                                uint64_t imsic_base, uint64_t guest_num) {
> -    g_assert_not_reached();
> -}
> -#endif
> 
> 
> 
> /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_riscv_virt.c.o: in function `virt_machine_init':
> /home/danielhb/work/qemu/build/../hw/riscv/virt.c:1466: undefined reference to 
> `kvm_riscv_aia_create'
> collect2: error: ld returned 1 exit status
> 
> 
> I'm no compiler expert by any means but it seems that the --enable-debug build does not 
> strip things
> out like the usual build does, e.g. it won't elide a 'if kvm_enabled()' block out by 
> checking that
> kvm_enabled() is always false.

The compiler certainly does eliminate 0 && foo(), even at -O0.

There must be something else going on.
Pointer to your tree?

r~

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

[Daniel Henrique Barboza]

On 8/29/23 20:30, Richard Henderson wrote:
> On 8/29/23 16:09, Daniel Henrique Barboza wrote:
>>> -- >8 --
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> @@ -79,7 +79,9 @@
>>>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>>>   static bool virt_use_kvm_aia(RISCVVirtState *s)
>>>   {
>>> -    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>>> +    return kvm_enabled()
>>> +        && kvm_irqchip_in_kernel()
>>> +        && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>>>   }
>>> ---
>>
>> It doesn't work. Same error:
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 388e52a294..ac710006e7 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -79,7 +79,8 @@
>>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>>   static bool virt_use_kvm_aia(RISCVVirtState *s)
>>   {
>> -    return kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>> +    return kvm_enabled() &&
>> +           kvm_irqchip_in_kernel() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>>   }
>>
>>   static const MemMapEntry virt_memmap[] = {
>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
>> index 01be45cc69..7d4b7c60e2 100644
>> --- a/target/riscv/kvm_riscv.h
>> +++ b/target/riscv/kvm_riscv.h
>> @@ -22,19 +22,9 @@
>>   void kvm_riscv_init_user_properties(Object *cpu_obj);
>>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
>>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>> -
>> -#ifdef CONFIG_KVM
>>   void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>>                             uint64_t aia_irq_num, uint64_t aia_msi_num,
>>                             uint64_t aplic_base, uint64_t imsic_base,
>>                             uint64_t guest_num);
>> -#else
>> -static inline void kvm_riscv_aia_create(MachineState *machine,
>> -                                uint64_t group_shift, uint64_t aia_irq_num,
>> -                                uint64_t aia_msi_num, uint64_t aplic_base,
>> -                                uint64_t imsic_base, uint64_t guest_num) {
>> -    g_assert_not_reached();
>> -}
>> -#endif
>>
>>
>>
>> /usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_riscv_virt.c.o: in function `virt_machine_init':
>> /home/danielhb/work/qemu/build/../hw/riscv/virt.c:1466: undefined reference to `kvm_riscv_aia_create'
>> collect2: error: ld returned 1 exit status
>>
>>
>> I'm no compiler expert by any means but it seems that the --enable-debug build does not strip things
>> out like the usual build does, e.g. it won't elide a 'if kvm_enabled()' block out by checking that
>> kvm_enabled() is always false.
> 
> 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


Building using --enable-debug fails:


[danielhb@grind build]$ ../configure --target-list=riscv64-softmmu --enable-debug && make -j
(...)
0.so /usr/lib64/libcairo-gobject.so /usr/lib64/libcairo.so /usr/lib64/libgdk_pixbuf-2.0.so /usr/lib64/libX11.so /usr/lib64/libvirglrenderer.so /usr/lib64/libcacard.so /usr/lib64/libusbredirparser.so /usr/lib64/libusb-1.0.so -lbrlapi @block.syms -lnuma /usr/lib64/liburing.so -lm /usr/lib64/libfuse3.so -lpthread /usr/lib64/iscsi/libiscsi.so -laio /usr/lib64/libcurl.so /usr/lib64/libnfs.so /usr/lib64/libssh.so -lrbd -lrados -lbz2 -lutil -Wl,--end-group
/usr/bin/ld: libqemu-riscv64-softmmu.fa.p/hw_riscv_virt.c.o: in function `virt_machine_init':
/home/danielhb/work/qemu/build/../hw/riscv/virt.c:1465: undefined reference to `kvm_riscv_aia_create'
/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'
collect2: error: ld returned 1 exit status

Building without --enable-debug works. gitlab CI doesn't seem to care with it because it's all
green with this tree.


The commits that introduced these errors are listed in the 'Fixes' tag of patches 1 and 2.


Thanks,

Daniel








> 
> r~

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

[Richard Henderson]

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,

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

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

[Richard Henderson]

On 8/30/23 02:34, Daniel Henrique Barboza wrote:
> 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"?

Yes, exactly.


r~