Hi all,
This series is a collection of fixes to get Linux running on the RPi4 as
dom0.
Conceptually there are only two significant changes:
- make sure not to call virt_to_page on vmalloc virt addresses (patch
#1)
- use phys_to_dma and dma_to_phys to translate phys to/from dma
addresses (all other patches)
In particular in regards to the second part, the RPi4 is the first
board where Xen can run that has the property that dma addresses are
different from physical addresses, and swiotlb-xen was written with the
assumption that phys addr == dma addr.
This series adds the phys_to_dma and dma_to_phys calls to make it work.
Cheers,
Stefano
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
---
drivers/xen/swiotlb-xen.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index b5e0492b07b9..958ee5517e0b 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -78,9 +78,9 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
return paddr;
}
-static inline dma_addr_t xen_virt_to_bus(void *address)
+static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(virt_to_phys(address));
+ return xen_phys_to_bus(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = *dma_handle;
- dev_addr = xen_phys_to_bus(phys);
+ dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -367,7 +367,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(phys);
+ dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -392,7 +392,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(map);
+ dev_addr = xen_phys_to_bus(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -536,7 +536,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_virt_to_bus(xen_io_tlb_end - 1) <= mask;
+ return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.17.1
From: Stefano Stabellini <[email protected]>
It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
instead. It will be useful not to have a start_dma_addr around with the
next patches.
Signed-off-by: Stefano Stabellini <[email protected]>
---
drivers/xen/swiotlb-xen.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a42129cba36e..b5e0492b07b9 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static u64 start_dma_addr;
-
/*
* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
* can be 32bit when dma_addr_t is 64bit leading to a loss in
@@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
m_ret = XEN_SWIOTLB_EFIXUP;
goto error;
}
- start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
if (early) {
if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
verbose))
@@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
+ map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), phys,
size, size, dir, attrs);
if (map == (phys_addr_t)DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
--
2.17.1
From: Boris Ostrovsky <[email protected]>
Don't just assume that virt_to_page works on all virtual addresses.
Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
virt addresses.
Signed-off-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
---
drivers/xen/swiotlb-xen.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index b6d27762c6f8..a42129cba36e 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
int order = get_order(size);
phys_addr_t phys;
u64 dma_mask = DMA_BIT_MASK(32);
+ struct page *pg;
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
@@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
+ pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
+ virt_to_page(vaddr);
if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
range_straddles_page_boundary(phys, size)) &&
- TestClearPageXenRemapped(virt_to_page(vaddr)))
+ TestClearPageXenRemapped(pg))
xen_destroy_contiguous_region(phys, order);
xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
--
2.17.1
On Wed, May 20, 2020 at 4:45 PM Stefano Stabellini
<[email protected]> wrote:
>
> Hi all,
>
> This series is a collection of fixes to get Linux running on the RPi4 as
> dom0.
>
> Conceptually there are only two significant changes:
>
> - make sure not to call virt_to_page on vmalloc virt addresses (patch
> #1)
> - use phys_to_dma and dma_to_phys to translate phys to/from dma
> addresses (all other patches)
>
> In particular in regards to the second part, the RPi4 is the first
> board where Xen can run that has the property that dma addresses are
> different from physical addresses, and swiotlb-xen was written with the
> assumption that phys addr == dma addr.
>
> This series adds the phys_to_dma and dma_to_phys calls to make it work.
Great to see this! Stefano, any chance you can put it in a branch some place
so I can test the final version?
Thanks,
Roman.
On Wed, 20 May 2020, Roman Shaposhnik wrote:
> On Wed, May 20, 2020 at 4:45 PM Stefano Stabellini
> <[email protected]> wrote:
> >
> > Hi all,
> >
> > This series is a collection of fixes to get Linux running on the RPi4 as
> > dom0.
> >
> > Conceptually there are only two significant changes:
> >
> > - make sure not to call virt_to_page on vmalloc virt addresses (patch
> > #1)
> > - use phys_to_dma and dma_to_phys to translate phys to/from dma
> > addresses (all other patches)
> >
> > In particular in regards to the second part, the RPi4 is the first
> > board where Xen can run that has the property that dma addresses are
> > different from physical addresses, and swiotlb-xen was written with the
> > assumption that phys addr == dma addr.
> >
> > This series adds the phys_to_dma and dma_to_phys calls to make it work.
>
> Great to see this! Stefano, any chance you can put it in a branch some place
> so I can test the final version?
Here it is, but keep in mind that it is based on Linux master (because
it is meant to go upstream):
git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git fix-rip4-v1
Hi,
On 21/05/2020 00:45, Stefano Stabellini wrote:
> From: Boris Ostrovsky <[email protected]>
>
> Don't just assume that virt_to_page works on all virtual addresses.
> Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
> virt addresses.
Can you provide an example where swiotlb is used with vmalloc()?
> Signed-off-by: Boris Ostrovsky <[email protected]>
> Signed-off-by: Stefano Stabellini <[email protected]>
> ---
> drivers/xen/swiotlb-xen.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index b6d27762c6f8..a42129cba36e 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> int order = get_order(size);
> phys_addr_t phys;
> u64 dma_mask = DMA_BIT_MASK(32);
> + struct page *pg;
>
> if (hwdev && hwdev->coherent_dma_mask)
> dma_mask = hwdev->coherent_dma_mask;
> @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> /* Convert the size to actually allocated. */
> size = 1UL << (order + XEN_PAGE_SHIFT);
>
> + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
> + virt_to_page(vaddr);
Common DMA code seems to protect this check with CONFIG_DMA_REMAP. Is it
something we want to do it here as well? Or is there any other condition
where vmalloc can happen?
> if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
> range_straddles_page_boundary(phys, size)) &&
> - TestClearPageXenRemapped(virt_to_page(vaddr)))
> + TestClearPageXenRemapped(pg))
> xen_destroy_contiguous_region(phys, order);
>
> xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
>
Cheers,
--
Julien Grall
Hi,
On 21/05/2020 00:45, Stefano Stabellini wrote:
> From: Stefano Stabellini <[email protected]>
>
> It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
> instead. It will be useful not to have a start_dma_addr around with the
> next patches.
>
> Signed-off-by: Stefano Stabellini <[email protected]>
> ---
> drivers/xen/swiotlb-xen.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index a42129cba36e..b5e0492b07b9 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
> * Quick lookup value of the bus address of the IOTLB.
> */
>
> -static u64 start_dma_addr;
> -
> /*
> * Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
> * can be 32bit when dma_addr_t is 64bit leading to a loss in
> @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> m_ret = XEN_SWIOTLB_EFIXUP;
> goto error;
> }
> - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
> if (early) {
> if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
> verbose))
> @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
> */
> trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>
> - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
> + map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), phys,
xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can
you explain how the two are equivalent?
Cheers,
--
Julien Grall
On Thu, 21 May 2020, Julien Grall wrote:
> Hi,
>
> On 21/05/2020 00:45, Stefano Stabellini wrote:
> > From: Boris Ostrovsky <[email protected]>
> >
> > Don't just assume that virt_to_page works on all virtual addresses.
> > Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
> > virt addresses.
>
> Can you provide an example where swiotlb is used with vmalloc()?
The issue was reported here happening on the Rasperry Pi 4:
https://marc.info/?l=xen-devel&m=158862573216800
If you are asking where in the Linux codebase the vmalloc is happening
specifically, I don't know for sure, my information is limited to the
stack trace that you see in the link (I don't have a Rasperry Pi 4 yet
but I shall have one soon.)
> > Signed-off-by: Boris Ostrovsky <[email protected]>
> > Signed-off-by: Stefano Stabellini <[email protected]>
> > ---
> > drivers/xen/swiotlb-xen.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index b6d27762c6f8..a42129cba36e 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
> > size, void *vaddr,
> > int order = get_order(size);
> > phys_addr_t phys;
> > u64 dma_mask = DMA_BIT_MASK(32);
> > + struct page *pg;
> > if (hwdev && hwdev->coherent_dma_mask)
> > dma_mask = hwdev->coherent_dma_mask;
> > @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
> > size, void *vaddr,
> > /* Convert the size to actually allocated. */
> > size = 1UL << (order + XEN_PAGE_SHIFT);
> > + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
> > + virt_to_page(vaddr);
>
> Common DMA code seems to protect this check with CONFIG_DMA_REMAP. Is it
> something we want to do it here as well? Or is there any other condition where
> vmalloc can happen?
I can see it in dma_direct_free_pages:
if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr))
vunmap(cpu_addr);
I wonder why the common DMA code does that. is_vmalloc_addr should work
regardless of CONFIG_DMA_REMAP. Maybe just for efficiency?
On Thu, 21 May 2020, Julien Grall wrote:
> Hi,
>
> On 21/05/2020 00:45, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[email protected]>
> >
> > It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
> > instead. It will be useful not to have a start_dma_addr around with the
> > next patches.
> >
> > Signed-off-by: Stefano Stabellini <[email protected]>
> > ---
> > drivers/xen/swiotlb-xen.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index a42129cba36e..b5e0492b07b9 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
> > * Quick lookup value of the bus address of the IOTLB.
> > */
> > -static u64 start_dma_addr;
> > -
> > /*
> > * Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
> > * can be 32bit when dma_addr_t is 64bit leading to a loss in
> > @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> > m_ret = XEN_SWIOTLB_EFIXUP;
> > goto error;
> > }
> > - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
> > if (early) {
> > if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
> > verbose))
> > @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> > *dev, struct page *page,
> > */
> > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
> > - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
> > + map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
> > phys,
>
> xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can you
> explain how the two are equivalent?
They are not equivalent. Looking at what swiotlb_tbl_map_single expects,
and also the implementation of swiotlb_init_with_tbl, I think
virt_to_phys is actually the one we want.
swiotlb_tbl_map_single compares the argument with __pa(tlb) which is
__pa(xen_io_tlb_start) which is virt_to_phys(xen_io_tlb_start).
Hi Stefano,
On 22/05/2020 04:54, Stefano Stabellini wrote:
> On Thu, 21 May 2020, Julien Grall wrote:
>> Hi,
>>
>> On 21/05/2020 00:45, Stefano Stabellini wrote:
>>> From: Boris Ostrovsky <[email protected]>
>>>
>>> Don't just assume that virt_to_page works on all virtual addresses.
>>> Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
>>> virt addresses.
>>
>> Can you provide an example where swiotlb is used with vmalloc()?
>
> The issue was reported here happening on the Rasperry Pi 4:
> https://marc.info/?l=xen-devel&m=158862573216800
Thanks, it would be good if the commit message contains a bit more details.
>
> If you are asking where in the Linux codebase the vmalloc is happening
> specifically, I don't know for sure, my information is limited to the
> stack trace that you see in the link (I don't have a Rasperry Pi 4 yet
> but I shall have one soon.)
Looking at the code there is a comment in xen_swiotlb_alloc_coherent()
suggesting that xen_alloc_coherent_pages() may return an ioremap'ped
region on Arm. So it feels to me that commit
b877ac9815a8fe7e5f6d7fdde3dc34652408840a "xen/swiotlb: remember having
called xen_create_contiguous_region()" has always been broken on Arm.
As an aside, your commit message also suggests this is an issue for
every virtual address used in swiotlb. But only the virt_to_page() call
in xen_swiotlb_free_coherent() is modified. Is it intended? If yes, I
think you want to update your commit message.
>
>
>>> Signed-off-by: Boris Ostrovsky <[email protected]>
>>> Signed-off-by: Stefano Stabellini <[email protected]>
>>> ---
>>> drivers/xen/swiotlb-xen.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>> index b6d27762c6f8..a42129cba36e 100644
>>> --- a/drivers/xen/swiotlb-xen.c
>>> +++ b/drivers/xen/swiotlb-xen.c
>>> @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
>>> size, void *vaddr,
>>> int order = get_order(size);
>>> phys_addr_t phys;
>>> u64 dma_mask = DMA_BIT_MASK(32);
>>> + struct page *pg;
>>> if (hwdev && hwdev->coherent_dma_mask)
>>> dma_mask = hwdev->coherent_dma_mask;
>>> @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
>>> size, void *vaddr,
>>> /* Convert the size to actually allocated. */
>>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>> + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
>>> + virt_to_page(vaddr);
>>
>> Common DMA code seems to protect this check with CONFIG_DMA_REMAP. Is it
>> something we want to do it here as well? Or is there any other condition where
>> vmalloc can happen?
>
> I can see it in dma_direct_free_pages:
>
> if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr))
> vunmap(cpu_addr);
>
> I wonder why the common DMA code does that. is_vmalloc_addr should work
> regardless of CONFIG_DMA_REMAP. Maybe just for efficiency?
is_vmalloc_addr() doesn't looks very expensive (although it is not an
inline function). So I am not sure the reason behind it. It might be
worth asking the author of the config.
Cheers,
--
Julien Grall
Hi,
On 22/05/2020 04:55, Stefano Stabellini wrote:
> On Thu, 21 May 2020, Julien Grall wrote:
>> Hi,
>>
>> On 21/05/2020 00:45, Stefano Stabellini wrote:
>>> From: Stefano Stabellini <[email protected]>
>>>
>>> It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
>>> instead. It will be useful not to have a start_dma_addr around with the
>>> next patches.
>>>
>>> Signed-off-by: Stefano Stabellini <[email protected]>
>>> ---
>>> drivers/xen/swiotlb-xen.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>>> index a42129cba36e..b5e0492b07b9 100644
>>> --- a/drivers/xen/swiotlb-xen.c
>>> +++ b/drivers/xen/swiotlb-xen.c
>>> @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
>>> * Quick lookup value of the bus address of the IOTLB.
>>> */
>>> -static u64 start_dma_addr;
>>> -
>>> /*
>>> * Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
>>> * can be 32bit when dma_addr_t is 64bit leading to a loss in
>>> @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
>>> m_ret = XEN_SWIOTLB_EFIXUP;
>>> goto error;
>>> }
>>> - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
>>> if (early) {
>>> if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
>>> verbose))
>>> @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
>>> *dev, struct page *page,
>>> */
>>> trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>>> - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
>>> + map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
>>> phys,
>>
>> xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can you
>> explain how the two are equivalent?
>
> They are not equivalent. Looking at what swiotlb_tbl_map_single expects,
> and also the implementation of swiotlb_init_with_tbl, I think
> virt_to_phys is actually the one we want.
>
> swiotlb_tbl_map_single compares the argument with __pa(tlb) which is
> __pa(xen_io_tlb_start) which is virt_to_phys(xen_io_tlb_start).
I can't find such check in master. What is your baseline? Could you
point to the exact line of code?
Cheers,
--
Julien Grall
On Fri, 22 May 2020, Julien Grall wrote:
> Hi Stefano,
>
> On 22/05/2020 04:54, Stefano Stabellini wrote:
> > On Thu, 21 May 2020, Julien Grall wrote:
> > > Hi,
> > >
> > > On 21/05/2020 00:45, Stefano Stabellini wrote:
> > > > From: Boris Ostrovsky <[email protected]>
> > > >
> > > > Don't just assume that virt_to_page works on all virtual addresses.
> > > > Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
> > > > virt addresses.
> > >
> > > Can you provide an example where swiotlb is used with vmalloc()?
> >
> > The issue was reported here happening on the Rasperry Pi 4:
> > https://marc.info/?l=xen-devel&m=158862573216800
>
> Thanks, it would be good if the commit message contains a bit more details.
>
> >
> > If you are asking where in the Linux codebase the vmalloc is happening
> > specifically, I don't know for sure, my information is limited to the
> > stack trace that you see in the link (I don't have a Rasperry Pi 4 yet
> > but I shall have one soon.)
>
> Looking at the code there is a comment in xen_swiotlb_alloc_coherent()
> suggesting that xen_alloc_coherent_pages() may return an ioremap'ped region on
> Arm. So it feels to me that commit b877ac9815a8fe7e5f6d7fdde3dc34652408840a
> "xen/swiotlb: remember having called xen_create_contiguous_region()" has
> always been broken on Arm.
Yes, I think you are right
> As an aside, your commit message also suggests this is an issue for every
> virtual address used in swiotlb. But only the virt_to_page() call in
> xen_swiotlb_free_coherent() is modified. Is it intended? If yes, I think you
> want to update your commit message.
I see, yes I can explain better
On Fri, 22 May 2020, Julien Grall wrote:
> On 22/05/2020 04:55, Stefano Stabellini wrote:
> > On Thu, 21 May 2020, Julien Grall wrote:
> > > Hi,
> > >
> > > On 21/05/2020 00:45, Stefano Stabellini wrote:
> > > > From: Stefano Stabellini <[email protected]>
> > > >
> > > > It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
> > > > instead. It will be useful not to have a start_dma_addr around with the
> > > > next patches.
> > > >
> > > > Signed-off-by: Stefano Stabellini <[email protected]>
> > > > ---
> > > > drivers/xen/swiotlb-xen.c | 5 +----
> > > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > > index a42129cba36e..b5e0492b07b9 100644
> > > > --- a/drivers/xen/swiotlb-xen.c
> > > > +++ b/drivers/xen/swiotlb-xen.c
> > > > @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
> > > > * Quick lookup value of the bus address of the IOTLB.
> > > > */
> > > > -static u64 start_dma_addr;
> > > > -
> > > > /*
> > > > * Both of these functions should avoid XEN_PFN_PHYS because
> > > > phys_addr_t
> > > > * can be 32bit when dma_addr_t is 64bit leading to a loss in
> > > > @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> > > > m_ret = XEN_SWIOTLB_EFIXUP;
> > > > goto error;
> > > > }
> > > > - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
> > > > if (early) {
> > > > if (swiotlb_init_with_tbl(xen_io_tlb_start,
> > > > xen_io_tlb_nslabs,
> > > > verbose))
> > > > @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> > > > *dev, struct page *page,
> > > > */
> > > > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
> > > > - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
> > > > + map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
> > > > phys,
> > >
> > > xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can
> > > you
> > > explain how the two are equivalent?
> >
> > They are not equivalent. Looking at what swiotlb_tbl_map_single expects,
> > and also the implementation of swiotlb_init_with_tbl, I think
> > virt_to_phys is actually the one we want.
> >
> > swiotlb_tbl_map_single compares the argument with __pa(tlb) which is
> > __pa(xen_io_tlb_start) which is virt_to_phys(xen_io_tlb_start).
>
> I can't find such check in master. What is your baseline? Could you point to
> the exact line of code?
My base is b85051e755b0e9d6dd8f17ef1da083851b83287d, which is master
from a couple of days back.
xen_swiotlb_init calls swiotlb_init_with_tbl which takes a virt address
as a parameter (xen_io_tlb_start), it gets converted to phys and stored
in io_tlb_start as a physical address.
Later, xen_swiotlb_map_page calls swiotlb_tbl_map_single passing a
dma addr as parameter (tbl_dma_addr). tbl_dma_addr is used to calculate
the right slot in the swiotlb buffer to use. (Strangely tbl_dma_addr is
a dma_addr_t and it is not converted to phys_addr_t before doing
operations... I think tbl_dma_addr should be a phys addr.)
The comparison with io_tlb_start is done here:
do {
while (iommu_is_span_boundary(index, nslots, offset_slots,
max_slots)) {
index += stride;
if (index >= io_tlb_nslabs)
index = 0;
if (index == wrap)
goto not_found;
}
index is io_tlb_start and offset_slots is derived by tbl_dma_addr.
I would like to ask the maintainers, Juergen, Boris, Konrad, whether you
have any more feedback before I send v2 of the series.
Cheers,
Stefano
On Wed, 20 May 2020, Stefano Stabellini wrote:
> Hi all,
>
> This series is a collection of fixes to get Linux running on the RPi4 as
> dom0.
>
> Conceptually there are only two significant changes:
>
> - make sure not to call virt_to_page on vmalloc virt addresses (patch
> #1)
> - use phys_to_dma and dma_to_phys to translate phys to/from dma
> addresses (all other patches)
>
> In particular in regards to the second part, the RPi4 is the first
> board where Xen can run that has the property that dma addresses are
> different from physical addresses, and swiotlb-xen was written with the
> assumption that phys addr == dma addr.
>
> This series adds the phys_to_dma and dma_to_phys calls to make it work.
>
>
> Cheers,
>
> Stefano
>
On 6/2/20 5:51 PM, Stefano Stabellini wrote:
> I would like to ask the maintainers, Juergen, Boris, Konrad, whether you
> have any more feedback before I send v2 of the series.
I think I only had one comment and that's all. Most were from Julien.
-boris
>
> Cheers,
>
> Stefano
>
>
> On Wed, 20 May 2020, Stefano Stabellini wrote:
>> Hi all,
>>
>> This series is a collection of fixes to get Linux running on the RPi4 as
>> dom0.
>>
>> Conceptually there are only two significant changes:
>>
>> - make sure not to call virt_to_page on vmalloc virt addresses (patch
>> #1)
>> - use phys_to_dma and dma_to_phys to translate phys to/from dma
>> addresses (all other patches)
>>
>> In particular in regards to the second part, the RPi4 is the first
>> board where Xen can run that has the property that dma addresses are
>> different from physical addresses, and swiotlb-xen was written with the
>> assumption that phys addr == dma addr.
>>
>> This series adds the phys_to_dma and dma_to_phys calls to make it work.
>>
>>
>> Cheers,
>>
>> Stefano
>>