From: Andrew Jones <ajones@ventanamicro.com>
To: Michael Tokarev <mjt@tls.msk.ru>
Cc: "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>,
"Alistair Francis" <alistair23@gmail.com>,
qemu-devel@nongnu.org,
"Richard Henderson" <richard.henderson@linaro.org>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Alistair Francis" <alistair.francis@wdc.com>
Subject: Re: [PULL v2 38/45] hw/riscv/virt.c: fix non-KVM --enable-debug build
Date: Tue, 12 Sep 2023 11:54:47 +0200 [thread overview]
Message-ID: <20230912-55e50d06c926f1720048eba9@orel> (raw)
In-Reply-To: <48e7e9bb-1fb5-5233-bdef-c958498b90c4@tls.msk.ru>
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
next prev parent reply other threads:[~2023-09-12 9:55 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-11 6:42 [PULL v2 00/45] riscv-to-apply queue Alistair Francis
2023-09-11 6:42 ` [PULL v2 01/45] target/riscv/cpu.c: do not run 'host' CPU with TCG Alistair Francis
2023-09-11 6:42 ` [PULL v2 02/45] hw/char/riscv_htif: Fix printing of console characters on big endian hosts Alistair Francis
2023-09-11 6:42 ` [PULL v2 03/45] hw/char/riscv_htif: Fix the console syscall " Alistair Francis
2023-09-11 6:42 ` [PULL v2 04/45] target/riscv/cpu.c: add zmmul isa string Alistair Francis
2023-09-11 6:42 ` [PULL v2 05/45] target/riscv/cpu.c: add smepmp " Alistair Francis
2023-09-11 6:42 ` [PULL v2 06/45] target/riscv: Fix page_check_range use in fault-only-first Alistair Francis
2023-09-11 6:42 ` [PULL v2 07/45] target/riscv: Use existing lookup tables for MixColumns Alistair Francis
2023-09-11 6:42 ` [PULL v2 08/45] target/riscv: Refactor some of the generic vector functionality Alistair Francis
2023-09-11 6:42 ` [PULL v2 09/45] target/riscv: Refactor vector-vector translation macro Alistair Francis
2023-09-11 6:42 ` [PULL v2 10/45] target/riscv: Remove redundant "cpu_vl == 0" checks Alistair Francis
2023-09-11 6:42 ` [PULL v2 11/45] target/riscv: Add Zvbc ISA extension support Alistair Francis
2023-09-11 6:42 ` [PULL v2 12/45] target/riscv: Move vector translation checks Alistair Francis
2023-09-11 6:42 ` [PULL v2 13/45] target/riscv: Refactor translation of vector-widening instruction Alistair Francis
2023-09-11 6:42 ` [PULL v2 14/45] target/riscv: Refactor some of the generic vector functionality Alistair Francis
2023-09-11 6:42 ` [PULL v2 15/45] target/riscv: Add Zvbb ISA extension support Alistair Francis
2023-09-11 6:42 ` [PULL v2 16/45] target/riscv: Add Zvkned " Alistair Francis
2023-09-11 6:42 ` [PULL v2 17/45] target/riscv: Add Zvknh " Alistair Francis
2023-09-11 6:42 ` [PULL v2 18/45] target/riscv: Add Zvksh " Alistair Francis
2023-09-11 6:42 ` [PULL v2 19/45] target/riscv: Add Zvkg " Alistair Francis
2023-09-11 6:42 ` [PULL v2 20/45] crypto: Create sm4_subword Alistair Francis
2023-09-11 6:42 ` [PULL v2 21/45] crypto: Add SM4 constant parameter CK Alistair Francis
2023-09-11 6:42 ` [PULL v2 22/45] target/riscv: Add Zvksed ISA extension support Alistair Francis
2023-09-11 6:42 ` [PULL v2 23/45] target/riscv: Implement WARL behaviour for mcountinhibit/mcounteren Alistair Francis
2023-09-11 6:42 ` [PULL v2 24/45] target/riscv: Add Zihintntl extension ISA string to DTS Alistair Francis
2023-09-11 6:43 ` [PULL v2 25/45] target/riscv: Fix zfa fleq.d and fltq.d Alistair Francis
2023-09-11 6:43 ` [PULL v2 26/45] hw/intc: Fix upper/lower mtime write calculation Alistair Francis
2023-09-11 6:43 ` [PULL v2 27/45] hw/intc: Make rtc variable names consistent Alistair Francis
2023-09-11 6:43 ` [PULL v2 28/45] linux-user/riscv: Use abi type for target_ucontext Alistair Francis
2023-09-11 6:43 ` [PULL v2 29/45] target/riscv: support the AIA device emulation with KVM enabled Alistair Francis
2023-09-11 6:43 ` [PULL v2 30/45] target/riscv: check the in-kernel irqchip support Alistair Francis
2023-09-11 6:43 ` [PULL v2 31/45] target/riscv: Create an KVM AIA irqchip Alistair Francis
2023-09-11 6:43 ` [PULL v2 32/45] target/riscv: update APLIC and IMSIC to support KVM AIA Alistair Francis
2023-09-11 6:43 ` [PULL v2 33/45] target/riscv: select KVM AIA in riscv virt machine Alistair Francis
2023-09-11 6:43 ` [PULL v2 34/45] hw/riscv: virt: Fix riscv,pmu DT node path Alistair Francis
2023-09-11 6:43 ` [PULL v2 35/45] target/riscv: Update CSR bits name for svadu extension Alistair Francis
2023-09-11 6:43 ` [PULL v2 36/45] target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0 Alistair Francis
2023-09-11 6:43 ` [PULL v2 37/45] riscv: zicond: make non-experimental Alistair Francis
2023-09-11 6:43 ` [PULL v2 38/45] hw/riscv/virt.c: fix non-KVM --enable-debug build Alistair Francis
2023-09-11 19:54 ` Michael Tokarev
2023-09-11 21:43 ` Daniel Henrique Barboza
2023-09-12 6:05 ` Michael Tokarev
2023-09-12 9:54 ` Andrew Jones [this message]
2023-09-12 10:43 ` Daniel Henrique Barboza
2023-09-11 6:43 ` [PULL v2 39/45] hw/intc/riscv_aplic.c " Alistair Francis
2023-09-11 6:43 ` [PULL v2 40/45] linux-user/riscv: Add new extensions to hwprobe Alistair Francis
2023-09-11 6:43 ` [PULL v2 41/45] target/riscv: Use accelerated helper for AES64KS1I Alistair Francis
2023-09-11 6:43 ` [PULL v2 42/45] target/riscv: Allocate itrigger timers only once Alistair Francis
2023-09-11 20:05 ` Michael Tokarev
2023-09-11 6:43 ` [PULL v2 43/45] target/riscv/pmp.c: respect mseccfg.RLB for pmpaddrX changes Alistair Francis
2023-09-11 6:43 ` [PULL v2 44/45] target/riscv: Align the AIA model to v1.0 ratified spec Alistair Francis
2023-09-11 6:43 ` [PULL v2 45/45] target/riscv: don't read CSR in riscv_csrrw_do64 Alistair Francis
2023-09-11 15:19 ` [PULL v2 00/45] riscv-to-apply queue Stefan Hajnoczi
2023-09-12 10:26 ` Michael Tokarev
2023-09-14 3:08 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230912-55e50d06c926f1720048eba9@orel \
--to=ajones@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=dbarboza@ventanamicro.com \
--cc=mjt@tls.msk.ru \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).