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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2021年8月11日 18:54
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [XEN RFC PATCH 04/40] xen/arm: return default DMA bit width
> when platform is not set
> 
> On 11.08.2021 12:23, Wei Chen wrote:
> > --- 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;
> 
> This is a no-go: Declarations need to live in a header which the producer
> and all consumers include. Else ...

Ok, I will place it to a header.

> 
> > --- 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;
> 
> ... a change here (of e.g. the type) will go unnoticed by the compiler,
> and the consumer of the variable may no longer work correctly.
> 

Sorry, I am not very clear about this comment.

> Also I'm afraid the description does not make clear why this variable
> is what you want to use. Connected to this is the question why you need
> to consume it on Arm in the first place, when x86 never had the need.
> 

Different Arm platforms may have different DMA bitsize. So in my previous
thought, If Arm platform doesn't provide any DMA bitsize info, I will
return the system DMA bitsize (dma_bitsize) in arch_get_dma_bitsize.
But your comment made me think again. My current change may have fallen
into a logical loophole. dma_bitsize as a high level variable, it's
value depends on xen boot command line or arch_get_dma_bitsize.
I can't use it for arch_get_dma_bitsize's input.

So I think, in next version, I will discard the changes of dma_bitsize,
just keep the changes in arch_get_dma_bitsize to return 0, when platform
hasn't specify the DMA bitsize. Just like:

unsigned int arch_get_dma_bitsize(void)
 {
-    return ( platform && platform->dma_bitsize ) ? platform->dma_bitsize : 32;
+    return ( platform && platform->dma_bitsize ) ? platform->dma_bitsize
+                                                 : 0;
 }

Thanks,
Wei Chen

> Jan