[PATCH 0/3] xen/arm: p2m: Perform local invalidation on vCPU migration

[PATCH 0/3] xen/arm: p2m: Perform local invalidation on vCPU migration

[Julien Grall]

Hi all,

This patch series is fixing a potential bug when multiple vCPU of a guest is
running on the same pCPU. See patch #3 for all the details.

Regards,

Julien Grall (3):
  xen/arm: hvm_domain does not need to be cacheline aligned
  xen/arm: Introduce INVALID_VCPU_ID
  xen/arm: p2m: Perform local TLB invalidation on vCPU migration

 xen/arch/arm/p2m.c           | 24 ++++++++++++++++++++++++
 xen/include/asm-arm/config.h |  2 ++
 xen/include/asm-arm/domain.h |  2 +-
 xen/include/asm-arm/p2m.h    |  3 +++
 4 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.11.0


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

[PATCH 1/3] xen/arm: hvm_domain does not need to be cacheline aligned

[Julien Grall]

hvm_domain only contains the HVM_PARAM that on ARM are not used often.
So it is not necessary to have hvm_domain fitting in a cacheline. Drop
it to save 128 bytes in the structure arch_domain.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/domain.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index f117e577f0..09fe502fc2 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -14,7 +14,7 @@
 struct hvm_domain
 {
     uint64_t              params[HVM_NR_PARAMS];
-}  __cacheline_aligned;
+};
 
 #ifdef CONFIG_ARM_64
 enum domain_type {
-- 
2.11.0


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

Re: [PATCH 1/3] xen/arm: hvm_domain does not need to be cacheline aligned

[Stefano Stabellini]

On Wed, 8 Mar 2017, Julien Grall wrote:
> hvm_domain only contains the HVM_PARAM that on ARM are not used often.
> So it is not necessary to have hvm_domain fitting in a cacheline. Drop
> it to save 128 bytes in the structure arch_domain.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/include/asm-arm/domain.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index f117e577f0..09fe502fc2 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -14,7 +14,7 @@
>  struct hvm_domain
>  {
>      uint64_t              params[HVM_NR_PARAMS];
> -}  __cacheline_aligned;
> +};
>  
>  #ifdef CONFIG_ARM_64
>  enum domain_type {
> -- 
> 2.11.0
> 

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

[PATCH 2/3] xen/arm: Introduce INVALID_VCPU_ID

[Julien Grall]

Define INVALID_VCPU_ID as MAX_VIRT_CPUS to avoid casting problem later
on. At the moment it can always fit in uint8_t.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/config.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index ba61f65a39..6a92f53f72 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -46,6 +46,8 @@
 #define MAX_VIRT_CPUS 8
 #endif
 
+#define INVALID_VCPU_ID MAX_VIRT_CPUS
+
 #define asmlinkage /* Nothing needed */
 
 #define __LINUX_ARM_ARCH__ 7
-- 
2.11.0


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

Re: [PATCH 2/3] xen/arm: Introduce INVALID_VCPU_ID

[Stefano Stabellini]

On Wed, 8 Mar 2017, Julien Grall wrote:
> Define INVALID_VCPU_ID as MAX_VIRT_CPUS to avoid casting problem later
> on. At the moment it can always fit in uint8_t.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/include/asm-arm/config.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index ba61f65a39..6a92f53f72 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -46,6 +46,8 @@
>  #define MAX_VIRT_CPUS 8
>  #endif
>  
> +#define INVALID_VCPU_ID MAX_VIRT_CPUS
> +
>  #define asmlinkage /* Nothing needed */
>  
>  #define __LINUX_ARM_ARCH__ 7
> -- 
> 2.11.0
> 

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

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

[Julien Grall]

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.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This patch is a candidate for backport to Xen 4.8, 4.7 and 4.6.
---
 xen/arch/arm/p2m.c        | 24 ++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h |  3 +++
 2 files changed, 27 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1fc6ca3bb2..626376090d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -130,6 +130,7 @@ void p2m_restore_state(struct vcpu *n)
 {
     register_t hcr;
     struct p2m_domain *p2m = &n->domain->arch.p2m;
+    uint8_t *last_vcpu_ran;
 
     if ( is_idle_vcpu(n) )
         return;
@@ -149,6 +150,17 @@ void p2m_restore_state(struct vcpu *n)
 
     WRITE_SYSREG(hcr, HCR_EL2);
     isb();
+
+    last_vcpu_ran = &p2m->last_vcpu_ran[smp_processor_id()];
+
+    /*
+     * Flush local TLB for the domain to prevent wrong TLB translation
+     * when running multiple vCPU of the same domain on a single pCPU.
+     */
+    if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
+        flush_tlb_local();
+
+    *last_vcpu_ran = n->vcpu_id;
 }
 
 static void p2m_flush_tlb(struct p2m_domain *p2m)
@@ -1247,6 +1259,7 @@ int p2m_init(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
     int rc = 0;
+    unsigned int cpu;
 
     rwlock_init(&p2m->lock);
     INIT_PAGE_LIST_HEAD(&p2m->pages);
@@ -1275,6 +1288,17 @@ int p2m_init(struct domain *d)
 
     rc = p2m_alloc_table(d);
 
+    /*
+     * Make sure that the type chosen to is able to store the an vCPU ID
+     * between 0 and the maximum of virtual CPUS supported as long as
+     * the INVALID_VCPU_ID.
+     */
+    BUILD_BUG_ON((1 << (sizeof(p2m->last_vcpu_ran[0]) * 8)) < MAX_VIRT_CPUS);
+    BUILD_BUG_ON((1 << (sizeof(p2m->last_vcpu_ran[0])* 8)) < INVALID_VCPU_ID);
+
+    for_each_possible_cpu(cpu)
+       p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID;
+
     return rc;
 }
 
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];
 };
 
 /*
-- 
2.11.0


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

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

[Stefano Stabellini]

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?

Also, I have one suggestion below.


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This patch is a candidate for backport to Xen 4.8, 4.7 and 4.6.
> ---
>  xen/arch/arm/p2m.c        | 24 ++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h |  3 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 1fc6ca3bb2..626376090d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -130,6 +130,7 @@ void p2m_restore_state(struct vcpu *n)
>  {
>      register_t hcr;
>      struct p2m_domain *p2m = &n->domain->arch.p2m;
> +    uint8_t *last_vcpu_ran;
>  
>      if ( is_idle_vcpu(n) )
>          return;
> @@ -149,6 +150,17 @@ void p2m_restore_state(struct vcpu *n)
>  
>      WRITE_SYSREG(hcr, HCR_EL2);
>      isb();
> +
> +    last_vcpu_ran = &p2m->last_vcpu_ran[smp_processor_id()];
> +
> +    /*
> +     * Flush local TLB for the domain to prevent wrong TLB translation
> +     * when running multiple vCPU of the same domain on a single pCPU.
> +     */
> +    if ( *last_vcpu_ran != INVALID_VCPU_ID && *last_vcpu_ran != n->vcpu_id )
> +        flush_tlb_local();
> +
> +    *last_vcpu_ran = n->vcpu_id;
>  }
>  
>  static void p2m_flush_tlb(struct p2m_domain *p2m)
> @@ -1247,6 +1259,7 @@ int p2m_init(struct domain *d)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
>      int rc = 0;
> +    unsigned int cpu;
>  
>      rwlock_init(&p2m->lock);
>      INIT_PAGE_LIST_HEAD(&p2m->pages);
> @@ -1275,6 +1288,17 @@ int p2m_init(struct domain *d)
>  
>      rc = p2m_alloc_table(d);
>  
> +    /*
> +     * Make sure that the type chosen to is able to store the an vCPU ID
> +     * between 0 and the maximum of virtual CPUS supported as long as
> +     * the INVALID_VCPU_ID.
> +     */
> +    BUILD_BUG_ON((1 << (sizeof(p2m->last_vcpu_ran[0]) * 8)) < MAX_VIRT_CPUS);
> +    BUILD_BUG_ON((1 << (sizeof(p2m->last_vcpu_ran[0])* 8)) < INVALID_VCPU_ID);
> +
> +    for_each_possible_cpu(cpu)
> +       p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID;
> +
>      return rc;
>  }
>  
> 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.

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

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

