[PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell]

The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.

This can happen in practice on ARM where foreign pages can be above 4GB even
though the local kernel does not have LPAE page tables enabled (which is
totally reasonable if the guest does not itself have >4GB of RAM). In this
case the kernel still maps the foreign pages at a phys addr below 4G (as it
must) but the resulting DMA address (returned by the grant map operation) is
much higher.

This is analogous to a hardware device which has its view of RAM mapped up
high for some reason.

This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
systems with more than 4GB of RAM.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/Kconfig          |    1 +
 drivers/xen/swiotlb-xen.c |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c1f1a7e..24307dc 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1885,6 +1885,7 @@ config XEN
 	depends on !GENERIC_ATOMIC64
 	select ARM_PSCI
 	select SWIOTLB_XEN
+	select ARCH_DMA_ADDR_T_64BIT
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
 
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 1eac073..b626c79 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -77,12 +77,22 @@ static u64 start_dma_addr;
 
 static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
 {
-	return phys_to_machine(XPADDR(paddr)).maddr;
+	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
+	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
+	dma |= paddr & ~PAGE_MASK;
+	return dma;
 }
 
 static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
 {
-	return machine_to_phys(XMADDR(baddr)).paddr;
+	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
+	phys_addr_t paddr = dma;
+
+	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
+
+	paddr |= baddr & ~PAGE_MASK;
+
+	return paddr;
 }
 
 static inline dma_addr_t xen_virt_to_bus(void *address)
-- 
1.7.10.4

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Stefano Stabellini]

On Fri, 17 Jan 2014, Ian Campbell wrote:
> The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
> 
> This can happen in practice on ARM where foreign pages can be above 4GB even
> though the local kernel does not have LPAE page tables enabled (which is
> totally reasonable if the guest does not itself have >4GB of RAM). In this
> case the kernel still maps the foreign pages at a phys addr below 4G (as it
> must) but the resulting DMA address (returned by the grant map operation) is
> much higher.

We are lucky that LPAE only supports 40 bits otherwise we would need to
change any other functions that use unsigned long for mfn, starting from
set_phys_to_machine.


> This is analogous to a hardware device which has its view of RAM mapped up
> high for some reason.
> 
> This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
> systems with more than 4GB of RAM.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/Kconfig          |    1 +
>  drivers/xen/swiotlb-xen.c |   14 ++++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c1f1a7e..24307dc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1885,6 +1885,7 @@ config XEN
>  	depends on !GENERIC_ATOMIC64
>  	select ARM_PSCI
>  	select SWIOTLB_XEN
> +	select ARCH_DMA_ADDR_T_64BIT
>  	help
>  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
>  
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 1eac073..b626c79 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -77,12 +77,22 @@ static u64 start_dma_addr;
>  
>  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
>  {
> -	return phys_to_machine(XPADDR(paddr)).maddr;
> +	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> +	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;

I'd add this comment:

/* avoid PFN_PHYS because phys_addr_t can be 32bit when dma_addr_t is
 * 64bit leading to a loss in information if the shift is done before
 * casting to 64bit. */

> +	dma |= paddr & ~PAGE_MASK;
> +	return dma;
>  }
>  
>  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
>  {
> -	return machine_to_phys(XMADDR(baddr)).paddr;
> +	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> +	phys_addr_t paddr = dma;
> +
> +	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */

This check is useless because PFN_PHYS contains a cast to (phys_addr_t)
that is 32 bit. I think you'll have to:

dma_addr_t dma = ((dma_addr_t)mfn_to_pfn(PFN_DOWN(baddr))) << PAGE_SHIFT;


> +	paddr |= baddr & ~PAGE_MASK;
> +
> +	return paddr;
>  }

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell]

