Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721893 > unrolled thread

Re: [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous systems from using GICv4

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous  systems from using GICv4 Christoffer Dall <cdall@linaro.org> - 2017-08-28 20:40 +0200
    Re: [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous  systems from using GICv4 Marc Zyngier <marc.zyngier@arm.com> - 2017-08-30 18:10 +0200
      Re: [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous  systems from using GICv4 Christoffer Dall <cdall@linaro.org> - 2017-08-30 23:10 +0200

#1721893 — Re: [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous systems from using GICv4

On Mon, Jul 31, 2017 at 06:26:34PM +0100, Marc Zyngier wrote:
> The GICv4 architecture doesn't prevent CPUs implementing GICv4 to
> cohabit with CPUs limited to GICv3 in the same system.
> 
> This is mad (the sheduler would have to be made aware of the v4

*scheduler

> capability), and we're certainly not going to support this any
> time soon. So let's check that all online CPUs are GICv4 capable,
> and disable the functionnality if not.

*functionality

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>  virt/kvm/arm/vgic/vgic-v3.c        | 22 +++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 1ea576c8126f..dfa4a51643d6 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -532,6 +532,8 @@
>  #define ICH_VTR_SEIS_MASK		(1 << ICH_VTR_SEIS_SHIFT)
>  #define ICH_VTR_A3V_SHIFT		21
>  #define ICH_VTR_A3V_MASK		(1 << ICH_VTR_A3V_SHIFT)
> +#define ICH_VTR_nV4_SHIFT		20
> +#define ICH_VTR_nV4_MASK		(1 << ICH_VTR_nV4_SHIFT)
>  
>  #define ICC_IAR1_EL1_SPURIOUS		0x3ff
>  
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 405733678c2f..252268e83ade 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -466,6 +466,18 @@ static int __init early_gicv4_enable(char *buf)
>  }
>  early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
>  
> +static void vgic_check_v4_cpuif(void *param)
> +{
> +	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
> +	bool *v4 = param, this_cpu_is_v4;
> +
> +	this_cpu_is_v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
> +	if (!this_cpu_is_v4)
> +		kvm_info("CPU%d is not GICv4 capable\n", smp_processor_id());
> +
> +	*v4 &= this_cpu_is_v4;

nit: You could make this function look slightly less scary by declaring
this_cpu_is_v4 on a separate line and not use a bitwise operator on a
boolean type.  Actually, having a function called 'check' with a side
effect is not the nicest thing either, so why not just return what this
particular CPU does and do the comparison in the calling loop.

#bikeshedding


Thanks,
-Christoffer



> +}
> +
>  /**
>   * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
>   * @node:	pointer to the DT node
> @@ -485,8 +497,16 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>  	kvm_vgic_global_state.can_emulate_gicv2 = false;
>  	kvm_vgic_global_state.ich_vtr_el2 = ich_vtr_el2;
>  
> -	/* GICv4 support? */
> +	/*
> +	 * GICv4 support? We need to check on all CPUs in case of some
> +	 * extremely creative form of big-little brain damage...
> +	 */
>  	if (info->has_v4) {
> +		int cpu;
> +
> +		for_each_online_cpu(cpu)
> +			smp_call_function_single(cpu, vgic_check_v4_cpuif,
> +						 &gicv4_enable, 1);
>  		kvm_vgic_global_state.has_gicv4 = gicv4_enable;
>  		kvm_info("GICv4 support %sabled\n",
>  			 gicv4_enable ? "en" : "dis");
> -- 
> 2.11.0
> 

[toc] | [next] | [standalone]

#1723478

On 28/08/17 19:35, Christoffer Dall wrote:
> On Mon, Jul 31, 2017 at 06:26:34PM +0100, Marc Zyngier wrote:
>> The GICv4 architecture doesn't prevent CPUs implementing GICv4 to
>> cohabit with CPUs limited to GICv3 in the same system.
>>
>> This is mad (the sheduler would have to be made aware of the v4
> 
> *scheduler
> 
>> capability), and we're certainly not going to support this any
>> time soon. So let's check that all online CPUs are GICv4 capable,
>> and disable the functionnality if not.
> 
> *functionality
> 
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>>  virt/kvm/arm/vgic/vgic-v3.c        | 22 +++++++++++++++++++++-
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 1ea576c8126f..dfa4a51643d6 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -532,6 +532,8 @@
>>  #define ICH_VTR_SEIS_MASK		(1 << ICH_VTR_SEIS_SHIFT)
>>  #define ICH_VTR_A3V_SHIFT		21
>>  #define ICH_VTR_A3V_MASK		(1 << ICH_VTR_A3V_SHIFT)
>> +#define ICH_VTR_nV4_SHIFT		20
>> +#define ICH_VTR_nV4_MASK		(1 << ICH_VTR_nV4_SHIFT)
>>  
>>  #define ICC_IAR1_EL1_SPURIOUS		0x3ff
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 405733678c2f..252268e83ade 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -466,6 +466,18 @@ static int __init early_gicv4_enable(char *buf)
>>  }
>>  early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
>>  
>> +static void vgic_check_v4_cpuif(void *param)
>> +{
>> +	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>> +	bool *v4 = param, this_cpu_is_v4;
>> +
>> +	this_cpu_is_v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
>> +	if (!this_cpu_is_v4)
>> +		kvm_info("CPU%d is not GICv4 capable\n", smp_processor_id());
>> +
>> +	*v4 &= this_cpu_is_v4;
> 
> nit: You could make this function look slightly less scary by declaring
> this_cpu_is_v4 on a separate line and not use a bitwise operator on a
> boolean type.  Actually, having a function called 'check' with a side
> effect is not the nicest thing either, so why not just return what this
> particular CPU does and do the comparison in the calling loop.

Fair enough.

> #bikeshedding

How about this on top:

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 01eaf4ee2f63..e471750dc0a1 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -469,18 +469,16 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
 static void vgic_check_v4_cpuif(void *param)
 {
 	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
-	bool *v4 = param, this_cpu_is_v4;
+	bool *v4 = param;
 
-	this_cpu_is_v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
-	if (!this_cpu_is_v4)
+	*v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
+	if (!*v4)
 		kvm_info("CPU%d is not GICv4 capable\n", smp_processor_id());
-
-	*v4 &= this_cpu_is_v4;
 }
 
 /**
  * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
- * @node:	pointer to the DT node
+ * @info:	pointer to the firmware-agnostic GIC information
  *
  * Returns 0 if a GICv3 has been found, returns an error code otherwise
  */
@@ -504,9 +502,14 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
 	if (info->has_v4) {
 		int cpu;
 
-		for_each_online_cpu(cpu)
+		for_each_online_cpu(cpu) {
+			bool enable;
+
 			smp_call_function_single(cpu, vgic_check_v4_cpuif,
-						 &gicv4_enable, 1);
+						 &enable, 1);
+			gicv4_enable = gicv4_enable && enable;
+		}
+
 		kvm_vgic_global_state.has_gicv4 = gicv4_enable;
 		kvm_info("GICv4 support %sabled\n",
 			 gicv4_enable ? "en" : "dis");

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1723666

On Wed, Aug 30, 2017 at 05:03:51PM +0100, Marc Zyngier wrote:
> On 28/08/17 19:35, Christoffer Dall wrote:
> > On Mon, Jul 31, 2017 at 06:26:34PM +0100, Marc Zyngier wrote:
> >> The GICv4 architecture doesn't prevent CPUs implementing GICv4 to
> >> cohabit with CPUs limited to GICv3 in the same system.
> >>
> >> This is mad (the sheduler would have to be made aware of the v4
> > 
> > *scheduler
> > 
> >> capability), and we're certainly not going to support this any
> >> time soon. So let's check that all online CPUs are GICv4 capable,
> >> and disable the functionnality if not.
> > 
> > *functionality
> > 
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  include/linux/irqchip/arm-gic-v3.h |  2 ++
> >>  virt/kvm/arm/vgic/vgic-v3.c        | 22 +++++++++++++++++++++-
> >>  2 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> >> index 1ea576c8126f..dfa4a51643d6 100644
> >> --- a/include/linux/irqchip/arm-gic-v3.h
> >> +++ b/include/linux/irqchip/arm-gic-v3.h
> >> @@ -532,6 +532,8 @@
> >>  #define ICH_VTR_SEIS_MASK		(1 << ICH_VTR_SEIS_SHIFT)
> >>  #define ICH_VTR_A3V_SHIFT		21
> >>  #define ICH_VTR_A3V_MASK		(1 << ICH_VTR_A3V_SHIFT)
> >> +#define ICH_VTR_nV4_SHIFT		20
> >> +#define ICH_VTR_nV4_MASK		(1 << ICH_VTR_nV4_SHIFT)
> >>  
> >>  #define ICC_IAR1_EL1_SPURIOUS		0x3ff
> >>  
> >> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> >> index 405733678c2f..252268e83ade 100644
> >> --- a/virt/kvm/arm/vgic/vgic-v3.c
> >> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> >> @@ -466,6 +466,18 @@ static int __init early_gicv4_enable(char *buf)
> >>  }
> >>  early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
> >>  
> >> +static void vgic_check_v4_cpuif(void *param)
> >> +{
> >> +	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
> >> +	bool *v4 = param, this_cpu_is_v4;
> >> +
> >> +	this_cpu_is_v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
> >> +	if (!this_cpu_is_v4)
> >> +		kvm_info("CPU%d is not GICv4 capable\n", smp_processor_id());
> >> +
> >> +	*v4 &= this_cpu_is_v4;
> > 
> > nit: You could make this function look slightly less scary by declaring
> > this_cpu_is_v4 on a separate line and not use a bitwise operator on a
> > boolean type.  Actually, having a function called 'check' with a side
> > effect is not the nicest thing either, so why not just return what this
> > particular CPU does and do the comparison in the calling loop.
> 
> Fair enough.
> 
> > #bikeshedding
> 
> How about this on top:
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 01eaf4ee2f63..e471750dc0a1 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -469,18 +469,16 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
>  static void vgic_check_v4_cpuif(void *param)
>  {
>  	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
> -	bool *v4 = param, this_cpu_is_v4;
> +	bool *v4 = param;
>  
> -	this_cpu_is_v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
> -	if (!this_cpu_is_v4)
> +	*v4 = !(ich_vtr_el2 & ICH_VTR_nV4_MASK);
> +	if (!*v4)
>  		kvm_info("CPU%d is not GICv4 capable\n", smp_processor_id());
> -
> -	*v4 &= this_cpu_is_v4;
>  }
>  
>  /**
>   * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
> - * @node:	pointer to the DT node
> + * @info:	pointer to the firmware-agnostic GIC information
>   *
>   * Returns 0 if a GICv3 has been found, returns an error code otherwise
>   */
> @@ -504,9 +502,14 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>  	if (info->has_v4) {
>  		int cpu;
>  
> -		for_each_online_cpu(cpu)
> +		for_each_online_cpu(cpu) {
> +			bool enable;
> +
>  			smp_call_function_single(cpu, vgic_check_v4_cpuif,
> -						 &gicv4_enable, 1);
> +						 &enable, 1);
> +			gicv4_enable = gicv4_enable && enable;
> +		}
> +
>  		kvm_vgic_global_state.has_gicv4 = gicv4_enable;
>  		kvm_info("GICv4 support %sabled\n",
>  			 gicv4_enable ? "en" : "dis");
> 

Looks good to me, thanks!
-Christoffer

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web