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

[Andrew Jones]

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:
> > 
> > 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.

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