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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 11/11/2020 00:03, Oleksandr wrote:
> On 16.10.20 11:41, Julien Grall wrote:
> > 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...
Aside the recursive call, there are two users for p2m_free_entry():
  - When we fail to shatter a superpage in OOM
  - When the entry is replaced by an entry with a different base

I wouldn't be too concerned to send spurious mapcache invalidation in an 
error path. So I don't think you need to know the new entry.
What you need to know if the type of the leaf.

Cheers,

--
Julien Grall