Re: [Xen-devel] [PATCH] xen/arm: p2m: Correctly flush TLB in create_p2m_entries

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-01-08 at 17:13 +0000, Stefano Stabellini wrote:
> On Wed, 8 Jan 2014, Julien Grall wrote:
> > The p2m is shared between VCPUs for each domain. Currently Xen only flush
> > TLB on the local PCPU. This could result to mismatch between the mapping in 
> > the
> > p2m and TLBs.
> > 
> > Flush TLBs used by this domain on every PCPU.
> > 
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> The fix makes sense to me.

Me too. Has anyone had a grep for similar issues?

(the reason for getting this wrong is that for cache flushes the "is"
suffix restricts the flush to the IS domain, whereas with tlb flushes
the "is" suffix broadcasts instead of keeping it local, which is a bit
confusing ;-))

> 
> > ---
> > 
> > This is a possible bug fix (found by reading the code) for Xen 4.4. I have
> > added a small optimisation to avoid flushing all TLBs when a VCPU of this
> > domain is running on the current cpu.
> > 
> > The downside of this patch is the function can be a little bit slower 
> > because
> > Xen is flushing more TLBs.
> 
> Yes, I wonder how much slower it is going to be, considering that the flush
> is executed for every iteration of the loop.

It might be better to set the current VMID to the target domain for the
duration of this function, we'd still need the broadcast but at least we
wouldn't be killing unrelated VMIDs.

Pulling the flush out of the loop would require great case WRT accesses
from other VCPUs, e.g. you'd have to put the pages on a list (page->list
might be available?) and issue the put_page() after the flush, otherwise
it might get recycled into another domain while the first domain still
has TLB entries for it.

Or is there always something outside this function which holds another
ref count such that the page definitely won't be freed by the put_page
here?

Actually, do the existing code not have this issue already? The put_page
is before the flush. If this bug does exist now then I'd be inclined to
consider this a bug fix for 4.4, rather than a potential optimisation
for 4.5.

While looking at this function I'm now wondering what happens to the
existing page on ALLOCATE or INSERT, is it leaked?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel