| Message ID | 20180601114132.22596-6-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Allow mappings for DMA backed buffers if grant table module > supports such: this extends grant device to not only map buffers > made of balloon pages, but also from buffers allocated with > dma_alloc_xxx. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > drivers/xen/gntdev.c | 99 ++++++++++++++++++++++++++++++++++++++- > include/uapi/xen/gntdev.h | 15 ++++++ > 2 files changed, 112 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index bd56653b9bbc..9813fc440c70 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -37,6 +37,9 @@ > #include <linux/slab.h> > #include <linux/highmem.h> > #include <linux/refcount.h> > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > +#include <linux/of_device.h> > +#endif > > #include <xen/xen.h> > #include <xen/grant_table.h> > @@ -72,6 +75,11 @@ struct gntdev_priv { > struct mutex lock; > struct mm_struct *mm; > struct mmu_notifier mn; > + > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + /* Device for which DMA memory is allocated. */ > + struct device *dma_dev; > +#endif > }; > > struct unmap_notify { > @@ -96,10 +104,27 @@ struct grant_map { > struct gnttab_unmap_grant_ref *kunmap_ops; > struct page **pages; > unsigned long pages_vm_start; > + > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + /* > + * If dmabuf_vaddr is not NULL then this mapping is backed by DMA > + * capable memory. > + */ > + > + struct device *dma_dev; > + /* Flags used to create this DMA buffer: GNTDEV_DMA_FLAG_XXX. */ > + int dma_flags; > + void *dma_vaddr; > + dma_addr_t dma_bus_addr; > + /* This is required for gnttab_dma_{alloc|free}_pages. */ How about /* Needed to avoid allocation in gnttab_dma_free_pages(). */ > + xen_pfn_t *frames; > +#endif > }; > > static int unmap_grant_pages(struct grant_map *map, int offset, int pages); > > +static struct miscdevice gntdev_miscdev; > + > /* ------------------------------------------------------------------ */ > > static void gntdev_print_maps(struct gntdev_priv *priv, > @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map) > if (map == NULL) > return; > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + if (map->dma_vaddr) { > + struct gnttab_dma_alloc_args args; > + > + args.dev = map->dma_dev; > + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; > + args.nr_pages = map->count; > + args.pages = map->pages; > + args.frames = map->frames; > + args.vaddr = map->dma_vaddr; > + args.dev_bus_addr = map->dma_bus_addr; > + > + gnttab_dma_free_pages(&args); > + } else > +#endif > if (map->pages) > gnttab_free_pages(map->count, map->pages); > + > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + kfree(map->frames); > +#endif Can this be done under if (map->dma_vaddr) ? In other words, is it possible for dma_vaddr to be NULL and still have unallocated frames pointer? > kfree(map->pages); > kfree(map->grants); > kfree(map->map_ops); > @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map) > kfree(map); > } > > -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) > +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count, > + int dma_flags) > { > struct grant_map *add; > int i; > @@ -155,6 +200,37 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) > NULL == add->pages) > goto err; > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + add->dma_flags = dma_flags; > + > + /* > + * Check if this mapping is requested to be backed > + * by a DMA buffer. > + */ > + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) { > + struct gnttab_dma_alloc_args args; > + > + add->frames = kcalloc(count, sizeof(add->frames[0]), > + GFP_KERNEL); > + if (!add->frames) > + goto err; > + > + /* Remember the device, so we can free DMA memory. */ > + add->dma_dev = priv->dma_dev; > + > + args.dev = priv->dma_dev; > + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; > + args.nr_pages = count; > + args.pages = add->pages; > + args.frames = add->frames; > + > + if (gnttab_dma_alloc_pages(&args)) > + goto err; > + > + add->dma_vaddr = args.vaddr; > + add->dma_bus_addr = args.dev_bus_addr; > + } else > +#endif > if (gnttab_alloc_pages(count, add->pages)) > goto err; > > @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map *map) > map->unmap_ops[i].handle = map->map_ops[i].handle; > if (use_ptemod) > map->kunmap_ops[i].handle = map->kmap_ops[i].handle; > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + else if (map->dma_vaddr) { > + unsigned long mfn; > + > + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); Not pfn_to_mfn()? -boris > + map->unmap_ops[i].dev_bus_addr = __pfn_to_phys(mfn); > + } > +#endif > } > return err; > } > @@ -548,6 +632,17 @@ static int gntdev_open(struct inode *inode, struct file *flip) > } > > flip->private_data = priv; > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > + priv->dma_dev = gntdev_miscdev.this_device; > + > + /* > + * The device is not spawn from a device tree, so arch_setup_dma_ops > + * is not called, thus leaving the device with dummy DMA ops. > + * Fix this call of_dma_configure() with a NULL node to set > + * default DMA ops. > + */ > + of_dma_configure(priv->dma_dev, NULL); > +#endif > pr_debug("priv %p\n", priv); > > return 0; > @@ -589,7 +684,7 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv, > return -EINVAL; > > err = -ENOMEM; > - map = gntdev_alloc_map(priv, op.count); > + map = gntdev_alloc_map(priv, op.count, 0 /* This is not a dma-buf. */); > if (!map) > return err; > > diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h > index 6d1163456c03..4b9d498a31d4 100644 > --- a/include/uapi/xen/gntdev.h > +++ b/include/uapi/xen/gntdev.h > @@ -200,4 +200,19 @@ struct ioctl_gntdev_grant_copy { > /* Send an interrupt on the indicated event channel */ > #define UNMAP_NOTIFY_SEND_EVENT 0x2 > > +/* > + * Flags to be used while requesting memory mapping's backing storage > + * to be allocated with DMA API. > + */ > + > +/* > + * The buffer is backed with memory allocated with dma_alloc_wc. > + */ > +#define GNTDEV_DMA_FLAG_WC (1 << 0) > + > +/* > + * The buffer is backed with memory allocated with dma_alloc_coherent. > + */ > +#define GNTDEV_DMA_FLAG_COHERENT (1 << 1) > + > #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: > On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Allow mappings for DMA backed buffers if grant table module >> supports such: this extends grant device to not only map buffers >> made of balloon pages, but also from buffers allocated with >> dma_alloc_xxx. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> drivers/xen/gntdev.c | 99 ++++++++++++++++++++++++++++++++++++++- >> include/uapi/xen/gntdev.h | 15 ++++++ >> 2 files changed, 112 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index bd56653b9bbc..9813fc440c70 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c >> @@ -37,6 +37,9 @@ >> #include <linux/slab.h> >> #include <linux/highmem.h> >> #include <linux/refcount.h> >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> +#include <linux/of_device.h> >> +#endif >> >> #include <xen/xen.h> >> #include <xen/grant_table.h> >> @@ -72,6 +75,11 @@ struct gntdev_priv { >> struct mutex lock; >> struct mm_struct *mm; >> struct mmu_notifier mn; >> + >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + /* Device for which DMA memory is allocated. */ >> + struct device *dma_dev; >> +#endif >> }; >> >> struct unmap_notify { >> @@ -96,10 +104,27 @@ struct grant_map { >> struct gnttab_unmap_grant_ref *kunmap_ops; >> struct page **pages; >> unsigned long pages_vm_start; >> + >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + /* >> + * If dmabuf_vaddr is not NULL then this mapping is backed by DMA >> + * capable memory. >> + */ >> + >> + struct device *dma_dev; >> + /* Flags used to create this DMA buffer: GNTDEV_DMA_FLAG_XXX. */ >> + int dma_flags; >> + void *dma_vaddr; >> + dma_addr_t dma_bus_addr; >> + /* This is required for gnttab_dma_{alloc|free}_pages. */ > How about > > /* Needed to avoid allocation in gnttab_dma_free_pages(). */ > Ok >> + xen_pfn_t *frames; >> +#endif >> }; >> >> static int unmap_grant_pages(struct grant_map *map, int offset, int pages); >> >> +static struct miscdevice gntdev_miscdev; >> + >> /* ------------------------------------------------------------------ */ >> >> static void gntdev_print_maps(struct gntdev_priv *priv, >> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map) >> if (map == NULL) >> return; >> >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + if (map->dma_vaddr) { >> + struct gnttab_dma_alloc_args args; >> + >> + args.dev = map->dma_dev; >> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; >> + args.nr_pages = map->count; >> + args.pages = map->pages; >> + args.frames = map->frames; >> + args.vaddr = map->dma_vaddr; >> + args.dev_bus_addr = map->dma_bus_addr; >> + >> + gnttab_dma_free_pages(&args); >> + } else >> +#endif >> if (map->pages) >> gnttab_free_pages(map->count, map->pages); >> + >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + kfree(map->frames); >> +#endif > > Can this be done under if (map->dma_vaddr) ? > In other words, is it > possible for dma_vaddr to be NULL and still have unallocated frames pointer? It is possible to have vaddr == NULL and frames != NULL as we allocate frames outside of gnttab_dma_alloc_pages which may fail. Calling kfree on NULL pointer is safe, so I see no reason to change this code. > >> kfree(map->pages); >> kfree(map->grants); >> kfree(map->map_ops); >> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map) >> kfree(map); >> } >> >> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) >> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count, >> + int dma_flags) >> { >> struct grant_map *add; >> int i; >> @@ -155,6 +200,37 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) >> NULL == add->pages) >> goto err; >> >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + add->dma_flags = dma_flags; >> + >> + /* >> + * Check if this mapping is requested to be backed >> + * by a DMA buffer. >> + */ >> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) { >> + struct gnttab_dma_alloc_args args; >> + >> + add->frames = kcalloc(count, sizeof(add->frames[0]), >> + GFP_KERNEL); >> + if (!add->frames) >> + goto err; >> + >> + /* Remember the device, so we can free DMA memory. */ >> + add->dma_dev = priv->dma_dev; >> + >> + args.dev = priv->dma_dev; >> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; >> + args.nr_pages = count; >> + args.pages = add->pages; >> + args.frames = add->frames; >> + >> + if (gnttab_dma_alloc_pages(&args)) >> + goto err; >> + >> + add->dma_vaddr = args.vaddr; >> + add->dma_bus_addr = args.dev_bus_addr; >> + } else >> +#endif >> if (gnttab_alloc_pages(count, add->pages)) >> goto err; >> >> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map *map) >> map->unmap_ops[i].handle = map->map_ops[i].handle; >> if (use_ptemod) >> map->kunmap_ops[i].handle = map->kmap_ops[i].handle; >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + else if (map->dma_vaddr) { >> + unsigned long mfn; >> + >> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > > Not pfn_to_mfn()? I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] and [2] Thus, drivers/xen/gntdev.c:408:10: error: implicit declaration of function ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); So, I'll keep __pfn_to_mfn > > > -boris Thank you, Oleksandr >> + map->unmap_ops[i].dev_bus_addr = __pfn_to_phys(mfn); >> + } >> +#endif >> } >> return err; >> } >> @@ -548,6 +632,17 @@ static int gntdev_open(struct inode *inode, struct file *flip) >> } >> >> flip->private_data = priv; >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + priv->dma_dev = gntdev_miscdev.this_device; >> + >> + /* >> + * The device is not spawn from a device tree, so arch_setup_dma_ops >> + * is not called, thus leaving the device with dummy DMA ops. >> + * Fix this call of_dma_configure() with a NULL node to set >> + * default DMA ops. >> + */ >> + of_dma_configure(priv->dma_dev, NULL); >> +#endif >> pr_debug("priv %p\n", priv); >> >> return 0; >> @@ -589,7 +684,7 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv, >> return -EINVAL; >> >> err = -ENOMEM; >> - map = gntdev_alloc_map(priv, op.count); >> + map = gntdev_alloc_map(priv, op.count, 0 /* This is not a dma-buf. */); >> if (!map) >> return err; >> >> diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h >> index 6d1163456c03..4b9d498a31d4 100644 >> --- a/include/uapi/xen/gntdev.h >> +++ b/include/uapi/xen/gntdev.h >> @@ -200,4 +200,19 @@ struct ioctl_gntdev_grant_copy { >> /* Send an interrupt on the indicated event channel */ >> #define UNMAP_NOTIFY_SEND_EVENT 0x2 >> >> +/* >> + * Flags to be used while requesting memory mapping's backing storage >> + * to be allocated with DMA API. >> + */ >> + >> +/* >> + * The buffer is backed with memory allocated with dma_alloc_wc. >> + */ >> +#define GNTDEV_DMA_FLAG_WC (1 << 0) >> + >> +/* >> + * The buffer is backed with memory allocated with dma_alloc_coherent. >> + */ >> +#define GNTDEV_DMA_FLAG_COHERENT (1 << 1) >> + >> #endif /* __LINUX_PUBLIC_GNTDEV_H__ */ > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel [1] https://elixir.bootlin.com/linux/v4.17/ident/pfn_to_mfn [2] https://elixir.bootlin.com/linux/v4.17/ident/__pfn_to_mfn
On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote: > On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: >> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map) >> if (map == NULL) >> return; >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + if (map->dma_vaddr) { >> + struct gnttab_dma_alloc_args args; >> + >> + args.dev = map->dma_dev; >> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; >> + args.nr_pages = map->count; >> + args.pages = map->pages; >> + args.frames = map->frames; >> + args.vaddr = map->dma_vaddr; >> + args.dev_bus_addr = map->dma_bus_addr; >> + >> + gnttab_dma_free_pages(&args); >> + } else >> +#endif >> if (map->pages) >> gnttab_free_pages(map->count, map->pages); >> + >> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> + kfree(map->frames); >> +#endif >> >> Can this be done under if (map->dma_vaddr) ? > >> In other words, is it >> possible for dma_vaddr to be NULL and still have unallocated frames >> pointer? > It is possible to have vaddr == NULL and frames != NULL as we > allocate frames outside of gnttab_dma_alloc_pages which > may fail. Calling kfree on NULL pointer is safe, I am not questioning safety of the code, I would like avoid another ifdef. > so > I see no reason to change this code. >> >>> kfree(map->pages); >>> kfree(map->grants); >>> kfree(map->map_ops); >>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map) >>> kfree(map); >>> } >>> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv >>> *priv, int count) >>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, >>> int count, >>> + int dma_flags) >>> { >>> struct grant_map *add; >>> int i; >>> @@ -155,6 +200,37 @@ static struct grant_map >>> *gntdev_alloc_map(struct gntdev_priv *priv, int count) >>> NULL == add->pages) >>> goto err; >>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>> + add->dma_flags = dma_flags; >>> + >>> + /* >>> + * Check if this mapping is requested to be backed >>> + * by a DMA buffer. >>> + */ >>> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) { >>> + struct gnttab_dma_alloc_args args; >>> + >>> + add->frames = kcalloc(count, sizeof(add->frames[0]), >>> + GFP_KERNEL); >>> + if (!add->frames) >>> + goto err; >>> + >>> + /* Remember the device, so we can free DMA memory. */ >>> + add->dma_dev = priv->dma_dev; >>> + >>> + args.dev = priv->dma_dev; >>> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>> + args.nr_pages = count; >>> + args.pages = add->pages; >>> + args.frames = add->frames; >>> + >>> + if (gnttab_dma_alloc_pages(&args)) >>> + goto err; >>> + >>> + add->dma_vaddr = args.vaddr; >>> + add->dma_bus_addr = args.dev_bus_addr; >>> + } else >>> +#endif >>> if (gnttab_alloc_pages(count, add->pages)) >>> goto err; >>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map >>> *map) >>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>> if (use_ptemod) >>> map->kunmap_ops[i].handle = map->kmap_ops[i].handle; >>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>> + else if (map->dma_vaddr) { >>> + unsigned long mfn; >>> + >>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >> >> Not pfn_to_mfn()? > I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] and [2] > Thus, > > drivers/xen/gntdev.c:408:10: error: implicit declaration of function > ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > > So, I'll keep __pfn_to_mfn How will this work on non-PV x86? -boris
On 06/07/2018 12:19 AM, Boris Ostrovsky wrote: > On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote: >> On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: >>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >>> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map) >>> if (map == NULL) >>> return; >>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC *Option 1: kfree(map->frames);* >>> + if (map->dma_vaddr) { >>> + struct gnttab_dma_alloc_args args; >>> + >>> + args.dev = map->dma_dev; >>> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>> + args.nr_pages = map->count; >>> + args.pages = map->pages; >>> + args.frames = map->frames; >>> + args.vaddr = map->dma_vaddr; >>> + args.dev_bus_addr = map->dma_bus_addr; >>> + >>> + gnttab_dma_free_pages(&args); *Option 2: kfree(map->frames);* >>> + } else >>> +#endif >>> if (map->pages) >>> gnttab_free_pages(map->count, map->pages); >>> + >>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>> + kfree(map->frames); >>> +#endif >>> >>> Can this be done under if (map->dma_vaddr) ? >>> In other words, is it >>> possible for dma_vaddr to be NULL and still have unallocated frames >>> pointer? >> It is possible to have vaddr == NULL and frames != NULL as we >> allocate frames outside of gnttab_dma_alloc_pages which >> may fail. Calling kfree on NULL pointer is safe, > > I am not questioning safety of the code, I would like avoid another ifdef. Ah, I now understand, so you are asking if we can have that kfree(map->frames); in the place *Option 2* I marked above. Unfortunately no: map->frames is allocated before we try to allocate DMA memory, e.g. before dma_vaddr is set: [...] add->frames = kcalloc(count, sizeof(add->frames[0]), GFP_KERNEL); if (!add->frames) goto err; [...] if (gnttab_dma_alloc_pages(&args)) goto err; add->dma_vaddr = args.vaddr; [...] err: gntdev_free_map(add); So, it is possible to enter gntdev_free_map with frames != NULL and dma_vaddr == NULL. Option 1 above cannot be used as map->frames is needed for gnttab_dma_free_pages(&args); and Option 2 cannot be used as frames != NULL and dma_vaddr == NULL. Thus, I think that unfortunately we need that #ifdef. Option 3 below can also be considered, but that seems to be not good as we free resources in different places which looks inconsistent. Sorry if I'm still missing your point. > >> so >> I see no reason to change this code. >>>> kfree(map->pages); >>>> kfree(map->grants); >>>> kfree(map->map_ops); >>>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map) >>>> kfree(map); >>>> } >>>> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv >>>> *priv, int count) >>>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, >>>> int count, >>>> + int dma_flags) >>>> { >>>> struct grant_map *add; >>>> int i; >>>> @@ -155,6 +200,37 @@ static struct grant_map >>>> *gntdev_alloc_map(struct gntdev_priv *priv, int count) >>>> NULL == add->pages) >>>> goto err; >>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>> + add->dma_flags = dma_flags; >>>> + >>>> + /* >>>> + * Check if this mapping is requested to be backed >>>> + * by a DMA buffer. >>>> + */ >>>> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) { >>>> + struct gnttab_dma_alloc_args args; >>>> + >>>> + add->frames = kcalloc(count, sizeof(add->frames[0]), >>>> + GFP_KERNEL); >>>> + if (!add->frames) >>>> + goto err; >>>> + >>>> + /* Remember the device, so we can free DMA memory. */ >>>> + add->dma_dev = priv->dma_dev; >>>> + >>>> + args.dev = priv->dma_dev; >>>> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>>> + args.nr_pages = count; >>>> + args.pages = add->pages; >>>> + args.frames = add->frames; >>>> + >>>> + if (gnttab_dma_alloc_pages(&args)) *Option 3: kfree(map->frames);* >>>> + goto err; >>>> + >>>> + add->dma_vaddr = args.vaddr; >>>> + add->dma_bus_addr = args.dev_bus_addr; >>>> + } else >>>> +#endif >>>> if (gnttab_alloc_pages(count, add->pages)) >>>> goto err; >>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map >>>> *map) >>>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>>> if (use_ptemod) >>>> map->kunmap_ops[i].handle = map->kmap_ops[i].handle; >>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>> + else if (map->dma_vaddr) { >>>> + unsigned long mfn; >>>> + >>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>> Not pfn_to_mfn()? >> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] and [2] >> Thus, >> >> drivers/xen/gntdev.c:408:10: error: implicit declaration of function >> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >> >> So, I'll keep __pfn_to_mfn > > How will this work on non-PV x86? So, you mean I need: #ifdef CONFIG_X86 mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); #else mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); #endif > -boris > > Thank you, Oleksandr
(Stefano, question for you at the end) On 06/07/2018 02:39 AM, Oleksandr Andrushchenko wrote: > On 06/07/2018 12:19 AM, Boris Ostrovsky wrote: >> On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote: >>> On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: >>>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >>>> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map >>>> *map) >>>> if (map == NULL) >>>> return; >>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > *Option 1: kfree(map->frames);* >>>> + if (map->dma_vaddr) { >>>> + struct gnttab_dma_alloc_args args; >>>> + >>>> + args.dev = map->dma_dev; >>>> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>>> + args.nr_pages = map->count; >>>> + args.pages = map->pages; >>>> + args.frames = map->frames; >>>> + args.vaddr = map->dma_vaddr; >>>> + args.dev_bus_addr = map->dma_bus_addr; >>>> + >>>> + gnttab_dma_free_pages(&args); > *Option 2: kfree(map->frames);* >>>> + } else >>>> +#endif >>>> if (map->pages) >>>> gnttab_free_pages(map->count, map->pages); >>>> + >>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>> + kfree(map->frames); >>>> +#endif >>>> >>>> Can this be done under if (map->dma_vaddr) ? >>>> In other words, is it >>>> possible for dma_vaddr to be NULL and still have unallocated frames >>>> pointer? >>> It is possible to have vaddr == NULL and frames != NULL as we >>> allocate frames outside of gnttab_dma_alloc_pages which >>> may fail. Calling kfree on NULL pointer is safe, >> >> I am not questioning safety of the code, I would like avoid another >> ifdef. > Ah, I now understand, so you are asking if we can have > that kfree(map->frames); in the place *Option 2* I marked above. > Unfortunately no: map->frames is allocated before we try to > allocate DMA memory, e.g. before dma_vaddr is set: > [...] > add->frames = kcalloc(count, sizeof(add->frames[0]), > GFP_KERNEL); > if (!add->frames) > goto err; > > [...] > if (gnttab_dma_alloc_pages(&args)) > goto err; > > add->dma_vaddr = args.vaddr; > [...] > err: > gntdev_free_map(add); > > So, it is possible to enter gntdev_free_map with > frames != NULL and dma_vaddr == NULL. Option 1 above cannot be used > as map->frames is needed for gnttab_dma_free_pages(&args); > and Option 2 cannot be used as frames != NULL and dma_vaddr == NULL. > Thus, I think that unfortunately we need that #ifdef. > Option 3 below can also be considered, but that seems to be not good > as we free resources in different places which looks inconsistent. I was only thinking of option 2. But if it is possible to have frames != NULL and dma_vaddr == NULL then perhaps we indeed will have to live with the extra ifdef. > > Sorry if I'm still missing your point. >> >>> so >>> I see no reason to change this code. >>>>> kfree(map->pages); >>>>> kfree(map->grants); >>>>> kfree(map->map_ops); >>>>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map >>>>> *map) >>>>> kfree(map); >>>>> } >>>>> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv >>>>> *priv, int count) >>>>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, >>>>> int count, >>>>> + int dma_flags) >>>>> { >>>>> struct grant_map *add; >>>>> int i; >>>>> @@ -155,6 +200,37 @@ static struct grant_map >>>>> *gntdev_alloc_map(struct gntdev_priv *priv, int count) >>>>> NULL == add->pages) >>>>> goto err; >>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>> + add->dma_flags = dma_flags; >>>>> + >>>>> + /* >>>>> + * Check if this mapping is requested to be backed >>>>> + * by a DMA buffer. >>>>> + */ >>>>> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | >>>>> GNTDEV_DMA_FLAG_COHERENT)) { >>>>> + struct gnttab_dma_alloc_args args; >>>>> + >>>>> + add->frames = kcalloc(count, sizeof(add->frames[0]), >>>>> + GFP_KERNEL); >>>>> + if (!add->frames) >>>>> + goto err; >>>>> + >>>>> + /* Remember the device, so we can free DMA memory. */ >>>>> + add->dma_dev = priv->dma_dev; >>>>> + >>>>> + args.dev = priv->dma_dev; >>>>> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>>>> + args.nr_pages = count; >>>>> + args.pages = add->pages; >>>>> + args.frames = add->frames; >>>>> + >>>>> + if (gnttab_dma_alloc_pages(&args)) > *Option 3: kfree(map->frames);* >>>>> + goto err; >>>>> + >>>>> + add->dma_vaddr = args.vaddr; >>>>> + add->dma_bus_addr = args.dev_bus_addr; >>>>> + } else >>>>> +#endif >>>>> if (gnttab_alloc_pages(count, add->pages)) >>>>> goto err; >>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map >>>>> *map) >>>>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>>>> if (use_ptemod) >>>>> map->kunmap_ops[i].handle = map->kmap_ops[i].handle; >>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>> + else if (map->dma_vaddr) { >>>>> + unsigned long mfn; >>>>> + >>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>> Not pfn_to_mfn()? >>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] >>> and [2] >>> Thus, >>> >>> drivers/xen/gntdev.c:408:10: error: implicit declaration of function >>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>> >>> So, I'll keep __pfn_to_mfn >> >> How will this work on non-PV x86? > So, you mean I need: > #ifdef CONFIG_X86 > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > #else > mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > #endif > I'd rather fix it in ARM code. Stefano, why does ARM uses the underscored version? -boris
On 06/08/2018 12:46 AM, Boris Ostrovsky wrote: > (Stefano, question for you at the end) > > On 06/07/2018 02:39 AM, Oleksandr Andrushchenko wrote: >> On 06/07/2018 12:19 AM, Boris Ostrovsky wrote: >>> On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote: >>>> On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: >>>>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: >>>>> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map >>>>> *map) >>>>> if (map == NULL) >>>>> return; >>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >> *Option 1: kfree(map->frames);* >>>>> + if (map->dma_vaddr) { >>>>> + struct gnttab_dma_alloc_args args; >>>>> + >>>>> + args.dev = map->dma_dev; >>>>> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>>>> + args.nr_pages = map->count; >>>>> + args.pages = map->pages; >>>>> + args.frames = map->frames; >>>>> + args.vaddr = map->dma_vaddr; >>>>> + args.dev_bus_addr = map->dma_bus_addr; >>>>> + >>>>> + gnttab_dma_free_pages(&args); >> *Option 2: kfree(map->frames);* >>>>> + } else >>>>> +#endif >>>>> if (map->pages) >>>>> gnttab_free_pages(map->count, map->pages); >>>>> + >>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>> + kfree(map->frames); >>>>> +#endif >>>>> >>>>> Can this be done under if (map->dma_vaddr) ? >>>>> In other words, is it >>>>> possible for dma_vaddr to be NULL and still have unallocated frames >>>>> pointer? >>>> It is possible to have vaddr == NULL and frames != NULL as we >>>> allocate frames outside of gnttab_dma_alloc_pages which >>>> may fail. Calling kfree on NULL pointer is safe, >>> I am not questioning safety of the code, I would like avoid another >>> ifdef. >> Ah, I now understand, so you are asking if we can have >> that kfree(map->frames); in the place *Option 2* I marked above. >> Unfortunately no: map->frames is allocated before we try to >> allocate DMA memory, e.g. before dma_vaddr is set: >> [...] >> add->frames = kcalloc(count, sizeof(add->frames[0]), >> GFP_KERNEL); >> if (!add->frames) >> goto err; >> >> [...] >> if (gnttab_dma_alloc_pages(&args)) >> goto err; >> >> add->dma_vaddr = args.vaddr; >> [...] >> err: >> gntdev_free_map(add); >> >> So, it is possible to enter gntdev_free_map with >> frames != NULL and dma_vaddr == NULL. Option 1 above cannot be used >> as map->frames is needed for gnttab_dma_free_pages(&args); >> and Option 2 cannot be used as frames != NULL and dma_vaddr == NULL. >> Thus, I think that unfortunately we need that #ifdef. >> Option 3 below can also be considered, but that seems to be not good >> as we free resources in different places which looks inconsistent. > > I was only thinking of option 2. But if it is possible to have frames != > NULL and dma_vaddr == NULL then perhaps we indeed will have to live with > the extra ifdef. ok > >> Sorry if I'm still missing your point. >>>> so >>>> I see no reason to change this code. >>>>>> kfree(map->pages); >>>>>> kfree(map->grants); >>>>>> kfree(map->map_ops); >>>>>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map >>>>>> *map) >>>>>> kfree(map); >>>>>> } >>>>>> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv >>>>>> *priv, int count) >>>>>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, >>>>>> int count, >>>>>> + int dma_flags) >>>>>> { >>>>>> struct grant_map *add; >>>>>> int i; >>>>>> @@ -155,6 +200,37 @@ static struct grant_map >>>>>> *gntdev_alloc_map(struct gntdev_priv *priv, int count) >>>>>> NULL == add->pages) >>>>>> goto err; >>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>> + add->dma_flags = dma_flags; >>>>>> + >>>>>> + /* >>>>>> + * Check if this mapping is requested to be backed >>>>>> + * by a DMA buffer. >>>>>> + */ >>>>>> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | >>>>>> GNTDEV_DMA_FLAG_COHERENT)) { >>>>>> + struct gnttab_dma_alloc_args args; >>>>>> + >>>>>> + add->frames = kcalloc(count, sizeof(add->frames[0]), >>>>>> + GFP_KERNEL); >>>>>> + if (!add->frames) >>>>>> + goto err; >>>>>> + >>>>>> + /* Remember the device, so we can free DMA memory. */ >>>>>> + add->dma_dev = priv->dma_dev; >>>>>> + >>>>>> + args.dev = priv->dma_dev; >>>>>> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; >>>>>> + args.nr_pages = count; >>>>>> + args.pages = add->pages; >>>>>> + args.frames = add->frames; >>>>>> + >>>>>> + if (gnttab_dma_alloc_pages(&args)) >> *Option 3: kfree(map->frames);* >>>>>> + goto err; >>>>>> + >>>>>> + add->dma_vaddr = args.vaddr; >>>>>> + add->dma_bus_addr = args.dev_bus_addr; >>>>>> + } else >>>>>> +#endif >>>>>> if (gnttab_alloc_pages(count, add->pages)) >>>>>> goto err; >>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map >>>>>> *map) >>>>>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>>>>> if (use_ptemod) >>>>>> map->kunmap_ops[i].handle = map->kmap_ops[i].handle; >>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>> + else if (map->dma_vaddr) { >>>>>> + unsigned long mfn; >>>>>> + >>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>> Not pfn_to_mfn()? >>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] >>>> and [2] >>>> Thus, >>>> >>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of function >>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>> >>>> So, I'll keep __pfn_to_mfn >>> How will this work on non-PV x86? >> So, you mean I need: >> #ifdef CONFIG_X86 >> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >> #else >> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >> #endif >> > I'd rather fix it in ARM code. Stefano, why does ARM uses the > underscored version? Do you want me to add one more patch for ARM to wrap __pfn_to_mfn with static inline for ARM? e.g. static inline ...pfn_to_mfn(...) { __pfn_to_mfn(); } > > -boris > Thank you, Oleksandr
On Fri, 8 Jun 2018, Oleksandr Andrushchenko wrote: > On 06/08/2018 12:46 AM, Boris Ostrovsky wrote: > > (Stefano, question for you at the end) > > > > On 06/07/2018 02:39 AM, Oleksandr Andrushchenko wrote: > > > On 06/07/2018 12:19 AM, Boris Ostrovsky wrote: > > > > On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote: > > > > > On 06/04/2018 11:12 PM, Boris Ostrovsky wrote: > > > > > > On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote: > > > > > > @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map > > > > > > *map) > > > > > > if (map == NULL) > > > > > > return; > > > > > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > > > *Option 1: kfree(map->frames);* > > > > > > + if (map->dma_vaddr) { > > > > > > + struct gnttab_dma_alloc_args args; > > > > > > + > > > > > > + args.dev = map->dma_dev; > > > > > > + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; > > > > > > + args.nr_pages = map->count; > > > > > > + args.pages = map->pages; > > > > > > + args.frames = map->frames; > > > > > > + args.vaddr = map->dma_vaddr; > > > > > > + args.dev_bus_addr = map->dma_bus_addr; > > > > > > + > > > > > > + gnttab_dma_free_pages(&args); > > > *Option 2: kfree(map->frames);* > > > > > > + } else > > > > > > +#endif > > > > > > if (map->pages) > > > > > > gnttab_free_pages(map->count, map->pages); > > > > > > + > > > > > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > > > > > > + kfree(map->frames); > > > > > > +#endif > > > > > > > > > > > > Can this be done under if (map->dma_vaddr) ? > > > > > > In other words, is it > > > > > > possible for dma_vaddr to be NULL and still have unallocated frames > > > > > > pointer? > > > > > It is possible to have vaddr == NULL and frames != NULL as we > > > > > allocate frames outside of gnttab_dma_alloc_pages which > > > > > may fail. Calling kfree on NULL pointer is safe, > > > > I am not questioning safety of the code, I would like avoid another > > > > ifdef. > > > Ah, I now understand, so you are asking if we can have > > > that kfree(map->frames); in the place *Option 2* I marked above. > > > Unfortunately no: map->frames is allocated before we try to > > > allocate DMA memory, e.g. before dma_vaddr is set: > > > [...] > > > add->frames = kcalloc(count, sizeof(add->frames[0]), > > > GFP_KERNEL); > > > if (!add->frames) > > > goto err; > > > > > > [...] > > > if (gnttab_dma_alloc_pages(&args)) > > > goto err; > > > > > > add->dma_vaddr = args.vaddr; > > > [...] > > > err: > > > gntdev_free_map(add); > > > > > > So, it is possible to enter gntdev_free_map with > > > frames != NULL and dma_vaddr == NULL. Option 1 above cannot be used > > > as map->frames is needed for gnttab_dma_free_pages(&args); > > > and Option 2 cannot be used as frames != NULL and dma_vaddr == NULL. > > > Thus, I think that unfortunately we need that #ifdef. > > > Option 3 below can also be considered, but that seems to be not good > > > as we free resources in different places which looks inconsistent. > > > > I was only thinking of option 2. But if it is possible to have frames != > > NULL and dma_vaddr == NULL then perhaps we indeed will have to live with > > the extra ifdef. > ok > > > > > Sorry if I'm still missing your point. > > > > > so > > > > > I see no reason to change this code. > > > > > > > kfree(map->pages); > > > > > > > kfree(map->grants); > > > > > > > kfree(map->map_ops); > > > > > > > @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map > > > > > > > *map) > > > > > > > kfree(map); > > > > > > > } > > > > > > > -static struct grant_map *gntdev_alloc_map(struct gntdev_priv > > > > > > > *priv, int count) > > > > > > > +static struct grant_map *gntdev_alloc_map(struct gntdev_priv > > > > > > > *priv, > > > > > > > int count, > > > > > > > + int dma_flags) > > > > > > > { > > > > > > > struct grant_map *add; > > > > > > > int i; > > > > > > > @@ -155,6 +200,37 @@ static struct grant_map > > > > > > > *gntdev_alloc_map(struct gntdev_priv *priv, int count) > > > > > > > NULL == add->pages) > > > > > > > goto err; > > > > > > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > > > > > > > + add->dma_flags = dma_flags; > > > > > > > + > > > > > > > + /* > > > > > > > + * Check if this mapping is requested to be backed > > > > > > > + * by a DMA buffer. > > > > > > > + */ > > > > > > > + if (dma_flags & (GNTDEV_DMA_FLAG_WC | > > > > > > > GNTDEV_DMA_FLAG_COHERENT)) { > > > > > > > + struct gnttab_dma_alloc_args args; > > > > > > > + > > > > > > > + add->frames = kcalloc(count, sizeof(add->frames[0]), > > > > > > > + GFP_KERNEL); > > > > > > > + if (!add->frames) > > > > > > > + goto err; > > > > > > > + > > > > > > > + /* Remember the device, so we can free DMA memory. */ > > > > > > > + add->dma_dev = priv->dma_dev; > > > > > > > + > > > > > > > + args.dev = priv->dma_dev; > > > > > > > + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; > > > > > > > + args.nr_pages = count; > > > > > > > + args.pages = add->pages; > > > > > > > + args.frames = add->frames; > > > > > > > + > > > > > > > + if (gnttab_dma_alloc_pages(&args)) > > > *Option 3: kfree(map->frames);* > > > > > > > + goto err; > > > > > > > + > > > > > > > + add->dma_vaddr = args.vaddr; > > > > > > > + add->dma_bus_addr = args.dev_bus_addr; > > > > > > > + } else > > > > > > > +#endif > > > > > > > if (gnttab_alloc_pages(count, add->pages)) > > > > > > > goto err; > > > > > > > @@ -325,6 +401,14 @@ static int map_grant_pages(struct > > > > > > > grant_map > > > > > > > *map) > > > > > > > map->unmap_ops[i].handle = map->map_ops[i].handle; > > > > > > > if (use_ptemod) > > > > > > > map->kunmap_ops[i].handle = > > > > > > > map->kmap_ops[i].handle; > > > > > > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > > > > > > > + else if (map->dma_vaddr) { > > > > > > > + unsigned long mfn; > > > > > > > + > > > > > > > + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > Not pfn_to_mfn()? > > > > > I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] > > > > > and [2] > > > > > Thus, > > > > > > > > > > drivers/xen/gntdev.c:408:10: error: implicit declaration of function > > > > > ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] > > > > > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > > > > > So, I'll keep __pfn_to_mfn > > > > How will this work on non-PV x86? > > > So, you mean I need: > > > #ifdef CONFIG_X86 > > > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > > > #else > > > mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > > > #endif > > > > > I'd rather fix it in ARM code. Stefano, why does ARM uses the > > underscored version? > Do you want me to add one more patch for ARM to wrap __pfn_to_mfn > with static inline for ARM? e.g. > static inline ...pfn_to_mfn(...) > { > __pfn_to_mfn(); > } A Xen on ARM guest doesn't actually know the mfns behind its own pseudo-physical pages. This is why we stopped using pfn_to_mfn and started using pfn_to_bfn instead, which will generally return "pfn", unless the page is a foreign grant. See include/xen/arm/page.h. pfn_to_bfn was also introduced on x86. For example, see the usage of pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care about other mapped grants, you can just use pfn_to_gfn, that always returns pfn. Also, for your information, we support different page granularities in Linux as a Xen guest, see the comment at include/xen/arm/page.h: /* * The pseudo-physical frame (pfn) used in all the helpers is always based * on Xen page granularity (i.e 4KB). * * A Linux page may be split across multiple non-contiguous Xen page so we * have to keep track with frame based on 4KB page granularity. * * PV drivers should never make a direct usage of those helpers (particularly * pfn_to_gfn and gfn_to_pfn). */ A Linux page could be 64K, but a Xen page is always 4K. A granted page is also 4K. We have helpers to take into account the offsets to map multiple Xen grants in a single Linux page, see for example drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have been converted to be able to work with 64K pages correctly, but if I remember correctly gntdev.c is the only remaining driver that doesn't support 64K pages yet, so you don't have to deal with it if you don't want to.
On 06/08/2018 01:59 PM, Stefano Stabellini wrote: > >>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>> grant_map >>>>>>>> *map) >>>>>>>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>>>>>>> if (use_ptemod) >>>>>>>> map->kunmap_ops[i].handle = >>>>>>>> map->kmap_ops[i].handle; >>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>> + else if (map->dma_vaddr) { >>>>>>>> + unsigned long mfn; >>>>>>>> + >>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> Not pfn_to_mfn()? >>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] >>>>>> and [2] >>>>>> Thus, >>>>>> >>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of function >>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>> >>>>>> So, I'll keep __pfn_to_mfn >>>>> How will this work on non-PV x86? >>>> So, you mean I need: >>>> #ifdef CONFIG_X86 >>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>> #else >>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>> #endif >>>> >>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>> underscored version? >> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >> with static inline for ARM? e.g. >> static inline ...pfn_to_mfn(...) >> { >> __pfn_to_mfn(); >> } > > A Xen on ARM guest doesn't actually know the mfns behind its own > pseudo-physical pages. This is why we stopped using pfn_to_mfn and > started using pfn_to_bfn instead, which will generally return "pfn", > unless the page is a foreign grant. See include/xen/arm/page.h. > pfn_to_bfn was also introduced on x86. For example, see the usage of > pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care > about other mapped grants, you can just use pfn_to_gfn, that always > returns pfn. I think then this code needs to use pfn_to_bfn(). > > Also, for your information, we support different page granularities in > Linux as a Xen guest, see the comment at include/xen/arm/page.h: > > /* > * The pseudo-physical frame (pfn) used in all the helpers is always based > * on Xen page granularity (i.e 4KB). > * > * A Linux page may be split across multiple non-contiguous Xen page so we > * have to keep track with frame based on 4KB page granularity. > * > * PV drivers should never make a direct usage of those helpers (particularly > * pfn_to_gfn and gfn_to_pfn). > */ > > A Linux page could be 64K, but a Xen page is always 4K. A granted page > is also 4K. We have helpers to take into account the offsets to map > multiple Xen grants in a single Linux page, see for example > drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have > been converted to be able to work with 64K pages correctly, but if I > remember correctly gntdev.c is the only remaining driver that doesn't > support 64K pages yet, so you don't have to deal with it if you don't > want to. I believe somewhere in this series there is a test for PAGE_SIZE vs. XEN_PAGE_SIZE. Right, Oleksandr? Thanks for the explanation. -boris
On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: > On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>> grant_map >>>>>>>>> *map) >>>>>>>>> map->unmap_ops[i].handle = map->map_ops[i].handle; >>>>>>>>> if (use_ptemod) >>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>> + unsigned long mfn; >>>>>>>>> + >>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>> Not pfn_to_mfn()? >>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] >>>>>>> and [2] >>>>>>> Thus, >>>>>>> >>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of function >>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> >>>>>>> So, I'll keep __pfn_to_mfn >>>>>> How will this work on non-PV x86? >>>>> So, you mean I need: >>>>> #ifdef CONFIG_X86 >>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>> #else >>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>> #endif >>>>> >>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>> underscored version? >>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>> with static inline for ARM? e.g. >>> static inline ...pfn_to_mfn(...) >>> { >>> __pfn_to_mfn(); >>> } >> A Xen on ARM guest doesn't actually know the mfns behind its own >> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >> started using pfn_to_bfn instead, which will generally return "pfn", >> unless the page is a foreign grant. See include/xen/arm/page.h. >> pfn_to_bfn was also introduced on x86. For example, see the usage of >> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care >> about other mapped grants, you can just use pfn_to_gfn, that always >> returns pfn. > > I think then this code needs to use pfn_to_bfn(). Ok > > >> Also, for your information, we support different page granularities in >> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >> >> /* >> * The pseudo-physical frame (pfn) used in all the helpers is always based >> * on Xen page granularity (i.e 4KB). >> * >> * A Linux page may be split across multiple non-contiguous Xen page so we >> * have to keep track with frame based on 4KB page granularity. >> * >> * PV drivers should never make a direct usage of those helpers (particularly >> * pfn_to_gfn and gfn_to_pfn). >> */ >> >> A Linux page could be 64K, but a Xen page is always 4K. A granted page >> is also 4K. We have helpers to take into account the offsets to map >> multiple Xen grants in a single Linux page, see for example >> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >> been converted to be able to work with 64K pages correctly, but if I >> remember correctly gntdev.c is the only remaining driver that doesn't >> support 64K pages yet, so you don't have to deal with it if you don't >> want to. > > I believe somewhere in this series there is a test for PAGE_SIZE vs. > XEN_PAGE_SIZE. Right, Oleksandr? Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, but I didn't touch gntdev for that. Do you want me to add yet another patch in the series to check for that? > Thanks for the explanation. > > -boris Thank you, Oleksandr
On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: > On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: > > On 06/08/2018 01:59 PM, Stefano Stabellini wrote: > > > > > > > > > > @@ -325,6 +401,14 @@ static int map_grant_pages(struct > > > > > > > > > > grant_map > > > > > > > > > > *map) > > > > > > > > > > map->unmap_ops[i].handle = > > > > > > > > > > map->map_ops[i].handle; > > > > > > > > > > if (use_ptemod) > > > > > > > > > > map->kunmap_ops[i].handle = > > > > > > > > > > map->kmap_ops[i].handle; > > > > > > > > > > +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC > > > > > > > > > > + else if (map->dma_vaddr) { > > > > > > > > > > + unsigned long mfn; > > > > > > > > > > + > > > > > > > > > > + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > > > > Not pfn_to_mfn()? > > > > > > > > I'd love to, but pfn_to_mfn is only defined for x86, not ARM: > > > > > > > > [1] > > > > > > > > and [2] > > > > > > > > Thus, > > > > > > > > > > > > > > > > drivers/xen/gntdev.c:408:10: error: implicit declaration of > > > > > > > > function > > > > > > > > ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] > > > > > > > > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > > > > > > > > > > > So, I'll keep __pfn_to_mfn > > > > > > > How will this work on non-PV x86? > > > > > > So, you mean I need: > > > > > > #ifdef CONFIG_X86 > > > > > > mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > #else > > > > > > mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); > > > > > > #endif > > > > > > > > > > > I'd rather fix it in ARM code. Stefano, why does ARM uses the > > > > > underscored version? > > > > Do you want me to add one more patch for ARM to wrap __pfn_to_mfn > > > > with static inline for ARM? e.g. > > > > static inline ...pfn_to_mfn(...) > > > > { > > > > __pfn_to_mfn(); > > > > } > > > A Xen on ARM guest doesn't actually know the mfns behind its own > > > pseudo-physical pages. This is why we stopped using pfn_to_mfn and > > > started using pfn_to_bfn instead, which will generally return "pfn", > > > unless the page is a foreign grant. See include/xen/arm/page.h. > > > pfn_to_bfn was also introduced on x86. For example, see the usage of > > > pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care > > > about other mapped grants, you can just use pfn_to_gfn, that always > > > returns pfn. > > > > I think then this code needs to use pfn_to_bfn(). > Ok > > > > > > > Also, for your information, we support different page granularities in > > > Linux as a Xen guest, see the comment at include/xen/arm/page.h: > > > > > > /* > > > * The pseudo-physical frame (pfn) used in all the helpers is always > > > based > > > * on Xen page granularity (i.e 4KB). > > > * > > > * A Linux page may be split across multiple non-contiguous Xen page so > > > we > > > * have to keep track with frame based on 4KB page granularity. > > > * > > > * PV drivers should never make a direct usage of those helpers > > > (particularly > > > * pfn_to_gfn and gfn_to_pfn). > > > */ > > > > > > A Linux page could be 64K, but a Xen page is always 4K. A granted page > > > is also 4K. We have helpers to take into account the offsets to map > > > multiple Xen grants in a single Linux page, see for example > > > drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have > > > been converted to be able to work with 64K pages correctly, but if I > > > remember correctly gntdev.c is the only remaining driver that doesn't > > > support 64K pages yet, so you don't have to deal with it if you don't > > > want to. > > > > I believe somewhere in this series there is a test for PAGE_SIZE vs. > > XEN_PAGE_SIZE. Right, Oleksandr? > Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, > but I didn't touch gntdev for that. Do you want me to add yet another patch > in the series to check for that? gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, so you are not going to break anything that is not already broken :-) If your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it might be good to add an in-code comment about it, just to make it easier to fix the whole of gntdev.c in the future. > > Thanks for the explanation.
On 06/11/2018 07:51 PM, Stefano Stabellini wrote: > On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: >> On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: >>> On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>>>> grant_map >>>>>>>>>>> *map) >>>>>>>>>>> map->unmap_ops[i].handle = >>>>>>>>>>> map->map_ops[i].handle; >>>>>>>>>>> if (use_ptemod) >>>>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>>>> + unsigned long mfn; >>>>>>>>>>> + >>>>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>> Not pfn_to_mfn()? >>>>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: >>>>>>>>> [1] >>>>>>>>> and [2] >>>>>>>>> Thus, >>>>>>>>> >>>>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of >>>>>>>>> function >>>>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>> >>>>>>>>> So, I'll keep __pfn_to_mfn >>>>>>>> How will this work on non-PV x86? >>>>>>> So, you mean I need: >>>>>>> #ifdef CONFIG_X86 >>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> #else >>>>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> #endif >>>>>>> >>>>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>>>> underscored version? >>>>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>>>> with static inline for ARM? e.g. >>>>> static inline ...pfn_to_mfn(...) >>>>> { >>>>> __pfn_to_mfn(); >>>>> } >>>> A Xen on ARM guest doesn't actually know the mfns behind its own >>>> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >>>> started using pfn_to_bfn instead, which will generally return "pfn", >>>> unless the page is a foreign grant. See include/xen/arm/page.h. >>>> pfn_to_bfn was also introduced on x86. For example, see the usage of >>>> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care >>>> about other mapped grants, you can just use pfn_to_gfn, that always >>>> returns pfn. >>> I think then this code needs to use pfn_to_bfn(). >> Ok >>> >>>> Also, for your information, we support different page granularities in >>>> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >>>> >>>> /* >>>> * The pseudo-physical frame (pfn) used in all the helpers is always >>>> based >>>> * on Xen page granularity (i.e 4KB). >>>> * >>>> * A Linux page may be split across multiple non-contiguous Xen page so >>>> we >>>> * have to keep track with frame based on 4KB page granularity. >>>> * >>>> * PV drivers should never make a direct usage of those helpers >>>> (particularly >>>> * pfn_to_gfn and gfn_to_pfn). >>>> */ >>>> >>>> A Linux page could be 64K, but a Xen page is always 4K. A granted page >>>> is also 4K. We have helpers to take into account the offsets to map >>>> multiple Xen grants in a single Linux page, see for example >>>> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >>>> been converted to be able to work with 64K pages correctly, but if I >>>> remember correctly gntdev.c is the only remaining driver that doesn't >>>> support 64K pages yet, so you don't have to deal with it if you don't >>>> want to. >>> I believe somewhere in this series there is a test for PAGE_SIZE vs. >>> XEN_PAGE_SIZE. Right, Oleksandr? >> Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, >> but I didn't touch gntdev for that. Do you want me to add yet another patch >> in the series to check for that? > gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, > so you are not going to break anything that is not already broken :-) If > your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it might be > good to add an in-code comment about it, just to make it easier to fix > the whole of gntdev.c in the future. > Yes, I just mean I can add something like [1] as a separate patch to the series, so we are on the safe side here > >>> Thanks for the explanation. [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/xen/xen_drm_front.c#n813
Hi Stefano, On 06/11/2018 05:51 PM, Stefano Stabellini wrote: > On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: >> On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: >>> On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>>>> grant_map >>>>>>>>>>> *map) >>>>>>>>>>> map->unmap_ops[i].handle = >>>>>>>>>>> map->map_ops[i].handle; >>>>>>>>>>> if (use_ptemod) >>>>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>>>> + unsigned long mfn; >>>>>>>>>>> + >>>>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>> Not pfn_to_mfn()? >>>>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: >>>>>>>>> [1] >>>>>>>>> and [2] >>>>>>>>> Thus, >>>>>>>>> >>>>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of >>>>>>>>> function >>>>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>> >>>>>>>>> So, I'll keep __pfn_to_mfn >>>>>>>> How will this work on non-PV x86? >>>>>>> So, you mean I need: >>>>>>> #ifdef CONFIG_X86 >>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> #else >>>>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>> #endif >>>>>>> >>>>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>>>> underscored version? >>>>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>>>> with static inline for ARM? e.g. >>>>> static inline ...pfn_to_mfn(...) >>>>> { >>>>> __pfn_to_mfn(); >>>>> } >>>> A Xen on ARM guest doesn't actually know the mfns behind its own >>>> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >>>> started using pfn_to_bfn instead, which will generally return "pfn", >>>> unless the page is a foreign grant. See include/xen/arm/page.h. >>>> pfn_to_bfn was also introduced on x86. For example, see the usage of >>>> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care >>>> about other mapped grants, you can just use pfn_to_gfn, that always >>>> returns pfn. >>> >>> I think then this code needs to use pfn_to_bfn(). >> Ok >>> >>> >>>> Also, for your information, we support different page granularities in >>>> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >>>> >>>> /* >>>> * The pseudo-physical frame (pfn) used in all the helpers is always >>>> based >>>> * on Xen page granularity (i.e 4KB). >>>> * >>>> * A Linux page may be split across multiple non-contiguous Xen page so >>>> we >>>> * have to keep track with frame based on 4KB page granularity. >>>> * >>>> * PV drivers should never make a direct usage of those helpers >>>> (particularly >>>> * pfn_to_gfn and gfn_to_pfn). >>>> */ >>>> >>>> A Linux page could be 64K, but a Xen page is always 4K. A granted page >>>> is also 4K. We have helpers to take into account the offsets to map >>>> multiple Xen grants in a single Linux page, see for example >>>> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >>>> been converted to be able to work with 64K pages correctly, but if I >>>> remember correctly gntdev.c is the only remaining driver that doesn't >>>> support 64K pages yet, so you don't have to deal with it if you don't >>>> want to. >>> >>> I believe somewhere in this series there is a test for PAGE_SIZE vs. >>> XEN_PAGE_SIZE. Right, Oleksandr? >> Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, >> but I didn't touch gntdev for that. Do you want me to add yet another patch >> in the series to check for that? > > gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, > so you are not going to break anything that is not already broken :-) If > your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it might be > good to add an in-code comment about it, just to make it easier to fix > the whole of gntdev.c in the future. Well, I think gntdev is capable of handling PAGE_SIZE != XEN_PAGE_SIZE. Let's imagine Linux is built with 64K pages. gntdev will map each grant at a 64K alignment. Although, I am not sure if patches for QEMU ever make it upstream (I think it is in Centos). Cheers,
Hi, On 06/11/2018 06:16 PM, Oleksandr Andrushchenko wrote: > On 06/11/2018 07:51 PM, Stefano Stabellini wrote: >> On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: >>> On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: >>>> On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>>>>> grant_map >>>>>>>>>>>> *map) >>>>>>>>>>>> map->unmap_ops[i].handle = >>>>>>>>>>>> map->map_ops[i].handle; >>>>>>>>>>>> if (use_ptemod) >>>>>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>>>>> + unsigned long mfn; >>>>>>>>>>>> + >>>>>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>>> Not pfn_to_mfn()? >>>>>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: >>>>>>>>>> [1] >>>>>>>>>> and [2] >>>>>>>>>> Thus, >>>>>>>>>> >>>>>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of >>>>>>>>>> function >>>>>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>> >>>>>>>>>> So, I'll keep __pfn_to_mfn >>>>>>>>> How will this work on non-PV x86? >>>>>>>> So, you mean I need: >>>>>>>> #ifdef CONFIG_X86 >>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>> #else >>>>>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>> #endif >>>>>>>> >>>>>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>>>>> underscored version? >>>>>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>>>>> with static inline for ARM? e.g. >>>>>> static inline ...pfn_to_mfn(...) >>>>>> { >>>>>> __pfn_to_mfn(); >>>>>> } >>>>> A Xen on ARM guest doesn't actually know the mfns behind its own >>>>> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >>>>> started using pfn_to_bfn instead, which will generally return "pfn", >>>>> unless the page is a foreign grant. See include/xen/arm/page.h. >>>>> pfn_to_bfn was also introduced on x86. For example, see the usage of >>>>> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care >>>>> about other mapped grants, you can just use pfn_to_gfn, that always >>>>> returns pfn. >>>> I think then this code needs to use pfn_to_bfn(). >>> Ok >>>> >>>>> Also, for your information, we support different page granularities in >>>>> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >>>>> >>>>> /* >>>>> * The pseudo-physical frame (pfn) used in all the helpers is >>>>> always >>>>> based >>>>> * on Xen page granularity (i.e 4KB). >>>>> * >>>>> * A Linux page may be split across multiple non-contiguous Xen >>>>> page so >>>>> we >>>>> * have to keep track with frame based on 4KB page granularity. >>>>> * >>>>> * PV drivers should never make a direct usage of those helpers >>>>> (particularly >>>>> * pfn_to_gfn and gfn_to_pfn). >>>>> */ >>>>> >>>>> A Linux page could be 64K, but a Xen page is always 4K. A granted page >>>>> is also 4K. We have helpers to take into account the offsets to map >>>>> multiple Xen grants in a single Linux page, see for example >>>>> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >>>>> been converted to be able to work with 64K pages correctly, but if I >>>>> remember correctly gntdev.c is the only remaining driver that doesn't >>>>> support 64K pages yet, so you don't have to deal with it if you don't >>>>> want to. >>>> I believe somewhere in this series there is a test for PAGE_SIZE vs. >>>> XEN_PAGE_SIZE. Right, Oleksandr? >>> Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, >>> but I didn't touch gntdev for that. Do you want me to add yet another >>> patch >>> in the series to check for that? >> gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, >> so you are not going to break anything that is not already broken :-) If >> your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it might be >> good to add an in-code comment about it, just to make it easier to fix >> the whole of gntdev.c in the future. >> > Yes, I just mean I can add something like [1] as a separate patch to the > series, > so we are on the safe side here See my comment on Stefano's e-mail. I believe gntdev is able to handle PAGE_SIZE != XEN_PAGE_SIZE. So I would rather keep the behavior we have today for such case. Cheers,
On 06/11/2018 08:46 PM, Julien Grall wrote: > Hi, > > On 06/11/2018 06:16 PM, Oleksandr Andrushchenko wrote: >> On 06/11/2018 07:51 PM, Stefano Stabellini wrote: >>> On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: >>>> On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: >>>>> On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>>>>>> grant_map >>>>>>>>>>>>> *map) >>>>>>>>>>>>> map->unmap_ops[i].handle = >>>>>>>>>>>>> map->map_ops[i].handle; >>>>>>>>>>>>> if (use_ptemod) >>>>>>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>>>>>> + unsigned long mfn; >>>>>>>>>>>>> + >>>>>>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>>>> Not pfn_to_mfn()? >>>>>>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: >>>>>>>>>>> [1] >>>>>>>>>>> and [2] >>>>>>>>>>> Thus, >>>>>>>>>>> >>>>>>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of >>>>>>>>>>> function >>>>>>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>>> >>>>>>>>>>> So, I'll keep __pfn_to_mfn >>>>>>>>>> How will this work on non-PV x86? >>>>>>>>> So, you mean I need: >>>>>>>>> #ifdef CONFIG_X86 >>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>> #else >>>>>>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>> #endif >>>>>>>>> >>>>>>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>>>>>> underscored version? >>>>>>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>>>>>> with static inline for ARM? e.g. >>>>>>> static inline ...pfn_to_mfn(...) >>>>>>> { >>>>>>> __pfn_to_mfn(); >>>>>>> } >>>>>> A Xen on ARM guest doesn't actually know the mfns behind its own >>>>>> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >>>>>> started using pfn_to_bfn instead, which will generally return "pfn", >>>>>> unless the page is a foreign grant. See include/xen/arm/page.h. >>>>>> pfn_to_bfn was also introduced on x86. For example, see the usage of >>>>>> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't >>>>>> care >>>>>> about other mapped grants, you can just use pfn_to_gfn, that always >>>>>> returns pfn. >>>>> I think then this code needs to use pfn_to_bfn(). >>>> Ok >>>>> >>>>>> Also, for your information, we support different page >>>>>> granularities in >>>>>> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >>>>>> >>>>>> /* >>>>>> * The pseudo-physical frame (pfn) used in all the helpers is >>>>>> always >>>>>> based >>>>>> * on Xen page granularity (i.e 4KB). >>>>>> * >>>>>> * A Linux page may be split across multiple non-contiguous >>>>>> Xen page so >>>>>> we >>>>>> * have to keep track with frame based on 4KB page granularity. >>>>>> * >>>>>> * PV drivers should never make a direct usage of those helpers >>>>>> (particularly >>>>>> * pfn_to_gfn and gfn_to_pfn). >>>>>> */ >>>>>> >>>>>> A Linux page could be 64K, but a Xen page is always 4K. A granted >>>>>> page >>>>>> is also 4K. We have helpers to take into account the offsets to map >>>>>> multiple Xen grants in a single Linux page, see for example >>>>>> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >>>>>> been converted to be able to work with 64K pages correctly, but if I >>>>>> remember correctly gntdev.c is the only remaining driver that >>>>>> doesn't >>>>>> support 64K pages yet, so you don't have to deal with it if you >>>>>> don't >>>>>> want to. >>>>> I believe somewhere in this series there is a test for PAGE_SIZE vs. >>>>> XEN_PAGE_SIZE. Right, Oleksandr? >>>> Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, >>>> but I didn't touch gntdev for that. Do you want me to add yet >>>> another patch >>>> in the series to check for that? >>> gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, >>> so you are not going to break anything that is not already broken >>> :-) If >>> your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it >>> might be >>> good to add an in-code comment about it, just to make it easier to fix >>> the whole of gntdev.c in the future. >>> >> Yes, I just mean I can add something like [1] as a separate patch to >> the series, >> so we are on the safe side here > > See my comment on Stefano's e-mail. I believe gntdev is able to handle > PAGE_SIZE != XEN_PAGE_SIZE. So I would rather keep the behavior we > have today for such case. > Sure, with a note that we waste most of a 64KiB page ;) > Cheers, >
Hi, On 06/11/2018 06:49 PM, Oleksandr Andrushchenko wrote: > On 06/11/2018 08:46 PM, Julien Grall wrote: >> Hi, >> >> On 06/11/2018 06:16 PM, Oleksandr Andrushchenko wrote: >>> On 06/11/2018 07:51 PM, Stefano Stabellini wrote: >>>> On Mon, 11 Jun 2018, Oleksandr Andrushchenko wrote: >>>>> On 06/08/2018 10:21 PM, Boris Ostrovsky wrote: >>>>>> On 06/08/2018 01:59 PM, Stefano Stabellini wrote: >>>>>>>>>>>>>> @@ -325,6 +401,14 @@ static int map_grant_pages(struct >>>>>>>>>>>>>> grant_map >>>>>>>>>>>>>> *map) >>>>>>>>>>>>>> map->unmap_ops[i].handle = >>>>>>>>>>>>>> map->map_ops[i].handle; >>>>>>>>>>>>>> if (use_ptemod) >>>>>>>>>>>>>> map->kunmap_ops[i].handle = >>>>>>>>>>>>>> map->kmap_ops[i].handle; >>>>>>>>>>>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC >>>>>>>>>>>>>> + else if (map->dma_vaddr) { >>>>>>>>>>>>>> + unsigned long mfn; >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>>>>> Not pfn_to_mfn()? >>>>>>>>>>>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: >>>>>>>>>>>> [1] >>>>>>>>>>>> and [2] >>>>>>>>>>>> Thus, >>>>>>>>>>>> >>>>>>>>>>>> drivers/xen/gntdev.c:408:10: error: implicit declaration of >>>>>>>>>>>> function >>>>>>>>>>>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration] >>>>>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>>>> >>>>>>>>>>>> So, I'll keep __pfn_to_mfn >>>>>>>>>>> How will this work on non-PV x86? >>>>>>>>>> So, you mean I need: >>>>>>>>>> #ifdef CONFIG_X86 >>>>>>>>>> mfn = pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>> #else >>>>>>>>>> mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); >>>>>>>>>> #endif >>>>>>>>>> >>>>>>>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the >>>>>>>>> underscored version? >>>>>>>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn >>>>>>>> with static inline for ARM? e.g. >>>>>>>> static inline ...pfn_to_mfn(...) >>>>>>>> { >>>>>>>> __pfn_to_mfn(); >>>>>>>> } >>>>>>> A Xen on ARM guest doesn't actually know the mfns behind its own >>>>>>> pseudo-physical pages. This is why we stopped using pfn_to_mfn and >>>>>>> started using pfn_to_bfn instead, which will generally return "pfn", >>>>>>> unless the page is a foreign grant. See include/xen/arm/page.h. >>>>>>> pfn_to_bfn was also introduced on x86. For example, see the usage of >>>>>>> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't >>>>>>> care >>>>>>> about other mapped grants, you can just use pfn_to_gfn, that always >>>>>>> returns pfn. >>>>>> I think then this code needs to use pfn_to_bfn(). >>>>> Ok >>>>>> >>>>>>> Also, for your information, we support different page >>>>>>> granularities in >>>>>>> Linux as a Xen guest, see the comment at include/xen/arm/page.h: >>>>>>> >>>>>>> /* >>>>>>> * The pseudo-physical frame (pfn) used in all the helpers is >>>>>>> always >>>>>>> based >>>>>>> * on Xen page granularity (i.e 4KB). >>>>>>> * >>>>>>> * A Linux page may be split across multiple non-contiguous >>>>>>> Xen page so >>>>>>> we >>>>>>> * have to keep track with frame based on 4KB page granularity. >>>>>>> * >>>>>>> * PV drivers should never make a direct usage of those helpers >>>>>>> (particularly >>>>>>> * pfn_to_gfn and gfn_to_pfn). >>>>>>> */ >>>>>>> >>>>>>> A Linux page could be 64K, but a Xen page is always 4K. A granted >>>>>>> page >>>>>>> is also 4K. We have helpers to take into account the offsets to map >>>>>>> multiple Xen grants in a single Linux page, see for example >>>>>>> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have >>>>>>> been converted to be able to work with 64K pages correctly, but if I >>>>>>> remember correctly gntdev.c is the only remaining driver that >>>>>>> doesn't >>>>>>> support 64K pages yet, so you don't have to deal with it if you >>>>>>> don't >>>>>>> want to. >>>>>> I believe somewhere in this series there is a test for PAGE_SIZE vs. >>>>>> XEN_PAGE_SIZE. Right, Oleksandr? >>>>> Not in gntdev. You might have seen this in xen-drmfront/xen-sndfront, >>>>> but I didn't touch gntdev for that. Do you want me to add yet >>>>> another patch >>>>> in the series to check for that? >>>> gntdev.c is already not capable of handling PAGE_SIZE != XEN_PAGE_SIZE, >>>> so you are not going to break anything that is not already broken >>>> :-) If >>>> your new gntdev.c code relies on PAGE_SIZE == XEN_PAGE_SIZE, it >>>> might be >>>> good to add an in-code comment about it, just to make it easier to fix >>>> the whole of gntdev.c in the future. >>>> >>> Yes, I just mean I can add something like [1] as a separate patch to >>> the series, >>> so we are on the safe side here >> >> See my comment on Stefano's e-mail. I believe gntdev is able to handle >> PAGE_SIZE != XEN_PAGE_SIZE. So I would rather keep the behavior we >> have today for such case. >> > Sure, with a note that we waste most of a 64KiB page ;) That's the second definition of "64KB page" ;). In the case of grants, it is actually quite hard to merge them in a single page. So quite a few places still allocate 64KB but only map the first 4KB. You would need to rework most the grant framework (not only gntdev) to avoid that waste. Patches are welcomed. Cheers,
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index bd56653b9bbc..9813fc440c70 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -37,6 +37,9 @@ #include <linux/slab.h> #include <linux/highmem.h> #include <linux/refcount.h> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC +#include <linux/of_device.h> +#endif #include <xen/xen.h> #include <xen/grant_table.h> @@ -72,6 +75,11 @@ struct gntdev_priv { struct mutex lock; struct mm_struct *mm; struct mmu_notifier mn; + +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + /* Device for which DMA memory is allocated. */ + struct device *dma_dev; +#endif }; struct unmap_notify { @@ -96,10 +104,27 @@ struct grant_map { struct gnttab_unmap_grant_ref *kunmap_ops; struct page **pages; unsigned long pages_vm_start; + +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + /* + * If dmabuf_vaddr is not NULL then this mapping is backed by DMA + * capable memory. + */ + + struct device *dma_dev; + /* Flags used to create this DMA buffer: GNTDEV_DMA_FLAG_XXX. */ + int dma_flags; + void *dma_vaddr; + dma_addr_t dma_bus_addr; + /* This is required for gnttab_dma_{alloc|free}_pages. */ + xen_pfn_t *frames; +#endif }; static int unmap_grant_pages(struct grant_map *map, int offset, int pages); +static struct miscdevice gntdev_miscdev; + /* ------------------------------------------------------------------ */ static void gntdev_print_maps(struct gntdev_priv *priv, @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map) if (map == NULL) return; +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + if (map->dma_vaddr) { + struct gnttab_dma_alloc_args args; + + args.dev = map->dma_dev; + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT; + args.nr_pages = map->count; + args.pages = map->pages; + args.frames = map->frames; + args.vaddr = map->dma_vaddr; + args.dev_bus_addr = map->dma_bus_addr; + + gnttab_dma_free_pages(&args); + } else +#endif if (map->pages) gnttab_free_pages(map->count, map->pages); + +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + kfree(map->frames); +#endif kfree(map->pages); kfree(map->grants); kfree(map->map_ops); @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map) kfree(map); } -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count, + int dma_flags) { struct grant_map *add; int i; @@ -155,6 +200,37 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count) NULL == add->pages) goto err; +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + add->dma_flags = dma_flags; + + /* + * Check if this mapping is requested to be backed + * by a DMA buffer. + */ + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) { + struct gnttab_dma_alloc_args args; + + add->frames = kcalloc(count, sizeof(add->frames[0]), + GFP_KERNEL); + if (!add->frames) + goto err; + + /* Remember the device, so we can free DMA memory. */ + add->dma_dev = priv->dma_dev; + + args.dev = priv->dma_dev; + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT; + args.nr_pages = count; + args.pages = add->pages; + args.frames = add->frames; + + if (gnttab_dma_alloc_pages(&args)) + goto err; + + add->dma_vaddr = args.vaddr; + add->dma_bus_addr = args.dev_bus_addr; + } else +#endif if (gnttab_alloc_pages(count, add->pages)) goto err; @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map *map) map->unmap_ops[i].handle = map->map_ops[i].handle; if (use_ptemod) map->kunmap_ops[i].handle = map->kmap_ops[i].handle; +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + else if (map->dma_vaddr) { + unsigned long mfn; + + mfn = __pfn_to_mfn(page_to_pfn(map->pages[i])); + map->unmap_ops[i].dev_bus_addr = __pfn_to_phys(mfn); + } +#endif } return err; } @@ -548,6 +632,17 @@ static int gntdev_open(struct inode *inode, struct file *flip) } flip->private_data = priv; +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC + priv->dma_dev = gntdev_miscdev.this_device; + + /* + * The device is not spawn from a device tree, so arch_setup_dma_ops + * is not called, thus leaving the device with dummy DMA ops. + * Fix this call of_dma_configure() with a NULL node to set + * default DMA ops. + */ + of_dma_configure(priv->dma_dev, NULL); +#endif pr_debug("priv %p\n", priv); return 0; @@ -589,7 +684,7 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv, return -EINVAL; err = -ENOMEM; - map = gntdev_alloc_map(priv, op.count); + map = gntdev_alloc_map(priv, op.count, 0 /* This is not a dma-buf. */); if (!map) return err; diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h index 6d1163456c03..4b9d498a31d4 100644 --- a/include/uapi/xen/gntdev.h +++ b/include/uapi/xen/gntdev.h @@ -200,4 +200,19 @@ struct ioctl_gntdev_grant_copy { /* Send an interrupt on the indicated event channel */ #define UNMAP_NOTIFY_SEND_EVENT 0x2 +/* + * Flags to be used while requesting memory mapping's backing storage + * to be allocated with DMA API. + */ + +/* + * The buffer is backed with memory allocated with dma_alloc_wc. + */ +#define GNTDEV_DMA_FLAG_WC (1 << 0) + +/* + * The buffer is backed with memory allocated with dma_alloc_coherent. + */ +#define GNTDEV_DMA_FLAG_COHERENT (1 << 1) + #endif /* __LINUX_PUBLIC_GNTDEV_H__ */