From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, March 7, 2024 11:49 AM
>
> On 3/7/2024 11:22 AM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, March 6, 2024 4:48 PM
> >>
> >> Introduce x86_64 and arm64 functions for getting the hypervisor version
> >> information and storing it in a structure for simpler parsing.
> >>
> >> Use the new function to get and parse the version at boot time. While at
> >> it, print the version in the same format for each architecture, and move
> >> the printing code to hv_common_init() so it is not duplicated.
> >
> > Isn't the format already the same for x86 and ARM64? A couple of
> > years ago they didn't match. But that was fixed in commit eeda29db98f4.
> >
>
> You're correct - I will amend the commit message. Thanks!
>
> >>
> >> Signed-off-by: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx>
> >> Acked-by: Wei Liu <wei.liu@xxxxxxxxxx>
> >> ---
> >> arch/arm64/hyperv/mshyperv.c | 19 ++++++++---------
> >> arch/x86/kernel/cpu/mshyperv.c | 35 ++++++++++++++-----------------
> >> drivers/hv/hv_common.c | 9 ++++++++
> >> include/asm-generic/hyperv-tlfs.h | 23 ++++++++++++++++++++
> >> include/asm-generic/mshyperv.h | 2 ++
> >> 5 files changed, 59 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/arch/arm64/hyperv/mshyperv.c
> >> b/arch/arm64/hyperv/mshyperv.c
> >> index f1b8a04ee9f2..55dc224d466d 100644
> >> --- a/arch/arm64/hyperv/mshyperv.c
> >> +++ b/arch/arm64/hyperv/mshyperv.c
> >> @@ -19,10 +19,18 @@
> >>
> >> static bool hyperv_initialized;
> >>
> >> +int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
> >> +{
> >> + hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION,
> >> + (struct hv_get_vp_registers_output *)info);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(hv_get_hypervisor_version);
> >
> > I don't think this need to be exported, at least not for the usage in
> > this patch. The caller in hv_common.c is never part of a module -- it's
> > always built-in. But maybe you are anticipating future use cases
> > from a module?
> >
>
> Yes, it will be used in a module eventually. Do you think I should remove
> this and the below export until they are actually needed?
I don't have a strong feeling either way if the module-based caller
comes along soon. But I know some reviewers don't want stuff
added until it is actually used.
>
> >> +
> >> static int __init hyperv_init(void)
> >> {
> >> struct hv_get_vp_registers_output result;
> >> - u32 a, b, c, d;
> >> u64 guest_id;
> >> int ret;
> >>
> >> @@ -54,15 +62,6 @@ static int __init hyperv_init(void)
> >> ms_hyperv.features, ms_hyperv.priv_high, ms_hyperv.hints,
> >> ms_hyperv.misc_features);
> >>
> >> - /* Get information about the Hyper-V host version */
> >> - hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION, &result);
> >> - a = result.as32.a;
> >> - b = result.as32.b;
> >> - c = result.as32.c;
> >> - d = result.as32.d;
> >> - pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
> >> - b >> 16, b & 0xFFFF, a, d & 0xFFFFFF, c, d >> 24);
> >> -
> >> ret = hv_common_init();
> >> if (ret)
> >> return ret;
> >> diff --git a/arch/x86/kernel/cpu/mshyperv.c
> >> b/arch/x86/kernel/cpu/mshyperv.c
> >> index d306f6184cee..03a3445faf7a 100644
> >> --- a/arch/x86/kernel/cpu/mshyperv.c
> >> +++ b/arch/x86/kernel/cpu/mshyperv.c
> >> @@ -350,13 +350,25 @@ static void __init reduced_hw_init(void)
> >> x86_init.irqs.pre_vector_init = x86_init_noop;
> >> }
> >>
> >> +int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
> >> +{
> >> + unsigned int hv_max_functions;
> >> +
> >> + hv_max_functions = cpuid_eax(HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS);
> >> + if (hv_max_functions < HYPERV_CPUID_VERSION) {
> >> + pr_err("%s: Could not detect Hyper-V version\n", __func__);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + cpuid(HYPERV_CPUID_VERSION, &info->eax, &info->ebx, &info->ecx, &info->edx);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(hv_get_hypervisor_version);
> >
> > Same for this EXPORT.
> >>> +
> >> static void __init ms_hyperv_init_platform(void)
> >> {
> >> int hv_max_functions_eax;
> >> - int hv_host_info_eax;
> >> - int hv_host_info_ebx;
> >> - int hv_host_info_ecx;
> >> - int hv_host_info_edx;
> >>
> >> #ifdef CONFIG_PARAVIRT
> >> pv_info.name = "Hyper-V";
> >> @@ -407,21 +419,6 @@ static void __init ms_hyperv_init_platform(void)
> >> pr_info("Hyper-V: running on a nested hypervisor\n");
> >> }
> >>
> >> - /*
> >> - * Extract host information.
> >> - */
> >> - if (hv_max_functions_eax >= HYPERV_CPUID_VERSION) {
> >> - hv_host_info_eax = cpuid_eax(HYPERV_CPUID_VERSION);
> >> - hv_host_info_ebx = cpuid_ebx(HYPERV_CPUID_VERSION);
> >> - hv_host_info_ecx = cpuid_ecx(HYPERV_CPUID_VERSION);
> >> - hv_host_info_edx = cpuid_edx(HYPERV_CPUID_VERSION);
> >> -
> >> - pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
> >> - hv_host_info_ebx >> 16, hv_host_info_ebx & 0xFFFF,
> >> - hv_host_info_eax, hv_host_info_edx & 0xFFFFFF,
> >> - hv_host_info_ecx, hv_host_info_edx >> 24);
> >> - }
> >> -
> >> if (ms_hyperv.features & HV_ACCESS_FREQUENCY_MSRS &&
> >> ms_hyperv.misc_features & HV_FEATURE_FREQUENCY_MSRS_AVAILABLE) {
> >> x86_platform.calibrate_tsc = hv_get_tsc_khz;
> >> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> >> index 2f1dd4b07f9a..4d72c528af68 100644
> >> --- a/drivers/hv/hv_common.c
> >> +++ b/drivers/hv/hv_common.c
> >> @@ -278,6 +278,15 @@ static void hv_kmsg_dump_register(void)
> >> int __init hv_common_init(void)
> >> {
> >> int i;
> >> + union hv_hypervisor_version_info version;
> >> +
> >> + /* Get information about the Hyper-V host version */
> >> + if (hv_get_hypervisor_version(&version) == 0) {
> >
> > The usual idiom would be:
> >
> > if (!hv_get_hypervisor_version(&version)) {
> >
> Thanks, I'll change it.
>
> >> + pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
> >> + version.major_version, version.minor_version,
> >> + version.build_number, version.service_number,
> >> + version.service_pack, version.service_branch);
> >> + }
> >>
> >> if (hv_is_isolation_supported())
> >> sysctl_record_panic_msg = 0;
> >> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> >> index 3d1b31f90ed6..32514a870b98 100644
> >> --- a/include/asm-generic/hyperv-tlfs.h
> >> +++ b/include/asm-generic/hyperv-tlfs.h
> >> @@ -817,6 +817,29 @@ struct hv_input_unmap_device_interrupt {
> >> #define HV_SOURCE_SHADOW_NONE 0x0
> >> #define HV_SOURCE_SHADOW_BRIDGE_BUS_RANGE 0x1
> >>
> >> +/*
> >> + * Version info reported by hypervisor
> >> + */
> >> +union hv_hypervisor_version_info {
> >> + struct {
> >> + u32 build_number;
> >> +
> >> + u32 minor_version : 16;
> >> + u32 major_version : 16;
> >> +
> >> + u32 service_pack;
> >> +
> >> + u32 service_number : 24;
> >> + u32 service_branch : 8;
> >> + };
> >> + struct {
> >> + u32 eax;
> >> + u32 ebx;
> >> + u32 ecx;
> >> + u32 edx;
> >
> > Nit: These names are x86-isms appearing in the generic portion
> > of hyperv-tlfs.h. On the ARM64 side I had called the four parts
> > "a", "b", "c", and "d" to be slightly more generic. But if want to
> > keep the x86 register names, I won't object.
> > > Michael
> >
>
> Good point. It's worth noting that these are now only used on the x86
> side as arguments to cpuid(), so I might just leave them as-is.
> Another option would be to add an x86-only union for this purpose:
>
> union hv_x86_hypervisor_version_info {
> struct hv_hypervisor_version_info info;
> struct {
> u32 eax;
> u32 ebx;
> u32 ecx;
> u32 edx;
> };
> };
>
> But that is probably overkill...
Yes, I agree that would be overkill. Again, I'm OK if you prefer
to keep the x86 register names.
Michael
