* [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region @ 2020-04-28 7:33 peng.fan 2020-04-28 8:02 ` Christoph Hellwig 2020-04-28 8:09 ` Jürgen Groß 0 siblings, 2 replies; 9+ messages in thread From: peng.fan @ 2020-04-28 7:33 UTC (permalink / raw) To: konrad.wilk, boris.ostrovsky, jgross, sstabellini Cc: xen-devel, iommu, Peng Fan, linux-kernel, linux-imx From: Peng Fan <peng.fan@nxp.com> When booting xen on i.MX8QM, met: " [ 3.602128] Unable to handle kernel paging request at virtual address 0000000000272d40 [ 3.610804] Mem abort info: [ 3.613905] ESR = 0x96000004 [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits [ 3.623211] SET = 0, FnV = 0 [ 3.626628] EA = 0, S1PTW = 0 [ 3.630128] Data abort info: [ 3.633362] ISV = 0, ISS = 0x00000004 [ 3.637630] CM = 0, WnR = 0 [ 3.640955] [0000000000272d40] user address but active_mm is swapper [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 3.654137] Modules linked in: [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) [ 3.677302] Workqueue: events deferred_probe_work_func [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 " In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or range_straddles_page_boundary(phys, size) are true, it will create contiguous region. So when free, we need to free contiguous region use upper check condition. Signed-off-by: Peng Fan <peng.fan@nxp.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index b6d27762c6f8..ab96e468584f 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || - range_straddles_page_boundary(phys, size)) && + if (((dev_addr + size - 1 > dma_mask) || + range_straddles_page_boundary(phys, size)) && TestClearPageXenRemapped(virt_to_page(vaddr))) xen_destroy_contiguous_region(phys, order); -- 2.16.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 7:33 [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region peng.fan @ 2020-04-28 8:02 ` Christoph Hellwig 2020-04-28 8:21 ` Peng Fan 2020-04-28 8:09 ` Jürgen Groß 1 sibling, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2020-04-28 8:02 UTC (permalink / raw) To: peng.fan Cc: jgross, sstabellini, konrad.wilk, linux-kernel, iommu, linux-imx, xen-devel, boris.ostrovsky On Tue, Apr 28, 2020 at 03:33:45PM +0800, peng.fan@nxp.com wrote: > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or > range_straddles_page_boundary(phys, size) are true, it will > create contiguous region. So when free, we need to free contiguous > region use upper check condition. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/xen/swiotlb-xen.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index b6d27762c6f8..ab96e468584f 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > - range_straddles_page_boundary(phys, size)) && > + if (((dev_addr + size - 1 > dma_mask) || > + range_straddles_page_boundary(phys, size)) && > TestClearPageXenRemapped(virt_to_page(vaddr))) No need for the inner braces. But more importantly please factor our a helper that can be used by alloc and free to make sure that they always stay in sync. Something like: static inline bool xen_swiotlb_need_contiguous_region(struct device *dev, phys_addr_t phys, size_t size) { return xen_phys_to_bus(phys) + size - 1 > dev->coherent_dma_mask || range_straddles_page_boundary(phys, size)) } ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 8:02 ` Christoph Hellwig @ 2020-04-28 8:21 ` Peng Fan 0 siblings, 0 replies; 9+ messages in thread From: Peng Fan @ 2020-04-28 8:21 UTC (permalink / raw) To: Christoph Hellwig Cc: jgross, sstabellini, konrad.wilk, linux-kernel, iommu, dl-linux-imx, xen-devel, boris.ostrovsky > Subject: Re: [PATCH] xen/swiotlb: correct the check for > xen_destroy_contiguous_region > > On Tue, Apr 28, 2020 at 03:33:45PM +0800, peng.fan@nxp.com wrote: > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) > > or range_straddles_page_boundary(phys, size) are true, it will create > > contiguous region. So when free, we need to free contiguous region use > > upper check condition. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/xen/swiotlb-xen.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..ab96e468584f 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, > size_t size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > - range_straddles_page_boundary(phys, size)) && > > + if (((dev_addr + size - 1 > dma_mask) || > > + range_straddles_page_boundary(phys, size)) && > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > No need for the inner braces. > > But more importantly please factor our a helper that can be used by alloc and > free to make sure that they always stay in sync. Something Thanks for reviewing. I'll take your suggestion in v2. Before that, I would wait to see if there are more comments in this patch, because there are several history commits touching this place. Thanks, Peng. > like: > > static inline bool xen_swiotlb_need_contiguous_region(struct device *dev, > phys_addr_t phys, size_t size) > { > > return xen_phys_to_bus(phys) + size - 1 > dev->coherent_dma_mask || > range_straddles_page_boundary(phys, size)) } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 7:33 [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region peng.fan 2020-04-28 8:02 ` Christoph Hellwig @ 2020-04-28 8:09 ` Jürgen Groß 2020-04-28 8:25 ` Peng Fan 2020-04-28 18:46 ` Stefano Stabellini 1 sibling, 2 replies; 9+ messages in thread From: Jürgen Groß @ 2020-04-28 8:09 UTC (permalink / raw) To: peng.fan, konrad.wilk, boris.ostrovsky, sstabellini Cc: xen-devel, iommu, linux-kernel, linux-imx On 28.04.20 09:33, peng.fan@nxp.com wrote: > From: Peng Fan <peng.fan@nxp.com> > > When booting xen on i.MX8QM, met: > " > [ 3.602128] Unable to handle kernel paging request at virtual address 0000000000272d40 > [ 3.610804] Mem abort info: > [ 3.613905] ESR = 0x96000004 > [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > [ 3.623211] SET = 0, FnV = 0 > [ 3.626628] EA = 0, S1PTW = 0 > [ 3.630128] Data abort info: > [ 3.633362] ISV = 0, ISS = 0x00000004 > [ 3.637630] CM = 0, WnR = 0 > [ 3.640955] [0000000000272d40] user address but active_mm is swapper > [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP > [ 3.654137] Modules linked in: > [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > [ 3.677302] Workqueue: events deferred_probe_work_func > [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 > [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) > [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] > [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > " > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or > range_straddles_page_boundary(phys, size) are true, it will > create contiguous region. So when free, we need to free contiguous > region use upper check condition. No, this will break PV guests on x86. I think there is something wrong with your setup in combination with the ARM xen_create_contiguous_region() implementation. Stefano? Juergen > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/xen/swiotlb-xen.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index b6d27762c6f8..ab96e468584f 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > - range_straddles_page_boundary(phys, size)) && > + if (((dev_addr + size - 1 > dma_mask) || > + range_straddles_page_boundary(phys, size)) && > TestClearPageXenRemapped(virt_to_page(vaddr))) > xen_destroy_contiguous_region(phys, order); > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 8:09 ` Jürgen Groß @ 2020-04-28 8:25 ` Peng Fan 2020-04-28 10:19 ` Jürgen Groß 2020-04-28 18:46 ` Stefano Stabellini 1 sibling, 1 reply; 9+ messages in thread From: Peng Fan @ 2020-04-28 8:25 UTC (permalink / raw) To: Jürgen Groß, konrad.wilk, boris.ostrovsky, sstabellini Cc: xen-devel, iommu, linux-kernel, dl-linux-imx > Subject: Re: [PATCH] xen/swiotlb: correct the check for > xen_destroy_contiguous_region > > On 28.04.20 09:33, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > When booting xen on i.MX8QM, met: > > " > > [ 3.602128] Unable to handle kernel paging request at virtual address > 0000000000272d40 > > [ 3.610804] Mem abort info: > > [ 3.613905] ESR = 0x96000004 > > [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 3.623211] SET = 0, FnV = 0 > > [ 3.626628] EA = 0, S1PTW = 0 > > [ 3.630128] Data abort info: > > [ 3.633362] ISV = 0, ISS = 0x00000004 > > [ 3.637630] CM = 0, WnR = 0 > > [ 3.640955] [0000000000272d40] user address but active_mm is > swapper > > [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP > > [ 3.654137] Modules linked in: > > [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > > [ 3.677302] Workqueue: events deferred_probe_work_func > > [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 > > [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) > > [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > > [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] > > [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > > " > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) > > or range_straddles_page_boundary(phys, size) are true, it will create > > contiguous region. So when free, we need to free contiguous region use > > upper check condition. > > No, this will break PV guests on x86. Could you share more details why alloc and free not matching for the check? Thanks, Peng. > > I think there is something wrong with your setup in combination with the ARM > xen_create_contiguous_region() implementation. > > Stefano? > > > Juergen > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/xen/swiotlb-xen.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..ab96e468584f 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, > size_t size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > - range_straddles_page_boundary(phys, size)) && > > + if (((dev_addr + size - 1 > dma_mask) || > > + range_straddles_page_boundary(phys, size)) && > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > xen_destroy_contiguous_region(phys, order); > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 8:25 ` Peng Fan @ 2020-04-28 10:19 ` Jürgen Groß 2020-04-28 17:25 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 9+ messages in thread From: Jürgen Groß @ 2020-04-28 10:19 UTC (permalink / raw) To: Peng Fan, konrad.wilk, boris.ostrovsky, sstabellini Cc: xen-devel, iommu, linux-kernel, dl-linux-imx On 28.04.20 10:25, Peng Fan wrote: >> Subject: Re: [PATCH] xen/swiotlb: correct the check for >> xen_destroy_contiguous_region >> >> On 28.04.20 09:33, peng.fan@nxp.com wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> When booting xen on i.MX8QM, met: >>> " >>> [ 3.602128] Unable to handle kernel paging request at virtual address >> 0000000000272d40 >>> [ 3.610804] Mem abort info: >>> [ 3.613905] ESR = 0x96000004 >>> [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits >>> [ 3.623211] SET = 0, FnV = 0 >>> [ 3.626628] EA = 0, S1PTW = 0 >>> [ 3.630128] Data abort info: >>> [ 3.633362] ISV = 0, ISS = 0x00000004 >>> [ 3.637630] CM = 0, WnR = 0 >>> [ 3.640955] [0000000000272d40] user address but active_mm is >> swapper >>> [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP >>> [ 3.654137] Modules linked in: >>> [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) >>> [ 3.677302] Workqueue: events deferred_probe_work_func >>> [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 >>> [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) >>> [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 >>> [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] >>> [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 >>> " >>> >>> In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) >>> or range_straddles_page_boundary(phys, size) are true, it will create >>> contiguous region. So when free, we need to free contiguous region use >>> upper check condition. >> >> No, this will break PV guests on x86. > > Could you share more details why alloc and free not matching for the check? xen_create_contiguous_region() is needed only in case: - the bus address is not within dma_mask, or - the memory region is not physically contiguous (can happen only for PV guests) In any case it should arrange for the memory to be suitable for the DMA operation, so to be contiguous and within dma_mask afterwards. So xen_destroy_contiguous_region() should only ever called for areas which match above criteria, as otherwise we can be sure xen_create_contiguous_region() was not used for making the area DMA-able in the beginning. And this is very important in the PV case, as in those guests the page tables are containing the host-PFNs, not the guest-PFNS, and xen_create_contiguous_region() will fiddle with host- vs. guest-PFN arrangements, and xen_destroy_contiguous_region() is reverting this fiddling. Any call of xen_destroy_contiguous_region() for an area it was not intended to be called for might swap physical pages beneath random virtual addresses, which was the reason for this test to be added by me. Juergen > > Thanks, > Peng. > >> >> I think there is something wrong with your setup in combination with the ARM >> xen_create_contiguous_region() implementation. >> >> Stefano? >> >> >> Juergen >> >>> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>> --- >>> drivers/xen/swiotlb-xen.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c >>> index b6d27762c6f8..ab96e468584f 100644 >>> --- a/drivers/xen/swiotlb-xen.c >>> +++ b/drivers/xen/swiotlb-xen.c >>> @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, >> size_t size, void *vaddr, >>> /* Convert the size to actually allocated. */ >>> size = 1UL << (order + XEN_PAGE_SHIFT); >>> >>> - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || >>> - range_straddles_page_boundary(phys, size)) && >>> + if (((dev_addr + size - 1 > dma_mask) || >>> + range_straddles_page_boundary(phys, size)) && >>> TestClearPageXenRemapped(virt_to_page(vaddr))) >>> xen_destroy_contiguous_region(phys, order); >>> >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 10:19 ` Jürgen Groß @ 2020-04-28 17:25 ` Konrad Rzeszutek Wilk 2020-04-28 18:37 ` Joe Jin 0 siblings, 1 reply; 9+ messages in thread From: Konrad Rzeszutek Wilk @ 2020-04-28 17:25 UTC (permalink / raw) To: Jürgen Groß, joe.jin Cc: Peng Fan, sstabellini, linux-kernel, iommu, dl-linux-imx, xen-devel, boris.ostrovsky On Tue, Apr 28, 2020 at 12:19:41PM +0200, Jürgen Groß wrote: > On 28.04.20 10:25, Peng Fan wrote: Adding Joe Jin. Joe, didn't you have some ideas on how this could be implemented? > > > Subject: Re: [PATCH] xen/swiotlb: correct the check for > > > xen_destroy_contiguous_region > > > > > > On 28.04.20 09:33, peng.fan@nxp.com wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > When booting xen on i.MX8QM, met: > > > > " > > > > [ 3.602128] Unable to handle kernel paging request at virtual address > > > 0000000000272d40 > > > > [ 3.610804] Mem abort info: > > > > [ 3.613905] ESR = 0x96000004 > > > > [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > > > > [ 3.623211] SET = 0, FnV = 0 > > > > [ 3.626628] EA = 0, S1PTW = 0 > > > > [ 3.630128] Data abort info: > > > > [ 3.633362] ISV = 0, ISS = 0x00000004 > > > > [ 3.637630] CM = 0, WnR = 0 > > > > [ 3.640955] [0000000000272d40] user address but active_mm is > > > swapper > > > > [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP > > > > [ 3.654137] Modules linked in: > > > > [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > > > > [ 3.677302] Workqueue: events deferred_probe_work_func > > > > [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 > > > > [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) > > > > [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > > > > [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] > > > > [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > > > > " > > > > > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) > > > > or range_straddles_page_boundary(phys, size) are true, it will create > > > > contiguous region. So when free, we need to free contiguous region use > > > > upper check condition. > > > > > > No, this will break PV guests on x86. > > > > Could you share more details why alloc and free not matching for the check? > > xen_create_contiguous_region() is needed only in case: > > - the bus address is not within dma_mask, or > - the memory region is not physically contiguous (can happen only for > PV guests) > > In any case it should arrange for the memory to be suitable for the > DMA operation, so to be contiguous and within dma_mask afterwards. So > xen_destroy_contiguous_region() should only ever called for areas > which match above criteria, as otherwise we can be sure > xen_create_contiguous_region() was not used for making the area DMA-able > in the beginning. > > And this is very important in the PV case, as in those guests the page > tables are containing the host-PFNs, not the guest-PFNS, and > xen_create_contiguous_region() will fiddle with host- vs. guest-PFN > arrangements, and xen_destroy_contiguous_region() is reverting this > fiddling. Any call of xen_destroy_contiguous_region() for an area it > was not intended to be called for might swap physical pages beneath > random virtual addresses, which was the reason for this test to be > added by me. > > > Juergen > > > > > Thanks, > > Peng. > > > > > > > > I think there is something wrong with your setup in combination with the ARM > > > xen_create_contiguous_region() implementation. > > > > > > Stefano? > > > > > > > > > Juergen > > > > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > --- > > > > drivers/xen/swiotlb-xen.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > > > index b6d27762c6f8..ab96e468584f 100644 > > > > --- a/drivers/xen/swiotlb-xen.c > > > > +++ b/drivers/xen/swiotlb-xen.c > > > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, > > > size_t size, void *vaddr, > > > > /* Convert the size to actually allocated. */ > > > > size = 1UL << (order + XEN_PAGE_SHIFT); > > > > > > > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > > > - range_straddles_page_boundary(phys, size)) && > > > > + if (((dev_addr + size - 1 > dma_mask) || > > > > + range_straddles_page_boundary(phys, size)) && > > > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > > > xen_destroy_contiguous_region(phys, order); > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 17:25 ` Konrad Rzeszutek Wilk @ 2020-04-28 18:37 ` Joe Jin 0 siblings, 0 replies; 9+ messages in thread From: Joe Jin @ 2020-04-28 18:37 UTC (permalink / raw) To: Konrad Rzeszutek Wilk, Jürgen Groß Cc: Peng Fan, sstabellini, linux-kernel, iommu, dl-linux-imx, xen-devel, boris.ostrovsky On 4/28/20 10:25 AM, Konrad Rzeszutek Wilk wrote: > On Tue, Apr 28, 2020 at 12:19:41PM +0200, Jürgen Groß wrote: >> On 28.04.20 10:25, Peng Fan wrote: > > Adding Joe Jin. > > Joe, didn't you have some ideas on how this could be implemented? > >>>> Subject: Re: [PATCH] xen/swiotlb: correct the check for >>>> xen_destroy_contiguous_region >>>> >>>> On 28.04.20 09:33, peng.fan@nxp.com wrote: >>>>> From: Peng Fan <peng.fan@nxp.com> >>>>> >>>>> When booting xen on i.MX8QM, met: >>>>> " >>>>> [ 3.602128] Unable to handle kernel paging request at virtual address >>>> 0000000000272d40 >>>>> [ 3.610804] Mem abort info: >>>>> [ 3.613905] ESR = 0x96000004 >>>>> [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits >>>>> [ 3.623211] SET = 0, FnV = 0 >>>>> [ 3.626628] EA = 0, S1PTW = 0 >>>>> [ 3.630128] Data abort info: >>>>> [ 3.633362] ISV = 0, ISS = 0x00000004 >>>>> [ 3.637630] CM = 0, WnR = 0 >>>>> [ 3.640955] [0000000000272d40] user address but active_mm is >>>> swapper >>>>> [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP >>>>> [ 3.654137] Modules linked in: >>>>> [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) >>>>> [ 3.677302] Workqueue: events deferred_probe_work_func >>>>> [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 >>>>> [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) >>>>> [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 >>>>> [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] >>>>> [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 >>>>> " >>>>> >>>>> In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) >>>>> or range_straddles_page_boundary(phys, size) are true, it will create >>>>> contiguous region. So when free, we need to free contiguous region use >>>>> upper check condition. >>>> >>>> No, this will break PV guests on x86. >>> >>> Could you share more details why alloc and free not matching for the check? >> >> xen_create_contiguous_region() is needed only in case: >> >> - the bus address is not within dma_mask, or >> - the memory region is not physically contiguous (can happen only for >> PV guests) >> >> In any case it should arrange for the memory to be suitable for the >> DMA operation, so to be contiguous and within dma_mask afterwards. So >> xen_destroy_contiguous_region() should only ever called for areas >> which match above criteria, as otherwise we can be sure >> xen_create_contiguous_region() was not used for making the area DMA-able >> in the beginning. I agreed with Juergen's explanation, That is my understanding. Peng, if panic caused by (dev_addr + size - 1 > dma_mask), you should check how you get the addr, if memory created by xen_create_contiguous_region(), memory must be with in [0 - dma_mask]. Thanks, Joe >> >> And this is very important in the PV case, as in those guests the page >> tables are containing the host-PFNs, not the guest-PFNS, and >> xen_create_contiguous_region() will fiddle with host- vs. guest-PFN >> arrangements, and xen_destroy_contiguous_region() is reverting this >> fiddling. Any call of xen_destroy_contiguous_region() for an area it >> was not intended to be called for might swap physical pages beneath >> random virtual addresses, which was the reason for this test to be >> added by me. >> >> >> Juergen >> >>> >>> Thanks, >>> Peng. >>> >>>> >>>> I think there is something wrong with your setup in combination with the ARM >>>> xen_create_contiguous_region() implementation. >>>> >>>> Stefano? >>>> >>>> >>>> Juergen >>>> >>>>> >>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>>>> --- >>>>> drivers/xen/swiotlb-xen.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c >>>>> index b6d27762c6f8..ab96e468584f 100644 >>>>> --- a/drivers/xen/swiotlb-xen.c >>>>> +++ b/drivers/xen/swiotlb-xen.c >>>>> @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, >>>> size_t size, void *vaddr, >>>>> /* Convert the size to actually allocated. */ >>>>> size = 1UL << (order + XEN_PAGE_SHIFT); >>>>> >>>>> - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || >>>>> - range_straddles_page_boundary(phys, size)) && >>>>> + if (((dev_addr + size - 1 > dma_mask) || >>>>> + range_straddles_page_boundary(phys, size)) && >>>>> TestClearPageXenRemapped(virt_to_page(vaddr))) >>>>> xen_destroy_contiguous_region(phys, order); >>>>> >>>>> >>> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region 2020-04-28 8:09 ` Jürgen Groß 2020-04-28 8:25 ` Peng Fan @ 2020-04-28 18:46 ` Stefano Stabellini 1 sibling, 0 replies; 9+ messages in thread From: Stefano Stabellini @ 2020-04-28 18:46 UTC (permalink / raw) To: Jürgen Groß Cc: peng.fan, sstabellini, konrad.wilk, linux-kernel, hch, iommu, linux-imx, xen-devel, boris.ostrovsky [-- Attachment #1: Type: text/plain, Size: 3850 bytes --] On Tue, 28 Apr 2020, Jürgen Groß wrote: > On 28.04.20 09:33, peng.fan@nxp.com wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > When booting xen on i.MX8QM, met: > > " > > [ 3.602128] Unable to handle kernel paging request at virtual address > > 0000000000272d40 > > [ 3.610804] Mem abort info: > > [ 3.613905] ESR = 0x96000004 > > [ 3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 3.623211] SET = 0, FnV = 0 > > [ 3.626628] EA = 0, S1PTW = 0 > > [ 3.630128] Data abort info: > > [ 3.633362] ISV = 0, ISS = 0x00000004 > > [ 3.637630] CM = 0, WnR = 0 > > [ 3.640955] [0000000000272d40] user address but active_mm is swapper > > [ 3.647983] Internal error: Oops: 96000004 [#1] PREEMPT SMP > > [ 3.654137] Modules linked in: > > [ 3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > > [ 3.677302] Workqueue: events deferred_probe_work_func > > [ 3.684253] imx6q-pcie 5f000000.pcie: PCI host bridge to bus 0000:00 > > [ 3.688297] pstate: 60000005 (nZCv daif -PAN -UAO) > > [ 3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > > [ 3.693993] pci_bus 0000:00: root bus resource [bus 00-ff] > > [ 3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > > " > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or > > range_straddles_page_boundary(phys, size) are true, it will > > create contiguous region. So when free, we need to free contiguous > > region use upper check condition. > > No, this will break PV guests on x86. > > I think there is something wrong with your setup in combination with > the ARM xen_create_contiguous_region() implementation. > > Stefano? Let me start by asking Peng a couple of questions: Peng, could you please add a printk to check which one of the two conditions is True for you? Is it (dev_addr + size - 1 > dma_mask) or range_straddles_page_boundary(phys, size)? Is hwdev->coherent_dma_mask set for your DMA capable device? Finally, is this patch supposed to fix the crash you are seeing? If not, can you tell where is the crash exactly? Juergen, keep in mind that xen_create_contiguous_region does nothing on ARM because in dom0 guest_phys == phys. xen_create_contiguous_region simply sets dma_handle to phys. Whatever condition caused the code to take the xen_create_contiguous_region branch in xen_swiotlb_alloc_coherent, it will also cause it to WARN in xen_swiotlb_free_coherent. range_straddles_page_boundary should never return True because guest_phys == phys. That leaves us with the dma_mask check: dev_addr + size - 1 <= dma_mask dev_addr is the dma_handle allocated by xen_alloc_coherent_pages. dma_mask is either DMA_BIT_MASK(32) or hwdev->coherent_dma_mask. The implementation of xen_alloc_coherent_pages has been recently changed to use dma_direct_alloc. Christoff, does dma_direct_alloc respect hwdev->coherent_dma_mask if present? Also, can it return highmem pages? > Juergen > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/xen/swiotlb-xen.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..ab96e468584f 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > - range_straddles_page_boundary(phys, size)) && > > + if (((dev_addr + size - 1 > dma_mask) || > > + range_straddles_page_boundary(phys, size)) && > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > xen_destroy_contiguous_region(phys, order); > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-28 18:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 7:33 [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region peng.fan 2020-04-28 8:02 ` Christoph Hellwig 2020-04-28 8:21 ` Peng Fan 2020-04-28 8:09 ` Jürgen Groß 2020-04-28 8:25 ` Peng Fan 2020-04-28 10:19 ` Jürgen Groß 2020-04-28 17:25 ` Konrad Rzeszutek Wilk 2020-04-28 18:37 ` Joe Jin 2020-04-28 18:46 ` Stefano Stabellini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).