This file is not needed for some time now. All the stubs implemented in
it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if
kvm_enabled()' blocks that the compiler will rip it out in non-KVM
builds.
We'll also add non-KVM stubs for all functions declared in kvm_riscv.h.
All stubs are implemented as g_asserted_not_reached(), meaning that we
won't support them in non-KVM builds. This is done by other kvm headers
like kvm_arm.h and kvm_ppc.h.
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/kvm-stub.c | 30 ------------------------------
target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++
target/riscv/meson.build | 2 +-
3 files changed, 32 insertions(+), 31 deletions(-)
delete mode 100644 target/riscv/kvm-stub.c
diff --git a/target/riscv/kvm-stub.c b/target/riscv/kvm-stub.c
deleted file mode 100644
index 4e8fc31a21..0000000000
--- a/target/riscv/kvm-stub.c
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * QEMU KVM RISC-V specific function stubs
- *
- * Copyright (c) 2020 Huawei Technologies Co., Ltd
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2 or later, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program. If not, see <http://www.gnu.org/licenses/>.
- */
-#include "qemu/osdep.h"
-#include "cpu.h"
-#include "kvm_riscv.h"
-
-void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
-{
- abort();
-}
-
-void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
-{
- abort();
-}
diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
index f6501e68e2..c9ecd9a967 100644
--- a/target/riscv/kvm_riscv.h
+++ b/target/riscv/kvm_riscv.h
@@ -19,6 +19,7 @@
#ifndef QEMU_KVM_RISCV_H
#define QEMU_KVM_RISCV_H
+#ifdef CONFIG_KVM
void kvm_riscv_cpu_add_kvm_properties(Object *obj);
void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
@@ -27,5 +28,35 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
uint64_t aplic_base, uint64_t imsic_base,
uint64_t guest_num);
void riscv_kvm_aplic_request(void *opaque, int irq, int level);
+#else
+static inline void kvm_riscv_cpu_add_kvm_properties(Object *obj)
+{
+ g_assert_not_reached();
+}
+
+static inline void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
+{
+ g_assert_not_reached();
+}
+
+static inline void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
+{
+ g_assert_not_reached();
+}
+
+static inline void kvm_riscv_aia_create(MachineState *machine,
+ uint64_t group_shift, uint64_t aia_irq_num,
+ uint64_t aia_msi_num, uint64_t aplic_base,
+ uint64_t imsic_base, uint64_t guest_num)
+{
+ g_assert_not_reached();
+}
+
+static inline void riscv_kvm_aplic_request(void *opaque, int irq, int level)
+{
+ g_assert_not_reached();
+}
+
+#endif /* CONFIG_KVM */
#endif
diff --git a/target/riscv/meson.build b/target/riscv/meson.build
index f0486183fa..3323b78b84 100644
--- a/target/riscv/meson.build
+++ b/target/riscv/meson.build
@@ -24,7 +24,7 @@ riscv_ss.add(files(
'zce_helper.c',
'vcrypto_helper.c'
))
-riscv_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'), if_false: files('kvm-stub.c'))
+riscv_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'))
riscv_system_ss = ss.source_set()
riscv_system_ss.add(files(
--
2.41.0On 6/9/23 11:16, Daniel Henrique Barboza wrote:
> This file is not needed for some time now. All the stubs implemented in
> it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if
> kvm_enabled()' blocks that the compiler will rip it out in non-KVM
> builds.
>
> We'll also add non-KVM stubs for all functions declared in kvm_riscv.h.
> All stubs are implemented as g_asserted_not_reached(), meaning that we
> won't support them in non-KVM builds. This is done by other kvm headers
> like kvm_arm.h and kvm_ppc.h.
Aren't them also protected by kvm_enabled()? Otherwise shouldn't they?
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/kvm-stub.c | 30 ------------------------------
> target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++
> target/riscv/meson.build | 2 +-
> 3 files changed, 32 insertions(+), 31 deletions(-)
> delete mode 100644 target/riscv/kvm-stub.c
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index f6501e68e2..c9ecd9a967 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -19,6 +19,7 @@
> #ifndef QEMU_KVM_RISCV_H
> #define QEMU_KVM_RISCV_H
>
> +#ifdef CONFIG_KVM
> void kvm_riscv_cpu_add_kvm_properties(Object *obj);
At a glance kvm_riscv_cpu_add_kvm_properties() is.
Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the
compiler to elide it.
> void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
> void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
> @@ -27,5 +28,35 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
> uint64_t aplic_base, uint64_t imsic_base,
> uint64_t guest_num);
> void riscv_kvm_aplic_request(void *opaque, int irq, int level);
> +#else
> +static inline void kvm_riscv_cpu_add_kvm_properties(Object *obj)
> +{
> + g_assert_not_reached();
> +}
> +
> +static inline void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
> +{
> + g_assert_not_reached();
> +}
> +
> +static inline void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> +{
> + g_assert_not_reached();
> +}
> +
> +static inline void kvm_riscv_aia_create(MachineState *machine,
> + uint64_t group_shift, uint64_t aia_irq_num,
> + uint64_t aia_msi_num, uint64_t aplic_base,
> + uint64_t imsic_base, uint64_t guest_num)
> +{
> + g_assert_not_reached();
> +}
> +
> +static inline void riscv_kvm_aplic_request(void *opaque, int irq, int level)
> +{
> + g_assert_not_reached();
> +}
> +
> +#endif /* CONFIG_KVM */
On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: > On 6/9/23 11:16, Daniel Henrique Barboza wrote: > > This file is not needed for some time now. All the stubs implemented in > > it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if > > kvm_enabled()' blocks that the compiler will rip it out in non-KVM > > builds. > > > > We'll also add non-KVM stubs for all functions declared in kvm_riscv.h. > > All stubs are implemented as g_asserted_not_reached(), meaning that we > > won't support them in non-KVM builds. This is done by other kvm headers > > like kvm_arm.h and kvm_ppc.h. > > Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? Yes, I think your earlier suggestion that we always invoke kvm functions from non-kvm files with a kvm_enabled() guard makes sense. > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > --- > > target/riscv/kvm-stub.c | 30 ------------------------------ > > target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ > > target/riscv/meson.build | 2 +- > > 3 files changed, 32 insertions(+), 31 deletions(-) > > delete mode 100644 target/riscv/kvm-stub.c > > > > diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h > > index f6501e68e2..c9ecd9a967 100644 > > --- a/target/riscv/kvm_riscv.h > > +++ b/target/riscv/kvm_riscv.h > > @@ -19,6 +19,7 @@ > > #ifndef QEMU_KVM_RISCV_H > > #define QEMU_KVM_RISCV_H > > +#ifdef CONFIG_KVM > > void kvm_riscv_cpu_add_kvm_properties(Object *obj); > > At a glance kvm_riscv_cpu_add_kvm_properties() is. > Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the > compiler to elide it. Yes, when building without CONFIG_KVM enabled it's actually better to not have the stubs, since the compiler will catch an unguarded kvm function call (assuming the kvm function is defined in a file which is only built with CONFIG_KVM). Unfortunately we don't have anything to protect developers from forgetting the kvm_enabled() guard when building a QEMU which supports both TCG and KVM. We could try to remember to put 'assert(kvm_enabled())' at the start of each of these types of functions. It looks like mips does that for a couple functions. Thanks, drew
On Mon, Sep 11, 2023 at 09:49:06AM +0200, Andrew Jones wrote: > On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: > > On 6/9/23 11:16, Daniel Henrique Barboza wrote: > > > This file is not needed for some time now. All the stubs implemented in > > > it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if > > > kvm_enabled()' blocks that the compiler will rip it out in non-KVM > > > builds. > > > > > > We'll also add non-KVM stubs for all functions declared in kvm_riscv.h. > > > All stubs are implemented as g_asserted_not_reached(), meaning that we > > > won't support them in non-KVM builds. This is done by other kvm headers > > > like kvm_arm.h and kvm_ppc.h. > > > > Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? > > Yes, I think your earlier suggestion that we always invoke kvm functions > from non-kvm files with a kvm_enabled() guard makes sense. > > > > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > > --- > > > target/riscv/kvm-stub.c | 30 ------------------------------ > > > target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ > > > target/riscv/meson.build | 2 +- > > > 3 files changed, 32 insertions(+), 31 deletions(-) > > > delete mode 100644 target/riscv/kvm-stub.c > > > > > > > diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h > > > index f6501e68e2..c9ecd9a967 100644 > > > --- a/target/riscv/kvm_riscv.h > > > +++ b/target/riscv/kvm_riscv.h > > > @@ -19,6 +19,7 @@ > > > #ifndef QEMU_KVM_RISCV_H > > > #define QEMU_KVM_RISCV_H > > > +#ifdef CONFIG_KVM > > > void kvm_riscv_cpu_add_kvm_properties(Object *obj); > > > > At a glance kvm_riscv_cpu_add_kvm_properties() is. > > Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the > > compiler to elide it. > > Yes, when building without CONFIG_KVM enabled it's actually better to not > have the stubs, since the compiler will catch an unguarded kvm function > call (assuming the kvm function is defined in a file which is only built > with CONFIG_KVM). > > Unfortunately we don't have anything to protect developers from forgetting > the kvm_enabled() guard when building a QEMU which supports both TCG and > KVM. We could try to remember to put 'assert(kvm_enabled())' at the start > of each of these types of functions. It looks like mips does that for a > couple functions. Eh, ignore this suggestion. We don't need asserts, because we have CI. As long as our CI does a CONFIG_KVM=n build and all KVM functions are in kvm- only files, then we'll always catch calls of KVM functions which are missing their kvm_enabled() guards. Thanks, drew
On 9/11/23 06:04, Andrew Jones wrote: > On Mon, Sep 11, 2023 at 09:49:06AM +0200, Andrew Jones wrote: >> On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: >>> On 6/9/23 11:16, Daniel Henrique Barboza wrote: >>>> This file is not needed for some time now. All the stubs implemented in >>>> it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if >>>> kvm_enabled()' blocks that the compiler will rip it out in non-KVM >>>> builds. >>>> >>>> We'll also add non-KVM stubs for all functions declared in kvm_riscv.h. >>>> All stubs are implemented as g_asserted_not_reached(), meaning that we >>>> won't support them in non-KVM builds. This is done by other kvm headers >>>> like kvm_arm.h and kvm_ppc.h. >>> >>> Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? >> >> Yes, I think your earlier suggestion that we always invoke kvm functions >> from non-kvm files with a kvm_enabled() guard makes sense. >> >>> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>> --- >>>> target/riscv/kvm-stub.c | 30 ------------------------------ >>>> target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ >>>> target/riscv/meson.build | 2 +- >>>> 3 files changed, 32 insertions(+), 31 deletions(-) >>>> delete mode 100644 target/riscv/kvm-stub.c >>> >>> >>>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h >>>> index f6501e68e2..c9ecd9a967 100644 >>>> --- a/target/riscv/kvm_riscv.h >>>> +++ b/target/riscv/kvm_riscv.h >>>> @@ -19,6 +19,7 @@ >>>> #ifndef QEMU_KVM_RISCV_H >>>> #define QEMU_KVM_RISCV_H >>>> +#ifdef CONFIG_KVM >>>> void kvm_riscv_cpu_add_kvm_properties(Object *obj); >>> >>> At a glance kvm_riscv_cpu_add_kvm_properties() is. >>> Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the >>> compiler to elide it. >> >> Yes, when building without CONFIG_KVM enabled it's actually better to not >> have the stubs, since the compiler will catch an unguarded kvm function >> call (assuming the kvm function is defined in a file which is only built >> with CONFIG_KVM). >> >> Unfortunately we don't have anything to protect developers from forgetting >> the kvm_enabled() guard when building a QEMU which supports both TCG and >> KVM. We could try to remember to put 'assert(kvm_enabled())' at the start >> of each of these types of functions. It looks like mips does that for a >> couple functions. > > Eh, ignore this suggestion. We don't need asserts, because we have CI. As > long as our CI does a CONFIG_KVM=n build and all KVM functions are in kvm- > only files, then we'll always catch calls of KVM functions which are > missing their kvm_enabled() guards. So, to sum up, get rid of the 'g_assert_not_reached()' stubs since they should be cropped by well placed kvm_enabled() calls in the code and, if that's not the case, a non-KVM build failure will let us know that there's something wrong. I can live with that. It's less code to deal with in the header as well. Thanks, Daniel > > Thanks, > drew
On 9/11/23 09:23, Daniel Henrique Barboza wrote: > > > On 9/11/23 06:04, Andrew Jones wrote: >> On Mon, Sep 11, 2023 at 09:49:06AM +0200, Andrew Jones wrote: >>> On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: >>>> On 6/9/23 11:16, Daniel Henrique Barboza wrote: >>>>> This file is not needed for some time now. All the stubs implemented in >>>>> it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if >>>>> kvm_enabled()' blocks that the compiler will rip it out in non-KVM >>>>> builds. >>>>> >>>>> We'll also add non-KVM stubs for all functions declared in kvm_riscv.h. >>>>> All stubs are implemented as g_asserted_not_reached(), meaning that we >>>>> won't support them in non-KVM builds. This is done by other kvm headers >>>>> like kvm_arm.h and kvm_ppc.h. >>>> >>>> Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? >>> >>> Yes, I think your earlier suggestion that we always invoke kvm functions >>> from non-kvm files with a kvm_enabled() guard makes sense. >>> >>>> >>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>> --- >>>>> target/riscv/kvm-stub.c | 30 ------------------------------ >>>>> target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ >>>>> target/riscv/meson.build | 2 +- >>>>> 3 files changed, 32 insertions(+), 31 deletions(-) >>>>> delete mode 100644 target/riscv/kvm-stub.c >>>> >>>> >>>>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h >>>>> index f6501e68e2..c9ecd9a967 100644 >>>>> --- a/target/riscv/kvm_riscv.h >>>>> +++ b/target/riscv/kvm_riscv.h >>>>> @@ -19,6 +19,7 @@ >>>>> #ifndef QEMU_KVM_RISCV_H >>>>> #define QEMU_KVM_RISCV_H >>>>> +#ifdef CONFIG_KVM >>>>> void kvm_riscv_cpu_add_kvm_properties(Object *obj); >>>> >>>> At a glance kvm_riscv_cpu_add_kvm_properties() is. >>>> Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the >>>> compiler to elide it. >>> >>> Yes, when building without CONFIG_KVM enabled it's actually better to not >>> have the stubs, since the compiler will catch an unguarded kvm function >>> call (assuming the kvm function is defined in a file which is only built >>> with CONFIG_KVM). >>> >>> Unfortunately we don't have anything to protect developers from forgetting >>> the kvm_enabled() guard when building a QEMU which supports both TCG and >>> KVM. We could try to remember to put 'assert(kvm_enabled())' at the start >>> of each of these types of functions. It looks like mips does that for a >>> couple functions. >> >> Eh, ignore this suggestion. We don't need asserts, because we have CI. As >> long as our CI does a CONFIG_KVM=n build and all KVM functions are in kvm- >> only files, then we'll always catch calls of KVM functions which are >> missing their kvm_enabled() guards. > > So, to sum up, get rid of the 'g_assert_not_reached()' stubs since they should > be cropped by well placed kvm_enabled() calls in the code and, if that's not > the case, a non-KVM build failure will let us know that there's something wrong. > > I can live with that. It's less code to deal with in the header as well. Thanks, I changed my mind after talking with Michael in this thread: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02567.html There are differences in how gcc and clang (and probably other toolchains) behave when choosing to crop code gated with 'if (kvm_enabled && something)'. It is better to have stubs for all KVM functions to avoid relying on compiler behavior. We should keep the stubs in this patch as is. Thanks, Daniel > > > Daniel > > >> >> Thanks, >> drew
On 12/9/23 12:48, Daniel Henrique Barboza wrote: > On 9/11/23 09:23, Daniel Henrique Barboza wrote: >> On 9/11/23 06:04, Andrew Jones wrote: >>> On Mon, Sep 11, 2023 at 09:49:06AM +0200, Andrew Jones wrote: >>>> On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: >>>>> On 6/9/23 11:16, Daniel Henrique Barboza wrote: >>>>>> This file is not needed for some time now. All the stubs >>>>>> implemented in >>>>>> it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in >>>>>> 'if >>>>>> kvm_enabled()' blocks that the compiler will rip it out in non-KVM >>>>>> builds. >>>>>> >>>>>> We'll also add non-KVM stubs for all functions declared in >>>>>> kvm_riscv.h. >>>>>> All stubs are implemented as g_asserted_not_reached(), meaning >>>>>> that we >>>>>> won't support them in non-KVM builds. This is done by other kvm >>>>>> headers >>>>>> like kvm_arm.h and kvm_ppc.h. >>>>> >>>>> Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? >>>> >>>> Yes, I think your earlier suggestion that we always invoke kvm >>>> functions >>>> from non-kvm files with a kvm_enabled() guard makes sense. >>>> >>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>>> --- >>>>>> target/riscv/kvm-stub.c | 30 ------------------------------ >>>>>> target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ >>>>>> target/riscv/meson.build | 2 +- >>>>>> 3 files changed, 32 insertions(+), 31 deletions(-) >>>>>> delete mode 100644 target/riscv/kvm-stub.c >>>>> >>>>> >>>>>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h >>>>>> index f6501e68e2..c9ecd9a967 100644 >>>>>> --- a/target/riscv/kvm_riscv.h >>>>>> +++ b/target/riscv/kvm_riscv.h >>>>>> @@ -19,6 +19,7 @@ >>>>>> #ifndef QEMU_KVM_RISCV_H >>>>>> #define QEMU_KVM_RISCV_H >>>>>> +#ifdef CONFIG_KVM >>>>>> void kvm_riscv_cpu_add_kvm_properties(Object *obj); >>>>> >>>>> At a glance kvm_riscv_cpu_add_kvm_properties() is. >>>>> Keep the prototype declared (before #ifdef CONFIG_KVM) is enough >>>>> for the >>>>> compiler to elide it. >>>> >>>> Yes, when building without CONFIG_KVM enabled it's actually better >>>> to not >>>> have the stubs, since the compiler will catch an unguarded kvm function >>>> call (assuming the kvm function is defined in a file which is only >>>> built >>>> with CONFIG_KVM). >>>> >>>> Unfortunately we don't have anything to protect developers from >>>> forgetting >>>> the kvm_enabled() guard when building a QEMU which supports both TCG >>>> and >>>> KVM. We could try to remember to put 'assert(kvm_enabled())' at the >>>> start >>>> of each of these types of functions. It looks like mips does that for a >>>> couple functions. >>> >>> Eh, ignore this suggestion. We don't need asserts, because we have >>> CI. As >>> long as our CI does a CONFIG_KVM=n build and all KVM functions are in >>> kvm- >>> only files, then we'll always catch calls of KVM functions which are >>> missing their kvm_enabled() guards. >> >> So, to sum up, get rid of the 'g_assert_not_reached()' stubs since >> they should >> be cropped by well placed kvm_enabled() calls in the code and, if >> that's not >> the case, a non-KVM build failure will let us know that there's >> something wrong. >> >> I can live with that. It's less code to deal with in the header as >> well. Thanks, > > > I changed my mind after talking with Michael in this thread: > > https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02567.html > > > There are differences in how gcc and clang (and probably other > toolchains) behave > when choosing to crop code gated with 'if (kvm_enabled && something)'. > It is > better to have stubs for all KVM functions to avoid relying on compiler > behavior. > > We should keep the stubs in this patch as is. Thanks, Don't get stuck at this; get your series merged, we'll have a look at that later, this is not important. Regards, Phil.
On 9/12/23 08:15, Philippe Mathieu-Daudé wrote: > On 12/9/23 12:48, Daniel Henrique Barboza wrote: >> On 9/11/23 09:23, Daniel Henrique Barboza wrote: >>> On 9/11/23 06:04, Andrew Jones wrote: >>>> On Mon, Sep 11, 2023 at 09:49:06AM +0200, Andrew Jones wrote: >>>>> On Wed, Sep 06, 2023 at 12:23:19PM +0200, Philippe Mathieu-Daudé wrote: >>>>>> On 6/9/23 11:16, Daniel Henrique Barboza wrote: >>>>>>> This file is not needed for some time now. All the stubs implemented in >>>>>>> it (kvm_riscv_reset_vcpu() and kvm_riscv_set_irq()) are wrapped in 'if >>>>>>> kvm_enabled()' blocks that the compiler will rip it out in non-KVM >>>>>>> builds. >>>>>>> >>>>>>> We'll also add non-KVM stubs for all functions declared in kvm_riscv.h. >>>>>>> All stubs are implemented as g_asserted_not_reached(), meaning that we >>>>>>> won't support them in non-KVM builds. This is done by other kvm headers >>>>>>> like kvm_arm.h and kvm_ppc.h. >>>>>> >>>>>> Aren't them also protected by kvm_enabled()? Otherwise shouldn't they? >>>>> >>>>> Yes, I think your earlier suggestion that we always invoke kvm functions >>>>> from non-kvm files with a kvm_enabled() guard makes sense. >>>>> >>>>>> >>>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>>>> --- >>>>>>> target/riscv/kvm-stub.c | 30 ------------------------------ >>>>>>> target/riscv/kvm_riscv.h | 31 +++++++++++++++++++++++++++++++ >>>>>>> target/riscv/meson.build | 2 +- >>>>>>> 3 files changed, 32 insertions(+), 31 deletions(-) >>>>>>> delete mode 100644 target/riscv/kvm-stub.c >>>>>> >>>>>> >>>>>>> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h >>>>>>> index f6501e68e2..c9ecd9a967 100644 >>>>>>> --- a/target/riscv/kvm_riscv.h >>>>>>> +++ b/target/riscv/kvm_riscv.h >>>>>>> @@ -19,6 +19,7 @@ >>>>>>> #ifndef QEMU_KVM_RISCV_H >>>>>>> #define QEMU_KVM_RISCV_H >>>>>>> +#ifdef CONFIG_KVM >>>>>>> void kvm_riscv_cpu_add_kvm_properties(Object *obj); >>>>>> >>>>>> At a glance kvm_riscv_cpu_add_kvm_properties() is. >>>>>> Keep the prototype declared (before #ifdef CONFIG_KVM) is enough for the >>>>>> compiler to elide it. >>>>> >>>>> Yes, when building without CONFIG_KVM enabled it's actually better to not >>>>> have the stubs, since the compiler will catch an unguarded kvm function >>>>> call (assuming the kvm function is defined in a file which is only built >>>>> with CONFIG_KVM). >>>>> >>>>> Unfortunately we don't have anything to protect developers from forgetting >>>>> the kvm_enabled() guard when building a QEMU which supports both TCG and >>>>> KVM. We could try to remember to put 'assert(kvm_enabled())' at the start >>>>> of each of these types of functions. It looks like mips does that for a >>>>> couple functions. >>>> >>>> Eh, ignore this suggestion. We don't need asserts, because we have CI. As >>>> long as our CI does a CONFIG_KVM=n build and all KVM functions are in kvm- >>>> only files, then we'll always catch calls of KVM functions which are >>>> missing their kvm_enabled() guards. >>> >>> So, to sum up, get rid of the 'g_assert_not_reached()' stubs since they should >>> be cropped by well placed kvm_enabled() calls in the code and, if that's not >>> the case, a non-KVM build failure will let us know that there's something wrong. >>> >>> I can live with that. It's less code to deal with in the header as well. Thanks, >> >> >> I changed my mind after talking with Michael in this thread: >> >> https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg02567.html >> >> >> There are differences in how gcc and clang (and probably other toolchains) behave >> when choosing to crop code gated with 'if (kvm_enabled && something)'. It is >> better to have stubs for all KVM functions to avoid relying on compiler behavior. >> >> We should keep the stubs in this patch as is. Thanks, > > Don't get stuck at this; get your series merged, we'll have a look > at that later, this is not important. Ok, so we'll simplify here and avoid the unneeded stubs for now as we initially discussed, while making sure we're not breaking builds with --enable-debug and clang. After this is merged we can reevaluate the best approach to take. Thanks, Daniel > > Regards, > > Phil.
© 2016 - 2024 Red Hat, Inc.