Re: [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable

[Andrew Jones]

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