| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <30b2d279-8459-4a72-aad4-29c1ece622b8@linux.dev>
Date: Mon, 28 Apr 2025 17:32:09 -0700
From: Atish Patra <atish.patra@...ux.dev>
To: Andrew Jones <ajones@...tanamicro.com>
Cc: Anup Patel <anup@...infault.org>, Atish Patra <atishp@...shpatra.org>,
Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
<palmer@...belt.com>, Alexandre Ghiti <alex@...ti.fr>, kvm@...r.kernel.org,
kvm-riscv@...ts.infradead.org, linux-riscv@...ts.infradead.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] KVM: riscv: selftests: Add vector extension tests
On 4/25/25 7:20 AM, Andrew Jones wrote:
> On Mon, Mar 24, 2025 at 05:40:31PM -0700, Atish Patra wrote:
>> Add vector related tests with the ISA extension standard template.
>> However, the vector registers are bit tricky as the register length is
>> variable based on vlenb value of the system. That's why the macros are
>> defined with a default and overidden with actual value at runtime.
>>
>> Signed-off-by: Atish Patra <atishp@...osinc.com>
>> ---
>> tools/testing/selftests/kvm/riscv/get-reg-list.c | 111 ++++++++++++++++++++++-
>> 1 file changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
>> index 8515921dfdbf..576ab8eb7368 100644
>> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
>> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
>> @@ -145,7 +145,9 @@ void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>> {
>> unsigned long isa_ext_state[KVM_RISCV_ISA_EXT_MAX] = { 0 };
>> struct vcpu_reg_sublist *s;
>> - uint64_t feature;
>> + uint64_t feature = 0;
>> + u64 reg, size;
>> + unsigned long vlenb_reg;
>> int rc;
>>
>> for (int i = 0; i < KVM_RISCV_ISA_EXT_MAX; i++)
>> @@ -173,6 +175,23 @@ void finalize_vcpu(struct kvm_vcpu *vcpu, struct vcpu_reg_list *c)
>> switch (s->feature_type) {
>> case VCPU_FEATURE_ISA_EXT:
>> feature = RISCV_ISA_EXT_REG(s->feature);
>> + if (s->feature == KVM_RISCV_ISA_EXT_V) {
>> + /* Enable V extension so that we can get the vlenb register */
>> + __vcpu_set_reg(vcpu, feature, 1);
> We probably want to bail here if __vcpu_set_reg returns an error.
>
Sure. What do you mean by bail here ?
Continue to the next reg or just assert if it returns error.
>> + /* Compute the correct vector register size */
>> + rc = __vcpu_get_reg(vcpu, s->regs[4], &vlenb_reg);
> I see regs[4] is the encoding for vlenb, but I think we need a comment or
> a define or something in order to reduce head scratching.
>
Sure. Defined a macro.
>> + if (rc < 0)
>> + /* The vector test may fail if the default reg size doesn't match */
> I guess this comment should be below the break. We could probably use some
> blank lines in this code too. But, more importantly, what does this
> comment mean? That things may not work despite what we're doing here? Or,
> I think it means that we're doing this just in case the default size we
> already have set doesn't match. Can we reword it?
It's the latter. I will try to reword it.
>> + break;
>> + size = __builtin_ctzl(vlenb_reg);
>> + size <<= KVM_REG_SIZE_SHIFT;
>> + for (int i = 0; i < 32; i++) {
>> + reg = KVM_REG_RISCV | KVM_REG_RISCV_VECTOR | size |
>> + KVM_REG_RISCV_VECTOR_REG(i);
>> + s->regs[5 + i] = reg;
>> + }
>> + __vcpu_set_reg(vcpu, feature, 0);
> Switch this to vcpu_set_reg() since we want to assert it worked.
Done.
>> + }
> This if (s->feature == KVM_RISCV_ISA_EXT_V) block can go above the switch
> since it's not dependent on feature_type. I'd probably also create a
> function for it in order to keep finalize_vcpu() tidy and help with the
> indentation depth.
done.
>> break;
>> case VCPU_FEATURE_SBI_EXT:
>> feature = RISCV_SBI_EXT_REG(s->feature);
>> @@ -408,6 +427,35 @@ static const char *fp_d_id_to_str(const char *prefix, __u64 id)
>> return strdup_printf("%lld /* UNKNOWN */", reg_off);
>> }
>>
>> +static const char *vector_id_to_str(const char *prefix, __u64 id)
>> +{
>> + /* reg_off is the offset into struct __riscv_v_ext_state */
>> + __u64 reg_off = id & ~(REG_MASK | KVM_REG_RISCV_VECTOR);
>> + int reg_index = 0;
>> +
>> + assert((id & KVM_REG_RISCV_TYPE_MASK) == KVM_REG_RISCV_VECTOR);
>> +
>> + if (reg_off >= KVM_REG_RISCV_VECTOR_REG(0))
>> + reg_index = reg_off - KVM_REG_RISCV_VECTOR_REG(0);
>> + switch (reg_off) {
>> + case KVM_REG_RISCV_VECTOR_REG(0) ...
>> + KVM_REG_RISCV_VECTOR_REG(31):
>> + return strdup_printf("KVM_REG_RISCV_VECTOR_REG(%d)", reg_index);
>> + case KVM_REG_RISCV_VECTOR_CSR_REG(vstart):
>> + return "KVM_REG_RISCV_VECTOR_CSR_REG(vstart)";
>> + case KVM_REG_RISCV_VECTOR_CSR_REG(vl):
>> + return "KVM_REG_RISCV_VECTOR_CSR_REG(vl)";
>> + case KVM_REG_RISCV_VECTOR_CSR_REG(vtype):
>> + return "KVM_REG_RISCV_VECTOR_CSR_REG(vtype)";
>> + case KVM_REG_RISCV_VECTOR_CSR_REG(vcsr):
>> + return "KVM_RISCV_VCPU_VECTOR_CSR_REG(vcsr)";
>> + case KVM_REG_RISCV_VECTOR_CSR_REG(vlenb):
>> + return "KVM_REG_RISCV_VECTOR_CSR_REG(vlenb)";
>> + }
>> +
>> + return strdup_printf("%lld /* UNKNOWN */", reg_off);
>> +}
>> +
>> #define KVM_ISA_EXT_ARR(ext) \
>> [KVM_RISCV_ISA_EXT_##ext] = "KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_" #ext
>>
>> @@ -635,6 +683,9 @@ void print_reg(const char *prefix, __u64 id)
>> case KVM_REG_SIZE_U128:
>> reg_size = "KVM_REG_SIZE_U128";
>> break;
>> + case KVM_REG_SIZE_U256:
>> + reg_size = "KVM_REG_SIZE_U256";
>> + break;
>> default:
>> printf("\tKVM_REG_RISCV | (%lld << KVM_REG_SIZE_SHIFT) | 0x%llx /* UNKNOWN */,\n",
>> (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT, id & ~REG_MASK);
>> @@ -666,6 +717,10 @@ void print_reg(const char *prefix, __u64 id)
>> printf("\tKVM_REG_RISCV | %s | KVM_REG_RISCV_FP_D | %s,\n",
>> reg_size, fp_d_id_to_str(prefix, id));
>> break;
>> + case KVM_REG_RISCV_VECTOR:
>> + printf("\tKVM_REG_RISCV | %s | KVM_REG_RISCV_VECTOR | %s,\n",
>> + reg_size, vector_id_to_str(prefix, id));
>> + break;
>> case KVM_REG_RISCV_ISA_EXT:
>> printf("\tKVM_REG_RISCV | %s | KVM_REG_RISCV_ISA_EXT | %s,\n",
>> reg_size, isa_ext_id_to_str(prefix, id));
>> @@ -870,6 +925,54 @@ static __u64 fp_d_regs[] = {
>> KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_D,
>> };
>>
>> +/* Define a default vector registers with length. This will be overwritten at runtime */
>> +static __u64 vector_regs[] = {
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_VECTOR |
>> + KVM_REG_RISCV_VECTOR_CSR_REG(vstart),
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_VECTOR |
>> + KVM_REG_RISCV_VECTOR_CSR_REG(vl),
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_VECTOR |
>> + KVM_REG_RISCV_VECTOR_CSR_REG(vtype),
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_VECTOR |
>> + KVM_REG_RISCV_VECTOR_CSR_REG(vcsr),
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_VECTOR |
>> + KVM_REG_RISCV_VECTOR_CSR_REG(vlenb),
> Let these lines stick out to be easier to read and ensure one register
> encoding per line (we don't care about line length at all in this file :-)
>
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(0),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(1),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(2),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(3),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(4),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(5),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(6),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(7),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(8),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(9),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(10),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(11),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(12),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(13),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(14),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(15),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(16),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(17),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(18),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(19),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(20),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(21),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(22),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(23),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(24),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(25),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(26),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(27),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(28),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(29),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(30),
>> + KVM_REG_RISCV | KVM_REG_SIZE_U128 | KVM_REG_RISCV_VECTOR | KVM_REG_RISCV_VECTOR_REG(31),
>> + KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE |
>> + KVM_RISCV_ISA_EXT_V,
> should also stick out
>
>> +};
>> +
>> #define SUBLIST_BASE \
>> {"base", .regs = base_regs, .regs_n = ARRAY_SIZE(base_regs), \
>> .skips_set = base_skips_set, .skips_set_n = ARRAY_SIZE(base_skips_set),}
>> @@ -894,6 +997,10 @@ static __u64 fp_d_regs[] = {
>> {"fp_d", .feature = KVM_RISCV_ISA_EXT_D, .regs = fp_d_regs, \
>> .regs_n = ARRAY_SIZE(fp_d_regs),}
>>
>> +#define SUBLIST_V \
>> + {"v", .feature = KVM_RISCV_ISA_EXT_V, .regs = vector_regs, \
>> + .regs_n = ARRAY_SIZE(vector_regs),}
> I'd also let this stick out since it won't even be 100 chars.
>
It is actually little longer than 100 (103) but it is definitely more
readable if it sticks out.
Fixed all the truncated lines.
>> +
>> #define KVM_ISA_EXT_SIMPLE_CONFIG(ext, extu) \
>> static __u64 regs_##ext[] = { \
>> KVM_REG_RISCV | KVM_REG_SIZE_ULONG | \
>> @@ -962,6 +1069,7 @@ KVM_SBI_EXT_SIMPLE_CONFIG(susp, SUSP);
>> KVM_ISA_EXT_SUBLIST_CONFIG(aia, AIA);
>> KVM_ISA_EXT_SUBLIST_CONFIG(fp_f, FP_F);
>> KVM_ISA_EXT_SUBLIST_CONFIG(fp_d, FP_D);
>> +KVM_ISA_EXT_SUBLIST_CONFIG(v, V);
>> KVM_ISA_EXT_SIMPLE_CONFIG(h, H);
>> KVM_ISA_EXT_SIMPLE_CONFIG(smnpm, SMNPM);
>> KVM_ISA_EXT_SUBLIST_CONFIG(smstateen, SMSTATEEN);
>> @@ -1034,6 +1142,7 @@ struct vcpu_reg_list *vcpu_configs[] = {
>> &config_fp_f,
>> &config_fp_d,
>> &config_h,
>> + &config_v,
>> &config_smnpm,
>> &config_smstateen,
>> &config_sscofpmf,
>>
>> --
>> 2.43.0
>>
> Thanks,
> drew
Powered by blists - more mailing lists