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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Plus - no leading underscore please.

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

Jan


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