RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files

From: Michael Kelley
Date: Tue Mar 17 2020 - 20:10:52 EST

Next message: Robert Richter: "Re: [PATCH 11/11] EDAC/ghes: Create one memory controller per physical memory array"
Previous message: Al Viro: "Re: [PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF"
In reply to: Arnd Bergmann: "Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

From: Marc Zyngier <maz@xxxxxxxxxx> Sent: Sunday, March 15, 2020 10:31 AM
>
> On Sat, 14 Mar 2020 15:35:10 +0000,
> Hi Michael,
>
> Michael Kelley <mikelley@xxxxxxxxxxxxx> wrote:
> >
> > hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
> > Functional Spec (TLFS). The TLFS is distinctly oriented to x86/x64,
> > and Hyper-V has not separated out the architecture-dependent parts into
> > x86/x64 vs. ARM64. So hyperv-tlfs.h includes information for ARM64
> > that is not yet formally published. The TLFS is available here:
> >
> > docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> >
> > mshyperv.h defines Linux-specific structures and routines for
> > interacting with Hyper-V on ARM64, and #includes the architecture-
> > independent part of mshyperv.h in include/asm-generic.
> >
> > Signed-off-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 2 +
> > arch/arm64/include/asm/hyperv-tlfs.h | 413
> +++++++++++++++++++++++++++++++++++
> > arch/arm64/include/asm/mshyperv.h | 115 ++++++++++
> > 3 files changed, 530 insertions(+)
> > create mode 100644 arch/arm64/include/asm/hyperv-tlfs.h
> > create mode 100644 arch/arm64/include/asm/mshyperv.h
>
> So this is a pretty large patch, mostly containing constants and other
> data structures that don't necessarily make sense immediately (or at
> least, I can't make sense of it without reading all the other 9
> patches and going back to patch #1).
>
> Could you please consider splitting this into more discreet bits that
> get added as required by the supporting code?

Yes, I'll do this in the next version.

>
> So here's only a few sparse comments:
>
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 58bb5c4..398cfdb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7809,6 +7809,8 @@ F: arch/x86/include/asm/trace/hyperv.h
> > F: arch/x86/include/asm/hyperv-tlfs.h
> > F: arch/x86/kernel/cpu/mshyperv.c
> > F: arch/x86/hyperv
> > +F: arch/arm64/include/asm/hyperv-tlfs.h
> > +F: arch/arm64/include/asm/mshyperv.h
> > F: drivers/clocksource/hyperv_timer.c
> > F: drivers/hid/hid-hyperv.c
> > F: drivers/hv/
> > diff --git a/arch/arm64/include/asm/hyperv-tlfs.h b/arch/arm64/include/asm/hyperv-
> tlfs.h
> > new file mode 100644
> > index 0000000..5e6a087
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/hyperv-tlfs.h
> > @@ -0,0 +1,413 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * This file contains definitions from the Hyper-V Hypervisor Top-Level
> > + * Functional Specification (TLFS):
> > + * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> > + *
> > + * Copyright (C) 2019, Microsoft, Inc.
> > + *
> > + * Author : Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > + */
> > +
> > +#ifndef _ASM_HYPERV_TLFS_H
> > +#define _ASM_HYPERV_TLFS_H
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * All data structures defined in the TLFS that are shared between Hyper-V
> > + * and a guest VM use Little Endian byte ordering. This matches the default
> > + * byte ordering of Linux running on ARM64, so no special handling is required.
> > + */
> > +
> > +
> > +/*
> > + * While not explicitly listed in the TLFS, Hyper-V always runs with a page
> > + * size of 4096. These definitions are used when communicating with Hyper-V
> > + * using guest physical pages and guest physical page addresses, since the
> > + * guest page size may not be 4096 on ARM64.
> > + */
> > +#define HV_HYP_PAGE_SHIFT 12
> > +#define HV_HYP_PAGE_SIZE (1 << HV_HYP_PAGE_SHIFT)
>
> Probably worth writing this as 1UL to be on the safe side.

Agreed.

