Re: [RFC PATCH v3 29/29] KVM: arm64: selftests: Introduce id_reg_test

[Eric Auger <eauger@redhat.com>]

Hi Reiji,
On 11/20/21 7:39 AM, Reiji Watanabe wrote:
>  Hi Eric,
> 
> On Thu, Nov 18, 2021 at 12:34 PM Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Reiji,
>>
>> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
>>> Introduce a test for aarch64 to validate basic behavior of
>>> KVM_GET_ONE_REG and KVM_SET_ONE_REG for ID registers.
>>>
>>> This test runs only when KVM_CAP_ARM_ID_REG_CONFIGURABLE is supported.
>>
>> That's great to get those tests along with the series.
>>
>> There are several tests actually. I would encourage you to drop a short
>> comment along with the each main test to summarize what it does.
> 
> Thank you for the review !
> Yes, I will add short comments for the main test to summarize what it does.
> 
> 
>>> +struct id_reg_test_info {
>>> +     char            *name;
>>> +     uint32_t        id;
>>> +     bool            can_clear;
>>> +     uint64_t        fixed_mask;
>>> +     uint64_t        org_val;
>> nit: original_val? or default_val?
> 
> That is an original (or initial) value that is set by KVM.
> I will change it to original_val.
> 
> 
>>> +void test_pmu_init(struct kvm_vm *vm, uint32_t vcpu)
>> I would remove the test_ prefix as it does not test anything but
>> enhances the initialization instead
> 
> Yes, I agree.
> I will remove test_ prefix from those names.
> 
>>> +{
>>> +     struct kvm_device_attr attr = {
>>> +             .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>>> +             .attr = KVM_ARM_VCPU_PMU_V3_INIT,
>>> +     };
>>> +     vcpu_ioctl(vm, vcpu, KVM_SET_DEVICE_ATTR, &attr);
>>> +}
>>> +
>>> +void test_sve_init(struct kvm_vm *vm, uint32_t vcpu)
>>> +{
>>> +     int feature = KVM_ARM_VCPU_SVE;
>>> +
>>> +     vcpu_ioctl(vm, vcpu, KVM_ARM_VCPU_FINALIZE, &feature);
>>> +}
>>> +
>>> +#define GICD_BASE_GPA                        0x8000000ULL
>>> +#define GICR_BASE_GPA                        0x80A0000ULL
>>> +
>>> +void test_vgic_init(struct kvm_vm *vm, uint32_t vcpu)
>>> +{
>>> +     /* We jsut need to configure gic v3 (we don't use it though) */
>>> +     vgic_v3_setup(vm, 1, GICD_BASE_GPA, GICR_BASE_GPA);
>>> +}
>>> +
>>> +#define      MAX_CAPS        2
>>> +struct feature_test_info {
>>> +     char    *name;  /* Feature Name (Debug information) */
>>> +     struct id_reg_test_info *sreg;  /* ID register for the feature */
>> ID register comprising the feature?
>>> +     int     shift;  /* Field of the ID register for the feature */
>> I guess you mean feature field bit shift
>>> +     int     min;    /* Min value to indicate the feature */
>> Min value that can be assigned to the feature field?
>>> +     bool    is_sign;        /* Is the field signed or unsigned ? */
>>> +     int     ncaps;          /* Number of valid Capabilities in caps[] */
>>> +     long    caps[MAX_CAPS]; /* Capabilities to indicate this feature */
>> I suggest: KVM_CAP_* capabilities requested to test this feature
>>> +     /* struct kvm_enable_cap to use the capability if needed */
>>> +     struct kvm_enable_cap   *opt_in_cap;
>>> +     bool    run_test;       /* Does guest run test for this feature ? */
>> s/run_test/guest_run?
>>> +     /* Initialization function for the feature as needed */
>> extra init sequence needed besides KVM CAP setting?
>>> +     void    (*init_feature)(struct kvm_vm *vm, uint32_t vcpuid);
>>> +     /* struct kvm_vcpu_init to opt-in the feature if needed */
>>> +     struct kvm_vcpu_init    *vcpu_init;
>>> +};
> 
> I am going to fix the comments as follows.
> (Or are any of them still unclear ?)
> 
>     /* ID register that identifies the presence of the feature */
>     struct id_reg_test_info *sreg;
> 
>     /* Bit shift for the field that identifies the presence of the feature */
>     int     shift;
> 
>     /* Min value of the field that indicates the presence of the feature */
>     int     min;    /* Min value to indicate the feature */
> 
>     /* KVM_CAP_* Capabilities to indicates that KVM supports this feature */
>     long    caps[MAX_CAPS]; /* Capabilities to indicate this feature */
> 
>     /* Should the guest check the ID register for this feature ? */
>     bool    run_test;
> 
>     /*
>      * Any extra function to configure the feature if needed.
>      * (e.g. KVM_ARM_VCPU_FINALIZE for SVE)
>      */
>     void    (*init_feature)(struct kvm_vm *vm, uint32_t vcpuid);
> 
>>> +
>>> +/* Test for optin CPU features */
>> opt-in?
> 
> I will fix it.
> 
>>> +static struct feature_test_info feature_test_info_table[] = {
>>> +     {
>>> +             .name = "SVE",
>>> +             .sreg = ID_REG_INFO(ID_AA64PFR0),
>>> +             .shift = ID_AA64PFR0_SVE_SHIFT,
>>> +             .min = 1,
>>> +             .caps = {KVM_CAP_ARM_SVE},
>>> +             .ncaps = 1,
>>> +             .init_feature = test_sve_init,
>>> +             .vcpu_init = &(struct kvm_vcpu_init) {
>>> +                     .features = {1ULL << KVM_ARM_VCPU_SVE},
>>> +             },
>>> +     },
>>> +     {
>>> +             .name = "GIC",
>>> +             .sreg = ID_REG_INFO(ID_AA64PFR0),
>>> +             .shift = ID_AA64PFR0_GIC_SHIFT,
>>> +             .min = 1,
>>> +             .caps = {KVM_CAP_IRQCHIP},
>>> +             .ncaps = 1,
>>> +             .init_feature = test_vgic_init,
>>> +     },
>>> +     {
>>> +             .name = "MTE",
>>> +             .sreg = ID_REG_INFO(ID_AA64PFR1),
>>> +             .shift = ID_AA64PFR1_MTE_SHIFT,
>>> +             .min = 2,
>>> +             .caps = {KVM_CAP_ARM_MTE},
>>> +             .ncaps = 1,
>>> +             .opt_in_cap = &(struct kvm_enable_cap) {
>>> +                             .cap = KVM_CAP_ARM_MTE,
>>> +             },
>>> +     },
>>> +     {
>>> +             .name = "PMUV3",
>>> +             .sreg = ID_REG_INFO(ID_AA64DFR0),
>>> +             .shift = ID_AA64DFR0_PMUVER_SHIFT,
>>> +             .min = 1,
>>> +             .init_feature = test_pmu_init,
>>> +             .caps = {KVM_CAP_ARM_PMU_V3},
>>> +             .ncaps = 1,
>>> +             .vcpu_init = &(struct kvm_vcpu_init) {
>>> +                     .features = {1ULL << KVM_ARM_VCPU_PMU_V3},
>>> +             },
>>> +     },
>>> +     {
>>> +             .name = "PERFMON",
>>> +             .sreg = ID_REG_INFO(ID_DFR0),
>>> +             .shift = ID_DFR0_PERFMON_SHIFT,
>>> +             .min = 3,
>>> +             .init_feature = test_pmu_init,
>>> +             .caps = {KVM_CAP_ARM_PMU_V3},
>>> +             .ncaps = 1,
>>> +             .vcpu_init = &(struct kvm_vcpu_init) {
>>> +                     .features = {1ULL << KVM_ARM_VCPU_PMU_V3},
>>> +             },
>>> +     },
>>> +};
>>> +
>>> +static int walk_id_reg_list(int (*fn)(struct id_reg_test_info *sreg, void *arg),
>>> +                         void *arg)
>>> +{
>>> +     int ret = 0, i;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(id_reg_list); i++) {
>>> +             ret = fn(&id_reg_list[i], arg);
>>> +             if (ret)
>>> +                     break;
>> none of your fn() function does return something != 0
> 
> I will change the callback function to return void instead of int.
> 
>>> +     }
>>> +     return ret;
>>> +}
>>> +
>>> +static int guest_code_id_reg_check_one(struct id_reg_test_info *sreg, void *arg)
>>> +{
>>> +     uint64_t val = sreg->read_reg();
>>> +
>>> +     GUEST_ASSERT_2(val == sreg->user_val, sreg->name, sreg->user_val);
>>> +     return 0;
>>> +}
>>> +
>>> +static void guest_code_id_reg_check_all(uint32_t cpu)
>>> +{
>>> +     walk_id_reg_list(guest_code_id_reg_check_one, NULL);
>>> +     GUEST_DONE();
>>> +}
>>> +
>>> +static void guest_code_do_nothing(uint32_t cpu)
>>> +{
>>> +     GUEST_DONE();
>>> +}
>>> +
>>> +static void guest_code_feature_check(uint32_t cpu)
>>> +{
>>> +     int i;
>>> +     struct feature_test_info *finfo;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(feature_test_info_table); i++) {
>>> +             finfo = &feature_test_info_table[i];
>>> +             if (finfo->run_test)
>>> +                     guest_code_id_reg_check_one(finfo->sreg, NULL);
>>> +     }
>>> +
>>> +     GUEST_DONE();
>>> +}
>>> +
>>> +static void guest_code_ptrauth_check(uint32_t cpuid)
>>> +{
>>> +     struct id_reg_test_info *sreg = ID_REG_INFO(ID_AA64ISAR1);
>>> +     uint64_t val = sreg->read_reg();
>>> +
>>> +     GUEST_ASSERT_2(val == sreg->user_val, "PTRAUTH", val);
>>> +     GUEST_DONE();
>>> +}
>>> +
>>> +static int reset_id_reg_info_one(struct id_reg_test_info *sreg, void *arg)
>> reset_id_reg_user_val_one()?
> 
> Thanks for the suggestion. I will fix that.
> 
>>> +{
>>> +     sreg->user_val = sreg->org_val;
>>> +     return 0;
>>> +}
>>> +
>>> +static void reset_id_reg_info(void)
>> reset_id_reg_user_val()?
> 
>  I will fix that.
> 
>>> +{
>>> +     walk_id_reg_list(reset_id_reg_info_one, NULL);
>>> +}
>>> +
>>> +static struct kvm_vm *test_vm_create_cap(uint32_t nvcpus,
>>> +             void (*guest_code)(uint32_t), struct kvm_vcpu_init *init,
>>> +             struct kvm_enable_cap *cap)
>>> +{
>>> +     struct kvm_vm *vm;
>>> +     uint32_t cpuid;
>>> +     uint64_t mem_pages;
>>> +
>>> +     mem_pages = DEFAULT_GUEST_PHY_PAGES + DEFAULT_STACK_PGS * nvcpus;
>>> +     mem_pages += mem_pages / (PTES_PER_MIN_PAGE * 2);
>>> +     mem_pages = vm_adjust_num_guest_pages(VM_MODE_DEFAULT, mem_pages);
>>
>>
>>> +
>>> +     vm = vm_create(VM_MODE_DEFAULT,
>>> +             DEFAULT_GUEST_PHY_PAGES + (DEFAULT_STACK_PGS * nvcpus),
>>> +             O_RDWR);
>> mem_pages must be used instead
>>
>> augere@virtlab-arm04:~/UPSTREAM/ML/tools/testing/selftests/kvm#
>> ./aarch64/id_reg_test
>> ==== Test Assertion Failure ====
>>   lib/kvm_util.c:825: vm_adjust_num_guest_pages(vm->mode, npages) == npages
>>   pid=11439 tid=11439 errno=0 - Success
>>      1  0x00000000004068cb: vm_userspace_mem_region_add at kvm_util.c:823
>>      2  0x00000000004071af: vm_create at kvm_util.c:319
>>      3  0x0000000000401afb: test_vm_create_cap at id_reg_test.c:508
>>      4  0x00000000004014a3: test_vm_create at id_reg_test.c:541
>>      5   (inlined by) init_id_reg_info at id_reg_test.c:1110
>>      6   (inlined by) main at id_reg_test.c:1125
>>      7  0x000003ffa7220de3: ?? ??:0
>>      8  0x00000000004015eb: _start at :?
>>   Number of guest pages is not compatible with the host. Try npages=528
> 
> Thank you for catching this (It didn't happen in my usual test environment).
> I will fix this.
> 
>>
>>
>> Don't you want to check the cap in a first place using kvm_check_cap and
>> cap->cap
> 
> It is done by the caller before trying to create the vm.
> 
> 
>>> +     if (cap)
>>> +             vm_enable_cap(vm, cap);
>>> +
>>> +     kvm_vm_elf_load(vm, program_invocation_name);
>>> +
>>> +     if (init && init->target == -1) {
>>> +             struct kvm_vcpu_init preferred;
>>> +
>>> +             vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &preferred);
>>> +             init->target = preferred.target;
>>> +     }
>>> +
>>> +     vm_init_descriptor_tables(vm);
>>> +     for (cpuid = 0; cpuid < nvcpus; cpuid++) {
>>> +             if (init)
>>> +                     aarch64_vcpu_add_default(vm, cpuid, init, guest_code);
>>> +             else
>>> +                     vm_vcpu_add_default(vm, cpuid, guest_code);
>> nit: vm_vcpu_add_default calls aarch64_vcpu_add_default(vm, vcpuid,
>> NULL, guest_code) so you can unconditionnaly call
>> aarch64_vcpu_add_default(vm, cpuid, init, guest_code)
> 
> Oh, thank you ! I will fix that (somehow I overlooked that...).
> 
>>> +
>>> +             vcpu_init_descriptor_tables(vm, cpuid);
>>> +     }
>>> +
>>> +     ucall_init(vm, NULL);
>>> +     return vm;
>>> +}
>>> +
>>> +static struct kvm_vm *test_vm_create(uint32_t nvcpus,
>>> +                                  void (*guest_code)(uint32_t),
>>> +                                  struct kvm_vcpu_init *init)
>>> +{
>>> +     return test_vm_create_cap(nvcpus, guest_code, init, NULL);
>>> +}
>> nit: not sure test_vm_create is needed. By the way it is already called
>> with init = NULL so we can call test_vm_create_cap with 2 NULL args
> 
> I will remove test_vm_create.
> 
> 
>>> +
>>> +static void test_vm_free(struct kvm_vm *vm)
>>> +{
>>> +     ucall_uninit(vm);
>>> +     kvm_vm_free(vm);
>>> +}
>>> +
>>> +#define      TEST_RUN(vm, cpu)       \
>>> +     (test_vcpu_run(__func__, __LINE__, vm, cpu, true))
>>> +
>>> +#define      TEST_RUN_NO_SYNC_DATA(vm, cpu)  \
>>> +     (test_vcpu_run(__func__, __LINE__, vm, cpu, false))
>>> +
>>> +static int test_vcpu_run(const char *test_name, int line,
>>> +                      struct kvm_vm *vm, uint32_t vcpuid, bool sync_data)
>>> +{
>>> +     struct ucall uc;
>>> +     int ret;
>>> +
>>> +     if (sync_data) {
>>> +             sync_global_to_guest(vm, id_reg_list);
>>> +             sync_global_to_guest(vm, feature_test_info_table);
>>> +     }
>>> +
>>> +     vcpu_args_set(vm, vcpuid, 1, vcpuid);
>>> +
>>> +     ret = _vcpu_run(vm, vcpuid);
>>> +     if (ret) {
>>> +             ret = errno;
>>> +             goto sync_exit;
>>> +     }
>>> +
>>> +     switch (get_ucall(vm, vcpuid, &uc)) {
>>> +     case UCALL_SYNC:
>>> +     case UCALL_DONE:
>>> +             ret = 0;
>>> +             break;
>>> +     case UCALL_ABORT:
>>> +             TEST_FAIL(
>>> +                 "%s (%s) at line %d (user %s at line %d), args[3]=0x%lx",
>>> +                 (char *)uc.args[0], (char *)uc.args[2], (int)uc.args[1],
>>> +                 test_name, line, uc.args[3]);
>>> +             break;
>>> +     default:
>>> +             TEST_FAIL("Unexpected guest exit\n");
>>> +     }
>>> +
>>> +sync_exit:
>>> +     if (sync_data) {
>>> +             sync_global_from_guest(vm, id_reg_list);
>>> +             sync_global_from_guest(vm, feature_test_info_table);
>>> +     }
>>> +     return ret;
>>> +}
>>> +
>>> +static int set_id_regs_after_run_test_one(struct id_reg_test_info *sreg,
>>> +                                       void *arg)
>>> +{
>>> +     struct kvm_vm *vm = arg;
>>> +     struct kvm_one_reg one_reg;
>>> +     uint32_t vcpuid = 0;
>>> +     uint64_t reg_val;
>>> +     int ret;
>>> +
>>> +     one_reg.addr = (uint64_t)&reg_val;
>>> +     one_reg.id = KVM_ARM64_SYS_REG(sreg->id);
>>> +
>>> +     vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, &one_reg);
>>> +     if ((reg_val != 0) && (sreg->can_clear)) {
>>> +             reg_val = 0;
>>> +             ret = _vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &one_reg);
>>> +             TEST_ASSERT(ret && errno == EINVAL,
>>> +                         "Changing ID reg value should fail\n");
>>> +     }
>>> +
>>> +     vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, &one_reg);> +   ret = _vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &one_reg);
>>> +     TEST_ASSERT(ret == 0, "Setting the same ID reg value should work\n");
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int set_id_regs_test_one(struct id_reg_test_info *sreg, void *arg)
>> if it is a test use test_ prefix
> 
> I will fix this.
> 
> 
>>> +{
>>> +     struct kvm_vm *vm = arg;
>>> +     struct kvm_one_reg one_reg;
>>> +     uint32_t vcpuid = 0;
>>> +     uint64_t reg_val;
>>> +
>>> +     one_reg.addr = (uint64_t)&reg_val;
>>> +     reset_id_reg_info();
>>> +
>>> +     one_reg.id = KVM_ARM64_SYS_REG(sreg->id);
>>> +     if (sreg->can_clear) {
>>> +             /* Change the register to 0 when possible */
>>> +             reg_val = 0;
>>> +             vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &one_reg);
>>> +             vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, &one_reg);
>>> +             TEST_ASSERT(reg_val == 0,
>>> +                 "GET(%s) didn't return 0 but 0x%lx", sreg->name, reg_val);
>>> +     }
>>> +
>>> +     /* Check if we can restore the initial value */
>>> +     reg_val = sreg->org_val;
>>> +     vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &one_reg);
>>> +     vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, &one_reg);
>>> +     TEST_ASSERT(reg_val == sreg->org_val,
>>> +                 "GET(%s) didn't return 0x%lx but 0x%lx",
>>> +                 sreg->name, sreg->org_val, reg_val);
>>> +     sreg->user_val = sreg->org_val;
>>> +     return 0;
>>> +}
>>> +
>>> +static void set_id_regs_test(void)
>> if it is a test use test_ prefix
> 
> I will fix this.
> 
>>> +{
>>> +     struct kvm_vm *vm;
>>> +     int ret;
>>> +
>>> +     reset_id_reg_info();
>>> +     vm = test_vm_create(1, guest_code_id_reg_check_all, NULL);
>> add test_vm_free()
> 
> I will fix this.
> 
>>> +
>>> +     ret = walk_id_reg_list(set_id_regs_test_one, vm);
>>> +     assert(!ret);
>>> +
>>> +     ret = TEST_RUN(vm, 0);
>>> +     TEST_ASSERT(!ret, "%s TEST_RUN failed, ret=0x%x", __func__, ret);
>>> +
>>> +     ret = walk_id_reg_list(set_id_regs_after_run_test_one, vm);
>>> +     assert(!ret);
>>> +}
>>> +
>>> +static bool caps_are_supported(long *caps, int ncaps)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; i < ncaps; i++) {
>>> +             if (kvm_check_cap(caps[i]) <= 0)
>>> +                     return false;
>>> +     }
>>> +     return true;
>>> +}
>>> +
>>> +static void test_feature_ptrauth(void)
>>> +{
>>> +     struct kvm_one_reg one_reg;
>>> +     struct kvm_vcpu_init init;
>>> +     struct kvm_vm *vm = NULL;
>>> +     struct id_reg_test_info *sreg = ID_REG_INFO(ID_AA64ISAR1);
>>> +     uint32_t vcpu = 0;
>>> +     int64_t rval;
>>> +     int ret;
>>> +     int apa, api, gpa, gpi;
>>> +     char *name = "PTRAUTH";
>>> +     long caps[2] = {KVM_CAP_ARM_PTRAUTH_ADDRESS,
>>> +                     KVM_CAP_ARM_PTRAUTH_GENERIC};
>>> +
>>> +     reset_id_reg_info();
>>> +     one_reg.addr = (uint64_t)&rval;
>>> +     one_reg.id = KVM_ARM64_SYS_REG(sreg->id);
>>> +
>>> +     if (caps_are_supported(caps, 2)) {
>>> +
>>> +             /* Test with feature enabled */
>>> +             memset(&init, 0, sizeof(init));
>>> +             init.target = -1;
>>> +             init.features[0] = (1ULL << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
>>> +                                 1ULL << KVM_ARM_VCPU_PTRAUTH_GENERIC);
>>> +             vm = test_vm_create_cap(1, guest_code_ptrauth_check, &init,
>>> +                                     NULL);
>>> +             vcpu_ioctl(vm, vcpu, KVM_GET_ONE_REG, &one_reg);
>>> +             apa = GET_ID_UFIELD(rval, ID_AA64ISAR1_APA_SHIFT);
>>> +             api = GET_ID_UFIELD(rval, ID_AA64ISAR1_API_SHIFT);
>>> +             gpa = GET_ID_UFIELD(rval, ID_AA64ISAR1_GPA_SHIFT);
>>> +             gpi = GET_ID_UFIELD(rval, ID_AA64ISAR1_GPI_SHIFT);
>>> +
>>> +             TEST_ASSERT((apa > 0) || (api > 0),
>>> +                         "Either apa(0x%x) or api(0x%x) must be available",
>>> +                         apa, gpa);
>>> +             TEST_ASSERT((gpa > 0) || (gpi > 0),
>>> +                         "Either gpa(0x%x) or gpi(0x%x) must be available",
>>> +                         gpa, gpi);
>>> +
>>> +             TEST_ASSERT((apa > 0) ^ (api > 0),
>>> +                         "Both apa(0x%x) and api(0x%x) must not be available",
>>> +                         apa, api);
>>> +             TEST_ASSERT((gpa > 0) ^ (gpi > 0),
>>> +                         "Both gpa(0x%x) and gpi(0x%x) must not be available",
>>> +                         gpa, gpi);
>>> +
>>> +             sreg->user_val = rval;
>>> +
>>> +             pr_debug("%s: Test with %s enabled (%s: 0x%lx)\n",
>>> +                      __func__, name, sreg->name, sreg->user_val);
>>> +             ret = TEST_RUN(vm, vcpu);
>>> +             TEST_ASSERT(!ret, "%s:KVM_RUN failed with %s enabled",
>>> +                         __func__, name);
>>> +             test_vm_free(vm);
>>> +     }
>>> +
>>> +     /* Test with feature disabled */
>>> +     reset_id_reg_info();
>>> +
>>> +     vm = test_vm_create(1, guest_code_feature_check, NULL);
>>> +     vcpu_ioctl(vm, vcpu, KVM_GET_ONE_REG, &one_reg);
>>> +
>>> +     apa = GET_ID_UFIELD(rval, ID_AA64ISAR1_APA_SHIFT);
>>> +     api = GET_ID_UFIELD(rval, ID_AA64ISAR1_API_SHIFT);
>>> +     gpa = GET_ID_UFIELD(rval, ID_AA64ISAR1_GPA_SHIFT);
>>> +     gpi = GET_ID_UFIELD(rval, ID_AA64ISAR1_GPI_SHIFT);
>>> +     TEST_ASSERT(!apa && !api && !gpa && !gpi,
>>> +         "apa(0x%x), api(0x%x), gpa(0x%x), gpi(0x%x) must be zero",
>>> +         apa, api, gpa, gpi);
>>> +
>>> +     pr_debug("%s: Test with %s disabled (%s: 0x%lx)\n",
>>> +              __func__, name, sreg->name, sreg->user_val);
>>> +
>>> +     ret = TEST_RUN(vm, vcpu);
>>> +     TEST_ASSERT(!ret, "%s TEST_RUN failed with %s enabled, ret=0x%x",
>>> +                 __func__, name, ret);
>>> +
>>> +     test_vm_free(vm);
>>> +}
>>> +
>>> +static bool feature_caps_are_available(struct feature_test_info *finfo)
>>> +{
>>> +     return ((finfo->ncaps > 0) &&
>>> +             caps_are_supported(finfo->caps, finfo->ncaps));
>>> +}
>>> +
>> comment with short explanation of what the test does
> 
> Yes, I will add a short explanation for each test.
> 
>>> +static void test_feature(struct feature_test_info *finfo)
>>> +{
>>> +     struct id_reg_test_info *sreg = finfo->sreg;
>>> +     struct kvm_one_reg one_reg;
>>> +     struct kvm_vcpu_init init, *initp = NULL;
>>> +     struct kvm_vm *vm = NULL;
>>> +     int64_t fval, reg_val;
>>> +     uint32_t vcpu = 0;
>>> +     bool is_sign = finfo->is_sign;
>>> +     int min = finfo->min;
>>> +     int shift = finfo->shift;
>>> +     int ret;
>>> +
>>> +     pr_debug("%s: %s (reg %s)\n", __func__, finfo->name, sreg->name);
>>> +
>>> +     reset_id_reg_info();
>>> +     finfo->run_test = 1;    /* Indicate that guest runs the test on it */
>>> +     one_reg.addr = (uint64_t)&reg_val;
>>> +     one_reg.id = KVM_ARM64_SYS_REG(sreg->id);
>>> +
>>> +     /* Test with feature enabled if the feature is available */
>> s/if the feature is available/if the feature is exposed in the default
>> ID_REG value and if the capabilities are supported at KVM level
> 
> Thank you for the suggestion. I will fix that.
> 
> 
>>> +void run_test(void)
>>> +{
>>> +     set_id_regs_test();
>>> +     test_feature_all();
>>> +     test_feature_ptrauth();
>>> +     test_feature_frac_all();
>>> +}
>>> +
>> basic comment would be helpful: attempts to clear a given id_reg and
>> populate the id_reg with the original value, and can_clear flag?
> 
> I will add some comments.
> 
>>> +static int init_id_reg_info_one(struct id_reg_test_info *sreg, void *arg)
>>> +{
>>> +     uint64_t reg_val;
>>> +     uint32_t vcpuid = 0;
>>> +     int ret;
>>> +     struct kvm_one_reg one_reg;
>>> +     struct kvm_vm *vm = arg;
>>> +
>>> +     one_reg.addr = (uint64_t)&reg_val;
>>> +     one_reg.id = KVM_ARM64_SYS_REG(sreg->id);
>>> +     vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, &one_reg);
>>> +     sreg->org_val = reg_val;
>>> +     sreg->user_val = reg_val;
>> nit: add a comment for user_val because it is not obvious why you set it
>> to reg_val.
> 
> I will add a comment for it.
> 
>>> +     if (sreg->org_val) {
>>> +             reg_val = 0;
>>> +             ret = _vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &one_reg);
>>> +             if (!ret)
>>> +                     sreg->can_clear = true;
>>> +     }
>>> +
>>> +     pr_debug("%s (0x%x): 0x%lx%s\n", sreg->name, sreg->id,
>>> +              sreg->org_val, sreg->can_clear ? ", can clear" : "");
>>> +
>>> +     return 0;
>>> +}
>>> +
>> add a comment? loop over the idreg list and populates each regid info
>> with the default, user and can_clear value
> 
> I will add a comment for the function.
> 
>>> +static void init_id_reg_info(void)
>>> +{
>>> +     struct kvm_vm *vm;
>>> +
>>> +     vm = test_vm_create(1, guest_code_do_nothing, NULL);
>>> +     walk_id_reg_list(init_id_reg_info_one, vm);
>>> +     test_vm_free(vm);
>>> +}
>>> +
>>> +int main(void)
>>> +{
>>> +
>>> +     setbuf(stdout, NULL);
>>> +
>>> +     if (kvm_check_cap(KVM_CAP_ARM_ID_REG_CONFIGURABLE) <= 0) {
>>> +             print_skip("KVM_CAP_ARM_ID_REG_CONFIGURABLE is not supported\n");
>>> +             exit(KSFT_SKIP);
>>> +     }
>>> +
>>> +     init_id_reg_info();
>>> +     run_test();
>>> +     return 0;
>>> +}
>>>
>>
>> After fixing the mem_pages stuff I get the following error on a cavium
>> machine.
>>
>> augere@virtlab-arm04:~/UPSTREAM/ML/tools/testing/selftests/kvm#
>> ./aarch64/id_reg_test
>> ==== Test Assertion Failure ====
>>   aarch64/id_reg_test.c:814: fval >= min
>>   pid=11692 tid=11692 errno=4 - Interrupted system call
>>      1  0x00000000004028d3: test_feature at id_reg_test.c:813
>>      2   (inlined by) test_feature_all at id_reg_test.c:863
>>      3   (inlined by) run_test at id_reg_test.c:1073
>>      4  0x000000000040156f: main at id_reg_test.c:1124
>>      5  0x000003ffa9420de3: ?? ??:0
>>      6  0x00000000004015eb: _start at :?
>>   PERFMON field of ID_DFR0 is too small (0)
>>
>> Fails on
>> test_feature: PERFMON (reg ID_DFR0)
>>
>> I will do my utmost to further debug
> 
> Thank you for running it in your environment and reporting this !
> This is very interesting...
> 
> It implies that the host's ID_DFR0_EL1.PerfMon is zero or 0xf
> (meaning FEAT_PMUv3 is not implemented) even though the host's
> ID_AA64DFR0_EL1.PMUVer indicates that FEAT_PMUv3 is implemented.
> 
> Would it be possible for you to check values of those two
> registers on the host (not on the guest) to see if both of them
> indicate the presence of FEAT_PMUv3 consistently ?

Here are both values printed in armpmu_register()
[   33.320901] armpmu_register perfmon=0x0 pmuver=0x4

        perfmon =
cpuid_feature_extract_unsigned_field(read_cpuid(ID_DFR0_EL1),
                        ID_DFR0_PERFMON_SHIFT);
        pmuver =
cpuid_feature_extract_unsigned_field(read_cpuid(ID_AA64DFR0_EL1),
                        ID_AA64DFR0_PMUVER_SHIFT);
        printk("%s perfmon=0x%x pmuver=0x%x\n", __func__, perfmon, pmuver);

My machine is a Gigabyte R181-T90 (ThunderX2)

Eric


Eric

> 
> Thanks,
> Reiji
>