Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
Hi Stefano, On 05/04/2022 21:49, Stefano Stabellini wrote: On Sat, 2 Apr 2022, Julien Grall wrote: Maybe it would be clearer if I write: !((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN)) It is not much a matter of clarity -- I just wanted to check with you the reasons for the if condition because, as you wrote, wrong tlb flushes can have catastrophic effects. That said, actually I prefer your second version: !((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN)) I have updated the patch to use this switch. Cheers, -- Julien Grall
Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
On Sat, 2 Apr 2022, Julien Grall wrote:
> Hi Stefano,
>
> On 02/04/2022 01:00, Stefano Stabellini wrote:
> > On Mon, 21 Feb 2022, Julien Grall wrote:
> > > From: Julien Grall
> > >
> > > Currently, the function xen_pt_update() will flush the TLBs even when
> > > the mappings are inserted. This is a bit wasteful because we don't
> > > allow mapping replacement. Even if we were, the flush would need to
> > > happen earlier because mapping replacement should use Break-Before-Make
> > > when updating the entry.
> > >
> > > A single call to xen_pt_update() can perform a single action. IOW, it
> > > is not possible to, for instance, mix inserting and removing mappings.
> > > Therefore, we can use `flags` to determine what action is performed.
> > >
> > > This change will be particularly help to limit the impact of switching
> > > boot time mapping to use xen_pt_update().
> > >
> > > Signed-off-by: Julien Grall
> > >
> > > ---
> > > Changes in v2:
> > > - New patch
> > > ---
> > > xen/arch/arm/mm.c | 17 ++---
> > > 1 file changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index fd16c1541ce2..7b4b9de8693e 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> > > mfn, unsigned int level,
> > > /* We should be here with a valid MFN. */
> > > ASSERT(!mfn_eq(mfn, INVALID_MFN));
> > > -/* We don't allow replacing any valid entry. */
> > > +/*
> > > + * We don't allow replacing any valid entry.
> > > + *
> > > + * Note that the function xen_pt_update() relies on this
> > > + * assumption and will skip the TLB flush. The function will need
> > > + * to be updated if the check is relaxed.
> > > + */
> > > if ( lpae_is_valid(entry) )
> > > {
> > > if ( lpae_is_mapping(entry, level) )
> > > @@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
> > > }
> > > /*
> > > - * Flush the TLBs even in case of failure because we may have
> > > + * The TLBs flush can be safely skipped when a mapping is inserted
> > > + * as we don't allow mapping replacement (see xen_pt_check_entry()).
> > > + *
> > > + * For all the other cases, the TLBs will be flushed unconditionally
> > > + * even if the mapping has failed. This is because we may have
> > >* partially modified the PT. This will prevent any unexpected
> > >* behavior afterwards.
> > >*/
> > > -flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> > > +if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
> > > +flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> >
> > I am trying to think of a care where the following wouldn't be enough
> > but I cannot come up with one:
> >
> > if ( mfn_eq(mfn, INVALID_MFN) )
> > flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
>
> _PAGE_PRESENT is not set for two cases: when removing a page or populating
> page-tables for a region. Both of them will expect an INVALID_MFN (see the two
> asserts in xen_pt_check_entry()).
>
> Therefore your solution should work. However, technically the 'mfn' is ignored
> in both situation (hence why this is an ASSERT() rather than a prod check).
>
> Also, I feel it is better to flush more than less (missing a flush could have
> catastrophic result). So I chose to be explicit in which case the flush can be
> skipped.
>
> Maybe it would be clearer if I write:
>
> !((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN))
It is not much a matter of clarity -- I just wanted to check with you
the reasons for the if condition because, as you wrote, wrong tlb
flushes can have catastrophic effects.
That said, actually I prefer your second version:
!((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN))
Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
Hi Stefano,
On 02/04/2022 01:00, Stefano Stabellini wrote:
On Mon, 21 Feb 2022, Julien Grall wrote:
From: Julien Grall
Currently, the function xen_pt_update() will flush the TLBs even when
the mappings are inserted. This is a bit wasteful because we don't
allow mapping replacement. Even if we were, the flush would need to
happen earlier because mapping replacement should use Break-Before-Make
when updating the entry.
A single call to xen_pt_update() can perform a single action. IOW, it
is not possible to, for instance, mix inserting and removing mappings.
Therefore, we can use `flags` to determine what action is performed.
This change will be particularly help to limit the impact of switching
boot time mapping to use xen_pt_update().
Signed-off-by: Julien Grall
---
Changes in v2:
- New patch
---
xen/arch/arm/mm.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fd16c1541ce2..7b4b9de8693e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn,
unsigned int level,
/* We should be here with a valid MFN. */
ASSERT(!mfn_eq(mfn, INVALID_MFN));
-/* We don't allow replacing any valid entry. */
+/*
+ * We don't allow replacing any valid entry.
+ *
+ * Note that the function xen_pt_update() relies on this
+ * assumption and will skip the TLB flush. The function will need
+ * to be updated if the check is relaxed.
+ */
if ( lpae_is_valid(entry) )
{
if ( lpae_is_mapping(entry, level) )
@@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
}
/*
- * Flush the TLBs even in case of failure because we may have
+ * The TLBs flush can be safely skipped when a mapping is inserted
+ * as we don't allow mapping replacement (see xen_pt_check_entry()).
+ *
+ * For all the other cases, the TLBs will be flushed unconditionally
+ * even if the mapping has failed. This is because we may have
* partially modified the PT. This will prevent any unexpected
* behavior afterwards.
*/
-flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
+if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
+flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
I am trying to think of a care where the following wouldn't be enough
but I cannot come up with one:
if ( mfn_eq(mfn, INVALID_MFN) )
flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
_PAGE_PRESENT is not set for two cases: when removing a page or
populating page-tables for a region. Both of them will expect an
INVALID_MFN (see the two asserts in xen_pt_check_entry()).
Therefore your solution should work. However, technically the 'mfn' is
ignored in both situation (hence why this is an ASSERT() rather than a
prod check).
Also, I feel it is better to flush more than less (missing a flush could
have catastrophic result). So I chose to be explicit in which case the
flush can be skipped.
Maybe it would be clearer if I write:
!((flags & _PAGE_PRESENT) && !mfn_eq(mfn, INVALID_MFN))
Cheers,
--
Julien Grall
Re: [PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> Currently, the function xen_pt_update() will flush the TLBs even when
> the mappings are inserted. This is a bit wasteful because we don't
> allow mapping replacement. Even if we were, the flush would need to
> happen earlier because mapping replacement should use Break-Before-Make
> when updating the entry.
>
> A single call to xen_pt_update() can perform a single action. IOW, it
> is not possible to, for instance, mix inserting and removing mappings.
> Therefore, we can use `flags` to determine what action is performed.
>
> This change will be particularly help to limit the impact of switching
> boot time mapping to use xen_pt_update().
>
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - New patch
> ---
> xen/arch/arm/mm.c | 17 ++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fd16c1541ce2..7b4b9de8693e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int level,
> /* We should be here with a valid MFN. */
> ASSERT(!mfn_eq(mfn, INVALID_MFN));
>
> -/* We don't allow replacing any valid entry. */
> +/*
> + * We don't allow replacing any valid entry.
> + *
> + * Note that the function xen_pt_update() relies on this
> + * assumption and will skip the TLB flush. The function will need
> + * to be updated if the check is relaxed.
> + */
> if ( lpae_is_valid(entry) )
> {
> if ( lpae_is_mapping(entry, level) )
> @@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
> }
>
> /*
> - * Flush the TLBs even in case of failure because we may have
> + * The TLBs flush can be safely skipped when a mapping is inserted
> + * as we don't allow mapping replacement (see xen_pt_check_entry()).
> + *
> + * For all the other cases, the TLBs will be flushed unconditionally
> + * even if the mapping has failed. This is because we may have
> * partially modified the PT. This will prevent any unexpected
> * behavior afterwards.
> */
> -flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
> +if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
> +flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
I am trying to think of a care where the following wouldn't be enough
but I cannot come up with one:
if ( mfn_eq(mfn, INVALID_MFN) )
flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
[PATCH v3 06/19] xen/arm: mm: Avoid flushing the TLBs when mapping are inserted
From: Julien Grall
Currently, the function xen_pt_update() will flush the TLBs even when
the mappings are inserted. This is a bit wasteful because we don't
allow mapping replacement. Even if we were, the flush would need to
happen earlier because mapping replacement should use Break-Before-Make
when updating the entry.
A single call to xen_pt_update() can perform a single action. IOW, it
is not possible to, for instance, mix inserting and removing mappings.
Therefore, we can use `flags` to determine what action is performed.
This change will be particularly help to limit the impact of switching
boot time mapping to use xen_pt_update().
Signed-off-by: Julien Grall
---
Changes in v2:
- New patch
---
xen/arch/arm/mm.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fd16c1541ce2..7b4b9de8693e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1104,7 +1104,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn,
unsigned int level,
/* We should be here with a valid MFN. */
ASSERT(!mfn_eq(mfn, INVALID_MFN));
-/* We don't allow replacing any valid entry. */
+/*
+ * We don't allow replacing any valid entry.
+ *
+ * Note that the function xen_pt_update() relies on this
+ * assumption and will skip the TLB flush. The function will need
+ * to be updated if the check is relaxed.
+ */
if ( lpae_is_valid(entry) )
{
if ( lpae_is_mapping(entry, level) )
@@ -1417,11 +1423,16 @@ static int xen_pt_update(unsigned long virt,
}
/*
- * Flush the TLBs even in case of failure because we may have
+ * The TLBs flush can be safely skipped when a mapping is inserted
+ * as we don't allow mapping replacement (see xen_pt_check_entry()).
+ *
+ * For all the other cases, the TLBs will be flushed unconditionally
+ * even if the mapping has failed. This is because we may have
* partially modified the PT. This will prevent any unexpected
* behavior afterwards.
*/
-flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
+if ( !(flags & _PAGE_PRESENT) || mfn_eq(mfn, INVALID_MFN) )
+flush_xen_tlb_range_va(virt, PAGE_SIZE * nr_mfns);
spin_unlock(&xen_pt_lock);
--
2.32.0
