Re: [Xen-devel] [PATCH 15/19] x86/vmce: emulate MSR_IA32_MCG_EXT_CTL

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 02/22/17 08:36 -0700, Jan Beulich wrote:
> >>> On 17.02.17 at 07:39, <haozhong.zhang@xxxxxxxxx> wrote:
> > @@ -190,6 +191,25 @@ int vmce_rdmsr(uint32_t msr, uint64_t *val)
> >              *val = ~0ULL;
> >          mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_CTL %#"PRIx64"\n", cur, 
> > *val);
> >          break;
> > +    case MSR_IA32_MCG_EXT_CTL:
> > +        /*
> > +         * If MCG_LMCE_P is present in guest MSR_IA32_MCG_CAP, the LMCE 
> > and LOCK
> > +         * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen , 
> > so it
> 
> Stray blank before comma.
> 
> > +         * does not need to check them here.
> > +         */
> > +        if ( !(cur->arch.vmce.mcg_cap & MCG_LMCE_P) )
> 
> Please invert the condition (swapping if and else blocks): This is not
> only easier to read, but also gives the compiler correct information
> on which case we expect to be the preferred (normal) one (at least
> in the long run).
>

will do

> > +        {
> > +            ret = -1;
> > +            mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL, not 
> > supported\n",
> > +                       cur);
> > +        }
> > +        else
> > +        {
> > +            *val = cur->arch.vmce.lmce_enabled ? MCG_EXT_CTL_LMCE_EN : 0;
> > +            mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL 
> > %#"PRIx64"\n",
> > +                       cur, *val);
> > +        }
> > +        break;
> >      default:
> 
> Even if this isn't currently the case for the rest of this switch
> statement, please add blank lines between non-fall-through case
> blocks.
>

ditto

> > --- a/xen/include/asm-x86/mce.h
> > +++ b/xen/include/asm-x86/mce.h
> > @@ -29,6 +29,7 @@ struct vmce {
> >      uint64_t mcg_status;
> >      spinlock_t lock;
> >      struct vmce_bank bank[GUEST_MC_BANK_NUM];
> > +    bool lmce_enabled;
> 
> I think this better goes ahead of bank[].
>

ditto

> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -599,6 +599,8 @@ struct hvm_vmce_vcpu {
> >      uint64_t caps;
> >      uint64_t mci_ctl2_bank0;
> >      uint64_t mci_ctl2_bank1;
> > +    uint8_t  lmce_enabled;
> > +    uint8_t  _pad[7];
> 
> This implicitly is a domctl interface change, so you need to bump the
> interface version there (this hasn't happened yet afaics after 4.8
> was branched off).
>

ditto

> Plus - no leading underscore please.

ditto

> 
> All of this said - is this really a per-vCPU property, instead of a
> per-domain one?

Per-vCPU. At least it can be set in the per-CPU way. Patch 16, which
implements the vLMCE injection, checks this per-vcpu flag when
injecting vMCE. If the flag is cleared, vMCE (w/ MCG_STATUS_LMCE
removed) will be broadcast to all vcpus.

Thanks,
Haozhong

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