[PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Andrew Cooper]

XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
lower than the real maxphysaddr, to avoid overflowing the superpage shadow
backpointer.

However, plenty of hardware has a physical address width less that 44 bits,
and the code added in shadow_domain_init() is a straight asignment.  This
causes gfn_bits to be increased beyond the physical address width on most
Intel consumer hardware (which has a width of 39).

It also means that the effective maxphysaddr for shadowed guests differs from
the value reported to the guest in CPUID.  This means that a guest can create
PTEs which either should fault but don't, or shouldn't fault but do.

Remove gfn_bits and rework its logic in terms of a guests maxphysaddr.
recalculate_cpuid_policy() is updated to properly clamp maxphysaddr between
the host maximum, a possibly-smaller shadow restriction, and 32 as a minimum.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/cpuid.c            | 20 ++++++++++++++------
 xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
 xen/arch/x86/mm/guest_walk.c    |  3 ++-
 xen/arch/x86/mm/hap/hap.c       |  2 --
 xen/arch/x86/mm/p2m.c           |  3 ++-
 xen/arch/x86/mm/shadow/common.c | 10 ----------
 xen/arch/x86/mm/shadow/multi.c  |  3 ++-
 xen/include/asm-x86/domain.h    |  3 ---
 8 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e0a387e..02a3451 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -1,4 +1,5 @@
 #include <xen/init.h>
+#include <xen/kconfig.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <asm/cpuid.h>
@@ -6,6 +7,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/vmx/vmcs.h>
+#include <asm/paging.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
 
@@ -424,7 +426,7 @@ void recalculate_cpuid_policy(struct domain *d)
     const struct cpuid_policy *max =
         is_pv_domain(d) ? &pv_max_policy : &hvm_max_policy;
     uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
-    unsigned int i;
+    unsigned int i, maxphysaddr_limit;
 
     p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
                                    p->basic.vendor_edx, gcv_guest);
@@ -502,11 +504,17 @@ void recalculate_cpuid_policy(struct domain *d)
 
     cpuid_featureset_to_policy(fs, p);
 
