[PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Ross Lagerwall]

Since commit 191b3f3344ee ("p2m/ept: enable PML in p2m-ept for
log-dirty"), the A and D bits of EPT paging entries are set
unconditionally, regardless of whether PML is enabled or not. This
causes a regression in Xen 4.6 on some processors due to Intel Errata
AVR41 -- HVM guests get severe memory corruption when the A bit is set
due to incorrect TLB flushing on mov to cr3. The errata affects the Atom
C2000 family (Avaton).

To fix, do not set the A bit on this processor family.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/mm/p2m-ept.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 2f3df91..1713a97 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -34,6 +34,8 @@
 
 #include "mm-locks.h"
 
+static bool_t __read_mostly cpu_has_ept_ad;
+
 #define atomic_read_ept_entry(__pepte)                              \
     ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
 
@@ -130,14 +132,14 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
             break;
         case p2m_ram_rw:
             entry->r = entry->w = entry->x = 1;
-            entry->a = entry->d = 1;
+            entry->a = entry->d = cpu_has_ept_ad;
             break;
         case p2m_mmio_direct:
             entry->r = entry->x = 1;
             entry->w = !rangeset_contains_singleton(mmio_ro_ranges,
                                                     entry->mfn);
-            entry->a = 1;
-            entry->d = entry->w;
+            entry->a = cpu_has_ept_ad;
+            entry->d = entry->w && cpu_has_ept_ad;
             break;
         case p2m_ram_logdirty:
             entry->r = entry->x = 1;
@@ -152,7 +154,7 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
                 entry->w = 1;
             else
                 entry->w = 0;
-            entry->a = 1;
+            entry->a = cpu_has_ept_ad;
             /* For both PML or non-PML cases we clear D bit anyway */
             entry->d = 0;
             break;
@@ -160,20 +162,20 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
         case p2m_ram_shared:
             entry->r = entry->x = 1;
             entry->w = 0;
-            entry->a = 1;
+            entry->a = cpu_has_ept_ad;
             entry->d = 0;
             break;
         case p2m_grant_map_rw:
         case p2m_map_foreign:
             entry->r = entry->w = 1;
             entry->x = 0;
-            entry->a = entry->d = 1;
+            entry->a = entry->d = cpu_has_ept_ad;
             break;
         case p2m_grant_map_ro:
         case p2m_mmio_write_dm:
             entry->r = 1;
             entry->w = entry->x = 0;
-            entry->a = 1;
+            entry->a = cpu_has_ept_ad;
             entry->d = 0;
             break;
     }
@@ -233,7 +235,7 @@ static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t *ept_entry)
 
     ept_entry->r = ept_entry->w = ept_entry->x = 1;
     /* Manually set A bit to avoid overhead of MMU having to write it later. */
-    ept_entry->a = 1;
+    ept_entry->a = cpu_has_ept_ad;
 
     ept_entry->suppress_ve = 1;
 
@@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
     p2m->memory_type_changed = ept_memory_type_changed;
     p2m->audit_p2m = NULL;
 
+    /* Work around Errata AVR41 on Avaton processors. */
+    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
+
     /* Set the memory type used when accessing EPT paging structures. */
     ept->ept_mt = EPT_DEFAULT_MT;
 
-- 
2.4.3

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Tim Deegan]

Hi,

At 13:39 +0100 on 28 Sep (1443447574), Ross Lagerwall wrote:
> Since commit 191b3f3344ee ("p2m/ept: enable PML in p2m-ept for
> log-dirty"), the A and D bits of EPT paging entries are set
> unconditionally, regardless of whether PML is enabled or not. This
> causes a regression in Xen 4.6 on some processors due to Intel Errata
> AVR41 -- HVM guests get severe memory corruption when the A bit is set
> due to incorrect TLB flushing on mov to cr3. The errata affects the Atom
> C2000 family (Avaton).
> 
> To fix, do not set the A bit on this processor family.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

This looks pretty good as a 4.6 fix, though:

> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>      p2m->memory_type_changed = ept_memory_type_changed;
>      p2m->audit_p2m = NULL;
>  
> +    /* Work around Errata AVR41 on Avaton processors. */
> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
> +

Shouldn't this check the family (a.k.a. boot_cpu_data.x86) too?

Cheers,

Tim.

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Wei Liu]

CC Kai Huang

