[PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

arm64 shares some code under arch/arm/xen, including mm.c.
However ZONE_DMA is removed by commit
ad67f5a6545("arm64: replace ZONE_DMA with ZONE_DMA32").
So introduce xen_set_gfp_dma for arm32/arm64 and using __GFP_DMA
for the former and __GFP_DMA32 for the latter.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Follow suggestion from Stefano,
 introduce static inline void xen_set_gfp_dma(gfp_t *flags) for arm32/arm64, and
 for arm64 using __GFP_DMA for the former and __GFP_DMA32 for the latter.

 arch/arm/include/asm/xen/page.h   | 5 +++++
 arch/arm/xen/mm.c                 | 2 +-
 arch/arm64/include/asm/xen/page.h | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index 31bbc803cecb..d08309c45e6c 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -1 +1,6 @@
 #include <xen/arm/page.h>
+
+static inline void xen_set_gfp_dma(gfp_t *flags)
+{
+	*flags |= __GFP_DMA;
+}
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d33b77e9add3..828f49dc95f9 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -28,7 +28,7 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
 
 	for_each_memblock(memory, reg) {
 		if (reg->base < (phys_addr_t)0xffffffff) {
-			flags |= __GFP_DMA;
+			xen_set_gfp_dma(&flags);
 			break;
 		}
 	}
diff --git a/arch/arm64/include/asm/xen/page.h b/arch/arm64/include/asm/xen/page.h
index 31bbc803cecb..5eeabf2c6494 100644
--- a/arch/arm64/include/asm/xen/page.h
+++ b/arch/arm64/include/asm/xen/page.h
@@ -1 +1,6 @@
 #include <xen/arm/page.h>
+
+static inline void xen_set_gfp_dma(gfp_t *flags)
+{
+	*flags |= __GFP_DMA32;
+}
-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Julien Grall]

Hi Peng,

On 30/08/2019 04:28, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> arm64 shares some code under arch/arm/xen, including mm.c.
> However ZONE_DMA is removed by commit
> ad67f5a6545("arm64: replace ZONE_DMA with ZONE_DMA32").
> So introduce xen_set_gfp_dma for arm32/arm64 and using __GFP_DMA
> for the former and __GFP_DMA32 for the latter.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>   Follow suggestion from Stefano,
>   introduce static inline void xen_set_gfp_dma(gfp_t *flags) for arm32/arm64, and
>   for arm64 using __GFP_DMA for the former and __GFP_DMA32 for the latter.
> 
>   arch/arm/include/asm/xen/page.h   | 5 +++++
>   arch/arm/xen/mm.c                 | 2 +-
>   arch/arm64/include/asm/xen/page.h | 5 +++++
>   3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
> index 31bbc803cecb..d08309c45e6c 100644
> --- a/arch/arm/include/asm/xen/page.h
> +++ b/arch/arm/include/asm/xen/page.h
> @@ -1 +1,6 @@
>   #include <xen/arm/page.h>
> +
> +static inline void xen_set_gfp_dma(gfp_t *flags)
> +{
> +	*flags |= __GFP_DMA;
> +}
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index d33b77e9add3..828f49dc95f9 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -28,7 +28,7 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
>   
>   	for_each_memblock(memory, reg) {
>   		if (reg->base < (phys_addr_t)0xffffffff) {
> -			flags |= __GFP_DMA;
> +			xen_set_gfp_dma(&flags);

The name of the helper is quite misleading, this is specific to swiotlb 
yet it gives the impression it can be used everywhere. The helper will 
actually set the flags in order to allocate memory below 4GB.

Also, I saw an e-mail suggesting that __GFP_DMA may be used on Arm64. So 
a user may think using xen_set_gfp_dma() will set _GFP_DMA and not 
_GFP_DMA32.

I know duplication is not great but it feels that duplicating the full 
function (or only the allocation part) would be the best. This would 
require to introduce a new file mm{32,64}.c in the respective arch 
directory.

Cheers,

-- 
Julien Grall
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Christoph Hellwig]

Can we take a step back and figure out what we want to do here?

AFAICS this function allocates memory for the swiotlb-xen buffer,
and that means it must be <= 32-bit addressable to satisfy the DMA API
guarantees.  That means we generally want to use GFP_DMA32 everywhere
that exists, but on systems with odd zones we might want to dip into
GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
thing on x86 at the moment.  So shouldn't we just have one common
routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32
set, then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set
and then try that, else default to GFP_KERNEL?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Stefano Stabellini]

