Messages in this thread | | | Subject | Re: [Xen-devel] [PATCH v2 5/9] xen/gntdev: Allow mappings for DMA buffers | From | Oleksandr Andrushchenko <> | Date | Fri, 8 Jun 2018 14:33:46 +0300 |
| |
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
| |