[PATCH v2 0/3] x86/mm: address observations made while working on XSA-388

[PATCH v2 0/3] x86/mm: address observations made while working on XSA-388

[Jan Beulich]

1: PoD: simplify / improve p2m_pod_cache_add()
2: altp2m: p2m_altp2m_get_or_propagate() should honor present page order
3: altp2m: p2m_altp2m_propagate_change() should honor present page order

The last one continues to be RFC, as there is an aspect I can't make sense
of. See there.

Jan

[PATCH v2 1/3] x86/PoD: simplify / improve p2m_pod_cache_add()

[Jan Beulich]

Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
pointless local variable "p". Make sure the MFN logged in a debugging
error message is actually the offending one. Return negative errno
values rather than -1 (presently no caller really cares, but imo this
should change). Adjust style.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Return -errno. Drop exclamation mark from log message.

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m
                   unsigned int order)
 {
     unsigned long i;
-    struct page_info *p;
     struct domain *d = p2m->domain;
+    mfn_t mfn = page_to_mfn(page);
 
 #ifndef NDEBUG
-    mfn_t mfn;
-
-    mfn = page_to_mfn(page);
-
     /* Check to make sure this is a contiguous region */
     if ( mfn_x(mfn) & ((1UL << order) - 1) )
     {
         printk("%s: mfn %lx not aligned order %u! (mask %lx)\n",
                __func__, mfn_x(mfn), order, ((1UL << order) - 1));
-        return -1;
+        return -EINVAL;
     }
 
-    for ( i = 0; i < 1UL << order ; i++)
+    for ( i = 0; i < (1UL << order); i++)
     {
-        struct domain * od;
+        const struct domain *od = page_get_owner(page + i);
 
-        p = mfn_to_page(mfn_add(mfn, i));
-        od = page_get_owner(p);
         if ( od != d )
         {
-            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
-                   __func__, mfn_x(mfn), d->domain_id,
-                   od ? od->domain_id : -1);
-            return -1;
+            printk("%s: mfn %lx owner: expected %pd, got %pd\n",
+                   __func__, mfn_x(mfn) + i, d, od);
+            return -EACCES;
         }
     }
 #endif
@@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m
      * promise to provide zero pages. So we scrub pages before using.
      */
     for ( i = 0; i < (1UL << order); i++ )
-        clear_domain_page(mfn_add(page_to_mfn(page), i));
+        clear_domain_page(mfn_add(mfn, i));
 
     /* First, take all pages off the domain list */
     lock_page_alloc(p2m);
-    for ( i = 0; i < 1UL << order ; i++ )
-    {
-        p = page + i;
-        page_list_del(p, &d->page_list);
-    }
-
+    for ( i = 0; i < (1UL << order); i++ )
+        page_list_del(page + i, &d->page_list);
     unlock_page_alloc(p2m);
 
     /* Then add to the appropriate populate-on-demand list. */

Ping: [PATCH v2 1/3] x86/PoD: simplify / improve p2m_pod_cache_add()

[Jan Beulich]

On 04.01.2022 10:48, Jan Beulich wrote:
> Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
> pointless local variable "p". Make sure the MFN logged in a debugging
> error message is actually the offending one. Return negative errno
> values rather than -1 (presently no caller really cares, but imo this
> should change). Adjust style.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Return -errno. Drop exclamation mark from log message.

