Re: [PATCH v4 8/8] KVM: riscv: selftests: add Zicfiss/Zicfilp extension to get-reg-list test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing base FWFT extension register in sbi_fwft_landing_pad_regs and sbi_fwft_shadow_stack_regs.
- [Low] Incorrect struct name referenced in comment for zicfiss_csr_id_to_str.
--

commit 277e1fec57d15d21d187626b4aa2c8ebecc81e21
Author: Inochi Amaoto <inochiama@gmail.com>

KVM: riscv: selftests: add Zicfiss/Zicfilp extension to get-reg-list test

This commit adds Zicfiss and Zicfilp extensions support to the KVM RISC-V 
get-reg-list selftest. It introduces parsing and printing of related CSRs 
and SBI FWFT configurations.

> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index 94e33b2ee7963..5963f62ce7302 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c

[ ... ]

> @@ -425,6 +429,18 @@ static const char *smstateen_csr_id_to_str(__u64 reg_off)
>  	return NULL;
>  }
>  
> +static const char *zicfiss_csr_id_to_str(__u64 reg_off)
> +{
> +	/* reg_off is the offset into struct kvm_riscv_cfi_csr */

[Severity: Low]
This isn't a bug, but there appears to be a typo in the comment. Should this 
reference struct kvm_riscv_zicfiss_csr instead of struct kvm_riscv_cfi_csr 
as defined in the KVM UAPI headers?

> +	switch (reg_off) {
> +	case KVM_REG_RISCV_CSR_ZICFISS_REG(ssp):
> +		return RISCV_CSR_ZICFISS(ssp);
> +	}
> +
> +	TEST_FAIL("Unknown zicfiss csr reg: 0x%llx", reg_off);
> +	return NULL;
> +}

[ ... ]

> @@ -954,6 +980,18 @@ static __u64 sbi_fwft_pte_ad_hw_updating_regs[] = {
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(pte_ad_hw_updating.value),
>  };
>  
> +static __u64 sbi_fwft_landing_pad_regs[] = {
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(landing_pad.enable),
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(landing_pad.flags),
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(landing_pad.value),
> +};
> +
> +static __u64 sbi_fwft_shadow_stack_regs[] = {
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(shadow_stack.enable),
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(shadow_stack.flags),
> +	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_STATE | KVM_REG_RISCV_SBI_FWFT | KVM_REG_RISCV_SBI_FWFT_REG(shadow_stack.value),
> +};

[Severity: Low]
Is the base FWFT extension register intentionally omitted from these sublists? 

Looking at sbi_fwft_pte_ad_hw_updating_regs defined just above, it includes:
KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_SBI_EXT | KVM_REG_RISCV_SBI_SINGLE | KVM_RISCV_SBI_EXT_FWFT

Should the base FWFT extension register also be included in 
sbi_fwft_landing_pad_regs and sbi_fwft_shadow_stack_regs to match the pattern 
established by other FWFT sublists?

> +
>  static __u64 zicbom_regs[] = {
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_CONFIG | KVM_REG_RISCV_CONFIG_REG(zicbom_block_size),
>  	KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZICBOM,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701020746.170157-1-inochiama@gmail.com?part=8