Re: [RFC PATCH v2 06/23] arm64/sve: Check SVE virtualisability

2018-11-16 Thread Alex Bennée 

Dave Martin  writes:

> On Thu, Nov 15, 2018 at 03:39:01PM +, Alex Bennée wrote:
>>
>> Dave Martin  writes:
>>
>> > Due to the way the effective SVE vector length is controlled and
>> > trapped at different exception levels, certain mismatches in the
>> > sets of vector lengths supported by different physical CPUs in the
>> > system may prevent straightforward virtualisation of SVE at parity
>> > with the host.
>> >
>> > This patch analyses the extent to which SVE can be virtualised
>> > safely without interfering with migration of vcpus between physical
>> > CPUs, and rejects late secondary CPUs that would erode the
>> > situation further.
>> >
>> > It is left up to KVM to decide what to do with this information.
>> >
>> > Signed-off-by: Dave Martin 
>> > ---
>> >
>> > Changes since RFCv1:
>> >
>> >  * The analysis done by this patch is the same as in the previous
>> >version, but the commit message the printks etc. have been reworded
>> >to avoid the suggestion that KVM is expected to work on a system with
>> >mismatched SVE implementations.
>> > ---
>> >  arch/arm64/include/asm/fpsimd.h |  1 +
>> >  arch/arm64/kernel/cpufeature.c  |  2 +-
>> >  arch/arm64/kernel/fpsimd.c  | 87 
>> > +++--
>> >  3 files changed, 76 insertions(+), 14 deletions(-)
>> >
>
> [...]
>
>> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>
> [...]
>
>> > @@ -623,11 +629,8 @@ int sve_get_current_vl(void)
>
> [...]
>
>> > +/* Bitmaps for temporary storage during manipulation of vector length 
>> > sets */
>> > +static DECLARE_BITMAP(sve_tmp_vq_map, SVE_VQ_MAX);
>>
>> This seems odd as a local global, why not declared locally when used?
>
> Could do.
>
> My original concern was that this is "big" and therefore it's impolite
> to allocate it on the stack.
>
> But on reflection, 64 bytes of stack is no big deal for a 64-bit
> architecture.  The affected functions probably spill more than than
> already, and these functions are called on well-defined paths which
> shouldn't have super-deep stacks already.
>
> [...]
>
>> > @@ -658,24 +662,60 @@ void __init sve_init_vq_map(void)
>> >   */
>> >  void sve_update_vq_map(void)
>> >  {
>> > -  sve_probe_vqs(sve_secondary_vq_map);
>> > -  bitmap_and(sve_vq_map, sve_vq_map, sve_secondary_vq_map, SVE_VQ_MAX);
>> > +  sve_probe_vqs(sve_tmp_vq_map);
>> > +  bitmap_and(sve_vq_map, sve_vq_map, sve_tmp_vq_map,
>> > + SVE_VQ_MAX);
>> > +  bitmap_or(sve_vq_partial_map, sve_vq_partial_map, sve_tmp_vq_map,
>> > +SVE_VQ_MAX);
>> >  }
>>
>> I'm not quite following what's going on here. This is tracking both the
>> vector lengths available on all CPUs and the ones available on at least
>> one CPU? This raises a some questions:
>>
>>   - do such franken-machines exist or are expected to exit?
>
> no, and yes respectively (Linux does not endorse the latter for now,
> since it results in a non-SMP system: we hide the asymmetries where
> possible by clamping the set of available vector lengths, but for
> KVM it's too hard and we don't aim to support it at all).
>
> Even if we don't recommend deploying a general-purpose OS on such a
> system, people will eventually try it.  So it's better to fail safe
> rather than silently doing the wrong thing.
>
>>   - how do we ensure this is always upto date?
>
> This gets updated for each early secondary CPU that comes up.  (Early
> secondaries' boot is serialised, so we shouldn't have to worry about
> races here.)
>
> The configuration is frozen by the time we enter userspace (hence
> __ro_after_init).
>
> Once all the early secondaries have come up, we commit to the best
> possible set of vector lengths for the CPUs that we know about, and we
> don't call this path any more: instead, each late secondary goes into
> sve_verify_vq_map() instead to check that those CPUs are compatible
> with the configuration we committed to.
>
> For context, take a look at
> arch/arm64/kernel/cpufeature.c:check_local_cpu_capabilities(), which is
> the common entry point for all secondary CPUs: that splits into
> update_cpu_capabilities() and verify_local_cpu_capabilities() paths for
> the two cases described above, calling down into sve_update_vq_map()
> and sve_verify_vq_map() as appropriate.
>
>>   - what happens when we hotplug a new CPU with less available VQ?
>
> We reject the CPU and throw it back to the firmware (see
> cpufeature.c:verify_sve_features()).
>
> This follows the precedent already set in verify_local_cpu_capabilities()
> etc.

