| Message ID | 20220520120937.28925-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
> On 20 May 2022, at 13:09, Julien Grall <julien@xen.org> 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> Hi Julien, It looks ok to me. I’ve also tested it starting, console-ing, destroying few guests and I’ve got no problem Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
On Fri, 20 May 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> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v4: > - Switch the check to a different expression that will still > result to the same truth table. > > 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 c4487dd7fc46..747083d820dd 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1119,7 +1119,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) ) > @@ -1434,11 +1440,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 >
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index c4487dd7fc46..747083d820dd 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1119,7 +1119,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) ) @@ -1434,11 +1440,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);