+ Juergen, Boris

On Fri, 30 Aug 2019, Christoph Hellwig wrote:
> Can we take a step back and figure out what we want to do here?
> 
> AFAICS this function allocates memory for the swiotlb-xen buffer,
> and that means it must be <= 32-bit addressable to satisfy the DMA API
> guarantees.  That means we generally want to use GFP_DMA32 everywhere
> that exists, but on systems with odd zones we might want to dip into
> GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
> thing on x86 at the moment.  So shouldn't we just have one common
> routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32
> set, then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set
> and then try that, else default to GFP_KERNEL?

Yes, for ARM/ARM64 it makes a lot of sense given that dom0 is 1:1 mapped
(pseudo-physical == physical).  I'll let Juergen and Boris comment on
the x86 side of things, but on x86 PV Dom0 is not 1:1 mapped so
GFP_DMA32 is probably not meaningful.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Christoph Hellwig]

On Fri, Aug 30, 2019 at 07:40:42PM -0700, Stefano Stabellini wrote:
> + Juergen, Boris
> 
> On Fri, 30 Aug 2019, Christoph Hellwig wrote:
> > Can we take a step back and figure out what we want to do here?
> > 
> > AFAICS this function allocates memory for the swiotlb-xen buffer,
> > and that means it must be <= 32-bit addressable to satisfy the DMA API
> > guarantees.  That means we generally want to use GFP_DMA32 everywhere
> > that exists, but on systems with odd zones we might want to dip into
> > GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
> > thing on x86 at the moment.  So shouldn't we just have one common
> > routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32
> > set, then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set
> > and then try that, else default to GFP_KERNEL?
> 
> Yes, for ARM/ARM64 it makes a lot of sense given that dom0 is 1:1 mapped
> (pseudo-physical == physical).  I'll let Juergen and Boris comment on
> the x86 side of things, but on x86 PV Dom0 is not 1:1 mapped so
> GFP_DMA32 is probably not meaningful.

But is it actually harmful?  If the GFP_DMA32 doesn't hurt we could
just use it there.  Or if that seems to ugly we can make the dma
flags dependents on a XEN_1TO1_MAPPED config option set by arm/arm64.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Juergen Gross]

On 02.09.19 17:57, Christoph Hellwig wrote:
> On Fri, Aug 30, 2019 at 07:40:42PM -0700, Stefano Stabellini wrote:
>> + Juergen, Boris
>>
>> On Fri, 30 Aug 2019, Christoph Hellwig wrote:
>>> Can we take a step back and figure out what we want to do here?
>>>
>>> AFAICS this function allocates memory for the swiotlb-xen buffer,
>>> and that means it must be <= 32-bit addressable to satisfy the DMA API
>>> guarantees.  That means we generally want to use GFP_DMA32 everywhere
>>> that exists, but on systems with odd zones we might want to dip into
>>> GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
>>> thing on x86 at the moment.  So shouldn't we just have one common
>>> routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32
>>> set, then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set
>>> and then try that, else default to GFP_KERNEL?
>>
>> Yes, for ARM/ARM64 it makes a lot of sense given that dom0 is 1:1 mapped
>> (pseudo-physical == physical).  I'll let Juergen and Boris comment on
>> the x86 side of things, but on x86 PV Dom0 is not 1:1 mapped so
>> GFP_DMA32 is probably not meaningful.
> 
> But is it actually harmful?  If the GFP_DMA32 doesn't hurt we could
> just use it there.  Or if that seems to ugly we can make the dma
> flags dependents on a XEN_1TO1_MAPPED config option set by arm/arm64.
> 