I think a few words to that effect in the function comments would be
helpful:

  /*
   * sve_update_vq_map only cares about CPUs at boot time and is called
   * serially for each one. Any CPUs added later via hotplug will fail
   * at sve_verify_vq_map if they don't match what is detected here.
   */

>
>>
>> >
>> >  /* Check whether the current CPU supports all VQs in the committed set */
>> >  int sve_verify_vq_map(void)
>> >  {
>> > -  int

Re: [RFC PATCH v2 06/23] arm64/sve: Check SVE virtualisability

2018-11-15 Thread Dave Martin 
On Thu, Nov 15, 2018 at 03:39:01PM +, Alex Bennée wrote:
> 
> Dave Martin  writes:
> 
> > Due to the way the effective SVE vector length is controlled and
> > trapped at different exception levels, certain mismatches in the
> > sets of vector lengths supported by different physical CPUs in the
> > system may prevent straightforward virtualisation of SVE at parity
> > with the host.
> >
> > This patch analyses the extent to which SVE can be virtualised
> > safely without interfering with migration of vcpus between physical
> > CPUs, and rejects late secondary CPUs that would erode the
> > situation further.
> >
> > It is left up to KVM to decide what to do with this information.
> >
> > Signed-off-by: Dave Martin 
> > ---
> >
> > Changes since RFCv1:
> >
> >  * The analysis done by this patch is the same as in the previous
> >version, but the commit message the printks etc. have been reworded
> >to avoid the suggestion that KVM is expected to work on a system with
> >mismatched SVE implementations.
> > ---
> >  arch/arm64/include/asm/fpsimd.h |  1 +
> >  arch/arm64/kernel/cpufeature.c  |  2 +-
> >  arch/arm64/kernel/fpsimd.c  | 87 
> > +++--
> >  3 files changed, 76 insertions(+), 14 deletions(-)
> >

[...]

> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c

[...]

> > @@ -623,11 +629,8 @@ int sve_get_current_vl(void)

[...]

> > +/* Bitmaps for temporary storage during manipulation of vector length sets 
> > */
> > +static DECLARE_BITMAP(sve_tmp_vq_map, SVE_VQ_MAX);
> 
> This seems odd as a local global, why not declared locally when used?

Could do.

My original concern was that this is "big" and therefore it's impolite
to allocate it on the stack.

But on reflection, 64 bytes of stack is no big deal for a 64-bit
architecture.  The affected functions probably spill more than than
already, and these functions are called on well-defined paths which
shouldn't have super-deep stacks already.

[...]

> > @@ -658,24 +662,60 @@ void __init sve_init_vq_map(void)
> >   */
> >  void sve_update_vq_map(void)
> >  {
> > -   sve_probe_vqs(sve_secondary_vq_map);
> > -   bitmap_and(sve_vq_map, sve_vq_map, sve_secondary_vq_map, SVE_VQ_MAX);
> > +   sve_probe_vqs(sve_tmp_vq_map);
> > +   bitmap_and(sve_vq_map, sve_vq_map, sve_tmp_vq_map,
> > +  SVE_VQ_MAX);
> > +   bitmap_or(sve_vq_partial_map, sve_vq_partial_map, sve_tmp_vq_map,
> > + SVE_VQ_MAX);
> >  }
> 
> I'm not quite following what's going on here. This is tracking both the
> vector lengths available on all CPUs and the ones available on at least
> one CPU? This raises a some questions:
> 
>   - do such franken-machines exist or are expected to exit?

no, and yes respectively (Linux does not endorse the latter for now,
since it results in a non-SMP system: we hide the asymmetries where
possible by clamping the set of available vector lengths, but for
KVM it's too hard and we don't aim to support it at all).

Even if we don't recommend deploying a general-purpose OS on such a
system, people will eventually try it.  So it's better to fail safe
rather than silently doing the wrong thing.

>   - how do we ensure this is always upto date?

This gets updated for each early secondary CPU that comes up.  (Early
secondaries' boot is serialised, so we shouldn't have to worry about
races here.)

The configuration is frozen by the time we enter userspace (hence
__ro_after_init).

Once all the early secondaries have come up, we commit to the best
possible set of vector lengths for the CPUs that we know about, and we
don't call this path any more: instead, each late secondary goes into
sve_verify_vq_map() instead to check that those CPUs are compatible
with the configuration we committed to.

For context, take a look at
arch/arm64/kernel/cpufeature.c:check_local_cpu_capabilities(), which is
the common entry point for all secondary CPUs: that splits into
update_cpu_capabilities() and verify_local_cpu_capabilities() paths for
the two cases described above, calling down into sve_update_vq_map()
and sve_verify_vq_map() as appropriate.

>   - what happens when we hotplug a new CPU with less available VQ?

We reject the CPU and throw it back to the firmware (see
cpufeature.c:verify_sve_features()).

This follows the precedent already set in verify_local_cpu_capabilities()
etc.

> 
> >
> >  /* Check whether the current CPU supports all VQs in the committed set */
> >  int sve_verify_vq_map(void)
> >  {
> > -   int ret = 0;
> > +   int ret = -EINVAL;
> > +   unsigned long b;
> >
> > -   sve_probe_vqs(sve_secondary_vq_map);
> > -   bitmap_andnot(sve_secondary_vq_map, sve_vq_map, sve_secondary_vq_map,
> > - SVE_VQ_MAX);
> > -   if (!bitmap_empty(sve_secondary_vq_map, SVE_VQ_MAX)) {
> > +   sve_probe_vqs(sve_tmp_vq_map);
> > +
> > +   bitmap_complement(sve_tmp_vq_map, sve_tmp_vq_map, SVE_VQ_MAX);
> > +   if (bitmap_intersects(sve_tmp_vq_map, sve_vq_map, SVE_VQ_MAX)) {
> >

Re: [RFC PATCH v2 06/23] arm64/sve: Check SVE virtualisability

2018-11-15 Thread Alex Bennée 

Dave Martin  writes:

> Due to the way the effective SVE vector length is controlled and
> trapped at different exception levels, certain mismatches in the
> sets of vector lengths supported by different physical CPUs in the
> system may prevent straightforward virtualisation of SVE at parity
> with the host.
>
> This patch analyses the extent to which SVE can be virtualised
> safely without interfering with migration of vcpus between physical
> CPUs, and rejects late secondary CPUs that would erode the
> situation further.
>
> It is left up to KVM to decide what to do with this information.
>
> Signed-off-by: Dave Martin 
> ---
>
> Changes since RFCv1:
>
>  * The analysis done by this patch is the same as in the previous
>version, but the commit message the printks etc. have been reworded
>to avoid the suggestion that KVM is expected to work on a system with
>mismatched SVE implementations.
> ---
>  arch/arm64/include/asm/fpsimd.h |  1 +
>  arch/arm64/kernel/cpufeature.c  |  2 +-
>  arch/arm64/kernel/fpsimd.c  | 87 
> +++--
>  3 files changed, 76 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index dd1ad39..964adc9 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -87,6 +87,7 @@ extern void sve_kernel_enable(const struct 
> arm64_cpu_capabilities *__unused);
>  extern u64 read_zcr_features(void);
>
>  extern int __ro_after_init sve_max_vl;
> +extern int __ro_after_init sve_max_virtualisable_vl;
>
>  #ifdef CONFIG_ARM64_SVE
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index e238b79..aa1a55b 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1531,7 +1531,7 @@ static void verify_sve_features(void)
>   unsigned int len = zcr & ZCR_ELx_LEN_MASK;
>
>   if (len < safe_len || sve_verify_vq_map()) {
> - pr_crit("CPU%d: SVE: required vector length(s) missing\n",
> + pr_crit("CPU%d: SVE: vector length support mismatch\n",
>   smp_processor_id());
>   cpu_die_early();
>   }
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 42aa154..d28042b 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -18,6 +18,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -48,6 +49,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define FPEXC_IOF(1 << 0)
>  #define FPEXC_DZF(1 << 1)
> @@ -130,14 +132,18 @@ static int sve_default_vl = -1;
>
>  /* Maximum supported vector length across all CPUs (initially poisoned) */
>  int __ro_after_init sve_max_vl = SVE_VL_MIN;
> +int __ro_after_init sve_max_virtualisable_vl = SVE_VL_MIN;
>  /* Set of available vector lengths, as vq_to_bit(vq): */
>  static __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
> +/* Set of vector lengths present on at least one cpu: */
> +static __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
>  static void __percpu *efi_sve_state;
>
>  #else /* ! CONFIG_ARM64_SVE */
>
>  /* Dummy declaration for code that will be optimised out: */
>  extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
> +extern __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
>  extern void __percpu *efi_sve_state;
>
>  #endif /* ! CONFIG_ARM64_SVE */
> @@ -623,11 +629,8 @@ int sve_get_current_vl(void)
>   return sve_prctl_status(0);
>  }
>
> -/*
> - * Bitmap for temporary storage of the per-CPU set of supported vector 
> lengths
> - * during secondary boot.
> - */
> -static DECLARE_BITMAP(sve_secondary_vq_map, SVE_VQ_MAX);
> +/* Bitmaps for temporary storage during manipulation of vector length sets */
> +static DECLARE_BITMAP(sve_tmp_vq_map, SVE_VQ_MAX);

This seems odd as a local global, why not declared locally when used?

>
>  static void sve_probe_vqs(DECLARE_BITMAP(map, SVE_VQ_MAX))
>  {
> @@ -650,6 +653,7 @@ static void sve_probe_vqs(DECLARE_BITMAP(map, SVE_VQ_MAX))
>  void __init sve_init_vq_map(void)
>  {
>   sve_probe_vqs(sve_vq_map);
> + bitmap_copy(sve_vq_partial_map, sve_vq_map, SVE_VQ_MAX);
>  }
>
>  /*
> @@ -658,24 +662,60 @@ void __init sve_init_vq_map(void)
>   */
>  void sve_update_vq_map(void)
>  {
> - sve_probe_vqs(sve_secondary_vq_map);
> - bitmap_and(sve_vq_map, sve_vq_map, sve_secondary_vq_map, SVE_VQ_MAX);
> + sve_probe_vqs(sve_tmp_vq_map);
> + bitmap_and(sve_vq_map, sve_vq_map, sve_tmp_vq_map,
> +SVE_VQ_MAX);
> + bitmap_or(sve_vq_partial_map, sve_vq_partial_map, sve_tmp_vq_map,
> +   SVE_VQ_MAX);
>  }

I'm not quite following what's going on here. This is tracking both the
vector lengths available on all CPUs and the ones available on at least
one CPU? This raises a some questions:

  - do such franken-machines exist or are expected to exit?
  - how do we

[RFC PATCH v2 06/23] arm64/sve: Check SVE virtualisability

2018-09-28 Thread Dave Martin 
Due to the way the effective SVE vector length is controlled and
trapped at different exception levels, certain mismatches in the
sets of vector lengths supported by different physical CPUs in the
system may prevent straightforward virtualisation of SVE at parity
with the host.

This patch analyses the extent to which SVE can be virtualised
safely without interfering with migration of vcpus between physical
CPUs, and rejects late secondary CPUs that would erode the
situation further.

It is left up to KVM to decide what to do with this information.

Signed-off-by: Dave Martin 
---

Changes since RFCv1:

 * The analysis done by this patch is the same as in the previous
   version, but the commit message the printks etc. have been reworded
   to avoid the suggestion that KVM is expected to work on a system with
   mismatched SVE implementations.
---
 arch/arm64/include/asm/fpsimd.h |  1 +
 arch/arm64/kernel/cpufeature.c  |  2 +-
 arch/arm64/kernel/fpsimd.c  | 87 +++--
 3 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index dd1ad39..964adc9 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -87,6 +87,7 @@ extern void sve_kernel_enable(const struct 
arm64_cpu_capabilities *__unused);
 extern u64 read_zcr_features(void);
 
 extern int __ro_after_init sve_max_vl;
+extern int __ro_after_init sve_max_virtualisable_vl;
 
 #ifdef CONFIG_ARM64_SVE
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index e238b79..aa1a55b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1531,7 +1531,7 @@ static void verify_sve_features(void)
unsigned int len = zcr & ZCR_ELx_LEN_MASK;
 
if (len < safe_len || sve_verify_vq_map()) {
-   pr_crit("CPU%d: SVE: required vector length(s) missing\n",
+   pr_crit("CPU%d: SVE: vector length support mismatch\n",
smp_processor_id());
cpu_die_early();
}
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 42aa154..d28042b 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -18,6 +18,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -48,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define FPEXC_IOF  (1 << 0)
 #define FPEXC_DZF  (1 << 1)
@@ -130,14 +132,18 @@ static int sve_default_vl = -1;
 
 /* Maximum supported vector length across all CPUs (initially poisoned) */
 int __ro_after_init sve_max_vl = SVE_VL_MIN;
+int __ro_after_init sve_max_virtualisable_vl = SVE_VL_MIN;
 /* Set of available vector lengths, as vq_to_bit(vq): */
 static __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
+/* Set of vector lengths present on at least one cpu: */
+static __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
 static void __percpu *efi_sve_state;
 
 #else /* ! CONFIG_ARM64_SVE */
 
 /* Dummy declaration for code that will be optimised out: */
 extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
+extern __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
 extern void __percpu *efi_sve_state;
 
 #endif /* ! CONFIG_ARM64_SVE */
@@ -623,11 +629,8 @@ int sve_get_current_vl(void)
return sve_prctl_status(0);
 }
 
-/*
- * Bitmap for temporary storage of the per-CPU set of supported vector lengths
- * during secondary boot.
- */
-static DECLARE_BITMAP(sve_secondary_vq_map, SVE_VQ_MAX);
+/* Bitmaps for temporary storage during manipulation of vector length sets */
+static DECLARE_BITMAP(sve_tmp_vq_map, SVE_VQ_MAX);
 
 static void sve_probe_vqs(DECLARE_BITMAP(map, SVE_VQ_MAX))
 {
@@ -650,6 +653,7 @@ static void sve_probe_vqs(DECLARE_BITMAP(map, SVE_VQ_MAX))
 void __init sve_init_vq_map(void)
 {
sve_probe_vqs(sve_vq_map);
+   bitmap_copy(sve_vq_partial_map, sve_vq_map, SVE_VQ_MAX);
 }
 
 /*
@@ -658,24 +662,60 @@ void __init sve_init_vq_map(void)
  */
 void sve_update_vq_map(void)
 {
-   sve_probe_vqs(sve_secondary_vq_map);
-   bitmap_and(sve_vq_map, sve_vq_map, sve_secondary_vq_map, SVE_VQ_MAX);
+   sve_probe_vqs(sve_tmp_vq_map);
+   bitmap_and(sve_vq_map, sve_vq_map, sve_tmp_vq_map,
+  SVE_VQ_MAX);
+   bitmap_or(sve_vq_partial_map, sve_vq_partial_map, sve_tmp_vq_map,
+ SVE_VQ_MAX);
 }
 
 /* Check whether the current CPU supports all VQs in the committed set */
 int sve_verify_vq_map(void)
 {
-   int ret = 0;
+   int ret = -EINVAL;
+   unsigned long b;
 
-   sve_probe_vqs(sve_secondary_vq_map);
-   bitmap_andnot(sve_secondary_vq_map, sve_vq_map, sve_secondary_vq_map,
- SVE_VQ_MAX);
-   if (!bitmap_empty(sve_secondary_vq_map, SVE_VQ_MAX)) {
+   sve_probe_vqs(sve_tmp_vq_map);
+
+   bitmap_complement(sve_tmp_vq_map, sve_tmp_vq_map, SVE_VQ_MAX);
+   if