Re: [PATCH v4 09/10] xen/arm: fix duplicate /reserved-memory node in Dom0

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Penny,

On 11/09/2023 12:39, Penny Zheng wrote:
> 
> 
> Hi Michal
> 
> On 2023/9/11 17:40, Michal Orzel wrote:
>> Hi Penny,
>>
>> On 11/09/2023 06:04, Penny Zheng wrote:
>>>
>>>
>>> In case there is a /reserved-memory node already present in the host dtb,
>>> current Xen codes would create yet another /reserved-memory node specially
>> s/codes/code/
>>
>>> for the static shm in Dom0 Device Tree.
>>>
>>> Xen will use write_properties() to copy the reserved memory nodes from host 
>>> dtb
>>> to Dom0 FDT, so we want to insert the shm node along with the copying.
>>> And avoiding duplication, we add a checking before make_resv_memory_node().
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>>>
>>> ---
>>> v3 -> v4:
>>> new commit
>>> ---
>>>   xen/arch/arm/domain_build.c       | 31 ++++++++++++++++++++++++++++---
>>>   xen/arch/arm/include/asm/kernel.h |  2 ++
>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 02d903be78..dad234e4b5 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct 
>>> kernel_info *kinfo,
>>>       return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
>>>   }
>>>
>>> +static int __init make_shm_memory_node(const struct domain *d,
>>> +                                       void *fdt,
>>> +                                       int addrcells, int sizecells,
>>> +                                       const struct kernel_info *kinfo);
>>>   static int __init write_properties(struct domain *d, struct kernel_info 
>>> *kinfo,
>>>                                      const struct dt_device_node *node)
>>>   {
>>> @@ -1549,6 +1553,23 @@ static int __init write_properties(struct domain *d, 
>>> struct kernel_info *kinfo,
>>>           }
>>>       }
>>>
>>> +    if ( dt_node_path_is_equal(node, "/reserved-memory") )
>>> +    {
>>> +        kinfo->resv_mem = true;
>> kinfo structure is used to store per-domain parameters and presence of 
>> reserved memory in host dtb
>> does not fit into this. Moreover, there is no need to introduce yet another 
>> flag for that given
>> that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks 
>> to check if there are
>> reserved regions.
>>
>>> +
>>> +        /* shared memory provided. */
>>> +        if ( kinfo->shminfo.nr_banks != 0 )
>>> +        {
>>> +            uint32_t addrcells = dt_n_addr_cells(node),
>>> +                     sizecells = dt_n_size_cells(node);
>>> +
>>> +            res = make_shm_memory_node(d, kinfo->fdt,
>>> +                                       addrcells, sizecells, kinfo);
>> I haven't yet looked at previous patches but does it make sense to request 
>> passing both kinfo->fdt and kinfo?
>> IMO, the latter would be sufficient. This would apply to other functions you 
>> modified.
>>
> 
> yes, the latter would be sufficient. I'll fix it in next version.
> 
> 
>>> +            if ( res )
>>> +                return res;
>>> +        }
>>> +    }
>>> +
>>>       return 0;
>>>   }
>>>
>>> @@ -2856,9 +2877,13 @@ static int __init handle_node(struct domain *d, 
>>> struct kernel_info *kinfo,
>>>                   return res;
>>>           }
>>>
>>> -        res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, 
>>> kinfo);
>>> -        if ( res )
>>> -            return res;
>>> +        /* Avoid duplicate /reserved-memory nodes in Device Tree */
>>> +        if ( !kinfo->resv_mem )
>> No need for a new flag as mentioned above. Just before this line of code 
>> there is a check:
>> if ( bootinfo.reserved_mem.nr_banks > 0 )
>> {
>>      res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
>>                              &bootinfo.reserved_mem);
>>      if ( res )
>>          return res;
>> }
>> so you can just append the following:
>> else
>> {
>>      res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
>>      if ( res )
>>          return res;
>> }
>> to achieve the same without a need for a new flag.
> 
> 
> Right now, bootinfo.reserved_mem is not only containing reserved regions
> described in host FDT /reserved-memory, but also the reserved ones for
> domain only, like in xen,static-mem = <xxx>.
> So simply with non-zero bootinfo.reserved_mem.nr_banks, we couldn't tell
> whether we had a /reserved-memory node in host FDT.
> 
> I agree that kinfo is not a good place to store whether host FDT has a
> /reserved-memory node. Maybe bootinfo.has_resv_memory_node?
Yes, I see. So we have 2 options:
1) Introduce flag under bootinfo (just like static_heap)
2) Inside make_resv_memory_node(), do a check e.g.:
for ( i = 0; i < bootinfo.reserved_mem.nr_banks && 
(bootinfo.reserved_mem.bank[i].type == MEMBANK_DEFAULT); i++ );
to see if there is a /reserved-memory node (i > 0).

~Michal