| Message ID | 20220411211015.3091615-4-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 Mon, Apr 11, 2022 at 2:10 PM Ben Gardon <bgardon@google.com> wrote: > > Move the code to read the binary stats descriptors to the KVM selftests > library. It will be re-used by other tests to check KVM behavior. > > No functional change intended. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 4 +++ > .../selftests/kvm/kvm_binary_stats_test.c | 9 ++---- > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++++ > 3 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 5ba3132f3110..c5f34551ff76 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -401,6 +401,10 @@ 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 read_vm_stats_header(int stats_fd, struct kvm_stats_header *header); > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header); > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *stats_desc); Same feedback here as the previous patch about getting rid of "vm_" in the function names. > > 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 22c22a90f15a..e4795bad7db6 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -62,14 +62,9 @@ static void stats_test(int stats_fd) > header.data_offset), > "Descriptor block is overlapped with data block"); > > - /* Allocate memory for stats descriptors */ > - stats_desc = calloc(header.num_desc, size_desc); > - TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > /* Read kvm stats descriptors */ > - ret = pread(stats_fd, stats_desc, > - size_desc * header.num_desc, header.desc_offset); > - TEST_ASSERT(ret == size_desc * header.num_desc, > - "Read KVM stats descriptors"); > + stats_desc = alloc_vm_stats_desc(stats_fd, &header); > + read_vm_stats_desc(stats_fd, &header, stats_desc); > > /* Sanity check for fields in descriptors */ > for (i = 0; i < header.num_desc; ++i) { > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 0caf28e324ed..e3ae26fbef03 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2564,3 +2564,32 @@ void read_vm_stats_header(int stats_fd, struct kvm_stats_header *header) > ret = read(stats_fd, header, sizeof(*header)); > TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > } > + > +static ssize_t stats_descs_size(struct kvm_stats_header *header) > +{ > + return header->num_desc * > + (sizeof(struct kvm_stats_desc) + header->name_size); > +} > + > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header) > +{ > + struct kvm_stats_desc *stats_desc; > + > + stats_desc = malloc(stats_descs_size(header)); > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > + > + return stats_desc; > +} > + > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *stats_desc) Reading the descriptors only needs to be done once per fd. So from an API point of view I don't think there's value in creating separate functions for the allocation and the read; just combine them in one. > +{ > + ssize_t ret; > + > + ret = pread(stats_fd, stats_desc, stats_descs_size(header), > + header->desc_offset); > + TEST_ASSERT(ret == stats_descs_size(header), > + "Read KVM stats descriptors"); > +} > -- > 2.35.1.1178.g4f1659d476-goog >
On Mon, Apr 11, 2022, Ben Gardon wrote: > Move the code to read the binary stats descriptors to the KVM selftests > library. It will be re-used by other tests to check KVM behavior. > > No functional change intended. > > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > .../selftests/kvm/include/kvm_util_base.h | 4 +++ > .../selftests/kvm/kvm_binary_stats_test.c | 9 ++---- > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++++ > 3 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > index 5ba3132f3110..c5f34551ff76 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > @@ -401,6 +401,10 @@ 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 read_vm_stats_header(int stats_fd, struct kvm_stats_header *header); > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header); > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *stats_desc); > > 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 22c22a90f15a..e4795bad7db6 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -62,14 +62,9 @@ static void stats_test(int stats_fd) > header.data_offset), > "Descriptor block is overlapped with data block"); > > - /* Allocate memory for stats descriptors */ > - stats_desc = calloc(header.num_desc, size_desc); > - TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > /* Read kvm stats descriptors */ > - ret = pread(stats_fd, stats_desc, > - size_desc * header.num_desc, header.desc_offset); > - TEST_ASSERT(ret == size_desc * header.num_desc, > - "Read KVM stats descriptors"); > + stats_desc = alloc_vm_stats_desc(stats_fd, &header); > + read_vm_stats_desc(stats_fd, &header, stats_desc); > > /* Sanity check for fields in descriptors */ > for (i = 0; i < header.num_desc; ++i) { > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 0caf28e324ed..e3ae26fbef03 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2564,3 +2564,32 @@ void read_vm_stats_header(int stats_fd, struct kvm_stats_header *header) > ret = read(stats_fd, header, sizeof(*header)); > TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > } > + > +static ssize_t stats_descs_size(struct kvm_stats_header *header) > +{ > + return header->num_desc * > + (sizeof(struct kvm_stats_desc) + header->name_size); > +} I was very confused on header->name_size. So this field means the maximum string size of a stats name, right? Can we update the comments in the kvm.h to specify that? By reading the comments, I don't really feel this is how we should use this field. hmm, if that is true, isn't this field a compile time value? Why do we have to get it at runtime? > + > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > + struct kvm_stats_header *header) > +{ > + struct kvm_stats_desc *stats_desc; > + > + stats_desc = malloc(stats_descs_size(header)); > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > + > + return stats_desc; > +} > + > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > + struct kvm_stats_desc *stats_desc) > +{ > + ssize_t ret; > + > + ret = pread(stats_fd, stats_desc, stats_descs_size(header), > + header->desc_offset); > + TEST_ASSERT(ret == stats_descs_size(header), > + "Read KVM stats descriptors"); > +} > -- > 2.35.1.1178.g4f1659d476-goog >
On Mon, Apr 11, 2022 at 5:55 PM Mingwei Zhang <mizhang@google.com> wrote: > > On Mon, Apr 11, 2022, Ben Gardon wrote: > > Move the code to read the binary stats descriptors to the KVM selftests > > library. It will be re-used by other tests to check KVM behavior. > > > > No functional change intended. > > > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > .../selftests/kvm/include/kvm_util_base.h | 4 +++ > > .../selftests/kvm/kvm_binary_stats_test.c | 9 ++---- > > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++++ > > 3 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h > > index 5ba3132f3110..c5f34551ff76 100644 > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h > > @@ -401,6 +401,10 @@ 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 read_vm_stats_header(int stats_fd, struct kvm_stats_header *header); > > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > > + struct kvm_stats_header *header); > > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > > + struct kvm_stats_desc *stats_desc); > > > > 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 22c22a90f15a..e4795bad7db6 100644 > > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > > @@ -62,14 +62,9 @@ static void stats_test(int stats_fd) > > header.data_offset), > > "Descriptor block is overlapped with data block"); > > > > - /* Allocate memory for stats descriptors */ > > - stats_desc = calloc(header.num_desc, size_desc); > > - TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > > /* Read kvm stats descriptors */ > > - ret = pread(stats_fd, stats_desc, > > - size_desc * header.num_desc, header.desc_offset); > > - TEST_ASSERT(ret == size_desc * header.num_desc, > > - "Read KVM stats descriptors"); > > + stats_desc = alloc_vm_stats_desc(stats_fd, &header); > > + read_vm_stats_desc(stats_fd, &header, stats_desc); > > > > /* Sanity check for fields in descriptors */ > > for (i = 0; i < header.num_desc; ++i) { > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > > index 0caf28e324ed..e3ae26fbef03 100644 > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > > @@ -2564,3 +2564,32 @@ void read_vm_stats_header(int stats_fd, struct kvm_stats_header *header) > > ret = read(stats_fd, header, sizeof(*header)); > > TEST_ASSERT(ret == sizeof(*header), "Read stats header"); > > } > > + > > +static ssize_t stats_descs_size(struct kvm_stats_header *header) > > +{ > > + return header->num_desc * > > + (sizeof(struct kvm_stats_desc) + header->name_size); > > +} > I was very confused on header->name_size. So this field means the > maximum string size of a stats name, right? Can we update the comments > in the kvm.h to specify that? By reading the comments, I don't really > feel this is how we should use this field. I believe that's right. I agree the documentation on that was a little confusing. > > hmm, if that is true, isn't this field a compile time value? Why do we > have to get it at runtime? It's compile time for the kernel but not for the userspace binaries which ultimately consume the stats. We could cheat in this selftest perhaps, but then it wouldn't be as good of a test. > > > + > > +/* Caller is responsible for freeing the returned kvm_stats_desc. */ > > +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, > > + struct kvm_stats_header *header) > > +{ > > + struct kvm_stats_desc *stats_desc; > > + > > + stats_desc = malloc(stats_descs_size(header)); > > + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); > > + > > + return stats_desc; > > +} > > + > > +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, > > + struct kvm_stats_desc *stats_desc) > > +{ > > + ssize_t ret; > > + > > + ret = pread(stats_fd, stats_desc, stats_descs_size(header), > > + header->desc_offset); > > + TEST_ASSERT(ret == stats_descs_size(header), > > + "Read KVM stats descriptors"); > > +} > > -- > > 2.35.1.1178.g4f1659d476-goog > >
On Tue, Apr 12, 2022, Ben Gardon wrote: > On Mon, Apr 11, 2022 at 5:55 PM Mingwei Zhang <mizhang@google.com> wrote: > > I was very confused on header->name_size. So this field means the > > maximum string size of a stats name, right? Can we update the comments > > in the kvm.h to specify that? By reading the comments, I don't really > > feel this is how we should use this field. > > I believe that's right. I agree the documentation on that was a little > confusing. Heh, a little. I got tripped up looking at this too. Give me a few minutes and I'll attach a cleanup patch to add comments and fix the myriad style issues. This whole file is painful to look at. Aside from violating preferred kernel style, it's horribly consistent with itself. Well, except for the 80 char limit, to which it has a fanatical devotion.
On Tue, Apr 12, 2022, Sean Christopherson wrote: > On Tue, Apr 12, 2022, Ben Gardon wrote: > > On Mon, Apr 11, 2022 at 5:55 PM Mingwei Zhang <mizhang@google.com> wrote: > > > I was very confused on header->name_size. So this field means the > > > maximum string size of a stats name, right? Can we update the comments > > > in the kvm.h to specify that? By reading the comments, I don't really > > > feel this is how we should use this field. > > > > I believe that's right. I agree the documentation on that was a little > > confusing. > > Heh, a little. I got tripped up looking at this too. Give me a few minutes and > I'll attach a cleanup patch to add comments and fix the myriad style issues. > > This whole file is painful to look at. Aside from violating preferred kernel > style, it's horribly consistent with itself. Well, except for the 80 char limit, > to which it has a fanatical devotion. If you send a new version of this series, can you add this on top? From: Sean Christopherson <seanjc@google.com> Date: Tue, 12 Apr 2022 11:49:59 -0700 Subject: [PATCH] KVM: selftests: Clean up coding style in binary stats test Fix a variety of code style violations and/or inconsistencies in the binary stats test. The 80 char limit is a soft limit and can and should be ignored/violated if doing so improves the overall code readability. Specifically, provide consistent indentation and don't split expressions at arbitrary points just to honor the 80 char limit. Opportunistically expand/add comments to call out the more subtle aspects of the code. Signed-off-by: Sean Christopherson <seanjc@google.com> --- .../selftests/kvm/kvm_binary_stats_test.c | 91 ++++++++++++------- 1 file changed, 56 insertions(+), 35 deletions(-) diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c index 97b180249ba0..944ed52e3f07 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -37,32 +37,42 @@ static void stats_test(int stats_fd) /* Read kvm stats header */ read_vm_stats_header(stats_fd, &header); + /* + * The base size of the descriptor is defined by KVM's ABI, but the + * size of the name field is variable as far as KVM's ABI is concerned. + * But, the size of name is constant for a given instance of KVM and + * is provided by KVM in the overall stats header. + */ size_desc = sizeof(*stats_desc) + header.name_size; /* Read kvm stats id string */ id = malloc(header.name_size); TEST_ASSERT(id, "Allocate memory for id string"); + ret = read(stats_fd, id, header.name_size); TEST_ASSERT(ret == header.name_size, "Read id string"); /* Check id string, that should start with "kvm" */ TEST_ASSERT(!strncmp(id, "kvm", 3) && strlen(id) < header.name_size, - "Invalid KVM stats type, id: %s", id); + "Invalid KVM stats type, id: %s", id); /* Sanity check for other fields in header */ if (header.num_desc == 0) { printf("No KVM stats defined!"); return; } - /* Check overlap */ - TEST_ASSERT(header.desc_offset > 0 && header.data_offset > 0 - && header.desc_offset >= sizeof(header) - && header.data_offset >= sizeof(header), - "Invalid offset fields in header"); + /* + * The descriptor and data offsets must be valid, they must not overlap + * the header, and the descriptor and data blocks must not overlap each + * other. Note, the data block is rechecked after its size is known. + */ + TEST_ASSERT(header.desc_offset && header.desc_offset >= sizeof(header) && + header.data_offset && header.data_offset >= sizeof(header), + "Invalid offset fields in header"); + TEST_ASSERT(header.desc_offset > header.data_offset || - (header.desc_offset + size_desc * header.num_desc <= - header.data_offset), - "Descriptor block is overlapped with data block"); + (header.desc_offset + size_desc * header.num_desc <= header.data_offset), + "Descriptor block is overlapped with data block"); /* Read kvm stats descriptors */ stats_desc = alloc_vm_stats_desc(stats_fd, &header); @@ -70,15 +80,22 @@ static void stats_test(int stats_fd) /* Sanity check for fields in descriptors */ for (i = 0; i < header.num_desc; ++i) { + /* + * Note, size_desc includes the of the name field, which is + * variable, i.e. this is NOT equivalent to &stats_desc[i]. + */ pdesc = (void *)stats_desc + i * size_desc; - /* Check type,unit,base boundaries */ - TEST_ASSERT((pdesc->flags & KVM_STATS_TYPE_MASK) - <= KVM_STATS_TYPE_MAX, "Unknown KVM stats type"); - TEST_ASSERT((pdesc->flags & KVM_STATS_UNIT_MASK) - <= KVM_STATS_UNIT_MAX, "Unknown KVM stats unit"); - TEST_ASSERT((pdesc->flags & KVM_STATS_BASE_MASK) - <= KVM_STATS_BASE_MAX, "Unknown KVM stats base"); - /* Check exponent for stats unit + + /* Check type, unit, and base boundaries */ + TEST_ASSERT((pdesc->flags & KVM_STATS_TYPE_MASK) <= KVM_STATS_TYPE_MAX, + "Unknown KVM stats type"); + TEST_ASSERT((pdesc->flags & KVM_STATS_UNIT_MASK) <= KVM_STATS_UNIT_MAX, + "Unknown KVM stats unit"); + TEST_ASSERT((pdesc->flags & KVM_STATS_BASE_MASK) <= KVM_STATS_BASE_MAX, + "Unknown KVM stats base"); + + /* + * Check exponent for stats unit * Exponent for counter should be greater than or equal to 0 * Exponent for unit bytes should be greater than or equal to 0 * Exponent for unit seconds should be less than or equal to 0 @@ -89,47 +106,51 @@ static void stats_test(int stats_fd) case KVM_STATS_UNIT_NONE: case KVM_STATS_UNIT_BYTES: case KVM_STATS_UNIT_CYCLES: - TEST_ASSERT(pdesc->exponent >= 0, - "Unsupported KVM stats unit"); + TEST_ASSERT(pdesc->exponent >= 0, "Unsupported KVM stats unit"); break; case KVM_STATS_UNIT_SECONDS: - TEST_ASSERT(pdesc->exponent <= 0, - "Unsupported KVM stats unit"); + TEST_ASSERT(pdesc->exponent <= 0, "Unsupported KVM stats unit"); break; } /* Check name string */ TEST_ASSERT(strlen(pdesc->name) < header.name_size, - "KVM stats name(%s) too long", pdesc->name); + "KVM stats name(%s) too long", pdesc->name); /* Check size field, which should not be zero */ - TEST_ASSERT(pdesc->size, "KVM descriptor(%s) with size of 0", - pdesc->name); + TEST_ASSERT(pdesc->size, + "KVM descriptor(%s) with size of 0", pdesc->name); /* Check bucket_size field */ switch (pdesc->flags & KVM_STATS_TYPE_MASK) { case KVM_STATS_TYPE_LINEAR_HIST: TEST_ASSERT(pdesc->bucket_size, - "Bucket size of Linear Histogram stats (%s) is zero", - pdesc->name); + "Bucket size of Linear Histogram stats (%s) is zero", + pdesc->name); break; default: TEST_ASSERT(!pdesc->bucket_size, - "Bucket size of stats (%s) is not zero", - pdesc->name); + "Bucket size of stats (%s) is not zero", + pdesc->name); } size_data += pdesc->size * sizeof(*stats_data); } - /* Check overlap */ - TEST_ASSERT(header.data_offset >= header.desc_offset - || header.data_offset + size_data <= header.desc_offset, - "Data block is overlapped with Descriptor block"); + + /* + * Now that the size of the data block is known, verify the data block + * doesn't overlap the descriptor block. + */ + TEST_ASSERT(header.data_offset >= header.desc_offset || + header.data_offset + size_data <= header.desc_offset, + "Data block is overlapped with Descriptor block"); + /* Check validity of all stats data size */ TEST_ASSERT(size_data >= header.num_desc * sizeof(*stats_data), - "Data size is not correct"); + "Data size is not correct"); + /* Check stats offset */ for (i = 0; i < header.num_desc; ++i) { pdesc = (void *)stats_desc + i * size_desc; TEST_ASSERT(pdesc->offset < size_data, - "Invalid offset (%u) for stats: %s", - pdesc->offset, pdesc->name); + "Invalid offset (%u) for stats: %s", + pdesc->offset, pdesc->name); } /* Read kvm stats data one by one */ base-commit: f9955a6aaa037a7a8198a817b9d272efcf10961a --
On Tue, Apr 12, 2022 at 1:03 PM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Apr 12, 2022, Sean Christopherson wrote: > > On Tue, Apr 12, 2022, Ben Gardon wrote: > > > On Mon, Apr 11, 2022 at 5:55 PM Mingwei Zhang <mizhang@google.com> wrote: > > > > I was very confused on header->name_size. So this field means the > > > > maximum string size of a stats name, right? Can we update the comments > > > > in the kvm.h to specify that? By reading the comments, I don't really > > > > feel this is how we should use this field. > > > > > > I believe that's right. I agree the documentation on that was a little > > > confusing. > > > > Heh, a little. I got tripped up looking at this too. Give me a few minutes and > > I'll attach a cleanup patch to add comments and fix the myriad style issues. > > > > This whole file is painful to look at. Aside from violating preferred kernel > > style, it's horribly consistent with itself. Well, except for the 80 char limit, > > to which it has a fanatical devotion. > > If you send a new version of this series, can you add this on top? Will do. > > From: Sean Christopherson <seanjc@google.com> > Date: Tue, 12 Apr 2022 11:49:59 -0700 > Subject: [PATCH] KVM: selftests: Clean up coding style in binary stats test > > Fix a variety of code style violations and/or inconsistencies in the > binary stats test. The 80 char limit is a soft limit and can and should > be ignored/violated if doing so improves the overall code readability. > > Specifically, provide consistent indentation and don't split expressions > at arbitrary points just to honor the 80 char limit. > > Opportunistically expand/add comments to call out the more subtle aspects > of the code. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > .../selftests/kvm/kvm_binary_stats_test.c | 91 ++++++++++++------- > 1 file changed, 56 insertions(+), 35 deletions(-) > > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > index 97b180249ba0..944ed52e3f07 100644 > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c > @@ -37,32 +37,42 @@ static void stats_test(int stats_fd) > /* Read kvm stats header */ > read_vm_stats_header(stats_fd, &header); > > + /* > + * The base size of the descriptor is defined by KVM's ABI, but the > + * size of the name field is variable as far as KVM's ABI is concerned. > + * But, the size of name is constant for a given instance of KVM and > + * is provided by KVM in the overall stats header. > + */ > size_desc = sizeof(*stats_desc) + header.name_size; > > /* Read kvm stats id string */ > id = malloc(header.name_size); > TEST_ASSERT(id, "Allocate memory for id string"); > + > ret = read(stats_fd, id, header.name_size); > TEST_ASSERT(ret == header.name_size, "Read id string"); > > /* Check id string, that should start with "kvm" */ > TEST_ASSERT(!strncmp(id, "kvm", 3) && strlen(id) < header.name_size, > - "Invalid KVM stats type, id: %s", id); > + "Invalid KVM stats type, id: %s", id); > > /* Sanity check for other fields in header */ > if (header.num_desc == 0) { > printf("No KVM stats defined!"); > return; > } > - /* Check overlap */ > - TEST_ASSERT(header.desc_offset > 0 && header.data_offset > 0 > - && header.desc_offset >= sizeof(header) > - && header.data_offset >= sizeof(header), > - "Invalid offset fields in header"); > + /* > + * The descriptor and data offsets must be valid, they must not overlap > + * the header, and the descriptor and data blocks must not overlap each > + * other. Note, the data block is rechecked after its size is known. > + */ > + TEST_ASSERT(header.desc_offset && header.desc_offset >= sizeof(header) && > + header.data_offset && header.data_offset >= sizeof(header), > + "Invalid offset fields in header"); > + > TEST_ASSERT(header.desc_offset > header.data_offset || > - (header.desc_offset + size_desc * header.num_desc <= > - header.data_offset), > - "Descriptor block is overlapped with data block"); > + (header.desc_offset + size_desc * header.num_desc <= header.data_offset), > + "Descriptor block is overlapped with data block"); > > /* Read kvm stats descriptors */ > stats_desc = alloc_vm_stats_desc(stats_fd, &header); > @@ -70,15 +80,22 @@ static void stats_test(int stats_fd) > > /* Sanity check for fields in descriptors */ > for (i = 0; i < header.num_desc; ++i) { > + /* > + * Note, size_desc includes the of the name field, which is > + * variable, i.e. this is NOT equivalent to &stats_desc[i]. > + */ > pdesc = (void *)stats_desc + i * size_desc; > - /* Check type,unit,base boundaries */ > - TEST_ASSERT((pdesc->flags & KVM_STATS_TYPE_MASK) > - <= KVM_STATS_TYPE_MAX, "Unknown KVM stats type"); > - TEST_ASSERT((pdesc->flags & KVM_STATS_UNIT_MASK) > - <= KVM_STATS_UNIT_MAX, "Unknown KVM stats unit"); > - TEST_ASSERT((pdesc->flags & KVM_STATS_BASE_MASK) > - <= KVM_STATS_BASE_MAX, "Unknown KVM stats base"); > - /* Check exponent for stats unit > + > + /* Check type, unit, and base boundaries */ > + TEST_ASSERT((pdesc->flags & KVM_STATS_TYPE_MASK) <= KVM_STATS_TYPE_MAX, > + "Unknown KVM stats type"); > + TEST_ASSERT((pdesc->flags & KVM_STATS_UNIT_MASK) <= KVM_STATS_UNIT_MAX, > + "Unknown KVM stats unit"); > + TEST_ASSERT((pdesc->flags & KVM_STATS_BASE_MASK) <= KVM_STATS_BASE_MAX, > + "Unknown KVM stats base"); > + > + /* > + * Check exponent for stats unit > * Exponent for counter should be greater than or equal to 0 > * Exponent for unit bytes should be greater than or equal to 0 > * Exponent for unit seconds should be less than or equal to 0 > @@ -89,47 +106,51 @@ static void stats_test(int stats_fd) > case KVM_STATS_UNIT_NONE: > case KVM_STATS_UNIT_BYTES: > case KVM_STATS_UNIT_CYCLES: > - TEST_ASSERT(pdesc->exponent >= 0, > - "Unsupported KVM stats unit"); > + TEST_ASSERT(pdesc->exponent >= 0, "Unsupported KVM stats unit"); > break; > case KVM_STATS_UNIT_SECONDS: > - TEST_ASSERT(pdesc->exponent <= 0, > - "Unsupported KVM stats unit"); > + TEST_ASSERT(pdesc->exponent <= 0, "Unsupported KVM stats unit"); > break; > } > /* Check name string */ > TEST_ASSERT(strlen(pdesc->name) < header.name_size, > - "KVM stats name(%s) too long", pdesc->name); > + "KVM stats name(%s) too long", pdesc->name); > /* Check size field, which should not be zero */ > - TEST_ASSERT(pdesc->size, "KVM descriptor(%s) with size of 0", > - pdesc->name); > + TEST_ASSERT(pdesc->size, > + "KVM descriptor(%s) with size of 0", pdesc->name); > /* Check bucket_size field */ > switch (pdesc->flags & KVM_STATS_TYPE_MASK) { > case KVM_STATS_TYPE_LINEAR_HIST: > TEST_ASSERT(pdesc->bucket_size, > - "Bucket size of Linear Histogram stats (%s) is zero", > - pdesc->name); > + "Bucket size of Linear Histogram stats (%s) is zero", > + pdesc->name); > break; > default: > TEST_ASSERT(!pdesc->bucket_size, > - "Bucket size of stats (%s) is not zero", > - pdesc->name); > + "Bucket size of stats (%s) is not zero", > + pdesc->name); > } > size_data += pdesc->size * sizeof(*stats_data); > } > - /* Check overlap */ > - TEST_ASSERT(header.data_offset >= header.desc_offset > - || header.data_offset + size_data <= header.desc_offset, > - "Data block is overlapped with Descriptor block"); > + > + /* > + * Now that the size of the data block is known, verify the data block > + * doesn't overlap the descriptor block. > + */ > + TEST_ASSERT(header.data_offset >= header.desc_offset || > + header.data_offset + size_data <= header.desc_offset, > + "Data block is overlapped with Descriptor block"); > + > /* Check validity of all stats data size */ > TEST_ASSERT(size_data >= header.num_desc * sizeof(*stats_data), > - "Data size is not correct"); > + "Data size is not correct"); > + > /* Check stats offset */ > for (i = 0; i < header.num_desc; ++i) { > pdesc = (void *)stats_desc + i * size_desc; > TEST_ASSERT(pdesc->offset < size_data, > - "Invalid offset (%u) for stats: %s", > - pdesc->offset, pdesc->name); > + "Invalid offset (%u) for stats: %s", > + pdesc->offset, pdesc->name); > } > > /* Read kvm stats data one by one */ > > base-commit: f9955a6aaa037a7a8198a817b9d272efcf10961a > -- >
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 5ba3132f3110..c5f34551ff76 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -401,6 +401,10 @@ 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 read_vm_stats_header(int stats_fd, struct kvm_stats_header *header); +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, + struct kvm_stats_header *header); +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *stats_desc); 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 22c22a90f15a..e4795bad7db6 100644 --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c @@ -62,14 +62,9 @@ static void stats_test(int stats_fd) header.data_offset), "Descriptor block is overlapped with data block"); - /* Allocate memory for stats descriptors */ - stats_desc = calloc(header.num_desc, size_desc); - TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); /* Read kvm stats descriptors */ - ret = pread(stats_fd, stats_desc, - size_desc * header.num_desc, header.desc_offset); - TEST_ASSERT(ret == size_desc * header.num_desc, - "Read KVM stats descriptors"); + stats_desc = alloc_vm_stats_desc(stats_fd, &header); + read_vm_stats_desc(stats_fd, &header, stats_desc); /* Sanity check for fields in descriptors */ for (i = 0; i < header.num_desc; ++i) { diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 0caf28e324ed..e3ae26fbef03 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2564,3 +2564,32 @@ void read_vm_stats_header(int stats_fd, struct kvm_stats_header *header) ret = read(stats_fd, header, sizeof(*header)); TEST_ASSERT(ret == sizeof(*header), "Read stats header"); } + +static ssize_t stats_descs_size(struct kvm_stats_header *header) +{ + return header->num_desc * + (sizeof(struct kvm_stats_desc) + header->name_size); +} + +/* Caller is responsible for freeing the returned kvm_stats_desc. */ +struct kvm_stats_desc *alloc_vm_stats_desc(int stats_fd, + struct kvm_stats_header *header) +{ + struct kvm_stats_desc *stats_desc; + + stats_desc = malloc(stats_descs_size(header)); + TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors"); + + return stats_desc; +} + +void read_vm_stats_desc(int stats_fd, struct kvm_stats_header *header, + struct kvm_stats_desc *stats_desc) +{ + ssize_t ret; + + ret = pread(stats_fd, stats_desc, stats_descs_size(header), + header->desc_offset); + TEST_ASSERT(ret == stats_descs_size(header), + "Read KVM stats descriptors"); +}
Move the code to read the binary stats descriptors to the KVM selftests library. It will be re-used by other tests to check KVM behavior. No functional change intended. Signed-off-by: Ben Gardon <bgardon@google.com> --- .../selftests/kvm/include/kvm_util_base.h | 4 +++ .../selftests/kvm/kvm_binary_stats_test.c | 9 ++---- tools/testing/selftests/kvm/lib/kvm_util.c | 29 +++++++++++++++++++ 3 files changed, 35 insertions(+), 7 deletions(-)