[PATCH v2 1/9] KVM: arm64: selftests: Use FIELD_GET() to extract ID register fields

[Reiji Watanabe]

Hi Oliver,

On Thu, Oct 20, 2022 at 12:08 PM Oliver Upton <oliver.upton at linux.dev> wrote:
>
> On Wed, Oct 19, 2022 at 10:41:54PM -0700, Reiji Watanabe wrote:
> > Use FIELD_GET() macro to extract ID register fields for existing
> > aarch64 selftests code. No functional change intended.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  tools/testing/selftests/kvm/aarch64/aarch32_id_regs.c  | 3 ++-
> >  tools/testing/selftests/kvm/aarch64/debug-exceptions.c | 3 ++-
> >  tools/testing/selftests/kvm/lib/aarch64/processor.c    | 7 ++++---
> >  3 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/aarch32_id_regs.c b/tools/testing/selftests/kvm/aarch64/aarch32_id_regs.c
> > index 6f9c1f19c7f6..b6a5e8861b35 100644
> > --- a/tools/testing/selftests/kvm/aarch64/aarch32_id_regs.c
> > +++ b/tools/testing/selftests/kvm/aarch64/aarch32_id_regs.c
> > @@ -13,6 +13,7 @@
> >  #include "kvm_util.h"
> >  #include "processor.h"
> >  #include "test_util.h"
> > +#include <linux/bitfield.h>
> >
> >  #define BAD_ID_REG_VAL       0x1badc0deul
> >
> > @@ -145,7 +146,7 @@ static bool vcpu_aarch64_only(struct kvm_vcpu *vcpu)
> >
> >       vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), &val);
> >
> > -     el0 = (val & ARM64_FEATURE_MASK(ID_AA64PFR0_EL0)) >> ID_AA64PFR0_EL0_SHIFT;
> > +     el0 = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL0), val);
> >       return el0 == ID_AA64PFR0_ELx_64BIT_ONLY;
> >  }
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index 947bd201435c..3808d3d75055 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -2,6 +2,7 @@
> >  #include <test_util.h>
> >  #include <kvm_util.h>
> >  #include <processor.h>
> > +#include <linux/bitfield.h>
> >
> >  #define MDSCR_KDE    (1 << 13)
> >  #define MDSCR_MDE    (1 << 15)
> > @@ -284,7 +285,7 @@ static int debug_version(struct kvm_vcpu *vcpu)
> >       uint64_t id_aa64dfr0;
> >
> >       vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &id_aa64dfr0);
> > -     return id_aa64dfr0 & 0xf;
> > +     return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), id_aa64dfr0);
> >  }
> >
> >  static void test_guest_debug_exceptions(void)
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 6f5551368944..7c96b931edd5 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -11,6 +11,7 @@
> >  #include "guest_modes.h"
> >  #include "kvm_util.h"
> >  #include "processor.h"
> > +#include <linux/bitfield.h>
> >
> >  #define DEFAULT_ARM64_GUEST_STACK_VADDR_MIN  0xac0000
> >
> > @@ -486,9 +487,9 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
> >       err = ioctl(vcpu_fd, KVM_GET_ONE_REG, &reg);
> >       TEST_ASSERT(err == 0, KVM_IOCTL_ERROR(KVM_GET_ONE_REG, vcpu_fd));
> >
> > -     *ps4k = ((val >> 28) & 0xf) != 0xf;
> > -     *ps64k = ((val >> 24) & 0xf) == 0;
> > -     *ps16k = ((val >> 20) & 0xf) != 0;
> > +     *ps4k = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN4), val) != 0xf;
> > +     *ps64k = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN64), val) == 0;
> > +     *ps16k = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN16), val) != 0;
>
> Not your code, but since we're here...
>
> Should we change the field values to use the #define's? Also, the test

I agree that would be better.

> for TGRAN64 looks wrong. We should test != ID_AA64MMFR0_TGRAN64_NI. A
> value greater than 0 would indicate an extension of the feature.

Yes, I thought about that, too.

I assumed the intention of the code was only 0x0 is defined as
64KB granule supported as of today unlike for other granule sizes,
which has more than one value that indicates the granule support.
But, considering principles of the ID scheme for fields in ID registers,
I think ">= ID_AA64MMFR0_TGRAN{4,16,64}_SUPPORTED_MIN" would be more
appropriate way of doing those check, although then TGRAN4 and TGRAN64
fields should be handled as signed fields (or we could do
"<= ID_AA64MMFR0_TGRAN{4,16,64}_SUPPORTED_MAX").

I can fix those if I have a chance to work on v3.

> But for this exact change:
>
> Reviewed-by: Oliver Upton <oliver.upton at linux.dev>

Thank you for the review!

Thanks,
Reiji