Ping?

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>                    unsigned int order)
>  {
>      unsigned long i;
> -    struct page_info *p;
>      struct domain *d = p2m->domain;
> +    mfn_t mfn = page_to_mfn(page);
>  
>  #ifndef NDEBUG
> -    mfn_t mfn;
> -
> -    mfn = page_to_mfn(page);
> -
>      /* Check to make sure this is a contiguous region */
>      if ( mfn_x(mfn) & ((1UL << order) - 1) )
>      {
>          printk("%s: mfn %lx not aligned order %u! (mask %lx)\n",
>                 __func__, mfn_x(mfn), order, ((1UL << order) - 1));
> -        return -1;
> +        return -EINVAL;
>      }
>  
> -    for ( i = 0; i < 1UL << order ; i++)
> +    for ( i = 0; i < (1UL << order); i++)
>      {
> -        struct domain * od;
> +        const struct domain *od = page_get_owner(page + i);
>  
> -        p = mfn_to_page(mfn_add(mfn, i));
> -        od = page_get_owner(p);
>          if ( od != d )
>          {
> -            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
> -                   __func__, mfn_x(mfn), d->domain_id,
> -                   od ? od->domain_id : -1);
> -            return -1;
> +            printk("%s: mfn %lx owner: expected %pd, got %pd\n",
> +                   __func__, mfn_x(mfn) + i, d, od);
> +            return -EACCES;
>          }
>      }
>  #endif
> @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>       * promise to provide zero pages. So we scrub pages before using.
>       */
>      for ( i = 0; i < (1UL << order); i++ )
> -        clear_domain_page(mfn_add(page_to_mfn(page), i));
> +        clear_domain_page(mfn_add(mfn, i));
>  
>      /* First, take all pages off the domain list */
>      lock_page_alloc(p2m);
> -    for ( i = 0; i < 1UL << order ; i++ )
> -    {
> -        p = page + i;
> -        page_list_del(p, &d->page_list);
> -    }
> -
> +    for ( i = 0; i < (1UL << order); i++ )
> +        page_list_del(page + i, &d->page_list);
>      unlock_page_alloc(p2m);
>  
>      /* Then add to the appropriate populate-on-demand list. */
> 
> 

Ping²: [PATCH v2 1/3] x86/PoD: simplify / improve p2m_pod_cache_add()

[Jan Beulich]

On 27.01.2022 16:04, Jan Beulich wrote:
> On 04.01.2022 10:48, Jan Beulich wrote:
>> Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
>> pointless local variable "p". Make sure the MFN logged in a debugging
>> error message is actually the offending one. Return negative errno
>> values rather than -1 (presently no caller really cares, but imo this
>> should change). Adjust style.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Return -errno. Drop exclamation mark from log message.
> 
> Ping?

Another 4 weeks have passed ...

Thanks for feedback either way, Jan

>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>>                    unsigned int order)
>>  {
>>      unsigned long i;
>> -    struct page_info *p;
>>      struct domain *d = p2m->domain;
>> +    mfn_t mfn = page_to_mfn(page);
>>  
>>  #ifndef NDEBUG
>> -    mfn_t mfn;
>> -
>> -    mfn = page_to_mfn(page);
>> -
>>      /* Check to make sure this is a contiguous region */
>>      if ( mfn_x(mfn) & ((1UL << order) - 1) )
>>      {
>>          printk("%s: mfn %lx not aligned order %u! (mask %lx)\n",
>>                 __func__, mfn_x(mfn), order, ((1UL << order) - 1));
>> -        return -1;
>> +        return -EINVAL;
>>      }
>>  
>> -    for ( i = 0; i < 1UL << order ; i++)
>> +    for ( i = 0; i < (1UL << order); i++)
>>      {
>> -        struct domain * od;
>> +        const struct domain *od = page_get_owner(page + i);
>>  
>> -        p = mfn_to_page(mfn_add(mfn, i));
>> -        od = page_get_owner(p);
>>          if ( od != d )
>>          {
>> -            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
>> -                   __func__, mfn_x(mfn), d->domain_id,
>> -                   od ? od->domain_id : -1);
>> -            return -1;
>> +            printk("%s: mfn %lx owner: expected %pd, got %pd\n",
>> +                   __func__, mfn_x(mfn) + i, d, od);
>> +            return -EACCES;
>>          }
>>      }
>>  #endif
>> @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>>       * promise to provide zero pages. So we scrub pages before using.
>>       */
>>      for ( i = 0; i < (1UL << order); i++ )
>> -        clear_domain_page(mfn_add(page_to_mfn(page), i));
>> +        clear_domain_page(mfn_add(mfn, i));
>>  
>>      /* First, take all pages off the domain list */
>>      lock_page_alloc(p2m);
>> -    for ( i = 0; i < 1UL << order ; i++ )
>> -    {
>> -        p = page + i;
>> -        page_list_del(p, &d->page_list);
>> -    }
>> -
>> +    for ( i = 0; i < (1UL << order); i++ )
>> +        page_list_del(page + i, &d->page_list);
>>      unlock_page_alloc(p2m);
>>  
>>      /* Then add to the appropriate populate-on-demand list. */
>>
>>
> 

