Re: [PATCH 2/8] xen: Use typesafe gfn/mfn in guest_physmap_* helpers

[Julien Grall <julien.grall@arm.com>]

Hi Jan,

On 14/06/16 13:56, Jan Beulich wrote:
>>>> On 14.06.16 at 14:07, <julien.grall@arm.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -665,21 +665,21 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
>>   }
>>
>>   int
>> -guest_physmap_remove_page(struct domain *d, unsigned long gfn,
>> -                          unsigned long mfn, unsigned int page_order)
>> +guest_physmap_remove_page(struct domain *d, gfn_t gfn,
>> +                          mfn_t mfn, unsigned int page_order)
>>   {
>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>       int rc;
>>       gfn_lock(p2m, gfn, page_order);
>> -    rc = p2m_remove_page(p2m, gfn, mfn, page_order);
>> +    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order);
>>       gfn_unlock(p2m, gfn, page_order);
>>       return rc;
>>   }
>>
>> -int
>> -guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>> -                        unsigned long mfn, unsigned int page_order,
>> -                        p2m_type_t t)
>> +static int
>> +__guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>
> At the very least only a single underscore please.
>
>> +                          unsigned long mfn, unsigned int page_order,
>> +                          p2m_type_t t)
>>   {
>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>       unsigned long i, ogfn;
>> @@ -838,6 +838,13 @@ out:
>>       return rc;
>>   }
>>
>> +/* XXX: To be removed when __guest_physmap_add_entry will use typesafe */
>
> But not just because of this (misleading) comment I really wonder
> what the point here is: Is it really that much more intrusive to
> change the function right away instead of introducing a wrapper?

I though it was intrusive because there are multiple place using adding 
a value to the mfn/gfn.

It might be less intrusive with your suggestion to add mfn_* wrappers 
for common operation. I will give another look.

> (The comment is misleading because __guest_physmap_add_entry
> shouldn't survive after the conversion to proper types, i.e. it is
> not the function below which is supposed to get removed.)
>
>> +int
>> +guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
>> +                        unsigned int page_order, p2m_type_t t)
>> +{
>> +    return __guest_physmap_add_entry(d, gfn_x(gfn), mfn_x(mfn), page_order, t);
>> +}
>
> Perhaps a better (wrapper-less) approach (if full conversion is indeed
> beyond scope) would be to simply rename the function parameters
> and have local variables named "gfn" and "mfn"?
>
>> @@ -2785,7 +2792,8 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>>                       unsigned long gpfn, domid_t foreigndom)
>>   {
>>       p2m_type_t p2mt, p2mt_prev;
>> -    unsigned long prev_mfn, mfn;
>> +    mfn_t prev_mfn;
>> +    unsigned long mfn;
>
> This looks to make things more inconsistent rather than more consistent.

The usage of the variable in p2m_add_foreign seems limited to a couple 
of place. I will use mfn_t for it too.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel