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

[Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>]

Andrew Cooper <andrew.cooper3@xxxxxxxxxx> writes:

> On 26/06/15 16:17, 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'.
>>
>> 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 v2:
>> - Instead of adding start_ prefix on ARM remove it on x86 [Jan Beulich,
>>   Ian Campbell, Razvan Cojocaru]
>>
>> Changes since v1:
>> - This patch is a successor of '[PATCH] x86/mm: use existing 'pfn' in
>>   p2m_get_mem_access', instead of fixing gfn_lock/gfn_unlock arguments we do
>>   s/pfn/gfn/g for both p2m_get_mem_access/p2m_set_mem_access [Andrew Cooper,
>>   Jan Beulich]
>>
>> P.S.
>> - The patch was compile-tested on x86 only.
>> ---
>>  xen/arch/arm/p2m.c           | 12 ++++++------
>>  xen/arch/x86/mm/p2m.c        | 24 ++++++++++++------------
>>  xen/include/xen/p2m-common.h | 12 ++++++------
>>  3 files changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 903fa3f..54c238c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1709,9 +1709,9 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
>> const struct npfec npfec)
>>  
>>  /*
>>   * Set access type for a region of pfns.
>> - * If start_pfn == -1ul, sets the default access type.
>> + * If gfn == -1ul, sets the default access type.
>>   */
>> -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
>> +long p2m_set_mem_access(struct domain *d, unsigned long gfn, uint32_t nr,
>>                          uint32_t start, uint32_t mask, xenmem_access_t 
>> access)
>>  {
>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> @@ -1752,14 +1752,14 @@ long p2m_set_mem_access(struct domain *d, unsigned 
>> long pfn, uint32_t nr,
>>      p2m->mem_access_enabled = true;
>>  
>>      /* If request to set default access. */
>> -    if ( pfn == ~0ul )
>> +    if ( gfn == ~0ul )
>
> Please here and everywhere else in the patch, use INVALID_GFN instead of
> ~0 or -1.
>
> With those changes and the comment style from Razvan,
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> However, if you feel up to it, it would be fantastic if you could
> substitute unsigned long for gfn_t, per the justification in e758ed1 and
> 177bd5f.  c/s 24036a5 is an example of a different API I fixed up in a
> similar way.

Sure, will do v4.

>
> ~Andrew

-- 
  Vitaly

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