Re: [PATCH v3 2/3] LoongArch: KVM: Implement vmid updating logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Precise single-GPA TLB flushes are replaced with full VMID reallocations on page faults for CPUs with `cpu_has_guestid`.
- [High] Data race on the `tlb_flush_pending` bitmask between non-atomic and atomic operations.
--

commit 79994ac14e7ca6c42512f0cad89100f577b61b57
Author: Bibo Mao <maobibo@loongson.cn>

LoongArch: KVM: Implement vmid updating logic

This commit implements VMID updating logic for LoongArch, calculating VMIDs
on a per-CPU basis similar to ASIDs. It adds checks during vCPU scheduling to
ensure the VMID is valid against the globally cached VMID.

> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> index a98f0284134a0..0099d0f0b8030 100644
> --- a/arch/loongarch/kvm/main.c
> +++ b/arch/loongarch/kvm/main.c
[ ... ]
> @@ -278,15 +281,54 @@ static void __kvm_check_vpid(struct kvm_vcpu *vcpu)
[ ... ]
> +static void __kvm_check_vmid(struct kvm_vcpu *vcpu)
> +{
> +	int cpu;
> +	unsigned long ver, old, vmid;
> +
> +	/* On some machines like 3A5000, vmid needs the same with vpid */
> +	if (!cpu_has_guestid) {
> +		vmid = vcpu->arch.vpid & vpid_mask;
> +		if (vcpu->arch.tgid != vmid) {
> +			vcpu->arch.tgid = vcpu->arch.vpid & vpid_mask;
> +			kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +		}
> +		return;
>  	}
> +
> +	cpu = smp_processor_id();
> +	if (cpumask_test_and_clear_cpu(cpu, &vcpu->kvm->arch.tlb_flush_pending))
> +		vcpu->kvm->arch.vmid[cpu] = 0;

[Severity: High]
Is there a risk of a data race on the tlb_flush_pending bitmask here? 
In this function, we perform an atomic read-modify-write operation using 
cpumask_test_and_clear_cpu(). However, kvm_arch_flush_remote_tlbs() 
concurrently performs a non-atomic memory write using cpumask_setall() 
on the same mask.

Mixing non-atomic stores with atomic operations on the same memory location 
can lead to lost bit updates. If a remote TLB flush request is lost due to 
this race, could a CPU continue executing with stale TLB entries?

> +
> +	/* Check if the vmid is out of an older version */
> +	ver = vcpu->kvm->arch.vmid[cpu] & ~vpid_mask;
> +	old = this_cpu_ptr(vcpu->kvm->arch.vmcs)->vmid_cache & ~vpid_mask;
> +	if (ver != old)
> +		kvm_update_vmid(vcpu, cpu);
> +
> +	vcpu->arch.tgid = vcpu->kvm->arch.vmid[cpu] & vpid_mask;
>  }
>  
>  void kvm_check_vpid(struct kvm_vcpu *vcpu)
[ ... ]
> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index e104897aa5328..e07ba28cc5655 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -893,7 +893,12 @@ static int kvm_map_page(struct kvm_vcpu *vcpu, unsigned long gpa, bool write)
>  		 * there is invalid tlb with small page
>  		 * need flush these invalid tlbs for current vcpu
>  		 */
> -		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> +		if (!cpu_has_ptw) {
> +			if (!cpu_has_guestid)
> +				kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> +			else
> +				cpumask_set_cpu(vcpu->cpu, &vcpu->kvm->arch.tlb_flush_pending);

[Severity: High]
Could this lead to a severe performance regression due to excessive TLB 
flushes? By setting the tlb_flush_pending bit instead of issuing a precise 
local single-page flush, this forces __kvm_check_vmid() to allocate a 
completely new VMID for the vCPU. 

This new VMID allocation causes the hardware to abandon all cached Stage 2 
TLB entries associated with the old VMID. Doing this on every TLB Modified 
exception (such as the first write to a tracked huge page) appears to 
guarantee a 100% TLB miss rate for existing mappings.

> +		}
>  		++kvm->stat.hugepages;
>  	}  else if (kvm_pte_huge(*ptep) && write)
>  		ptep = kvm_split_huge(vcpu, ptep, gfn);
> @@ -929,8 +934,11 @@ int kvm_handle_mm_fault(struct kvm_vcpu *vcpu, unsigned long gpa, bool write, in
>  		 *
>  		 * With SW PTW, invalid TLB is added in TLB refill exception.
>  		 */
> -		vcpu->arch.flush_gpa = gpa;
> -		kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +		if (!cpu_has_guestid) {
> +			vcpu->arch.flush_gpa = gpa;
> +			kvm_make_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +		} else
> +			cpumask_set_cpu(vcpu->cpu, &vcpu->kvm->arch.tlb_flush_pending);

[Severity: High]
Similar to the huge page mapping case above, does replacing the precise 
KVM_REQ_TLB_FLUSH_GPA request with the tlb_flush_pending bit cause a full 
VMID cycle when mapping normal pages? If so, this would flush the entire 
TLB for the VM on this CPU during routine page faults.

>  	}
>  
>  	return 0;
> @@ -940,6 +948,22 @@ void kvm_arch_sync_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot)
>  {
>  }
>  
> +int kvm_arch_flush_remote_tlbs(struct kvm *kvm)
> +{
> +	/*
> +	 * Queue a TLB invalidation for each CPU to perform on next
> +	 * vcpu loading
> +	 */
> +	if (cpu_has_guestid) {
> +		cpumask_setall(&kvm->arch.tlb_flush_pending);

[Severity: High]
Is this non-atomic memory write to tlb_flush_pending safe against concurrent 
atomic operations? As noted earlier, __kvm_check_vmid() uses the atomic 
cpumask_test_and_clear_cpu() on this same mask. Mixing non-atomic stores 
with atomic operations on the same memory location can cause data corruption 
and lost updates.

> +		/* Be sure that other CPUS can watch the changes */
> +		smp_wmb();
> +	}
> +
> +	/* Return 1 continue to send ipi to running vCPUs */
> +	return 1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831094716.4016652-1-maobibo@loongson.cn?part=2