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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2020 20:18, Andrew Cooper wrote:
> 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.

Yeah, changed along these lines.

>>  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>

Thanks.

> , 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.)

Oh, indeed. I did look for obvious style issues but didn't spot this
(still quite obvious) one. I've done so and also added blank lines
between the case blocks.

Jan