[PATCH v4] KVM: Check the allocation of pv cpu mask

[PATCH v4] KVM: Check the allocation of pv cpu mask

[lihaiwei.kernel]

From: Haiwei Li <lihaiwei@tencent.com>

check the allocation of per-cpu __pv_cpu_mask. Init
'send_IPI_mask_allbutself' only when successful and check the allocation
of __pv_cpu_mask in 'kvm_flush_tlb_others'.

Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
---
v1 -> v2:
 * add CONFIG_SMP for kvm_send_ipi_mask_allbutself to prevent build error
v2 -> v3:
 * always check the allocation of __pv_cpu_mask in kvm_flush_tlb_others
v3 -> v4:
 * mov kvm_setup_pv_ipi to kvm_alloc_cpumask and get rid of kvm_apic_init

 arch/x86/kernel/kvm.c | 53 +++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 42c6e0deff9e..be28203cc098 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -547,16 +547,6 @@ static void kvm_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
 	__send_ipi_mask(local_mask, vector);
 }
 
-/*
- * Set the IPI entry points
- */
-static void kvm_setup_pv_ipi(void)
-{
-	apic->send_IPI_mask = kvm_send_ipi_mask;
-	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
-	pr_info("setup PV IPIs\n");
-}
-
 static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
 {
 	int cpu;
@@ -619,6 +609,11 @@ static void kvm_flush_tlb_others(const struct cpumask *cpumask,
 	struct kvm_steal_time *src;
 	struct cpumask *flushmask = this_cpu_cpumask_var_ptr(__pv_cpu_mask);
 
+	if (unlikely(!flushmask)) {
+		native_flush_tlb_others(cpumask, info);
+		return;
+	}
+
 	cpumask_copy(flushmask, cpumask);
 	/*
 	 * We have to call flush only on online vCPUs. And
@@ -732,10 +727,6 @@ static uint32_t __init kvm_detect(void)
 
 static void __init kvm_apic_init(void)
 {
-#if defined(CONFIG_SMP)
-	if (pv_ipi_supported())
-		kvm_setup_pv_ipi();
-#endif
 }
 
 static void __init kvm_init_platform(void)
@@ -765,10 +756,18 @@ static __init int activate_jump_labels(void)
 }
 arch_initcall(activate_jump_labels);
 
+static void kvm_free_cpumask(void)
+{
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu)
+		free_cpumask_var(per_cpu(__pv_cpu_mask, cpu));
+}
+
 static __init int kvm_alloc_cpumask(void)
 {
 	int cpu;
-	bool alloc = false;
+	bool alloc = false, alloced = true;
 
 	if (!kvm_para_available() || nopv)
 		return 0;
@@ -783,10 +782,30 @@ static __init int kvm_alloc_cpumask(void)
 
 	if (alloc)
 		for_each_possible_cpu(cpu) {
-			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_cpu_mask, cpu),
-				GFP_KERNEL, cpu_to_node(cpu));
+			if (!zalloc_cpumask_var_node(
+				per_cpu_ptr(&__pv_cpu_mask, cpu),
+				GFP_KERNEL, cpu_to_node(cpu))) {
+				alloced = false;
+				break;
+			}
 		}
 
+#if defined(CONFIG_SMP)
+	/* Set the IPI entry points */
+	if (pv_ipi_supported()) {
+		apic->send_IPI_mask = kvm_send_ipi_mask;
+		if (alloced)
+			apic->send_IPI_mask_allbutself =
+				kvm_send_ipi_mask_allbutself;
+		pr_info("setup PV IPIs\n");
+	}
+#endif
+
+	if (!alloced) {
+		kvm_free_cpumask();
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 arch_initcall(kvm_alloc_cpumask);
-- 
2.18.4

Re: [PATCH v4] KVM: Check the allocation of pv cpu mask

[Vitaly Kuznetsov]

lihaiwei.kernel@gmail.com writes:

> From: Haiwei Li <lihaiwei@tencent.com>
>
> check the allocation of per-cpu __pv_cpu_mask. Init
> 'send_IPI_mask_allbutself' only when successful and check the allocation
> of __pv_cpu_mask in 'kvm_flush_tlb_others'.
>
> Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
> ---
> v1 -> v2:
>  * add CONFIG_SMP for kvm_send_ipi_mask_allbutself to prevent build error
> v2 -> v3:
>  * always check the allocation of __pv_cpu_mask in kvm_flush_tlb_others
> v3 -> v4:
>  * mov kvm_setup_pv_ipi to kvm_alloc_cpumask and get rid of kvm_apic_init
>
>  arch/x86/kernel/kvm.c | 53 +++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 42c6e0deff9e..be28203cc098 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -547,16 +547,6 @@ static void kvm_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
>  	__send_ipi_mask(local_mask, vector);
>  }
>  
> -/*
> - * Set the IPI entry points
> - */
> -static void kvm_setup_pv_ipi(void)
> -{
> -	apic->send_IPI_mask = kvm_send_ipi_mask;
> -	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
> -	pr_info("setup PV IPIs\n");
> -}
> -
>  static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
>  {
>  	int cpu;
> @@ -619,6 +609,11 @@ static void kvm_flush_tlb_others(const struct cpumask *cpumask,
>  	struct kvm_steal_time *src;
>  	struct cpumask *flushmask = this_cpu_cpumask_var_ptr(__pv_cpu_mask);
>  
> +	if (unlikely(!flushmask)) {
> +		native_flush_tlb_others(cpumask, info);
> +		return;
> +	}
> +
>  	cpumask_copy(flushmask, cpumask);
>  	/*
>  	 * We have to call flush only on online vCPUs. And
> @@ -732,10 +727,6 @@ static uint32_t __init kvm_detect(void)
>  
>  static void __init kvm_apic_init(void)
>  {
> -#if defined(CONFIG_SMP)
> -	if (pv_ipi_supported())
> -		kvm_setup_pv_ipi();
> -#endif
>  }

Do we still need the now-empty function?

>  
>  static void __init kvm_init_platform(void)
> @@ -765,10 +756,18 @@ static __init int activate_jump_labels(void)
>  }
>  arch_initcall(activate_jump_labels);
>  
> +static void kvm_free_cpumask(void)
> +{
> +	unsigned int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		free_cpumask_var(per_cpu(__pv_cpu_mask, cpu));
> +}
> +
>  static __init int kvm_alloc_cpumask(void)
>  {
>  	int cpu;
> -	bool alloc = false;
> +	bool alloc = false, alloced = true;
>  
>  	if (!kvm_para_available() || nopv)
>  		return 0;
> @@ -783,10 +782,30 @@ static __init int kvm_alloc_cpumask(void)
>  
>  	if (alloc)
>  		for_each_possible_cpu(cpu) {
> -			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_cpu_mask, cpu),
> -				GFP_KERNEL, cpu_to_node(cpu));
> +			if (!zalloc_cpumask_var_node(
> +				per_cpu_ptr(&__pv_cpu_mask, cpu),
> +				GFP_KERNEL, cpu_to_node(cpu))) {
> +				alloced = false;
> +				break;
> +			}
>  		}
>  
> +#if defined(CONFIG_SMP)
> +	/* Set the IPI entry points */
> +	if (pv_ipi_supported()) {

What if we define pv_ipi_supported() in !CONFIG_SMP case as 'false'?

The code we have above:

        if (pv_tlb_flush_supported())
		alloc = true;

#if defined(CONFIG_SMP)
        if (pv_ipi_supported())
		alloc = true;
#endif

      	if (alloc)
...

will transform into 'if (pv_tlb_flush_supported() ||
pv_ipi_supported())' and we'll get rid of 'alloc' variable.

Also, we can probably get rid of this new 'alloced' variable and switch
to checking if the cpumask for the last CPU in cpu_possible_mask is not
NULL.
 
> +		apic->send_IPI_mask = kvm_send_ipi_mask;
> +		if (alloced)
> +			apic->send_IPI_mask_allbutself =
> +				kvm_send_ipi_mask_allbutself;
> +		pr_info("setup PV IPIs\n");

I'd rather not set 'apic->send_IPI_mask = kvm_send_ipi_mask' in case we
failed to alloc cpumask too. It is weird that in case of an allocation
failure *some* IPIs will use the PV path and some won't. It's going to
be a nightmare to debug.

> +	}
> +#endif
> +
> +	if (!alloced) {
> +		kvm_free_cpumask();
> +		return -ENOMEM;
> +	}
> +
>  	return 0;
>  }
>  arch_initcall(kvm_alloc_cpumask);

-- 
Vitaly

Re: [PATCH v4] KVM: Check the allocation of pv cpu mask

[Haiwei Li]

On 20/10/19 19:23, Vitaly Kuznetsov wrote:
> lihaiwei.kernel@gmail.com writes:
> 
>> From: Haiwei Li <lihaiwei@tencent.com>
>>
>> check the allocation of per-cpu __pv_cpu_mask. Init
>> 'send_IPI_mask_allbutself' only when successful and check the allocation
>> of __pv_cpu_mask in 'kvm_flush_tlb_others'.
>>
>> Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
>> ---
>> v1 -> v2:
>>   * add CONFIG_SMP for kvm_send_ipi_mask_allbutself to prevent build error
>> v2 -> v3:
>>   * always check the allocation of __pv_cpu_mask in kvm_flush_tlb_others
>> v3 -> v4:
>>   * mov kvm_setup_pv_ipi to kvm_alloc_cpumask and get rid of kvm_apic_init
>>
>>   arch/x86/kernel/kvm.c | 53 +++++++++++++++++++++++++++++--------------
>>   1 file changed, 36 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 42c6e0deff9e..be28203cc098 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -547,16 +547,6 @@ static void kvm_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
>>   	__send_ipi_mask(local_mask, vector);
>>   }
>>   
>> -/*
>> - * Set the IPI entry points
>> - */
>> -static void kvm_setup_pv_ipi(void)
>> -{
>> -	apic->send_IPI_mask = kvm_send_ipi_mask;
>> -	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
>> -	pr_info("setup PV IPIs\n");
>> -}
>> -
>>   static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
>>   {
>>   	int cpu;
>> @@ -619,6 +609,11 @@ static void kvm_flush_tlb_others(const struct cpumask *cpumask,
>>   	struct kvm_steal_time *src;
>>   	struct cpumask *flushmask = this_cpu_cpumask_var_ptr(__pv_cpu_mask);
>>   
>> +	if (unlikely(!flushmask)) {
>> +		native_flush_tlb_others(cpumask, info);
>> +		return;
>> +	}
>> +
>>   	cpumask_copy(flushmask, cpumask);
>>   	/*
>>   	 * We have to call flush only on online vCPUs. And
>> @@ -732,10 +727,6 @@ static uint32_t __init kvm_detect(void)
>>   
>>   static void __init kvm_apic_init(void)
>>   {
>> -#if defined(CONFIG_SMP)
>> -	if (pv_ipi_supported())
>> -		kvm_setup_pv_ipi();
>> -#endif
>>   }
> 
> Do we still need the now-empty function?

It's not necessary. I will remove it.

> 
>>   
>>   static void __init kvm_init_platform(void)
>> @@ -765,10 +756,18 @@ static __init int activate_jump_labels(void)
>>   }
>>   arch_initcall(activate_jump_labels);
>>   
>> +static void kvm_free_cpumask(void)
>> +{
>> +	unsigned int cpu;
>> +
>> +	for_each_possible_cpu(cpu)
>> +		free_cpumask_var(per_cpu(__pv_cpu_mask, cpu));
>> +}
>> +
>>   static __init int kvm_alloc_cpumask(void)
>>   {
>>   	int cpu;
>> -	bool alloc = false;
>> +	bool alloc = false, alloced = true;
>>   
>>   	if (!kvm_para_available() || nopv)
>>   		return 0;
>> @@ -783,10 +782,30 @@ static __init int kvm_alloc_cpumask(void)
>>   
>>   	if (alloc)
>>   		for_each_possible_cpu(cpu) {
>> -			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_cpu_mask, cpu),
>> -				GFP_KERNEL, cpu_to_node(cpu));
>> +			if (!zalloc_cpumask_var_node(
>> +				per_cpu_ptr(&__pv_cpu_mask, cpu),
>> +				GFP_KERNEL, cpu_to_node(cpu))) {
>> +				alloced = false;
>> +				break;
>> +			}
>>   		}
>>   
>> +#if defined(CONFIG_SMP)
>> +	/* Set the IPI entry points */
>> +	if (pv_ipi_supported()) {
> 
> What if we define pv_ipi_supported() in !CONFIG_SMP case as 'false'?
> 
> The code we have above:
> 
>          if (pv_tlb_flush_supported())
> 		alloc = true;
> 
> #if defined(CONFIG_SMP)
>          if (pv_ipi_supported())
> 		alloc = true;
> #endif
> 
>        	if (alloc)
> ...
> 
> will transform into 'if (pv_tlb_flush_supported() ||
> pv_ipi_supported())' and we'll get rid of 'alloc' variable.
> 
> Also, we can probably get rid of this new 'alloced' variable and switch
> to checking if the cpumask for the last CPU in cpu_possible_mask is not
> NULL.

Get it. It's a good point. I will do it. Thanks for your patience and 
kindness.

>   
>> +		apic->send_IPI_mask = kvm_send_ipi_mask;
>> +		if (alloced)
>> +			apic->send_IPI_mask_allbutself =
>> +				kvm_send_ipi_mask_allbutself;
>> +		pr_info("setup PV IPIs\n");
> 
> I'd rather not set 'apic->send_IPI_mask = kvm_send_ipi_mask' in case we
> failed to alloc cpumask too. It is weird that in case of an allocation
> failure *some* IPIs will use the PV path and some won't. It's going to
> be a nightmare to debug.

Agree. And 'pv_ops.mmu.tlb_remove_table = tlb_remove_table' should not 
be set either. What do you think? Thanks.

     Haiwei Li

Re: [PATCH v4] KVM: Check the allocation of pv cpu mask

[Vitaly Kuznetsov]

Haiwei Li <lihaiwei.kernel@gmail.com> writes:

> And 'pv_ops.mmu.tlb_remove_table = tlb_remove_table' should not 
> be set either.

AFAIU by looking at the commit which added it (48a8b97cfd80 "x86/mm:
Only use tlb_remove_table() for paravirt") it sholdn't hurt much. We
could've avoided the assignment but it happens much earlier, in
kvm_guest_init() and there's no good way to un-patch pvops back.

-- 
Vitaly