On Fri, 2014-01-17 at 18:09 +0000, Stefano Stabellini wrote:
> On Fri, 17 Jan 2014, Ian Campbell wrote:
> > The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> > causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
> > 
> > This can happen in practice on ARM where foreign pages can be above 4GB even
> > though the local kernel does not have LPAE page tables enabled (which is
> > totally reasonable if the guest does not itself have >4GB of RAM). In this
> > case the kernel still maps the foreign pages at a phys addr below 4G (as it
> > must) but the resulting DMA address (returned by the grant map operation) is
> > much higher.
> 
> We are lucky that LPAE only supports 40 bits otherwise we would need to
> change any other functions that use unsigned long for mfn, starting from
> set_phys_to_machine.

Yes, the use of unsigned long for pfn is a larger problem for the kernel
on any 32-bit arch with LPAE type extensions, I think.

> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 1eac073..b626c79 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -77,12 +77,22 @@ static u64 start_dma_addr;
> >  
> >  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
> >  {
> > -	return phys_to_machine(XPADDR(paddr)).maddr;
> > +	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> > +	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
> 
> I'd add this comment:
> 
> /* avoid PFN_PHYS because phys_addr_t can be 32bit when dma_addr_t is
>  * 64bit leading to a loss in information if the shift is done before
>  * casting to 64bit. */
> 
> > +	dma |= paddr & ~PAGE_MASK;
> > +	return dma;
> >  }
> >  
> >  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
> >  {
> > -	return machine_to_phys(XMADDR(baddr)).paddr;
> > +	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> > +	phys_addr_t paddr = dma;
> > +
> > +	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> 
> This check is useless because PFN_PHYS contains a cast to (phys_addr_t)
> that is 32 bit. I think you'll have to:
> 
> dma_addr_t dma = ((dma_addr_t)mfn_to_pfn(PFN_DOWN(baddr))) << PAGE_SHIFT;

That's what I originally had, then I spotted PFN_PHYS and decided it
would be a nice cleanup -- oops!

v2 coming up.

Ian.

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Konrad Rzeszutek Wilk]

On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
> The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
> 
> This can happen in practice on ARM where foreign pages can be above 4GB even
> though the local kernel does not have LPAE page tables enabled (which is
> totally reasonable if the guest does not itself have >4GB of RAM). In this
> case the kernel still maps the foreign pages at a phys addr below 4G (as it
> must) but the resulting DMA address (returned by the grant map operation) is
> much higher.
> 
> This is analogous to a hardware device which has its view of RAM mapped up
> high for some reason.
> 
> This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
> systems with more than 4GB of RAM.

There was another patch posted by somebody from Citrix for a fix on 
32-bit x86 dom0 with more than 4GB of RAM (for x86 platforms).

Their fix was in the generic parts of code. Changing most of the 'unsigned'
to 'phys_addr_t' or such. Is his patch better or will this patch replace his?

It was " dma-mapping: dma_alloc_coherent_mask return dma_addr_t"
https://lkml.org/lkml/2013/12/10/593

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/Kconfig          |    1 +
>  drivers/xen/swiotlb-xen.c |   14 ++++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c1f1a7e..24307dc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1885,6 +1885,7 @@ config XEN
>  	depends on !GENERIC_ATOMIC64
>  	select ARM_PSCI
>  	select SWIOTLB_XEN
> +	select ARCH_DMA_ADDR_T_64BIT
>  	help
>  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
>  
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 1eac073..b626c79 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -77,12 +77,22 @@ static u64 start_dma_addr;
>  
>  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
>  {
> -	return phys_to_machine(XPADDR(paddr)).maddr;
> +	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> +	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
> +	dma |= paddr & ~PAGE_MASK;
> +	return dma;
>  }
>  
>  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
>  {
> -	return machine_to_phys(XMADDR(baddr)).paddr;
> +	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> +	phys_addr_t paddr = dma;
> +
> +	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> +
> +	paddr |= baddr & ~PAGE_MASK;
> +
> +	return paddr;
>  }
>  
>  static inline dma_addr_t xen_virt_to_bus(void *address)
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell]

On Tue, 2014-01-21 at 17:03 -0500, Konrad Rzeszutek Wilk wrote:

