[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V6 1/2] xen/gnttab: Store frame GFN in struct page_info on Arm



Hi Jan,

On 24/06/2022 07:45, Jan Beulich wrote:
On 23.06.2022 19:50, Julien Grall wrote:
On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
@@ -1505,7 +1507,23 @@ int xenmem_add_to_physmap_one(
       }
/* Map at new location. */
-    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);

+    if ( !p2m_is_ram(t) || !is_xen_heap_mfn(mfn) )
+        rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);

I would expand the comment above to explain why you need a different
path for xenheap mapped as RAM. AFAICT, this is because we need to call
page_set_xenheap_gfn().

+    else
+    {
+        struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+        p2m_write_lock(p2m);
+        if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)), INVALID_GFN) )

Sorry to only notice it now. This check will also change the behavior
for XENMAPSPACE_shared_info. Now, we are only allowed to map the shared
info once.

I believe this is fine because AFAICT x86 already prevents it. But this
is probably something that ought to be explained in the already long
commit message.

If by "prevent" you mean x86 unmaps the page from its earlier GFN, then
yes. But this means that Arm would better follow that model instead of
returning -EBUSY in this case. Just think of kexec-ing or a boot loader
wanting to map shared info or grant table: There wouldn't necessarily
be an explicit unmap.

I spent some time to think about this. There is a potential big issue with implicit unmapping from its earlier GFN. Imagine the boot loader decided to map the page in place of a RAM.

If the boot loader didn't unmap the page then when the OS map again, we would have a hole in the RAM. The OS may not know that and it may end up to use a page as RAM (and crash).

The problem is the same for kexec and AFAIU that's why we need to use soft-reset when kexec-ing.

So overall, I think we should prevent implicit unmap. So it would help to enforce that the bootloader (or any other components) clean-up behind themselves (i.e. unmap the page and populate if necessary).

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.