>
> > +#define HV_HYP_PAGE_MASK (~(HV_HYP_PAGE_SIZE - 1))
> > +
> > +/*
> > + * These Hyper-V registers provide information equivalent to the CPUID
> > + * instruction on x86/x64.
> > + */
> > +#define HV_REGISTER_HYPERVISOR_VERSION 0x00000100 /*CPUID
> 0x40000002 */
> > +#define HV_REGISTER_PRIVILEGES_AND_FEATURES 0x00000200 /*CPUID
> 0x40000003 */
> > +#define HV_REGISTER_FEATURES 0x00000201 /*CPUID
> 0x40000004 */
> > +#define HV_REGISTER_IMPLEMENTATION_LIMITS 0x00000202 /*CPUID
> 0x40000005 */
> > +#define HV_ARM64_REGISTER_INTERFACE_VERSION 0x00090006 /*CPUID
> 0x40000001 */
> > +
> > +/*
> > + * Feature identification. HvRegisterPrivilegesAndFeaturesInfo returns a
> > + * 128-bit value with flags indicating which features are available to the
> > + * partition based upon the current partition privileges. The 128-bit
> > + * value is broken up with different portions stored in different 32-bit
> > + * fields in the ms_hyperv structure.
> > + */
> > +
> > +/* Partition Reference Counter available*/
> > +#define HV_MSR_TIME_REF_COUNT_AVAILABLE BIT(1)
> > +
> > +/* Synthetic Timers available */
> > +#define HV_MSR_SYNTIMER_AVAILABLE BIT(3)
> > +
> > +/* Reference TSC available */
> > +#define HV_MSR_REFERENCE_TSC_AVAILABLE BIT(9)
> > +
> > +
> > +/*
> > + * This group of flags is in the high order 64-bits of the returned
> > + * 128-bit value. Note that this set of bit positions differs from what
> > + * is used on x86/x64 architecture.
> > + */
> > +
> > +/* Crash MSRs available */
> > +#define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE BIT(8)
>
> It is confusing that you don't have a single bit space for all these
> flags. It'd probably help if you had a structure describing this
> 128bit value in multiple 32bit or 64bit words, and indicating which
> field the bit position is relevant to.

I'll add this in the next version.

>
> [...]
>
> > +/* Define the hypercall status result */
> > +
> > +union hv_hypercall_status {
> > + u64 as_uint64;
>
> nit: it'd be more consistent if as_uint64 was actually a uint64 type.

Agreed.

>
> > + struct {
> > + u16 status;
> > + u16 reserved;
> > + u16 reps_completed; /* Low 12 bits */
> > + u16 reserved2;
> > + };
> > +};
> > +
> > +/* hypercall status code */
> > +#define HV_STATUS_SUCCESS 0
> > +#define HV_STATUS_INVALID_HYPERCALL_CODE 2
> > +#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
> > +#define HV_STATUS_INVALID_ALIGNMENT 4
> > +#define HV_STATUS_INSUFFICIENT_MEMORY 11
> > +#define HV_STATUS_INVALID_CONNECTION_ID 18
> > +#define HV_STATUS_INSUFFICIENT_BUFFERS 19
> > +
> > +/* Define input and output layout for Get VP Register hypercall */
> > +struct hv_get_vp_register_input {
> > + u64 partitionid;
> > + u32 vpindex;
> > + u8 inputvtl;
> > + u8 padding[3];
> > + u32 name0;
> > + u32 name1;
> > +} __packed;
> > +
> > +struct hv_get_vp_register_output {
> > + u64 registervaluelow;
> > + u64 registervaluehigh;
> > +} __packed;
> > +
> > +#define HV_FLUSH_ALL_PROCESSORS BIT(0)
> > +#define HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES BIT(1)
> > +#define HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY BIT(2)
>
> I"m curious: Are these supposed to be PV'd TLB invalidation
> operations?

Yes, they are. Hyper-V provides PV TLB flush hypercalls on ARM64, but
this patch set doesn't use those hypercalls. I'll remove the definitions.

