Re: [PATCH 04/37] xen: introduce an arch helper for default dma zone status

[Jan Beulich <jbeulich@suse.com>]

On 18.01.2022 08:51, Wei Chen wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 2022年1月18日 0:11
>> 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.)
> 
> On Arm, we still can have some devices with limited DMA capability.
> And we also don't force all such devices to use IOMMU. This devices
> will affect the dma_bitsize. Like RPi platform, it sets its dma_bitsize
> to 30. But in multiple NUMA nodes system, Arm doesn't have a default
> DMA zone. Multiple nodes is not a constraint on dma_bitsize. And some
> previous discussions are placed here [1].

I'm afraid that doesn't give me more clues. For example, in the mail
being replied to there I find "That means, only first 4GB memory can
be used for DMA." Yet that's not an implication from setting
dma_bitsize. DMA is fine to occur to any address. The special address
range is being held back in case in particular Dom0 is in need of such
a range to perform I/O to _some_ devices.

>>> --- 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 ...
>>
> 
> I will fix them in next version. But I am not very clear about
> this comment "of that you don't need a conditional expression above",
> The "above" indicates this line:
> "return ( num_online_nodes() > 1 ) ? 1 : 0;"?

Yes. Even without the use of bool such an expression is a more
complicated form of

    return num_online_nodes() > 1;

where we'd prefer to use the simpler variant for being easier to
read / follow.

Jan

> [1] https://lists.xenproject.org/archives/html/xen-devel/2021-08/msg00772.html
> 
>