Re: [PATCH v2 4/5] LoongArch: KVM: Implement vmid updating logic

On 2026/8/6 上午10:30, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing early return in `__kvm_check_vmid` for machines without guest ID support breaks VMID/VPID synchronization.
> - [Medium] Data race between non-atomic `cpumask_setall()` and atomic bitwise operations on `tlb_flush_pending`.
> - [Low] Dead code: `kvm_arch_flush_remote_tlbs_memslot` is a fictitious arch hook not called by KVM core.
> --
> 
> --- Patch [4]: [PATCH v2 4/5] LoongArch: KVM: Implement vmid updating logic ---
> commit 1626f2256d93ca3c5c607a6a950ca7531c03169a
> Author: Bibo Mao <[email protected]>
> Date:   Thu Aug 6 02:07:38 2026 +0000
> 
>      LoongArch: KVM: Implement vmid updating logic
>      
>      VMID calculation method is the same with ASID on LoongArch, it is
>      percpu vmid calculation method. For every physical CPU, VMID of
>      different VM is different, and it is the same for different vCPUs
>      of the same VM.
> 
>> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
>> index c36e62ab0d917..72c1d23156e50 100644
>> --- a/arch/loongarch/kvm/main.c
>> +++ b/arch/loongarch/kvm/main.c
> [ ... ]
>> @@ -291,6 +316,20 @@ static void __kvm_check_vmid(struct kvm_vcpu *vcpu)
>>   			kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
>>   		}
>>   	}
>> +
>> +	cpu = smp_processor_id();
>> +	if (cpumask_test_and_clear_cpu(cpu, &vcpu->kvm->arch.tlb_flush_pending))
>> +		vcpu->kvm->arch.vmid[cpu] = 0;
>> +
>> +	/*
>> +	 * Check if our vmid is 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.hw_vmid = vcpu->kvm->arch.vmid[cpu] & vpid_mask;
> 
> [Severity: High]
> Does this unconditionally overwrite vcpu->arch.hw_vmid for machines without
> guest ID support? On machines like 3A5000, hw_vmid is set to match vpid
> above, but without an early return or else block, it falls through and gets
> overwritten here with a dynamically allocated vmid.
yes, early return is missing in if (!cpu_has_guestid) sentence.
--- a/arch/loongarch/kvm/main.c
+++ b/arch/loongarch/kvm/main.c
@@ -315,6 +315,7 @@ static void __kvm_check_vmid(struct kvm_vcpu *vcpu)
                         vcpu->arch.hw_vmid = vcpu->arch.vpid & vpid_mask;
                         kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
                 }
+               return;
         }
> 
>>   }
>>   
>>   void kvm_check_vpid(struct kvm_vcpu *vcpu)
>> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
>> index 714640a2e6142..80e7a8bdc344a 100644
>> --- a/arch/loongarch/kvm/mmu.c
>> +++ b/arch/loongarch/kvm/mmu.c
> [ ... ]
>> @@ -942,6 +943,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: Medium]
> Could calling cpumask_setall() cause a data race with concurrent atomic
> bit operations?
> 
> cpumask_setall() uses a non-atomic memset() internally, while
> __kvm_check_vmid() and kvm_handle_mm_fault() use atomic read-modify-write
> operations like cpumask_test_and_clear_cpu() and cpumask_set_cpu() on the
> same cpumask.
This is under investigation, if data of memset() is not lost or 
invalidated, it will send IPI to destination vCPU and check the flag again.

Regards
Bibo Mao
> 
>> +		/* Be sure that other CPUS can watch the changes */
>> +		smp_wmb();
>> +	}
>> +
>> +	/* Return 1 continue to send ipi to running vCPUs */
>> +	return 1;
>> +}
>> +
>> +void kvm_arch_flush_remote_tlbs_memslot(struct kvm *kvm,
>> +					const struct kvm_memory_slot *memslot)
>> +{
>> +	kvm_flush_remote_tlbs(kvm);
>> +}
> 
> [Severity: Low]
> Is kvm_arch_flush_remote_tlbs_memslot() dead code? There does not appear to be
> a KVM core hook by this name, so this architecture-specific function won't be
> called by the KVM core during memslot updates.
>