Re: [PULL v2 38/45] hw/riscv/virt.c: fix non-KVM --enable-debug build

[Daniel Henrique Barboza]

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:
> >
> > f30d8589-8b59-2fd7-c38c-3f79508a4ac6@linaro.org/">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:
>
> 20230911211317.28773-1-philmd@linaro.org/T/#t">https://lore.kernel.org/qemu-devel/20230911211317.28773-1-philmd@linaro.org/T/#t
>   (fix v4)
> ZP9Cmqgy2H3ypDf3@redhat.com/T/#t">https://lore.kernel.org/qemu-devel/ZP9Cmqgy2H3ypDf3@redhat.com/T/#t (fix v3)
> 28c832bc-2fbf-8caa-e141-51288fc0d544@linaro.org/T/#t">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
> 8ee6684b-cdc3-78cb-9b76-e5875743bdcf@tls.msk.ru/T/#m65801e9edf31688a45722271a97e628ec98a0c23">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