[Xen-devel] RE: [PATCH 2/6] MCE: Not GP fault when guest write non 0s or 1s to MCA CTL MSRs.

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

>-----Original Message-----
>From: Christoph Egger [mailto:Christoph.Egger@xxxxxxx]
>Sent: Thursday, January 28, 2010 4:12 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; Frank.Vanderlinden@xxxxxxx; Jan Beulich;
>xen-devel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [PATCH 2/6] MCE: Not GP fault when guest write non 0s or 1s to MCA
>CTL MSRs.
>
>On Thursday 28 January 2010 06:55:53 Jiang, Yunhong wrote:
>> Not GP fault when guest write non 0s or 1s to MCA CTL MSRs.
>>
>> a) For Mci_CTL MSR, Guest can write any value to it. When read back, it
>> will be ANDed with the physical value. Some bit in physical value can be 0,
>> either because read-only in hardware (like masked by AMD's Mci_CTL_MASK),
>> or because Xen didn't enable it. If guest write some bit as 0, while that
>> bit is 1 in host, we will not inject MCE corresponding that bank to guest,
>> as we can't distinguish if the MCE is caused by the guest-cleared bit.
>>
>> b) For MCG_CTL MSR, guest can write any value to it. When read back, it
>> will be ANDed with the physical value. If guest does not write all 1s. In
>> mca_ctl_conflict(), we simply not inject any vMCE to guest if some bit is
>> set in physical MSR while is cleared in guest 's vMCG_CTL MSR.
>>
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@xxxxxxxxx>
>>
>>
>> diff -r b2a7600b71cc xen/arch/x86/cpu/mcheck/mce.c
>> --- a/xen/arch/x86/cpu/mcheck/mce.c  Tue Jan 26 00:52:19 2010 +0800
>> +++ b/xen/arch/x86/cpu/mcheck/mce.c  Tue Jan 26 00:57:19 2010 +0800
>> @@ -30,6 +30,11 @@ unsigned int nr_mce_banks;
>>  unsigned int nr_mce_banks;
>>
>>  static uint64_t g_mcg_cap;
>> +
>> +/* Real value in physical CTL MSR */
>> +static uint64_t h_mcg_ctl = 0UL;
>> +static uint64_t *h_mci_ctrl;
>> +int firstbank;
>
>The physical MSRs are per physical cpu-core on AMD CPUs.
>The data structure does not reflect this.
>
>Other than that this patch is fine with me.

Christoph, thanks for your review very much.

Yes, the MSRs are mostly per cpu-core in Intel side also, although some sharing 
may happen.
On AMD platform, will different CPU has different ctrl value? Or will Xen setup 
different value to different CPU? I assume they should be same, am I right? 

Thanks
--jyh