[Julien Grall]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

On 08/03/2017 19:48, Stefano Stabellini wrote:
> 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)?

The problem is you allocate more memory than my current solution. As 
your code below clearly show it, p2m_domain is directly integrated in 
arch_domain.

The structure domain can never be bigger than PAGE_SIZE (see 
alloc_domanin_struct) and we will always allocate a page no matter the 
size of the structure domain. This patch is just filling hole in the page.

So why would we want to switch to something the increase the memory impact?

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Wed, 8 Mar 2017, Julien Grall wrote:
> On 08/03/2017 19:48, Stefano Stabellini wrote:
> > 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)?
> 
> The problem is you allocate more memory than my current solution. As your code
> below clearly show it, p2m_domain is directly integrated in arch_domain.
> 
> The structure domain can never be bigger than PAGE_SIZE (see
> alloc_domanin_struct) and we will always allocate a page no matter the size of
> the structure domain. This patch is just filling hole in the page.
> 
> So why would we want to switch to something the increase the memory impact?

I see your point, it is a good idea to use the remaining space in struct
domain. The only issue is that a user could set CONFIG_NR_CPUS large
enough to push last_vcpu_ran beyond the page boundary. I think we need
to add a BUILD_BUG_ON(sizeof(struct domain) >= PAGE_SIZE).

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

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