-    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
-    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
-                                d->arch.paging.gfn_bits + PAGE_SHIFT);
-    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
-                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
+    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
+
+    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
+         (!is_pv_domain(d) || opt_allow_superpage) )
+    {
+        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
+        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);
+    }
+
+    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, maxphysaddr_limit);
+    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
 
     p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5acb88a..4ea8a6b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1380,7 +1380,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     }
 
     if ( (gpa & ~PAGE_MASK) ||
-         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
+         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index a67fd5a..5ad8cf6 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
     /* If this guest has a restricted physical address space then the
      * target GFN must fit within it. */
     if ( !(rc & _PAGE_PRESENT)
-         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
+         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
         rc |= _PAGE_INVALID_BITS;
 
     return rc;
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 6dbb3cc..928cd5e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -447,8 +447,6 @@ void hap_domain_init(struct domain *d)
 {
     INIT_PAGE_LIST_HEAD(&d->arch.paging.hap.freelist);
 
-    d->arch.paging.gfn_bits = hap_paddr_bits - PAGE_SHIFT;
-
     /* Use HAP logdirty mechanism. */
     paging_log_dirty_init(d, hap_enable_log_dirty,
                           hap_disable_log_dirty,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6548e9f..fc706c6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1790,7 +1790,8 @@ void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
 {
     struct page_info *page;
 
-    if ( gfn_x(gfn) >> p2m->domain->arch.paging.gfn_bits )
+    if ( gfn_x(gfn) >>
+         (p2m->domain->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *rc = _PAGE_INVALID_BIT;
         return NULL;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index a619d65..2235a0a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -52,16 +52,6 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.freelist);
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
 
-    d->arch.paging.gfn_bits = paddr_bits - PAGE_SHIFT;
-#ifndef CONFIG_BIGMEM
-    /*
-     * Shadowed superpages store GFNs in 32-bit page_info fields.
-     * Note that we cannot use guest_supports_superpages() here.
-     */
-    if ( !is_pv_domain(d) || opt_allow_superpage )
-        d->arch.paging.gfn_bits = 32;
-#endif
-
     /* Use shadow pagetables for log-dirty support */
     paging_log_dirty_init(d, sh_enable_log_dirty,
                           sh_disable_log_dirty, sh_clean_dirty_bitmap);
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index d4090d7..e951daf 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -537,7 +537,8 @@ _sh_propagate(struct vcpu *v,
 
     /* Check there's something for the shadows to map to */
     if ( (!p2m_is_valid(p2mt) && !p2m_is_grant(p2mt))
-         || gfn_x(target_gfn) >> d->arch.paging.gfn_bits )
+         || gfn_x(target_gfn) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *sp = shadow_l1e_empty();
         goto done;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index e6c7e13..2270e96 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -195,9 +195,6 @@ struct paging_domain {
     /* log dirty support */
     struct log_dirty_domain log_dirty;
 
-    /* Number of valid bits in a gfn. */
-    unsigned int gfn_bits;
-
     /* preemption handling */
     struct {
         const struct domain *dom;
-- 
2.1.4


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

Re: [PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Tim Deegan]

Hi,

At 12:36 +0000 on 08 Feb (1486557378), Andrew Cooper wrote:
> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
> backpointer.
> 
> However, plenty of hardware has a physical address width less that 44 bits,
> and the code added in shadow_domain_init() is a straight asignment.  This
> causes gfn_bits to be increased beyond the physical address width on most
> Intel consumer hardware (which has a width of 39).

Once again, I think this is actually OK.  But cpuid and shadow not
agreeing on the limit is a bug and I'm happy for it to be resolved
this way.

> It also means that the effective maxphysaddr for shadowed guests differs from
> the value reported to the guest in CPUID.  This means that a guest can create
> PTEs which either should fault but don't, or shouldn't fault but do.

Can it really create a PTE that shouldn't fault but does?  AFAICS the
cpuid policy can report a lower value than 44 but not a higher one.

> Remove gfn_bits and rework its logic in terms of a guests maxphysaddr.
> recalculate_cpuid_policy() is updated to properly clamp maxphysaddr between
> the host maximum, a possibly-smaller shadow restriction, and 32 as a minimum.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

> @@ -502,11 +504,17 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      cpuid_featureset_to_policy(fs, p);
>  
> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
> -    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
> +    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> +
> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> +         (!is_pv_domain(d) || opt_allow_superpage) )
> +    {
> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> +        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);

I don't like this implementation detail escaping from x86/mm/shadow;
can we have a cpuid_limit_paddr_bits(d) that the shadow code can call
to tell cpuid about this?  Or perhaps a paging_max_paddr_bits(d) so
cpuid can ask paging when it needs to know?

In either case I wonder whether there needs to be some trigger of
recalculate_cpuid_policy() once shadow mode is enabled -- I guess
currently it relies on it being called by something late enough in hvm
domain creation?

> +    }
> +
> +    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, maxphysaddr_limit);
> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);

No longer requiring 36 bits for pae/pse36?  This is mentioned in the
description but not explained.

The rest of this looks fine to me.

Cheers,

Tim.

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

Re: [PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Andrew Cooper]

On 08/02/17 14:12, Tim Deegan wrote:
> Hi,
>
> At 12:36 +0000 on 08 Feb (1486557378), Andrew Cooper wrote:
>> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
>> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
>> backpointer.
>>
>> However, plenty of hardware has a physical address width less that 44 bits,
>> and the code added in shadow_domain_init() is a straight asignment.  This
>> causes gfn_bits to be increased beyond the physical address width on most
>> Intel consumer hardware (which has a width of 39).
> Once again, I think this is actually OK.  But cpuid and shadow not
> agreeing on the limit is a bug and I'm happy for it to be resolved
> this way.
>
>> It also means that the effective maxphysaddr for shadowed guests differs from
>> the value reported to the guest in CPUID.  This means that a guest can create
>> PTEs which either should fault but don't, or shouldn't fault but do.
> Can it really create a PTE that shouldn't fault but does?  AFAICS the
> cpuid policy can report a lower value than 44 but not a higher one.

Most Intel hardware has a width of 39.
Most AMD hardware has a width of 48.

In both cases with this change, CPUID reports these values, but the
shadow code limits at 44 bits.

On Intel, a guest can get an PTE shadowed which should have faulted and
didn't (because the smfn is actually within 39 bits), while on AMD, a
guest can try to create a PTE which shouldn't fault (because CPUID says
anything up to 48 bits is fine) yet does fault because the shadow code
rejects anything above 44 bits.

>
>> Remove gfn_bits and rework its logic in terms of a guests maxphysaddr.
>> recalculate_cpuid_policy() is updated to properly clamp maxphysaddr between
>> the host maximum, a possibly-smaller shadow restriction, and 32 as a minimum.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> @@ -502,11 +504,17 @@ void recalculate_cpuid_policy(struct domain *d)
>>  
>>      cpuid_featureset_to_policy(fs, p);
>>  
>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>> -    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>> +    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>> +
>> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
>> +         (!is_pv_domain(d) || opt_allow_superpage) )
>> +    {
>> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
>> +        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);
> I don't like this implementation detail escaping from x86/mm/shadow;
> can we have a cpuid_limit_paddr_bits(d) that the shadow code can call
> to tell cpuid about this?  Or perhaps a paging_max_paddr_bits(d) so
> cpuid can ask paging when it needs to know?

I will see about doing this.

> In either case I wonder whether there needs to be some trigger of
> recalculate_cpuid_policy() once shadow mode is enabled -- I guess
> currently it relies on it being called by something late enough in hvm
> domain creation?

Yes, although now I am worried about migrating PV guests.  I will double
check to see whether that path works sensibly or not, because we can't
have maxphysaddr suddenly shrink under the feet of a running guest.

>
>> +    }
>> +
>> +    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, maxphysaddr_limit);
>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
> No longer requiring 36 bits for pae/pse36?  This is mentioned in the
> description but not explained.

32/36 is only the default to be assumed if no maxphysaddr is provided. 
We always provide maxphysaddr, and passing 32 on a massive machine is
legitimate.

I don't think we can reasonably get away with offering a VM less than 32
(which is why the check exists), but it is perfectly reasonable to give
the VM 34 if we tell it that it only has 16GB of physical address space
to use.

~Andrew

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

Re: [PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Tim Deegan]

At 15:29 +0000 on 08 Feb (1486567798), Andrew Cooper wrote:
> On Intel, a guest can get an PTE shadowed which should have faulted and
> didn't (because the smfn is actually within 39 bits)

Yes.

, while on AMD, a
> guest can try to create a PTE which shouldn't fault (because CPUID says
> anything up to 48 bits is fine) yet does fault because the shadow code
> rejects anything above 44 bits.

But in the code you're replacing (just below) the CPUID value is
capped at the paging.gfn_bits+PAGE_SHIFT, so won't it report 44?

