[Xen-devel,for-next,06/16] xen/arm: Extend copy_to_guest to support copying from/to guest physical address

Message ID 20171123183210.12045-7-julien.grall@linaro.org
State New
Headers show
Series
  • xen/arm: Stage-2 handling cleanup
Related show

Commit Message

Julien Grall Nov. 23, 2017, 6:32 p.m.
The only differences between copy_to_guest and access_guest_memory_by_ipa are:
    - The latter does not support copying data crossing page boundary
    - The former is copying from/to guest VA whilst the latter from
    guest PA

copy_to_guest can easily be extended to support copying from/to guest
physical address. For that a new bit is used to tell whether linear
address or ipa is been used.

Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
This also has the benefits to extend the use of it, it is now possible
to copy data crossing page boundary.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/guestcopy.c | 86 ++++++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 47 deletions(-)

Comments

Stefano Stabellini Dec. 6, 2017, 1:22 a.m. | #1
On Thu, 23 Nov 2017, Julien Grall wrote:
> The only differences between copy_to_guest and access_guest_memory_by_ipa are:
>     - The latter does not support copying data crossing page boundary
>     - The former is copying from/to guest VA whilst the latter from
>     guest PA
> 
> copy_to_guest can easily be extended to support copying from/to guest
> physical address. For that a new bit is used to tell whether linear
> address or ipa is been used.
> 
> Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
> This also has the benefits to extend the use of it, it is now possible
> to copy data crossing page boundary.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Ah! This is the reason why previous patches were not using vaddr_t. It
makes sense now. May I suggest we use something different from paddr_t
in copy_guest for addr type? I don't think is correct to specify addr as
paddr_t when it could be vaddr_t; in the future we could have type
checks on them.

I suggest we specify it as u64, but if you have a better idea go for it.


