RE: [XEN RFC PATCH 04/40] xen/arm: return default DMA bit width when platform is not set

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月19日 21:28
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 04/40] xen/arm: return default DMA bit width
> when platform is not set
> 
> Hi,
> 
> On 11/08/2021 11:23, Wei Chen wrote:
> > From: Hongda Deng <Hongda.Deng@xxxxxxx>
> >
> > In current code, arch_get_dma_bitsize will return 32 when platorm
> > or platform->dma_bitsize is not set. It's not resonable, for Arm,
> 
> s/resonable/reasonable/
> 

Ok

> > we don't require to reserve DMA memory. So we set dma_bitsize always
> > be 0. In NO-NUMA system, arch_get_dma_bitsize will not be invoked,
> > so dma_bitsize will not be overrided by this function.
> 
> arch_get_dma_bitsize() is also used to allocate dom0 memory. We need to
> be able to allocate some DMA-able memory that can be used by every devices.
> 
> > But in NUMA
> > system, once the online nodes are greater than 1, this function will
> > be invoked. The dma_bitsize will be limited to 32. That means, only
> > first 4GB memory can be used for DMA. But that's against our hardware
> > design. We don't have that kind of restriction on hardware.
> 
> What do you mean by "hardware design"? Are you referring to the server
> you boot Xen on?
> 

Yes. I will change it to some neutral words. something like:
"But that could not reflect some hardware's real DMA ability. They may not
have kind of restriction on hardware." ?


> Anyway, there are plenty of platform out that have devices which can't
> DMA into memory above 32-bit. On RPI, this is even lower (30-bit).
> 
> So I would be cautious to change the default limit.
> 

How about return 0 when platform doesn't specify the limit?
In my opinion, arbitrary to give 32 on AArch64 doesn't make sense.
But as you mentioned, if Xen is running on a platform with DMA limitation,
but we have not listed this platform in Xen supported list, Xen cannot
get DMA limit from platform->dma_bitsize. In this case, return 0 will
also cause some issue.

> At the moment, the only place on Arm where we need DMA-able memory is
> for dom0. This is allocated at boot and can't change afterwards (for now).
> 

For Dom0, we squash the patch#5 into this patch?

> So I would explore to remove the NUMA check for drop the DMA zone. FAOD,
> both suggestion are for Arm only. For x86, they need to be kept.
> 

Without introducing new flag, such as lowmem_for_dma, it's a little
hard to skip the numa node check. Unless we crudely add #ifdef ARCH to 
common code, which is not what we want to see ...
      if ( !dma_bitsize && (num_online_nodes() > 1) )
          dma_bitsize = arch_get_dma_bitsize();

> > Only
> > platform setting can override dma_bitsize. So in this patch, we
> > return default dma_bitsize, when platform and platorm->dma_bitsize
> > are not set.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Hongda Deng <Hongda.Deng@xxxxxxx>
> > ---
> >   xen/arch/arm/platform.c | 4 +++-
> >   xen/common/page_alloc.c | 2 +-
> >   2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> > index 4db5bbb4c5..0a27fef9a4 100644
> > --- a/xen/arch/arm/platform.c
> > +++ b/xen/arch/arm/platform.c
> > @@ -27,6 +27,7 @@ extern const struct platform_desc _splatform[],
> _eplatform[];
> >   /* Pointer to the current platform description */
> >   static const struct platform_desc *platform;
> >
> > +extern unsigned int dma_bitsize;
> >
> >   static bool __init platform_is_compatible(const struct platform_desc
> *plat)
> >   {
> > @@ -157,7 +158,8 @@ bool platform_device_is_blacklisted(const struct
> dt_device_node *node)
> >
> >   unsigned int arch_get_dma_bitsize(void)
> >   {
> > -    return ( platform && platform->dma_bitsize ) ? platform-
> >dma_bitsize : 32;
> > +    return ( platform && platform->dma_bitsize ) ? platform-
> >dma_bitsize
> > +                                                 : dma_bitsize;
> >   }
> >
> >   /*
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 958ba0cd92..0f0cae5a4e 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -227,7 +227,7 @@ static bool __read_mostly scrub_debug;
> >    * Bit width of the DMA heap -- used to override NUMA-node-first.
> >    * allocation strategy, which can otherwise exhaust low memory.
> >    */
> > -static unsigned int dma_bitsize;
> > +unsigned int dma_bitsize;
> >   integer_param("dma_bits", dma_bitsize);
> >
> >   /* Offlined page list, protected by heap_lock. */
> >
> 
> Cheers,
> 
> --
> Julien Grall