From: Julien Grall <jgrall@amazon.com>

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 <jgrall@amazon.com>

---
    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

On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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 <jgrall@amazon.com>
> 
> ---
>     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);

Hi Stefano,

On 02/04/2022 01:00, Stefano Stabellini wrote:
> On Mon, 21 Feb 2022, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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 <jgrall@amazon.com>
>>
>> ---
>>      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

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 <jgrall@amazon.com>
> > > 
> > > 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 <jgrall@amazon.com>
> > > 
> > > ---
> > >      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))

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