xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
@ 2016-05-13 13:33 Andrew Cooper
  2016-05-13 14:13 ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2016-05-13 13:33 UTC (permalink / raw)
  To: Xen-devel
  Cc: Kevin Tian, Wei Liu, Jan Beulich, George Dunlap, Andrew Cooper,
	Tim Deegan, Jun Nakajima

hostmode->p2m_ga_to_gfn() is a plain PT walker, and is not appropriate for a
general L1 p2m walk.  It is fine for AMD as NPT share the same format as
normal pagetables.  For Intel EPT however, it is wrong.

The translation ends up correct (as the formats are sufficiently similar), but
the control bits in lower 12 bits differ in meaning.  A plain PT walker sets
A/D bits (bits 5 and 6) as it walks, but in EPT tables, these are the IPAT and
top bit of EMT (caching type).  This in turn causes problem when the EPT
tables are subsequently used.

Replace hostmode->p2m_ga_to_gfn() with nestedhap_walk_L1_p2m() in
paging_gva_to_gfn(), which is the correct function for the task.  This
involves making nestedhap_walk_L1_p2m() non-static, and adding
vmx_vmcs_enter/exit() pairs to nvmx_hap_walk_L1_p2m() as it is now reachable
from contexts other than v == current.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c         |  4 ++++
 xen/arch/x86/mm/hap/nested_hap.c    |  2 +-
 xen/arch/x86/mm/p2m.c               | 19 ++++++++++++++-----
 xen/include/asm-x86/hvm/nestedhvm.h |  4 ++++
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 35f3687..faa8b69 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2036,6 +2036,8 @@ nvmx_hap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
     uint32_t rwx_rights = (access_x << 2) | (access_w << 1) | access_r;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
 
+    vmx_vmcs_enter(v);
+
     __vmread(EXIT_QUALIFICATION, &exit_qual);
     rc = nept_translate_l2ga(v, L2_gpa, page_order, rwx_rights, &gfn, p2m_acc,
                              &exit_qual, &exit_reason);
@@ -2060,6 +2062,8 @@ nvmx_hap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
         break;
     }
 
+    vmx_vmcs_exit(v);
+
     return rc;
 }
 
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 9cee5a0..d41bb09 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
  * walk is successful, the translated value is returned in
  * L1_gpa. The result value tells what to do next.
  */
-static int
+int
 nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
                       unsigned int *page_order, uint8_t *p2m_acc,
                       bool_t access_r, bool_t access_w, bool_t access_x)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 94eabf6..de5791b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
 
     if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
     {
-        unsigned long gfn;
+        unsigned long l2_gfn, l1_gfn;
         struct p2m_domain *p2m;
         const struct paging_mode *mode;
-        uint32_t pfec_21 = *pfec;
         uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
+        uint8_t l1_p2ma;
+        unsigned int l1_page_order;
+        int rv;
 
         /* translate l2 guest va into l2 guest gfn */
         p2m = p2m_get_nestedp2m(v, np2m_base);
         mode = paging_get_nestedmode(v);
-        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
+        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
+
+        if ( l2_gfn == INVALID_GFN )
+            return INVALID_GFN;
 
         /* translate l2 guest gfn into l1 guest gfn */
-        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
-                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
+        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
+                                   1,
+                                   !!(*pfec & PFEC_write_access),
+                                   !!(*pfec & PFEC_insn_fetch));
+
+        return rv == NESTEDHVM_PAGEFAULT_DONE ? l1_gfn : INVALID_GFN;
     }
 
     return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index cf1a8f4..bc82425 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -56,6 +56,10 @@ bool_t nestedhvm_vcpu_in_guestmode(struct vcpu *v);
 int nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     bool_t access_r, bool_t access_w, bool_t access_x);
 
+int nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
+                          unsigned int *page_order, uint8_t *p2m_acc,
+                          bool_t access_r, bool_t access_w, bool_t access_x);
+
 /* IO permission map */
 unsigned long *nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed);
 
