Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
On Wed, Apr 07, 2021 at 12:14:29PM +0200, Auger Eric wrote:
> >> +int _kvm_create_device(struct kvm_vm *vm, uint64_t type, bool test)
> >> +{
> >> + struct kvm_create_device create_dev;
> >> + int ret;
> >> +
> >> + create_dev.type = type;
> >> + create_dev.fd = -1;
> >> + create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
> >> + ret = ioctl(vm_get_fd(vm), KVM_CREATE_DEVICE, _dev);
> >> + if (ret == -1)
> >> + return -errno;
> >> + return test ? 0 : create_dev.fd;
> >
> > Something like this belongs in the non underscore prefixed wrappers.
> I need at least to return the create_dev.fd or do you want me to add an
> extra int *fd parameter?
> What about:
>
> if (ret < 0)
> return ret;
> return test ? 0 : create_dev.fd;
Maybe the underscore version of kvm_create_device isn't necessary. If
the non-underscore version isn't flexible enough, then just use the
ioctl directly from the test code with its own struct kvm_create_device
Being able to call ioctls directly from test code is what vm_get_fd()
is for, otherwise you could just use vm->fd.
Thanks,
drew
Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
Hi Drew,
On 4/6/21 5:09 PM, Andrew Jones wrote:
>
> Hi Eric,
>
> It looks like Marc already picked this patch up, but, FWIW, here's
> a few more comments you may consider.
I will send a fixup patch on top of the one taken my Marc. Few comments
below.
>
> On Mon, Apr 05, 2021 at 06:39:41PM +0200, Eric Auger wrote:
>> The tests exercise the VGIC_V3 device creation including the
>> associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:
>>
>> - KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
>> - KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>>
>> Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
>> and especially the GICR_TYPER read. The goal was to test the case
>> recently fixed by commit 23bde34771f1
>> ("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").
>>
>> The API under test can be found at
>> Documentation/virt/kvm/devices/arm-vgic-v3.rst
>>
>> Signed-off-by: Eric Auger
>>
>> ---
>>
>> v4 -> v5:
>> - simplify the last bit tests given the simpler interpretation
>> of the spec
>>
>> v3 -> v4:
>> - update .gitignore
>> - More vgic-mmio-v3.c change into the previous patch
>> - rename fuzz_dist_rdist into test_dist_rdist
>> - cleanup in run_vcpu and guest_code
>> - max_ipa_bits is global
>> - s/fuzz/subtest
>> - added test_kvm_device,
>> - moved ucall_init() just before the cpu run
>> - use vm_create_default_with_vcpus
>> - use vm_gic struct, vm_gic_create, vm_gic_destroy
>> - revwrite util.c helpers to comply with the usual style
>> ---
>> tools/testing/selftests/kvm/.gitignore| 1 +
>> tools/testing/selftests/kvm/Makefile | 1 +
>> .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++
>> .../testing/selftests/kvm/include/kvm_util.h | 9 +
>> tools/testing/selftests/kvm/lib/kvm_util.c| 77 +++
>> 5 files changed, 673 insertions(+)
>> create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c
>>
>> diff --git a/tools/testing/selftests/kvm/.gitignore
>> b/tools/testing/selftests/kvm/.gitignore
>> index 7bd7e776c266..bb862f91f640 100644
>> --- a/tools/testing/selftests/kvm/.gitignore
>> +++ b/tools/testing/selftests/kvm/.gitignore
>> @@ -1,6 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> /aarch64/get-reg-list
>> /aarch64/get-reg-list-sve
>> +/aarch64/vgic_init
>> /s390x/memop
>> /s390x/resets
>> /s390x/sync_regs_test
>> diff --git a/tools/testing/selftests/kvm/Makefile
>> b/tools/testing/selftests/kvm/Makefile
>> index 67eebb53235f..2fd4801de9ca 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
>>
>> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
>> +TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
>> TEST_GEN_PROGS_aarch64 += demand_paging_test
>> TEST_GEN_PROGS_aarch64 += dirty_log_test
>> TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>> b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>> new file mode 100644
>> index ..be1a7c0d0527
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>> @@ -0,0 +1,585 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * vgic init sequence tests
>> + *
>> + * Copyright (C) 2020, Red Hat, Inc.
>> + */
>> +#define _GNU_SOURCE
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +#include "processor.h"
>> +
>> +#define NR_VCPUS4
>> +
>> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index)
>> (((uint64_t)(count) << 52) | \
>> +((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
>> +#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
>> +
>> +#define GICR_TYPER 0x8
>> +
>> +struct vm_gic {
>> +struct kvm_vm *vm;
>> +int gic_fd;
>> +};
>> +
>> +int max_ipa_bits;
>
> static
done
>
>> +
>> +/* helper to access a redistributor register */
>> +static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
>> + uint32_t *val, bool write)
>> +{
>> +uint64_t attr = REG_OFFSET(vcpu, offset);
>> +
>> +return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
>> + attr, val, write);
>> +}
>> +
>> +/* dummy guest code */
>> +static void guest_code(void)
>> +{
>> +GUEST_SYNC(0);
>> +GUEST_SYNC(1);
>> +GUEST_SYNC(2);
>> +GUEST_DONE();
>> +}
>> +
>> +/* we don't want to assert on run execution, hence that helper */
>> +static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
>> +{
>> +int ret;
>> +
>> +vcpu_args_set(vm, vcpuid, 1);
>
> You don't need the above vcpu_args_set call since guest_code doesn't take
> any arguments.
ok
>
>> +ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
>> +get_ucall(vm, vcpuid, NULL);
>
> You're not checking the result of get_ucall, so there's no need for the
> call.
Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
On Tue, 06 Apr 2021 16:09:16 +0100, Andrew Jones wrote: > > > Hi Eric, > > It looks like Marc already picked this patch up, but, FWIW, here's > a few more comments you may consider. FWIW, I'm happy to drop this patch from the queue (the series is on its own branch for this reason), or take reworks on top. But given that we're getting close to release time and this series has some userspace visibile impacts, I want to let it simmer in -next for a few days before sending the pull request. Thanks, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
Hi Eric,
It looks like Marc already picked this patch up, but, FWIW, here's
a few more comments you may consider.
On Mon, Apr 05, 2021 at 06:39:41PM +0200, Eric Auger wrote:
> The tests exercise the VGIC_V3 device creation including the
> associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:
>
> - KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
> - KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>
> Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
> and especially the GICR_TYPER read. The goal was to test the case
> recently fixed by commit 23bde34771f1
> ("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").
>
> The API under test can be found at
> Documentation/virt/kvm/devices/arm-vgic-v3.rst
>
> Signed-off-by: Eric Auger
>
> ---
>
> v4 -> v5:
> - simplify the last bit tests given the simpler interpretation
> of the spec
>
> v3 -> v4:
> - update .gitignore
> - More vgic-mmio-v3.c change into the previous patch
> - rename fuzz_dist_rdist into test_dist_rdist
> - cleanup in run_vcpu and guest_code
> - max_ipa_bits is global
> - s/fuzz/subtest
> - added test_kvm_device,
> - moved ucall_init() just before the cpu run
> - use vm_create_default_with_vcpus
> - use vm_gic struct, vm_gic_create, vm_gic_destroy
> - revwrite util.c helpers to comply with the usual style
> ---
> tools/testing/selftests/kvm/.gitignore| 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++
> .../testing/selftests/kvm/include/kvm_util.h | 9 +
> tools/testing/selftests/kvm/lib/kvm_util.c| 77 +++
> 5 files changed, 673 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore
> b/tools/testing/selftests/kvm/.gitignore
> index 7bd7e776c266..bb862f91f640 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> /aarch64/get-reg-list
> /aarch64/get-reg-list-sve
> +/aarch64/vgic_init
> /s390x/memop
> /s390x/resets
> /s390x/sync_regs_test
> diff --git a/tools/testing/selftests/kvm/Makefile
> b/tools/testing/selftests/kvm/Makefile
> index 67eebb53235f..2fd4801de9ca 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
>
> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
> +TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
> TEST_GEN_PROGS_aarch64 += demand_paging_test
> TEST_GEN_PROGS_aarch64 += dirty_log_test
> TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> new file mode 100644
> index ..be1a7c0d0527
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * vgic init sequence tests
> + *
> + * Copyright (C) 2020, Red Hat, Inc.
> + */
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define NR_VCPUS 4
> +
> +#define REDIST_REGION_ATTR_ADDR(count, base, flags, index)
> (((uint64_t)(count) << 52) | \
> + ((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
> +#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
> +
> +#define GICR_TYPER 0x8
> +
> +struct vm_gic {
> + struct kvm_vm *vm;
> + int gic_fd;
> +};
> +
> +int max_ipa_bits;
static
> +
> +/* helper to access a redistributor register */
> +static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> + uint32_t *val, bool write)
> +{
> + uint64_t attr = REG_OFFSET(vcpu, offset);
> +
> + return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
> + attr, val, write);
> +}
> +
> +/* dummy guest code */
> +static void guest_code(void)
> +{
> + GUEST_SYNC(0);
> + GUEST_SYNC(1);
> + GUEST_SYNC(2);
> + GUEST_DONE();
> +}
> +
> +/* we don't want to assert on run execution, hence that helper */
> +static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> + int ret;
> +
> + vcpu_args_set(vm, vcpuid, 1);
You don't need the above vcpu_args_set call since guest_code doesn't take
any arguments.
> + ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
> + get_ucall(vm, vcpuid, NULL);
You're not checking the result of get_ucall, so there's no need for the
call.
> +
> + if (ret)
> + return -errno;
> + return 0;
> +}
> +
> +static struct vm_gic vm_gic_create(void)
> +{
> + struct vm_gic v;
> +
> + v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> + v.gic_fd = kvm_create_device(v.vm,
[PATCH v6 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests
The tests exercise the VGIC_V3 device creation including the
associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:
- KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST
- KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
Some other tests dedicate to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
and especially the GICR_TYPER read. The goal was to test the case
recently fixed by commit 23bde34771f1
("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").
The API under test can be found at
Documentation/virt/kvm/devices/arm-vgic-v3.rst
Signed-off-by: Eric Auger
---
v4 -> v5:
- simplify the last bit tests given the simpler interpretation
of the spec
v3 -> v4:
- update .gitignore
- More vgic-mmio-v3.c change into the previous patch
- rename fuzz_dist_rdist into test_dist_rdist
- cleanup in run_vcpu and guest_code
- max_ipa_bits is global
- s/fuzz/subtest
- added test_kvm_device,
- moved ucall_init() just before the cpu run
- use vm_create_default_with_vcpus
- use vm_gic struct, vm_gic_create, vm_gic_destroy
- revwrite util.c helpers to comply with the usual style
---
tools/testing/selftests/kvm/.gitignore| 1 +
tools/testing/selftests/kvm/Makefile | 1 +
.../testing/selftests/kvm/aarch64/vgic_init.c | 585 ++
.../testing/selftests/kvm/include/kvm_util.h | 9 +
tools/testing/selftests/kvm/lib/kvm_util.c| 77 +++
5 files changed, 673 insertions(+)
create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c
diff --git a/tools/testing/selftests/kvm/.gitignore
b/tools/testing/selftests/kvm/.gitignore
index 7bd7e776c266..bb862f91f640 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
/aarch64/get-reg-list
/aarch64/get-reg-list-sve
+/aarch64/vgic_init
/s390x/memop
/s390x/resets
/s390x/sync_regs_test
diff --git a/tools/testing/selftests/kvm/Makefile
b/tools/testing/selftests/kvm/Makefile
index 67eebb53235f..2fd4801de9ca 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -78,6 +78,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
+TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
TEST_GEN_PROGS_aarch64 += demand_paging_test
TEST_GEN_PROGS_aarch64 += dirty_log_test
TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c
b/tools/testing/selftests/kvm/aarch64/vgic_init.c
new file mode 100644
index ..be1a7c0d0527
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vgic init sequence tests
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define NR_VCPUS 4
+
+#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) (((uint64_t)(count)
<< 52) | \
+ ((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
+#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
+
+#define GICR_TYPER 0x8
+
+struct vm_gic {
+ struct kvm_vm *vm;
+ int gic_fd;
+};
+
+int max_ipa_bits;
+
+/* helper to access a redistributor register */
+static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
+uint32_t *val, bool write)
+{
+ uint64_t attr = REG_OFFSET(vcpu, offset);
+
+ return _kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
+ attr, val, write);
+}
+
+/* dummy guest code */
+static void guest_code(void)
+{
+ GUEST_SYNC(0);
+ GUEST_SYNC(1);
+ GUEST_SYNC(2);
+ GUEST_DONE();
+}
+
+/* we don't want to assert on run execution, hence that helper */
+static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+ int ret;
+
+ vcpu_args_set(vm, vcpuid, 1);
+ ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
+ get_ucall(vm, vcpuid, NULL);
+
+ if (ret)
+ return -errno;
+ return 0;
+}
+
+static struct vm_gic vm_gic_create(void)
+{
+ struct vm_gic v;
+
+ v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
+ v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+ TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
+
+ return v;
+}
+
+static void vm_gic_destroy(struct vm_gic *v)
+{
+ close(v->gic_fd);
+ kvm_vm_free(v->vm);
+}
+
+/**
+ * Helper routine that performs KVM device tests in general and
+ * especially ARM_VGIC_V3 ones. Eventually the ARM_VGIC_V3
+ * device gets created, a legacy RDIST region is set at @0x0
+ * and a DIST region is set @0x6
+ */
+static void subtest_dist_rdist(struct vm_gic *v)
+{
+ int ret;
+ uint64_t addr;
+
+ /* Check existing group/attributes */
+ ret
