Message-ID: <MW2PR2101MB1052D91D46EBDFE004B2F549D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
Date:   Wed, 18 Mar 2020 00:10:46 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     "will@...nel.org" <will@...nel.org>,
        "ardb@...nel.org" <ardb@...nel.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "olaf@...fle.de" <olaf@...fle.de>,
        "apw@...onical.com" <apw@...onical.com>,
        vkuznets <vkuznets@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "marcelo.cerri@...onical.com" <marcelo.cerri@...onical.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Sunil Muthuswamy <sunilmut@...rosoft.com>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>
Subject: RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files

From: Marc Zyngier <maz@...nel.org> Sent: Sunday, March 15, 2020 10:31 AM
> 
> On Sat, 14 Mar 2020 15:35:10 +0000,
> Hi Michael,
> 
> Michael Kelley <mikelley@...rosoft.com> 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@...rosoft.com>
> > ---
> >  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@...rosoft.com>
> > + */
> > +
> > +#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@...rosoft.com>
> > + */
> > +
> > +#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.

Powered by blists - more mailing lists