On Mon, Sep 28, 2015 at 01:39:34PM +0100, Ross Lagerwall wrote:
> Since commit 191b3f3344ee ("p2m/ept: enable PML in p2m-ept for
> log-dirty"), the A and D bits of EPT paging entries are set
> unconditionally, regardless of whether PML is enabled or not. This
> causes a regression in Xen 4.6 on some processors due to Intel Errata
> AVR41 -- HVM guests get severe memory corruption when the A bit is set
> due to incorrect TLB flushing on mov to cr3. The errata affects the Atom
> C2000 family (Avaton).
> 
> To fix, do not set the A bit on this processor family.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/arch/x86/mm/p2m-ept.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 2f3df91..1713a97 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -34,6 +34,8 @@
>  
>  #include "mm-locks.h"
>  
> +static bool_t __read_mostly cpu_has_ept_ad;
> +
>  #define atomic_read_ept_entry(__pepte)                              \
>      ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
>  
> @@ -130,14 +132,14 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>              break;
>          case p2m_ram_rw:
>              entry->r = entry->w = entry->x = 1;
> -            entry->a = entry->d = 1;
> +            entry->a = entry->d = cpu_has_ept_ad;
>              break;
>          case p2m_mmio_direct:
>              entry->r = entry->x = 1;
>              entry->w = !rangeset_contains_singleton(mmio_ro_ranges,
>                                                      entry->mfn);
> -            entry->a = 1;
> -            entry->d = entry->w;
> +            entry->a = cpu_has_ept_ad;
> +            entry->d = entry->w && cpu_has_ept_ad;
>              break;
>          case p2m_ram_logdirty:
>              entry->r = entry->x = 1;
> @@ -152,7 +154,7 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>                  entry->w = 1;
>              else
>                  entry->w = 0;
> -            entry->a = 1;
> +            entry->a = cpu_has_ept_ad;
>              /* For both PML or non-PML cases we clear D bit anyway */
>              entry->d = 0;
>              break;
> @@ -160,20 +162,20 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>          case p2m_ram_shared:
>              entry->r = entry->x = 1;
>              entry->w = 0;
> -            entry->a = 1;
> +            entry->a = cpu_has_ept_ad;
>              entry->d = 0;
>              break;
>          case p2m_grant_map_rw:
>          case p2m_map_foreign:
>              entry->r = entry->w = 1;
>              entry->x = 0;
> -            entry->a = entry->d = 1;
> +            entry->a = entry->d = cpu_has_ept_ad;
>              break;
>          case p2m_grant_map_ro:
>          case p2m_mmio_write_dm:
>              entry->r = 1;
>              entry->w = entry->x = 0;
> -            entry->a = 1;
> +            entry->a = cpu_has_ept_ad;
>              entry->d = 0;
>              break;
>      }
> @@ -233,7 +235,7 @@ static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t *ept_entry)
>  
>      ept_entry->r = ept_entry->w = ept_entry->x = 1;
>      /* Manually set A bit to avoid overhead of MMU having to write it later. */
> -    ept_entry->a = 1;
> +    ept_entry->a = cpu_has_ept_ad;
>  
>      ept_entry->suppress_ve = 1;
>  
> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>      p2m->memory_type_changed = ept_memory_type_changed;
>      p2m->audit_p2m = NULL;
>  
> +    /* Work around Errata AVR41 on Avaton processors. */
> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
> +
>      /* Set the memory type used when accessing EPT paging structures. */
>      ept->ept_mt = EPT_DEFAULT_MT;
>  
> -- 
> 2.4.3

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Jan Beulich]

>>> On 28.09.15 at 14:43, <tim@xen.org> wrote:
> At 13:39 +0100 on 28 Sep (1443447574), Ross Lagerwall wrote:
>> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>      p2m->memory_type_changed = ept_memory_type_changed;
>>      p2m->audit_p2m = NULL;
>>  
>> +    /* Work around Errata AVR41 on Avaton processors. */
>> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
>> +
> 
> Shouldn't this check the family (a.k.a. boot_cpu_data.x86) too?

Definitely. Considering that the spec update says that the firmware
may contain a workaround (microcode update) perhaps even the
stepping and microcode level would need looking at.

Jan

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Jan Beulich]

>>> On 28.09.15 at 14:39, <ross.lagerwall@citrix.com> wrote:
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -34,6 +34,8 @@
>  
>  #include "mm-locks.h"
>  
> +static bool_t __read_mostly cpu_has_ept_ad;

This should be
#define cpu_has_ept_ad (vmx_ept_vpid_cap & VMX_EPT_AD_BIT)
put next to the respective other ones in vmx.h.

> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>      p2m->memory_type_changed = ept_memory_type_changed;
>      p2m->audit_p2m = NULL;
>  
> +    /* Work around Errata AVR41 on Avaton processors. */
> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;

And this one then should turn off said flag (i.e. needs to be moved
elsewhere).

Plus PML initialization should get a respective check added.

Jan

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Andrew Cooper]

On 28/09/15 15:03, Jan Beulich wrote:
>>>> On 28.09.15 at 14:43, <tim@xen.org> wrote:
>> At 13:39 +0100 on 28 Sep (1443447574), Ross Lagerwall wrote:
>>> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>>      p2m->memory_type_changed = ept_memory_type_changed;
>>>      p2m->audit_p2m = NULL;
>>>  
>>> +    /* Work around Errata AVR41 on Avaton processors. */
>>> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
>>> +
>> Shouldn't this check the family (a.k.a. boot_cpu_data.x86) too?
> Definitely. Considering that the spec update says that the firmware
> may contain a workaround (microcode update) perhaps even the
> stepping and microcode level would need looking at.

