Re: [PATCH v3 07/19] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings()

2022-04-05 Thread Stefano Stabellini 
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 
> > > 
> > > 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 
> > > Signed-off-by: Julien Grall 
> > 
> > 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 */

Re: [PATCH v3 07/19] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings()

2022-04-02 Thread Julien Grall 

Hi Stefano,

On 02/04/2022 01:04, Stefano Stabellini wrote:

On Mon, 21 Feb 2022, Julien Grall wrote:

From: Julien Grall 

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 
Signed-off-by: Julien Grall 


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,

--
Julien Grall

Re: [PATCH v3 07/19] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings()

2022-04-01 Thread Stefano Stabellini 
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall 
> 
> 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 
> Signed-off-by: Julien Grall 

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.

[PATCH v3 07/19] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings()

2022-02-21 Thread Julien Grall 
From: Julien Grall 

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 
Signed-off-by: Julien Grall 

---
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);
 }
 
 /*
-- 
2.32.0