-- 
2.1.4


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 13:33 [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker Andrew Cooper
@ 2016-05-13 14:13 ` Jan Beulich
  2016-05-13 15:00   ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2016-05-13 14:13 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Kevin Tian, Wei Liu, George Dunlap, Tim Deegan, Xen-devel, Jun Nakajima

>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/mm/hap/nested_hap.c
> +++ b/xen/arch/x86/mm/hap/nested_hap.c
> @@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
>   * walk is successful, the translated value is returned in
>   * L1_gpa. The result value tells what to do next.
>   */
> -static int
> +int
>  nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
>                        unsigned int *page_order, uint8_t *p2m_acc,

The function becoming non-static widens the visibility of the bogus
uint8_t here (should be p2m_access_t afaics). Granted this isn't an
issue the patch introduces, but I would prefer this to be adjusted
prior to dropping the static here...

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>  
>      if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
>      {
> -        unsigned long gfn;
> +        unsigned long l2_gfn, l1_gfn;
>          struct p2m_domain *p2m;
>          const struct paging_mode *mode;
> -        uint32_t pfec_21 = *pfec;
>          uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
> +        uint8_t l1_p2ma;

... avoiding proliferation of the quirkiness.

> +        unsigned int l1_page_order;
> +        int rv;
>  
>          /* translate l2 guest va into l2 guest gfn */
>          p2m = p2m_get_nestedp2m(v, np2m_base);
>          mode = paging_get_nestedmode(v);
> -        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
> +        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
> +
> +        if ( l2_gfn == INVALID_GFN )
> +            return INVALID_GFN;
>  
>          /* translate l2 guest gfn into l1 guest gfn */
> -        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
> -                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
> +        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
> +                                   1,
> +                                   !!(*pfec & PFEC_write_access),
> +                                   !!(*pfec & PFEC_insn_fetch));
> +
> +        return rv == NESTEDHVM_PAGEFAULT_DONE ? l1_gfn : INVALID_GFN;
>      }

I'm really happy to see this getting cleaned up - I've stumbled across
the apparent brokenness here more than once, without ever finding
the time to help the situation. One question though: Is the return
value here correct even for l1_page_order > 0?

Jan


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 14:13 ` Jan Beulich
@ 2016-05-13 15:00   ` Andrew Cooper
  2016-05-13 15:19     ` Jan Beulich
  2016-05-13 15:24     ` [PATCH " Andrew Cooper
  0 siblings, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2016-05-13 15:00 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Kevin Tian, Wei Liu, George Dunlap, Tim Deegan, Xen-devel, Jun Nakajima

On 13/05/16 15:13, Jan Beulich wrote:
>>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>> @@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
>>   * walk is successful, the translated value is returned in
>>   * L1_gpa. The result value tells what to do next.
>>   */
>> -static int
>> +int
>>  nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
>>                        unsigned int *page_order, uint8_t *p2m_acc,
> The function becoming non-static widens the visibility of the bogus
> uint8_t here (should be p2m_access_t afaics). Granted this isn't an
> issue the patch introduces, but I would prefer this to be adjusted
> prior to dropping the static here...
>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>>  
>>      if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
>>      {
>> -        unsigned long gfn;
>> +        unsigned long l2_gfn, l1_gfn;
>>          struct p2m_domain *p2m;
>>          const struct paging_mode *mode;
>> -        uint32_t pfec_21 = *pfec;
>>          uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
>> +        uint8_t l1_p2ma;
> ... avoiding proliferation of the quirkiness.

Will do.

>
>> +        unsigned int l1_page_order;
>> +        int rv;
>>  
>>          /* translate l2 guest va into l2 guest gfn */
>>          p2m = p2m_get_nestedp2m(v, np2m_base);
>>          mode = paging_get_nestedmode(v);
>> -        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>> +        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>> +
>> +        if ( l2_gfn == INVALID_GFN )
>> +            return INVALID_GFN;
>>  
>>          /* translate l2 guest gfn into l1 guest gfn */
>> -        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
>> -                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
>> +        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
>> +                                   1,
>> +                                   !!(*pfec & PFEC_write_access),
>> +                                   !!(*pfec & PFEC_insn_fetch));
>> +
>> +        return rv == NESTEDHVM_PAGEFAULT_DONE ? l1_gfn : INVALID_GFN;
>>      }
> I'm really happy to see this getting cleaned up - I've stumbled across
> the apparent brokenness here more than once, without ever finding
> the time to help the situation. One question though: Is the return
> value here correct even for l1_page_order > 0?

Not completely sure.  This code is messy, and my current testing is "it
now doesn't crash where it used to crash before".

The specific path being tripped over was __hvm_copy() performing an
instruction fetch for emulation from the L2 guest.

Looking at the code, if your caller only cares about 4K mappings, you
will get the correct translation, due to the adjustment
nept_walk_tables() makes.

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 15:00   ` Andrew Cooper
@ 2016-05-13 15:19     ` Jan Beulich
  2016-05-13 17:25       ` [PATCH v2 " Andrew Cooper
  2016-05-13 15:24     ` [PATCH " Andrew Cooper
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2016-05-13 15:19 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Kevin Tian, Wei Liu, GeorgeDunlap, Tim Deegan, Xen-devel, Jun Nakajima

>>> On 13.05.16 at 17:00, <andrew.cooper3@citrix.com> wrote:
> On 13/05/16 15:13, Jan Beulich wrote:
>>>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
>>> +        unsigned int l1_page_order;
>>> +        int rv;
>>>  
>>>          /* translate l2 guest va into l2 guest gfn */
>>>          p2m = p2m_get_nestedp2m(v, np2m_base);
>>>          mode = paging_get_nestedmode(v);
>>> -        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>>> +        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>>> +
>>> +        if ( l2_gfn == INVALID_GFN )
>>> +            return INVALID_GFN;
>>>  
>>>          /* translate l2 guest gfn into l1 guest gfn */
>>> -        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
>>> -                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
>>> +        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
>>> +                                   1,
>>> +                                   !!(*pfec & PFEC_write_access),
>>> +                                   !!(*pfec & PFEC_insn_fetch));
>>> +
>>> +        return rv == NESTEDHVM_PAGEFAULT_DONE ? l1_gfn : INVALID_GFN;
>>>      }
>> I'm really happy to see this getting cleaned up - I've stumbled across
>> the apparent brokenness here more than once, without ever finding
>> the time to help the situation. One question though: Is the return
>> value here correct even for l1_page_order > 0?
> 
> Not completely sure.  This code is messy, and my current testing is "it
> now doesn't crash where it used to crash before".
> 
> The specific path being tripped over was __hvm_copy() performing an
> instruction fetch for emulation from the L2 guest.
> 
> Looking at the code, if your caller only cares about 4K mappings, you
> will get the correct translation, due to the adjustment
> nept_walk_tables() makes.

Ah, indeed. But very odd for it to be done there, the more that "all
callers" is exactly one afaics. In that case may I suggest adding an
assertion to check that the respective bits in L1 and L2 GFN match?

Jan


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 15:00   ` Andrew Cooper
  2016-05-13 15:19     ` Jan Beulich
@ 2016-05-13 15:24     ` Andrew Cooper
  2016-05-13 15:31       ` Jan Beulich
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2016-05-13 15:24 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Kevin Tian, Wei Liu, George Dunlap, Tim Deegan, Xen-devel, Jun Nakajima

On 13/05/16 16:00, Andrew Cooper wrote:
> On 13/05/16 15:13, Jan Beulich wrote:
>>>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>>> @@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
>>>   * walk is successful, the translated value is returned in
>>>   * L1_gpa. The result value tells what to do next.
>>>   */
>>> -static int
>>> +int
>>>  nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
>>>                        unsigned int *page_order, uint8_t *p2m_acc,
>> The function becoming non-static widens the visibility of the bogus
>> uint8_t here (should be p2m_access_t afaics). Granted this isn't an
>> issue the patch introduces, but I would prefer this to be adjusted
>> prior to dropping the static here...
>>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>>>  
>>>      if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
>>>      {
>>> -        unsigned long gfn;
>>> +        unsigned long l2_gfn, l1_gfn;
>>>          struct p2m_domain *p2m;
>>>          const struct paging_mode *mode;
>>> -        uint32_t pfec_21 = *pfec;
>>>          uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
>>> +        uint8_t l1_p2ma;
>> ... avoiding proliferation of the quirkiness.
> Will do.

It turns out that this is currently hard, due to a tangle of header files.

I will put this on my TODO list, but this point, I have more important
things to do for 4.7.

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 15:24     ` [PATCH " Andrew Cooper
@ 2016-05-13 15:31       ` Jan Beulich
  0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-05-13 15:31 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Kevin Tian, Wei Liu, GeorgeDunlap, Tim Deegan, Xen-devel, Jun Nakajima

>>> On 13.05.16 at 17:24, <andrew.cooper3@citrix.com> wrote:
> On 13/05/16 16:00, Andrew Cooper wrote:
>> On 13/05/16 15:13, Jan Beulich wrote:
>>>>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>>>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>>>> @@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain 
> *p2m,
>>>>   * walk is successful, the translated value is returned in
>>>>   * L1_gpa. The result value tells what to do next.
>>>>   */
>>>> -static int
>>>> +int
>>>>  nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
>>>>                        unsigned int *page_order, uint8_t *p2m_acc,
>>> The function becoming non-static widens the visibility of the bogus
>>> uint8_t here (should be p2m_access_t afaics). Granted this isn't an
>>> issue the patch introduces, but I would prefer this to be adjusted
>>> prior to dropping the static here...
>>>
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>>>>  
>>>>      if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
>>>>      {
>>>> -        unsigned long gfn;
>>>> +        unsigned long l2_gfn, l1_gfn;
>>>>          struct p2m_domain *p2m;
>>>>          const struct paging_mode *mode;
>>>> -        uint32_t pfec_21 = *pfec;
>>>>          uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
>>>> +        uint8_t l1_p2ma;
>>> ... avoiding proliferation of the quirkiness.
>> Will do.
> 
> It turns out that this is currently hard, due to a tangle of header files.

And I withdraw the request then.

> I will put this on my TODO list, but this point, I have more important
> things to do for 4.7.

Thanks, understood.

Jan


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 15:19     ` Jan Beulich
@ 2016-05-13 17:25       ` Andrew Cooper
  2016-05-17  6:06         ` Tian, Kevin
                           ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Andrew Cooper @ 2016-05-13 17:25 UTC (permalink / raw)
  To: Xen-devel
  Cc: Kevin Tian, Wei Liu, Jan Beulich, George Dunlap, Andrew Cooper,
	Tim Deegan, Jun Nakajima

hostmode->p2m_ga_to_gfn() is a plain PT walker, and is not appropriate for a
general L1 p2m walk.  It is fine for AMD as NPT share the same format as
normal pagetables.  For Intel EPT however, it is wrong.

The translation ends up correct (as the formats are sufficiently similar), but
the control bits in lower 12 bits differ in meaning.  A plain PT walker sets
A/D bits (bits 5 and 6) as it walks, but in EPT tables, these are the IPAT and
top bit of EMT (caching type).  This in turn causes problem when the EPT
tables are subsequently used.

Replace hostmode->p2m_ga_to_gfn() with nestedhap_walk_L1_p2m() in
paging_gva_to_gfn(), which is the correct function for the task.  This
involves making nestedhap_walk_L1_p2m() non-static, and adding
vmx_vmcs_enter/exit() pairs to nvmx_hap_walk_L1_p2m() as it is now reachable
from contexts other than v == current.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>

v2:
 * ASSERT() that the translation doesn't go wrong with superpages
---
 xen/arch/x86/hvm/vmx/vvmx.c         |  4 ++++
 xen/arch/x86/mm/hap/nested_hap.c    |  2 +-
 xen/arch/x86/mm/p2m.c               | 29 ++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/nestedhvm.h |  4 ++++
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 35f3687..faa8b69 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2036,6 +2036,8 @@ nvmx_hap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
     uint32_t rwx_rights = (access_x << 2) | (access_w << 1) | access_r;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
 
+    vmx_vmcs_enter(v);
+
     __vmread(EXIT_QUALIFICATION, &exit_qual);
     rc = nept_translate_l2ga(v, L2_gpa, page_order, rwx_rights, &gfn, p2m_acc,
                              &exit_qual, &exit_reason);
@@ -2060,6 +2062,8 @@ nvmx_hap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
         break;
     }
 
+    vmx_vmcs_exit(v);
+
     return rc;
 }
 
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 9cee5a0..d41bb09 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
  * walk is successful, the translated value is returned in
  * L1_gpa. The result value tells what to do next.
  */
-static int
+int
 nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
                       unsigned int *page_order, uint8_t *p2m_acc,
                       bool_t access_r, bool_t access_w, bool_t access_x)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 94eabf6..9b19769 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2081,20 +2081,39 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
 
     if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
     {
-        unsigned long gfn;
+        unsigned long l2_gfn, l1_gfn;
         struct p2m_domain *p2m;
         const struct paging_mode *mode;
-        uint32_t pfec_21 = *pfec;
         uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
+        uint8_t l1_p2ma;
+        unsigned int l1_page_order;
+        int rv;
 
         /* translate l2 guest va into l2 guest gfn */
         p2m = p2m_get_nestedp2m(v, np2m_base);
         mode = paging_get_nestedmode(v);
-        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
+        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
+
+        if ( l2_gfn == INVALID_GFN )
+            return INVALID_GFN;
 
         /* translate l2 guest gfn into l1 guest gfn */
-        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
-                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
+        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
+                                   1,
+                                   !!(*pfec & PFEC_write_access),
+                                   !!(*pfec & PFEC_insn_fetch));
+
+        if ( rv != NESTEDHVM_PAGEFAULT_DONE )
+            return INVALID_GFN;
+
+        /*
+         * Sanity check that l1_gfn can be used properly as a 4K mapping, even
+         * if it mapped by a nested superpage.
+         */
+        ASSERT((l2_gfn & ((1ul << l1_page_order) - 1)) ==
+               (l1_gfn & ((1ul << l1_page_order) - 1)));
+
+        return l1_gfn;
     }
 
     return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index cf1a8f4..bc82425 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -56,6 +56,10 @@ bool_t nestedhvm_vcpu_in_guestmode(struct vcpu *v);
 int nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     bool_t access_r, bool_t access_w, bool_t access_x);
 
