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

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> 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.
> 

OK, agree all comments, will update accordingly and resubmit after that.

Thanks,
Jinsong

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