Re: [Xen-devel] [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

Stefano,

> Ah! Ops :-)
>
> DEBUG_KERNEL?

You said DEBUG_DRIVER last time.
Both DEBUG_KERNEL and DEBUG_DRIVER are not used in code. DEBUG_KERNEL
is used through Kconfigs to make available debug options to other
different stuff, which consequently define DEBUG through appropriate
makefiles.
DEBUG_DRIVER enables DEBUG macro in base, opp and power makefiles.

Sincerely,
Andrii Anisov.


On Tue, Jan 31, 2017 at 9:21 PM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
>> On Tue, Jan 31, 2017 at 11:15:50AM -0800, Stefano Stabellini wrote:
>> > On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
>> > > On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
>> > > > From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>> > > >
>> > > > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>> > > > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> > > > ---
>> > > >  arch/arm/xen/mm.c         |  1 +
>> > > >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
>> > > >  include/xen/swiotlb-xen.h |  6 ++++++
>> > > >  3 files changed, 29 insertions(+)
>> > > >
>> > > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
>> > > > index cd1684e..76ea48a 100644
>> > > > --- a/arch/arm/xen/mm.c
>> > > > +++ b/arch/arm/xen/mm.c
>> > > > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
>> > > >         .dma_supported = xen_swiotlb_dma_supported,
>> > > >         .set_dma_mask = xen_swiotlb_set_dma_mask,
>> > > >         .mmap = xen_swiotlb_dma_mmap,
>> > > > +       .get_sgtable = xen_swiotlb_get_sgtable,
>> > > >  };
>> > > >
>> > > >  int __init xen_mm_init(void)
>> > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> > > > index 8ac36b4..a809d43 100644
>> > > > --- a/drivers/xen/swiotlb-xen.c
>> > > > +++ b/drivers/xen/swiotlb-xen.c
>> > > > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct 
>> > > > vm_area_struct *vma,
>> > > >         return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
>> > > >  }
>> > > >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
>> > > > +
>> > > > +/*
>> > > > + * Following function should be called with the local pages only.
>> > >
>> > > What does 'local pages' mean?
>> > >
>> > > > + */
>> > > > +int
>> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>> > > > +                       void *cpu_addr, dma_addr_t handle, size_t size,
>> > > > +                       unsigned long attrs)
>> > > > +{
>> > > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>> > > > +       if (__generic_dma_ops(dev)->get_sgtable) {
>> > > > +#ifdef DEBUG
>> > > > +               unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
>> > > > +               BUG_ON (!page_is_ram(bfn));
>> > > > +#endif
>> > >
>> > > Could you remove the above please?
>> >
>> > I asked him to add it (see
>> > http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
>> > this function cannot work on foreign pages. From the commit message
>> > (d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
>> > to be called on buffers returned by dma_alloc_coherent, thus it should
>> > be safe.  However, if that's not the case in any of the drivers, it
>> > would lead to memory corruptions. The two lines under ifdef DEBUG are an
>> > nice way to catch this kind of errors. However, given that they cost a
>> > few cpu cycles more than necessary, I wouldn't enable them in
>> > production, hence the ifdef DEBUG.
>>
>> But there are no Kconfig DEBUG - so you may as well just do
>> #if 0
>>
>> around the code.
>

>
>> > > > +               return __generic_dma_ops(dev)->get_sgtable(dev, sgt, 
>> > > > cpu_addr,
>> > > > +                                                          handle, 
>> > > > size, attrs);
>> > > > +       }
>> > > > +#endif
>> > > > +       return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, 
>> > > > size);
>> > > > +}
>> > > > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
>> > > > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
>> > > > index 5c8f4c8..c554c23 100644
>> > > > --- a/include/xen/swiotlb-xen.h
>> > > > +++ b/include/xen/swiotlb-xen.h
>> > > > @@ -60,4 +60,10 @@ extern int
>> > > >  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>> > > >                         void *cpu_addr, dma_addr_t dma_addr, size_t 
>> > > > size,
>> > > >                         unsigned long attrs);
>> > > > +
>> > > > +extern int
>> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>> > > > +                       void *cpu_addr, dma_addr_t handle, size_t size,
>> > > > +                       unsigned long attrs);
>> > >
>> > > And perhaps fix this to be aligned properly?
>> > > > +
>> > > >  #endif /* __LINUX_SWIOTLB_XEN_H */
>> > > > --
>> > > > 2.7.4
>> > > >
>> > >
>>

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