>
> > +#define HV_FLUSH_USE_EXTENDED_RANGE_FORMAT BIT(3)
> > +
> > +enum HV_GENERIC_SET_FORMAT {
> > + HV_GENERIC_SET_SPARSE_4K,
> > + HV_GENERIC_SET_ALL,
> > +};
> > +
> > +/*
> > + * The Hyper-V TimeRefCount register and the TSC
> > + * page provide a guest VM clock with 100ns tick rate
> > + */
> > +#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
> > +
> > +/*
> > + * The fields in this structure are set by Hyper-V and read
> > + * by the Linux guest. They should be accessed with READ_ONCE()
> > + * so the compiler doesn't optimize in a way that will cause
> > + * problems. The union pads the size out to the page size
> > + * used to communicate with Hyper-V.
> > + */
> > +struct ms_hyperv_tsc_page {
> > + union {
> > + struct {
> > + u32 tsc_sequence;
> > + u32 reserved1;
> > + u64 tsc_scale;
> > + s64 tsc_offset;
> > + } __packed;
> > + u8 reserved2[HV_HYP_PAGE_SIZE];
> > + };
> > +};
> > +
> > +/* Define the number of synthetic interrupt sources. */
> > +#define HV_SYNIC_SINT_COUNT (16)
> > +/* Define the expected SynIC version. */
> > +#define HV_SYNIC_VERSION_1 (0x1)
> > +
> > +#define HV_SYNIC_CONTROL_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SIMP_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SIEFP_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SINT_MASKED (1ULL << 16)
> > +#define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17)
> > +#define HV_SYNIC_SINT_VECTOR_MASK (0xFF)
>
> Let's me guess: a PV interrupt controller? Do you really need this?
> Specially as I don't see any PV irqchip driver in this submission...
>

No, the above definitions aren't needed. I'll remove them.

Hyper-V does provide a limited synthetic interrupt controller that's
implemented entirely in architecture independent code and has been
used on the x86 side since the beginning of Hyper-V support. It
pre-dates me by a lot of years, and I've never considered whether it
should be modeled as an irqchip.

