From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-ID: <20230830133503.711138-2-dbarboza@ventanamicro.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 496c17c644..5edc1d98d2 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,
@@ -808,7 +808,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
 
     g_free(intc_phandles);
 
-    if (virt_use_kvm_aia(s)) {
+    if (kvm_enabled() && virt_use_kvm_aia(s)) {
         *irq_mmio_phandle = xplic_phandles[0];
         *irq_virtio_phandle = xplic_phandles[0];
         *irq_pcie_phandle = xplic_phandles[0];
@@ -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,
-- 
2.41.0

11.09.2023 09:43, Alistair Francis:
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> 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")

This is probably commit 48c2c33c52, not dbdb99948e.

..
>       /* 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,
...

As has been discovered earlier (see "target/i386: Restrict system-specific
features from user emulation" threads), this is not enough, - compiler does
not always eliminate if (0 && foo) { bar; } construct, it's too fragile and
does not actually work.  Either some #ifdef'fery is needed here or some other,
more explicit, way to eliminate such code, like introducing stub functions.

I know it's too late and this change is already in, but unfortunately that's
when I noticed this.  While the "Fixes:" tag can't be changed anymore, a more
proper fix for the actual problem (with the proper Fixes tag now) can still
be applied on top of this fix.

/mjt

On 9/11/23 16:54, Michael Tokarev wrote:
> 11.09.2023 09:43, Alistair Francis:
>> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>
>> 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")
> 
> This is probably commit 48c2c33c52, not dbdb99948e.
> 
> ..
>>       /* 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,
> ...
> 
> As has been discovered earlier (see "target/i386: Restrict system-specific
> features from user emulation" threads), this is not enough, - compiler does
> not always eliminate if (0 && foo) { bar; } construct, it's too fragile and
> does not actually work.  Either some #ifdef'fery is needed here or some other,
> more explicit, way to eliminate such code, like introducing stub functions.
> 
> I know it's too late and this change is already in, but unfortunately that's
> when I noticed this.  While the "Fixes:" tag can't be changed anymore, a more
> proper fix for the actual problem (with the proper Fixes tag now) can still
> be applied on top of this fix.

This works fine on current master on my machine:

$ ../configure --cc=clang --target-list=riscv64-softmmu,riscv64-linux-user,riscv32-softmmu,riscv32-linux-user --enable-debug
$ make -j

So I'm not sure what do you mean by 'actual problem'. If you refer to the non-existence
of stub functions, earlier today we had a review by Phil in this patch here where I was
adding stubs for all KVM functions:

https://lore.kernel.org/qemu-riscv/f30d8589-8b59-2fd7-c38c-3f79508a4ac6@linaro.org/

Phil mentioned that we don't need a function stub if the KVM only function is protected by
"kvm_enabled()". And this is fine, but then, from what you're saying, we can't rely on
(kvm_enabled() && foo) working all the time, so we're one conditional away from breaking
things it seems.

I think we need to make up our minds and standardize. Do we rely on the compiler to optimize
stuff out, or do we use stubs/ifdefs? I'm fine with both options, as long as we pick one and
stick with it.


Thanks,

Daniel

> 
> /mjt

12.09.2023 00:43, Daniel Henrique Barboza:
> On 9/11/23 16:54, Michael Tokarev wrote:
...
>>>       /* 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,
>> ...
>>
>> As has been discovered earlier (see "target/i386: Restrict system-specific
>> features from user emulation" threads), this is not enough, - compiler does
>> not always eliminate if (0 && foo) { bar; } construct, it's too fragile and
>> does not actually work.  Either some #ifdef'fery is needed here or some other,
>> more explicit, way to eliminate such code, like introducing stub functions.
>>
>> I know it's too late and this change is already in, but unfortunately that's
>> when I noticed this.  While the "Fixes:" tag can't be changed anymore, a more
>> proper fix for the actual problem (with the proper Fixes tag now) can still
>> be applied on top of this fix.
> 
> This works fine on current master on my machine:
> 
> $ ../configure --cc=clang --target-list=riscv64-softmmu,riscv64-linux-user,riscv32-softmmu,riscv32-linux-user --enable-debug
> $ make -j
> 
> So I'm not sure what do you mean by 'actual problem'. If you refer to the non-existence
> of stub functions, earlier today we had a review by Phil in this patch here where I was
> adding stubs for all KVM functions:
> 
> https://lore.kernel.org/qemu-riscv/f30d8589-8b59-2fd7-c38c-3f79508a4ac6@linaro.org/
> 
> Phil mentioned that we don't need a function stub if the KVM only function is protected by
> "kvm_enabled()". And this is fine, but then, from what you're saying, we can't rely on
> (kvm_enabled() && foo) working all the time, so we're one conditional away from breaking
> things it seems.

Please see:

https://lore.kernel.org/qemu-devel/20230911211317.28773-1-philmd@linaro.org/T/#t  (fix v4)
https://lore.kernel.org/qemu-devel/ZP9Cmqgy2H3ypDf3@redhat.com/T/#t (fix v3)
https://lore.kernel.org/qemu-devel/28c832bc-2fbf-8caa-e141-51288fc0d544@linaro.org/T/#t (fix v2)
https://lore.kernel.org/qemu-devel/ZP74b%2FByEaVW5bZO@redhat.com/T/#t (fix v1)

and the original issue at
https://lore.kernel.org/qemu-devel/8ee6684b-cdc3-78cb-9b76-e5875743bdcf@tls.msk.ru/T/#m65801e9edf31688a45722271a97e628ec98a0c23
(this is an i386 pullreq which removed stub functions in presence of (!kvm_enabled() && foo)).

It is exactly the same issue as this one, with exactly the same fix, which resulted in
breakage.  I dunno why your build succeeded, but the whole thing is.. not easy.

/mjt

On 9/12/23 03:05, Michael Tokarev wrote:
> 12.09.2023 00:43, Daniel Henrique Barboza:
>> On 9/11/23 16:54, Michael Tokarev wrote:
> ...
>>>>       /* 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,
>>> ...
>>>
>>> As has been discovered earlier (see "target/i386: Restrict system-specific
>>> features from user emulation" threads), this is not enough, - compiler does
>>> not always eliminate if (0 && foo) { bar; } construct, it's too fragile and
>>> does not actually work.  Either some #ifdef'fery is needed here or some other,
>>> more explicit, way to eliminate such code, like introducing stub functions.
>>>
>>> I know it's too late and this change is already in, but unfortunately that's
>>> when I noticed this.  While the "Fixes:" tag can't be changed anymore, a more
>>> proper fix for the actual problem (with the proper Fixes tag now) can still
>>> be applied on top of this fix.
>>
>> This works fine on current master on my machine:
>>
>> $ ../configure --cc=clang --target-list=riscv64-softmmu,riscv64-linux-user,riscv32-softmmu,riscv32-linux-user --enable-debug
>> $ make -j
>>
>> So I'm not sure what do you mean by 'actual problem'. If you refer to the non-existence
>> of stub functions, earlier today we had a review by Phil in this patch here where I was
>> adding stubs for all KVM functions:
>>
>> https://lore.kernel.org/qemu-riscv/f30d8589-8b59-2fd7-c38c-3f79508a4ac6@linaro.org/
>>
>> Phil mentioned that we don't need a function stub if the KVM only function is protected by
>> "kvm_enabled()". And this is fine, but then, from what you're saying, we can't rely on
>> (kvm_enabled() && foo) working all the time, so we're one conditional away from breaking
>> things it seems.
> 
> Please see:
> 
> https://lore.kernel.org/qemu-devel/20230911211317.28773-1-philmd@linaro.org/T/#t  (fix v4)
> https://lore.kernel.org/qemu-devel/ZP9Cmqgy2H3ypDf3@redhat.com/T/#t (fix v3)
> https://lore.kernel.org/qemu-devel/28c832bc-2fbf-8caa-e141-51288fc0d544@linaro.org/T/#t (fix v2)
> https://lore.kernel.org/qemu-devel/ZP74b%2FByEaVW5bZO@redhat.com/T/#t (fix v1)
> 
> and the original issue at
> https://lore.kernel.org/qemu-devel/8ee6684b-cdc3-78cb-9b76-e5875743bdcf@tls.msk.ru/T/#m65801e9edf31688a45722271a97e628ec98a0c23
> (this is an i386 pullreq which removed stub functions in presence of (!kvm_enabled() && foo)).
> 
> It is exactly the same issue as this one, with exactly the same fix, which resulted in
> breakage.  I dunno why your build succeeded, but the whole thing is.. not easy.

If you take a look at this patch:

[PATCH v2 10/19] target/riscv: remove kvm-stub.c

We're adding stubs for all KVM functions like ARM does in kvm_arm.h.

IIUC this is enough to avoid this scenario where a certain compiler/toolchain might
behave in a way we didn't anticipate and break builds.

I'll add more context in its commit msg mentioning this thread and why adding
stubs is safer than relying on the compiler cropping code via kvm_enabled().


Thanks,

Daniel


> 
> /mjt

On Tue, Sep 12, 2023 at 09:05:41AM +0300, Michael Tokarev wrote:
> 12.09.2023 00:43, Daniel Henrique Barboza:
> > On 9/11/23 16:54, Michael Tokarev wrote:
> ...
> > > >       /* 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,
> > > ...
> > > 
> > > As has been discovered earlier (see "target/i386: Restrict system-specific
> > > features from user emulation" threads), this is not enough, - compiler does
> > > not always eliminate if (0 && foo) { bar; } construct, it's too fragile and
> > > does not actually work.  Either some #ifdef'fery is needed here or some other,
> > > more explicit, way to eliminate such code, like introducing stub functions.
> > > 
> > > I know it's too late and this change is already in, but unfortunately that's
> > > when I noticed this.  While the "Fixes:" tag can't be changed anymore, a more
> > > proper fix for the actual problem (with the proper Fixes tag now) can still
> > > be applied on top of this fix.
> > 
> > This works fine on current master on my machine:
> > 
> > $ ../configure --cc=clang --target-list=riscv64-softmmu,riscv64-linux-user,riscv32-softmmu,riscv32-linux-user --enable-debug
> > $ make -j
> > 
> > So I'm not sure what do you mean by 'actual problem'. If you refer to the non-existence
> > of stub functions, earlier today we had a review by Phil in this patch here where I was
> > adding stubs for all KVM functions:
> > 
> > https://lore.kernel.org/qemu-riscv/f30d8589-8b59-2fd7-c38c-3f79508a4ac6@linaro.org/
> > 
> > Phil mentioned that we don't need a function stub if the KVM only function is protected by
> > "kvm_enabled()". And this is fine, but then, from what you're saying, we can't rely on
> > (kvm_enabled() && foo) working all the time, so we're one conditional away from breaking
> > things it seems.
> 
> Please see:
> 
> https://lore.kernel.org/qemu-devel/20230911211317.28773-1-philmd@linaro.org/T/#t  (fix v4)
> https://lore.kernel.org/qemu-devel/ZP9Cmqgy2H3ypDf3@redhat.com/T/#t (fix v3)
> https://lore.kernel.org/qemu-devel/28c832bc-2fbf-8caa-e141-51288fc0d544@linaro.org/T/#t (fix v2)
> https://lore.kernel.org/qemu-devel/ZP74b%2FByEaVW5bZO@redhat.com/T/#t (fix v1)
> 
> and the original issue at
> https://lore.kernel.org/qemu-devel/8ee6684b-cdc3-78cb-9b76-e5875743bdcf@tls.msk.ru/T/#m65801e9edf31688a45722271a97e628ec98a0c23
> (this is an i386 pullreq which removed stub functions in presence of (!kvm_enabled() && foo)).
> 
> It is exactly the same issue as this one, with exactly the same fix, which resulted in
> breakage.  I dunno why your build succeeded, but the whole thing is.. not easy.

virt_use_kvm_aia() gets compiled even without KVM enabled since it's in
the same file and not under a CONFIG_KVM or any guard. We're planning on
moving KVM functions to KVM-only files, which will only be compiled with
KVM enabled. We also wanted to remove stubs and just depend on
kvm_enabled() and the compiler, but now it looks like we got overly
excited about that. Considering clang, the stubs will need to stay.

Thanks,
drew