+int nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
+                          unsigned int *page_order, uint8_t *p2m_acc,
+                          bool_t access_r, bool_t access_w, bool_t access_x);
+
 /* IO permission map */
 unsigned long *nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed);
 
-- 
2.1.4


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 17:25       ` [PATCH v2 " Andrew Cooper
@ 2016-05-17  6:06         ` Tian, Kevin
  2016-05-18 13:36         ` George Dunlap
  2016-05-18 13:50         ` Wei Liu
  2 siblings, 0 replies; 10+ messages in thread
From: Tian, Kevin @ 2016-05-17  6:06 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: George Dunlap, Wei Liu, Tim Deegan, Nakajima, Jun, Jan Beulich

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, May 14, 2016 1:26 AM
> 
> hostmode->p2m_ga_to_gfn() is a plain PT walker, and is not appropriate for a
> general L1 p2m walk.  It is fine for AMD as NPT share the same format as
> normal pagetables.  For Intel EPT however, it is wrong.
> 
> The translation ends up correct (as the formats are sufficiently similar), but
> the control bits in lower 12 bits differ in meaning.  A plain PT walker sets
> A/D bits (bits 5 and 6) as it walks, but in EPT tables, these are the IPAT and
> top bit of EMT (caching type).  This in turn causes problem when the EPT
> tables are subsequently used.
> 
> Replace hostmode->p2m_ga_to_gfn() with nestedhap_walk_L1_p2m() in
> paging_gva_to_gfn(), which is the correct function for the task.  This
> involves making nestedhap_walk_L1_p2m() non-static, and adding
> vmx_vmcs_enter/exit() pairs to nvmx_hap_walk_L1_p2m() as it is now reachable
> from contexts other than v == current.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 17:25       ` [PATCH v2 " Andrew Cooper
  2016-05-17  6:06         ` Tian, Kevin