Re: [PATCH v2 1/3] x86/PoD: simplify / improve p2m_pod_cache_add()

[George Dunlap]

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

On Tue, Jan 4, 2022 at 9:48 AM Jan Beulich <jbeulich@suse.com> wrote:

> Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
> pointless local variable "p". Make sure the MFN logged in a debugging
> error message is actually the offending one. Return negative errno
> values rather than -1 (presently no caller really cares, but imo this
> should change). Adjust style.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>

Sorry, not sure how I managed to miss this:

Reviewed-by: George Dunlap <george.dunlap@cloud.com>

[-- Attachment #2: Type: text/html, Size: 1165 bytes --]

[PATCH v2 2/3] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Jan Beulich]

Prior to XSA-304 the only caller merely happened to not use any further
the order value that it passes into the function. Already then this was
a latent issue: The function really should, in the "get" case, hand back
the order the underlying mapping actually uses (or actually the smaller
of the two), such that (going forward) there wouldn't be any action on
unrelated mappings (in particular ones which did already diverge from
the host P2M).

Similarly in the "propagate" case only the smaller of the two orders
should actually get used for creating the new entry, again to avoid
altering mappings which did already diverge from the host P2M.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1832,7 +1832,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
          * altp2m.
          */
         if ( p2m_altp2m_get_or_propagate(p2m, gfn, &mfn, &p2mt,
-                                         &p2ma, page_order) )
+                                         &p2ma, &page_order) )
         {
             /* Entry was copied from host -- retry fault */
             rc = 1;
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2198,10 +2198,11 @@ bool_t p2m_switch_vcpu_altp2m_by_id(stru
  */
 bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
                                  mfn_t *mfn, p2m_type_t *p2mt,
-                                 p2m_access_t *p2ma, unsigned int page_order)
+                                 p2m_access_t *p2ma, unsigned int *page_order)
 {
     p2m_type_t ap2mt;
     p2m_access_t ap2ma;
+    unsigned int cur_order;
     unsigned long mask;
     gfn_t gfn;
     mfn_t amfn;
@@ -2214,7 +2215,10 @@ bool p2m_altp2m_get_or_propagate(struct
      */
     p2m_lock(ap2m);
 
-    amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, NULL);
+    amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, &cur_order);
+
+    if ( cur_order > *page_order )
+        cur_order = *page_order;
 
     if ( !mfn_eq(amfn, INVALID_MFN) )
     {
@@ -2222,6 +2226,7 @@ bool p2m_altp2m_get_or_propagate(struct
         *mfn  = amfn;
         *p2mt = ap2mt;
         *p2ma = ap2ma;
+        *page_order = cur_order;
         return false;
     }
 
@@ -2229,6 +2234,7 @@ bool p2m_altp2m_get_or_propagate(struct
     if ( mfn_eq(*mfn, INVALID_MFN) )
     {
         p2m_unlock(ap2m);
+        *page_order = cur_order;
         return false;
     }
 
@@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct
      * to the start of the superpage.  NB that we repupose `amfn`
      * here.
      */
-    mask = ~((1UL << page_order) - 1);
+    mask = ~((1UL << cur_order) - 1);
     amfn = _mfn(mfn_x(*mfn) & mask);
     gfn = _gfn(gfn_l & mask);
 
-    rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma);
+    rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
     p2m_unlock(ap2m);
 
     if ( rc )
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -852,7 +852,7 @@ void p2m_flush_altp2m(struct domain *d);
 /* Alternate p2m paging */
 bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
                                  mfn_t *mfn, p2m_type_t *p2mt,
-                                 p2m_access_t *p2ma, unsigned int page_order);
+                                 p2m_access_t *p2ma, unsigned int *page_order);
 
 /* Make a specific alternate p2m valid */
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);