> [...]
>
> > diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
> > new file mode 100644
> > index 0000000..60b3f68
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/mshyperv.h
> > @@ -0,0 +1,115 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * Linux-specific definitions for managing interactions with Microsoft's
> > + * Hyper-V hypervisor. The definitions in this file are specific to
> > + * the ARM64 architecture. See include/asm-generic/mshyperv.h for
> > + * definitions are that architecture independent.
> > + *
> > + * Definitions that are specified in the Hyper-V Top Level Functional
> > + * Spec (TLFS) should not go in this file, but should instead go in
> > + * hyperv-tlfs.h.
> > + *
> > + * Copyright (C) 2019, Microsoft, Inc.
> > + *
> > + * Author : Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > + */
> > +
> > +#ifndef _ASM_MSHYPERV_H
> > +#define _ASM_MSHYPERV_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdesc.h>
> > +#include <linux/arm-smccc.h>
> > +#include <asm/hyperv-tlfs.h>
> > +
> > +/*
> > + * Define the IRQ numbers/vectors used by Hyper-V VMbus interrupts
> > + * and by STIMER0 Direct Mode interrupts. Hyper-V should be supplying
> > + * these values through ACPI, but there are no other interrupting
> > + * devices in a Hyper-V VM on ARM64, so it's OK to hard code for now.
>
> I'm not convinced it is OK. If you don't try to do the right thing
> now, what is the incentive to do it later? Starting to hard code
> things is akin to going back to the ARM board files of old. Been
> there, managed to fix it, not going back to it again anytime soon.

I'll check back with the Hyper-V guys on getting appropriate values
assigned in ACPI.

>
> > + * The "CALLBACK_VECTOR" terminology is a left-over from the x86/x64
> > + * world that is used in architecture independent Hyper-V code.
> > + */
> > +#define HYPERVISOR_CALLBACK_VECTOR 16
> > +#define HV_STIMER0_IRQNR 17
> > +
> > +extern u64 hv_do_hvc(u64 control, ...);
> > +extern u64 hv_do_hvc_fast_get(u64 control, u64 input1, u64 input2, u64 input3,
> > + struct hv_get_vp_register_output *output);
> > +
> > +/*
> > + * Declare calls to get and set Hyper-V VP register values on ARM64, which
> > + * requires a hypercall.
> > + */
> > +extern void hv_set_vpreg(u32 reg, u64 value);
> > +extern u64 hv_get_vpreg(u32 reg);
> > +extern void hv_get_vpreg_128(u32 reg, struct hv_get_vp_register_output *result);
> > +
> > +/*
> > + * Use the Hyper-V provided stimer0 as the timer that is made
> > + * available to the architecture independent Hyper-V drivers.
> > + */
> > +#define hv_init_timer(timer, tick) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > +#define hv_init_timer_config(timer, val) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > +#define hv_get_current_tick(tick) \
> > + (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > +
> > +#define hv_get_simp(val) (val = hv_get_vpreg(HV_REGISTER_SIPP))
> > +#define hv_set_simp(val) hv_set_vpreg(HV_REGISTER_SIPP, val)
> > +
> > +#define hv_get_siefp(val) (val = hv_get_vpreg(HV_REGISTER_SIFP))
> > +#define hv_set_siefp(val) hv_set_vpreg(HV_REGISTER_SIFP, val)
> > +
> > +#define hv_get_synic_state(val) (val = hv_get_vpreg(HV_REGISTER_SCONTROL))
> > +#define hv_set_synic_state(val) hv_set_vpreg(HV_REGISTER_SCONTROL, val)
> > +
> > +#define hv_get_vp_index(index) (index = hv_get_vpreg(HV_REGISTER_VPINDEX))
> > +
> > +#define hv_signal_eom() hv_set_vpreg(HV_REGISTER_EOM, 0)
> > +
> > +/*
> > + * Hyper-V SINT registers are numbered sequentially, so we can just
> > + * add the SINT number to the register number of SINT0
> > + */
> > +#define hv_get_synint_state(sint_num, val) \
> > + (val = hv_get_vpreg(HV_REGISTER_SINT0 + sint_num))
> > +#define hv_set_synint_state(sint_num, val) \
> > + hv_set_vpreg(HV_REGISTER_SINT0 + sint_num, val)
> > +
> > +#define hv_get_crash_ctl(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_CRASH_CTL))
> > +#define hv_get_time_ref_count(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > +#define hv_get_reference_tsc(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_REFERENCE_TSC))
> > +#define hv_set_reference_tsc(val) \
> > + hv_set_vpreg(HV_REGISTER_REFERENCE_TSC, val)
> > +#define hv_enable_vdso_clocksource()
> > +#define hv_set_clocksource_vdso(val) \
> > + ((val).vdso_clock_mode = VDSO_CLOCKMODE_NONE)
> > +
> > +#if IS_ENABLED(CONFIG_HYPERV)
>
> I don't think this guards anything useful.

You are probably right. I'll double-check.

>
> > +#define hv_enable_stimer0_percpu_irq(irq) enable_percpu_irq(irq, 0)
> > +#define hv_disable_stimer0_percpu_irq(irq) disable_percpu_irq(irq)
>
> and this looks pretty premature.

I'm not sure I understand your comment about "premature". Could
you clarify?

>
> > +#endif
> > +
> > +/* ARM64 specific code to read the hardware clock */
> > +#define hv_get_raw_timer() arch_timer_read_counter()
> > +
> > +/* SMCCC hypercall parameters */
> > +#define HV_SMCCC_FUNC_NUMBER 1
> > +#define HV_FUNC_ID ARM_SMCCC_CALL_VAL( \
> > + ARM_SMCCC_STD_CALL, \
> > + ARM_SMCCC_SMC_64, \
> > + ARM_SMCCC_OWNER_VENDOR_HYP, \
>
> This is only defined in patch #2...

Indeed. :-( I'll fix as part of breaking up this patch into smaller
pieces.

Michael

>
> > + HV_SMCCC_FUNC_NUMBER)
> > +
> > +#include <asm-generic/mshyperv.h>
> > +
> > +#endif
>
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.

Next message: Robert Richter: "Re: [PATCH 11/11] EDAC/ghes: Create one memory controller per physical memory array"
Previous message: Al Viro: "Re: [PATCH 1/4] exfat: Simplify exfat_utf8_d_hash() for code points above U+FFFF"
In reply to: Arnd Bergmann: "Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]