[Julien Grall]

Hi Stefano,

On 03/14/2017 10:39 PM, Stefano Stabellini wrote:
> On Wed, 8 Mar 2017, Julien Grall wrote:
>> On 08/03/2017 19:48, Stefano Stabellini wrote:
>>> On Wed, 8 Mar 2017, Julien Grall wrote:
> I see your point, it is a good idea to use the remaining space in struct
> domain. The only issue is that a user could set CONFIG_NR_CPUS large
> enough to push last_vcpu_ran beyond the page boundary. I think we need
> to add a BUILD_BUG_ON(sizeof(struct domain) >= PAGE_SIZE).

There is already a BUILD_BUG_ON(sizeof(struct domain) > PAGE_SIZE) in 
alloc_domain_struct. Would it be enough for you?

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Tue, 14 Mar 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/14/2017 10:39 PM, Stefano Stabellini wrote:
> > On Wed, 8 Mar 2017, Julien Grall wrote:
> > > On 08/03/2017 19:48, Stefano Stabellini wrote:
> > > > On Wed, 8 Mar 2017, Julien Grall wrote:
> > I see your point, it is a good idea to use the remaining space in struct
> > domain. The only issue is that a user could set CONFIG_NR_CPUS large
> > enough to push last_vcpu_ran beyond the page boundary. I think we need
> > to add a BUILD_BUG_ON(sizeof(struct domain) >= PAGE_SIZE).
> 
> There is already a BUILD_BUG_ON(sizeof(struct domain) > PAGE_SIZE) in
> alloc_domain_struct. Would it be enough for you?

Yeah, we don't need two :)

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

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

[Julien Grall]

On 08/03/2017 19:48, Stefano Stabellini wrote:
> 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)?

The problem is you allocate more memory than my current solution. As 
your code below clearly show it, p2m_domain is directly integrated in 
arch_domain.

The structure domain can never be bigger than PAGE_SIZE (see 
alloc_domanin_struct) and we will always allocate a page no matter the 
size of the structure domain. This patch is just filling hole in the page.

So why would we want to switch to something that increase the memory impact?

Cheers,

-- 
Julien Grall

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