[PATCH 0/2] x86/P2M: allow 2M superpage use for shadowed guests

[PATCH 0/2] x86/P2M: allow 2M superpage use for shadowed guests

[Jan Beulich]

I did notice this anomaly in the context of IOMMU side work.

1: shadow: slightly consolidate sh_unshadow_for_p2m_change()
2: P2M: allow 2M superpage use for shadowed guests

Jan

[PATCH 1/2] x86/shadow: slightly consolidate sh_unshadow_for_p2m_change()

[Jan Beulich]

In preparation for reactivating the presently dead 2M page path of the
function,
- also deal with the case of replacing an L1 page table all in one go,
- pull common checks out of the switch(). This includes extending a
  _PAGE_PRESENT check to L1 as well, which presumably was deemed
  redundant with p2m_is_valid() || p2m_is_grant(), but I think we are
  better off being explicit in all cases,
- replace a p2m_is_ram() check in the 2M case by an explicit
  _PAGE_PRESENT one, to make more obvious that the subsequent
  l1e_get_mfn() actually retrieves something that is actually an MFN.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -801,7 +801,7 @@ static void sh_unshadow_for_p2m_change(s
                                        l1_pgentry_t old, l1_pgentry_t new,
                                        unsigned int level)
 {
-    mfn_t omfn = l1e_get_mfn(old);
+    mfn_t omfn = l1e_get_mfn(old), nmfn;
     unsigned int oflags = l1e_get_flags(old);
     p2m_type_t p2mt = p2m_flags_to_type(oflags);
     bool flush = false;
@@ -813,19 +813,30 @@ static void sh_unshadow_for_p2m_change(s
     if ( unlikely(!d->arch.paging.shadow.total_pages) )
         return;
 
+    /* Only previously present / valid entries need processing. */
+    if ( !(oflags & _PAGE_PRESENT) ||
+         (!p2m_is_valid(p2mt) && !p2m_is_grant(p2mt)) ||
+         !mfn_valid(omfn) )
+        return;
+
+    nmfn = l1e_get_flags(new) & _PAGE_PRESENT ? l1e_get_mfn(new) : INVALID_MFN;
+
     switch ( level )
     {
     default:
         /*
          * The following assertion is to make sure we don't step on 1GB host
-         * page support of HVM guest.
+         * page support of HVM guest. Plus we rely on ->set_entry() to never
+         * get called with orders above PAGE_ORDER_2M, not even to install
+         * non-present entries (which in principle ought to be fine even
+         * without respective large page support).
          */
-        ASSERT(!((oflags & _PAGE_PRESENT) && (oflags & _PAGE_PSE)));
+        ASSERT_UNREACHABLE();
         break;
 
     /* If we're removing an MFN from the p2m, remove it from the shadows too */
     case 1:
-        if ( (p2m_is_valid(p2mt) || p2m_is_grant(p2mt)) && mfn_valid(omfn) )
+        if ( !mfn_eq(nmfn, omfn) )
         {
             sh_remove_all_shadows_and_parents(d, omfn);
             if ( sh_remove_all_mappings(d, omfn, _gfn(gfn)) )
@@ -839,14 +850,9 @@ static void sh_unshadow_for_p2m_change(s
      * scheme, that's OK, but otherwise they must be unshadowed.
      */
     case 2:
-        if ( !(oflags & _PAGE_PRESENT) || !(oflags & _PAGE_PSE) )
-            break;
-
-        if ( p2m_is_valid(p2mt) && mfn_valid(omfn) )
         {
             unsigned int i;
-            mfn_t nmfn = l1e_get_mfn(new);
-            l1_pgentry_t *npte = NULL;
+            l1_pgentry_t *npte = NULL, *opte = NULL;
 
             /* If we're replacing a superpage with a normal L1 page, map it */
             if ( (l1e_get_flags(new) & _PAGE_PRESENT) &&
@@ -854,24 +860,39 @@ static void sh_unshadow_for_p2m_change(s
                  mfn_valid(nmfn) )
                 npte = map_domain_page(nmfn);
 
+            /* If we're replacing a normal L1 page, map it as well. */
+            if ( !(oflags & _PAGE_PSE) )
+                opte = map_domain_page(omfn);
+
             gfn &= ~(L1_PAGETABLE_ENTRIES - 1);
 
             for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
             {
-                if ( !npte ||
-                     !p2m_is_ram(p2m_flags_to_type(l1e_get_flags(npte[i]))) ||
-                     !mfn_eq(l1e_get_mfn(npte[i]), omfn) )
+                if ( opte )
+                {
+                    if ( !(l1e_get_flags(opte[i]) & _PAGE_PRESENT) )
+                        continue;
+                    omfn = l1e_get_mfn(opte[i]);
+                }
+
+                if ( npte )
+                    nmfn = l1e_get_flags(npte[i]) & _PAGE_PRESENT
+                           ? l1e_get_mfn(npte[i]) : INVALID_MFN;
+
+                if ( !mfn_eq(nmfn, omfn) )
                 {
                     /* This GFN->MFN mapping has gone away */
                     sh_remove_all_shadows_and_parents(d, omfn);
                     if ( sh_remove_all_mappings(d, omfn, _gfn(gfn + i)) )
                         flush = true;
                 }
+
                 omfn = mfn_add(omfn, 1);
+                nmfn = mfn_add(nmfn, 1);
             }
 
-            if ( npte )
-                unmap_domain_page(npte);
+            unmap_domain_page(opte);
+            unmap_domain_page(npte);
         }
 
         break;

Re: [PATCH 1/2] x86/shadow: slightly consolidate sh_unshadow_for_p2m_change()

[George Dunlap]

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



> On 9 Dec 2021, at 11:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> In preparation for reactivating the presently dead 2M page path of the
> function,
> - also deal with the case of replacing an L1 page table all in one go,
> - pull common checks out of the switch(). This includes extending a
>  _PAGE_PRESENT check to L1 as well, which presumably was deemed
>  redundant with p2m_is_valid() || p2m_is_grant(), but I think we are
>  better off being explicit in all cases,
> - replace a p2m_is_ram() check in the 2M case by an explicit
>  _PAGE_PRESENT one, to make more obvious that the subsequent
>  l1e_get_mfn() actually retrieves something that is actually an MFN.

Each of these changes requires careful checking to make sure there aren’t any bugs introduced.  I’d feel much more comfortable giving an R-b of they were broken out into separate patches.

 -George


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/shadow: slightly consolidate sh_unshadow_for_p2m_change()

[Jan Beulich]

On 24.06.2022 21:16, George Dunlap wrote:
> 
> 
>> On 9 Dec 2021, at 11:26, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> In preparation for reactivating the presently dead 2M page path of the
>> function,
>> - also deal with the case of replacing an L1 page table all in one go,
>> - pull common checks out of the switch(). This includes extending a
>>  _PAGE_PRESENT check to L1 as well, which presumably was deemed
>>  redundant with p2m_is_valid() || p2m_is_grant(), but I think we are
>>  better off being explicit in all cases,
>> - replace a p2m_is_ram() check in the 2M case by an explicit
>>  _PAGE_PRESENT one, to make more obvious that the subsequent
>>  l1e_get_mfn() actually retrieves something that is actually an MFN.
> 
> Each of these changes requires careful checking to make sure there aren’t any bugs introduced.  I’d feel much more comfortable giving an R-b of they were broken out into separate patches.

I'll see what I can do. It has been quite some time, but iirc trying
to do things separately didn't work out very well.

Jan

[PATCH 2/2] x86/P2M: allow 2M superpage use for shadowed guests

[Jan Beulich]

For guests in shadow mode the P2M table gets used only by software. The
only place where it matters whether superpages in the P2M can be dealt
with is sh_unshadow_for_p2m_change(). That function has been capabale of
handling them even before commit 0ca1669871f8a ("P2M: check whether hap
mode is enabled before using 2mb pages") disabled 2M use in this case
for dubious reasons ("potential errors when hap is disabled").

While doing this, move "order" into more narrow scope and replace the
local variable "d" by a new "hap" one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Strictly speaking "fn_mask" could also be "unsigned int"; I wasn't sure
whether changing that would cause objections.

While at least sh_unshadow_for_p2m_change() presently relies on this
behavior, it is somewhat odd (and inefficient) for p2m_set_entry() to
split even non-present mappings.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -631,28 +631,22 @@ struct page_info *p2m_get_page_from_gfn(
 int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, mfn_t mfn,
                   unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
 {
-    struct domain *d = p2m->domain;
+    bool hap = hap_enabled(p2m->domain);
     unsigned long todo = 1ul << page_order;
-    unsigned int order;
     int set_rc, rc = 0;
 
     ASSERT(gfn_locked_by_me(p2m, gfn));
 
     while ( todo )
     {
-        if ( hap_enabled(d) )
-        {
-            unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0;
-
-            fn_mask |= gfn_x(gfn) | todo;
-
-            order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) &&
-                     hap_has_1gb) ? PAGE_ORDER_1G :
-                    (!(fn_mask & ((1ul << PAGE_ORDER_2M) - 1)) &&
-                     hap_has_2mb) ? PAGE_ORDER_2M : PAGE_ORDER_4K;
-        }
-        else
-            order = 0;
+        unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0) |
+                                gfn_x(gfn) | todo;
+        unsigned int order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) &&
+                              hap && hap_has_1gb)
+                             ? PAGE_ORDER_1G
+                             : (!(fn_mask & ((1ul << PAGE_ORDER_2M) - 1)) &&
+                                (!hap || hap_has_2mb))
+                               ? PAGE_ORDER_2M : PAGE_ORDER_4K;
 
         set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
         if ( set_rc )

Re: [PATCH 2/2] x86/P2M: allow 2M superpage use for shadowed guests

[George Dunlap]

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



> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote:
> 
> For guests in shadow mode the P2M table gets used only by software. The
> only place where it matters whether superpages in the P2M can be dealt
> with is sh_unshadow_for_p2m_change().

It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here.  Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify?

e.g., all other accesses to the p2m in the shadow code are via get_gfn_[something](), which (because it’s in the p2m code) handles p2m superpages correctly?

Everything else looks good here.

> That function has been capabale of
> handling them even before commit 0ca1669871f8a ("P2M: check whether hap
> mode is enabled before using 2mb pages") disabled 2M use in this case
> for dubious reasons ("potential errors when hap is disabled").

I’m glad the days of random patches being checked in without comment or discussion are behind us...

 -George


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] x86/P2M: allow 2M superpage use for shadowed guests

[Jan Beulich]

On 24.06.2022 21:27, George Dunlap wrote:
> 
> 
>> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> For guests in shadow mode the P2M table gets used only by software. The
>> only place where it matters whether superpages in the P2M can be dealt
>> with is sh_unshadow_for_p2m_change().
> 
> It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here.  Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify?

Would

"The table is never made accessible by hardware for address translation,
 and the only checks of _PAGE_PSE in P2M entries in shadow code are in
 this function (all others are against guest page table entries)."

look sufficient to you?

> e.g., all other accesses to the p2m in the shadow code are via get_gfn_[something](), which (because it’s in the p2m code) handles p2m superpages correctly?

Well, yes - I don't think I need to reason about generic P2M code being
super-page aware?

Jan

Re: [PATCH 2/2] x86/P2M: allow 2M superpage use for shadowed guests

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 1080 bytes --]



> On 27 Jun 2022, at 16:33, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 24.06.2022 21:27, George Dunlap wrote:
>> 
>> 
>>> On 9 Dec 2021, at 11:27, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> For guests in shadow mode the P2M table gets used only by software. The
>>> only place where it matters whether superpages in the P2M can be dealt
>>> with is sh_unshadow_for_p2m_change().
>> 
>> It’s easy to verify that this patch is doing what it claims to do; but whether it’s correct or not depends on the veracity of this claim here. Rather than me having to duplicate whatever work you did to come to this conclusion, can you briefly explain why it’s true in a way that I can easily verify?
> 
> Would
> 
> "The table is never made accessible by hardware for address translation,
> and the only checks of _PAGE_PSE in P2M entries in shadow code are in
> this function (all others are against guest page table entries)."
> 
> look sufficient to you?

Sorry for the delay responding to this — yes, I think this would do, thanks.

 -George


[-- Attachment #1.2: Type: text/html, Size: 7640 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Ping: [PATCH 0/2] x86/P2M: allow 2M superpage use for shadowed guests

[Jan Beulich]

On 09.12.2021 12:25, Jan Beulich wrote:
> I did notice this anomaly in the context of IOMMU side work.
> 
> 1: shadow: slightly consolidate sh_unshadow_for_p2m_change()
> 2: P2M: allow 2M superpage use for shadowed guests

This has been pending for over half a year. Anyone?

Thanks, Jan

Re: Ping: [PATCH 0/2] x86/P2M: allow 2M superpage use for shadowed guests

[George Dunlap]

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



> On 23 Jun 2022, at 13:00, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.12.2021 12:25, Jan Beulich wrote:
>> I did notice this anomaly in the context of IOMMU side work.
>> 
>> 1: shadow: slightly consolidate sh_unshadow_for_p2m_change()
>> 2: P2M: allow 2M superpage use for shadowed guests
> 
> This has been pending for over half a year. Anyone?

I can put it on a list of things to look at tomorrow.

 -George


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]