(nb: I posted a v3 at
http://article.gmane.org/gmane.linux.ports.arm.kernel/295594
)

> On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
> > The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> > causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
> > 
> > This can happen in practice on ARM where foreign pages can be above 4GB even
> > though the local kernel does not have LPAE page tables enabled (which is
> > totally reasonable if the guest does not itself have >4GB of RAM). In this
> > case the kernel still maps the foreign pages at a phys addr below 4G (as it
> > must) but the resulting DMA address (returned by the grant map operation) is
> > much higher.
> > 
> > This is analogous to a hardware device which has its view of RAM mapped up
> > high for some reason.
> > 
> > This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
> > systems with more than 4GB of RAM.
> 
> There was another patch posted by somebody from Citrix for a fix on 
> 32-bit x86 dom0 with more than 4GB of RAM (for x86 platforms).
> 
> Their fix was in the generic parts of code. Changing most of the 'unsigned'
> to 'phys_addr_t' or such. Is his patch better or will this patch replace his?

I believe they are orthogonal, or at least I'm not (yet) hitting the
same issue as Stefano P, the alloc cohoerent code paths are not involved
in the issue I'm seeing because it involves foreign pages whose
MFN/dma_addr is very high, not DMA to devices which are up high.

Ian.

> It was " dma-mapping: dma_alloc_coherent_mask return dma_addr_t"
> https://lkml.org/lkml/2013/12/10/593
> 
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/Kconfig          |    1 +
> >  drivers/xen/swiotlb-xen.c |   14 ++++++++++++--
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index c1f1a7e..24307dc 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1885,6 +1885,7 @@ config XEN
> >  	depends on !GENERIC_ATOMIC64
> >  	select ARM_PSCI
> >  	select SWIOTLB_XEN
> > +	select ARCH_DMA_ADDR_T_64BIT
> >  	help
> >  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> >  
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 1eac073..b626c79 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -77,12 +77,22 @@ static u64 start_dma_addr;
> >  
> >  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
> >  {
> > -	return phys_to_machine(XPADDR(paddr)).maddr;
> > +	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> > +	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
> > +	dma |= paddr & ~PAGE_MASK;
> > +	return dma;
> >  }
> >  
> >  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
> >  {
> > -	return machine_to_phys(XMADDR(baddr)).paddr;
> > +	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> > +	phys_addr_t paddr = dma;
> > +
> > +	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> > +
> > +	paddr |= baddr & ~PAGE_MASK;
> > +
> > +	return paddr;
> >  }
> >  
> >  static inline dma_addr_t xen_virt_to_bus(void *address)
> > -- 
> > 1.7.10.4
> > 

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Stefano Stabellini]

On Wed, 22 Jan 2014, Ian Campbell wrote:
> On Tue, 2014-01-21 at 17:03 -0500, Konrad Rzeszutek Wilk wrote:
> 
> (nb: I posted a v3 at
> http://article.gmane.org/gmane.linux.ports.arm.kernel/295594
> )
> 
> > On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
> > > The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> > > causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
> > > 
> > > This can happen in practice on ARM where foreign pages can be above 4GB even
> > > though the local kernel does not have LPAE page tables enabled (which is
> > > totally reasonable if the guest does not itself have >4GB of RAM). In this
> > > case the kernel still maps the foreign pages at a phys addr below 4G (as it
> > > must) but the resulting DMA address (returned by the grant map operation) is
> > > much higher.
> > > 
> > > This is analogous to a hardware device which has its view of RAM mapped up
> > > high for some reason.
> > > 
> > > This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
> > > systems with more than 4GB of RAM.
> > 
> > There was another patch posted by somebody from Citrix for a fix on 
> > 32-bit x86 dom0 with more than 4GB of RAM (for x86 platforms).
> > 
> > Their fix was in the generic parts of code. Changing most of the 'unsigned'
> > to 'phys_addr_t' or such. Is his patch better or will this patch replace his?
> 
> I believe they are orthogonal, or at least I'm not (yet) hitting the
> same issue as Stefano P, the alloc cohoerent code paths are not involved
> in the issue I'm seeing because it involves foreign pages whose
> MFN/dma_addr is very high, not DMA to devices which are up high.

