Hi Oleksandr,
On 26/11/2021 13:51, Oleksandr wrote:
On 25.11.21 21:04, Julien Grall wrote:
{
+ mfn_t mfn = lpae_get_mfn(pte);
+
ASSERT(p2m_is_valid(pte));
/*
@@ -731,11 +733,22 @@ static void p2m_put_l3_page(const lpae_t pte)
*/
if ( p2m_is_foreign(pte.p2m.type) )
{
- mfn_t mfn = lpae_get_mfn(pte);
-
ASSERT(mfn_valid(mfn));
put_page(mfn_to_page(mfn));
}
+
+#ifdef CONFIG_GRANT_TABLE
+ /*
+ * Check whether we deal with grant table page. As the grant
table page
+ * is xen_heap page and its entry has known p2m type, detect it
and mark
+ * the stored GFN as invalid. Although this check is not precise
and we
+ * might end up updating this for other xen_heap pages, this
action is
+ * harmless to these pages since only grant table pages have
this field
+ * in use. So, at worst, unnecessary action might be performed.
+ */
+ if ( (pte.p2m.type == p2m_ram_rw) && is_xen_heap_mfn(mfn) )
I would use p2m_is_ram() to cover read-only mapping. I think it would
also be better to use an ``else if`` so it is clear that this doesn't
cover foreign mapping (it is possible to map xenheap page from another
domain).
ok, will use p2m_is_ram() and ``else if`` construct, however I don't
entirely understand why we also want/need to include read-only pages (as
type is set to p2m_ram_rw in xenmem_add_to_physmap_one() for case
XENMAPSPACE_grant_table)?
Most of this code is already ready to be used by other xenheap pages
(see other part of the discussion). So I would like to use p2m_is_ram()
as it reduces the risk of us forgetting that read-only are not handled.
[...]
+ page_get_frame_gfn(pg_); \
+})
#define gnttab_need_iommu_mapping(d) \
(is_domain_direct_mapped(d) && is_iommu_enabled(d))
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 7b5e7b7..a00c5f5 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -98,9 +98,17 @@ struct page_info
#define PGT_writable_page PG_mask(1, 1) /* has writable
mappings? */
#define PGT_type_mask PG_mask(1, 1) /* Bits 31 or
63. */
- /* Count of uses of this frame as its current type. */
-#define PGT_count_width PG_shift(2)
-#define PGT_count_mask ((1UL<<PGT_count_width)-1)
+ /* 2-bit count of uses of this frame as its current type. */
+#define PGT_count_mask PG_mask(3, 3)
+
+/*
+ * Stored in bits [28:0] or [60:0] GFN if page is used for grant
table frame.
I think this wording is conflicting with ...
+ * This only valid for the xenheap pages.
... this becase xen heap pages are used in other situations. But I
would prefer if the comment doesn't mention grant-table frame. This
would allow use to repurpose the field for other xenheap if needed.
Typo: This *is* only valid
ok, so how about to simply mention it's purpose as xenheap GFN here and
down this header?
For example,
Stored in bits [28:0] or [60:0] GFN if page is xenheap page.
BTW, shall I rename the access helpers page_set(get)_frame_gfn() as
well? For me the frame is associated with grant-table.
Something to: page_set(get)_xenheap_gfn() or even page_set(get)_gfn().
Naming them to page_{set, get)_xenheap_gfn() sounds like a good idea.
It would be clearer what is the purpose of the two helpers.
+#define arch_alloc_xenheap_page(p) page_set_frame_gfn(p, INVALID_GFN)
+#define arch_free_xenheap_page(p) \
+ BUG_ON(!gfn_eq(page_get_frame_gfn(p), INVALID_GFN))
I believe this BUG_ON() could be triggered if gnttab_map_frame()
succeeds but then we fail to insert the entry in the P2M. This means
we would need to revert changes done in gnttab_map_frame() in case of
failure.
However, I am still a bit unease with the BUG_ON(). A domain will not
necessarily remove the grant-table mapping from its P2M before
shutting down. So you are relying on Xen to go through the P2M before
we free the page.
This is the case today, but I am not sure we would want to rely on it
because it will be hard to remember this requirement if we decide to
optimize p2m_relinquish().
One possibility would be to add a comment in p2m_relinquish(). That's
assuming there are no other places which could lead to false
positively hit the BUG().
These make me think that it would be better (safer and simpler) to just
remove this BUG_ON() for now. Do you agree?
I would drop the BUG_ON() here.
Cheers,
--
Julien Grall
