| Message ID | 20220221102218.33785-9-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
Hi Julien, > -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien > Grall > Sent: 2022年2月21日 18:22 > To: xen-devel@lists.xenproject.org > Cc: julien@xen.org; Julien Grall <Julien.Grall@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Julien Grall > <jgrall@amazon.com> > Subject: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using > map_pages_to_xen() > > From: Julien Grall <julien.grall@arm.com> > > 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 In my opinion I think it may should be: ... read-only as Xen has no business ... instead of: ... read-only has Xen as no business ... For this and other patches before this: Reviewed-by: Hongda Deng <Hongda.Heng@arm.com> > modify the host Device Tree. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > 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"); > } > > return fdt_virt; > -- > 2.32.0 > Cheers, --- Hongda
On 18/03/2022 07:36, Hongda Deng wrote: > Hi Julien, Hi Hongda, >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien >> Grall >> Sent: 2022年2月21日 18:22 >> To: xen-devel@lists.xenproject.org >> Cc: julien@xen.org; Julien Grall <Julien.Grall@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Julien Grall >> <jgrall@amazon.com> >> Subject: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using >> map_pages_to_xen() >> >> From: Julien Grall <julien.grall@arm.com> >> >> 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 > > In my opinion I think it may should be: > ... read-only as Xen has no business ... > instead of: > ... read-only has Xen as no business ... You are right, I have updated the commit message. > > For this and other patches before this: > Reviewed-by: Hongda Deng <Hongda.Heng@arm.com> There is one bug in patch #5 (I sent an e-mail with the possible fix). Can you confirm you are still happy with me to keep your reviewed-by for that patch? For the other patches, I have already committed patch #1-#3. So I will add your tag on patches #4, #6, #7, #8. Thank you for the review! Cheers,
On Mon, 21 Feb 2022, Julien Grall wrote: > From: Julien Grall <julien.grall@arm.com> > > 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@arm.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > 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.
Hi Stefano, On 02/04/2022 01:10, Stefano Stabellini wrote: > On Mon, 21 Feb 2022, Julien Grall wrote: >> From: Julien Grall <julien.grall@arm.com> >> >> 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@arm.com> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> 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,
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"); } return fdt_virt;