|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Xen/MCE: adjust for future new vMCE model
Jan Beulich wrote:
>>>> On 04.07.12 at 15:08, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c Wed Jun 27 09:36:43 2012 +0200
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c Thu Jul 05 04:28:48 2012 +0800
>> @@ -19,36 +19,42 @@ #include "mce.h"
>> #include "x86_mca.h"
>>
>> +/*
>> + * Emulalte 2 banks for guest
>> + * Bank0: reserved for 'bank0 quirk' occur at some very old
>> processors: + * 1). Intel cpu whose family-model valuse < 06-1A; +
>> * 2). AMD K7 + * Bank1: used to transfer error info to guest
>> + */
>> +#define GUEST_BANK_NUM 2
>> +
>> #define dom_vmce(x) ((x)->arch.vmca_msrs)
>>
>> static uint64_t __read_mostly g_mcg_cap;
>>
>> -/* Real value in physical CTL MSR */
>> -static uint64_t __read_mostly h_mcg_ctl;
>> -static uint64_t *__read_mostly h_mci_ctrl;
>> -
>> int vmce_init_msr(struct domain *d)
>> {
>> dom_vmce(d) = xmalloc(struct domain_mca_msrs); if (
>> !dom_vmce(d) ) return -ENOMEM;
>>
>> - dom_vmce(d)->mci_ctl = xmalloc_array(uint64_t, nr_mce_banks);
>> + dom_vmce(d)->mci_ctl = xmalloc_array(uint64_t, GUEST_BANK_NUM);
>> if ( !dom_vmce(d)->mci_ctl )
>> {
>> xfree(dom_vmce(d));
>> return -ENOMEM;
>> }
>> memset(dom_vmce(d)->mci_ctl, ~0,
>> - nr_mce_banks * sizeof(*dom_vmce(d)->mci_ctl));
>> + GUEST_BANK_NUM * sizeof(*dom_vmce(d)->mci_ctl));
>>
>> dom_vmce(d)->mcg_status = 0x0;
>> - dom_vmce(d)->mcg_ctl = ~(uint64_t)0x0;
>> dom_vmce(d)->nr_injection = 0;
>>
>> INIT_LIST_HEAD(&dom_vmce(d)->impact_header);
>> spin_lock_init(&dom_vmce(d)->lock);
>>
>> + g_mcg_cap = MCG_TES_P | MCG_SER_P | GUEST_BANK_NUM; +
>> return 0;
>> }
>>
>> @@ -68,7 +74,7 @@
>>
>> int vmce_restore_vcpu(struct vcpu *v, uint64_t caps) {
>> - if ( caps & ~g_mcg_cap & ~MCG_CAP_COUNT & ~MCG_CTL_P )
>> + if ( caps & ~g_mcg_cap & ~MCG_CAP_COUNT )
>
> Shouldn't you also check bank count being GUEST_BANK_NUM?
>
>> {
>> dprintk(XENLOG_G_ERR, "%s restore: unsupported MCA
>> capabilities" " %#" PRIx64 " for d%d:v%u
>> (supported: %#Lx)\n", @@ -93,9 +99,10 @@ switch ( msr &
>> (MSR_IA32_MC0_CTL | 3) ) {
>> case MSR_IA32_MC0_CTL:
>> - if ( bank < nr_mce_banks )
>> - *val = vmce->mci_ctl[bank] &
>> - (h_mci_ctrl ? h_mci_ctrl[bank] : ~0UL);
>> + if ( bank < GUEST_BANK_NUM ) {
>
> This being false is impossible afaict (provided guest's MCG_CAP
> is never permitted to hold other than GUEST_BANK_NUM).
>
> However, if the intention is to support an eventual future need
> to grow that number, than an else clause would be needed
> setting *val to ~0. But read on...
>
>> + BUG_ON( vmce->mci_ctl[bank] != ~0UL );
>> + *val = vmce->mci_ctl[bank];
>
> Why do you retain the (per-domain!) array if all slots are always
> expected to be ~0 anyway?
>
>> + }
>> mce_printk(MCE_VERBOSE, "MCE: rdmsr MC%u_CTL 0x%"PRIx64"\n",
>> bank, *val);
>> break;
>> @@ -187,11 +194,9 @@
>> *val);
>> break;
>> case MSR_IA32_MCG_CTL:
>> - /* Always 0 if no CTL support */
>> - if ( cur->arch.mcg_cap & MCG_CTL_P )
>> - *val = vmce->mcg_ctl & h_mcg_ctl;
>> - mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n",
>> - *val);
>> + BUG_ON( cur->arch.mcg_cap & MCG_CTL_P );
>> + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); + ret =
>> -1; break;
>> default:
>> ret = mce_bank_msr(cur, msr) ? bank_mce_rdmsr(cur, msr,
>> val) : 0; @@ -220,8 +225,10 @@ switch ( msr & (MSR_IA32_MC0_CTL
>> | 3) ) {
>> case MSR_IA32_MC0_CTL:
>> - if ( bank < nr_mce_banks )
>> - vmce->mci_ctl[bank] = val;
>> + /*
>> + * if guest crazy clear any bit of MCi_CTL,
>> + * treat it as not implement and ignore write change it. +
>> */ break;
>> case MSR_IA32_MC0_STATUS:
>> if ( entry && (entry->bank == bank) )
>> @@ -305,7 +312,9 @@
>> switch ( msr )
>> {
>> case MSR_IA32_MCG_CTL:
>> - vmce->mcg_ctl = val;
>> + BUG_ON( cur->arch.mcg_cap & MCG_CTL_P );
>> + mce_printk(MCE_QUIET, "MCE: no MCG_CTL\n"); + ret =
>> -1; break;
>> case MSR_IA32_MCG_STATUS:
>> vmce->mcg_status = val;
>> @@ -520,52 +529,6 @@
>> }
>> #endif
>>
>> -int vmce_init(struct cpuinfo_x86 *c)
>> -{
>> - u64 value;
>> - unsigned int i;
>> -
>> - if ( !h_mci_ctrl )
>> - {
>> - h_mci_ctrl = xmalloc_array(uint64_t, nr_mce_banks);
>> - if (!h_mci_ctrl)
>> - {
>> - dprintk(XENLOG_INFO, "Failed to alloc h_mci_ctrl\n");
>> - return -ENOMEM;
>> - }
>> - /* Don't care banks before firstbank */
>> - memset(h_mci_ctrl, ~0,
>> - min(firstbank, nr_mce_banks) * sizeof(*h_mci_ctrl));
>> - for (i = firstbank; i < nr_mce_banks; i++)
>> - rdmsrl(MSR_IA32_MCx_CTL(i), h_mci_ctrl[i]);
>> - }
>> -
>> - rdmsrl(MSR_IA32_MCG_CAP, value);
>> - /* For Guest vMCE usage */
>> - g_mcg_cap = value & (MCG_CAP_COUNT | MCG_CTL_P | MCG_TES_P |
>> MCG_SER_P);
>> - if (value & MCG_CTL_P)
>> - rdmsrl(MSR_IA32_MCG_CTL, h_mcg_ctl);
>> -
>> - return 0;
>> -}
>> -
>> -static int mca_ctl_conflict(struct mcinfo_bank *bank, struct domain
>> *d) -{
>> - int bank_nr;
>> -
>> - if ( !bank || !d || !h_mci_ctrl )
>> - return 1;
>> -
>> - /* Will MCE happen in host if If host mcg_ctl is 0? */
>> - if ( ~d->arch.vmca_msrs->mcg_ctl & h_mcg_ctl )
>> - return 1;
>> -
>> - bank_nr = bank->mc_bank;
>> - if (~d->arch.vmca_msrs->mci_ctl[bank_nr] & h_mci_ctrl[bank_nr] )
>> - return 1;
>> - return 0;
>> -}
>> -
>> static int is_hvm_vmce_ready(struct mcinfo_bank *bank, struct
>> domain *d) { struct vcpu *v;
>> @@ -619,14 +582,6 @@
>> if (no_vmce)
>> return 0;
>>
>> - /* Guest has different MCE ctl value setting */
>> - if (mca_ctl_conflict(bank, d))
>> - {
>> - dprintk(XENLOG_WARNING,
>> - "No vmce, guest has different mca control setting\n");
>> - return 0;
>> - }
>> -
>> return 1;
>> }
>>
>
> Also I seem to recall that there was going to be a need to
> save/restore MCi_CTL2, but there's nothing being done in that
> direction.
>
> Jan
Yes, but that would be done at new vMCE.
Current vMCE didn't enable MCG_CMCI_P, and MCi_CTL2 are not used, so we don't
save/restore MCi_CTL2 at this version.
Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |