| Message ID | 1485887426-9016-3-git-send-email-andrii.anisov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote: > From: Andrii Anisov <andrii_anisov@epam.com> > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > --- > 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? > + 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 >
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@epam.com> > > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > --- > > 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. > > + 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 > > >
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@epam.com> > > > > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > --- > > > 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 > > > > >
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@epam.com> > > > > > > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > --- > > > > 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. Ah! Ops :-) DEBUG_KERNEL? > > > > + 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 > > > > > > > >
Dear Konrad, You are not correct here: > But there are no Kconfig DEBUG - so you may as well just do > #if 0 > > around the code. DEBUG macro is widely used in the kernel, just try grepping it through the code. Elementary example is pr_debug definition which is resolved through DEBUG macro, also reasonable pieces of debug code are widely guarded by DEBUG macro. DEBUG macro could be globally across drivers defined by CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile. Sincerely, Andrii Anisov.
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@kernel.org> 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@epam.com> >> > > > >> > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> >> > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> >> > > > --- >> > > > 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 >> > > > >> > > >>
Yup, Following is wrong: > DEBUG macro could be globally across drivers defined by > CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile. DEBUG is not defined globally. And there is no debug option to enable DEBUG in drivers/xen/Makefile. Should it be added? Sincerely, Andrii Anisov.
Hi, On 01/02/2017 10:42, Andrii Anisov wrote: >> 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. Technically this check should be done on any debug build for safety. So I would add new Kconfig DEBUG_KENREL that is selected by DEBUG_KERNEL. Cheers,
On 01/02/2017 10:57, Julien Grall wrote: > Hi, > > On 01/02/2017 10:42, Andrii Anisov wrote: >>> 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. > > Technically this check should be done on any debug build for safety. So > I would add new Kconfig DEBUG_KENREL that is selected by DEBUG_KERNEL. Sent the e-mail too quickly, sorry. I meant adding DEBUG_XEN selected by DEBUG_KERNEL. Cheers
On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote: > Yup, > > Following is wrong: > > DEBUG macro could be globally across drivers defined by > > CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile. > DEBUG is not defined globally. And there is no debug option to enable > DEBUG in drivers/xen/Makefile. > Should it be added? I am not exactly sure why you guys insist on having this, but the lets leave it as #if 0 along with a comment saying why and what the purpose of this is. > > Sincerely, > Andrii Anisov.
Hi Konrad, On 01/02/2017 14:25, Konrad Rzeszutek Wilk wrote: > On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote: >> Yup, >> >> Following is wrong: >>> DEBUG macro could be globally across drivers defined by >>> CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile. >> DEBUG is not defined globally. And there is no debug option to enable >> DEBUG in drivers/xen/Makefile. >> Should it be added? > > I am not exactly sure why you guys insist on having this, but > the lets leave it as #if 0 along with a comment saying why and what > the purpose of this is. From my understanding, the function has been implemented with the assumption of the page will always belongs to the domain and not foreign. A user will unlikely know that it needs to remove #if 0 in order to check the driver is doing the right thing. So I think we should have an easy way to enable this code in kernel build. If you are not happy with the Kconfig, what would be the other way to have this enabled without requiring the user to modify the code? Cheers,
On Wed, Feb 01, 2017 at 02:30:53PM +0000, Julien Grall wrote: > Hi Konrad, > > On 01/02/2017 14:25, Konrad Rzeszutek Wilk wrote: > > On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote: > > > Yup, > > > > > > Following is wrong: > > > > DEBUG macro could be globally across drivers defined by > > > > CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile. > > > DEBUG is not defined globally. And there is no debug option to enable > > > DEBUG in drivers/xen/Makefile. > > > Should it be added? > > > > I am not exactly sure why you guys insist on having this, but > > the lets leave it as #if 0 along with a comment saying why and what > > the purpose of this is. > > From my understanding, the function has been implemented with the assumption > of the page will always belongs to the domain and not foreign. > > A user will unlikely know that it needs to remove #if 0 in order to check > the driver is doing the right thing. So I think we should have an easy way > to enable this code in kernel build. > > If you are not happy with the Kconfig, what would be the other way to have > this enabled without requiring the user to modify the code? Just leave the #if 0 in the code and add a comment explaining why it is there. > > Cheers, > > -- > Julien Grall
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. + */ +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 + 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); + #endif /* __LINUX_SWIOTLB_XEN_H */