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

[Andrew Cooper <amc96@srcf.net>]

On 01/12/2021 10:53, 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. Adjust style.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -58,14 +58,10 @@ 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) )
>      {
> @@ -74,17 +70,14 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>          return -1;
>      }
>  
> -    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);
> +            printk("%s: mfn %lx owner: expected %pd, got %pd!\n",
> +                   __func__, mfn_x(mfn) + i, d, od);

Take the opportunity to drop the superfluous punctuation?


Looking through this code, no callers check for any errors, and the only
failure paths are in a debug region.  The function really ought to
become void, or at the very least, switch to -EINVAL to avoid aliasing
-EPERM.

Furthermore, in all(?) cases that it fails, we'll leak the domain
allocated page, which at the very best means the VM is going to hit
memory limit problems.  i.e. nothing good can come.

Both failures are internal memory handling errors in Xen, so the least
invasive option is probably to switch to ASSERT() (for the alignment
check), and ASSERT_UNREACHABLE() here.  That also addresses the issue
that these printk()'s aren't ratelimited, and used within several loops.

>              return -1;
>          }
>      }
> @@ -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));

(For a future change,) this is double scrubbing in most cases.

~Andrew

>  
>      /* 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. */
>
>