Re: [Xen-devel] [PATCH v2 1/2] arm/mem_access: adjust check_and_get_page to not rely on current

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Dec 12, 2016 at 4:46 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 12/12/16 07:47, Tamas K Lengyel wrote:
>>
>>
>>
>> On Dec 12, 2016 00:42, "Jan Beulich" <JBeulich@xxxxxxxx
>> <mailto:JBeulich@xxxxxxxx>> wrote:
>>
>>     >>> On 09.12.16 at 20:59, <tamas.lengyel@xxxxxxxxxxxx
>>     <mailto:tamas.lengyel@xxxxxxxxxxxx>> wrote:
>>     > --- a/xen/arch/arm/p2m.c
>>     > +++ b/xen/arch/arm/p2m.c
>>     > @@ -1461,7 +1461,8 @@ 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(vaddr_t gva, unsigned long flag,
>>     > +                                  const struct vcpu *v)
>>     >  {
>>     >      long rc;
>>     >      paddr_t ipa;
>>     > @@ -1470,7 +1471,7 @@ p2m_mem_access_check_and_get_page(vaddr_t
>>     gva, unsigned long flag)
>>     >      xenmem_access_t xma;
>>     >      p2m_type_t t;
>>     >      struct page_info *page = NULL;
>>     > -    struct p2m_domain *p2m = &current->domain->arch.p2m;
>>     > +    struct p2m_domain *p2m = &v->domain->arch.p2m;
>>     >
>>     >      rc = gva_to_ipa(gva, &ipa, flag);
>>     >      if ( rc < 0 )
>>     > @@ -1482,7 +1483,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(v->domain, gfn, &xma);
>>     >      if ( rc < 0 )
>>     >          goto err;
>>     >
>>     > @@ -1546,7 +1547,7 @@ p2m_mem_access_check_and_get_page(vaddr_t
>>     gva, unsigned long flag)
>>     >
>>     >      page = mfn_to_page(mfn_x(mfn));
>>     >
>>     > -    if ( unlikely(!get_page(page, current->domain)) )
>>     > +    if ( unlikely(!get_page(page, v->domain)) )
>>     >          page = NULL;
>>     >
>>     >  err:
>>
>>     Looking at these changes, wouldn't it be more reasonable to hand
>>     the function a struct domain *?
>>
>>
>> In its current state it might be but I believe with altp2m we will need
>> the vcpu struct here anyway.
>
>
> Not only the altp2m. I keep mentioning a bigger issue, but it seems been
> ignored every time...

I wouldn't say ignored. I think this is the first description with
details that I see of the problem you passingly mentioned previously
and I still don't have a way to reproduce it.

>
> The translation VA to IPA (guest physical address) is done using hardware.
> If the underlying memory of the stage-1 page table is protected, so the
> translation will fail. Given that this function is used in hypercall to
> retrieve the page associated to a buffer, it means that it will not be
> possible to do hypercall when the page table used to find the buffer IPA has
> not been touched.

This function specifically works around the case where the page of the
guest pagetable is not accessible due to mem_access, when the hardware
based lookup doesn't work. This function checks what the fault was,
checks the page type and the mem_access rights to determine whether
the fault was legit, or if it was due to mem_access. If it was
mem_access it gets the page without involving the hardware. I'm not
following what you describe afterwards regarding the buffer and what
you mean by "the buffer IPA has not been touched". Care to elaborate?

Thanks,
Tamas

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