Re: [PATCH v2 3/5] LoongArch: KVM: Add separate vmid feature support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch introduces `if (!cpu_has_guestid)` feature gates to bypass existing VPID and TLB flush mechanisms, but completely omits the alternative logic for when the feature is enabled, introducing severe latent vulnerabilities (broken VM isolation, skipped TLB flushes).
--

--- Patch [3]: [PATCH v2 3/5] LoongArch: KVM: Add separate vmid feature support ---
commit 6bfee108dace797d9e6015bf026032adaef591e9
Author: Bibo Mao <[email protected]>

LoongArch: KVM: Add separate vmid feature support

Feature cpu_has_guestid is used to check whether separate vmid/vpid
is supported or not. It is disabled by default, add this feature
checking, there is no function change.

> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> index ac804e77e844f..c36e62ab0d917 100644
> --- a/arch/loongarch/kvm/main.c
> +++ b/arch/loongarch/kvm/main.c
> @@ -218,7 +218,8 @@ static void kvm_update_vpid(struct kvm_vcpu *vcpu, int cpu)
>  		++vpid; /* vpid 0 reserved for root */
>  
>  		/* start new vpid cycle */
> -		kvm_flush_tlb_all();
> +		if (!cpu_has_guestid)
> +			kvm_flush_tlb_all();

[Severity: Critical]
If cpu_has_guestid is true, this skips the global TLB flush on VPID
wrap-around. Is there an alternative flush mechanism that should be
implemented here when the feature is enabled?

>  	}
>  
>  	context->vpid_cache = vpid;
> @@ -282,10 +283,13 @@ static void __kvm_check_vmid(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long vmid;
>  
> -	vmid = vcpu->arch.vpid & vpid_mask;
> -	if (vcpu->arch.hw_vmid != vmid) {
> -		vcpu->arch.hw_vmid = vcpu->arch.vpid & vpid_mask;
> -		kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +	/* On some machines like 3A5000, vmid needs the same with vpid */
> +	if (!cpu_has_guestid) {
> +		vmid = vcpu->arch.vpid & vpid_mask;
> +		if (vcpu->arch.hw_vmid != vmid) {
> +			vcpu->arch.hw_vmid = vcpu->arch.vpid & vpid_mask;
> +			kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +		}
>  	}
>  }

[Severity: Critical]
When cpu_has_guestid is true, the hw_vmid update is skipped entirely without
alternative logic, leaving hw_vmid uninitialized (0).

When kvm_switch_to_guest() in arch/loongarch/kvm/switch.S loads hw_vmid into
the hardware Guest ID register (LOONGARCH_CSR_GTLBC), will all VMs share
VMID 0, breaking stage-2 VM isolation?

> diff --git a/arch/loongarch/kvm/mmu.c b/arch/loongarch/kvm/mmu.c
> index e104897aa5328..714640a2e6142 100644
> --- a/arch/loongarch/kvm/mmu.c
> +++ b/arch/loongarch/kvm/mmu.c
> @@ -929,8 +929,10 @@ 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);
> +		}
>  	}
>  
>  	return 0;

[Severity: Critical]
Are we missing a TLB flush request for stale translation entries when
cpu_has_guestid is enabled?

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31a..0b2355aec9322 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -210,7 +210,8 @@ static int kvm_check_requests(struct kvm_vcpu *vcpu)
>  		return RESUME_GUEST;
>  
>  	if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
> -		vcpu->arch.vpid = 0;  /* Drop vpid for this vCPU */
> +		if (!cpu_has_guestid)
> +			vcpu->arch.vpid = 0;  /* Drop vpid for this vCPU */
>  
>  	if (kvm_dirty_ring_check_request(vcpu))
>  		return RESUME_HOST;

[Severity: Critical]
This consumes the KVM_REQ_TLB_FLUSH request but skips dropping the VPID when
cpu_has_guestid is true. Could this effectively swallow the request without
performing the actual flush logic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3