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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.10.20 11:41, Julien Grall wrote:

Hi Jan, Julien

Sorry for the late response.

> 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)) )
> > > +    {
> > > +#ifdef CONFIG_IOREQ_SERVER
> > > +        if ( domain_has_ioreq_server(p2m->domain) &&
> > > +             (p2m->domain == current->domain) && 
> > > p2m_is_ram(orig_pte.p2m.type) )
> > > +            p2m->domain->qemu_mapcache_invalidate = true;
> > > +#endif
> > >           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
> > entries?
> 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.
Well, but inside p2m_free_entry() we don't have a new entry in order to 
check whether new MFN is actually different. An extra arg only comes to 
mind...
[Didn't update call sites yet and didn't tested]

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 5bb23df..4001f46 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -739,7 +739,7 @@ static void p2m_put_l3_page(const lpae_t pte)

 /* Free lpae sub-tree behind an entry */
 static void p2m_free_entry(struct p2m_domain *p2m,
-                           lpae_t entry, unsigned int level)
+                           lpae_t entry, lpae_t new_entry, unsigned int 
level)
 {
     unsigned int i;
     lpae_t *table;
@@ -750,17 +750,19 @@ static void p2m_free_entry(struct p2m_domain *p2m,
     if ( !p2m_is_valid(entry) )
         return;

-    /* Nothing to do but updating the stats if the entry is a 
super-page. */
-    if ( p2m_is_superpage(entry, level) )
+    if ( p2m_is_superpage(entry, level) || (level == 3) )
     {
-        p2m->stats.mappings[level]--;
-        return;
-    }
+#ifdef CONFIG_IOREQ_SERVER
+        if ( domain_has_ioreq_server(p2m->domain) &&
+             (p2m->domain == current->domain) &&
+             !mfn_eq(lpae_get_mfn(new_entry), lpae_get_mfn(entry)) &&
+             p2m_is_ram(entry.p2m.type) )
+            p2m->domain->qemu_mapcache_invalidate = true;
+#endif

-    if ( level == 3 )
-    {
         p2m->stats.mappings[level]--;
-        p2m_put_l3_page(entry);
+        if ( level == 3 )
+            p2m_put_l3_page(entry);
         return;
     }

(END)

--
Regards,

Oleksandr Tyshchenko