> >> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
> >> -    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> >> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> >> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
> >> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
> >> +    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> >> +
> >> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> >> +         (!is_pv_domain(d) || opt_allow_superpage) )
> >> +    {
> >> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> >> +        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);
> > I don't like this implementation detail escaping from x86/mm/shadow;
> > can we have a cpuid_limit_paddr_bits(d) that the shadow code can call
> > to tell cpuid about this?  Or perhaps a paging_max_paddr_bits(d) so
> > cpuid can ask paging when it needs to know?
> 
> I will see about doing this.
> 
> > In either case I wonder whether there needs to be some trigger of
> > recalculate_cpuid_policy() once shadow mode is enabled -- I guess
> > currently it relies on it being called by something late enough in hvm
> > domain creation?
> 
> Yes, although now I am worried about migrating PV guests.  I will double
> check to see whether that path works sensibly or not, because we can't
> have maxphysaddr suddenly shrink under the feet of a running guest.

We're lucky here: the shadow width restriction doesn't apply to PV
guests.

> >> +    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, maxphysaddr_limit);
> >> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
> > No longer requiring 36 bits for pae/pse36?  This is mentioned in the
> > description but not explained.
> 
> 32/36 is only the default to be assumed if no maxphysaddr is provided. 
> We always provide maxphysaddr, and passing 32 on a massive machine is
> legitimate.

OK.

Tim.

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

[PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Andrew Cooper]

XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
lower than the real maxphysaddr, to avoid overflowing the superpage shadow
backpointer.

However, plenty of hardware has a physical address width less that 44 bits,
and the code added in shadow_domain_init() is a straight assignment.  This
causes gfn_bits to be increased beyond the physical address width on most
Intel consumer hardware (typically a width of 39, which is the number reported
to the guest via CPUID).

If the guest intentionally creates a PTE referencing a physical address
between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
address.  However, the shadow code accepts the PTE, shadows it, and the
virtual address works normally.

Introduce paging_max_paddr_bits() to calculate the largest guest physical
address supportable by the paging infrastructure, and update
recalculate_cpuid_policy() to take this into account when clamping the guests
cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
guests maxphysaddr.

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

v2:
 * Introduce paging_max_paddr_bits() rather than moving paging logic into
   recalculate_cpuid_policy().
 * Rewrite half of the commit message.
---
 xen/arch/x86/cpuid.c            |  7 +++----
 xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
 xen/arch/x86/mm/guest_walk.c    |  3 ++-
 xen/arch/x86/mm/hap/hap.c       |  2 --
 xen/arch/x86/mm/p2m.c           |  3 ++-
 xen/arch/x86/mm/shadow/common.c | 10 ----------
 xen/arch/x86/mm/shadow/multi.c  |  3 ++-
 xen/include/asm-x86/domain.h    |  3 ---
 xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
 9 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e0a387e..3378f7a 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -6,6 +6,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/vmx/vmcs.h>
+#include <asm/paging.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
 
@@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
 
     cpuid_featureset_to_policy(fs, p);
 
-    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
     p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
