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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 8 Mar 2017, Julien Grall wrote:
> 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.

Maybe I misunderstood your answer or my suggestion wasn't clear. What
is the problem with something like the following (untested)?

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bb327da..457038a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -635,6 +635,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags,
     if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
         goto fail;
 
+    d->arch.p2m.last_vcpu_ran = xmalloc_array(uint8_t, num_possible_cpus());
     return 0;
 
 fail:
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0899523..606a875 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -96,6 +96,8 @@ struct p2m_domain {
 
     /* back pointer to domain */
     struct domain *domain;
+
+    uint8_t *last_vcpu_ran;
 };
 
 /*


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