Re: [Xen-devel] [PATCH 16/19] x86/vmce: enable injecting LMCE to guest on Intel host

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 02/22/17 08:48 -0700, Jan Beulich wrote:
> >>> On 17.02.17 at 07:39, <haozhong.zhang@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/cpu/mcheck/mcaction.c
> > +++ b/xen/arch/x86/cpu/mcheck/mcaction.c
> > @@ -88,17 +88,19 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
> >                      goto vmce_failed;
> >                  }
> >  
> > -                if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> > +                vmce_vcpuid = global->mc_vcpuid;
> > +                if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> > +                     (vmce_vcpuid == -1 ||
> 
> What is this -1 matching up with? Or to say it differently, this needs a
> #define used at both producing and consuming sides.

It comes from mca_init_global

> 
> > +                      global->mc_domid != bank->mc_domid ||
> 
> I don't understand this check.

It serves for MCE injection test (i.e. xen-mceinj). The check is
always false for LMCE that comes from the real hardware error. But for
xen-mceinj, which may specify both the injected domain and the
injected physical CPU, it's hard for xen-mceinj to ensure, when the
injection really happens, the injected CPU is still the one on which a
vCPU of the injected domain was running. So I add this check to avoid
injecting to the wrong domain.

> 
> > --- a/xen/arch/x86/cpu/mcheck/vmce.c
> > +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> > @@ -444,14 +444,21 @@ static int vcpu_fill_mc_msrs(struct vcpu *v, uint64_t 
> > mcg_status,
> >  }
> >  
> >  int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
> > -                   uint64_t gstatus, bool broadcast)
> > +                   uint64_t gstatus, int vmce_vcpuid)
> >  {
> >      struct vcpu *v = d->vcpu[0];
> > +    bool broadcast = (vmce_vcpuid == VMCE_INJECT_BROADCAST);
> >      int ret;
> >  
> >      if ( mc_bank->mc_domid == (uint16_t)~0 )
> >          return -EINVAL;
> >  
> > +    if ( (gstatus & MCG_STATUS_LMCE) && !broadcast )
> > +        v = d->vcpu[vmce_vcpuid];
> 
> While the ID looks to be coming from a trustworthy source, I'd
> still prefer if there was at least an ASSERT() for it to be in range.
>

will do

> > +    if ( broadcast )
> > +        gstatus &= ~MCG_STATUS_LMCE;
> 
> Please combine the two if()s:
> 
>     if ( broadcast )
>         ...
>     else if ( gstatus & MCG_STATUS_LMCE )
>         ...
>
ditto

Thanks,
Haozhong


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel