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

Re: [PATCH V2 21/23] xen/arm: Add mapcache invalidation handling

Hi Jan,

On 16/10/2020 07:29, Jan Beulich wrote:
On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
@@ -1067,7 +1068,14 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
      if ( p2m_is_valid(orig_pte) &&
           !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
+    {
+        if ( domain_has_ioreq_server(p2m->domain) &&
+             (p2m->domain == current->domain) && p2m_is_ram(orig_pte.p2m.type) 
+            p2m->domain->qemu_mapcache_invalidate = true;
          p2m_free_entry(p2m, orig_pte, level);
+    }

For all I have to say here, please bear in mind that I don't know
the internals of Arm memory management.

The first odd thing here the merely MFN-based condition. It may
well be that's sufficient, if there's no way to get a "not present"
entry with an MFN matching any valid MFN. (This isn't just with
your addition, but even before.
Invalid entries are always zeroed. So in theory the problem could arise if MFN 0 used in the guest. It should not be possible on staging, but I agree this should be fixed.

Given how p2m_free_entry() works (or is supposed to work in the
long run), is the new code you add guaranteed to only alter leaf

This path may also be called with tables. I think we want to move the check in p2m_free_entry() so we can find the correct leaf type.

If not, the freeing of page tables needs deferring until
after qemu has dropped its mappings.

Freeing the page tables doesn't release a page. So may I ask why we would need to defer it?

And with there being refcounting only for foreign pages, how do
you prevent the freeing of the page just unmapped before qemu has
dropped its possible mapping?
QEMU mappings can only be done using the foreign mapping interface. This means that page reference count will be incremented for each QEMU mappings. Therefore the page cannot disappear until QEMU dropped the last reference.

On the x86 side this problem is one
of the reasons why PVH Dom0 isn't "supported", yet. At least a
respective code comment would seem advisable, so the issue to be
addressed won't be forgotten.

Are you sure? Isn't because you don't take a reference on foreign pages while mapping it?

Anyway, Arm has supported foreign mapping since its inception. So if there is a bug, then it should be fixed.


Julien Grall



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