| Message ID | alpine.DEB.2.02.1411061239520.22875@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
On 06/11/2014 12:42, Stefano Stabellini wrote: > On Thu, 6 Nov 2014, Julien Grall wrote: >> >> On 06/11/2014 11:46, Stefano Stabellini wrote: >>> Hello Julien, >> >> Hi Stefano, >> >>> I didn't manage to reproduce the issue you are seeing but I think you >>> are right: non-LPAE kernels could get in trouble by calling >>> xen_bus_to_phys. This problem is not ARM specific per se, but it doesn't >>> occur on x86 because config XEN depends on X86_32 && X86_PAE. >>> >>> The following patch should fix the issue: >>> >>> >>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c >>> index ac5d41b..489620d 100644 >>> --- a/drivers/xen/swiotlb-xen.c >>> +++ b/drivers/xen/swiotlb-xen.c >>> @@ -96,8 +96,6 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t >>> baddr) >>> dma_addr_t dma = (dma_addr_t)pfn << PAGE_SHIFT; >>> phys_addr_t paddr = dma; >>> >>> - BUG_ON(paddr != dma); /* truncation has occurred, should never happen >>> */ >>> - >> >> Are you sure that removing this BUG_ON is safe? There is some of place where >> we pass a physical address rather than a DMA. See xen_swiotlb_sync_single. > > Yes, it is safe, but we do need to change those call sites to pass > dev_addr instead, like I have done with unmap_single in the patch I > appended in the last email: Ok. I will give a look to modify all the call site. Regards, > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index ac5d41b..489620d 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -449,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, dma_addr_t dev_addr, > > BUG_ON(dir == DMA_NONE); > > - xen_dma_unmap_page(hwdev, paddr, size, dir, attrs); > + xen_dma_unmap_page(hwdev, dev_addr, size, dir, attrs); > > /* NOTE: We use dev_addr here, not paddr! */ > if (is_xen_swiotlb_buffer(dev_addr)) { >
On Thu, 6 Nov 2014, Julien Grall wrote: > On 06/11/2014 12:42, Stefano Stabellini wrote: > > On Thu, 6 Nov 2014, Julien Grall wrote: > > > > > > On 06/11/2014 11:46, Stefano Stabellini wrote: > > > > Hello Julien, > > > > > > Hi Stefano, > > > > > > > I didn't manage to reproduce the issue you are seeing but I think you > > > > are right: non-LPAE kernels could get in trouble by calling > > > > xen_bus_to_phys. This problem is not ARM specific per se, but it doesn't > > > > occur on x86 because config XEN depends on X86_32 && X86_PAE. > > > > > > > > The following patch should fix the issue: > > > > > > > > > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > > > index ac5d41b..489620d 100644 > > > > --- a/drivers/xen/swiotlb-xen.c > > > > +++ b/drivers/xen/swiotlb-xen.c > > > > @@ -96,8 +96,6 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t > > > > baddr) > > > > dma_addr_t dma = (dma_addr_t)pfn << PAGE_SHIFT; > > > > phys_addr_t paddr = dma; > > > > > > > > - BUG_ON(paddr != dma); /* truncation has occurred, should never happen > > > > */ > > > > - > > > > > > Are you sure that removing this BUG_ON is safe? There is some of place > > > where > > > we pass a physical address rather than a DMA. See xen_swiotlb_sync_single. > > > > Yes, it is safe, but we do need to change those call sites to pass > > dev_addr instead, like I have done with unmap_single in the patch I > > appended in the last email: > > Ok. I will give a look to modify all the call site. No need, I am already writing a new patch to be part of the next version of my series. > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index ac5d41b..489620d 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -449,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, > > dma_addr_t dev_addr, > > > > BUG_ON(dir == DMA_NONE); > > > > - xen_dma_unmap_page(hwdev, paddr, size, dir, attrs); > > + xen_dma_unmap_page(hwdev, dev_addr, size, dir, attrs); > > > > /* NOTE: We use dev_addr here, not paddr! */ > > if (is_xen_swiotlb_buffer(dev_addr)) { > > > > -- > Julien Grall >
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index ac5d41b..489620d 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -449,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, dma_addr_t dev_addr, BUG_ON(dir == DMA_NONE); - xen_dma_unmap_page(hwdev, paddr, size, dir, attrs); + xen_dma_unmap_page(hwdev, dev_addr, size, dir, attrs); /* NOTE: We use dev_addr here, not paddr! */ if (is_xen_swiotlb_buffer(dev_addr)) {