@ 2016-05-18 13:36         ` George Dunlap
  2016-05-18 13:50         ` Wei Liu
  2 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2016-05-18 13:36 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Kevin Tian, Wei Liu, Jan Beulich, Tim Deegan, Xen-devel, Jun Nakajima

On Fri, May 13, 2016 at 6:25 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> hostmode->p2m_ga_to_gfn() is a plain PT walker, and is not appropriate for a
> general L1 p2m walk.  It is fine for AMD as NPT share the same format as
> normal pagetables.  For Intel EPT however, it is wrong.
>
> The translation ends up correct (as the formats are sufficiently similar), but
> the control bits in lower 12 bits differ in meaning.  A plain PT walker sets
> A/D bits (bits 5 and 6) as it walks, but in EPT tables, these are the IPAT and
> top bit of EMT (caching type).  This in turn causes problem when the EPT
> tables are subsequently used.
>
> Replace hostmode->p2m_ga_to_gfn() with nestedhap_walk_L1_p2m() in
> paging_gva_to_gfn(), which is the correct function for the task.  This
> involves making nestedhap_walk_L1_p2m() non-static, and adding
> vmx_vmcs_enter/exit() pairs to nvmx_hap_walk_L1_p2m() as it is now reachable
> from contexts other than v == current.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker
  2016-05-13 17:25       ` [PATCH v2 " Andrew Cooper
  2016-05-17  6:06         ` Tian, Kevin
  2016-05-18 13:36         ` George Dunlap
@ 2016-05-18 13:50         ` Wei Liu
  2 siblings, 0 replies; 10+ messages in thread
