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