Re: [PATCH] xen/dt: Skip non-RAM reserved-memory regions in make_memory_node()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 28-Oct-26 22:17, Kuan-Wei Chiu wrote:
> Xen creates a second /memory node for Dom0 from
> bootinfo_get_reserved_mem() without checking whether those regions
> belong to host RAM. However, /reserved-memory can also describe
> on-chip SRAM or firmware regions outside system RAM. Listing them in a
> /memory node causes Dom0 to register non-RAM addresses as system RAM,
> conflicting with device MMIO mappings.
It's worth adding a reference to Linux binding for nvmem-rmem as an example (the
only one so far?) of /reserved-memory describing non RAM. Please also mention
Linux comment at early_init_dt_reserve_memory().

> 
> Skip reserved-memory banks outside the host RAM banks reported by
> bootinfo_get_mem() in make_memory_node().
Have you considered EFI boot? There we don't use /memory and bootinfo_get_mem()
contains usable memory. IOW I think it does not contain reserved memory. In such
scenario, we would skip the reserved regions because they are not described in
memory membank. So it would be a regression in behavior.

> 
> Fixes: 248faa637d2f ("xen/arm: add reserved-memory regions to the dom0 memory 
> node")
> Signed-off-by: Kuan-Wei Chiu <visitorckw@xxxxxxxxx>
> ---
> Found while trying to bring up Xen on new hardware.
> Tested on real silicon.
> 
>  xen/common/device-tree/domain-build.c | 42 +++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/device-tree/domain-build.c 
> b/xen/common/device-tree/domain-build.c
> index 30a59abfa7..33baf150a6 100644
> --- a/xen/common/device-tree/domain-build.c
> +++ b/xen/common/device-tree/domain-build.c
> @@ -504,6 +504,34 @@ int __init make_chosen_node(const struct kernel_info 
> *kinfo)
>      return res;
>  }
>  
> +static bool __init is_in_host_ram(const struct membank *bank)
> +{
> +    const struct membanks *ram = bootinfo_get_mem();
> +    paddr_t start = bank->start;
> +    paddr_t end = start + bank->size;
> +    unsigned int i;
> +
> +    if ( !bank->size || end < start )
You can omit the empty check. Empty banks are not added to membanks.

> +        return false;
> +
> +    for ( i = 0; i < ram->nr_banks; i++ )
> +    {
> +        paddr_t ram_start = ram->bank[i].start;
> +        paddr_t ram_end = ram_start + ram->bank[i].size;
> +
> +        if ( start < ram_start )
> +            return false;
> +
> +        if ( end <= ram_end )
> +            return true;
> +
> +        if ( start < ram_end )
> +            start = ram_end;
> +    }
> +
> +    return false;
> +}
> +
>  int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
>                              int sizecells, const struct membanks *mem)
>  {
> @@ -513,14 +541,20 @@ int __init make_memory_node(const struct kernel_info 
> *kinfo, int addrcells,
>      int nr_cells = 0;
>      __be32 reg[DT_MEM_NODE_REG_RANGE_SIZE];
>      __be32 *cells;
> +    bool is_resv = (mem == bootinfo_get_reserved_mem());
>  
>      if ( mem->nr_banks == 0 )
>          return -ENOENT;
>  
>      /* find the first memory range that is reserved for device (or firmware) 
> */
> -    for ( i = 0; i < mem->nr_banks &&
> -                 (mem->bank[i].type != MEMBANK_DEFAULT); i++ )
> -        ;
> +    for ( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        if ( mem->bank[i].type != MEMBANK_DEFAULT )
> +            continue;
> +        if ( is_resv && !is_in_host_ram(&mem->bank[i]) )
> +            continue;
This filter (MEMBANK_DEFAULT and your check) is now duplicated in two places.
Please add a helper for it. Also adjust the now stale comment.

> +        break;
> +    }
>  
>      if ( i == mem->nr_banks )
>          return 0;
> @@ -544,6 +578,8 @@ int __init make_memory_node(const struct kernel_info 
> *kinfo, int addrcells,
>          if ( (mem->bank[i].type == MEMBANK_STATIC_DOMAIN) ||
>               (mem->bank[i].type == MEMBANK_FDT_RESVMEM) )
>              continue;
> +        if ( is_resv && !is_in_host_ram(&mem->bank[i]) )
> +            continue;
>  
>          nr_cells += reg_size;
>          BUG_ON(nr_cells > ARRAY_SIZE(reg));

~Michal