> ---
>  xen/arch/arm/guestcopy.c | 86 ++++++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 487f5ab82d..be53bee559 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -8,6 +8,31 @@
>  #define COPY_flush_dcache   (1U << 0)
>  #define COPY_from_guest     (0U << 1)
>  #define COPY_to_guest       (1U << 1)
> +#define COPY_ipa            (0U << 2)
> +#define COPY_linear         (1U << 2)
> +
> +static struct page_info *translate_get_page(struct vcpu *v, paddr_t addr,
> +                                            bool linear, bool write)
> +{
> +    p2m_type_t p2mt;
> +    struct page_info *page;
> +
> +    if ( linear )
> +        return get_page_from_gva(v, addr, write ? GV2M_WRITE : GV2M_READ);
> +
> +    page = get_page_from_gfn(v->domain, paddr_to_pfn(addr), &p2mt, P2M_ALLOC);
> +
> +    if ( !page )
> +        return NULL;
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        return NULL;
> +    }
> +
> +    return page;
> +}
>  
>  static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>                                  struct vcpu *v, unsigned int flags)
> @@ -21,8 +46,8 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>          struct page_info *page;
>  
> -        page = get_page_from_gva(v, addr,
> -                                 (flags & COPY_to_guest) ? GV2M_WRITE : GV2M_READ);
> +        page = translate_get_page(v, addr, flags & COPY_linear,
> +                                  flags & COPY_to_guest);
>          if ( page == NULL )
>              return len;
>  
> @@ -63,73 +88,40 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>  unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>  {
>      return copy_guest((void *)from, (unsigned long)to, len,
> -                      current, COPY_to_guest);
> +                      current, COPY_to_guest | COPY_linear);
>  }
>  
>  unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>                                               unsigned len)
>  {
>      return copy_guest((void *)from, (unsigned long)to, len,
> -                      current, COPY_to_guest | COPY_flush_dcache);
> +                      current, COPY_to_guest | COPY_flush_dcache | COPY_linear);
>  }
>  
>  unsigned long raw_clear_guest(void *to, unsigned len)
>  {
> -    return copy_guest(NULL, (unsigned long)to, len, current, COPY_to_guest);
> +    return copy_guest(NULL, (unsigned long)to, len, current,
> +                      COPY_to_guest | COPY_linear);
>  }
>  
>  unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
>  {
> -    return copy_guest(to, (unsigned long)from, len, current, COPY_from_guest);
> +    return copy_guest(to, (unsigned long)from, len, current,
> +                      COPY_from_guest | COPY_linear);
>  }
>  
> -/*
> - * Temporarily map one physical guest page and copy data to or from it.
> - * The data to be copied cannot cross a page boundary.
> - */
>  int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
>                                 uint32_t size, bool is_write)
>  {
> -    struct page_info *page;
> -    uint64_t offset = gpa & ~PAGE_MASK;  /* Offset within the mapped page */
> -    p2m_type_t p2mt;
> -    void *p;
> -
> -    /* Do not cross a page boundary. */
> -    if ( size > (PAGE_SIZE - offset) )
> -    {
> -        printk(XENLOG_G_ERR "d%d: guestcopy: memory access crosses page boundary.\n",
> -               d->domain_id);
> -        return -EINVAL;
> -    }

I don't know if we necessarely care about this, but with this change
this error path goes away. Do we want to keep it?


> -    page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
> -    if ( !page )
> -    {
> -        printk(XENLOG_G_ERR "d%d: guestcopy: failed to get table entry.\n",
> -               d->domain_id);
> -        return -EINVAL;
> -    }
> -
> -    if ( !p2m_is_ram(p2mt) )
> -    {
> -        put_page(page);
> -        printk(XENLOG_G_ERR "d%d: guestcopy: guest memory should be RAM.\n",
> -               d->domain_id);
> -        return -EINVAL;
> -    }
> +    unsigned long left;
> +    int flags = COPY_ipa;
>  
> -    p = __map_domain_page(page);
> +    flags |= is_write ? COPY_to_guest : COPY_from_guest;
>  
> -    if ( is_write )
> -        memcpy(p + offset, buf, size);
> -    else
> -        memcpy(buf, p + offset, size);
> +    /* P2M is shared between all vCPUs, so the vcpu used does not matter. */
> +    left = copy_guest(buf, gpa, size, d->vcpu[0], flags);

fair enough, then why not use current?


> -    unmap_domain_page(p);
> -    put_page(page);
> -
> -    return 0;
> +    return (!left) ? 0 : -EINVAL;
>  }
>  
>  /*
> -- 
> 2.11.0
>
Stefano Stabellini Dec. 6, 2017, 1:24 a.m. | #2
On Tue, 5 Dec 2017, Stefano Stabellini wrote:
> On Thu, 23 Nov 2017, Julien Grall wrote:
> > The only differences between copy_to_guest and access_guest_memory_by_ipa are:
> >     - The latter does not support copying data crossing page boundary
> >     - The former is copying from/to guest VA whilst the latter from
> >     guest PA
> > 
> > copy_to_guest can easily be extended to support copying from/to guest
> > physical address. For that a new bit is used to tell whether linear
> > address or ipa is been used.
> > 
> > Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
> > This also has the benefits to extend the use of it, it is now possible
> > to copy data crossing page boundary.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Ah! This is the reason why previous patches were not using vaddr_t. It
> makes sense now. May I suggest we use something different from paddr_t
> in copy_guest for addr type? I don't think is correct to specify addr as
> paddr_t when it could be vaddr_t; in the future we could have type
> checks on them.
> 
> I suggest we specify it as u64, but if you have a better idea go for it.
> 
> 
> > ---
> >  xen/arch/arm/guestcopy.c | 86 ++++++++++++++++++++++--------------------------
> >  1 file changed, 39 insertions(+), 47 deletions(-)
> > 
> > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> > index 487f5ab82d..be53bee559 100644
> > --- a/xen/arch/arm/guestcopy.c
> > +++ b/xen/arch/arm/guestcopy.c
> > @@ -8,6 +8,31 @@
> >  #define COPY_flush_dcache   (1U << 0)
> >  #define COPY_from_guest     (0U << 1)
> >  #define COPY_to_guest       (1U << 1)
> > +#define COPY_ipa            (0U << 2)
> > +#define COPY_linear         (1U << 2)
> > +
> > +static struct page_info *translate_get_page(struct vcpu *v, paddr_t addr,
> > +                                            bool linear, bool write)
> > +{
> > +    p2m_type_t p2mt;
> > +    struct page_info *page;
> > +
> > +    if ( linear )
> > +        return get_page_from_gva(v, addr, write ? GV2M_WRITE : GV2M_READ);
> > +
> > +    page = get_page_from_gfn(v->domain, paddr_to_pfn(addr), &p2mt, P2M_ALLOC);
> > +
> > +    if ( !page )
> > +        return NULL;
> > +
> > +    if ( !p2m_is_ram(p2mt) )
> > +    {
> > +        put_page(page);
> > +        return NULL;
> > +    }
> > +
> > +    return page;
> > +}
> >  
> >  static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
> >                                  struct vcpu *v, unsigned int flags)
> > @@ -21,8 +46,8 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
> >          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
> >          struct page_info *page;
> >  
> > -        page = get_page_from_gva(v, addr,
> > -                                 (flags & COPY_to_guest) ? GV2M_WRITE : GV2M_READ);
> > +        page = translate_get_page(v, addr, flags & COPY_linear,
> > +                                  flags & COPY_to_guest);
> >          if ( page == NULL )
> >              return len;
> >  
> > @@ -63,73 +88,40 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
> >  unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
> >  {
> >      return copy_guest((void *)from, (unsigned long)to, len,
> > -                      current, COPY_to_guest);
> > +                      current, COPY_to_guest | COPY_linear);
> >  }
> >  
> >  unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
> >                                               unsigned len)
> >  {
> >      return copy_guest((void *)from, (unsigned long)to, len,
> > -                      current, COPY_to_guest | COPY_flush_dcache);
> > +                      current, COPY_to_guest | COPY_flush_dcache | COPY_linear);
> >  }
> >  
> >  unsigned long raw_clear_guest(void *to, unsigned len)
> >  {
> > -    return copy_guest(NULL, (unsigned long)to, len, current, COPY_to_guest);
> > +    return copy_guest(NULL, (unsigned long)to, len, current,
> > +                      COPY_to_guest | COPY_linear);
> >  }
> >  
> >  unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
> >  {
> > -    return copy_guest(to, (unsigned long)from, len, current, COPY_from_guest);
> > +    return copy_guest(to, (unsigned long)from, len, current,
> > +                      COPY_from_guest | COPY_linear);
> >  }
> >  
> > -/*
> > - * Temporarily map one physical guest page and copy data to or from it.
> > - * The data to be copied cannot cross a page boundary.
> > - */
> >  int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
> >                                 uint32_t size, bool is_write)
> >  {
> > -    struct page_info *page;
> > -    uint64_t offset = gpa & ~PAGE_MASK;  /* Offset within the mapped page */
> > -    p2m_type_t p2mt;
> > -    void *p;
> > -
> > -    /* Do not cross a page boundary. */
> > -    if ( size > (PAGE_SIZE - offset) )
> > -    {
> > -        printk(XENLOG_G_ERR "d%d: guestcopy: memory access crosses page boundary.\n",
> > -               d->domain_id);
> > -        return -EINVAL;
> > -    }
> 
> I don't know if we necessarely care about this, but with this change
> this error path goes away. Do we want to keep it?
> 
> 
> > -    page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
> > -    if ( !page )
> > -    {
> > -        printk(XENLOG_G_ERR "d%d: guestcopy: failed to get table entry.\n",
> > -               d->domain_id);
> > -        return -EINVAL;
> > -    }
> > -
> > -    if ( !p2m_is_ram(p2mt) )
> > -    {
> > -        put_page(page);
> > -        printk(XENLOG_G_ERR "d%d: guestcopy: guest memory should be RAM.\n",
> > -               d->domain_id);
> > -        return -EINVAL;
> > -    }
> > +    unsigned long left;
> > +    int flags = COPY_ipa;
> >  
> > -    p = __map_domain_page(page);
> > +    flags |= is_write ? COPY_to_guest : COPY_from_guest;
> >  
> > -    if ( is_write )
> > -        memcpy(p + offset, buf, size);
> > -    else
> > -        memcpy(buf, p + offset, size);
> > +    /* P2M is shared between all vCPUs, so the vcpu used does not matter. */
> > +    left = copy_guest(buf, gpa, size, d->vcpu[0], flags);
> 
> fair enough, then why not use current?

Because it could be called from another domain, of course. Makes sense.

 
> > -    unmap_domain_page(p);
> > -    put_page(page);
> > -
> > -    return 0;
> > +    return (!left) ? 0 : -EINVAL;
> >  }
> >  
> >  /*
> > -- 
> > 2.11.0
> > 
>
Julien Grall Dec. 6, 2017, 12:22 p.m. | #3
Hi Stefano,

On 12/06/2017 01:22 AM, Stefano Stabellini wrote:
> On Thu, 23 Nov 2017, Julien Grall wrote:
>> The only differences between copy_to_guest and access_guest_memory_by_ipa are:
>>      - The latter does not support copying data crossing page boundary
>>      - The former is copying from/to guest VA whilst the latter from
>>      guest PA
>>
>> copy_to_guest can easily be extended to support copying from/to guest
>> physical address. For that a new bit is used to tell whether linear
>> address or ipa is been used.
>>
>> Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
>> This also has the benefits to extend the use of it, it is now possible
>> to copy data crossing page boundary.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Ah! This is the reason why previous patches were not using vaddr_t. It
> makes sense now. May I suggest we use something different from paddr_t
> in copy_guest for addr type? I don't think is correct to specify addr as
> paddr_t when it could be vaddr_t; in the future we could have type
> checks on them.
> 
> I suggest we specify it as u64, but if you have a better idea go for it.

We should not use more u64 in the code. uint64_t could be a solution but 
even that, I don't see the reason. How are you sure the physical address 
will always fit in 64-bit?

On the other side, very likely vaddr_t will fit in paddr_t. So paddr_t 
is the right way to go for me.

> 
> 
>> ---
>>   xen/arch/arm/guestcopy.c | 86 ++++++++++++++++++++++--------------------------
>>   1 file changed, 39 insertions(+), 47 deletions(-)
>>
>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>> index 487f5ab82d..be53bee559 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -8,6 +8,31 @@
>>   #define COPY_flush_dcache   (1U << 0)
>>   #define COPY_from_guest     (0U << 1)
>>   #define COPY_to_guest       (1U << 1)
>> +#define COPY_ipa            (0U << 2)
>> +#define COPY_linear         (1U << 2)
>> +
>> +static struct page_info *translate_get_page(struct vcpu *v, paddr_t addr,
>> +                                            bool linear, bool write)
>> +{
>> +    p2m_type_t p2mt;
>> +    struct page_info *page;
>> +
>> +    if ( linear )
>> +        return get_page_from_gva(v, addr, write ? GV2M_WRITE : GV2M_READ);
>> +
>> +    page = get_page_from_gfn(v->domain, paddr_to_pfn(addr), &p2mt, P2M_ALLOC);
>> +
>> +    if ( !page )
>> +        return NULL;
>> +
>> +    if ( !p2m_is_ram(p2mt) )
>> +    {
>> +        put_page(page);
>> +        return NULL;
>> +    }
>> +
>> +    return page;
>> +}
>>   
>>   static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>>                                   struct vcpu *v, unsigned int flags)
>> @@ -21,8 +46,8 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>>           unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>>           struct page_info *page;
>>   
>> -        page = get_page_from_gva(v, addr,
>> -                                 (flags & COPY_to_guest) ? GV2M_WRITE : GV2M_READ);
>> +        page = translate_get_page(v, addr, flags & COPY_linear,
>> +                                  flags & COPY_to_guest);
>>           if ( page == NULL )
>>               return len;
>>   
>> @@ -63,73 +88,40 @@ static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
>>   unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
>>   {
>>       return copy_guest((void *)from, (unsigned long)to, len,
>> -                      current, COPY_to_guest);
>> +                      current, COPY_to_guest | COPY_linear);
>>   }
>>   
>>   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>>                                                unsigned len)
>>   {
>>       return copy_guest((void *)from, (unsigned long)to, len,
>> -                      current, COPY_to_guest | COPY_flush_dcache);
>> +                      current, COPY_to_guest | COPY_flush_dcache | COPY_linear);
>>   }
>>   
>>   unsigned long raw_clear_guest(void *to, unsigned len)
>>   {
>> -    return copy_guest(NULL, (unsigned long)to, len, current, COPY_to_guest);
>> +    return copy_guest(NULL, (unsigned long)to, len, current,
>> +                      COPY_to_guest | COPY_linear);
>>   }
>>   
>>   unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
>>   {
>> -    return copy_guest(to, (unsigned long)from, len, current, COPY_from_guest);
>> +    return copy_guest(to, (unsigned long)from, len, current,
>> +                      COPY_from_guest | COPY_linear);
>>   }
>>   
>> -/*
>> - * Temporarily map one physical guest page and copy data to or from it.
>> - * The data to be copied cannot cross a page boundary.
>> - */
>>   int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
>>                                  uint32_t size, bool is_write)
>>   {
>> -    struct page_info *page;
>> -    uint64_t offset = gpa & ~PAGE_MASK;  /* Offset within the mapped page */
>> -    p2m_type_t p2mt;
>> -    void *p;
>> -
>> -    /* Do not cross a page boundary. */
>> -    if ( size > (PAGE_SIZE - offset) )
>> -    {
>> -        printk(XENLOG_G_ERR "d%d: guestcopy: memory access crosses page boundary.\n",
>> -               d->domain_id);
>> -        return -EINVAL;
>> -    }
> 
> I don't know if we necessarely care about this, but with this change
> this error path goes away. Do we want to keep it?

There are strictly no reason to prevent cross-boundary for IPA when we 
do support them for VA.

Cheers,
Stefano Stabellini Dec. 7, 2017, 11:01 p.m. | #4
On Wed, 6 Dec 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/06/2017 01:22 AM, Stefano Stabellini wrote:
> > On Thu, 23 Nov 2017, Julien Grall wrote:
> > > The only differences between copy_to_guest and access_guest_memory_by_ipa
> > > are:
> > >      - The latter does not support copying data crossing page boundary
> > >      - The former is copying from/to guest VA whilst the latter from
> > >      guest PA
> > > 
> > > copy_to_guest can easily be extended to support copying from/to guest
> > > physical address. For that a new bit is used to tell whether linear
> > > address or ipa is been used.
> > > 
> > > Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
> > > This also has the benefits to extend the use of it, it is now possible
> > > to copy data crossing page boundary.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > Ah! This is the reason why previous patches were not using vaddr_t. It
> > makes sense now. May I suggest we use something different from paddr_t
> > in copy_guest for addr type? I don't think is correct to specify addr as
> > paddr_t when it could be vaddr_t; in the future we could have type
> > checks on them.
> > 
> > I suggest we specify it as u64, but if you have a better idea go for it.
> 
> We should not use more u64 in the code. uint64_t could be a solution but even
> that, I don't see the reason. How are you sure the physical address will
> always fit in 64-bit?
> 
> On the other side, very likely vaddr_t will fit in paddr_t. So paddr_t is the
> right way to go for me.

What about introducing xaddr_t?
Or at least:

  static struct page_info *translate_get_page(struct vcpu *v, paddr_t /*or vaddr_t */ addr

 
> > 
> > > ---
> > >   xen/arch/arm/guestcopy.c | 86
> > > ++++++++++++++++++++++--------------------------
> > >   1 file changed, 39 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> > > index 487f5ab82d..be53bee559 100644
> > > --- a/xen/arch/arm/guestcopy.c
> > > +++ b/xen/arch/arm/guestcopy.c
> > > @@ -8,6 +8,31 @@
> > >   #define COPY_flush_dcache   (1U << 0)
> > >   #define COPY_from_guest     (0U << 1)
> > >   #define COPY_to_guest       (1U << 1)
> > > +#define COPY_ipa            (0U << 2)
> > > +#define COPY_linear         (1U << 2)
> > > +
> > > +static struct page_info *translate_get_page(struct vcpu *v, paddr_t addr,
> > > +                                            bool linear, bool write)
> > > +{
> > > +    p2m_type_t p2mt;
> > > +    struct page_info *page;
> > > +
> > > +    if ( linear )
> > > +        return get_page_from_gva(v, addr, write ? GV2M_WRITE :
> > > GV2M_READ);
> > > +
> > > +    page = get_page_from_gfn(v->domain, paddr_to_pfn(addr), &p2mt,
> > > P2M_ALLOC);
> > > +
> > > +    if ( !page )
> > > +        return NULL;
> > > +
> > > +    if ( !p2m_is_ram(p2mt) )
> > > +    {
> > > +        put_page(page);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return page;
> > > +}
> > >     static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int
> > > len,
> > >                                   struct vcpu *v, unsigned int flags)
> > > @@ -21,8 +46,8 @@ static unsigned long copy_guest(void *buf, paddr_t addr,
> > > unsigned int len,
> > >           unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
> > >           struct page_info *page;
> > >   -        page = get_page_from_gva(v, addr,
> > > -                                 (flags & COPY_to_guest) ? GV2M_WRITE :
> > > GV2M_READ);
> > > +        page = translate_get_page(v, addr, flags & COPY_linear,
> > > +                                  flags & COPY_to_guest);
> > >           if ( page == NULL )
> > >               return len;
> > >   @@ -63,73 +88,40 @@ static unsigned long copy_guest(void *buf, paddr_t
> > > addr, unsigned int len,
> > >   unsigned long raw_copy_to_guest(void *to, const void *from, unsigned
> > > len)
> > >   {
> > >       return copy_guest((void *)from, (unsigned long)to, len,
> > > -                      current, COPY_to_guest);
> > > +                      current, COPY_to_guest | COPY_linear);
> > >   }
> > >     unsigned long raw_copy_to_guest_flush_dcache(void *to, const void
> > > *from,
> > >                                                unsigned len)
> > >   {
> > >       return copy_guest((void *)from, (unsigned long)to, len,
> > > -                      current, COPY_to_guest | COPY_flush_dcache);
> > > +                      current, COPY_to_guest | COPY_flush_dcache |
> > > COPY_linear);
> > >   }
> > >     unsigned long raw_clear_guest(void *to, unsigned len)
> > >   {
> > > -    return copy_guest(NULL, (unsigned long)to, len, current,
> > > COPY_to_guest);
> > > +    return copy_guest(NULL, (unsigned long)to, len, current,
> > > +                      COPY_to_guest | COPY_linear);
> > >   }
> > >     unsigned long raw_copy_from_guest(void *to, const void __user *from,
> > > unsigned len)
> > >   {
> > > -    return copy_guest(to, (unsigned long)from, len, current,
> > > COPY_from_guest);
> > > +    return copy_guest(to, (unsigned long)from, len, current,
> > > +                      COPY_from_guest | COPY_linear);
> > >   }
> > >   -/*
> > > - * Temporarily map one physical guest page and copy data to or from it.
> > > - * The data to be copied cannot cross a page boundary.
> > > - */
> > >   int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
> > >                                  uint32_t size, bool is_write)
> > >   {
> > > -    struct page_info *page;
> > > -    uint64_t offset = gpa & ~PAGE_MASK;  /* Offset within the mapped page
> > > */
> > > -    p2m_type_t p2mt;
> > > -    void *p;
> > > -
> > > -    /* Do not cross a page boundary. */
> > > -    if ( size > (PAGE_SIZE - offset) )
> > > -    {
> > > -        printk(XENLOG_G_ERR "d%d: guestcopy: memory access crosses page
> > > boundary.\n",
> > > -               d->domain_id);
> > > -        return -EINVAL;
> > > -    }
> > 
> > I don't know if we necessarely care about this, but with this change
> > this error path goes away. Do we want to keep it?
> 
> There are strictly no reason to prevent cross-boundary for IPA when we do
> support them for VA.
> 
> Cheers,
> 
> -- 
> Julien Grall
>
Julien Grall Dec. 8, 2017, 3:24 p.m. | #5
Hi Stefano,

On 07/12/17 23:01, Stefano Stabellini wrote:
> On Wed, 6 Dec 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 12/06/2017 01:22 AM, Stefano Stabellini wrote:
>>> On Thu, 23 Nov 2017, Julien Grall wrote:
>>>> The only differences between copy_to_guest and access_guest_memory_by_ipa
>>>> are:
>>>>       - The latter does not support copying data crossing page boundary
>>>>       - The former is copying from/to guest VA whilst the latter from
>>>>       guest PA
>>>>
>>>> copy_to_guest can easily be extended to support copying from/to guest
>>>> physical address. For that a new bit is used to tell whether linear
>>>> address or ipa is been used.
>>>>
>>>> Lastly access_guest_memory_by_ipa is reimplemented using copy_to_guest.
>>>> This also has the benefits to extend the use of it, it is now possible
>>>> to copy data crossing page boundary.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> Ah! This is the reason why previous patches were not using vaddr_t. It
>>> makes sense now. May I suggest we use something different from paddr_t
>>> in copy_guest for addr type? I don't think is correct to specify addr as
>>> paddr_t when it could be vaddr_t; in the future we could have type
>>> checks on them.
>>>
>>> I suggest we specify it as u64, but if you have a better idea go for it.
>>
>> We should not use more u64 in the code. uint64_t could be a solution but even
>> that, I don't see the reason. How are you sure the physical address will
>> always fit in 64-bit?
>>
>> On the other side, very likely vaddr_t will fit in paddr_t. So paddr_t is the
>> right way to go for me.
> 
> What about introducing xaddr_t?

I would prefer uint64_t in that case. xaddr_t is quite confusing to read 
and could be misused.

> Or at least:
> 
>    static struct page_info *translate_get_page(struct vcpu *v, paddr_t /*or vaddr_t */ addr

I can do that as well. What's your preference?

Cheers,
Stefano Stabellini Dec. 8, 2017, 9:13 p.m. | #6
On Fri, 8 Dec 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/12/17 23:01, Stefano Stabellini wrote:
> > On Wed, 6 Dec 2017, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 12/06/2017 01:22 AM, Stefano Stabellini wrote:
> > > > On Thu, 23 Nov 2017, Julien Grall wrote:
> > > > > The only differences between copy_to_guest and
> > > > > access_guest_memory_by_ipa
> > > > > are:
> > > > >       - The latter does not support copying data crossing page
> > > > > boundary
> > > > >       - The former is copying from/to guest VA whilst the latter from
> > > > >       guest PA
> > > > > 
> > > > > copy_to_guest can easily be extended to support copying from/to guest
> > > > > physical address. For that a new bit is used to tell whether linear
> > > > > address or ipa is been used.
> > > > > 
> > > > > Lastly access_guest_memory_by_ipa is reimplemented using
> > > > > copy_to_guest.
> > > > > This also has the benefits to extend the use of it, it is now possible
> > > > > to copy data crossing page boundary.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > > > 
> > > > Ah! This is the reason why previous patches were not using vaddr_t. It
> > > > makes sense now. May I suggest we use something different from paddr_t
> > > > in copy_guest for addr type? I don't think is correct to specify addr as
> > > > paddr_t when it could be vaddr_t; in the future we could have type
> > > > checks on them.
> > > > 
> > > > I suggest we specify it as u64, but if you have a better idea go for it.
> > > 
> > > We should not use more u64 in the code. uint64_t could be a solution but
> > > even
> > > that, I don't see the reason. How are you sure the physical address will
> > > always fit in 64-bit?
> > > 
> > > On the other side, very likely vaddr_t will fit in paddr_t. So paddr_t is
> > > the
> > > right way to go for me.
> > 
> > What about introducing xaddr_t?
> 
> I would prefer uint64_t in that case. xaddr_t is quite confusing to read and
> could be misused.
> 
> > Or at least:
> > 
> >    static struct page_info *translate_get_page(struct vcpu *v, paddr_t /*or
> > vaddr_t */ addr
> 
> I can do that as well. What's your preference?

I prefer uint64_t

Patch

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 487f5ab82d..be53bee559 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -8,6 +8,31 @@ 
 #define COPY_flush_dcache   (1U << 0)
 #define COPY_from_guest     (0U << 1)
 #define COPY_to_guest       (1U << 1)
+#define COPY_ipa            (0U << 2)
+#define COPY_linear         (1U << 2)
+
+static struct page_info *translate_get_page(struct vcpu *v, paddr_t addr,
+                                            bool linear, bool write)
+{
+    p2m_type_t p2mt;
+    struct page_info *page;
+
+    if ( linear )
+        return get_page_from_gva(v, addr, write ? GV2M_WRITE : GV2M_READ);
+
+    page = get_page_from_gfn(v->domain, paddr_to_pfn(addr), &p2mt, P2M_ALLOC);
+
+    if ( !page )
+        return NULL;
+
+    if ( !p2m_is_ram(p2mt) )
+    {
+        put_page(page);
+        return NULL;
+    }
+
+    return page;
+}
 
 static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
                                 struct vcpu *v, unsigned int flags)
@@ -21,8 +46,8 @@  static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
         unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
         struct page_info *page;
 
-        page = get_page_from_gva(v, addr,
-                                 (flags & COPY_to_guest) ? GV2M_WRITE : GV2M_READ);
+        page = translate_get_page(v, addr, flags & COPY_linear,
+                                  flags & COPY_to_guest);
         if ( page == NULL )
             return len;
 
@@ -63,73 +88,40 @@  static unsigned long copy_guest(void *buf, paddr_t addr, unsigned int len,
 unsigned long raw_copy_to_guest(void *to, const void *from, unsigned len)
 {
     return copy_guest((void *)from, (unsigned long)to, len,
-                      current, COPY_to_guest);
+                      current, COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
                                              unsigned len)
 {
     return copy_guest((void *)from, (unsigned long)to, len,
-                      current, COPY_to_guest | COPY_flush_dcache);
+                      current, COPY_to_guest | COPY_flush_dcache | COPY_linear);
 }
 
 unsigned long raw_clear_guest(void *to, unsigned len)
 {
-    return copy_guest(NULL, (unsigned long)to, len, current, COPY_to_guest);
+    return copy_guest(NULL, (unsigned long)to, len, current,
+                      COPY_to_guest | COPY_linear);
 }
 
 unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
 {
-    return copy_guest(to, (unsigned long)from, len, current, COPY_from_guest);
+    return copy_guest(to, (unsigned long)from, len, current,
+                      COPY_from_guest | COPY_linear);
 }
 
-/*
- * Temporarily map one physical guest page and copy data to or from it.
- * The data to be copied cannot cross a page boundary.
- */
 int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
                                uint32_t size, bool is_write)
 {
-    struct page_info *page;
-    uint64_t offset = gpa & ~PAGE_MASK;  /* Offset within the mapped page */
-    p2m_type_t p2mt;
-    void *p;
-
-    /* Do not cross a page boundary. */
-    if ( size > (PAGE_SIZE - offset) )
-    {
-        printk(XENLOG_G_ERR "d%d: guestcopy: memory access crosses page boundary.\n",
-               d->domain_id);
-        return -EINVAL;
-    }
-
-    page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
-    if ( !page )
-    {
-        printk(XENLOG_G_ERR "d%d: guestcopy: failed to get table entry.\n",
-               d->domain_id);
-        return -EINVAL;
-    }
-
-    if ( !p2m_is_ram(p2mt) )
-    {
-        put_page(page);
-        printk(XENLOG_G_ERR "d%d: guestcopy: guest memory should be RAM.\n",
-               d->domain_id);
-        return -EINVAL;
-    }
+    unsigned long left;
+    int flags = COPY_ipa;
 
-    p = __map_domain_page(page);
+    flags |= is_write ? COPY_to_guest : COPY_from_guest;
 
-    if ( is_write )
-        memcpy(p + offset, buf, size);
-    else
-        memcpy(buf, p + offset, size);
+    /* P2M is shared between all vCPUs, so the vcpu used does not matter. */
+    left = copy_guest(buf, gpa, size, d->vcpu[0], flags);
 
-    unmap_domain_page(p);
-    put_page(page);
-
-    return 0;
+    return (!left) ? 0 : -EINVAL;
 }
 
 /*