Re: [PATCH for-4.19? 1/2] xen/x86: account for max guest gfn and number of foreign mappings in the p2m

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2024 18:58, Roger Pau Monne wrote:
> Keep track of the maximum gfn that has ever been populated into the p2m, and
> also account for the number of foreign mappings.  Such information will be
> needed in order to remove foreign mappings during teardown for HVM guests.

Is "needed" the right term? We could e.g. traverse the P2M tree (didn't look
at patch 2 yet as to how exactly you use these two new fields there), at which
point we might get away without either or both of these extra statistics,
while at the same time also not needing to iterate over a gigantic range of
GFNs. Going from populated page tables would roughly match "max_gfn", with the
benefit of certain removals of P2M entries then also shrinking the upper bound.

> @@ -1049,6 +1057,8 @@ static inline int p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>          if ( !page_get_owner_and_reference(mfn_to_page(nfn)) )
>              return -EBUSY;
>  
> +        p2m->nr_foreign++;
> +
>          break;
>  
>      default:
> @@ -1069,6 +1079,7 @@ static inline int p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>              return -EINVAL;
>          }
>          put_page(mfn_to_page(ofn));
> +        p2m->nr_foreign--;
>          break;

Like for the ioreq accounting I'm a little worried of putting this here,
especially with the decrement thus coming ahead of the actual page table
update, but probably I'm overly concerned here. The put_page() living here
would clearly be doing bigger damage if not unconditionally followed by a page
table write. IOW - just a remark, no request for any kind of change.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -413,6 +413,8 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t gfn, 
> mfn_t mfn,
>          set_rc = p2m->set_entry(p2m, gfn, mfn, order, p2mt, p2ma, -1);
>          if ( set_rc )
>              rc = set_rc;
> +        else
> +            p2m->max_gfn = gfn_max(gfn_add(gfn, 1u << order), p2m->max_gfn);

For one a (new) field named "max_..." wants to record the maximum value, not
one above. And then you want to use 1UL, to match ...

>          gfn = gfn_add(gfn, 1UL << order);
>          if ( !mfn_eq(mfn, INVALID_MFN) )

... surrounding code (more just out of context).

Further I can't really convince myself that doing the update just here is
enough, or whether alternatively the update wouldn't want to be further
constrained to happen just on newly set foreign entries. In that latter
case it would be far easier to reason whether doing the update just here is
sufficient. Plus iirc foreign entries are also necessarily order-0 (else
p2m_entry_modify() wouldn't be correct as is), which would allow to store
just the gfn we have in hands, thus resulting in the field then being
properly named (as to its prefix; it would likely want to become
"max_foreign_gfn" then).

Jan