I'd rather have it only if needed. Especially on X86 PV dom0 I'd like to
avoid GFP_DMA32 as memory below 4GB (guest physical) might be rather
scarce in some configurations.

I think X86 PVH dom0 should need GFP_DMA32, too, as the limit is related
to the address as communicated to the device (before being translated by
the IOMMU), right? This would mean on a X86 kernel configured to support
PV and PVH the test for setting GFP_DMA32 can't depend on a config
option alone, it needs to be dynamic.

BTW, for PV guests the DMA address width is handled via
xen_create_contiguous_region().


Juergen

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Peng Fan]

> Subject: Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64
> 
> + Juergen, Boris
> 
> On Fri, 30 Aug 2019, Christoph Hellwig wrote:
> > Can we take a step back and figure out what we want to do here?
> >
> > AFAICS this function allocates memory for the swiotlb-xen buffer, and
> > that means it must be <= 32-bit addressable to satisfy the DMA API
> > guarantees.  That means we generally want to use GFP_DMA32
> everywhere
> > that exists, but on systems with odd zones we might want to dip into
> > GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
> > thing on x86 at the moment.  So shouldn't we just have one common
> > routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32 set,
> > then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set and
> > then try that, else default to GFP_KERNEL?
> 
> Yes, for ARM/ARM64 it makes a lot of sense given that dom0 is 1:1 mapped
> (pseudo-physical == physical).  I'll let Juergen and Boris comment on the x86
> side of things, but on x86 PV Dom0 is not 1:1 mapped so
> GFP_DMA32 is probably not meaningful.

If we only take ARM/ARM64, so could the following patch be ok?

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d33b77e9add3..e5a6a73b2e06 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -28,7 +28,11 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)

        for_each_memblock(memory, reg) {
                if (reg->base < (phys_addr_t)0xffffffff) {
+#ifdef CONFIG_ZONE_DMA32
+                       flags |= __GFP_DMA32;
+#else
                        flags |= __GFP_DMA;
+#endif
                        break;
                }
        }

Thanks,
Peng.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64

[Stefano Stabellini]

On Wed, 11 Sep 2019, Peng Fan wrote:
> > Subject: Re: [PATCH V2] arm: xen: mm: use __GPF_DMA32 for arm64
> > 
> > + Juergen, Boris
> > 
> > On Fri, 30 Aug 2019, Christoph Hellwig wrote:
> > > Can we take a step back and figure out what we want to do here?
> > >
> > > AFAICS this function allocates memory for the swiotlb-xen buffer, and
> > > that means it must be <= 32-bit addressable to satisfy the DMA API
> > > guarantees.  That means we generally want to use GFP_DMA32
> > everywhere
> > > that exists, but on systems with odd zones we might want to dip into
> > > GFP_DMA.  This also means swiotlb-xen doesn't actually do the right
> > > thing on x86 at the moment.  So shouldn't we just have one common
> > > routine in swiotlb-xen.c that checks if we have CONFIG_ZONE_DMA32 set,
> > > then try GFP_DMA32, and if not check if CONFIG_ZONE_DMA is set and
> > > then try that, else default to GFP_KERNEL?
> > 
> > Yes, for ARM/ARM64 it makes a lot of sense given that dom0 is 1:1 mapped
> > (pseudo-physical == physical).  I'll let Juergen and Boris comment on the x86
> > side of things, but on x86 PV Dom0 is not 1:1 mapped so
> > GFP_DMA32 is probably not meaningful.
> 
> If we only take ARM/ARM64, so could the following patch be ok?
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index d33b77e9add3..e5a6a73b2e06 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -28,7 +28,11 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> 
>         for_each_memblock(memory, reg) {
>                 if (reg->base < (phys_addr_t)0xffffffff) {
> +#ifdef CONFIG_ZONE_DMA32
> +                       flags |= __GFP_DMA32;
> +#else
>                         flags |= __GFP_DMA;
> +#endif
>                         break;
>                 }
>         }

I am OK with something like this, but at that point we can use
IS_ENABLED(CONFIG_ZONE_DMA32) for the check.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel