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@xxxxxxxxxx> 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)®_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)®_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)®_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)®_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
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
