Re: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 02/04/2022 01:10, Stefano Stabellini wrote:
> On Mon, 21 Feb 2022, Julien Grall wrote:
> > From: Julien Grall <julien.grall@xxxxxxx>
> >
> > Now that map_pages_to_xen() has been extended to support 2MB mappings,
> > we can replace the create_mappings() calls by map_pages_to_xen() calls.
> >
> > The mapping can also be marked read-only has Xen as no business to
> > modify the host Device Tree.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ---
> >      Changes in v2:
> >          - Add my AWS signed-off-by
> >          - Fix typo in the commit message
> > ---
> >   xen/arch/arm/mm.c | 18 +++++++++++++-----
> >   1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index f088a4b2de96..24de8dcb9042 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -559,6 +559,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> >       paddr_t offset;
> >       void *fdt_virt;
> >       uint32_t size;
> > +    int rc;
> >   
> >       /*
> >        * Check whether the physical FDT address is set and meets the minimum
> > @@ -574,8 +575,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> >       /* The FDT is mapped using 2MB superpage */
> >       BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
> >   
> > -    create_mappings(xen_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
> > -                    SZ_2M >> PAGE_SHIFT, SZ_2M);
> > +    rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr),
> > +                          SZ_2M >> PAGE_SHIFT,
> > +                          PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +    if ( rc )
> > +        panic("Unable to map the device-tree.\n");
> > +
> >   
> >       offset = fdt_paddr % SECOND_SIZE;
> >       fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> > @@ -589,9 +594,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
> >   
> >       if ( (offset + size) > SZ_2M )
> >       {
> > -        create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
> > -                        paddr_to_pfn(base_paddr + SZ_2M),
> > -                        SZ_2M >> PAGE_SHIFT, SZ_2M);
> > +        rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> > +                              maddr_to_mfn(base_paddr + SZ_2M),
> > +                              SZ_2M >> PAGE_SHIFT,
> > +                              PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> > +        if ( rc )
> > +            panic("Unable to map the device-tree\n");
> >       }
> Very good! :-)
>
> I have a small preference for making the change to PAGE_HYPERVISOR_RO in
> a separate patch because it would make it easier to revert in the
> future if we need so (e.g. overlays...). But it is OK either way.
The mapping is only used for early boot. For runtime we are relocating 
the FDT and it is writable.
That said, I don't think the FDT should ever be writable. The size of 
the FDT is bounded and therefore you will likely not be able to add a 
new property/node without relocating it.
I haven't looked at latest DT overlay series. But in the previous 
version I was under the impression that only the unflatten version would 
be touched. IOW, the flatten version would be untouched. Can you confirm 
this is still the case?
Cheers,

--
Julien Grall