Re: [PATCH 08/15] xen/x86: p2m: Use typesafe gfn for the P2M callbacks get_entry and set_entry

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

Hi,

On 13/09/2017 20:08, Razvan Cojocaru wrote:
>> @@ -142,7 +142,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>                            vm_event_request_t **req_ptr)
>>  {
>>      struct vcpu *v = current;
>> -    unsigned long gfn = gpa >> PAGE_SHIFT;
>> +    gfn_t gfn = gaddr_to_gfn(gpa);
>>      struct domain *d = v->domain;
>>      struct p2m_domain *p2m = NULL;
>>      mfn_t mfn;
>> @@ -215,7 +215,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>          *req_ptr = req;
>>
>>          req->reason = VM_EVENT_REASON_MEM_ACCESS;
>> -        req->u.mem_access.gfn = gfn;
>> +        req->u.mem_access.gfn = gfn_x(gfn);
>>          req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
>>          if ( npfec.gla_valid )
>>          {
>> @@ -247,7 +247,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>      unsigned long gfn_l = gfn_x(gfn);
>
> I'd drop gfn_l completely here and just use gfn_x(gfn) in the two places
> below where it's used. It would get rid of a line of code in this
> function. But I wouldn't hold the patch over this, so if nobody else has
> comments it's fine as is.

Technically more clean-up could be done in every functions. If you 
noticed it, I kept the changes minimal because this is already a boring 
but necessary job and I really don't want to explore all the possible 
clean-up in each single function... Feel free to do more clean-up.

>>      int rc;
>>
>> -    mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
>> +    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>
>>      /* Check host p2m if no valid entry in alternate */
>>      if ( !mfn_valid(mfn) )
>> @@ -264,16 +264,16 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>          if ( page_order != PAGE_ORDER_4K )
>>          {
>>              unsigned long mask = ~((1UL << page_order) - 1);
>> -            unsigned long gfn2_l = gfn_l & mask;
>> +            gfn_t gfn2 = _gfn(gfn_l & mask);
>>              mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>
>> -            rc = ap2m->set_entry(ap2m, gfn2_l, mfn2, page_order, t, old_a, 1);
>> +            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>              if ( rc )
>>                  return rc;
>>          }
>>      }
>>
>> -    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
>> +    return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
>>                           (current->domain != d));
>
> If you need to send V2, could you please also indent the last line so
> that '(' is under the 'a' in 'ap2m'? You don't have to, but it'd be a
> coding style fix coming in on top of this patch.

Will do.

>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index 23c0518733..dff214cf7b 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -674,11 +674,12 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>>   * Returns: 0 for success, -errno for failure
>>   */
>>  static int
>> -ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>> +ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
>>                unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
>>                int sve)
>>  {
>>      ept_entry_t *table, *ept_entry = NULL;
>> +    unsigned long gfn = gfn_x(gfn_t);
>
> Should we call this gfn_l, as done above?
>
>>      unsigned long gfn_remainder = gfn;
>>      unsigned int i, target = order / EPT_TABLE_ORDER;
>>      unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? (gfn | mfn_x(mfn)) : gfn;
>> @@ -910,11 +911,12 @@ out:
>>
>>  /* Read ept p2m entries */
>>  static mfn_t ept_get_entry(struct p2m_domain *p2m,
>> -                           unsigned long gfn, p2m_type_t *t, p2m_access_t* a,
>> +                           gfn_t gfn_t, p2m_type_t *t, p2m_access_t* a,
>>                             p2m_query_t q, unsigned int *page_order,
>>                             bool_t *sve)
>>  {
>>      ept_entry_t *table = map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
>> +    unsigned long gfn = gfn_x(gfn_t);
>
> gfn_l here too?

[...]

>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>> index 0e63d6ed11..57878b1886 100644
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -479,12 +479,13 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>
>>  /* Returns: 0 for success, -errno for failure */
>>  static int
>> -p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>> +p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
>>                   unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma,
>>                   int sve)
>>  {
>>      /* XXX -- this might be able to be faster iff current->domain == d */
>>      void *table;
>> +    unsigned long gfn = gfn_x(gfn_t);
>
> gfn_l?
>
>>      unsigned long i, gfn_remainder = gfn;
>>      l1_pgentry_t *p2m_entry, entry_content;
>>      /* Intermediate table to free if we're replacing it with a superpage. */
>> @@ -731,11 +732,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>>  }
>>
>>  static mfn_t
>> -p2m_pt_get_entry(struct p2m_domain *p2m, unsigned long gfn,
>> +p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_t,
>>                   p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
>>                   unsigned int *page_order, bool_t *sve)
>>  {
>>      mfn_t mfn;
>> +    unsigned long gfn = gfn_x(gfn_t);
>
> gfn_l?

If you do that you would have to rename all gfn to gfn_l in the function 
increase a bit more the size of this boring patch. The right solution 
would be to make the rest of the code typesafe, but that's not my plan. 
I have done enough typesafe clean-up myself, so I will leave it with

unsigned long gfn = gfn_x(gfn_);

Until someone step up and decide to do further clean-up.

Cheers,

-- 
Julien Grall

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