Re: [PATCH 2/6] x86/mm: p2m_add_foreign() is HVM-only

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2020 16:26, Jan Beulich wrote:
> This is together with its only caller, xenmem_add_to_physmap_one().

I can't parse this sentence.  Perhaps "... as is it's only caller," as a
follow-on from the subject sentence.

>  Move
> the latter next to p2m_add_foreign(), allowing this one to become static
> at the same time.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, although...

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2639,7 +2646,114 @@ int p2m_add_foreign(struct domain *tdom,
>      return rc;
>  }
>  
> -#ifdef CONFIG_HVM
> +int xenmem_add_to_physmap_one(
> +    struct domain *d,
> +    unsigned int space,
> +    union add_to_physmap_extra extra,
> +    unsigned long idx,
> +    gfn_t gpfn)
> +{
> +    struct page_info *page = NULL;
> +    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
> +    mfn_t prev_mfn;
> +    int rc = 0;
> +    mfn_t mfn = INVALID_MFN;
> +    p2m_type_t p2mt;
> +
> +    switch ( space )
> +    {
> +        case XENMAPSPACE_shared_info:
> +            if ( idx == 0 )
> +                mfn = virt_to_mfn(d->shared_info);
> +            break;
> +        case XENMAPSPACE_grant_table:
> +            rc = gnttab_map_frame(d, idx, gpfn, &mfn);
> +            if ( rc )
> +                return rc;
> +            break;
> +        case XENMAPSPACE_gmfn:
> +        {
> +            p2m_type_t p2mt;
> +
> +            gfn = idx;
> +            mfn = get_gfn_unshare(d, gfn, &p2mt);
> +            /* If the page is still shared, exit early */
> +            if ( p2m_is_shared(p2mt) )
> +            {
> +                put_gfn(d, gfn);
> +                return -ENOMEM;
> +            }
> +            page = get_page_from_mfn(mfn, d);
> +            if ( unlikely(!page) )
> +                mfn = INVALID_MFN;
> +            break;
> +        }
> +        case XENMAPSPACE_gmfn_foreign:
> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
> +        default:
> +            break;

... seeing as the function is moving wholesale, can we at least correct
the indention, to save yet another large churn in the future?  (If it
were me, I'd go as far as deleting the default case as well.)

~Andrew