Re: [Xen-devel] [Patch 3/6] Xen/MCE: vMCE emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.12 at 11:40, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:

>From here on, I think this is to go in only after 4.2. Hence
eventual resubmission wouldn't be necessary until after 4.2
went out.

> -int intel_mce_rdmsr(const struct vcpu *, uint32_t msr, uint64_t *val);
> -int intel_mce_wrmsr(struct vcpu *, uint32_t msr, uint64_t val);
> +void intel_vmce_mci_ctl2_rdmsr(const struct vcpu *, uint32_t msr, uint64_t 
> *val);
> +void intel_vmce_mci_ctl2_wrmsr(struct vcpu *, uint32_t msr, uint64_t val);

I don't see a need for renaming those - they could well serve to
deal with eventual other Intel specific additions to the MSR space
in the future.

> -int intel_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> +void intel_vmce_mci_ctl2_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>  {
> -    int ret = 0;
> +    int bank = msr - MSR_IA32_MC0_CTL2;

'unsigned int' here allows ...

>  
> -    if ( msr >= MSR_IA32_MC0_CTL2 &&
> -         msr < MSR_IA32_MCx_CTL2(v->arch.mcg_cap & MCG_CAP_COUNT) )
> +    if ( (bank >= 0) && (bank < GUEST_BANK_NUM) )

... the first check here to be dropped (and is more natural as
well as in line with other code in this file).

>  void vmce_init_vcpu(struct vcpu *v)
>  {
> -    v->arch.mcg_cap = GUEST_MCG_CAP;
> +    int i;
> +
> +    /* global MCA MSRs init */
> +    v->arch.vmce.mcg_cap = GUEST_MCG_CAP;
> +    v->arch.vmce.mcg_status = 0;
> +
> +    /* per-bank MCA MSRs init */
> +    for ( i = 0; i < GUEST_BANK_NUM; i++ )
> +    {
> +        v->arch.vmce.bank[i].mci_status = 0;
> +        v->arch.vmce.bank[i].mci_addr = 0;
> +        v->arch.vmce.bank[i].mci_misc = 0;
> +        v->arch.vmce.bank[i].mci_ctl2 = 0;
> +    }

memset()?

> @@ -3,28 +3,46 @@
>  #ifndef _XEN_X86_MCE_H
>  #define _XEN_X86_MCE_H
>  
> -/* This entry is for recording bank nodes for the impacted domain,
> - * put into impact_header list. */
> -struct bank_entry {
> -    struct list_head list;
> -    uint16_t bank;
> +/*
> + * Emulalte 2 banks for guest
> + * Bank0: reserved for 'bank0 quirk' occur at some very old processors:
> + *   1). Intel cpu whose family-model value < 06-1A;
> + *   2). AMD K7
> + * Bank1: used to transfer error info to guest
> + */
> +#define BANK0 0
> +#define BANK1 1

These two look superfluous.

> +#define GUEST_BANK_NUM 2

This one (pluse the BANK* ones if you strongly feel they should be
kept) should get MC... added somewhere in their names, as this is
a header that's not private to the MCE code.

> +
> +/*
> + * MCG_SER_P:  software error recovery supported
> + * MCG_TES_P:  to avoid MCi_status bit56:53 model specific
> + * MCG_CMCI_P: expose CMCI capability but never really inject it to guest,
> + *             for sake of performance since guest not polling periodically
> + */
> +#define GUEST_MCG_CAP (MCG_SER_P | MCG_TES_P | MCG_CMCI_P | GUEST_BANK_NUM)

Didn't we settle on not enabling CMCI_P and TES_P for AMD CPUs?

> +
> +/* Filter MSCOD model specific error code to guest */
> +#define MCi_STATUS_MSCOD_MASK (~(0x0ffffUL << 16))

Is that really correct, especially for both 32- and 64-bit?

Jan


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