>
>Christoph
>
>
>>
>>  static void intpose_init(void);
>>  static void mcinfo_clear(struct mc_info *);
>> @@ -642,6 +647,21 @@ void mcheck_init(struct cpuinfo_x86 *c)
>>              break;
>>      }
>>
>> +    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;
>> +        }
>> +        /* Don't care banks before firstbank */
>> +        memset(h_mci_ctrl, 0xff, sizeof(h_mci_ctrl));
>> +        for (i = firstbank; i < nr_mce_banks; i++)
>> +            rdmsrl(MSR_IA32_MC0_CTL + 4*i, h_mci_ctrl[i]);
>> +    }
>> +    if (g_mcg_cap & MCG_CTL_P)
>> +        rdmsrl(MSR_IA32_MCG_CTL, h_mcg_ctl);
>>      set_poll_bankmask(c);
>>      if (!inited)
>>              printk(XENLOG_INFO "CPU%i: No machine check initialization\n",
>> @@ -708,7 +728,8 @@ int mce_rdmsr(uint32_t msr, uint64_t *va
>>              *val);
>>          break;
>>      case MSR_IA32_MCG_CTL:
>> -        *val = d->arch.vmca_msrs.mcg_ctl;
>> +        /* Always 0 if no CTL support */
>> +        *val = d->arch.vmca_msrs.mcg_ctl & h_mcg_ctl;
>>          mce_printk(MCE_VERBOSE, "MCE: rdmsr MCG_CTL 0x%"PRIx64"\n",
>>              *val);
>>          break;
>> @@ -723,7 +744,8 @@ int mce_rdmsr(uint32_t msr, uint64_t *va
>>          switch (msr & (MSR_IA32_MC0_CTL | 3))
>>          {
>>          case MSR_IA32_MC0_CTL:
>> -            *val = d->arch.vmca_msrs.mci_ctl[bank];
>> +            *val = d->arch.vmca_msrs.mci_ctl[bank] &
>> +                    (h_mci_ctrl ? h_mci_ctrl[bank] : ~0UL);
>>              mce_printk(MCE_VERBOSE, "MCE: rdmsr MC%u_CTL
>0x%"PRIx64"\n",
>>                       bank, *val);
>>              break;
>> @@ -805,13 +827,6 @@ int mce_wrmsr(u32 msr, u64 val)
>>      switch ( msr )
>>      {
>>      case MSR_IA32_MCG_CTL:
>> -        if ( val && (val + 1) )
>> -        {
>> -            mce_printk(MCE_QUIET, "MCE: val \"%"PRIx64"\" written "
>> -                     "to MCG_CTL should be all 0s or 1s\n", val);
>> -            ret = -1;
>> -            break;
>> -        }
>>          d->arch.vmca_msrs.mcg_ctl = val;
>>          break;
>>      case MSR_IA32_MCG_STATUS:
>> @@ -855,14 +870,6 @@ int mce_wrmsr(u32 msr, u64 val)
>>          switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>          {
>>          case MSR_IA32_MC0_CTL:
>> -            if ( val && (val + 1) )
>> -            {
>> -                mce_printk(MCE_QUIET, "MCE: val written to MC%u_CTL "
>> -                         "should be all 0s or 1s (is %"PRIx64")\n",
>> -                         bank, val);
>> -                ret = -1;
>> -                break;
>> -            }
>>              d->arch.vmca_msrs.mci_ctl[bank] = val;
>>              break;
>>          case MSR_IA32_MC0_STATUS:
>> @@ -1162,6 +1169,23 @@ void intpose_inval(unsigned int cpu_nr,
>>      (r) <= MSR_IA32_MC0_MISC + (nr_mce_banks - 1) * 4 && \
>>      ((r) - MSR_IA32_MC0_CTL) % 4 != 0)      /* excludes MCi_CTL */
>>
>> +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 x86_mc_msrinject_verify(struct xen_mc_msrinject *mci)
>>  {
>>      struct cpuinfo_x86 *c;
>> diff -r b2a7600b71cc xen/arch/x86/cpu/mcheck/mce.h
>> --- a/xen/arch/x86/cpu/mcheck/mce.h  Tue Jan 26 00:52:19 2010 +0800
>> +++ b/xen/arch/x86/cpu/mcheck/mce.h  Tue Jan 26 00:52:20 2010 +0800
>> @@ -39,6 +39,8 @@ void amd_nonfatal_mcheck_init(struct cpu
>>  void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c);
>>
>>  u64 mce_cap_init(void);
>> +extern int firstbank;
>> +int mca_ctl_conflict(struct mcinfo_bank *bank, struct domain *d);
>>
>>  int intel_mce_rdmsr(uint32_t msr, uint64_t *val);
>>  int intel_mce_wrmsr(uint32_t msr, uint64_t val);
>> diff -r b2a7600b71cc xen/arch/x86/cpu/mcheck/mce_intel.c
>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c    Tue Jan 26 00:52:19 2010 +0800
>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c    Tue Jan 26 00:52:20 2010 +0800
>> @@ -20,7 +20,6 @@ int ser_support = 0;
>>  int ser_support = 0;
>>
>>  static int nr_intel_ext_msrs = 0;
>> -static int firstbank;
>>
>>  /* Below are for MCE handling */
>>  struct mce_softirq_barrier {
>> @@ -361,7 +360,15 @@ static void intel_UCR_handler(struct mci
>>                         *  the mfn in question) */
>>                        BUG_ON( result->owner == DOMID_COW );
>>                        if ( result->owner != DOMID_XEN ) {
>> +
>>                            d = get_domain_by_id(result->owner);
>> +                          if ( mca_ctl_conflict(bank, d) )
>> +                          {
>> +                              /* Guest has different MCE ctl with
>> hypervisor */ +                              put_domain(d);
>> +                              return;
>> +                          }
>> +
>>                            gfn =
>>                                mfn_to_gmfn(d, ((bank->mc_addr) >>
>> PAGE_SHIFT)); bank->mc_addr =
>
>
>
>--
>---to satisfy European Law for business letters:
>Advanced Micro Devices GmbH
>Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel