Re: [PATCH v10] KVM: selftests: riscv: Add lazy V extension enablement for guests

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Lazy V extension enablement fails on hardware that does not populate stval for illegal instructions.
- [Medium] Storing a per-vCPU V-extension availability flag in a VM-wide shared structure causes test failures in mixed-vCPU VMs.
--

--- Patch [1]: [PATCH v10] KVM: selftests: riscv: Add lazy V extension enablement for guests ---
commit a24ebca96066b2892004c26d282cf14c6162da61
Author: jinrui <[email protected]>
KVM: selftests: riscv: Add lazy V extension enablement for guests

This commit adds lazy V extension enablement for guests in KVM RISC-V selftests
to handle auto-vectorized instructions when sstatus.VS is off. It registers a
full exception vector table, advertises V to KVM, and lazily enables V in
route_exception().

> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index ded5429f34483..ffd84212c9b0d 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
[ ... ]
> @@ -348,8 +346,22 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, u32 vcpu_id)
>  	/* Setup sscratch for guest_get_vcpuid() */
>  	vcpu_set_reg(vcpu, RISCV_GENERAL_CSR_REG(sscratch), vcpu_id);
>  
> -	/* Setup default exception vector of guest */
> -	vcpu_set_reg(vcpu, RISCV_GENERAL_CSR_REG(stvec), (unsigned long)guest_unexp_trap);
> +	/*
> +	 * Advertise V to KVM so -O2 auto-vectorization in guest code is valid;
> +	 * ignore errors since the tests work without V too. Use the full
> +	 * exception vector table (which lazily enables V in route_exception())
> +	 * as the default handler; vm_init_vector_tables() is idempotent.
> +	 */
> +	__vcpu_set_reg(vcpu, RISCV_ISA_EXT_REG(KVM_RISCV_ISA_EXT_V), 1);
> +	vm_init_vector_tables(vm);
> +	vcpu_init_vector_tables(vcpu);
> +
> +	/* Record V availability for route_exception(), which runs in guest context. */
> +	{
> +		struct handlers *h = addr_gva2hva(vm, vm->handlers);
> +
> +		h->v_available = __vcpu_has_isa_ext(vcpu, KVM_RISCV_ISA_EXT_V);

[Severity: Medium]
Does this overwrite a shared VM-wide state with a per-vCPU capability?

The handlers structure is allocated once per VM in vm_init_vector_tables().
If a VM is created with a mix of vCPUs, and the last added vCPU has the V
extension, h->v_available will be true globally for all vCPUs.

If a non-V vCPU encounters an illegal instruction that happens to decode as a
vector instruction, could route_exception() see h->v_available as true and
continually set SR_VS_INITIAL, causing an infinite exception loop?

> +	}
>  
>  	return vcpu;
>  }
[ ... ]
> @@ -432,6 +468,20 @@ void route_exception(struct pt_regs *regs)
>  		ec = 0;
>  	}
>  
> +	/*
> +	 * Lazily enable V on the first vector instruction: if the faulting
> +	 * instruction (stval) decodes as vector while VS is off, set VS to
> +	 * Initial and re-execute it, like the kernel's
> +	 * riscv_v_first_use_handler(). Genuinely illegal instructions fall
> +	 * through to the unexpected-exception path.
> +	 */
> +	if (!(regs->cause & CAUSE_IRQ_FLAG) && ec == EXC_INST_ILLEGAL &&
> +	    handlers && handlers->v_available && !(regs->status & SR_VS) &&
> +	    insn_is_vector((u32)regs->badaddr)) {

[Severity: Medium]
Does this assume hardware always populates stval (which is mapped to
regs->badaddr) for illegal instruction exceptions?

If a test executes on hardware that does not populate stval and leaves it as 0,
insn_is_vector() would evaluate to false.

Would this cause genuinely auto-vectorized instructions to unexpectedly fall
through to kvm_exit_unexpected_exception() instead of being lazily enabled?

Should this fall back to reading the instruction directly from guest memory at
regs->epc when badaddr is 0?

> +		regs->status |= SR_VS_INITIAL;
> +		return;
> +	}

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