| Message ID | 20220221102218.33785-8-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
On Mon, 21 Feb 2022, Julien Grall wrote: > From: Julien Grall <julien.grall@arm.com> > > Now that xen_pt_update_entry() is able to deal with different mapping > size, we can replace the open-coding of the page-tables update by a call > to modify_xen_mappings(). > > As the function is not meant to fail, a BUG_ON() is added to check the > return. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> Nice! > --- > Changes in v2: > - Stay consistent with how function name are used in the commit > message > - Add my AWS signed-off-by > --- > xen/arch/arm/mm.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 7b4b9de8693e..f088a4b2de96 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -599,11 +599,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr) > > void __init remove_early_mappings(void) > { > - lpae_t pte = {0}; > - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte); > - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M), > - pte); > - flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE); > + int rc; > + > + rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END, > + _PAGE_BLOCK); > + BUG_ON(rc); Am I right that we are actually destroying the mapping, which usually is done by calling destroy_xen_mappings, but we cannot call destroy_xen_mappings in this case because it doesn't take a flags parameter? If so, then I would add a flags parameter to destroy_xen_mappings instead of calling modify_xen_mappings just to pass _PAGE_BLOCK. But I don't feel strongly about it so if you don't feel like making the change to destroy_xen_mappings, you can add my acked-by here anyway.
Hi Stefano, On 02/04/2022 01:04, Stefano Stabellini wrote: > On Mon, 21 Feb 2022, Julien Grall wrote: >> From: Julien Grall <julien.grall@arm.com> >> >> Now that xen_pt_update_entry() is able to deal with different mapping >> size, we can replace the open-coding of the page-tables update by a call >> to modify_xen_mappings(). >> >> As the function is not meant to fail, a BUG_ON() is added to check the >> return. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Nice! > > >> --- >> Changes in v2: >> - Stay consistent with how function name are used in the commit >> message >> - Add my AWS signed-off-by >> --- >> xen/arch/arm/mm.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index 7b4b9de8693e..f088a4b2de96 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -599,11 +599,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr) >> >> void __init remove_early_mappings(void) >> { >> - lpae_t pte = {0}; >> - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte); >> - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M), >> - pte); >> - flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE); >> + int rc; >> + >> + rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END, >> + _PAGE_BLOCK); >> + BUG_ON(rc); > > Am I right that we are actually destroying the mapping, which usually is > done by calling destroy_xen_mappings, but we cannot call > destroy_xen_mappings in this case because it doesn't take a flags > parameter? You are right. > > If so, then I would add a flags parameter to destroy_xen_mappings > instead of calling modify_xen_mappings just to pass _PAGE_BLOCK. > But I don't feel strongly about it so if you don't feel like making the > change to destroy_xen_mappings, you can add my acked-by here anyway. destroy_xen_mappings() is a function used by common code. This is the only place so far where I need to pass _PAGE_BLOCK and I don't expect it to be used by the common code any time soon. So I am not in favor to add an extra parameter for destroy_xen_mappings(). Would you prefer if I open-code the call to xen_pt_update? Cheers,
On Sat, 2 Apr 2022, Julien Grall wrote: > On 02/04/2022 01:04, Stefano Stabellini wrote: > > On Mon, 21 Feb 2022, Julien Grall wrote: > > > From: Julien Grall <julien.grall@arm.com> > > > > > > Now that xen_pt_update_entry() is able to deal with different mapping > > > size, we can replace the open-coding of the page-tables update by a call > > > to modify_xen_mappings(). > > > > > > As the function is not meant to fail, a BUG_ON() is added to check the > > > return. > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > Nice! > > > > > > > --- > > > Changes in v2: > > > - Stay consistent with how function name are used in the commit > > > message > > > - Add my AWS signed-off-by > > > --- > > > xen/arch/arm/mm.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > > > index 7b4b9de8693e..f088a4b2de96 100644 > > > --- a/xen/arch/arm/mm.c > > > +++ b/xen/arch/arm/mm.c > > > @@ -599,11 +599,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr) > > > void __init remove_early_mappings(void) > > > { > > > - lpae_t pte = {0}; > > > - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), > > > pte); > > > - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + > > > SZ_2M), > > > - pte); > > > - flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE); > > > + int rc; > > > + > > > + rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END, > > > + _PAGE_BLOCK); > > > + BUG_ON(rc); > > > > Am I right that we are actually destroying the mapping, which usually is > > done by calling destroy_xen_mappings, but we cannot call > > destroy_xen_mappings in this case because it doesn't take a flags > > parameter? > > You are right. > > > > > If so, then I would add a flags parameter to destroy_xen_mappings > > instead of calling modify_xen_mappings just to pass _PAGE_BLOCK. > > But I don't feel strongly about it so if you don't feel like making the > > change to destroy_xen_mappings, you can add my acked-by here anyway. > > destroy_xen_mappings() is a function used by common code. This is the only > place so far where I need to pass _PAGE_BLOCK and I don't expect it to be used > by the common code any time soon. > > So I am not in favor to add an extra parameter for destroy_xen_mappings(). > > Would you prefer if I open-code the call to xen_pt_update? No need, just add a one-line in-code comment like: /* destroy the _PAGE_BLOCK mapping */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 7b4b9de8693e..f088a4b2de96 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -599,11 +599,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr) void __init remove_early_mappings(void) { - lpae_t pte = {0}; - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte); - write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M), - pte); - flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE); + int rc; + + rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END, + _PAGE_BLOCK); + BUG_ON(rc); } /*