Yes, the two issues are orthogonal.
It is worth noting that the problem reported by StefanoP is not fatal:
it should just cause more bouncing on the swiotlb buffer than it is
strictly necessary (dma_mask gets truncated).

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Stefano Panella]

On 01/22/2014 12:11 PM, Stefano Stabellini wrote:
> On Wed, 22 Jan 2014, Ian Campbell wrote:
>> On Tue, 2014-01-21 at 17:03 -0500, Konrad Rzeszutek Wilk wrote:
>>
>> (nb: I posted a v3 at
>> http://article.gmane.org/gmane.linux.ports.arm.kernel/295594
>> )
>>
>>> On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
>>>> The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
>>>> causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.
>>>>
>>>> This can happen in practice on ARM where foreign pages can be above 4GB even
>>>> though the local kernel does not have LPAE page tables enabled (which is
>>>> totally reasonable if the guest does not itself have >4GB of RAM). In this
>>>> case the kernel still maps the foreign pages at a phys addr below 4G (as it
>>>> must) but the resulting DMA address (returned by the grant map operation) is
>>>> much higher.
>>>>
>>>> This is analogous to a hardware device which has its view of RAM mapped up
>>>> high for some reason.
>>>>
>>>> This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
>>>> systems with more than 4GB of RAM.
>>> There was another patch posted by somebody from Citrix for a fix on
>>> 32-bit x86 dom0 with more than 4GB of RAM (for x86 platforms).
>>>
>>> Their fix was in the generic parts of code. Changing most of the 'unsigned'
>>> to 'phys_addr_t' or such. Is his patch better or will this patch replace his?
>> I believe they are orthogonal, or at least I'm not (yet) hitting the
>> same issue as Stefano P, the alloc cohoerent code paths are not involved
>> in the issue I'm seeing because it involves foreign pages whose
>> MFN/dma_addr is very high, not DMA to devices which are up high.
> Yes, the two issues are orthogonal.
> It is worth noting that the problem reported by StefanoP is not fatal:
> it should just cause more bouncing on the swiotlb buffer than it is
> strictly necessary (dma_mask gets truncated).
I agree it is not fatal, but would it be worth not truncating the 
dma_mask for devices capable of using this?
I was under the impression that the memory returned (<4GB) if we 
truncate that is limited and should be used for PCI devices not capable 
of 64bit addressing.

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Stefano Stabellini]

On Wed, 22 Jan 2014, Stefano Panella wrote:
> On 01/22/2014 12:11 PM, Stefano Stabellini wrote:
> > On Wed, 22 Jan 2014, Ian Campbell wrote:
> > > On Tue, 2014-01-21 at 17:03 -0500, Konrad Rzeszutek Wilk wrote:
> > > 
> > > (nb: I posted a v3 at
> > > http://article.gmane.org/gmane.linux.ports.arm.kernel/295594
> > > )
> > > 
> > > > On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
> > > > > The use of phys_to_machine and machine_to_phys in the phys<=>bus
> > > > > conversions
> > > > > causes us to lose the top bits of the DMA address if the size of a DMA
> > > > > address is not the same as the size of the phyiscal address.
> > > > > 
> > > > > This can happen in practice on ARM where foreign pages can be above
> > > > > 4GB even
> > > > > though the local kernel does not have LPAE page tables enabled (which
> > > > > is
> > > > > totally reasonable if the guest does not itself have >4GB of RAM). In
> > > > > this
> > > > > case the kernel still maps the foreign pages at a phys addr below 4G
> > > > > (as it
> > > > > must) but the resulting DMA address (returned by the grant map
> > > > > operation) is
> > > > > much higher.
> > > > > 
> > > > > This is analogous to a hardware device which has its view of RAM
> > > > > mapped up
> > > > > high for some reason.
> > > > > 
> > > > > This patch makes I/O to foreign pages (specifically blkif) work on
> > > > > 32-bit ARM
> > > > > systems with more than 4GB of RAM.
> > > > There was another patch posted by somebody from Citrix for a fix on
> > > > 32-bit x86 dom0 with more than 4GB of RAM (for x86 platforms).
> > > > 
> > > > Their fix was in the generic parts of code. Changing most of the
> > > > 'unsigned'
> > > > to 'phys_addr_t' or such. Is his patch better or will this patch replace
> > > > his?
> > > I believe they are orthogonal, or at least I'm not (yet) hitting the
> > > same issue as Stefano P, the alloc cohoerent code paths are not involved
> > > in the issue I'm seeing because it involves foreign pages whose
> > > MFN/dma_addr is very high, not DMA to devices which are up high.
> > Yes, the two issues are orthogonal.
> > It is worth noting that the problem reported by StefanoP is not fatal:
> > it should just cause more bouncing on the swiotlb buffer than it is
> > strictly necessary (dma_mask gets truncated).
> I agree it is not fatal, but would it be worth not truncating the dma_mask for
> devices capable of using this?
> I was under the impression that the memory returned (<4GB) if we truncate that
> is limited and should be used for PCI devices not capable of 64bit addressing.

