| Message ID | 20220330174621.1567317-3-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Add a cap to disable NX hugepages on a VM | expand |
On Wed, Mar 30, 2022 at 10:46 AM Ben Gardon <bgardon@google.com> wrote: > > Add kvm_util library functions to read KVM stats through the binary > stats interface and then dump them to stdout when running the binary > stats test. Subsequent commits will extend the kvm_util code and use it > to make assertions in a test for NX hugepages. > > CC: Jing Zhang <jingzhangos@google.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 1 + > .../selftests/kvm/kvm_binary_stats_test.c | 3 + > tools/testing/selftests/kvm/lib/kvm_util.c | 143 ++++++++++++++++++ > 3 files changed, 147 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 976aaaba8769..4783fd1cd4cf 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid); > > int vm_get_stats_fd(struct kvm_vm *vm); > int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid); > +void dump_vm_stats(struct kvm_vm *vm); > > uint32_t guest_get_vcpuid(void); > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > index 17f65d514915..afc4701ce8dd 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm) > stats_test(stats_fd); > close(stats_fd); > TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); > + > + /* Dump VM stats */ > + dump_vm_stats(vm); > } > > static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id) > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 11a692cf4570..f87df68b150d 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid) > > return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL); > } > + > +/* Caller is responsible for freeing the returned kvm_stats_header. */ > +static struct kvm_stats_header *read_vm_stats_header(int stats_fd) > +{ > + struct kvm_stats_header *header; > + ssize_t ret; > + > + /* Read kvm stats header */ > + header = malloc(sizeof(*header)); > + TEST_ASSERT(header, "Allocate memory for stats header"); > + > + ret = read(stats_fd, header, sizeof(*header)); > + TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > + > + return header; > +} > + > +static void dump_header(int stats_fd, struct kvm_stats_header *header) > +{ > + ssize_t ret; > + char *id; > + > + printf("flags: %u\n", header->flags); > + printf("name size: %u\n", header->name_size); > + printf("num_desc: %u\n", header->num_desc); > + printf("id_offset: %u\n", header->id_offset); > + printf("desc_offset: %u\n", header->desc_offset); > + printf("data_offset: %u\n", header->data_offset); > + > + /* Read kvm stats id string */ > + id = malloc(header->name_size); > + TEST_ASSERT(id, "Allocate memory for id string"); > + ret = pread(stats_fd, id, header->name_size, header->id_offset); > + TEST_ASSERT(ret == header->name_size, "Read id string"); > + > + printf("id: %s\n", id); > + > + free(id); > +} > + > +static ssize_t stats_desc_size(struct kvm_stats_header *header) > +{ > + return sizeof(struct kvm_stats_desc) + header->name_size; > +} > + > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header) > +{ > + struct kvm_stats_desc *stats_desc; > + size_t size_desc; > + ssize_t ret; > + > + size_desc = header->num_desc * stats_desc_size(header); > + > + /* Allocate memory for stats descriptors */ > + stats_desc = malloc(size_desc); > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > + > + /* Read kvm stats descriptors */ > + ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset); > + TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors"); > + > + return stats_desc; > +} > + > +/* Caller is responsible for freeing the memory *data. */ > +static int read_stat_data(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc, uint64_t **data) > +{ > + u64 *stats_data; > + ssize_t ret; > + > + stats_data = malloc(desc->size * sizeof(*stats_data)); > + > + ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data), > + header->data_offset + desc->offset); > + > + /* ret is in bytes. */ > + ret = ret / sizeof(*stats_data); > + > + TEST_ASSERT(ret == desc->size, > + "Read data of KVM stats: %s", desc->name); > + > + *data = stats_data; > + > + return ret; > +} > + > +static void dump_stat(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc) > +{ > + u64 *stats_data; > + ssize_t ret; > + int i; > + > + printf("\tflags: %u\n", desc->flags); > + printf("\texponent: %u\n", desc->exponent); > + printf("\tsize: %u\n", desc->size); > + printf("\toffset: %u\n", desc->offset); > + printf("\tbucket_size: %u\n", desc->bucket_size); > + printf("\tname: %s\n", (char *)&desc->name); > + > + ret = read_stat_data(stats_fd, header, desc, &stats_data); > + > + printf("\tdata: %lu", *stats_data); > + for (i = 1; i < ret; i++) > + printf(", %lu", *(stats_data + i)); > + printf("\n\n"); > + > + free(stats_data); > +} > + > +void dump_vm_stats(struct kvm_vm *vm) > +{ > + struct kvm_stats_desc *stats_desc; > + struct kvm_stats_header *header; > + struct kvm_stats_desc *desc; > + size_t size_desc; > + int stats_fd; > + int i; > + > + stats_fd = vm_get_stats_fd(vm); > + > + header = read_vm_stats_header(stats_fd); > + dump_header(stats_fd, header); > + > + stats_desc = read_vm_stats_desc(stats_fd, header); > + > + size_desc = stats_desc_size(header); > + > + /* Read kvm stats data one by one */ > + for (i = 0; i < header->num_desc; ++i) { > + desc = (void *)stats_desc + (i * size_desc); > + dump_stat(stats_fd, header, desc); > + } > + > + free(stats_desc); > + free(header); > + > + close(stats_fd); > +} > + > -- > 2.35.1.1021.g381101b075-goog > Reviewed-by: Jing Zhang <jingzhangos@google.com>
On Wed, Mar 30, 2022 at 10:46:12AM -0700, Ben Gardon wrote: > Add kvm_util library functions to read KVM stats through the binary > stats interface and then dump them to stdout when running the binary > stats test. Subsequent commits will extend the kvm_util code and use it > to make assertions in a test for NX hugepages. > > CC: Jing Zhang <jingzhangos@google.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 1 + > .../selftests/kvm/kvm_binary_stats_test.c | 3 + > tools/testing/selftests/kvm/lib/kvm_util.c | 143 ++++++++++++++++++ > 3 files changed, 147 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 976aaaba8769..4783fd1cd4cf 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid); > > int vm_get_stats_fd(struct kvm_vm *vm); > int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid); > +void dump_vm_stats(struct kvm_vm *vm); > > uint32_t guest_get_vcpuid(void); > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > index 17f65d514915..afc4701ce8dd 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm) > stats_test(stats_fd); > close(stats_fd); > TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); > + > + /* Dump VM stats */ > + dump_vm_stats(vm); > } > > static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id) > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 11a692cf4570..f87df68b150d 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid) > > return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL); > } > + > +/* Caller is responsible for freeing the returned kvm_stats_header. */ > +static struct kvm_stats_header *read_vm_stats_header(int stats_fd) > +{ > + struct kvm_stats_header *header; > + ssize_t ret; > + > + /* Read kvm stats header */ > + header = malloc(sizeof(*header)); > + TEST_ASSERT(header, "Allocate memory for stats header"); > + > + ret = read(stats_fd, header, sizeof(*header)); > + TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > + > + return header; > +} It seems like this helper could be used in kvm_binary_stats_test.c to eliminate duplicate code. > + > +static void dump_header(int stats_fd, struct kvm_stats_header *header) > +{ > + ssize_t ret; > + char *id; > + > + printf("flags: %u\n", header->flags); > + printf("name size: %u\n", header->name_size); > + printf("num_desc: %u\n", header->num_desc); > + printf("id_offset: %u\n", header->id_offset); > + printf("desc_offset: %u\n", header->desc_offset); > + printf("data_offset: %u\n", header->data_offset); > + > + /* Read kvm stats id string */ > + id = malloc(header->name_size); > + TEST_ASSERT(id, "Allocate memory for id string"); > + ret = pread(stats_fd, id, header->name_size, header->id_offset); > + TEST_ASSERT(ret == header->name_size, "Read id string"); > + > + printf("id: %s\n", id); > + > + free(id); > +} > + > +static ssize_t stats_desc_size(struct kvm_stats_header *header) > +{ > + return sizeof(struct kvm_stats_desc) + header->name_size; > +} > + > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header) > +{ > + struct kvm_stats_desc *stats_desc; > + size_t size_desc; > + ssize_t ret; > + > + size_desc = header->num_desc * stats_desc_size(header); > + > + /* Allocate memory for stats descriptors */ > + stats_desc = malloc(size_desc); > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > + > + /* Read kvm stats descriptors */ > + ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset); > + TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors"); > + > + return stats_desc; > +} Same with this helper. > + > +/* Caller is responsible for freeing the memory *data. */ > +static int read_stat_data(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc, uint64_t **data) > +{ > + u64 *stats_data; > + ssize_t ret; > + > + stats_data = malloc(desc->size * sizeof(*stats_data)); > + > + ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data), > + header->data_offset + desc->offset); > + > + /* ret is in bytes. */ > + ret = ret / sizeof(*stats_data); > + > + TEST_ASSERT(ret == desc->size, > + "Read data of KVM stats: %s", desc->name); > + > + *data = stats_data; > + > + return ret; > +} Same with this helper. > + > +static void dump_stat(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *desc) > +{ > + u64 *stats_data; > + ssize_t ret; > + int i; > + > + printf("\tflags: %u\n", desc->flags); > + printf("\texponent: %u\n", desc->exponent); > + printf("\tsize: %u\n", desc->size); > + printf("\toffset: %u\n", desc->offset); > + printf("\tbucket_size: %u\n", desc->bucket_size); > + printf("\tname: %s\n", (char *)&desc->name); > + > + ret = read_stat_data(stats_fd, header, desc, &stats_data); > + > + printf("\tdata: %lu", *stats_data); > + for (i = 1; i < ret; i++) > + printf(", %lu", *(stats_data + i)); > + printf("\n\n"); > + > + free(stats_data); > +} > + > +void dump_vm_stats(struct kvm_vm *vm) > +{ > + struct kvm_stats_desc *stats_desc; > + struct kvm_stats_header *header; > + struct kvm_stats_desc *desc; > + size_t size_desc; > + int stats_fd; > + int i; > + > + stats_fd = vm_get_stats_fd(vm); > + > + header = read_vm_stats_header(stats_fd); > + dump_header(stats_fd, header); > + > + stats_desc = read_vm_stats_desc(stats_fd, header); > + > + size_desc = stats_desc_size(header); > + > + /* Read kvm stats data one by one */ > + for (i = 0; i < header->num_desc; ++i) { > + desc = (void *)stats_desc + (i * size_desc); > + dump_stat(stats_fd, header, desc); > + } > + > + free(stats_desc); > + free(header); > + > + close(stats_fd); > +} > + > -- > 2.35.1.1021.g381101b075-goog >
On Tue, Apr 5, 2022 at 3:19 PM David Matlack <dmatlack@google.com> wrote: > > On Wed, Mar 30, 2022 at 10:46:12AM -0700, Ben Gardon wrote: > > Add kvm_util library functions to read KVM stats through the binary > > stats interface and then dump them to stdout when running the binary > > stats test. Subsequent commits will extend the kvm_util code and use it > > to make assertions in a test for NX hugepages. > > > > CC: Jing Zhang <jingzhangos@google.com> > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > .../selftests/kvm/include/kvm_util_base.h | 1 + > > .../selftests/kvm/kvm_binary_stats_test.c | 3 + > > tools/testing/selftests/kvm/lib/kvm_util.c | 143 ++++++++++++++++++ > > 3 files changed, 147 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index 976aaaba8769..4783fd1cd4cf 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid); > > > > int vm_get_stats_fd(struct kvm_vm *vm); > > int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid); > > +void dump_vm_stats(struct kvm_vm *vm); > > > > uint32_t guest_get_vcpuid(void); > > > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > index 17f65d514915..afc4701ce8dd 100644 > > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm) > > stats_test(stats_fd); > > close(stats_fd); > > TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); > > + > > + /* Dump VM stats */ > > + dump_vm_stats(vm); > > } > > > > static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id) > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index 11a692cf4570..f87df68b150d 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid) > > > > return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL); > > } > > + > > +/* Caller is responsible for freeing the returned kvm_stats_header. */ > > +static struct kvm_stats_header *read_vm_stats_header(int stats_fd) > > +{ > > + struct kvm_stats_header *header; > > + ssize_t ret; > > + > > + /* Read kvm stats header */ > > + header = malloc(sizeof(*header)); > > + TEST_ASSERT(header, "Allocate memory for stats header"); > > + > > + ret = read(stats_fd, header, sizeof(*header)); > > + TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > > + > > + return header; > > +} > > It seems like this helper could be used in kvm_binary_stats_test.c to > eliminate duplicate code. It could, but I think the duplicate code in that test has value in being verbose and well commented and having a bunch of checks to assert things the regular library function isn't interested in. I'd prefer to keep the duplication as-is. > > > + > > +static void dump_header(int stats_fd, struct kvm_stats_header *header) > > +{ > > + ssize_t ret; > > + char *id; > > + > > + printf("flags: %u\n", header->flags); > > + printf("name size: %u\n", header->name_size); > > + printf("num_desc: %u\n", header->num_desc); > > + printf("id_offset: %u\n", header->id_offset); > > + printf("desc_offset: %u\n", header->desc_offset); > > + printf("data_offset: %u\n", header->data_offset); > > + > > + /* Read kvm stats id string */ > > + id = malloc(header->name_size); > > + TEST_ASSERT(id, "Allocate memory for id string"); > > + ret = pread(stats_fd, id, header->name_size, header->id_offset); > > + TEST_ASSERT(ret == header->name_size, "Read id string"); > > + > > + printf("id: %s\n", id); > > + > > + free(id); > > +} > > + > > +static ssize_t stats_desc_size(struct kvm_stats_header *header) > > +{ > > + return sizeof(struct kvm_stats_desc) + header->name_size; > > +} > > + > > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > > +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd, > > + struct kvm_stats_header *header) > > +{ > > + struct kvm_stats_desc *stats_desc; > > + size_t size_desc; > > + ssize_t ret; > > + > > + size_desc = header->num_desc * stats_desc_size(header); > > + > > + /* Allocate memory for stats descriptors */ > > + stats_desc = malloc(size_desc); > > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > > + > > + /* Read kvm stats descriptors */ > > + ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset); > > + TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors"); > > + > > + return stats_desc; > > +} > > Same with this helper. > > > + > > +/* Caller is responsible for freeing the memory *data. */ > > +static int read_stat_data(int stats_fd, struct kvm_stats_header *header, > > + struct kvm_stats_desc *desc, uint64_t **data) > > +{ > > + u64 *stats_data; > > + ssize_t ret; > > + > > + stats_data = malloc(desc->size * sizeof(*stats_data)); > > + > > + ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data), > > + header->data_offset + desc->offset); > > + > > + /* ret is in bytes. */ > > + ret = ret / sizeof(*stats_data); > > + > > + TEST_ASSERT(ret == desc->size, > > + "Read data of KVM stats: %s", desc->name); > > + > > + *data = stats_data; > > + > > + return ret; > > +} > > Same with this helper. > > > + > > +static void dump_stat(int stats_fd, struct kvm_stats_header *header, > > + struct kvm_stats_desc *desc) > > +{ > > + u64 *stats_data; > > + ssize_t ret; > > + int i; > > + > > + printf("\tflags: %u\n", desc->flags); > > + printf("\texponent: %u\n", desc->exponent); > > + printf("\tsize: %u\n", desc->size); > > + printf("\toffset: %u\n", desc->offset); > > + printf("\tbucket_size: %u\n", desc->bucket_size); > > + printf("\tname: %s\n", (char *)&desc->name); > > + > > + ret = read_stat_data(stats_fd, header, desc, &stats_data); > > + > > + printf("\tdata: %lu", *stats_data); > > + for (i = 1; i < ret; i++) > > + printf(", %lu", *(stats_data + i)); > > + printf("\n\n"); > > + > > + free(stats_data); > > +} > > + > > +void dump_vm_stats(struct kvm_vm *vm) > > +{ > > + struct kvm_stats_desc *stats_desc; > > + struct kvm_stats_header *header; > > + struct kvm_stats_desc *desc; > > + size_t size_desc; > > + int stats_fd; > > + int i; > > + > > + stats_fd = vm_get_stats_fd(vm); > > + > > + header = read_vm_stats_header(stats_fd); > > + dump_header(stats_fd, header); > > + > > + stats_desc = read_vm_stats_desc(stats_fd, header); > > + > > + size_desc = stats_desc_size(header); > > + > > + /* Read kvm stats data one by one */ > > + for (i = 0; i < header->num_desc; ++i) { > > + desc = (void *)stats_desc + (i * size_desc); > > + dump_stat(stats_fd, header, desc); > > + } > > + > > + free(stats_desc); > > + free(header); > > + > > + close(stats_fd); > > +} > > + > > -- > > 2.35.1.1021.g381101b075-goog > >
On Wed, Mar 30, 2022, Ben Gardon wrote: > Add kvm_util library functions to read KVM stats through the binary > stats interface and then dump them to stdout when running the binary > stats test. Subsequent commits will extend the kvm_util code and use it > to make assertions in a test for NX hugepages. Why? Spamming my console with info that has zero meaning to me and is useless when the test passes is not helpful. Even on failure, I don't see what the user is going to do with this information, all of the asserts are completly unrelated to the stats themselves.
On Fri, Apr 8, 2022 at 12:52 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Mar 30, 2022, Ben Gardon wrote: > > Add kvm_util library functions to read KVM stats through the binary > > stats interface and then dump them to stdout when running the binary > > stats test. Subsequent commits will extend the kvm_util code and use it > > to make assertions in a test for NX hugepages. > > Why? Spamming my console with info that has zero meaning to me and is useless > when the test passes is not helpful. Even on failure, I don't see what the user > is going to do with this information, all of the asserts are completly unrelated > to the stats themselves. Debugging could be another reason, I suspect? I remember when I tried to use the interface, there is really no API that tells me "did I add this stat successfully and/or correctly?" I think having a general print so that developer/debugging folk could just 'grep mystat' to verify that would be helpful in the future. Otherwise, they have to write code themselves to do the dirty print...
On Thu, Jun 30, 2022, Mingwei Zhang wrote: > On Fri, Apr 8, 2022 at 12:52 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Wed, Mar 30, 2022, Ben Gardon wrote: > > > Add kvm_util library functions to read KVM stats through the binary > > > stats interface and then dump them to stdout when running the binary > > > stats test. Subsequent commits will extend the kvm_util code and use it > > > to make assertions in a test for NX hugepages. > > > > Why? Spamming my console with info that has zero meaning to me and is useless > > when the test passes is not helpful. Even on failure, I don't see what the user > > is going to do with this information, all of the asserts are completly unrelated > > to the stats themselves. > > Debugging could be another reason, I suspect? I remember when I tried > to use the interface, there is really no API that tells me "did I add > this stat successfully and/or correctly?" I think having a general > print so that developer/debugging folk could just 'grep mystat' to > verify that would be helpful in the future. > > Otherwise, they have to write code themselves to do the dirty print... I've no objection to adding a --verbose option or a #define of some form, but make it opt-in, not on by default.
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 976aaaba8769..4783fd1cd4cf 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid); int vm_get_stats_fd(struct kvm_vm *vm); int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid); +void dump_vm_stats(struct kvm_vm *vm); uint32_t guest_get_vcpuid(void); diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index 17f65d514915..afc4701ce8dd 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm) stats_test(stats_fd); close(stats_fd); TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed"); + + /* Dump VM stats */ + dump_vm_stats(vm); } static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 11a692cf4570..f87df68b150d 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid) return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL); } + +/* Caller is responsible for freeing the returned kvm_stats_header. */ +static struct kvm_stats_header *read_vm_stats_header(int stats_fd) +{ + struct kvm_stats_header *header; + ssize_t ret; + + /* Read kvm stats header */ + header = malloc(sizeof(*header)); + TEST_ASSERT(header, "Allocate memory for stats header"); + + ret = read(stats_fd, header, sizeof(*header)); + TEST_ASSERT(ret == sizeof(*header), "Read stats header"); + + return header; +} + +static void dump_header(int stats_fd, struct kvm_stats_header *header) +{ + ssize_t ret; + char *id; + + printf("flags: %u\n", header->flags); + printf("name size: %u\n", header->name_size); + printf("num_desc: %u\n", header->num_desc); + printf("id_offset: %u\n", header->id_offset); + printf("desc_offset: %u\n", header->desc_offset); + printf("data_offset: %u\n", header->data_offset); + + /* Read kvm stats id string */ + id = malloc(header->name_size); + TEST_ASSERT(id, "Allocate memory for id string"); + ret = pread(stats_fd, id, header->name_size, header->id_offset); + TEST_ASSERT(ret == header->name_size, "Read id string"); + + printf("id: %s\n", id); + + free(id); +} + +static ssize_t stats_desc_size(struct kvm_stats_header *header) +{ + return sizeof(struct kvm_stats_desc) + header->name_size; +} + +/* Caller is responsible for freeing the returned kvm_stats_desc. */ +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd, + struct kvm_stats_header *header) +{ + struct kvm_stats_desc *stats_desc; + size_t size_desc; + ssize_t ret; + + size_desc = header->num_desc * stats_desc_size(header); + + /* Allocate memory for stats descriptors */ + stats_desc = malloc(size_desc); + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); + + /* Read kvm stats descriptors */ + ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset); + TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors"); + + return stats_desc; +} + +/* Caller is responsible for freeing the memory *data. */ +static int read_stat_data(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *desc, uint64_t **data) +{ + u64 *stats_data; + ssize_t ret; + + stats_data = malloc(desc->size * sizeof(*stats_data)); + + ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data), + header->data_offset + desc->offset); + + /* ret is in bytes. */ + ret = ret / sizeof(*stats_data); + + TEST_ASSERT(ret == desc->size, + "Read data of KVM stats: %s", desc->name); + + *data = stats_data; + + return ret; +} + +static void dump_stat(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *desc) +{ + u64 *stats_data; + ssize_t ret; + int i; + + printf("\tflags: %u\n", desc->flags); + printf("\texponent: %u\n", desc->exponent); + printf("\tsize: %u\n", desc->size); + printf("\toffset: %u\n", desc->offset); + printf("\tbucket_size: %u\n", desc->bucket_size); + printf("\tname: %s\n", (char *)&desc->name); + + ret = read_stat_data(stats_fd, header, desc, &stats_data); + + printf("\tdata: %lu", *stats_data); + for (i = 1; i < ret; i++) + printf(", %lu", *(stats_data + i)); + printf("\n\n"); + + free(stats_data); +} + +void dump_vm_stats(struct kvm_vm *vm) +{ + struct kvm_stats_desc *stats_desc; + struct kvm_stats_header *header; + struct kvm_stats_desc *desc; + size_t size_desc; + int stats_fd; + int i; + + stats_fd = vm_get_stats_fd(vm); + + header = read_vm_stats_header(stats_fd); + dump_header(stats_fd, header); + + stats_desc = read_vm_stats_desc(stats_fd, header); + + size_desc = stats_desc_size(header); + + /* Read kvm stats data one by one */ + for (i = 0; i < header->num_desc; ++i) { + desc = (void *)stats_desc + (i * size_desc); + dump_stat(stats_fd, header, desc); + } + + free(stats_desc); + free(header); + + close(stats_fd); +} +
Add kvm_util library functions to read KVM stats through the binary stats interface and then dump them to stdout when running the binary stats test. Subsequent commits will extend the kvm_util code and use it to make assertions in a test for NX hugepages. CC: Jing Zhang <jingzhangos@google.com> Signed-off-by: Ben Gardon <bgardon@google.com> --- .../selftests/kvm/include/kvm_util_base.h | 1 + .../selftests/kvm/kvm_binary_stats_test.c | 3 + tools/testing/selftests/kvm/lib/kvm_util.c | 143 ++++++++++++++++++ 3 files changed, 147 insertions(+)