RE: [Xen-devel] [PATCH 4] MCA physical address check when calculate domain

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

> 
> This wasn't very logical before, and would probably be good if it
> got changed while you re-structure it anyway: Rather than
> calling maddr_get_owner() and *then* checking "who", that
> check should be added to the other surrounding ones, thus
> avoiding the possibly pointless call.
> 
> Further I wonder whether there aren't more cases for which the
> domain ID could be set, as well as whether the !MISCV case
> shouldn't also assume the address to be physical. Wouldn't that
> be the case e.g. on older CPUs?
> 
> Jan
> 

Thanks Jan for comments!
I update the mca_cleanup_4.patch according to your comments, as attached.

As the cases of domain ID be set, for poller and cmci handler it set here, and 
for mce handler it would be set at mce softirq context. Seems it's a historic 
topic between Intel and AMD guys when xen mca coding at earlier time.
Here I don't want to involve into the earlier topic. I just do some cleanup, 
for example, add more strict check for physical address to make sure domain id 
calculation is safe.

As for physical addr, the addr in MCi_ADDR reg may be linear add/ physical add/ 
setment offset.
according to Intel SDM, the addr in MCi_ADDR reg is physical addr only when:
1). MISCV bit of MCi_STATUS set;
2). ADDRV bit of MCi_STATUS set;
3). address mode of MCi_MISC (bit 6~8) = 010;


================
MCA physical address check when calculate domain

Bank addr maybe invalid, or
Bank addr maybe physical/memory/linear address or segment offset.
This patch add mca MCi_STATUS_MISCV/MCi_STATUS_ADDRV check, and add physical 
address verify,
so that it work safe when calculate domain.

Signed-off-by: Liu, Jinsong <jinsong.liu@xxxxxxxxx>

diff -r 1d8639e2c825 xen/arch/x86/cpu/mcheck/mce.c
--- a/xen/arch/x86/cpu/mcheck/mce.c     Sun May 08 13:39:40 2011 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce.c     Tue May 10 13:56:30 2011 +0800
@@ -151,7 +151,6 @@ static struct mcinfo_bank *mca_init_bank
                                          struct mc_info *mi, int bank)
 {
     struct mcinfo_bank *mib;
-    uint64_t addr=0, misc = 0;
 
     if (!mi)
         return NULL;
@@ -170,22 +169,23 @@ static struct mcinfo_bank *mca_init_bank
     mib->common.size = sizeof (struct mcinfo_bank);
     mib->mc_bank = bank;
 
-    addr = misc = 0;
     if (mib->mc_status & MCi_STATUS_MISCV)
         mib->mc_misc = mca_rdmsr(MSR_IA32_MCx_MISC(bank));
 
     if (mib->mc_status & MCi_STATUS_ADDRV)
-    {
         mib->mc_addr = mca_rdmsr(MSR_IA32_MCx_ADDR(bank));
 
-        if (mfn_valid(paddr_to_pfn(mib->mc_addr))) {
-            struct domain *d;
+    if ((mib->mc_status & MCi_STATUS_MISCV) &&
+        (mib->mc_status & MCi_STATUS_ADDRV) &&
+        ((mib->mc_misc & MCi_MISC_ADDRMOD_MASK) == MCi_MISC_PHYSMOD) && 
+        (who == MCA_POLLER || who == MCA_CMCI_HANDLER) &&
+        (mfn_valid(paddr_to_pfn(mib->mc_addr))))
+    {
+        struct domain *d;
 
-            d = maddr_get_owner(mib->mc_addr);
-            if (d != NULL && (who == MCA_POLLER ||
-                              who == MCA_CMCI_HANDLER))
-                mib->mc_domid = d->domain_id;
-        }
+        d = maddr_get_owner(mib->mc_addr);
+        if (d)
+            mib->mc_domid = d->domain_id;
     }
 
     if (who == MCA_CMCI_HANDLER) {

================

Regards,
Jinsong

Attachment: mca-cleanup-4.patch
Description: mca-cleanup-4.patch

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