-                                d->arch.paging.gfn_bits + PAGE_SHIFT);
-    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
-                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
+                                paging_max_paddr_bits(d));
+    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
 
     p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9c61b5b..774a11f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     }
 
     if ( (gpa & ~PAGE_MASK) ||
-         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
+         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index a67fd5a..5ad8cf6 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
     /* If this guest has a restricted physical address space then the
      * target GFN must fit within it. */
     if ( !(rc & _PAGE_PRESENT)
-         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
+         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
         rc |= _PAGE_INVALID_BITS;
 
     return rc;
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 6dbb3cc..928cd5e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -447,8 +447,6 @@ void hap_domain_init(struct domain *d)
 {
     INIT_PAGE_LIST_HEAD(&d->arch.paging.hap.freelist);
 
-    d->arch.paging.gfn_bits = hap_paddr_bits - PAGE_SHIFT;
-
     /* Use HAP logdirty mechanism. */
     paging_log_dirty_init(d, hap_enable_log_dirty,
                           hap_disable_log_dirty,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index db33153..69c69c7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1783,7 +1783,8 @@ void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
 {
     struct page_info *page;
 
-    if ( gfn_x(gfn) >> p2m->domain->arch.paging.gfn_bits )
+    if ( gfn_x(gfn) >>
+         (p2m->domain->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *rc = _PAGE_INVALID_BIT;
         return NULL;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index a619d65..2235a0a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -52,16 +52,6 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.freelist);
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
 
-    d->arch.paging.gfn_bits = paddr_bits - PAGE_SHIFT;
-#ifndef CONFIG_BIGMEM
-    /*
-     * Shadowed superpages store GFNs in 32-bit page_info fields.
-     * Note that we cannot use guest_supports_superpages() here.
-     */
-    if ( !is_pv_domain(d) || opt_allow_superpage )
-        d->arch.paging.gfn_bits = 32;
-#endif
-
     /* Use shadow pagetables for log-dirty support */
     paging_log_dirty_init(d, sh_enable_log_dirty,
                           sh_disable_log_dirty, sh_clean_dirty_bitmap);
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index d4090d7..e951daf 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -537,7 +537,8 @@ _sh_propagate(struct vcpu *v,
 
     /* Check there's something for the shadows to map to */
     if ( (!p2m_is_valid(p2mt) && !p2m_is_grant(p2mt))
-         || gfn_x(target_gfn) >> d->arch.paging.gfn_bits )
+         || gfn_x(target_gfn) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *sp = shadow_l1e_empty();
         goto done;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index e6c7e13..2270e96 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -195,9 +195,6 @@ struct paging_domain {
     /* log dirty support */
     struct log_dirty_domain log_dirty;
 
-    /* Number of valid bits in a gfn. */
-    unsigned int gfn_bits;
-
     /* preemption handling */
     struct {
         const struct domain *dom;
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index cec6bfd..5c2df8a 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -25,6 +25,7 @@
 #define _XEN_PAGING_H
 
 #include <xen/mm.h>
+#include <xen/kconfig.h>
 #include <public/domctl.h>
 #include <xen/sched.h>
 #include <xen/perfc.h>
@@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
 int paging_set_allocation(struct domain *d, unsigned int pages,
                           bool *preempted);
 
+/* Maxphysaddr supportable by the paging infrastructure. */
+static inline unsigned int paging_max_paddr_bits(const struct domain *d)
+{
+    unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
+
+    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
+         (!is_pv_domain(d) || opt_allow_superpage) )
+    {
+        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
+        bits = min(bits, 32U + PAGE_SHIFT);
+    }
+
+    return bits;
+}
+
 #endif /* XEN_PAGING_H */
 
 /*
-- 
2.1.4


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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Jan Beulich]

>>> On 13.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote:
> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      cpuid_featureset_to_policy(fs, p);
>  
> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
> +                                paging_max_paddr_bits(d));
> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);

The re-write of the commit message doesn't appear to have
resulted in the PAE/PSE36 part being explained any better. I'm
also unconvinced that exposing namely PSE36 to a guest
with less than 36 physical address bits would be compatible with
old OSes not knowing of CPUID leaf 0x80000008 yet - they
would legitimately assume 36 bits. The same presumably also
goes for PAE.

> @@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
>  int paging_set_allocation(struct domain *d, unsigned int pages,
>                            bool *preempted);
>  
> +/* Maxphysaddr supportable by the paging infrastructure. */
> +static inline unsigned int paging_max_paddr_bits(const struct domain *d)
> +{
> +    unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> +
> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> +         (!is_pv_domain(d) || opt_allow_superpage) )
> +    {
> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> +        bits = min(bits, 32U + PAGE_SHIFT);
> +    }
> +
> +    return bits;
> +}

Does this really need to be an inline function? With the overall goal
of not including every header everywhere, I particularly dislike the
need to include xen/kconfig.h here for things to build.

Jan


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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Andrew Cooper]

On 13/02/17 12:36, Jan Beulich wrote:
>>>> On 13.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>  
>>      cpuid_featureset_to_policy(fs, p);
>>  
>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>> +                                paging_max_paddr_bits(d));
>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
> The re-write of the commit message doesn't appear to have
> resulted in the PAE/PSE36 part being explained any better. I'm
> also unconvinced that exposing namely PSE36 to a guest
> with less than 36 physical address bits would be compatible with
> old OSes not knowing of CPUID leaf 0x80000008 yet - they
> would legitimately assume 36 bits. The same presumably also
> goes for PAE.

Hmm ok.  With the other bugfix of not dropping the first line, this hunk
is now simply:

@@ -504,7 +505,7 @@ void recalculate_cpuid_policy(struct domain *d)
 
     p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
     p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
-                                d->arch.paging.gfn_bits + PAGE_SHIFT);
+                                paging_max_paddr_bits(d));
     p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
                                 (p->basic.pae || p->basic.pse36) ? 36 :
32);
 

>
>> @@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
>>  int paging_set_allocation(struct domain *d, unsigned int pages,
>>                            bool *preempted);
>>  
>> +/* Maxphysaddr supportable by the paging infrastructure. */
>> +static inline unsigned int paging_max_paddr_bits(const struct domain *d)
>> +{
>> +    unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>> +
>> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
>> +         (!is_pv_domain(d) || opt_allow_superpage) )
>> +    {
>> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
>> +        bits = min(bits, 32U + PAGE_SHIFT);
>> +    }
>> +
>> +    return bits;
>> +}
> Does this really need to be an inline function? With the overall goal
> of not including every header everywhere, I particularly dislike the
> need to include xen/kconfig.h here for things to build.

It is not on a critically path, but it seems wasteful to force something
this small to be out of line.

As for kconfig.h, it is tiny.  What is the problem with adding it here? 
We already pull generated/autoconf.h into everything via xen/config.h

~Andrew

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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Tim Deegan]

At 16:04 +0000 on 14 Feb (1487088291), Andrew Cooper wrote:
> Hmm ok.  With the other bugfix of not dropping the first line, this hunk
> is now simply:
> 
> @@ -504,7 +505,7 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> +                                paging_max_paddr_bits(d));
>      p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>                                  (p->basic.pae || p->basic.pse36) ? 36 :
> 32);

That looks good to me.  Reviewed-by: Tim Deegan <tim@xen.org>

> >> @@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
> >>  int paging_set_allocation(struct domain *d, unsigned int pages,
> >>                            bool *preempted);
> >>  
> >> +/* Maxphysaddr supportable by the paging infrastructure. */
> >> +static inline unsigned int paging_max_paddr_bits(const struct domain *d)
> >> +{
> >> +    unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> >> +
> >> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> >> +         (!is_pv_domain(d) || opt_allow_superpage) )
> >> +    {
> >> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> >> +        bits = min(bits, 32U + PAGE_SHIFT);
> >> +    }
> >> +
> >> +    return bits;
> >> +}
> > Does this really need to be an inline function? With the overall goal
> > of not including every header everywhere, I particularly dislike the
> > need to include xen/kconfig.h here for things to build.
> 
> It is not on a critically path, but it seems wasteful to force something
> this small to be out of line.

