Re: [PATCH] xen: Add support to passthrough reserved memory carveouts to DomU

[Beleswar Prasad Padhi <b-padhi@xxxxxx>]

Hello,

On 15/09/26 15:28, Orzel, Michal wrote:
> Hi,
>
> On 15-Sep-26 10:28, Beleswar Padhi wrote:
>> Currently Xen only looks for /passthrough and /aliases nodes. Add
>> support to parse the /reserved-memory node and pass it through as
>> requested to DomUs. This is useful to enable many usecases in DomUs
>> like Remoteproc firmware carveouts, CMA carveouts etc.
>>
>> If Xen's static shared memory carveout is found, it is placed as child
>> nodes under the top level /reserved-memory node as also done in commit
>> 51a2b3f10918 ("xen/arm: fix duplicate /reserved-memory node in Dom0").
>>
>> Signed-off-by: Beleswar Padhi <b-padhi@xxxxxx>
> I'm afraid adding support for passthroughing /reserved-memory nodes like this
> patch does is unfortunately not enough to help with the common use cases these
> days. /reserved-memory nodes got complex from Xen PoV mainly because of the
> "reusable" property that lets OS (say Linux) use such memory for whatever it
> wants until device asks for it. The common use case is "shared-dma-pool" and 
> CMA
> buffers. They specify "reusable" (hence no "no-map"). This causes a few 
> issues:
> 1) Xen maps memory for passthroughed regions for domUs as device memory (keep 
> in
> mind the rules from Arm ARM for joining stage 1 and stage 2 memory attributes 
> -
> device type is the most stringent one and wins) but for shared memory it 
> should
> be normal memory.
>
> 2) Linux can use such reserved memory region as normal RAM and hence use it 
> for
> Xen shared buffers (e.g. hypercall buffers, shared memory, etc.). As per
> arch-arm.h any memory shared between guest and Xen needs to be of normal type.
>
> 3) Xen does not give reserved memory to the allocators and does not have 
> backing
> page_info structs for such pages. If such memory ends up being used with Xen
> (e.g. to map foreign pages) such access will fail because Xen will not be able
> to get pages for such region.
>
> Problem 3) exists today also for hwdom. Problems 1) and 2) are gone because 
> Xen
> maps memory of passthroughed nodes for hwdom in a more relaxed way so guest
> attributes win.
>
> This is high on my TODO list because there is a real need from customers to 
> add
> a proper support but I don't have any bandwidth this year to do any work.
>
> I guess your solution would only work for the simplest use case which is for
> /reserved-memory nodes with only "no-map" property present. We could limit the
> support for only such nodes (check if it contains "reusable" property and if 
> so,
> bail out) as a temporary solution if this helps.


Thanks for the detailed feedback. Let me come back with more reading
to decide if I can implement passing through 'reusable' carveouts or we
can go with only 'no-map' temporarily.

Thanks,
Beleswar

