I realize this series has been pending for a long time, but I don't
recall any indication that it would have been dropped. Hence as a
first try, a few comments on this relatively simple change. I'm
sorry it to have taken so long to get to it.
On 23.09.2021 14:02, Wei Chen wrote:
> In current code, when Xen is running in a multiple nodes NUMA
> system, it will set dma_bitsize in end_boot_allocator to reserve
> some low address memory for DMA.
>
> There are some x86 implications in current implementation. Becuase
> on x86, memory starts from 0. On a multiple nodes NUMA system, if
> a single node contains the majority or all of the DMA memory. x86
> prefer to give out memory from non-local allocations rather than
> exhausting the DMA memory ranges. Hence x86 use dma_bitsize to set
> aside some largely arbitrary amount memory for DMA memory ranges.
> The allocations from these memory ranges would happen only after
> exhausting all other nodes' memory.
>
> But the implications are not shared across all architectures. For
> example, Arm doesn't have these implications. So in this patch, we
> introduce an arch_have_default_dmazone helper for arch to determine
> that it need to set dma_bitsize for reserve DMA allocations or not.
How would Arm guarantee availability of memory below a certain
boundary for limited-capability devices? Or is there no need
because there's an assumption that I/O for such devices would
always pass through an IOMMU, lifting address size restrictions?
(I guess in a !PV build on x86 we could also get rid of such a
reservation.)
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -371,6 +371,11 @@ unsigned int __init arch_get_dma_bitsize(void)
> + PAGE_SHIFT, 32);
> }
>
> +unsigned int arch_have_default_dmazone(void)
> +{
> + return ( num_online_nodes() > 1 ) ? 1 : 0;
> +}
According to the expression and ...
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1889,7 +1889,7 @@ void __init end_boot_allocator(void)
> }
> nr_bootmem_regions = 0;
>
> - if ( !dma_bitsize && (num_online_nodes() > 1) )
> + if ( !dma_bitsize && arch_have_default_dmazone() )
> dma_bitsize = arch_get_dma_bitsize();
... the use site, you mean the function to return boolean. Please
indicate so by making it have a return type of "bool". Independent
of that you don't need a conditional expression above, nor
(malformed) use of parentheses. I further wonder whether ...
> --- a/xen/include/asm-arm/numa.h
> +++ b/xen/include/asm-arm/numa.h
> @@ -25,6 +25,11 @@ extern mfn_t first_valid_mfn;
> #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> #define __node_distance(a, b) (20)
>
> +static inline unsigned int arch_have_default_dmazone(void)
> +{
> + return 0;
> +}
... like this one, x86'es couldn't be inline as well. If indeed
it can't be, making it a macro may still be better (and avoid a
further comment regarding the lack of __init).
Jan
