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>
In-Reply-To: <Y2lXld6G+Hp0FW3A@google.com>
From:   Vipin Sharma <vipinsh@google.com>
Date:   Mon, 7 Nov 2022 17:45:07 -0800
Message-ID: <CAHVum0ewgeA81T2ttPGB0V-2njdc1oqTjQWziKzTu_pzN+srxw@mail.gmail.com>
Subject: Re: [PATCH 4/6] KVM: selftests: Make Hyper-V guest OS ID common
To:     David Matlack <dmatlack@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 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.

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