Yeah, Linux should certainly not truncate the dma_mask.

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Konrad Rzeszutek Wilk]

On Fri, Jan 17, 2014 at 05:24:53PM +0000, Ian Campbell wrote:
> The use of phys_to_machine and machine_to_phys in the phys<=>bus conversions
> causes us to lose the top bits of the DMA address if the size of a DMA address is not the same as the size of the phyiscal address.

.. physical
> 
> This can happen in practice on ARM where foreign pages can be above 4GB even
> though the local kernel does not have LPAE page tables enabled (which is
> totally reasonable if the guest does not itself have >4GB of RAM). In this
> case the kernel still maps the foreign pages at a phys addr below 4G (as it
> must) but the resulting DMA address (returned by the grant map operation) is
> much higher.
> 
> This is analogous to a hardware device which has its view of RAM mapped up
> high for some reason.
> 
> This patch makes I/O to foreign pages (specifically blkif) work on 32-bit ARM
> systems with more than 4GB of RAM.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/Kconfig          |    1 +
>  drivers/xen/swiotlb-xen.c |   14 ++++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c1f1a7e..24307dc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1885,6 +1885,7 @@ config XEN
>  	depends on !GENERIC_ATOMIC64
>  	select ARM_PSCI
>  	select SWIOTLB_XEN
> +	select ARCH_DMA_ADDR_T_64BIT
>  	help
>  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
>  
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 1eac073..b626c79 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -77,12 +77,22 @@ static u64 start_dma_addr;
>  
>  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
>  {
> -	return phys_to_machine(XPADDR(paddr)).maddr;

Why not change 'phys_addr_t' to be unsigned long? Wouldn't
that solve the problem as well?

Or make 'xmaddr_t' and 'xpaddr_t' use 'unsigned long' instead
of phys_addr_t?


> +	unsigned long mfn = pfn_to_mfn(PFN_DOWN(paddr));
> +	dma_addr_t dma = (dma_addr_t)mfn << PAGE_SHIFT;
> +	dma |= paddr & ~PAGE_MASK;
> +	return dma;
>  }
>  
>  static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
>  {
> -	return machine_to_phys(XMADDR(baddr)).paddr;
> +	dma_addr_t dma = PFN_PHYS(mfn_to_pfn(PFN_DOWN(baddr)));
> +	phys_addr_t paddr = dma;
> +
> +	BUG_ON(paddr != dma); /* truncation has occurred, should never happen */
> +
> +	paddr |= baddr & ~PAGE_MASK;
> +
> +	return paddr;
>  }
>  
>  static inline dma_addr_t xen_virt_to_bus(void *address)
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell]

