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

[Kuan-Wei Chiu <visitorckw@xxxxxxxxx>]

Hi Michal,

On Mon, Nov 02, 2026 at 02:02:20PM +0100, Orzel, Michal wrote:
> 
> 
> 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().

Ack.

> 
> > 
> > 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.
> 

I'm not quite familiar with EFI, so I indeed didn't consider the EFI
boot scenario in the first place. I think I should skip the
is_in_host_ram() check when efi_enabled(EFI_BOOT)?

> > 
> > 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.

Ack.

> 
> > +        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.
> 

Ack.

Regards,
Kuan-Wei

> > +        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
>