Re: [PATCH v11 4/6] target-arm: kvm64: detect guest RAS EXTENSION feature

[gengdongjiu]

Hi Peter,
  Sorry for my late response.

> 
> On 18 August 2017 at 15:23, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote:
> > check if kvm supports guest RAS EXTENSION. if so, set corresponding
> > feature bit for vcpu.
> >
> > Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
> > ---
> >  linux-headers/linux/kvm.h | 1 +
> >  target/arm/cpu.h          | 3 +++
> >  target/arm/kvm64.c        | 8 ++++++++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> > index 7971a4f..2aa176e 100644
> > --- a/linux-headers/linux/kvm.h
> > +++ b/linux-headers/linux/kvm.h
> > @@ -929,6 +929,7 @@ struct kvm_ppc_resize_hpt {  #define
> > KVM_CAP_PPC_SMT_POSSIBLE 147  #define KVM_CAP_HYPERV_SYNIC2 148
> > #define KVM_CAP_HYPERV_VP_INDEX 149
> > +#define KVM_CAP_ARM_RAS_EXTENSION 150
> >
> >  #ifdef KVM_CAP_IRQ_ROUTING
> >
> 
> Hi. Changes to linux-headers need to be done as a patch of their own created using scripts/update-linux-headers.sh run against a mainline
> kernel tree (and with a commit message that quotes the kernel commit hash used). This ensures that we have a consistent set of headers
> that don't diverge from the kernel copy.
Sure, I will, thanks a lot for your reminder.

> 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h index
> > b39d64a..6b0961b 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -611,6 +611,8 @@ struct ARMCPU {
> >
> >      /* CPU has memory protection unit */
> >      bool has_mpu;
> > +    /* CPU has ras extension unit */
> > +    bool has_ras_extension;
> >      /* PMSAv7 MPU number of supported regions */
> >      uint32_t pmsav7_dregion;
> >
> > @@ -1229,6 +1231,7 @@ enum arm_features {
> >      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
> >      ARM_FEATURE_PMU, /* has PMU support */
> >      ARM_FEATURE_VBAR, /* has cp15 VBAR */
> > +    ARM_FEATURE_RAS_EXTENSION, /*has RAS extension support */
> 
> Missing space after '/*' ?
Yes, thanks for the pointing out.

> 
> >  };
> >
> >  static inline int arm_feature(CPUARMState *env, int feature) diff
> > --git a/target/arm/kvm64.c b/target/arm/kvm64.c index a16abc8..0781367
> > 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -518,6 +518,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          unset_feature(&env->features, ARM_FEATURE_PMU);
> >      }
> >
> > +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_RAS_EXTENSION)) {
> > +        cpu->has_ras_extension = true;
> > +        set_feature(&env->features, ARM_FEATURE_RAS_EXTENSION);
> > +    } else {
> > +        cpu->has_ras_extension = false;
> > +        unset_feature(&env->features, ARM_FEATURE_RAS_EXTENSION);
> > +    }
> > +
> 
> Shouldn't we need to also tell the kernel that we actually want it to expose RAS to the guest? Compare the PMU code in this function,
> where we set a kvm_init_features bit to do this.
> (This suggests that your ABI for the kernel part of this feature may not be correct?)

In the PMU code, it indeed sets a kvm_init_features bit. Here ARM James has a concern that we are depend on the host CPU RAS extension,
He means that if userspace receives the SIGBUS delivered by host memory_failure(), user space should record the CPER for guest
and handling the error regardless whether host CPU supports RAS extension. But I think if user space receives the SIGBUS signal, that means
host CPU RAS module detects the error or CPU consumes the poison data, thus we should check whether physical CPU support RAS extension. 

> 
> You should also not be calling set_feature() here -- if the CPU features bit doesn't say "this CPU should have the RAS extensions" we
> shouldn't create a CPU with them. Instead you should set it in kvm_arm_get_host_cpu_features() (again, compare the PMU code).

Understand, I will loop you to another mail thread to consult with you that whether userspace should detect CPU RAS extension.
If all agree to detect CPU RAS feature, I will fix the issue that you pointing out.

> 
> thanks
> -- PMM