On Wed, 2014-01-22 at 16:01 -0500, Konrad Rzeszutek Wilk wrote:
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 1eac073..b626c79 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -77,12 +77,22 @@ static u64 start_dma_addr;
> >  
> >  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
> >  {
> > -	return phys_to_machine(XPADDR(paddr)).maddr;
> 
> Why not change 'phys_addr_t' to be unsigned long? Wouldn't
> that solve the problem as well?

It would, but it is fundamentally the wrong thing to do.

If the kernel is configured without LPAE (ARM's PAE extensions) then it
is configured for a 32-bit physical address space, throughout its page
table handling and else where. Pretending that physical addresses are
64-bits would have all sorts of knock on effects both in terms of type
mismatches and the space used by data structures doubling etc.

Enabling LPAE would also solve this issue but we don't want to force
that constraint onto Xen guests or dom0. Not least because of the knock
on affect on distro installers etc.

There is nothing fundamentally wrong with 32-bit phys addr with 64-bit
dma addr and it is the correct solution to this configuration.

> 
> Or make 'xmaddr_t' and 'xpaddr_t' use 'unsigned long' instead
> of phys_addr_t?

phys_addr_t is unsigned long already, so that won't help. And you don't
want to expand those for the same reasons you don't want to expand
phys_addr_t.

Ian.

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Konrad Rzeszutek Wilk]

Ian Campbell <Ian.Campbell@citrix.com> wrote:
>On Wed, 2014-01-22 at 16:01 -0500, Konrad Rzeszutek Wilk wrote:
>> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> > index 1eac073..b626c79 100644
>> > --- a/drivers/xen/swiotlb-xen.c
>> > +++ b/drivers/xen/swiotlb-xen.c
>> > @@ -77,12 +77,22 @@ static u64 start_dma_addr;
>> >  
>> >  static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
>> >  {
>> > -	return phys_to_machine(XPADDR(paddr)).maddr;
>> 
>> Why not change 'phys_addr_t' to be unsigned long? Wouldn't
>> that solve the problem as well?
>
>It would, but it is fundamentally the wrong thing to do.
>
>If the kernel is configured without LPAE (ARM's PAE extensions) then it
>is configured for a 32-bit physical address space, throughout its page
>table handling and else where. Pretending that physical addresses are
>64-bits would have all sorts of knock on effects both in terms of type
>mismatches and the space used by data structures doubling etc
>
>Enabling LPAE would also solve this issue but we don't want to force
>that constraint onto Xen guests or dom0. Not least because of the knock
>on affect on distro installers etc.
>
>There is nothing fundamentally wrong with 32-bit phys addr with 64-bit
>dma addr and it is the correct solution to this configuration.

Right. 

And I presume dma_addr_t is 64bit regardless of PAE and non-PAE?
 (Sorry I am on my phone and its hard to do SSH and cscope).

Based on your explanation I believe the patch is fine though I have to work out carefully the casting it does in my mind.

>
>> 
>> Or make 'xmaddr_t' and 'xpaddr_t' use 'unsigned long' instead
>> of phys_addr_t?
>
>phys_addr_t is unsigned long already, so that won't help. And you don't
>want to expand those for the same reasons you don't want to expand
>phys_addr_t.
>
>Ian.

Re: [PATCH] xen: swiotlb: handle sizeof(dma_addr_t) != sizeof(phys_addr_t)

[Ian Campbell]

On Thu, 2014-01-23 at 12:42 -0500, Konrad Rzeszutek Wilk wrote:
> And I presume dma_addr_t is 64bit regardless of PAE and non-PAE?

Correct. Or rather, correct after this patch which adds "select
ARCH_DMA_ADDR_T_64BIT" to the Kconfig.

>  (Sorry I am on my phone and its hard to do SSH and cscope).

You're doing very well to quote everything properly, so no worries!

> Based on your explanation I believe the patch is fine though I have to
> work out carefully the casting it does in my mind.

OK, thanks.

Ian.