Re: [Xen-devel] [PATCH v4] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/07/15 11:48, George Dunlap wrote:
> On 06/29/2015 04:45 PM, Vitaly Kuznetsov wrote:
>> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as 
>> input.
>>
>> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
>> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
>>
>> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
>> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
>> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
>>
>> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on 
>> ARM)
>> interfaces to using gft_t instead of unsigned long and update all users of
>> these functions.
>>
>> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed 
>> to
>> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
>> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
>> second argument.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>> Changes since v3:
>> - Comment codying style fix [Razvan Cojocaru]
>> - Use INVALID_GFN instead of ~0 and -1 [Andrew Cooper]
>> - Convert p2m_get_mem_access/p2m_set_mem_access interfaces to using gfn_t
>>   [Andrew Cooper]
> But you missed a change...
>
>> @@ -1600,9 +1600,11 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned 
>> long gla,
>>      return (p2ma == p2m_access_n2rwx);
>>  }
>>  
>> -/* Set access type for a region of pfns.
>> - * If start_pfn == -1ul, sets the default access type */
>> -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
>> +/*
>> + * Set access type for a region of gfns.
>> + * If gfn == INVALID_GFN, sets the default access type.
>> + */
>> +long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>                          uint32_t start, uint32_t mask, xenmem_access_t 
>> access)
>>  {
>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> @@ -1638,18 +1640,19 @@ long p2m_set_mem_access(struct domain *d, unsigned 
>> long pfn, uint32_t nr,
>>          return -EINVAL;
>>      }
>>  
>> -    /* If request to set default access */
>> -    if ( pfn == ~0ul )
>> +    /* If request to set default access. */
>> +    if ( gfn_x(gfn) == INVALID_GFN )
>>      {
>>          p2m->default_access = a;
>>          return 0;
>>      }
>>  
>>      p2m_lock(p2m);
>> -    for ( pfn += start; nr > start; ++pfn )
>> +    for ( gfn = _gfn(gfn_x(gfn) + start); nr > start;
>> +          gfn = _gfn(gfn_x(gfn) + 1) )
> Now in p2m_set_mem_access(), rather than just using an unsigned long in
> the loop iterating over gfns, you do this thing where you convert gfn_t
> to unsigned long, add one, and then convert it back to gfn_t again.
>
> I can't see any comments in v3 that suggest you doing that, and it seems
> a bit clunky.  Is that really necessary?  Wouldn't it be better to
> declare a local variable?
>
> I'm not strongly opinionated on this one, it just seems a bit strange.
>
> Everything else looks good, thanks.

Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
typesafety don't allow for simply adding 1 to a typesafe variable.

In a cases like this, I think it is acceptable to keep a unsigned long
shadow variable and manipulate it is a plain integer.  The eventual
_gfn() required to pass it further down the callchain will help to
visually re-enforce the appropriate type.

After all, the entire point of these typesafes are to try and avoid
accidentally mixing up the different address spaces, but a function
which takes a typesafe, loops over a subset and passes the same typesafe
further down can probably be trusted to DTRT, catching errors at review
time. 

Off the top of my head, the only functions which would normally expect
to mix and match the typesafes are the pagetable walking ones.

~Andrew

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