Re: [Xen-devel] [PATCH 3/3] xen/arm: p2m: Perform local TLB invalidation on vCPU migration

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 08/03/17 18:58, Stefano Stabellini wrote:
> On Wed, 8 Mar 2017, Julien Grall wrote:
> > The ARM architecture allows an OS to have per-CPU page tables, as it
> > guarantees that TLBs never migrate from one CPU to another.
> >
> > This works fine until this is done in a guest. Consider the following
> > scenario:
> >     - vcpu-0 maps P to V
> >     - vpcu-1 maps P' to V
> >
> > If run on the same physical CPU, vcpu-1 can hit in TLBs generated by
> > vcpu-0 accesses, and access the wrong physical page.
> >
> > The solution to this is to keep a per-p2m map of which vCPU ran the last
> > on each given pCPU and invalidate local TLBs if two vPCUs from the same
> > VM run on the same CPU.
> >
> > Unfortunately it is not possible to allocate per-cpu variable on the
> > fly. So for now the size of the array is NR_CPUS, this is fine because
> > we still have space in the structure domain. We may want to add an
> > helper to allocate per-cpu variable in the future.
> I think I understand the problem, thanks for the description. I have a
> question: when a vcpu of a different domain is scheduled on a pcpu,
> there is no impact on tlb flushes in relation to this problem, is there?
>
> For example:
>
> 1) vcpu0/domain1 -> pcpu0
> 2) vcpu0/domain2 -> pcpu0
> 3) vcpu1/domain1 -> pcpu0
>
> In 3) we still need to do the flush_tlb_local(), no matter if 2)
> happened or not, right?
That's correct. The last vCPU running is stored per p2m domain and 
belongs to the associated domain.
> Also, I have one suggestion below.
[...]

> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index 0899523084..18c57f936e 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -96,6 +96,9 @@ struct p2m_domain {
> >
> >      /* back pointer to domain */
> >      struct domain *domain;
> > +
> > +    /* Keeping track on which CPU this p2m was used and for which vCPU */
> > +    uint8_t last_vcpu_ran[NR_CPUS];
> >  };
> NR_CPUS is usually much larger than cpu_possible_map. I think it would
> be best to allocate last_vcpu_ran dynamically (after cpu_possible_map is
> set). It should reduce the memory impact significantly.
Well, I thought about it when I wrote the patch. p2m_domain is currently 
embedded in the structure domain and Xen will always allocate a page 
even if structure is smaller. So for now, static allocation is the best.
If in the future we decide to allocate multiple p2m_domain for a domain 
(e.g for feature like altp2m), then the benefit of dynamic allocation is 
still questionable. The altp2m code will already use more memory because 
of the page table, so I don't think it is a concern to use 128-bytes 
more per p2m here.
Also, I noticed that NR_CPUS is set to 128 for both 32-bit and 64-bits, 
but technically 32-bit will not use more than 8 CPUs due to the 
limitation on GICv2 (we don't support GICv3 on 32-bit).
Cheers,

--
Julien Grall

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