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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 11.11.20 21:27, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien.


> 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.
Thank you for the clarification, sounds reasonable.


> What you need to know if the type of the leaf.
Yes, to check whether it is a RAM page.


--
Regards,

Oleksandr Tyshchenko