Re: [PATCH v2 2/3] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Tamas K Lengyel]

On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Prior to XSA-304 the only caller merely happened to not use any further
> the order value that it passes into the function. Already then this was
> a latent issue: The function really should, in the "get" case, hand back
> the order the underlying mapping actually uses (or actually the smaller
> of the two), such that (going forward) there wouldn't be any action on
> unrelated mappings (in particular ones which did already diverge from
> the host P2M).
>
> Similarly in the "propagate" case only the smaller of the two orders
> should actually get used for creating the new entry, again to avoid
> altering mappings which did already diverge from the host P2M.

I don't really understand the reason why you want to return the
page_order from the altp2m here. The only check that uses the
page_order following is the super-page shattering check for XSA-304
but that's done on the hostp2m. So you would want to know what the
page_order is on the hosp2m, not the altp2m, no?

In either case, in all the setups we use altp2m we never use any
superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I
never trusted the complexity of superpage shattering and its
implementation in Xen as it is very hard to follow.

Tamas

Re: [PATCH v2 2/3] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Jan Beulich]

On 04.01.2022 16:00, Tamas K Lengyel wrote:
> On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Prior to XSA-304 the only caller merely happened to not use any further
>> the order value that it passes into the function. Already then this was
>> a latent issue: The function really should, in the "get" case, hand back
>> the order the underlying mapping actually uses (or actually the smaller
>> of the two), such that (going forward) there wouldn't be any action on
>> unrelated mappings (in particular ones which did already diverge from
>> the host P2M).
>>
>> Similarly in the "propagate" case only the smaller of the two orders
>> should actually get used for creating the new entry, again to avoid
>> altering mappings which did already diverge from the host P2M.
> 
> I don't really understand the reason why you want to return the
> page_order from the altp2m here. The only check that uses the
> page_order following is the super-page shattering check for XSA-304
> but that's done on the hostp2m. So you would want to know what the
> page_order is on the hosp2m, not the altp2m, no?

From what I see I would say the XSA-304 action is on the altp2m,
not the host one - the p2m_set_entry() invocation passes "p2m",
which gets set immediately prior to the call to
p2m_altp2m_get_or_propagate(). This is also what gets passed into
the function. It's the host p2m only when !altp2m_active(currd).

> In either case, in all the setups we use altp2m we never use any
> superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I
> never trusted the complexity of superpage shattering and its
> implementation in Xen as it is very hard to follow.

Hmm, interesting. If you're aware of bugs there, would you mind
letting us know the details? There shouldn't be a need to use
command line options to make altp2m actually work. If there's an
incompatibility, and if we can't get that fixed, perhaps use of
superpages would want suppressing when altp2m gets enabled for a
guest? Right now, without this being enforced (nor spelled out
anywhere in, say, a code comment), I don't think we should make
assumptions like this. Hence the patch.

Jan

Re: [PATCH v2 2/3] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Tamas K Lengyel]

On Tue, Jan 4, 2022 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.01.2022 16:00, Tamas K Lengyel wrote:
> > On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Prior to XSA-304 the only caller merely happened to not use any further
> >> the order value that it passes into the function. Already then this was
> >> a latent issue: The function really should, in the "get" case, hand back
> >> the order the underlying mapping actually uses (or actually the smaller
> >> of the two), such that (going forward) there wouldn't be any action on
> >> unrelated mappings (in particular ones which did already diverge from
> >> the host P2M).
> >>
> >> Similarly in the "propagate" case only the smaller of the two orders
> >> should actually get used for creating the new entry, again to avoid
> >> altering mappings which did already diverge from the host P2M.
> >
> > I don't really understand the reason why you want to return the
> > page_order from the altp2m here. The only check that uses the
> > page_order following is the super-page shattering check for XSA-304
> > but that's done on the hostp2m. So you would want to know what the
> > page_order is on the hosp2m, not the altp2m, no?
>
> From what I see I would say the XSA-304 action is on the altp2m,
> not the host one - the p2m_set_entry() invocation passes "p2m",
> which gets set immediately prior to the call to
> p2m_altp2m_get_or_propagate(). This is also what gets passed into
> the function. It's the host p2m only when !altp2m_active(currd).

