| Message ID | 20220221102218.33785-7-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 <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,
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,
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);