[PATCH 4] MCA physical address check when calculate domain

[PATCH 4] MCA physical address check when calculate domain

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

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@intel.com>

diff -r e4e1efda200f xen/arch/x86/cpu/mcheck/mce.c
--- a/xen/arch/x86/cpu/mcheck/mce.c	Thu May 05 13:54:29 2011 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce.c	Fri May 06 13:56:21 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) && 
+          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 != NULL && (who == MCA_POLLER ||
+                          who == MCA_CMCI_HANDLER))
+            mib->mc_domid = d->domain_id;
     }
 
     if (who == MCA_CMCI_HANDLER) {

[-- Attachment #2: mca-cleanup-4.patch --]
[-- Type: application/octet-stream, Size: 1931 bytes --]

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@intel.com>

diff -r e4e1efda200f xen/arch/x86/cpu/mcheck/mce.c
--- a/xen/arch/x86/cpu/mcheck/mce.c	Thu May 05 13:54:29 2011 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce.c	Fri May 06 13:56:21 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) && 
+          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 != NULL && (who == MCA_POLLER ||
+                          who == MCA_CMCI_HANDLER))
+            mib->mc_domid = d->domain_id;
     }
 
     if (who == MCA_CMCI_HANDLER) {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 4] MCA physical address check when calculate domain

[Jan Beulich]

>>> On 07.05.11 at 22:29, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 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@intel.com>
> 
> diff -r e4e1efda200f xen/arch/x86/cpu/mcheck/mce.c
> --- a/xen/arch/x86/cpu/mcheck/mce.c	Thu May 05 13:54:29 2011 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce.c	Fri May 06 13:56:21 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) && 
> +          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 != NULL && (who == MCA_POLLER ||
> +                          who == MCA_CMCI_HANDLER))
> +            mib->mc_domid = d->domain_id;

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

>      }
>  
>      if (who == MCA_CMCI_HANDLER) {

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

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]


> 
> 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@intel.com>

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 #2: mca-cleanup-4.patch --]
[-- Type: application/octet-stream, Size: 1904 bytes --]

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@intel.com>

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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

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

[Jan Beulich]

>>> On 10.05.11 at 08:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 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;

I realize this is what's being documented currently. Going back to the
newest hard copy manual I still have (PentiumPro, which luckily is the
first one where the banked implementation is described), there's no
MCi_MISC (it's documented, but said to not be implemented on these
old CPUs), and the description for the address reads "The address
returned is either 32-bit virtual, 32-bit linear, or 36-bit physical". Now
I certainly don't care much about PPro anymore, but I wonder when
MCi_MISC was first implemented in the way your patch is using it.

Further, the current manual also makes a distinction between
"Physical Address" and "Memory Address", and additionally has
a "Generic" type - all without further explanation.

Jan

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

[Liu, Jinsong]

Jan Beulich wrote:
>>>> On 10.05.11 at 08:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>> 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;
> 
> I realize this is what's being documented currently. Going back to the
> newest hard copy manual I still have (PentiumPro, which luckily is the
> first one where the banked implementation is described), there's no
> MCi_MISC (it's documented, but said to not be implemented on these
> old CPUs), and the description for the address reads "The address
> returned is either 32-bit virtual, 32-bit linear, or 36-bit
> physical". Now I certainly don't care much about PPro anymore, but I
> wonder when MCi_MISC was first implemented in the way your patch is
> using it. 
> 

Seems needn't care about when MCi_MISC first implemented. MCi_STATUS_MISCV check can make sure accessing MCi_MISC safely.

If really want to know MCi_MISC implemented at which processor, we can use cpuid DF_DM as clue. 
Different MCi_MISC implemented at different DF_DM processors (refer Intel SDM, Appendix B).
Currently there are 22 MCi_MISC (refer Table B-2), generally
MC0~4_MISC implement for all P6 processors;
MC5_MISC after 06_0F;
MC6_MISC after 06_1D;
...
MC21_MISC after 06_2E;