Oh, yea, I see. OK, makes sense.

Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>

>
> > In either case, in all the setups we use altp2m we never use any
> > superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I
> > never trusted the complexity of superpage shattering and its
> > implementation in Xen as it is very hard to follow.
>
> Hmm, interesting. If you're aware of bugs there, would you mind
> letting us know the details? There shouldn't be a need to use
> command line options to make altp2m actually work. If there's an
> incompatibility, and if we can't get that fixed, perhaps use of
> superpages would want suppressing when altp2m gets enabled for a
> guest? Right now, without this being enforced (nor spelled out
> anywhere in, say, a code comment), I don't think we should make
> assumptions like this. Hence the patch.

I don't know of any bugs, it's just the complexity of page-shattering
always had a "smell", thus I opted to recommend superpages being
always disabled. With the added complexity of altp2m it's even more so
the case.

Tamas

[PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Jan Beulich]

For higher order mappings the comparison against p2m->min_remapped_gfn
needs to take the upper bound of the covered GFN range into account, not
just the base GFN.

Otherwise, i.e. when dropping a mapping and not overlapping the remapped
range, XXX.

Note that there's no need to call get_gfn_type_access() ahead of the
check against the remapped range boundaries: None of its outputs are
needed earlier. Local variables get moved into the more narrow scope to
demonstrate this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I may be entirely wrong and hence that part of the change may also be
wrong, but I'm having trouble seeing why the original
"!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
goal there to pre-fill entries that were previously invalid, instead of
undoing prior intentional divergence from the host P2M? (I have
intentionally not reflected this aspect in the description yet; I can't
really write a description of this without understanding what's going on
in case the original code was correct.)

When cur_order is below the passed in page_order, the p2m_set_entry() is
of course not covering the full range. This could be put in a loop, but
locking looks to be a little problematic: If a set of lower order pages
gets re-combined to a large one while already having progressed into the
range, we'd need to carefully back off. Alternatively the full incoming
GFN range could be held locked for the entire loop; this would likely
mean relying on gfn_lock() actually resolving to p2m_lock(). But perhaps
that's not a big problem, considering that core functions like
p2m_get_gfn_type_access() or __put_gfn() assume so, too (because of
not taking the order into account at all)?

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2532,9 +2532,6 @@ int p2m_altp2m_propagate_change(struct d
                                 p2m_type_t p2mt, p2m_access_t p2ma)
 {
     struct p2m_domain *p2m;
-    p2m_access_t a;
-    p2m_type_t t;
-    mfn_t m;
     unsigned int i;
     unsigned int reset_count = 0;
     unsigned int last_reset_idx = ~0;
@@ -2551,12 +2548,30 @@ int p2m_altp2m_propagate_change(struct d
             continue;
 
         p2m = d->arch.altp2m_p2m[i];
-        m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL);
 
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+        {
+            p2m_access_t a;
+            p2m_type_t t;
+            unsigned int cur_order;
+
+            if ( mfn_eq(get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0,
+                                            &cur_order),
+                        INVALID_MFN) )
+            {
+                int rc = p2m_set_entry(p2m, gfn, mfn, min(cur_order, page_order),
+                                       p2mt, p2ma);
+
+                /* Best effort: Don't bail on error. */
+                if ( !ret )
+                    ret = rc;
+            }
+
+            __put_gfn(p2m, gfn_x(gfn));
+        }
         /* Check for a dropped page that may impact this altp2m */
