Re: [Xen-devel] [PATCH v2 15/25] arm/altp2m: Extend __p2m_lookup.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/04/2016 02:04 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> This commit extends the functionality of the function "__p2m_lookup".
>> The function "__p2m_lookup" performs the necessary steps gathering
>> information concerning memory attributes and the p2m table level a
>> specific gfn is mapped to. Thus, we extend the function's prototype so
>> that the caller can optionally get these information for further
>> processing.
>>
>> Also, we extend the function prototype of "__p2m_lookup" to hold an
>> argument of type "struct p2m_domain*", as we need to distinguish between
>> the host's p2m and different altp2m views. While doing so, we needed to
>> extend the function's prototypes of the following functions:
>>
>> * __p2m_get_mem_access
>> * p2m_mem_access_and_get_page
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>>  xen/arch/arm/p2m.c | 59
>> ++++++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 39 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 784f8da..326e343 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -168,15 +168,22 @@ void p2m_flush_tlb(struct p2m_domain *p2m)
>>      }
>>  }
>>
>> +static int __p2m_get_mem_access(struct p2m_domain*, gfn_t,
>> xenmem_access_t*);
>> +
>>  /*
>>   * Lookup the MFN corresponding to a domain's GFN.
>>   *
>>   * There are no processor functions to do a stage 2 only lookup
>> therefore we
>>   * do a a software walk.
>> + *
>> + * Optionally, __p2m_lookup takes arguments to provide information
>> about the
>> + * p2m type, the p2m table level the paddr is mapped to, associated mem
>> + * attributes, and memory access rights.
>>   */
>> -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>> +static mfn_t __p2m_lookup(struct p2m_domain *p2m, gfn_t gfn,
>> p2m_type_t *t,
>> +                          unsigned int *level, unsigned int *mattr,
>> +                          xenmem_access_t *xma)
>
> Please give a look at my series "xen/arm: Rework the P2M code to
> follow break-before-make sequence" [1], it will expose a clean
> interface to retrieve all the necessary information.
>

Alright, thank you.

>>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>>  {
>>      mfn_t ret;
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>
>>      p2m_read_lock(p2m);
>> -    ret = __p2m_lookup(d, gfn, t);
>> +    ret = __p2m_lookup(p2m, gfn, t, NULL, NULL, NULL);
>>      p2m_read_unlock(p2m);
>>
>>      return ret;
>> @@ -479,10 +499,9 @@ static int p2m_create_table(struct p2m_domain
>> *p2m, lpae_t *entry,
>>      return 0;
>>  }
>>
>> -static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>> +static int __p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
>>                                  xenmem_access_t *access)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>      void *i;
>>      unsigned int index;
>>
>> @@ -525,7 +544,7 @@ static int __p2m_get_mem_access(struct domain *d,
>> gfn_t gfn,
>>           * No setting was found in the Radix tree. Check if the
>>           * entry exists in the page-tables.
>>           */
>> -        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
>> +        mfn_t mfn = __p2m_lookup(p2m, gfn, NULL, NULL, NULL, NULL);
>>
>>          if ( mfn_eq(mfn, INVALID_MFN) )
>>              return -ESRCH;
>> @@ -1519,7 +1538,7 @@ mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
>>   * we indeed found a conflicting mem_access setting.
>>   */
>>  static struct page_info*
>> -p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>> +p2m_mem_access_check_and_get_page(struct p2m_domain *p2m, vaddr_t
>> gva, unsigned long flag)
>>  {
>>      long rc;
>>      paddr_t ipa;
>> @@ -1539,7 +1558,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>> unsigned long flag)
>>       * We do this first as this is faster in the default case when no
>>       * permission is set on the page.
>>       */
>> -    rc = __p2m_get_mem_access(current->domain, gfn, &xma);
>> +    rc = __p2m_get_mem_access(p2m, gfn, &xma);
>>      if ( rc < 0 )
>>          goto err;
>>
>> @@ -1588,7 +1607,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
>> unsigned long flag)
>>       * We had a mem_access permission limiting the access, but the
>> page type
>>       * could also be limiting, so we need to check that as well.
>>       */
>> -    mfn = __p2m_lookup(current->domain, gfn, &t);
>> +    mfn = __p2m_lookup(p2m, gfn, &t, NULL, NULL, NULL);
>>      if ( mfn_eq(mfn, INVALID_MFN) )
>>          goto err;
>>
>> @@ -1671,7 +1690,7 @@ struct page_info *get_page_from_gva(struct vcpu
>> *v, vaddr_t va,
>>
>>  err:
>>      if ( !page && p2m->mem_access_enabled )
>> -        page = p2m_mem_access_check_and_get_page(va, flags);
>> +        page = p2m_mem_access_check_and_get_page(p2m, va, flags);
>
> p2m_mem_access_check_and_get_page should take a vCPU in parameter and
> not a p2m.
>
> I know the function is already buggy because it assumes that the vCPU
> == current. But let's not broken more this function.
>

Alright.

>>
>>      p2m_read_unlock(p2m);
>>
>> @@ -1948,7 +1967,7 @@ int p2m_get_mem_access(struct domain *d, gfn_t
>> gfn,
>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>
>>      p2m_read_lock(p2m);
>> -    ret = __p2m_get_mem_access(d, gfn, access);
>> +    ret = __p2m_get_mem_access(p2m, gfn, access);
>>      p2m_read_unlock(p2m);
>>
>>      return ret;
>>
>
> Regards,
>
> [1]
> https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg02952.html
>

Best regards,
~Sergej


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