| Message ID | 1567175255-1798-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] arm: xen: mm: use __GPF_DMA32 for arm64 | expand |
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,
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?
+ 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.
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.
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
> 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.
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.
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; +}