-        if ( mfn_eq(mfn, INVALID_MFN) &&
-             gfn_x(gfn) >= p2m->min_remapped_gfn &&
-             gfn_x(gfn) <= p2m->max_remapped_gfn )
+        else if ( gfn_x(gfn) + (1UL << page_order) > p2m->min_remapped_gfn &&
+                  gfn_x(gfn) <= p2m->max_remapped_gfn )
         {
             if ( !reset_count++ )
             {
@@ -2566,8 +2581,6 @@ int p2m_altp2m_propagate_change(struct d
             else
             {
                 /* At least 2 altp2m's impacted, so reset everything */
-                __put_gfn(p2m, gfn_x(gfn));
-
                 for ( i = 0; i < MAX_ALTP2M; i++ )
                 {
                     if ( i == last_reset_idx ||
@@ -2581,16 +2594,6 @@ int p2m_altp2m_propagate_change(struct d
                 break;
             }
         }
-        else if ( !mfn_eq(m, INVALID_MFN) )
-        {
-            int rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
-
-            /* Best effort: Don't bail on error. */
-            if ( !ret )
-                ret = rc;
-        }
-
-        __put_gfn(p2m, gfn_x(gfn));
     }
 
     altp2m_list_unlock(d);

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel]

> I may be entirely wrong and hence that part of the change may also be
> wrong, but I'm having trouble seeing why the original
> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
> goal there to pre-fill entries that were previously invalid, instead of
> undoing prior intentional divergence from the host P2M? (I have
> intentionally not reflected this aspect in the description yet; I can't
> really write a description of this without understanding what's going on
> in case the original code was correct.)

This function only gets called from p2m-ept when the hostp2m gets an
update. In that case this check goes through all altp2m's to see if
any of them has an entry for what just got changed in the host, and
overwrites the altp2m with that from the host. If there is no entry in
the altp2m it doesn't pre-populate. That should only happen if the
altp2m actually needs it and runs into a pagefault. So it is correct
as-is, albeit being a subtle (and undocumented) behavior of the
hostp2m and its effect on the altp2m's. But that's why we never
actually make any changes on the hostp2m, we always create an altp2m
and apply changes (mem_access/remapping) there.

Tamas

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Jan Beulich]

On 04.01.2022 18:48, Tamas K Lengyel wrote:
>> I may be entirely wrong and hence that part of the change may also be
>> wrong, but I'm having trouble seeing why the original
>> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
>> goal there to pre-fill entries that were previously invalid, instead of
>> undoing prior intentional divergence from the host P2M? (I have
>> intentionally not reflected this aspect in the description yet; I can't
>> really write a description of this without understanding what's going on
>> in case the original code was correct.)
> 
> This function only gets called from p2m-ept when the hostp2m gets an
> update. In that case this check goes through all altp2m's to see if
> any of them has an entry for what just got changed in the host, and
> overwrites the altp2m with that from the host. If there is no entry in
> the altp2m it doesn't pre-populate. That should only happen if the
> altp2m actually needs it and runs into a pagefault. So it is correct
> as-is, albeit being a subtle (and undocumented) behavior of the
> hostp2m and its effect on the altp2m's. But that's why we never
> actually make any changes on the hostp2m, we always create an altp2m
> and apply changes (mem_access/remapping) there.

Thanks for the explanation. Effectively this means that the call to
get_gfn_type_access() can simply be get_gfn_query(). For the patch
this means that I shouldn't check its return value and also continue
to pass the new order rather than the minimum of the two (as was the
case before), as all we're after is the locking of the GFN. It would
be nice if you could confirm this before I submit a non-RFC v3.

What I still don't understand is why the function blindly replaces
any possible entry in the altp2m, i.e. any possible override
mapping, not even taking into account the original p2m_access_t.

Jan

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel]

On Wed, Jan 5, 2022 at 3:59 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.01.2022 18:48, Tamas K Lengyel wrote:
> >> I may be entirely wrong and hence that part of the change may also be
> >> wrong, but I'm having trouble seeing why the original
> >> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
> >> goal there to pre-fill entries that were previously invalid, instead of
> >> undoing prior intentional divergence from the host P2M? (I have
> >> intentionally not reflected this aspect in the description yet; I can't
> >> really write a description of this without understanding what's going on
> >> in case the original code was correct.)
> >
> > This function only gets called from p2m-ept when the hostp2m gets an
> > update. In that case this check goes through all altp2m's to see if
> > any of them has an entry for what just got changed in the host, and
> > overwrites the altp2m with that from the host. If there is no entry in
> > the altp2m it doesn't pre-populate. That should only happen if the
> > altp2m actually needs it and runs into a pagefault. So it is correct
> > as-is, albeit being a subtle (and undocumented) behavior of the
> > hostp2m and its effect on the altp2m's. But that's why we never
> > actually make any changes on the hostp2m, we always create an altp2m
> > and apply changes (mem_access/remapping) there.
>
> Thanks for the explanation. Effectively this means that the call to
> get_gfn_type_access() can simply be get_gfn_query(). For the patch
> this means that I shouldn't check its return value and also continue
> to pass the new order rather than the minimum of the two (as was the
> case before), as all we're after is the locking of the GFN. It would
> be nice if you could confirm this before I submit a non-RFC v3.

I'm a little lost here.

>
> What I still don't understand is why the function blindly replaces
> any possible entry in the altp2m, i.e. any possible override
> mapping, not even taking into account the original p2m_access_t.

I think the idea was that any changes to the hostp2m will just get
reflected in the altp2m's as a short-cut. If you have many custom
settings in different altp2ms, simply setting the entry in the hostp2m
will ensure all altp2m's get synced to that same setting instead of
having to do a hypercall for each altp2m. I don't see much use of it
otherwise and if we wanted to switch it to leave the altp2m entries
as-is I wouldn't object.

Tamas

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Jan Beulich]

On 05.01.2022 17:25, Tamas K Lengyel wrote:
> On Wed, Jan 5, 2022 at 3:59 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.01.2022 18:48, Tamas K Lengyel wrote:
>>>> I may be entirely wrong and hence that part of the change may also be
>>>> wrong, but I'm having trouble seeing why the original
>>>> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
>>>> goal there to pre-fill entries that were previously invalid, instead of
>>>> undoing prior intentional divergence from the host P2M? (I have
>>>> intentionally not reflected this aspect in the description yet; I can't
>>>> really write a description of this without understanding what's going on
>>>> in case the original code was correct.)
>>>
>>> This function only gets called from p2m-ept when the hostp2m gets an
>>> update. In that case this check goes through all altp2m's to see if
>>> any of them has an entry for what just got changed in the host, and
>>> overwrites the altp2m with that from the host. If there is no entry in
>>> the altp2m it doesn't pre-populate. That should only happen if the
>>> altp2m actually needs it and runs into a pagefault. So it is correct
>>> as-is, albeit being a subtle (and undocumented) behavior of the
>>> hostp2m and its effect on the altp2m's. But that's why we never
>>> actually make any changes on the hostp2m, we always create an altp2m
>>> and apply changes (mem_access/remapping) there.
>>
>> Thanks for the explanation. Effectively this means that the call to
>> get_gfn_type_access() can simply be get_gfn_query(). For the patch
>> this means that I shouldn't check its return value and also continue
>> to pass the new order rather than the minimum of the two (as was the
>> case before), as all we're after is the locking of the GFN. It would
>> be nice if you could confirm this before I submit a non-RFC v3.
> 
> I'm a little lost here.

Let me start with simpler questions then:

What's the purpose of calling get_gfn_type_access()?

Independent of the answer to the previous question, why isn't it
get_gfn_query() that is called?

What's the purpose of the "a" local variable? (While "t" also is
otherwise unused, it can't be eliminated as even get_gfn_query()
requires its address to be taken.)

Why is p2m_set_entry() called only when the original entry didn't
resolve to INVALID_MFN?

>> What I still don't understand is why the function blindly replaces
>> any possible entry in the altp2m, i.e. any possible override
>> mapping, not even taking into account the original p2m_access_t.
> 
> I think the idea was that any changes to the hostp2m will just get
> reflected in the altp2m's as a short-cut. If you have many custom
> settings in different altp2ms, simply setting the entry in the hostp2m
> will ensure all altp2m's get synced to that same setting instead of
> having to do a hypercall for each altp2m. I don't see much use of it
> otherwise and if we wanted to switch it to leave the altp2m entries
> as-is I wouldn't object.

Hmm, I continue to be puzzled. Let's take the XSA-304 workaround as an
example. Suppose an introspection agent has removed X from a 4k page
in an altp2m of a guest. Suppose one of the vCPU-s of this guest runs
on the host p2m. If this vCPU hits the (presumably) 2M or 1G mapping
covering said 4k page for an insn fetch, the page will be shattered
and the removed X in one (or more) of the altp2m-s will, afaict, be
lost. This looks like a bug to me.

