[PATCH v3 5/5] KVM: arm64: selftests: Test for setting ID register from usersapce

[Jing Zhang]

Hi Zenghui,

I don't have a Cortex A72 to fully verify the fix. Could you help
verify the following change?

diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
index bac05210b539..f17454dc6d9e 100644
--- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -224,13 +224,20 @@ uint64_t get_safe_value(const struct
reg_ftr_bits *ftr_bits, uint64_t ftr)
 {
        uint64_t ftr_max = GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0);

-       if (ftr_bits->type == FTR_UNSIGNED) {
+       if (ftr_bits->sign == FTR_UNSIGNED) {
                switch (ftr_bits->type) {
                case FTR_EXACT:
                        ftr = ftr_bits->safe_val;
                        break;
                case FTR_LOWER_SAFE:
-                       if (ftr > 0)
+                       uint64_t min_safe = 0;
+
+                       if (!strcmp(ftr_bits->name, "ID_AA64DFR0_EL1_DebugVer"))
+                               min_safe = ID_AA64DFR0_EL1_DebugVer_IMP;
+                       else if (!strcmp(ftr_bits->name, "ID_DFR0_EL1_CopDbg"))
+                               min_safe = ID_DFR0_EL1_CopDbg_Armv8;
+
+                       if (ftr > min_safe)
                                ftr--;
                        break;
                case FTR_HIGHER_SAFE:
@@ -252,7 +259,12 @@ uint64_t get_safe_value(const struct reg_ftr_bits
*ftr_bits, uint64_t ftr)
                        ftr = ftr_bits->safe_val;
                        break;
                case FTR_LOWER_SAFE:
-                       if (ftr > 0)
+                       uint64_t min_safe = 0;
+
+                       if (!strcmp(ftr_bits->name, "ID_DFR0_EL1_PerfMon"))
+                               min_safe = ID_DFR0_EL1_PerfMon_PMUv3;
+
+                       if (ftr > min_safe)
                                ftr--;
                        break;
                case FTR_HIGHER_SAFE:
@@ -276,7 +288,7 @@ uint64_t get_invalid_value(const struct
reg_ftr_bits *ftr_bits, uint64_t ftr)
 {
        uint64_t ftr_max = GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0);

-       if (ftr_bits->type == FTR_UNSIGNED) {
+       if (ftr_bits->sign == FTR_UNSIGNED) {
                switch (ftr_bits->type) {
                case FTR_EXACT:
                        ftr = max((uint64_t)ftr_bits->safe_val + 1, ftr + 1);

On Mon, Jan 8, 2024 at 2:40 PM Oliver Upton <oliver.upton at linux.dev> wrote:
>
> Hi Zenghui,
>
> On Fri, Jan 05, 2024 at 05:07:08PM +0800, Zenghui Yu wrote:
> > On 2023/10/19 16:38, Eric Auger wrote:
> >
> > > > +static const struct reg_ftr_bits ftr_id_aa64dfr0_el1[] = {
> > > > + S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, PMUVer, 0),
> > >
> > > Strictly speaking this is not always safe to have a lower value. For
> > > instance: From Armv8.1, if FEAT_PMUv3 is implemented, the value 0b0001
> > > is not permitted. But I guess this consistency is to be taken into
> > > account by the user space. But may be wort a comment. Here and below
> > >
> > > You may at least clarify what does mean 'safe'
> > >
> > > > + REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DebugVer, 0),
> >
> > I've seen the following failure on Cortex A72 where
> > ID_AA64DFR0_EL1.DebugVer is 6.
>
> Ah, yes, the test is wrong. KVM enforces a minimum value of 0x6 on this
> field, yet get_safe_value() returns 0x5 for the field.
>
> Jing, do you have time to check this test for similar failures and send
> out a fix for Zenghui's observations?
>
> > # ./aarch64/set_id_regs
> > TAP version 13
> > 1..79
> > ok 1 ID_AA64DFR0_EL1_PMUVer
> > ==== Test Assertion Failure ====
> >   include/kvm_util_base.h:553: !ret
> >   pid=2288505 tid=2288505 errno=22 - Invalid argument
> >      1        0x0000000000402787: vcpu_set_reg at kvm_util_base.h:553
> > (discriminator 6)
> >      2         (inlined by) test_reg_set_success at set_id_regs.c:342
> > (discriminator 6)
> >      3         (inlined by) test_user_set_reg at set_id_regs.c:413 (discriminator
> > 6)
> >      4        0x0000000000401943: main at set_id_regs.c:475
> >      5        0x0000ffffbdd5d03b: ?? ??:0
> >      6        0x0000ffffbdd5d113: ?? ??:0
> >      7        0x0000000000401a2f: _start at ??:?
> >   KVM_SET_ONE_REG failed, rc: -1 errno: 22 (Invalid argument)
>
> --
> Thanks,
> Oliver

Thanks,
Jing