>
> ~Michal
>  > ---
>> Hello all,
>>
>> Sending this patch as discussed in:
>> https://lore.kernel.org/all/0e556bb5-269d-47d7-9ae8-68f06eee9fc0@xxxxxx/
>>
>> Test Configurations & Logs:
>> https://gist.github.com/3V3RYONE/bef3bb51459f8b823605261b35494a7e
>>
>> Thanks,
>> Beleswar
>>
>>  xen/common/device-tree/dom0less-build.c | 45 +++++++++++++++++++------
>>  1 file changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/common/device-tree/dom0less-build.c 
>> b/xen/common/device-tree/dom0less-build.c
>> index fcbeb8adbd..e9581327ee 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -334,7 +334,7 @@ static int __init handle_prop_pfdt(struct kernel_info 
>> *kinfo,
>>  static int __init scan_pfdt_node(struct kernel_info *kinfo, const void 
>> *pfdt,
>>                                   int nodeoff,
>>                                   uint32_t address_cells, uint32_t 
>> size_cells,
>> -                                 bool scan_passthrough_prop)
>> +                                 bool scan_passthrough_prop, bool 
>> resv_mem_node)
>>  {
>>      int rc = 0;
>>      void *fdt = kinfo->fdt;
>> @@ -358,13 +358,20 @@ static int __init scan_pfdt_node(struct kernel_info 
>> *kinfo, const void *pfdt,
>>      while ( node_next > 0 )
>>      {
>>          rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, 
>> size_cells,
>> -                            scan_passthrough_prop);
>> +                            scan_passthrough_prop, false);
>>          if ( rc )
>>              return rc;
>>  
>>          node_next = fdt_next_subnode(pfdt, node_next);
>>      }
>>  
>> +    if ( resv_mem_node )
>> +    {
>> +        rc = make_shm_resv_memory_node(kinfo, address_cells, size_cells);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>>      return fdt_end_node(fdt);
>>  }
>>  
>> @@ -389,7 +396,8 @@ static int __init check_partial_fdt(void *pfdt, size_t 
>> size)
>>  }
>>  
>>  static int __init domain_handle_dtb_boot_module(struct domain *d,
>> -                                                struct kernel_info *kinfo)
>> +                                                struct kernel_info *kinfo,
>> +                                                bool 
>> *found_reserved_mem_node)
>>  {
>>      void *pfdt;
>>      int res, node_next;
>> @@ -430,8 +438,8 @@ static int __init domain_handle_dtb_boot_module(struct 
>> domain *d,
>>              continue;
>>  
>>          /*
>> -         * Only scan /$(interrupt_controller) /aliases /passthrough,
>> -         * ignore the rest.
>> +         * Only scan /$(interrupt_controller) /aliases /passthrough
>> +         * /reserved-memory, ignore the rest.
>>           * They don't have to be parsed in order.
>>           *
>>           * Take the interrupt controller phandle value from the special
>> @@ -445,7 +453,7 @@ static int __init domain_handle_dtb_boot_module(struct 
>> domain *d,
>>              res = scan_pfdt_node(kinfo, pfdt, node_next,
>>                                   DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
>>                                   DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
>> -                                 false);
>> +                                 false, false);
>>              if ( res )
>>                  goto out;
>>              continue;
>> @@ -455,11 +463,22 @@ static int __init domain_handle_dtb_boot_module(struct 
>> domain *d,
>>              res = scan_pfdt_node(kinfo, pfdt, node_next,
>>                                   DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
>>                                   DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
>> -                                 true);
>> +                                 true, false);
>>              if ( res )
>>                  goto out;
>>              continue;
>>          }
>> +        if ( dt_node_cmp(name, "reserved-memory") == 0 )
>> +        {
>> +            res = scan_pfdt_node(kinfo, pfdt, node_next,
>> +                                 DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
>> +                                 DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
>> +                                 false, true);
>> +            if ( res )
>> +                goto out;
>> +            *found_reserved_mem_node = true;
>> +            continue;
>> +        }
>>      }
>>  
>>   out:
>> @@ -486,6 +505,7 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>  {
>>      int addrcells, sizecells;
>>      int ret, fdt_size = DOMU_DTB_SIZE;
>> +    bool found_reserved_mem_node = false;
>>  
>>      BUILD_BUG_ON(DOMU_DTB_SIZE > SZ_2M);
>>  
>> @@ -538,7 +558,7 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>       */
>>      if ( kinfo->dtb )
>>      {
>> -        ret = domain_handle_dtb_boot_module(d, kinfo);
>> +        ret = domain_handle_dtb_boot_module(d, kinfo, 
>> &found_reserved_mem_node);
>>          if ( ret )
>>              goto err;
>>      }
>> @@ -556,9 +576,12 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>      if ( ret )
>>          goto err;
>>  
>> -    ret = make_resv_memory_node(kinfo, addrcells, sizecells);
>> -    if ( ret )
>> -        goto err;
>> +    if ( !found_reserved_mem_node )
>> +    {
>> +        ret = make_resv_memory_node(kinfo, addrcells, sizecells);
>> +        if ( ret )
>> +            goto err;
>> +    }
>>  
>>      ret = make_intc_domU_node(kinfo);
>>      if ( ret )