Jan

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel]

> Hmm, I continue to be puzzled. Let's take the XSA-304 workaround as an
> example. Suppose an introspection agent has removed X from a 4k page
> in an altp2m of a guest. Suppose one of the vCPU-s of this guest runs
> on the host p2m. If this vCPU hits the (presumably) 2M or 1G mapping
> covering said 4k page for an insn fetch, the page will be shattered
> and the removed X in one (or more) of the altp2m-s will, afaict, be
> lost. This looks like a bug to me.

Yeap, that can happen if you are using large pages and allow execution
on the hosp2m. We explicitly disable large pages when we use altp2m's
though so it's not an issue for us. Someone implementing an
introspection solution where they keep large pages would have to
pre-shatter all the large pages in the hostp2m and only then apply the
altp2m changes. Or have a separate altp2m view that's used only for
execution and the hostp2m is never used. So the way things are can
certainly be worked with and it's not a show-stopper, it's just
convoluted and you can certainly have bugs if you do it wrong that
would be hard to figure out.

As I said, I don't see much upside in why the current propagation
mechanism is in place and we don't use it, so if someone wants to
switch it because of preference or because it's less error-prone, I
wouldn't object.

Tamas

Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel]

On Thu, Jan 6, 2022 at 8:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.01.2022 17:25, Tamas K Lengyel wrote:
> > On Wed, Jan 5, 2022 at 3:59 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 04.01.2022 18:48, Tamas K Lengyel wrote:
> >>>> I may be entirely wrong and hence that part of the change may also be
> >>>> wrong, but I'm having trouble seeing why the original
> >>>> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
> >>>> goal there to pre-fill entries that were previously invalid, instead of
> >>>> undoing prior intentional divergence from the host P2M? (I have
> >>>> intentionally not reflected this aspect in the description yet; I can't
> >>>> really write a description of this without understanding what's going on
> >>>> in case the original code was correct.)
> >>>
> >>> This function only gets called from p2m-ept when the hostp2m gets an
> >>> update. In that case this check goes through all altp2m's to see if
> >>> any of them has an entry for what just got changed in the host, and
> >>> overwrites the altp2m with that from the host. If there is no entry in
> >>> the altp2m it doesn't pre-populate. That should only happen if the
> >>> altp2m actually needs it and runs into a pagefault. So it is correct
> >>> as-is, albeit being a subtle (and undocumented) behavior of the
> >>> hostp2m and its effect on the altp2m's. But that's why we never
> >>> actually make any changes on the hostp2m, we always create an altp2m
> >>> and apply changes (mem_access/remapping) there.
> >>
> >> Thanks for the explanation. Effectively this means that the call to
> >> get_gfn_type_access() can simply be get_gfn_query(). For the patch
> >> this means that I shouldn't check its return value and also continue
> >> to pass the new order rather than the minimum of the two (as was the
> >> case before), as all we're after is the locking of the GFN. It would
> >> be nice if you could confirm this before I submit a non-RFC v3.
> >
> > I'm a little lost here.
>
> Let me start with simpler questions then:
>
> What's the purpose of calling get_gfn_type_access()?

Only locking the gfn AFAICT.

> Independent of the answer to the previous question, why isn't it
> get_gfn_query() that is called?

The author of the code probably didn't see any difference between the
two. Or just didn't know there is another function.

> What's the purpose of the "a" local variable? (While "t" also is
> otherwise unused, it can't be eliminated as even get_gfn_query()
> requires its address to be taken.)

The a/t variables are ununsed.

>
> Why is p2m_set_entry() called only when the original entry didn't
> resolve to INVALID_MFN?

AFAICT there was never a clear design decision for why that's in
place. The only utility for it is fast propagation of settings across
all altp2m's that already have an entry in place. As per the other
part of the discussion it could be removed so existing entries in
altp2m's don't get overwritten. But we should most definitely NOT
pre-populate entries here for altp2ms.

Tamas