Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220411211015.3091615-1-bgardon@google.com> <20220411211015.3091615-4-bgardon@google.com>
 <YlTN3yq1iBPkw6Aa@google.com>
In-Reply-To: <YlTN3yq1iBPkw6Aa@google.com>
From:   Ben Gardon <bgardon@google.com>
Date:   Tue, 12 Apr 2022 11:56:45 -0700
Message-ID: <CANgfPd8mZ9-zQBvK=OASQ+n7eq_FpvpStMc_yD-UsmFdQ3OCvA@mail.gmail.com>
Subject: Re: [PATCH v4 03/10] KVM: selftests: Read binary stats desc in lib
To:     Mingwei Zhang <mizhang@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Xu <peterx@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Peter Shier <pshier@google.com>,
        David Dunn <daviddunn@google.com>,
        Junaid Shahid <junaids@google.com>,
        Jim Mattson <jmattson@google.com>,
        David Matlack <dmatlack@google.com>,
        Jing Zhang <jingzhangos@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
> >