From: Wei Liu @ 2016-05-18 13:50 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Kevin Tian, Wei Liu, Jan Beulich, George Dunlap, Tim Deegan,
	Xen-devel, Jun Nakajima

On Fri, May 13, 2016 at 06:25:41PM +0100, Andrew Cooper wrote:
> hostmode->p2m_ga_to_gfn() is a plain PT walker, and is not appropriate for a
> general L1 p2m walk.  It is fine for AMD as NPT share the same format as
> normal pagetables.  For Intel EPT however, it is wrong.
> 
> The translation ends up correct (as the formats are sufficiently similar), but
> the control bits in lower 12 bits differ in meaning.  A plain PT walker sets
> A/D bits (bits 5 and 6) as it walks, but in EPT tables, these are the IPAT and
> top bit of EMT (caching type).  This in turn causes problem when the EPT
> tables are subsequently used.
> 
> Replace hostmode->p2m_ga_to_gfn() with nestedhap_walk_L1_p2m() in
> paging_gva_to_gfn(), which is the correct function for the task.  This
> involves making nestedhap_walk_L1_p2m() non-static, and adding
> vmx_vmcs_enter/exit() pairs to nvmx_hap_walk_L1_p2m() as it is now reachable
> from contexts other than v == current.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-05-18 13:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-13 13:33 [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker Andrew Cooper
2016-05-13 14:13 ` Jan Beulich
2016-05-13 15:00   ` Andrew Cooper
2016-05-13 15:19     ` Jan Beulich
2016-05-13 17:25       ` [PATCH v2 " Andrew Cooper
2016-05-17  6:06         ` Tian, Kevin
2016-05-18 13:36         ` George Dunlap
2016-05-18 13:50         ` Wei Liu
2016-05-13 15:24     ` [PATCH " Andrew Cooper
2016-05-13 15:31       ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).