There are no details available as to what firmware fix might be
available, nor how to determine whether the issue is fixed.

~Andrew

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Jan Beulich]

>>> On 28.09.15 at 16:48, <andrew.cooper3@citrix.com> wrote:
> On 28/09/15 15:03, Jan Beulich wrote:
>>>>> On 28.09.15 at 14:43, <tim@xen.org> wrote:
>>> At 13:39 +0100 on 28 Sep (1443447574), Ross Lagerwall wrote:
>>>> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>>>      p2m->memory_type_changed = ept_memory_type_changed;
>>>>      p2m->audit_p2m = NULL;
>>>>  
>>>> +    /* Work around Errata AVR41 on Avaton processors. */
>>>> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
>>>> +
>>> Shouldn't this check the family (a.k.a. boot_cpu_data.x86) too?
>> Definitely. Considering that the spec update says that the firmware
>> may contain a workaround (microcode update) perhaps even the
>> stepping and microcode level would need looking at.
> 
> There are no details available as to what firmware fix might be
> available, nor how to determine whether the issue is fixed.

Which is why we've been awaiting input from Intel forever since
the submission of the first version of the patch. But of course I
agree that we can always relax the check later on.

Jan

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[George Dunlap]

On 28/09/15 15:09, Jan Beulich wrote:
>>>> On 28.09.15 at 14:39, <ross.lagerwall@citrix.com> wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -34,6 +34,8 @@
>>  
>>  #include "mm-locks.h"
>>  
>> +static bool_t __read_mostly cpu_has_ept_ad;
> 
> This should be
> #define cpu_has_ept_ad (vmx_ept_vpid_cap & VMX_EPT_AD_BIT)
> put next to the respective other ones in vmx.h.
> 
>> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>      p2m->memory_type_changed = ept_memory_type_changed;
>>      p2m->audit_p2m = NULL;
>>  
>> +    /* Work around Errata AVR41 on Avaton processors. */
>> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
> 
> And this one then should turn off said flag (i.e. needs to be moved
> elsewhere).
> 
> Plus PML initialization should get a respective check added.

In case there was any doubt, I am following this thread, and so far
haven't had anything to disagree with the direction Tim and Jan are
suggesting.

 -George

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Kai Huang]

On Mon, Sep 28, 2015 at 10:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 28.09.15 at 14:39, <ross.lagerwall@citrix.com> wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -34,6 +34,8 @@
>>
>>  #include "mm-locks.h"
>>
>> +static bool_t __read_mostly cpu_has_ept_ad;
>
> This should be
> #define cpu_has_ept_ad (vmx_ept_vpid_cap & VMX_EPT_AD_BIT)
> put next to the respective other ones in vmx.h.

+1.

>
>> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>      p2m->memory_type_changed = ept_memory_type_changed;
>>      p2m->audit_p2m = NULL;
>>
>> +    /* Work around Errata AVR41 on Avaton processors. */
>> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
>
> And this one then should turn off said flag (i.e. needs to be moved
> elsewhere).

However if cpu_has_ept_ad is going to be a macro definition as
suggested above, looks we need another variable.

>
> Plus PML initialization should get a respective check added.

My first thinking is checking whether cpu_has_ept_ad is set in ept_enable_pml.

Btw sorry I am taking vacation until Oct.8 so please expect slow
response if you need my comments.

Thanks,
-Kai

>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

Re: [PATCHv2 for-4.6] p2m/ept: Work around hardware errata setting A bit

[Wei Liu]

On Mon, Sep 28, 2015 at 01:39:34PM +0100, Ross Lagerwall wrote:
> Since commit 191b3f3344ee ("p2m/ept: enable PML in p2m-ept for
> log-dirty"), the A and D bits of EPT paging entries are set
> unconditionally, regardless of whether PML is enabled or not. This
> causes a regression in Xen 4.6 on some processors due to Intel Errata
> AVR41 -- HVM guests get severe memory corruption when the A bit is set
> due to incorrect TLB flushing on mov to cr3. The errata affects the Atom
> C2000 family (Avaton).

Typo here, should be Avoton.

> 
[...]
> @@ -1150,6 +1152,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>      p2m->memory_type_changed = ept_memory_type_changed;
>      p2m->audit_p2m = NULL;
>  
> +    /* Work around Errata AVR41 on Avaton processors. */

Same here, Avoton.

> +    cpu_has_ept_ad = boot_cpu_data.x86_model != 0x4d;
> +
>      /* Set the memory type used when accessing EPT paging structures. */
>      ept->ept_mt = EPT_DEFAULT_MT;
>  
> -- 
> 2.4.3