It could be a macro, too.  FWIW I agree with you that a static inline
is best.

Cheers,

Tim.

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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[George Dunlap]

On 13/02/17 11:00, Andrew Cooper wrote:
> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
> backpointer.
> 
> However, plenty of hardware has a physical address width less that 44 bits,
> and the code added in shadow_domain_init() is a straight assignment.  This
> causes gfn_bits to be increased beyond the physical address width on most
> Intel consumer hardware (typically a width of 39, which is the number reported
> to the guest via CPUID).
> 
> If the guest intentionally creates a PTE referencing a physical address
> between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
> address.  However, the shadow code accepts the PTE, shadows it, and the
> virtual address works normally.
> 
> Introduce paging_max_paddr_bits() to calculate the largest guest physical
> address supportable by the paging infrastructure, and update
> recalculate_cpuid_policy() to take this into account when clamping the guests
> cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
> guests maxphysaddr.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> 
> v2:
>  * Introduce paging_max_paddr_bits() rather than moving paging logic into
>    recalculate_cpuid_policy().
>  * Rewrite half of the commit message.
> ---
>  xen/arch/x86/cpuid.c            |  7 +++----
>  xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
>  xen/arch/x86/mm/guest_walk.c    |  3 ++-
>  xen/arch/x86/mm/hap/hap.c       |  2 --
>  xen/arch/x86/mm/p2m.c           |  3 ++-
>  xen/arch/x86/mm/shadow/common.c | 10 ----------
>  xen/arch/x86/mm/shadow/multi.c  |  3 ++-
>  xen/include/asm-x86/domain.h    |  3 ---
>  xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
>  9 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index e0a387e..3378f7a 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -6,6 +6,7 @@
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/svm/svm.h>
>  #include <asm/hvm/vmx/vmcs.h>
> +#include <asm/paging.h>
>  #include <asm/processor.h>
>  #include <asm/xstate.h>
>  
> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>  
>      cpuid_featureset_to_policy(fs, p);
>  
> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
> +                                paging_max_paddr_bits(d));
> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
>  
>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>  
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 9c61b5b..774a11f 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>      }
>  
>      if ( (gpa & ~PAGE_MASK) ||
> -         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
> +         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
>      {
>          vmfail_invalid(regs);
>          return X86EMUL_OKAY;
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index a67fd5a..5ad8cf6 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      /* If this guest has a restricted physical address space then the
>       * target GFN must fit within it. */
>      if ( !(rc & _PAGE_PRESENT)
> -         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
> +         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
> +         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )

This pattern, of taking a gfn and shifting it by
(cpuid->ectd.maxphysaddr-PAGE_SHIFT) to see if it's valid happens
several times; it seems like for both clarity and avoiding mistakes, it
would be better if it were put into a macro.

Everything else looks good to me.  (No opinion on the other questions
raised so far.)

 -George


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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Andrew Cooper]

On 14/02/17 17:42, George Dunlap wrote:
> On 13/02/17 11:00, Andrew Cooper wrote:
>> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
>> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
>> backpointer.
>>
>> However, plenty of hardware has a physical address width less that 44 bits,
>> and the code added in shadow_domain_init() is a straight assignment.  This
>> causes gfn_bits to be increased beyond the physical address width on most
>> Intel consumer hardware (typically a width of 39, which is the number reported
>> to the guest via CPUID).
>>
>> If the guest intentionally creates a PTE referencing a physical address
>> between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
>> address.  However, the shadow code accepts the PTE, shadows it, and the
>> virtual address works normally.
>>
>> Introduce paging_max_paddr_bits() to calculate the largest guest physical
>> address supportable by the paging infrastructure, and update
>> recalculate_cpuid_policy() to take this into account when clamping the guests
>> cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
>> guests maxphysaddr.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> v2:
>>  * Introduce paging_max_paddr_bits() rather than moving paging logic into
>>    recalculate_cpuid_policy().
>>  * Rewrite half of the commit message.
>> ---
>>  xen/arch/x86/cpuid.c            |  7 +++----
>>  xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
>>  xen/arch/x86/mm/guest_walk.c    |  3 ++-
>>  xen/arch/x86/mm/hap/hap.c       |  2 --
>>  xen/arch/x86/mm/p2m.c           |  3 ++-
>>  xen/arch/x86/mm/shadow/common.c | 10 ----------
>>  xen/arch/x86/mm/shadow/multi.c  |  3 ++-
>>  xen/include/asm-x86/domain.h    |  3 ---
>>  xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
>>  9 files changed, 26 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>> index e0a387e..3378f7a 100644
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -6,6 +6,7 @@
>>  #include <asm/hvm/nestedhvm.h>
>>  #include <asm/hvm/svm/svm.h>
>>  #include <asm/hvm/vmx/vmcs.h>
>> +#include <asm/paging.h>
>>  #include <asm/processor.h>
>>  #include <asm/xstate.h>
>>  
>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>  
>>      cpuid_featureset_to_policy(fs, p);
>>  
>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>> +                                paging_max_paddr_bits(d));
>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
>>  
>>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>>  
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>> index 9c61b5b..774a11f 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>      }
>>  
>>      if ( (gpa & ~PAGE_MASK) ||
>> -         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
>> +         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
>>      {
>>          vmfail_invalid(regs);
>>          return X86EMUL_OKAY;
>> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
>> index a67fd5a..5ad8cf6 100644
>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>>      /* If this guest has a restricted physical address space then the
>>       * target GFN must fit within it. */
>>      if ( !(rc & _PAGE_PRESENT)
>> -         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
>> +         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
>> +         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
> This pattern, of taking a gfn and shifting it by
> (cpuid->ectd.maxphysaddr-PAGE_SHIFT) to see if it's valid happens
> several times; it seems like for both clarity and avoiding mistakes, it
> would be better if it were put into a macro.
>
> Everything else looks good to me.  (No opinion on the other questions
> raised so far.)

static inline unsigned int gfn_bits(const struct domain *d)
{
    return d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT;
}

?

I do like that idea.  It would certainly make all of the callsites more
readable.

I can happily fold that change in if others agree.

~Andrew

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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[George Dunlap]

On 14/02/17 17:45, Andrew Cooper wrote:
> On 14/02/17 17:42, George Dunlap wrote:
>> On 13/02/17 11:00, Andrew Cooper wrote:
>>> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
>>> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
>>> backpointer.
>>>
>>> However, plenty of hardware has a physical address width less that 44 bits,
>>> and the code added in shadow_domain_init() is a straight assignment.  This
>>> causes gfn_bits to be increased beyond the physical address width on most
>>> Intel consumer hardware (typically a width of 39, which is the number reported
>>> to the guest via CPUID).
>>>
>>> If the guest intentionally creates a PTE referencing a physical address
>>> between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
>>> address.  However, the shadow code accepts the PTE, shadows it, and the
>>> virtual address works normally.
>>>
>>> Introduce paging_max_paddr_bits() to calculate the largest guest physical
>>> address supportable by the paging infrastructure, and update
>>> recalculate_cpuid_policy() to take this into account when clamping the guests
>>> cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
>>> guests maxphysaddr.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>
>>> v2:
>>>  * Introduce paging_max_paddr_bits() rather than moving paging logic into
>>>    recalculate_cpuid_policy().
>>>  * Rewrite half of the commit message.
>>> ---
>>>  xen/arch/x86/cpuid.c            |  7 +++----
>>>  xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
>>>  xen/arch/x86/mm/guest_walk.c    |  3 ++-
>>>  xen/arch/x86/mm/hap/hap.c       |  2 --
>>>  xen/arch/x86/mm/p2m.c           |  3 ++-
>>>  xen/arch/x86/mm/shadow/common.c | 10 ----------
>>>  xen/arch/x86/mm/shadow/multi.c  |  3 ++-
>>>  xen/include/asm-x86/domain.h    |  3 ---
>>>  xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
>>>  9 files changed, 26 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>>> index e0a387e..3378f7a 100644
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -6,6 +6,7 @@
>>>  #include <asm/hvm/nestedhvm.h>
>>>  #include <asm/hvm/svm/svm.h>
>>>  #include <asm/hvm/vmx/vmcs.h>
>>> +#include <asm/paging.h>
>>>  #include <asm/processor.h>
>>>  #include <asm/xstate.h>
>>>  
>>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>>  
>>>      cpuid_featureset_to_policy(fs, p);
>>>  
>>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>>>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>>> +                                paging_max_paddr_bits(d));
>>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
>>>  
>>>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>>>  
>>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>>> index 9c61b5b..774a11f 100644
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>>      }
>>>  
>>>      if ( (gpa & ~PAGE_MASK) ||
>>> -         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
>>> +         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
>>>      {
>>>          vmfail_invalid(regs);
>>>          return X86EMUL_OKAY;
>>> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
>>> index a67fd5a..5ad8cf6 100644
>>> --- a/xen/arch/x86/mm/guest_walk.c
>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>> @@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>>>      /* If this guest has a restricted physical address space then the
>>>       * target GFN must fit within it. */
>>>      if ( !(rc & _PAGE_PRESENT)
>>> -         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
>>> +         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
>>> +         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
>> This pattern, of taking a gfn and shifting it by
>> (cpuid->ectd.maxphysaddr-PAGE_SHIFT) to see if it's valid happens
>> several times; it seems like for both clarity and avoiding mistakes, it
>> would be better if it were put into a macro.
>>
>> Everything else looks good to me.  (No opinion on the other questions
>> raised so far.)
> 
> static inline unsigned int gfn_bits(const struct domain *d)
> {
>     return d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT;
> }
> 
> ?
> 
> I do like that idea.  It would certainly make all of the callsites more
> readable.
> 
> I can happily fold that change in if others agree.

I was thinking of going further:

static inline bool guest_gfn_valid(domain *d, gfn_t gfn)
{
    return !!(gfn_x(gfn) >> (d->arch.cpuid...) )
}

("Valid" might be a bit ambiguous, but I can't think of a better
description off the top of my head.)

 -George

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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Andrew Cooper]

On 14/02/17 17:49, George Dunlap wrote:
> On 14/02/17 17:45, Andrew Cooper wrote:
>> On 14/02/17 17:42, George Dunlap wrote:
>>> On 13/02/17 11:00, Andrew Cooper wrote:
>>>> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
>>>> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
>>>> backpointer.
>>>>
>>>> However, plenty of hardware has a physical address width less that 44 bits,
>>>> and the code added in shadow_domain_init() is a straight assignment.  This
>>>> causes gfn_bits to be increased beyond the physical address width on most
>>>> Intel consumer hardware (typically a width of 39, which is the number reported
>>>> to the guest via CPUID).
>>>>
>>>> If the guest intentionally creates a PTE referencing a physical address
>>>> between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
>>>> address.  However, the shadow code accepts the PTE, shadows it, and the
>>>> virtual address works normally.
>>>>
>>>> Introduce paging_max_paddr_bits() to calculate the largest guest physical
>>>> address supportable by the paging infrastructure, and update
>>>> recalculate_cpuid_policy() to take this into account when clamping the guests
>>>> cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
>>>> guests maxphysaddr.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Tim Deegan <tim@xen.org>
>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>
>>>> v2:
>>>>  * Introduce paging_max_paddr_bits() rather than moving paging logic into
>>>>    recalculate_cpuid_policy().
>>>>  * Rewrite half of the commit message.
>>>> ---
>>>>  xen/arch/x86/cpuid.c            |  7 +++----
>>>>  xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
>>>>  xen/arch/x86/mm/guest_walk.c    |  3 ++-
>>>>  xen/arch/x86/mm/hap/hap.c       |  2 --
>>>>  xen/arch/x86/mm/p2m.c           |  3 ++-
>>>>  xen/arch/x86/mm/shadow/common.c | 10 ----------
>>>>  xen/arch/x86/mm/shadow/multi.c  |  3 ++-
>>>>  xen/include/asm-x86/domain.h    |  3 ---
>>>>  xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
>>>>  9 files changed, 26 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>>>> index e0a387e..3378f7a 100644
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -6,6 +6,7 @@
>>>>  #include <asm/hvm/nestedhvm.h>
>>>>  #include <asm/hvm/svm/svm.h>
>>>>  #include <asm/hvm/vmx/vmcs.h>
>>>> +#include <asm/paging.h>
>>>>  #include <asm/processor.h>
>>>>  #include <asm/xstate.h>
>>>>  
>>>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>>>  
>>>>      cpuid_featureset_to_policy(fs, p);
>>>>  
>>>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>>>>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>>>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>>>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>>>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>>>> +                                paging_max_paddr_bits(d));
>>>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
>>>>  
>>>>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>>>>  
>>>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>>>> index 9c61b5b..774a11f 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>>> @@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>>>      }
>>>>  
>>>>      if ( (gpa & ~PAGE_MASK) ||
>>>> -         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
>>>> +         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
>>>>      {
>>>>          vmfail_invalid(regs);
>>>>          return X86EMUL_OKAY;
>>>> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
>>>> index a67fd5a..5ad8cf6 100644
>>>> --- a/xen/arch/x86/mm/guest_walk.c
>>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>>> @@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>>>>      /* If this guest has a restricted physical address space then the
>>>>       * target GFN must fit within it. */
>>>>      if ( !(rc & _PAGE_PRESENT)
>>>> -         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
>>>> +         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
>>>> +         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
>>> This pattern, of taking a gfn and shifting it by
>>> (cpuid->ectd.maxphysaddr-PAGE_SHIFT) to see if it's valid happens
>>> several times; it seems like for both clarity and avoiding mistakes, it
>>> would be better if it were put into a macro.
>>>
>>> Everything else looks good to me.  (No opinion on the other questions
>>> raised so far.)
>> static inline unsigned int gfn_bits(const struct domain *d)
>> {
>>     return d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT;
>> }
>>
>> ?
>>
>> I do like that idea.  It would certainly make all of the callsites more
>> readable.
>>
>> I can happily fold that change in if others agree.
> I was thinking of going further:
>
> static inline bool guest_gfn_valid(domain *d, gfn_t gfn)
> {
>     return !!(gfn_x(gfn) >> (d->arch.cpuid...) )
> }
>
> ("Valid" might be a bit ambiguous, but I can't think of a better
> description off the top of my head.)

Hmm.  That would be ok for the mm code, but more awkward in
nvmx_handle_vmxon() which works in terms of gpa rather than gfn.

Of course, that could be worked around with _gfn(gpa >> PAGE_SHIFT) so
isn't the end of the world.

~Andrew

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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[Jan Beulich]

>>> On 14.02.17 at 17:04, <andrew.cooper3@citrix.com> wrote:
> On 13/02/17 12:36, Jan Beulich wrote:
>>>>> On 13.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote:
>>> @@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
>>>  int paging_set_allocation(struct domain *d, unsigned int pages,
>>>                            bool *preempted);
>>>  
>>> +/* Maxphysaddr supportable by the paging infrastructure. */
>>> +static inline unsigned int paging_max_paddr_bits(const struct domain *d)
>>> +{
>>> +    unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>>> +
>>> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
>>> +         (!is_pv_domain(d) || opt_allow_superpage) )
>>> +    {
>>> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
>>> +        bits = min(bits, 32U + PAGE_SHIFT);
>>> +    }
>>> +
>>> +    return bits;
>>> +}
>> Does this really need to be an inline function? With the overall goal
>> of not including every header everywhere, I particularly dislike the
>> need to include xen/kconfig.h here for things to build.
> 
> It is not on a critically path, but it seems wasteful to force something
> this small to be out of line.
> 
> As for kconfig.h, it is tiny.  What is the problem with adding it here? 
> We already pull generated/autoconf.h into everything via xen/config.h

Well, it's a general pattern, and hence should be of concern
irrespective of header size. Furthermore including such generic
headers in a frequently used header like this one will sooner or
later unavoidably lead to its explicit inclusion being omitted from
other source files. That in turn will require a patch touching
perhaps many unrelated files, should the inclusion in the header
here ever be removed again.

But no, I'm not intending to block the patch because of this
aspect, I merely wanted you to reconsider the placement.

Jan


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

Re: [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr

[George Dunlap]

On 14/02/17 17:56, Andrew Cooper wrote:
> On 14/02/17 17:49, George Dunlap wrote:
>> On 14/02/17 17:45, Andrew Cooper wrote:
>>> On 14/02/17 17:42, George Dunlap wrote:
>>>> On 13/02/17 11:00, Andrew Cooper wrote:
>>>>> XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
>>>>> lower than the real maxphysaddr, to avoid overflowing the superpage shadow
>>>>> backpointer.
>>>>>
>>>>> However, plenty of hardware has a physical address width less that 44 bits,
>>>>> and the code added in shadow_domain_init() is a straight assignment.  This
>>>>> causes gfn_bits to be increased beyond the physical address width on most
>>>>> Intel consumer hardware (typically a width of 39, which is the number reported
>>>>> to the guest via CPUID).
>>>>>
>>>>> If the guest intentionally creates a PTE referencing a physical address
>>>>> between 39 and 44 bits, the result should be #PF[RSVD] for using the virtual
>>>>> address.  However, the shadow code accepts the PTE, shadows it, and the
>>>>> virtual address works normally.
>>>>>
>>>>> Introduce paging_max_paddr_bits() to calculate the largest guest physical
>>>>> address supportable by the paging infrastructure, and update
>>>>> recalculate_cpuid_policy() to take this into account when clamping the guests
>>>>> cpuid_policy to reality.  Remove gfn_bits and rework its users in terms of a
>>>>> guests maxphysaddr.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Tim Deegan <tim@xen.org>
>>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>
>>>>> v2:
>>>>>  * Introduce paging_max_paddr_bits() rather than moving paging logic into
>>>>>    recalculate_cpuid_policy().
>>>>>  * Rewrite half of the commit message.
>>>>> ---
>>>>>  xen/arch/x86/cpuid.c            |  7 +++----
>>>>>  xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
>>>>>  xen/arch/x86/mm/guest_walk.c    |  3 ++-
>>>>>  xen/arch/x86/mm/hap/hap.c       |  2 --
>>>>>  xen/arch/x86/mm/p2m.c           |  3 ++-
>>>>>  xen/arch/x86/mm/shadow/common.c | 10 ----------
>>>>>  xen/arch/x86/mm/shadow/multi.c  |  3 ++-
>>>>>  xen/include/asm-x86/domain.h    |  3 ---
>>>>>  xen/include/asm-x86/paging.h    | 16 ++++++++++++++++
>>>>>  9 files changed, 26 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>>>>> index e0a387e..3378f7a 100644
>>>>> --- a/xen/arch/x86/cpuid.c
>>>>> +++ b/xen/arch/x86/cpuid.c
>>>>> @@ -6,6 +6,7 @@
>>>>>  #include <asm/hvm/nestedhvm.h>
>>>>>  #include <asm/hvm/svm/svm.h>
>>>>>  #include <asm/hvm/vmx/vmcs.h>
>>>>> +#include <asm/paging.h>
>>>>>  #include <asm/processor.h>
>>>>>  #include <asm/xstate.h>
>>>>>  
>>>>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>>>>  
>>>>>      cpuid_featureset_to_policy(fs, p);
>>>>>  
>>>>> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
>>>>>      p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
>>>>> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
>>>>> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
>>>>> -                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
>>>>> +                                paging_max_paddr_bits(d));
>>>>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
>>>>>  
>>>>>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>>>>>  
>>>>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>>>>> index 9c61b5b..774a11f 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>>>> @@ -1381,7 +1381,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>>>>      }
>>>>>  
>>>>>      if ( (gpa & ~PAGE_MASK) ||
>>>>> -         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
>>>>> +         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
>>>>>      {
>>>>>          vmfail_invalid(regs);
>>>>>          return X86EMUL_OKAY;
>>>>> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
>>>>> index a67fd5a..5ad8cf6 100644
>>>>> --- a/xen/arch/x86/mm/guest_walk.c
>>>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>>>> @@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>>>>>      /* If this guest has a restricted physical address space then the
>>>>>       * target GFN must fit within it. */
>>>>>      if ( !(rc & _PAGE_PRESENT)
>>>>> -         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
>>>>> +         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
>>>>> +         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
>>>> This pattern, of taking a gfn and shifting it by
>>>> (cpuid->ectd.maxphysaddr-PAGE_SHIFT) to see if it's valid happens
>>>> several times; it seems like for both clarity and avoiding mistakes, it
>>>> would be better if it were put into a macro.
>>>>
>>>> Everything else looks good to me.  (No opinion on the other questions
>>>> raised so far.)
>>> static inline unsigned int gfn_bits(const struct domain *d)
>>> {
>>>     return d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT;
>>> }
>>>
>>> ?
>>>
>>> I do like that idea.  It would certainly make all of the callsites more
>>> readable.
>>>
>>> I can happily fold that change in if others agree.
>> I was thinking of going further:
>>
>> static inline bool guest_gfn_valid(domain *d, gfn_t gfn)
>> {
>>     return !!(gfn_x(gfn) >> (d->arch.cpuid...) )
>> }
>>
>> ("Valid" might be a bit ambiguous, but I can't think of a better
>> description off the top of my head.)
> 
> Hmm.  That would be ok for the mm code, but more awkward in
> nvmx_handle_vmxon() which works in terms of gpa rather than gfn.
> 
> Of course, that could be worked around with _gfn(gpa >> PAGE_SHIFT) so
> isn't the end of the world.

Or you could make guest_gpa_valid(), and have guest_gfn_valid.  But I
don't mind too much as long as there's some sort of macro that makes it
easier to read and harder to make a mistake.

 -George


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