Re: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
Hi Stefano,
On 02/04/2022 01:10, Stefano Stabellini wrote:
On Mon, 21 Feb 2022, Julien Grall wrote:
From: Julien Grall
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
Signed-off-by: Julien Grall
---
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
Re: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> 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
> Signed-off-by: Julien Grall
>
> ---
> 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.
Re: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
On 18/03/2022 07:36, Hongda Deng wrote: Hi Julien, Hi Hongda, -Original Message- From: Xen-devel On Behalf Of Julien Grall Sent: 2022年2月21日 18:22 To: xen-devel@lists.xenproject.org Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini ; Bertrand Marquis ; Volodymyr Babchuk ; Julien Grall Subject: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen() From: Julien Grall 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 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, -- Julien Grall
RE: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月21日 18:22
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk ; Julien Grall
>
> Subject: [PATCH v3 08/19] xen/arm: mm: Re-implement early_fdt_map() using
> map_pages_to_xen()
>
> From: Julien Grall
>
> 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
> modify the host Device Tree.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> 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