However, if want to check exactually how many MCi_MISC reg a processor has, it has to check 'count' field of IA32_MCG_CAP.
Take SNB processor (refer Table B-10) as example: It only support MC0~3_MISC openly (this does mean it has no other MCi_MISC, but means we can use MC0~3_MISC SAFELY).


> Further, the current manual also makes a distinction between
> "Physical Address" and "Memory Address", and additionally has
> a "Generic" type - all without further explanation.
> 
> Jan

Memory address is from the view of memory controller, say, channel number, as far as I understand. 
I don't know what 'Generic' means either.

Thanks,
Jinsong

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

[Jan Beulich]

>>> On 10.05.11 at 12:46, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> Jan Beulich wrote:
>>>>> On 10.05.11 at 08:38, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>>> 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;
>> 
>> I realize this is what's being documented currently. Going back to the
>> newest hard copy manual I still have (PentiumPro, which luckily is the
>> first one where the banked implementation is described), there's no
>> MCi_MISC (it's documented, but said to not be implemented on these
>> old CPUs), and the description for the address reads "The address
>> returned is either 32-bit virtual, 32-bit linear, or 36-bit
>> physical". Now I certainly don't care much about PPro anymore, but I
>> wonder when MCi_MISC was first implemented in the way your patch is
>> using it. 
>> 
> 
> Seems needn't care about when MCi_MISC first implemented. MCi_STATUS_MISCV 
> check can make sure accessing MCi_MISC safely.

That wasn't my point. The question is whether there's a way to tell the
address format when there's no MCi_MISC implemented (or whether
all but *very* old CPUs have these registers for *all* their banks).

Jan

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

[Liu, Jinsong]

Jan Beulich wrote:
>>>> On 10.05.11 at 12:46, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>> Jan Beulich wrote:
>>>>>> On 10.05.11 at 08:38, "Liu, Jinsong" <jinsong.liu@intel.com>
>>>>>> wrote: 
>>>> 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;
>>> 
>>> I realize this is what's being documented currently. Going back to
>>> the newest hard copy manual I still have (PentiumPro, which luckily
>>> is the first one where the banked implementation is described),
>>> there's no MCi_MISC (it's documented, but said to not be
>>> implemented on these old CPUs), and the description for the address
>>> reads "The address returned is either 32-bit virtual, 32-bit
>>> linear, or 36-bit physical". Now I certainly don't care much about
>>> PPro anymore, but I wonder when MCi_MISC was first implemented in
>>> the way your patch is using it. 
>>> 
>> 
>> Seems needn't care about when MCi_MISC first implemented.
>> MCi_STATUS_MISCV check can make sure accessing MCi_MISC safely.
> 
> That wasn't my point. The question is whether there's a way to tell
> the address format when there's no MCi_MISC implemented (or whether
> all but *very* old CPUs have these registers for *all* their banks).
> 
> Jan

CC Tony, our RAS expert.

For the case MCi_MISC not implemented, I don't know exactly. 
no MCi_MISC implemented only occur at very old Intel cpu. More exactly, it's only for P5 processors. After P6, all processors has MCi_MISC as far as we check till now.
For P5 processors, s/w make no sense to figure out address format --> the basic action is to show the error at console and then aborting execution, according to Intel SDM.

And, according to Intel SDM, for Machine check MSRs in the Pentium 4, Intel Xeon, and P6 family processors, they have these MSRs for all their banks.
However, I think SDM didn't guarantee so in all future processor, so s/w much check MISCV/ADDRV strictly abide by SDM, and handle different cases correctly at MCE handler and GP handler.

Thanks,
Jinsong

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

[Luck, Tony]

Before Nehalem memory errors were reported via NMI ... use of
MCA for this (and thus use of MCi_ADDR) begins with the memory
controller moving on-die.

So if you are interested in memory errors - you are out of luck
on pre-NHM systems.

MCi_ADDR on the pre-NHM systems was sometimes used to report
cache errors (index/rank/...)

-Tony