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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +                      global->mc_domid != bank->mc_domid ||

I don't understand this check.

> --- 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.

> +    if ( broadcast )
> +        gstatus &= ~MCG_STATUS_LMCE;

Please combine the two if()s:

    if ( broadcast )
        ...
    else if ( gstatus & MCG_STATUS_LMCE )
        ...

Jan


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