| Message ID | 20200805091640.11134-3-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Introduce kvm-steal-time | expand |
Hi Drew, On 8/5/20 11:16 AM, Andrew Jones wrote: > When we compile without KVM support !defined(CONFIG_KVM) we generate > stubs for functions that the linker will still encounter. Sometimes > these stubs can be executed safely and are placed in paths where they > get executed with or without KVM. Other functions should never be > called without KVM. Those functions should be guarded by kvm_enabled(), > but should also be robust to refactoring mistakes. Putting a > g_assert_not_reached() in the function should help. Additionally, > the g_assert_not_reached() calls may actually help the linker remove > some code. > > We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't > necessary at all - the only caller is in kvm.c > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 17 deletions(-) > > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h > index adb38514bf20..0da00eb6b20c 100644 > --- a/target/arm/kvm_arm.h > +++ b/target/arm/kvm_arm.h > @@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level); > > #else > > -static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) > -{ > - /* > - * This should never actually be called in the "not KVM" case, > - * but set up the fields to indicate an error anyway. > - */ > - cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE; > - cpu->host_cpu_probe_failed = true; > -} > - > +/* > + * It's safe to call these functions without KVM support. > + * They should either do nothing or return "not supported". > + */ > static inline void kvm_arm_add_vcpu_properties(Object *obj) {} this one also is guarded by kvm_enabled() in target/arm/cpu.c don't you want to add g_assert_not_reached()? > > static inline bool kvm_arm_aarch32_supported(void) > @@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void) > return false; > } > > +/* > + * These functions should never actually be called without KVM support. > + */ > +static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) > +{ > + g_assert_not_reached(); > +} > + > static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms) > { > - return -ENOENT; > + g_assert_not_reached(); > } > > static inline int kvm_arm_vgic_probe(void) > { > - return 0; > + g_assert_not_reached(); > } > > -static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {} > -static inline void kvm_arm_pmu_init(CPUState *cs) {} > +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > +{ > + g_assert_not_reached(); > +} > > -static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {} > +static inline void kvm_arm_pmu_init(CPUState *cs) > +{ > + g_assert_not_reached(); > +} > + > +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) > +{ > + g_assert_not_reached(); > +} > > -static inline void kvm_arm_get_virtual_time(CPUState *cs) {} > -static inline void kvm_arm_put_virtual_time(CPUState *cs) {} > #endif > > static inline const char *gic_class_name(void) > Besides, Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
On Wed, Aug 12, 2020 at 12:16:43PM +0200, Auger Eric wrote: > Hi Drew, > > On 8/5/20 11:16 AM, Andrew Jones wrote: > > When we compile without KVM support !defined(CONFIG_KVM) we generate > > stubs for functions that the linker will still encounter. Sometimes > > these stubs can be executed safely and are placed in paths where they > > get executed with or without KVM. Other functions should never be > > called without KVM. Those functions should be guarded by kvm_enabled(), > > but should also be robust to refactoring mistakes. Putting a > > g_assert_not_reached() in the function should help. Additionally, > > the g_assert_not_reached() calls may actually help the linker remove > > some code. > > > > We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't > > necessary at all - the only caller is in kvm.c > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++----------------- > > 1 file changed, 27 insertions(+), 17 deletions(-) > > > > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h > > index adb38514bf20..0da00eb6b20c 100644 > > --- a/target/arm/kvm_arm.h > > +++ b/target/arm/kvm_arm.h > > @@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level); > > > > #else > > > > -static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) > > -{ > > - /* > > - * This should never actually be called in the "not KVM" case, > > - * but set up the fields to indicate an error anyway. > > - */ > > - cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE; > > - cpu->host_cpu_probe_failed = true; > > -} > > - > > +/* > > + * It's safe to call these functions without KVM support. > > + * They should either do nothing or return "not supported". > > + */ > > static inline void kvm_arm_add_vcpu_properties(Object *obj) {} > this one also is guarded by kvm_enabled() in target/arm/cpu.c > don't you want to add g_assert_not_reached()? Yes, that would make sense. I'll do that for v3. > > > > static inline bool kvm_arm_aarch32_supported(void) > > @@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void) > > return false; > > } > > > > +/* > > + * These functions should never actually be called without KVM support. > > + */ > > +static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) > > +{ > > + g_assert_not_reached(); > > +} > > + > > static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms) > > { > > - return -ENOENT; > > + g_assert_not_reached(); > > } > > > > static inline int kvm_arm_vgic_probe(void) > > { > > - return 0; > > + g_assert_not_reached(); > > } > > > > -static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {} > > -static inline void kvm_arm_pmu_init(CPUState *cs) {} > > +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > > +{ > > + g_assert_not_reached(); > > +} > > > > -static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {} > > +static inline void kvm_arm_pmu_init(CPUState *cs) > > +{ > > + g_assert_not_reached(); > > +} > > + > > +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) > > +{ > > + g_assert_not_reached(); > > +} > > > > -static inline void kvm_arm_get_virtual_time(CPUState *cs) {} > > -static inline void kvm_arm_put_virtual_time(CPUState *cs) {} > > #endif > > > > static inline const char *gic_class_name(void) > > > > Besides, > Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks, drew
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h index adb38514bf20..0da00eb6b20c 100644 --- a/target/arm/kvm_arm.h +++ b/target/arm/kvm_arm.h @@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level); #else -static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) -{ - /* - * This should never actually be called in the "not KVM" case, - * but set up the fields to indicate an error anyway. - */ - cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE; - cpu->host_cpu_probe_failed = true; -} - +/* + * It's safe to call these functions without KVM support. + * They should either do nothing or return "not supported". + */ static inline void kvm_arm_add_vcpu_properties(Object *obj) {} static inline bool kvm_arm_aarch32_supported(void) @@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void) return false; } +/* + * These functions should never actually be called without KVM support. + */ +static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) +{ + g_assert_not_reached(); +} + static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms) { - return -ENOENT; + g_assert_not_reached(); } static inline int kvm_arm_vgic_probe(void) { - return 0; + g_assert_not_reached(); } -static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {} -static inline void kvm_arm_pmu_init(CPUState *cs) {} +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) +{ + g_assert_not_reached(); +} -static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {} +static inline void kvm_arm_pmu_init(CPUState *cs) +{ + g_assert_not_reached(); +} + +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) +{ + g_assert_not_reached(); +} -static inline void kvm_arm_get_virtual_time(CPUState *cs) {} -static inline void kvm_arm_put_virtual_time(CPUState *cs) {} #endif static inline const char *gic_class_name(void)
When we compile without KVM support !defined(CONFIG_KVM) we generate stubs for functions that the linker will still encounter. Sometimes these stubs can be executed safely and are placed in paths where they get executed with or without KVM. Other functions should never be called without KVM. Those functions should be guarded by kvm_enabled(), but should also be robust to refactoring mistakes. Putting a g_assert_not_reached() in the function should help. Additionally, the g_assert_not_reached() calls may actually help the linker remove some code. We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't necessary at all - the only caller is in kvm.c Signed-off-by: Andrew Jones <drjones@redhat.com> --- target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-)