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-3f79508a4...@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-phi...@linaro.org/T/#t
> (fix v4)
> https://lore.kernel.org/qemu-devel/zp9cmqgy2h3yp...@redhat.com/T/#t (fix v3)
> https://lore.kernel.org/qemu-devel/28c832bc-2fbf-8caa-e141-51288fc0d...@linaro.org/T/#t
> (fix v2)
> https://lore.kernel.org/qemu-devel/zp74b%2fbyeavw5...@redhat.com/T/#t (fix v1)
>
> and the original issue at
> https://lore.kernel.org/qemu-devel/8ee6684b-cdc3-78cb-9b76-e5875743b...@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
