Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Aug 2020, Simon Leiner wrote:
> On 20.08.20 20:35, Stefano Stabellini wrote:
> > Thank for the well-written analysis of the problem. The following 
> should
> > work to translate the virtual address properly in xenbus_grant_ring:
> > 
> >     if (is_vmalloc_addr(vaddr))
> >             page = vmalloc_to_page(vaddr);
> >     else
> >             page = virt_to_page(vaddr);
> 
> Great idea, thanks! I modified it lightly (see below) and it did indeed 
> work! I'm wondering though whether the check for vmalloc'd addresses 
> should be included directly in the ARM implementation of virt_to_gfn. 
> As far as I see, this should not break anything, but might impose a 
> small performance overhead in cases where it is known for sure that we 
> are dealing with directly mapped memory. What do you think?

Thanks for testing!

We could ask the relevant maintainers for feedback, but I think it is
probably intended that virt_to_gfn doesn't work on vmalloc addresses.
That's because vmalloc addresses are not typically supposed to be used
like that.



> diff --git a/drivers/xen/xenbus/xenbus_client.c 
> b/drivers/xen/xenbus/xenbus_client.c
> index e17ca8156171..d7a97f946f2f 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -344,6 +344,21 @@ static void xenbus_switch_fatal(struct xenbus_device 
> *dev, int depth, int err,
>         __xenbus_switch_state(dev, XenbusStateClosing, 1);
>  }
>  
> +/**
> + * vaddr_to_gfn
> + * @vaddr: any virtual address
> + * 
> + * Returns the guest frame number (GFN) corresponding to vaddr.
> + */
> +static inline unsigned long vaddr_to_gfn(void *vaddr)
> +{
> +   if (is_vmalloc_addr(vaddr)) {
> +       return pfn_to_gfn(vmalloc_to_pfn(vaddr));
> +   } else {
> +       return virt_to_gfn(vaddr);
> +   }
> +}
> +

For the same reason as above, I would rather have the check inside
xenbus_grant_ring, rather than above in a generic function:

- if this is a special case the check should be inside xenbus_grant_ring
- if this is not a special case, then the fix should be in virt_to_gfn
  as you mentioned

either way, I wouldn't introduce this function here

Juergen, do you agree with this?


>  /**
>   * xenbus_grant_ring
>   * @dev: xenbus device
> @@ -364,7 +379,7 @@ int xenbus_grant_ring(struct xenbus_device *dev, void 
> *vaddr,
>  
>     for (i = 0; i < nr_pages; i++) {
>         err = gnttab_grant_foreign_access(dev->otherend_id,
> -                         virt_to_gfn(vaddr), 0);
> +                         vaddr_to_gfn(vaddr), 0);
>         if (err < 0) {
>             xenbus_dev_fatal(dev, err,
>                      "granting access to ring page");