Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221105045704.2315186-1-vipinsh@google.com> <20221105045704.2315186-5-vipinsh@google.com>
 <Y2lXld6G+Hp0FW3A@google.com> <CAHVum0ewgeA81T2ttPGB0V-2njdc1oqTjQWziKzTu_pzN+srxw@mail.gmail.com>
In-Reply-To: <CAHVum0ewgeA81T2ttPGB0V-2njdc1oqTjQWziKzTu_pzN+srxw@mail.gmail.com>
From:   David Matlack <dmatlack@google.com>
Date:   Tue, 8 Nov 2022 09:56:57 -0800
Message-ID: <CALzav=djtqYAchW0x=riSEtvoQAbpotjBeibBLWVQg4J2T5iiA@mail.gmail.com>
Subject: Re: [PATCH 4/6] KVM: selftests: Make Hyper-V guest OS ID common
To:     Vipin Sharma <vipinsh@google.com>
Cc:     seanjc@google.com, pbonzini@redhat.com, vkuznets@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 7, 2022 at 5:45 PM Vipin Sharma <vipinsh@google.com> wrote:
>
> On Mon, Nov 7, 2022 at 11:08 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On Fri, Nov 04, 2022 at 09:57:02PM -0700, Vipin Sharma wrote:
> > > Make guest OS ID calculation common to all hyperv tests and similar to
> > > hv_generate_guest_id().
> >
> > This commit makes the HV_LINUX_VENDOR_ID and adds LINUX_VERSION_CODE
> > to existing tests. Can you split out the latter to a separate commit?
> > Also what's the reason to add LINUX_VERSION_CODE to the mix?
> >
>
> I looked at the implementation in selftest and what is in the
> include/asm-generic/mshyperv.h for the hv_generate_guest_id()
> function, both looked different. I thought it would be nice to have
> consistent logic at both places.
>
> I can remove LINUX_VERISON_CODE if you prefer.

Using LINUX_VERSION_CODE here assumes the selftest will run on the
same kernel that it was compiled with. This might not be the case in
practice, e.g. if anyone runs the latest upstream selftests against
their internal kernel (something we've discussed doing recently).

The right way to incorporate the Linux version code would be for the
selftest to query it from the host dynamically and use that (at which
point hv_linux_guest_id() would actually have to be a function).

But since you don't strictly need it, it's probably best to just omit
it for the time being.


>
> > >
> > > Signed-off-by: Vipin Sharma <vipinsh@google.com>
> > > ---
> > >  tools/testing/selftests/kvm/include/x86_64/hyperv.h  | 10 ++++++++++
> > >  tools/testing/selftests/kvm/x86_64/hyperv_clock.c    |  2 +-
> > >  tools/testing/selftests/kvm/x86_64/hyperv_features.c |  6 ++----
> > >  tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c |  2 +-
> > >  4 files changed, 14 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/x86_64/hyperv.h b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> > > index 075fd29071a6..9d8c325af1d9 100644
> > > --- a/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> > > +++ b/tools/testing/selftests/kvm/include/x86_64/hyperv.h
> > > @@ -9,6 +9,10 @@
> > >  #ifndef SELFTEST_KVM_HYPERV_H
> > >  #define SELFTEST_KVM_HYPERV_H
> > >
> > > +#include <linux/version.h>
> > > +
> > > +#define HV_LINUX_VENDOR_ID                   0x8100
> > > +
> > >  #define HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS        0x40000000
> > >  #define HYPERV_CPUID_INTERFACE                       0x40000001
> > >  #define HYPERV_CPUID_VERSION                 0x40000002
> > > @@ -189,4 +193,10 @@
> > >  /* hypercall options */
> > >  #define HV_HYPERCALL_FAST_BIT                BIT(16)
> > >
> > > +static inline uint64_t hv_linux_guest_id(void)
> > > +{
> > > +     return ((uint64_t)HV_LINUX_VENDOR_ID << 48) |
> > > +            ((uint64_t)LINUX_VERSION_CODE << 16);
> >
> > This can be a compile-time constant (i.e. macro).
> >
>
> Yes, I will make it a macro in the next version.
>
> > > +}
> > > +
> > >  #endif /* !SELFTEST_KVM_HYPERV_H */
> > > diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
> > > index d576bc8ce823..f9112c5dc3f7 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
> > > @@ -104,7 +104,7 @@ static void guest_main(struct ms_hyperv_tsc_page *tsc_page, vm_paddr_t tsc_page_
> > >
> > >       /* Set Guest OS id to enable Hyper-V emulation */
> > >       GUEST_SYNC(1);
> > > -     wrmsr(HV_X64_MSR_GUEST_OS_ID, (u64)0x8100 << 48);
> > > +     wrmsr(HV_X64_MSR_GUEST_OS_ID, hv_linux_guest_id());
> > >       GUEST_SYNC(2);
> > >
> > >       check_tsc_msr_rdtsc();
> > > diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> > > index 6b443ce456b6..b5a42cf1ad9d 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> > > @@ -13,8 +13,6 @@
> > >  #include "processor.h"
> > >  #include "hyperv.h"
> > >
> > > -#define LINUX_OS_ID ((u64)0x8100 << 48)
> > > -
> > >  static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
> > >                               vm_vaddr_t output_address, uint64_t *hv_status)
> > >  {
> > > @@ -71,7 +69,7 @@ static void guest_hcall(vm_vaddr_t pgs_gpa, struct hcall_data *hcall)
> > >
> > >       GUEST_ASSERT(hcall->control);
> > >
> > > -     wrmsr(HV_X64_MSR_GUEST_OS_ID, LINUX_OS_ID);
> > > +     wrmsr(HV_X64_MSR_GUEST_OS_ID, hv_linux_guest_id());
> > >       wrmsr(HV_X64_MSR_HYPERCALL, pgs_gpa);
> > >
> > >       if (!(hcall->control & HV_HYPERCALL_FAST_BIT)) {
> > > @@ -169,7 +167,7 @@ static void guest_test_msrs_access(void)
> > >                        */
> > >                       msr->idx = HV_X64_MSR_GUEST_OS_ID;
> > >                       msr->write = 1;
> > > -                     msr->write_val = LINUX_OS_ID;
> > > +                     msr->write_val = hv_linux_guest_id();
> > >                       msr->available = 1;
> > >                       break;
> > >               case 3:
> > > diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
> > > index a380ad7bb9b3..2c13a144b04c 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
> > > @@ -69,7 +69,7 @@ static void __attribute__((__flatten__)) guest_code(struct svm_test_data *svm)
> > >
> > >       GUEST_SYNC(1);
> > >
> > > -     wrmsr(HV_X64_MSR_GUEST_OS_ID, (u64)0x8100 << 48);
> > > +     wrmsr(HV_X64_MSR_GUEST_OS_ID, hv_linux_guest_id());
> > >
> > >       GUEST_ASSERT(svm->vmcb_gpa);
> > >       /* Prepare for L2 execution. */
> > > --
> > > 2.38.1.273.g43a17bfeac-goog
> > >