[PATCH v6 00/10] Subject: Enable Linux guests on Hyper-V on ARM64

[PATCH v6 00/10] Subject: Enable Linux guests on Hyper-V on ARM64

[Michael Kelley]

This series enables Linux guests running on Hyper-V on ARM64
hardware. New ARM64-specific code in arch/arm64/hyperv initializes
Hyper-V, including its interrupts and hypercall mechanism.
Existing architecture independent drivers for Hyper-V's VMbus and
synthetic devices just work when built for ARM64. Hyper-V code is
built and included in the image and modules only if CONFIG_HYPERV
is enabled.

The ten patches are organized as follows:
1) Add include files that define the Hyper-V interface as
   described in the Hyper-V Top Level Functional Spec (TLFS), plus
   additional definitions specific to Linux running on Hyper-V.

2) Add #define for vendor specific owner definition to linux/arm-smccc.h

3) thru 7) Add core Hyper-V support on ARM64, including hypercalls,
   interrupt handlers, kexec & panic handlers, and core hypervisor
   initialization.

8) Update the existing VMbus driver to generalize interrupt
   management across x86/x64 and ARM64.

9) Export screen_info so it may be used by the Hyper-V frame buffer
   driver built as a module. It's already exported for x86,
   powerpc, and alpha architectures.

10) Make CONFIG_HYPERV selectable on ARM64 in addition to x86/x64.

Some areas of Linux guests on Hyper-V on ARM64 are a work-
in-progress:

* Hyper-V on ARM64 currently runs with a 4 Kbyte page size, but
  allows guests with 16K/64K page size. However, the Linux drivers
  for Hyper-V synthetic devices assume the guest page size is 4K.
  This patch set lays the groundwork for larger guest page sizes,
  but additional patches are coming to update these drivers.

* The Hyper-V vPCI driver at drivers/pci/host/pci-hyperv.c has
  x86/x64-specific code and is not being built for ARM64. Fixing
  this driver to enable vPCI devices on ARM64 will be done later.

In a few cases, terminology from the x86/x64 world has been carried
over into the ARM64 code ("MSR", "TSC").  Hyper-V still uses the
x86/x64 terminology and has not replaced it with something more
generic, so the code uses the Hyper-V terminology.  This will be
fixed when Hyper-V updates the usage in the TLFS.

This patch set is based on a 5.6-rc5 linux-next tree.

Changes in v6:
* Use SMCCC hypercall interface instead of direct invocation
  of HVC instruction and the Hyper-V hypercall interface
  [Marc Zyngier]
* Reimplemented functions to alloc/free Hyper-V size pages
  using kmalloc/kfree since kmalloc now guarantees alignment of
  power of 2 size allocations [Marc Zyngier]
* Export screen_info in arm64 architecture so it can be used
  by the Hyper-V buffer driver built as a module
* Renamed source file arch/arm64/hyperv/hv_init.c to hv_core.c
  to better reflect its content
* Fixed the bit position of certain feature flags presented by
  Hyper-V to the guest.  The bit positions on ARM64 don't match
  the position on x86 like originally thought.
* Minor fixups to rebase to 5.6-rc5 linux-next

Changes in v5:
* Minor fixups to rebase to 5.4-rc1 linux-next

Changes in v4:
* Moved clock-related code into an architecture independent
  Hyper-V clocksource driver that is already upstream. Clock
  related code is removed from this patch set except for the
  ARM64 specific interrupt handler. [Marc Zyngier]
* Separately upstreamed the split of mshyperv.h into arch independent
  and arch dependent portions. The arch independent portion has been
  removed from this patch set.
* Divided patch #2 of the series into multiple smaller patches
  [Marc Zyngier]
* Changed a dozen or so smaller things based on feedback
  [Marc Zyngier, Will Deacon]
* Added functions to alloc/free Hyper-V size pages for use by
  drivers for Hyper-V synthetic devices when updated to not assume
  guest page size and Hyper-v page size are the same

Changes in v3:
* Added initialization of hv_vp_index array like was recently
  added on x86 branch [KY Srinivasan]
* Changed Hyper-V ARM64 register symbols to be all uppercase 
  instead of mixed case [KY Srinivasan]
* Separated mshyperv.h into two files, one architecture
  independent and one architecture dependent. After this code
  is upstream, will make changes to the x86 code to use the
  architecture independent file and remove duplication. And
  once we have a multi-architecture Hyper-V TLFS, will do a
  separate patch to split hyperv-tlfs.h in the same way.
  [KY Srinivasan]
* Minor tweaks to rebase to latest linux-next code

Changes in v2:
* Removed patch to implement slow_virt_to_phys() on ARM64.
  Use of slow_virt_to_phys() in arch independent Hyper-V
  drivers has been eliminated by commit 6ba34171bcbd
  ("Drivers: hv: vmbus: Remove use of slow_virt_to_phys()")
* Minor tweaks to rebase to latest linux-next code

Michael Kelley (10):
  arm64: hyperv: Add core Hyper-V include files
  arm/arm64: smccc-1.1: Add vendor specific owner definition
  arm64: hyperv: Add hypercall and register access functions
  arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
  arm64: hyperv: Add interrupt handlers for VMbus and stimer
  arm64: hyperv: Add kexec and panic handlers
  arm64: hyperv: Initialize hypervisor on boot
  Drivers: hv: vmbus: Add hooks for per-CPU IRQ
  arm64: efi: Export screen_info
  Drivers: hv: Enable Hyper-V code to be built on ARM64

 MAINTAINERS                          |   3 +
 arch/arm64/Kbuild                    |   1 +
 arch/arm64/hyperv/Makefile           |   2 +
 arch/arm64/hyperv/hv_core.c          | 418 +++++++++++++++++++++++++++++++++++
 arch/arm64/hyperv/mshyperv.c         | 165 ++++++++++++++
 arch/arm64/include/asm/hyperv-tlfs.h | 413 ++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/mshyperv.h    | 115 ++++++++++
 arch/arm64/kernel/efi.c              |   1 +
 arch/x86/include/asm/mshyperv.h      |   4 +
 drivers/hv/Kconfig                   |   3 +-
 drivers/hv/hv.c                      |   3 +
 include/asm-generic/mshyperv.h       |   5 +
 include/linux/arm-smccc.h            |   1 +
 13 files changed, 1133 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/hyperv/Makefile
 create mode 100644 arch/arm64/hyperv/hv_core.c
 create mode 100644 arch/arm64/hyperv/mshyperv.c
 create mode 100644 arch/arm64/include/asm/hyperv-tlfs.h
 create mode 100644 arch/arm64/include/asm/mshyperv.h

-- 
1.8.3.1

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

[Michael Kelley]

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@microsoft.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

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@microsoft.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)
+#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)
+
+/* STIMER direct mode is available */
+#define HV_STIMER_DIRECT_MODE_AVAILABLE		BIT(13)
+
+/*
+ * Implementation recommendations in register
+ * HvRegisterFeaturesInfo. Indicates which behaviors the hypervisor
+ * recommends the OS implement for optimal performance.
+ */
+
+/*
+ * Recommend not using Auto EOI
+ */
+#define HV_DEPRECATING_AEOI_RECOMMENDED		BIT(9)
+
+/*
+ * Synthetic register definitions equivalent to MSRs on x86/x64
+ */
+#define HV_REGISTER_CRASH_P0		0x00000210
+#define HV_REGISTER_CRASH_P1		0x00000211
+#define HV_REGISTER_CRASH_P2		0x00000212
+#define HV_REGISTER_CRASH_P3		0x00000213
+#define HV_REGISTER_CRASH_P4		0x00000214
+#define HV_REGISTER_CRASH_CTL		0x00000215
+
+#define HV_REGISTER_GUEST_OSID		0x00090002
+#define HV_REGISTER_VPINDEX		0x00090003
+#define HV_REGISTER_TIME_REFCOUNT	0x00090004
+#define HV_REGISTER_REFERENCE_TSC	0x00090017
+
+#define HV_REGISTER_SINT0		0x000A0000
+#define HV_REGISTER_SINT1		0x000A0001
+#define HV_REGISTER_SINT2		0x000A0002
+#define HV_REGISTER_SINT3		0x000A0003
+#define HV_REGISTER_SINT4		0x000A0004
+#define HV_REGISTER_SINT5		0x000A0005
+#define HV_REGISTER_SINT6		0x000A0006
+#define HV_REGISTER_SINT7		0x000A0007
+#define HV_REGISTER_SINT8		0x000A0008
+#define HV_REGISTER_SINT9		0x000A0009
+#define HV_REGISTER_SINT10		0x000A000A
+#define HV_REGISTER_SINT11		0x000A000B
+#define HV_REGISTER_SINT12		0x000A000C
+#define HV_REGISTER_SINT13		0x000A000D
+#define HV_REGISTER_SINT14		0x000A000E
+#define HV_REGISTER_SINT15		0x000A000F
+#define HV_REGISTER_SCONTROL		0x000A0010
+#define HV_REGISTER_SVERSION		0x000A0011
+#define HV_REGISTER_SIFP		0x000A0012
+#define HV_REGISTER_SIPP		0x000A0013
+#define HV_REGISTER_EOM			0x000A0014
+#define HV_REGISTER_SIRBP		0x000A0015
+
+#define HV_REGISTER_STIMER0_CONFIG	0x000B0000
+#define HV_REGISTER_STIMER0_COUNT	0x000B0001
+#define HV_REGISTER_STIMER1_CONFIG	0x000B0002
+#define HV_REGISTER_STIMER1_COUNT	0x000B0003
+#define HV_REGISTER_STIMER2_CONFIG	0x000B0004
+#define HV_REGISTER_STIMER2_COUNT	0x000B0005
+#define HV_REGISTER_STIMER3_CONFIG	0x000B0006
+#define HV_REGISTER_STIMER3_COUNT	0x000B0007
+
+/*
+ * Crash notification flags.
+ */
+#define HV_CRASH_CTL_CRASH_NOTIFY_MSG	BIT_ULL(62)
+#define HV_CRASH_CTL_CRASH_NOTIFY	BIT_ULL(63)
+
+/*
+ * The guest OS needs to register the guest ID with the hypervisor.
+ * The guest ID is a 64 bit entity and the structure of this ID is
+ * specified in the Hyper-V TLFS.
+ */
+#define HV_LINUX_VENDOR_ID              0x8100
+
+/* Declare the various hypercall operations. */
+#define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE	0x0002
+#define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST	0x0003
+#define HVCALL_NOTIFY_LONG_SPIN_WAIT		0x0008
+#define HVCALL_SEND_IPI				0x000b
+#define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX	0x0013
+#define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX	0x0014
+#define HVCALL_SEND_IPI_EX			0x0015
+#define HVCALL_GET_VP_REGISTERS			0x0050
+#define HVCALL_SET_VP_REGISTERS			0x0051
+#define HVCALL_POST_MESSAGE			0x005c
+#define HVCALL_SIGNAL_EVENT			0x005d
+#define HVCALL_RETARGET_INTERRUPT		0x007e
+#define HVCALL_START_VIRTUAL_PROCESSOR		0x0099
+
+/* Declare standard hypercall field values. */
+#define HV_PARTITION_ID_SELF                    ((u64)-1)
+#define HV_VP_INDEX_SELF                        ((u32)-2)
+
+#define HV_HYPERCALL_FAST_BIT                   BIT(16)
+#define HV_HYPERCALL_REP_COUNT_1                BIT_ULL(32)
+#define HV_HYPERCALL_RESULT_MASK                GENMASK_ULL(15, 0)
+
+/* Define the hypercall status result */
+
+union hv_hypercall_status {
+	u64 as_uint64;
+	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)
+#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)
+
+#define HV_SYNIC_STIMER_COUNT		(4)
+
+/* Define synthetic interrupt controller message constants. */
+#define HV_MESSAGE_SIZE			(256)
+#define HV_MESSAGE_PAYLOAD_BYTE_COUNT	(240)
+#define HV_MESSAGE_PAYLOAD_QWORD_COUNT	(30)
+
+/* Define hypervisor message types. */
+enum hv_message_type {
+	HVMSG_NONE			= 0x00000000,
+
+	/* Memory access messages. */
+	HVMSG_UNMAPPED_GPA		= 0x80000000,
+	HVMSG_GPA_INTERCEPT		= 0x80000001,
+
+	/* Timer notification messages. */
+	HVMSG_TIMER_EXPIRED		= 0x80000010,
+
+	/* Error messages. */
+	HVMSG_INVALID_VP_REGISTER_VALUE	= 0x80000020,
+	HVMSG_UNRECOVERABLE_EXCEPTION	= 0x80000021,
+	HVMSG_UNSUPPORTED_FEATURE	= 0x80000022,
+
+	/* Trace buffer complete messages. */
+	HVMSG_EVENTLOG_BUFFERCOMPLETE	= 0x80000040,
+};
+
+/* Define synthetic interrupt controller message flags. */
+union hv_message_flags {
+	__u8 asu8;
+	struct {
+		__u8 msg_pending:1;
+		__u8 reserved:7;
+	} __packed;
+};
+
+/* Define port identifier type. */
+union hv_port_id {
+	__u32 asu32;
+	struct {
+		__u32 id:24;
+		__u32 reserved:8;
+	}  __packed u;
+};
+
+/* Define synthetic interrupt controller message header. */
+struct hv_message_header {
+	__u32 message_type;
+	__u8 payload_size;
+	union hv_message_flags message_flags;
+	__u8 reserved[2];
+	union {
+		__u64 sender;
+		union hv_port_id port;
+	};
+} __packed;
+
+/* Define synthetic interrupt controller message format. */
+struct hv_message {
+	struct hv_message_header header;
+	union {
+		__u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
+	} u;
+} __packed;
+
+/* Define the synthetic interrupt message page layout. */
+struct hv_message_page {
+	struct hv_message sint_message[HV_SYNIC_SINT_COUNT];
+} __packed;
+
+/* Define timer message payload structure. */
+struct hv_timer_message_payload {
+	__u32 timer_index;
+	__u32 reserved;
+	__u64 expiration_time;	/* When the timer expired */
+	__u64 delivery_time;	/* When the message was delivered */
+} __packed;
+
+#define HV_STIMER_ENABLE		(1ULL << 0)
+#define HV_STIMER_PERIODIC		(1ULL << 1)
+#define HV_STIMER_LAZY			(1ULL << 2)
+#define HV_STIMER_AUTOENABLE		(1ULL << 3)
+#define HV_STIMER_SINT(config)		(__u8)(((config) >> 16) & 0x0F)
+
+
+/* Define synthetic interrupt controller flag constants. */
+#define HV_EVENT_FLAGS_COUNT		(256 * 8)
+#define HV_EVENT_FLAGS_LONG_COUNT	(256 / sizeof(unsigned long))
+
+/*
+ * Timer configuration register.
+ */
+union hv_stimer_config {
+	u64 as_uint64;
+	struct {
+		u64 enable:1;
+		u64 periodic:1;
+		u64 lazy:1;
+		u64 auto_enable:1;
+		u64 apic_vector:8;
+		u64 direct_mode:1;
+		u64 reserved_z0:3;
+		u64 sintx:4;
+		u64 reserved_z1:44;
+	} __packed;
+};
+
+
+/* Define the synthetic interrupt controller event flags format. */
+union hv_synic_event_flags {
+	unsigned long flags[HV_EVENT_FLAGS_LONG_COUNT];
+};
+
+/* Define SynIC control register. */
+union hv_synic_scontrol {
+	u64 as_uint64;
+	struct {
+		u64 enable:1;
+		u64 reserved:63;
+	} __packed;
+};
+
+/* Define synthetic interrupt source. */
+union hv_synic_sint {
+	u64 as_uint64;
+	struct {
+		u64 vector:8;
+		u64 reserved1:8;
+		u64 masked:1;
+		u64 auto_eoi:1;
+		u64 reserved2:46;
+	} __packed;
+};
+
+/* Define the format of the SIMP register */
+union hv_synic_simp {
+	u64 as_uint64;
+	struct {
+		u64 simp_enabled:1;
+		u64 preserved:11;
+		u64 base_simp_gpa:52;
+	} __packed;
+};
+
+/* Define the format of the SIEFP register */
+union hv_synic_siefp {
+	u64 as_uint64;
+	struct {
+		u64 siefp_enabled:1;
+		u64 preserved:11;
+		u64 base_siefp_gpa:52;
+	} __packed;
+};
+
+struct hv_vpset {
+	u64 format;
+	u64 valid_bank_mask;
+	u64 bank_contents[];
+} __packed;
+
+
+#endif
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@microsoft.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.
+ * 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)
+#define hv_enable_stimer0_percpu_irq(irq)	enable_percpu_irq(irq, 0)
+#define hv_disable_stimer0_percpu_irq(irq)	disable_percpu_irq(irq)
+#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,	\
+				HV_SMCCC_FUNC_NUMBER)
+
+#include <asm-generic/mshyperv.h>
+
+#endif
-- 
1.8.3.1

[PATCH v6 02/10] arm/arm64: smccc-1.1: Add vendor specific owner definition

[Michael Kelley]

In the SMCCC v1.1 spec, the Owning Entity Number in the Function
Identifier may have a value of 6 for a vendor specific hypervisor
service call. Add this #define.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 include/linux/arm-smccc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 59494df..8229203 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -46,6 +46,7 @@
 #define ARM_SMCCC_OWNER_OEM		3
 #define ARM_SMCCC_OWNER_STANDARD	4
 #define ARM_SMCCC_OWNER_STANDARD_HYP	5
+#define ARM_SMCCC_OWNER_VENDOR_HYP	6
 #define ARM_SMCCC_OWNER_TRUSTED_APP	48
 #define ARM_SMCCC_OWNER_TRUSTED_APP_END	49
 #define ARM_SMCCC_OWNER_TRUSTED_OS	50
-- 
1.8.3.1

[PATCH v6 03/10] arm64: hyperv: Add hypercall and register access functions

[Michael Kelley]

Add ARM64-specific code to make Hyper-V hypercalls and to
access virtual processor synthetic registers via hypercalls.
Hypercalls follow the SMC Calling Convention spec v1.1.

This code is architecture dependent and is mostly driven by
architecture independent code in the VMbus driver and the
Hyper-V timer clocksource driver.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 MAINTAINERS                 |   1 +
 arch/arm64/Kbuild           |   1 +
 arch/arm64/hyperv/Makefile  |   2 +
 arch/arm64/hyperv/hv_core.c | 167 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 171 insertions(+)
 create mode 100644 arch/arm64/hyperv/Makefile
 create mode 100644 arch/arm64/hyperv/hv_core.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 398cfdb..64ad2bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7811,6 +7811,7 @@ 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:	arch/arm64/hyperv
 F:	drivers/clocksource/hyperv_timer.c
 F:	drivers/hid/hid-hyperv.c
 F:	drivers/hv/
diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
index d646582..7a37608 100644
--- a/arch/arm64/Kbuild
+++ b/arch/arm64/Kbuild
@@ -3,4 +3,5 @@ obj-y			+= kernel/ mm/
 obj-$(CONFIG_NET)	+= net/
 obj-$(CONFIG_KVM)	+= kvm/
 obj-$(CONFIG_XEN)	+= xen/
+obj-$(subst m,y,$(CONFIG_HYPERV))	+= hyperv/
 obj-$(CONFIG_CRYPTO)	+= crypto/
diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
new file mode 100644
index 0000000..1697d30
--- /dev/null
+++ b/arch/arm64/hyperv/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y		:= hv_core.o
diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
new file mode 100644
index 0000000..272c348
--- /dev/null
+++ b/arch/arm64/hyperv/hv_core.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Initialization of the interface with Microsoft's Hyper-V hypervisor,
+ * and various low level utility routines for interacting with Hyper-V.
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+
+#include <linux/types.h>
+#include <linux/version.h>
+#include <linux/export.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/hyperv.h>
+#include <linux/arm-smccc.h>
+#include <asm-generic/bug.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+
+/*
+ * hv_do_hypercall- Invoke the specified hypercall
+ */
+u64 hv_do_hypercall(u64 control, void *input, void *output)
+{
+	u64 input_address;
+	u64 output_address;
+	struct arm_smccc_res res;
+
+	input_address = input ? virt_to_phys(input) : 0;
+	output_address = output ? virt_to_phys(output) : 0;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control,
+			  input_address, output_address, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_hypercall);
+
+/*
+ * hv_do_fast_hypercall8 -- Invoke the specified hypercall
+ * with arguments in registers instead of physical memory.
+ * Avoids the overhead of virt_to_phys for simple hypercalls.
+ */
+
+u64 hv_do_fast_hypercall8(u16 code, u64 input)
+{
+	u64 control;
+	struct arm_smccc_res res;
+
+	control = (u64)code | HV_HYPERCALL_FAST_BIT;
+
+	arm_smccc_1_1_hvc(HV_FUNC_ID, control, input, &res);
+	return res.a0;
+}
+EXPORT_SYMBOL_GPL(hv_do_fast_hypercall8);
+
+
+/*
+ * Set a single VP register to a 64-bit value.
+ */
+void hv_set_vpreg(u32 msr, u64 value)
+{
+	union hv_hypercall_status status;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_hvc(
+		HV_FUNC_ID,
+		HVCALL_SET_VP_REGISTERS | HV_HYPERCALL_FAST_BIT |
+			HV_HYPERCALL_REP_COUNT_1,
+		HV_PARTITION_ID_SELF,
+		HV_VP_INDEX_SELF,
+		msr,
+		0,
+		value,
+		0,
+		&res);
+	status.as_uint64 = res.a0;
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * setting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON(status.status != HV_STATUS_SUCCESS);
+}
+EXPORT_SYMBOL_GPL(hv_set_vpreg);
+
+/*
+ * Get the value of a single VP register.  One version
+ * returns just 64 bits and another returns the full 128 bits.
+ * The two versions are separate to avoid complicating the
+ * calling sequence for the more frequently used 64 bit version.
+ */
+
+/*
+ * Input and output memory allocation sizes are rounded up to a power
+ * of 2 so kmalloc() will guarantee alignment. In turn, the alignment
+ * ensures that the allocations don't cross a page boundary, which is
+ * required by the hypercall interface.
+ */
+#define INPUTSIZE (4 * sizeof(u64))
+#define OUTPUTSIZE (2 * sizeof(u64))
+
+static void __hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
+{
+	union hv_hypercall_status		status;
+	struct hv_get_vp_register_input		*input;
+
+	BUILD_BUG_ON(sizeof(*input) > INPUTSIZE);
+
+	input = kzalloc(INPUTSIZE, GFP_ATOMIC);
+
+	input->partitionid = HV_PARTITION_ID_SELF;
+	input->vpindex = HV_VP_INDEX_SELF;
+	input->inputvtl = 0;
+	input->name0 = msr;
+	input->name1 = 0;
+
+
+	status.as_uint64 = hv_do_hypercall(
+		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COUNT_1,
+		input, res);
+
+	/*
+	 * Something is fundamentally broken in the hypervisor if
+	 * getting a VP register fails. There's really no way to
+	 * continue as a guest VM, so panic.
+	 */
+	BUG_ON(status.status != HV_STATUS_SUCCESS);
+
+	kfree(input);
+}
+
+u64 hv_get_vpreg(u32 msr)
+{
+	struct hv_get_vp_register_output	*output;
+	u64					result;
+
+	BUILD_BUG_ON(sizeof(*output) > OUTPUTSIZE);
+	output = kmalloc(OUTPUTSIZE, GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, output);
+
+	result = output->registervaluelow;
+	kfree(output);
+	return result;
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg);
+
+void hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
+{
+	struct hv_get_vp_register_output	*output;
+
+	BUILD_BUG_ON(sizeof(*output) > OUTPUTSIZE);
+	output = kmalloc(OUTPUTSIZE, GFP_ATOMIC);
+
+	__hv_get_vpreg_128(msr, output);
+
+	res->registervaluelow = output->registervaluelow;
+	res->registervaluehigh = output->registervaluehigh;
+	kfree(output);
+}
+EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
-- 
1.8.3.1

[PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Michael Kelley]

Add ARM64-specific code to allocate memory with HV_HYP_PAGE_SIZE
size and alignment. These are for use when pages need to be shared
with Hyper-V. Separate functions are needed as the page size used
by Hyper-V may not be the same as the guest page size.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/arm64/hyperv/hv_core.c    | 34 ++++++++++++++++++++++++++++++++++
 include/asm-generic/mshyperv.h |  5 +++++
 2 files changed, 39 insertions(+)

diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
index 272c348..4aa6b8f 100644
--- a/arch/arm64/hyperv/hv_core.c
+++ b/arch/arm64/hyperv/hv_core.c
@@ -17,12 +17,46 @@
 #include <linux/slab.h>
 #include <linux/hyperv.h>
 #include <linux/arm-smccc.h>
+#include <linux/string.h>
 #include <asm-generic/bug.h>
 #include <asm/hyperv-tlfs.h>
 #include <asm/mshyperv.h>
 
 
 /*
+ * Functions for allocating and freeing memory with size and
+ * alignment HV_HYP_PAGE_SIZE. These functions are needed because
+ * the guest page size may not be the same as the Hyper-V page
+ * size. We depend upon kmalloc() aligning power-of-two size
+ * allocations to the allocation size boundary, so that the
+ * allocated memory appears to Hyper-V as a page of the size
+ * it expects.
+ *
+ * These functions are used by arm64 specific code as well as
+ * arch independent Hyper-V drivers.
+ */
+
+void *hv_alloc_hyperv_page(void)
+{
+	BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
+	return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
+
+void *hv_alloc_hyperv_zeroed_page(void)
+{
+	return kzalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(hv_alloc_hyperv_zeroed_page);
+
+void hv_free_hyperv_page(unsigned long addr)
+{
+	kfree((void *)addr);
+}
+EXPORT_SYMBOL_GPL(hv_free_hyperv_page);
+
+
+/*
  * hv_do_hypercall- Invoke the specified hypercall
  */
 u64 hv_do_hypercall(u64 control, void *input, void *output)
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index b3f1082..1a7c4c5 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -99,6 +99,11 @@ static inline void vmbus_signal_eom(struct hv_message *msg, u32 old_msg_type)
 void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs));
 void hv_remove_crash_handler(void);
 
+void *hv_alloc_hyperv_page(void);
+void *hv_alloc_hyperv_zeroed_page(void);
+void hv_free_hyperv_page(unsigned long addr);
+
+
 #if IS_ENABLED(CONFIG_HYPERV)
 /*
  * Hypervisor's notion of virtual processor ID is different from
-- 
1.8.3.1

[PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer

[Michael Kelley]

Add ARM64-specific code to set up and handle the interrupts
generated by Hyper-V for VMbus messages and for stimer expiration.

This code is architecture dependent and is mostly driven by
architecture independent code in the VMbus driver and the
Hyper-V timer clocksource driver.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/arm64/hyperv/Makefile   |   2 +-
 arch/arm64/hyperv/mshyperv.c | 139 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/hyperv/mshyperv.c

diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
index 1697d30..87c31c0 100644
--- a/arch/arm64/hyperv/Makefile
+++ b/arch/arm64/hyperv/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-y		:= hv_core.o
+obj-y		:= hv_core.o mshyperv.o
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
new file mode 100644
index 0000000..ae6ece6
--- /dev/null
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Core routines for interacting with Microsoft's Hyper-V hypervisor,
+ * including setting up VMbus and STIMER interrupts, and handling
+ * crashes and kexecs. These interactions are through a set of
+ * static "handler" variables set by the architecture independent
+ * VMbus and STIMER drivers.
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author : Michael Kelley <mikelley@microsoft.com>
+ */
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/interrupt.h>
+#include <linux/kexec.h>
+#include <linux/acpi.h>
+#include <linux/ptrace.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+static void (*vmbus_handler)(void);
+static void (*hv_stimer0_handler)(void);
+
+static int vmbus_irq;
+static long __percpu *vmbus_evt;
+static long __percpu *stimer0_evt;
+
+irqreturn_t hyperv_vector_handler(int irq, void *dev_id)
+{
+	vmbus_handler();
+	return IRQ_HANDLED;
+}
+
+/* Must be done just once */
+void hv_setup_vmbus_irq(void (*handler)(void))
+{
+	int result;
+
+	vmbus_handler = handler;
+	vmbus_irq = acpi_register_gsi(NULL, HYPERVISOR_CALLBACK_VECTOR,
+				 ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+	if (vmbus_irq <= 0) {
+		pr_err("Can't register Hyper-V VMBus GSI. Error %d",
+			vmbus_irq);
+		vmbus_irq = 0;
+		return;
+	}
+	vmbus_evt = alloc_percpu(long);
+	result = request_percpu_irq(vmbus_irq, hyperv_vector_handler,
+			"Hyper-V VMbus", vmbus_evt);
+	if (result) {
+		pr_err("Can't request Hyper-V VMBus IRQ %d. Error %d",
+			vmbus_irq, result);
+		free_percpu(vmbus_evt);
+		acpi_unregister_gsi(vmbus_irq);
+		vmbus_irq = 0;
+	}
+}
+EXPORT_SYMBOL_GPL(hv_setup_vmbus_irq);
+
+/* Must be done just once */
+void hv_remove_vmbus_irq(void)
+{
+	if (vmbus_irq) {
+		free_percpu_irq(vmbus_irq, vmbus_evt);
+		free_percpu(vmbus_evt);
+		acpi_unregister_gsi(vmbus_irq);
+	}
+}
+EXPORT_SYMBOL_GPL(hv_remove_vmbus_irq);
+
+/* Must be done by each CPU */
+void hv_enable_vmbus_irq(void)
+{
+	enable_percpu_irq(vmbus_irq, 0);
+}
+EXPORT_SYMBOL_GPL(hv_enable_vmbus_irq);
+
+/* Must be done by each CPU */
+void hv_disable_vmbus_irq(void)
+{
+	disable_percpu_irq(vmbus_irq);
+}
+EXPORT_SYMBOL_GPL(hv_disable_vmbus_irq);
+
+/* Routines to do per-architecture handling of STIMER0 when in Direct Mode */
+
+static irqreturn_t hv_stimer0_vector_handler(int irq, void *dev_id)
+{
+	if (hv_stimer0_handler)
+		hv_stimer0_handler();
+	return IRQ_HANDLED;
+}
+
+int hv_setup_stimer0_irq(int *irq, int *vector, void (*handler)(void))
+{
+	int localirq;
+	int result;
+
+	localirq = acpi_register_gsi(NULL, HV_STIMER0_IRQNR,
+			ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+	if (localirq <= 0) {
+		pr_err("Can't register Hyper-V stimer0 GSI. Error %d",
+			localirq);
+		*irq = 0;
+		return -1;
+	}
+	stimer0_evt = alloc_percpu(long);
+	result = request_percpu_irq(localirq, hv_stimer0_vector_handler,
+					 "Hyper-V stimer0", stimer0_evt);
+	if (result) {
+		pr_err("Can't request Hyper-V stimer0 IRQ %d. Error %d",
+			localirq, result);
+		free_percpu(stimer0_evt);
+		acpi_unregister_gsi(localirq);
+		*irq = 0;
+		return -1;
+	}
+
+	hv_stimer0_handler = handler;
+	*vector = HV_STIMER0_IRQNR;
+	*irq = localirq;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hv_setup_stimer0_irq);
+
+void hv_remove_stimer0_irq(int irq)
+{
+	hv_stimer0_handler = NULL;
+	if (irq) {
+		free_percpu_irq(irq, stimer0_evt);
+		free_percpu(stimer0_evt);
+		acpi_unregister_gsi(irq);
+	}
+}
+EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
-- 
1.8.3.1

[PATCH v6 06/10] arm64: hyperv: Add kexec and panic handlers

[Michael Kelley]

Add functions to set up and remove kexec and panic
handlers, and to inform Hyper-V about a guest panic.
These functions are called from architecture independent
code in the VMbus driver.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/arm64/hyperv/hv_core.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/hyperv/mshyperv.c | 26 +++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
index 4aa6b8f..8d6de9f 100644
--- a/arch/arm64/hyperv/hv_core.c
+++ b/arch/arm64/hyperv/hv_core.c
@@ -199,3 +199,64 @@ void hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
 	kfree(output);
 }
 EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
+
+void hyperv_report_panic(struct pt_regs *regs, long err)
+{
+	static bool panic_reported;
+	u64 guest_id;
+
+	/*
+	 * We prefer to report panic on 'die' chain as we have proper
+	 * registers to report, but if we miss it (e.g. on BUG()) we need
+	 * to report it on 'panic'.
+	 */
+	if (panic_reported)
+		return;
+	panic_reported = true;
+
+	guest_id = hv_get_vpreg(HV_REGISTER_GUEST_OSID);
+
+	/*
+	 * Hyper-V provides the ability to store only 5 values.
+	 * Pick the passed in error value, the guest_id, and the PC.
+	 * The first two general registers are added arbitrarily.
+	 */
+	hv_set_vpreg(HV_REGISTER_CRASH_P0, err);
+	hv_set_vpreg(HV_REGISTER_CRASH_P1, guest_id);
+	hv_set_vpreg(HV_REGISTER_CRASH_P2, regs->pc);
+	hv_set_vpreg(HV_REGISTER_CRASH_P3, regs->regs[0]);
+	hv_set_vpreg(HV_REGISTER_CRASH_P4, regs->regs[1]);
+
+	/*
+	 * Let Hyper-V know there is crash data available
+	 */
+	hv_set_vpreg(HV_REGISTER_CRASH_CTL, HV_CRASH_CTL_CRASH_NOTIFY);
+}
+EXPORT_SYMBOL_GPL(hyperv_report_panic);
+
+/*
+ * hyperv_report_panic_msg - report panic message to Hyper-V
+ * @pa: physical address of the panic page containing the message
+ * @size: size of the message in the page
+ */
+void hyperv_report_panic_msg(phys_addr_t pa, size_t size)
+{
+	/*
+	 * P3 to contain the physical address of the panic page & P4 to
+	 * contain the size of the panic data in that page. Rest of the
+	 * registers are no-op when the NOTIFY_MSG flag is set.
+	 */
+	hv_set_vpreg(HV_REGISTER_CRASH_P0, 0);
+	hv_set_vpreg(HV_REGISTER_CRASH_P1, 0);
+	hv_set_vpreg(HV_REGISTER_CRASH_P2, 0);
+	hv_set_vpreg(HV_REGISTER_CRASH_P3, pa);
+	hv_set_vpreg(HV_REGISTER_CRASH_P4, size);
+
+	/*
+	 * Let Hyper-V know there is crash data available along with
+	 * the panic message.
+	 */
+	hv_set_vpreg(HV_REGISTER_CRASH_CTL,
+	       (HV_CRASH_CTL_CRASH_NOTIFY | HV_CRASH_CTL_CRASH_NOTIFY_MSG));
+}
+EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index ae6ece6..c58940d 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -23,6 +23,8 @@
 
 static void (*vmbus_handler)(void);
 static void (*hv_stimer0_handler)(void);
+static void (*hv_kexec_handler)(void);
+static void (*hv_crash_handler)(struct pt_regs *regs);
 
 static int vmbus_irq;
 static long __percpu *vmbus_evt;
@@ -137,3 +139,27 @@ void hv_remove_stimer0_irq(int irq)
 	}
 }
 EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
+
+void hv_setup_kexec_handler(void (*handler)(void))
+{
+	hv_kexec_handler = handler;
+}
+EXPORT_SYMBOL_GPL(hv_setup_kexec_handler);
+
+void hv_remove_kexec_handler(void)
+{
+	hv_kexec_handler = NULL;
+}
+EXPORT_SYMBOL_GPL(hv_remove_kexec_handler);
+
+void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs))
+{
+	hv_crash_handler = handler;
+}
+EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
+
+void hv_remove_crash_handler(void)
+{
+	hv_crash_handler = NULL;
+}
+EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
-- 
1.8.3.1

[PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot

[Michael Kelley]

Add ARM64-specific code to initialize the Hyper-V
hypervisor when booting as a guest VM. Provide functions
and data structures indicating hypervisor status that
are needed by VMbus driver.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/arm64/hyperv/hv_core.c | 156 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
index 8d6de9f..b496f59 100644
--- a/arch/arm64/hyperv/hv_core.c
+++ b/arch/arm64/hyperv/hv_core.c
@@ -13,14 +13,47 @@
 #include <linux/types.h>
 #include <linux/version.h>
 #include <linux/export.h>
+#include <linux/vmalloc.h>
 #include <linux/mm.h>
+#include <linux/acpi.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/hyperv.h>
 #include <linux/arm-smccc.h>
 #include <linux/string.h>
+#include <linux/cpuhotplug.h>
+#include <linux/psci.h>
+#include <linux/sched_clock.h>
 #include <asm-generic/bug.h>
 #include <asm/hyperv-tlfs.h>
 #include <asm/mshyperv.h>
+#include <asm/sysreg.h>
+#include <clocksource/hyperv_timer.h>
+
+static bool    hyperv_initialized;
+
+struct         ms_hyperv_info ms_hyperv __ro_after_init;
+EXPORT_SYMBOL_GPL(ms_hyperv);
+
+u32            *hv_vp_index;
+EXPORT_SYMBOL_GPL(hv_vp_index);
+
+u32            hv_max_vp_index;
+EXPORT_SYMBOL_GPL(hv_max_vp_index);
+
+static int hv_cpu_init(unsigned int cpu)
+{
+	u64 msr_vp_index;
+
+	hv_get_vp_index(msr_vp_index);
+
+	hv_vp_index[smp_processor_id()] = msr_vp_index;
+
+	if (msr_vp_index > hv_max_vp_index)
+		hv_max_vp_index = msr_vp_index;
+
+	return 0;
+}
 
 
 /*
@@ -57,6 +90,117 @@ void hv_free_hyperv_page(unsigned long addr)
 
 
 /*
+ * This function is invoked via the ACPI clocksource probe mechanism. We
+ * don't actually use any values from the ACPI GTDT table, but we set up
+ * the Hyper-V synthetic clocksource and do other initialization for
+ * interacting with Hyper-V the first time.  Using early_initcall to invoke
+ * this function is too late because interrupts are already enabled at that
+ * point, and hv_init_clocksource() must run before interrupts are enabled.
+ *
+ * 1. Setup the guest ID.
+ * 2. Get features and hints info from Hyper-V
+ * 3. Setup per-cpu VP indices.
+ * 4. Initialize the Hyper-V clocksource.
+ */
+
+static int __init hyperv_init(struct acpi_table_header *table)
+{
+	struct hv_get_vp_register_output result;
+	u32	a, b, c, d;
+	u64	guest_id;
+	int	i, cpuhp;
+
+	/*
+	 * If we're in a VM on Hyper-V, the ACPI hypervisor_id field will
+	 * have the string "MsHyperV".
+	 */
+	if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
+		return -EINVAL;
+
+	/* Setup the guest ID */
+	guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
+	hv_set_vpreg(HV_REGISTER_GUEST_OSID, guest_id);
+
+	/* Get the features and hints from Hyper-V */
+	hv_get_vpreg_128(HV_REGISTER_PRIVILEGES_AND_FEATURES, &result);
+	ms_hyperv.features = lower_32_bits(result.registervaluelow);
+	ms_hyperv.misc_features = lower_32_bits(result.registervaluehigh);
+
+	hv_get_vpreg_128(HV_REGISTER_FEATURES, &result);
+	ms_hyperv.hints = lower_32_bits(result.registervaluelow);
+
+	pr_info("Hyper-V: Features 0x%x, misc features 0x%x, hints 0x%x\n",
+		ms_hyperv.features, ms_hyperv.misc_features, ms_hyperv.hints);
+
+	/*
+	 * Hyper-V on ARM64 doesn't support AutoEOI.  Add the hint
+	 * that tells architecture independent code not to use this
+	 * feature.
+	 */
+	ms_hyperv.hints |= HV_DEPRECATING_AEOI_RECOMMENDED;
+
+	/* Get information about the Hyper-V host version */
+	hv_get_vpreg_128(HV_REGISTER_HYPERVISOR_VERSION, &result);
+	a = lower_32_bits(result.registervaluelow);
+	b = upper_32_bits(result.registervaluelow);
+	c = lower_32_bits(result.registervaluehigh);
+	d = upper_32_bits(result.registervaluehigh);
+	pr_info("Hyper-V: Host Build %d.%d.%d.%d-%d-%d\n",
+		b >> 16, b & 0xFFFF, a, d & 0xFFFFFF, c, d >> 24);
+
+	/* Allocate and initialize percpu VP index array */
+	hv_vp_index = kmalloc_array(num_possible_cpus(), sizeof(*hv_vp_index),
+				    GFP_KERNEL);
+	if (!hv_vp_index)
+		return -ENOMEM;
+
+	for (i = 0; i < num_possible_cpus(); i++)
+		hv_vp_index[i] = VP_INVAL;
+
+	cpuhp = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+			"arm64/hyperv_init:online", hv_cpu_init, NULL);
+	if (cpuhp < 0)
+		goto free_vp_index;
+
+	hv_init_clocksource();
+	if (hv_stimer_alloc())
+		goto remove_cpuhp_state;
+
+	hyperv_initialized = true;
+	return 0;
+
+remove_cpuhp_state:
+	cpuhp_remove_state(cpuhp);
+free_vp_index:
+	kfree(hv_vp_index);
+	hv_vp_index = NULL;
+	return -EINVAL;
+}
+TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);
+
+
+/*
+ * Called from hv_init_clocksource() to do ARM64
+ * specific initialization of the sched clock
+ */
+void __init hv_setup_sched_clock(void *sched_clock)
+{
+	sched_clock_register(sched_clock, 64, HV_CLOCK_HZ);
+}
+
+/*
+ * This routine is called before kexec/kdump, it does the required cleanup.
+ */
+void hyperv_cleanup(void)
+{
+	/* Reset our OS id */
+	hv_set_vpreg(HV_REGISTER_GUEST_OSID, 0);
+
+}
+EXPORT_SYMBOL_GPL(hyperv_cleanup);
+
+
+/*
  * hv_do_hypercall- Invoke the specified hypercall
  */
 u64 hv_do_hypercall(u64 control, void *input, void *output)
@@ -260,3 +404,15 @@ void hyperv_report_panic_msg(phys_addr_t pa, size_t size)
 	       (HV_CRASH_CTL_CRASH_NOTIFY | HV_CRASH_CTL_CRASH_NOTIFY_MSG));
 }
 EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
+
+bool hv_is_hyperv_initialized(void)
+{
+	return hyperv_initialized;
+}
+EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
+
+bool hv_is_hibernation_supported(void)
+{
+	return false;
+}
+EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
-- 
1.8.3.1

[PATCH v6 08/10] Drivers: hv: vmbus: Add hooks for per-CPU IRQ

[Michael Kelley]

Add hooks to enable/disable a per-CPU IRQ for VMbus. These hooks
are in the architecture independent setup and shutdown paths for
Hyper-V, and are needed by Linux guests on Hyper-V on ARM64.  The
x86/x64 implementation is null because VMbus interrupts on x86/x64
don't use an IRQ.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/include/asm/mshyperv.h | 4 ++++
 drivers/hv/hv.c                 | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 1c42ecb..0e5db78 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -59,6 +59,10 @@ typedef int (*hyperv_fill_flush_list_func)(
 #endif
 void hyperv_vector_handler(struct pt_regs *regs);
 
+/* On x86/x64, there isn't a real IRQ to be enabled/disable */
+static inline void hv_enable_vmbus_irq(void) {}
+static inline void hv_disable_vmbus_irq(void) {}
+
 /*
  * Routines for stimer0 Direct Mode handling.
  * On x86/x64, there are no percpu actions to take.
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 6098e0c..035d3df 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -180,6 +180,7 @@ void hv_synic_enable_regs(unsigned int cpu)
 	hv_set_siefp(siefp.as_uint64);
 
 	/* Setup the shared SINT. */
+	hv_enable_vmbus_irq();
 	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
 
 	shared_sint.vector = HYPERVISOR_CALLBACK_VECTOR;
@@ -241,6 +242,8 @@ void hv_synic_disable_regs(unsigned int cpu)
 	hv_get_synic_state(sctrl.as_uint64);
 	sctrl.enable = 0;
 	hv_set_synic_state(sctrl.as_uint64);
+
+	hv_disable_vmbus_irq();
 }
 
 int hv_synic_cleanup(unsigned int cpu)
-- 
1.8.3.1

[PATCH v6 09/10] arm64: efi: Export screen_info

[Michael Kelley]

The Hyper-V frame buffer driver may be built as a module, and
it needs access to screen_info. So export screen_info.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/arm64/kernel/efi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index d0cf596..8ff557a 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -55,6 +55,7 @@ static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
 
 /* we will fill this structure from the stub, so don't put it in .bss */
 struct screen_info screen_info __section(.data);
+EXPORT_SYMBOL(screen_info);
 
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
-- 
1.8.3.1

[PATCH v6 10/10] Drivers: hv: Enable Hyper-V code to be built on ARM64

[Michael Kelley]

Update drivers/hv/Kconfig so CONFIG_HYPERV can be selected on
ARM64, causing the Hyper-V specific code to be built.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 79e5356..1113e49 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -4,7 +4,8 @@ menu "Microsoft Hyper-V guest support"
 
 config HYPERV
 	tristate "Microsoft Hyper-V client drivers"
-	depends on X86 && ACPI && X86_LOCAL_APIC && HYPERVISOR_GUEST
+	depends on ACPI && \
+			((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) || ARM64)
 	select PARAVIRT
 	select X86_HV_CALLBACK_VECTOR
 	help
-- 
1.8.3.1

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

[Marc Zyngier]

On Sat, 14 Mar 2020 15:35:10 +0000,
Hi Michael,

Michael Kelley <mikelley@microsoft.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@microsoft.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?

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@microsoft.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.

> +#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.

[...]

> +/* 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.

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

> +#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...

[...]

> 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@microsoft.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.

> + * 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.

> +#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.

> +#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...

> +				HV_SMCCC_FUNC_NUMBER)
> +
> +#include <asm-generic/mshyperv.h>
> +
> +#endif

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH v6 06/10] arm64: hyperv: Add kexec and panic handlers

[Marc Zyngier]

On Sat, 14 Mar 2020 15:35:15 +0000,
Michael Kelley <mikelley@microsoft.com> wrote:
> 
> Add functions to set up and remove kexec and panic
> handlers, and to inform Hyper-V about a guest panic.
> These functions are called from architecture independent
> code in the VMbus driver.
> 
> This code is built only when CONFIG_HYPERV is enabled.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  arch/arm64/hyperv/hv_core.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/hyperv/mshyperv.c | 26 +++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
> index 4aa6b8f..8d6de9f 100644
> --- a/arch/arm64/hyperv/hv_core.c
> +++ b/arch/arm64/hyperv/hv_core.c
> @@ -199,3 +199,64 @@ void hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
>  	kfree(output);
>  }
>  EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
> +
> +void hyperv_report_panic(struct pt_regs *regs, long err)
> +{
> +	static bool panic_reported;
> +	u64 guest_id;
> +
> +	/*
> +	 * We prefer to report panic on 'die' chain as we have proper
> +	 * registers to report, but if we miss it (e.g. on BUG()) we need
> +	 * to report it on 'panic'.
> +	 */
> +	if (panic_reported)
> +		return;
> +	panic_reported = true;

How does this work when multiple vcpus are crashing at once? Are you
guaranteed to be single-threaded at this point?

> +
> +	guest_id = hv_get_vpreg(HV_REGISTER_GUEST_OSID);
> +
> +	/*
> +	 * Hyper-V provides the ability to store only 5 values.
> +	 * Pick the passed in error value, the guest_id, and the PC.
> +	 * The first two general registers are added arbitrarily.
> +	 */
> +	hv_set_vpreg(HV_REGISTER_CRASH_P0, err);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P1, guest_id);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P2, regs->pc);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P3, regs->regs[0]);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P4, regs->regs[1]);

How about reporting useful information, a pointer to some data
structure describing the fault? As it is, the usefulness of this is
pretty dubious.

> +
> +	/*
> +	 * Let Hyper-V know there is crash data available
> +	 */
> +	hv_set_vpreg(HV_REGISTER_CRASH_CTL, HV_CRASH_CTL_CRASH_NOTIFY);
> +}
> +EXPORT_SYMBOL_GPL(hyperv_report_panic);
> +
> +/*
> + * hyperv_report_panic_msg - report panic message to Hyper-V
> + * @pa: physical address of the panic page containing the message
> + * @size: size of the message in the page
> + */
> +void hyperv_report_panic_msg(phys_addr_t pa, size_t size)
> +{
> +	/*
> +	 * P3 to contain the physical address of the panic page & P4 to
> +	 * contain the size of the panic data in that page. Rest of the
> +	 * registers are no-op when the NOTIFY_MSG flag is set.
> +	 */
> +	hv_set_vpreg(HV_REGISTER_CRASH_P0, 0);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P1, 0);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P2, 0);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P3, pa);
> +	hv_set_vpreg(HV_REGISTER_CRASH_P4, size);
> +
> +	/*
> +	 * Let Hyper-V know there is crash data available along with
> +	 * the panic message.
> +	 */
> +	hv_set_vpreg(HV_REGISTER_CRASH_CTL,
> +	       (HV_CRASH_CTL_CRASH_NOTIFY | HV_CRASH_CTL_CRASH_NOTIFY_MSG));
> +}
> +EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index ae6ece6..c58940d 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -23,6 +23,8 @@
>  
>  static void (*vmbus_handler)(void);
>  static void (*hv_stimer0_handler)(void);
> +static void (*hv_kexec_handler)(void);
> +static void (*hv_crash_handler)(struct pt_regs *regs);

Why is this in the arch-specific code? Yes, it lives in the x86 arch
code too, but I don't see what prevents it from being moved to the
vmbus_drv.c code.

>  
>  static int vmbus_irq;
>  static long __percpu *vmbus_evt;
> @@ -137,3 +139,27 @@ void hv_remove_stimer0_irq(int irq)
>  	}
>  }
>  EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
> +
> +void hv_setup_kexec_handler(void (*handler)(void))
> +{
> +	hv_kexec_handler = handler;
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_kexec_handler);
> +
> +void hv_remove_kexec_handler(void)
> +{
> +	hv_kexec_handler = NULL;
> +}
> +EXPORT_SYMBOL_GPL(hv_remove_kexec_handler);
> +
> +void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs))
> +{
> +	hv_crash_handler = handler;
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
> +
> +void hv_remove_crash_handler(void)
> +{
> +	hv_crash_handler = NULL;
> +}
> +EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
> -- 
> 1.8.3.1
> 
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Arnd Bergmann]

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>
> The Hyper-V frame buffer driver may be built as a module, and
> it needs access to screen_info. So export screen_info.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Is there any chance of using a more modern KMS based driver for the screen
than the old fbdev subsystem? I had hoped to one day completely remove
support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
architectures.

     Arnd

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Arnd Bergmann]

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>  /*
> + * Functions for allocating and freeing memory with size and
> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> + * the guest page size may not be the same as the Hyper-V page
> + * size. We depend upon kmalloc() aligning power-of-two size
> + * allocations to the allocation size boundary, so that the
> + * allocated memory appears to Hyper-V as a page of the size
> + * it expects.
> + *
> + * These functions are used by arm64 specific code as well as
> + * arch independent Hyper-V drivers.
> + */
> +
> +void *hv_alloc_hyperv_page(void)
> +{
> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);

I don't think there is any guarantee that kmalloc() returns page-aligned
allocations in general. How about using get_free_pages()
to implement this?

       Arnd

Re: [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer

[Arnd Bergmann]

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>
> Add ARM64-specific code to set up and handle the interrupts
> generated by Hyper-V for VMbus messages and for stimer expiration.
>
> This code is architecture dependent and is mostly driven by
> architecture independent code in the VMbus driver and the
> Hyper-V timer clocksource driver.
>
> This code is built only when CONFIG_HYPERV is enabled.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

This looks like it should be a nested irqchip driver instead, so your
device drivers can use the normal request_irq() functions etc.

Is anything preventing you from doing that? If so, please describe
that in the changelog and in a comment in the driver.

     Arnd

Re: [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot

[Arnd Bergmann]

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>
> Add ARM64-specific code to initialize the Hyper-V
> hypervisor when booting as a guest VM. Provide functions
> and data structures indicating hypervisor status that
> are needed by VMbus driver.
>
> This code is built only when CONFIG_HYPERV is enabled.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  arch/arm64/hyperv/hv_core.c | 156 ++++++++++++++++++++++++++++++++++++++++++++

As you are effectively adding a new clocksource driver here, please move the
code to drivers/clocksource and send the patch to the respective maintainers
(added to Cc here), splitting it out from the rest of the patch.

You should also describe why your platform doesn't just use the normal
architected timer interface.

> +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);

This looks like it registers a driver for the same device as the normal
arch timer. Won't that clash?

     Arnd

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[gregkh]

On Mon, Mar 16, 2020 at 09:22:43AM +0100, Arnd Bergmann wrote:
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >  /*
> > + * Functions for allocating and freeing memory with size and
> > + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > + * the guest page size may not be the same as the Hyper-V page
> > + * size. We depend upon kmalloc() aligning power-of-two size
> > + * allocations to the allocation size boundary, so that the
> > + * allocated memory appears to Hyper-V as a page of the size
> > + * it expects.
> > + *
> > + * These functions are used by arm64 specific code as well as
> > + * arch independent Hyper-V drivers.
> > + */
> > +
> > +void *hv_alloc_hyperv_page(void)
> > +{
> > +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> > +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > +}
> > +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> 
> I don't think there is any guarantee that kmalloc() returns page-aligned
> allocations in general. How about using get_free_pages()
> to implement this?

Even if it was guaranteed, a pointless wrapper like this is not needed
or ok, and shouldn't be created, just use kmalloc.

thanks,

greg k-h

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Marc Zyngier]

On 2020-03-16 08:22, Arnd Bergmann wrote:
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> 
> wrote:
>>  /*
>> + * Functions for allocating and freeing memory with size and
>> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
>> + * the guest page size may not be the same as the Hyper-V page
>> + * size. We depend upon kmalloc() aligning power-of-two size
>> + * allocations to the allocation size boundary, so that the
>> + * allocated memory appears to Hyper-V as a page of the size
>> + * it expects.
>> + *
>> + * These functions are used by arm64 specific code as well as
>> + * arch independent Hyper-V drivers.
>> + */
>> +
>> +void *hv_alloc_hyperv_page(void)
>> +{
>> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
>> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
>> +}
>> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> 
> I don't think there is any guarantee that kmalloc() returns 
> page-aligned
> allocations in general.

I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b: 
guarantee
natural alignment for kmalloc(power-of-two)").

> How about using get_free_pages() to implement this?

This would certainly work, at the expense of a lot of wasted memory when
PAGE_SIZE isn't 4k.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Arnd Bergmann]

On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> On 2020-03-16 08:22, Arnd Bergmann wrote:
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > wrote:
> >>  /*
> >> + * Functions for allocating and freeing memory with size and
> >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> >> + * the guest page size may not be the same as the Hyper-V page
> >> + * size. We depend upon kmalloc() aligning power-of-two size
> >> + * allocations to the allocation size boundary, so that the
> >> + * allocated memory appears to Hyper-V as a page of the size
> >> + * it expects.
> >> + *
> >> + * These functions are used by arm64 specific code as well as
> >> + * arch independent Hyper-V drivers.
> >> + */
> >> +
> >> +void *hv_alloc_hyperv_page(void)
> >> +{
> >> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> >> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> >> +}
> >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> >
> > I don't think there is any guarantee that kmalloc() returns
> > page-aligned
> > allocations in general.
>
> I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> guarantee
> natural alignment for kmalloc(power-of-two)").
>
> > How about using get_free_pages() to implement this?
>
> This would certainly work, at the expense of a lot of wasted memory when
> PAGE_SIZE isn't 4k.

I'm sure this is the least of your problems when the guest runs with
a large base page size, you've already wasted most of your memory
otherwise then.

    Arnd

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

[Arnd Bergmann]

On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:

> +
> +/* 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;

Are you sure these need to be made byte-aligned according to the
specification? If the structure itself is aligned to 64 bit, better mark only
the individual fields that are misaligned as __packed.

If the structure is aligned to only 32-bit addresses instead of
64-bit, mark it as "__packed __aligned(4)" to let the compiler
generate better code for accessing it.

Also, in order to write portable code, it would be helpful to mark
all the fields as explicitly little-endian, and use __le32_to_cpu()
etc for accessing them.

> +/* Define synthetic interrupt controller message flags. */
> +union hv_message_flags {
> +       __u8 asu8;
> +       struct {
> +               __u8 msg_pending:1;
> +               __u8 reserved:7;
> +       } __packed;
> +};

For similar reasons, please avoid bit fields and just use a
bit mask on the first member of the union.

The __packed annotation here is not meaningful,
as the total size is already only a single byte.

> +/* Define port identifier type. */
> +union hv_port_id {
> +       __u32 asu32;
> +       struct {
> +               __u32 id:24;
> +               __u32 reserved:8;
> +       }  __packed u;
> +};

Here, the __packed annotation is inconsistent with the use
in the rest of the file: marking only one member of the union
as __packed means that the union itself is still expected to
be 4 byte aligned. I would expect that either all of these
structures have a sensible alignment, or they are all
completely unaligned.

> + * 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))

In general, we prefer inline functions over macros in header files.

> +#if IS_ENABLED(CONFIG_HYPERV)
> +#define hv_enable_stimer0_percpu_irq(irq)      enable_percpu_irq(irq, 0)
> +#define hv_disable_stimer0_percpu_irq(irq)     disable_percpu_irq(irq)
> +#endif

Should there be an #else definition here? It helps readability
to have the two versions (with and without hyperv support) close
together rather than in different files. If there is no other
definition, just drop the #if.

     Arnd

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

[Michael Kelley]

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

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

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> 
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> 
> > +
> > +/* 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;
> 
> Are you sure these need to be made byte-aligned according to the
> specification? If the structure itself is aligned to 64 bit, better mark only
> the individual fields that are misaligned as __packed.
> 
> If the structure is aligned to only 32-bit addresses instead of
> 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> generate better code for accessing it.

None of the fields are misaligned and it will always be aligned to 64-bit
addresses, so there should be no padding needed or added.  There was
a discussion of __packed and the Hyper-V data structures in general on
LKML here:  https://lkml.org/lkml/2018/11/30/848.  Adding __packed was
done as a preventative measure, not because anything was actually
broken.  Marking as __aligned(8) here would indicate the correct intent,
though the use of the structure always ensures 64-bit alignment.

> 
> Also, in order to write portable code, it would be helpful to mark
> all the fields as explicitly little-endian, and use __le32_to_cpu()
> etc for accessing them.

There's an opening comment in this file stating that all data
structures shared between Hyper-V and a guest VM are little
endian.  Is there some other marking to consider using?

We have definitely not allowed for the case of Hyper-V running on
a big endian architecture.  There are a *lot* of messages and data
structures passed between the guest and Hyper-V, and coding
to handle either endianness is a big project.  I'm doubtful
of the value until and unless we actually have a need for it.

> 
> > +/* Define synthetic interrupt controller message flags. */
> > +union hv_message_flags {
> > +       __u8 asu8;
> > +       struct {
> > +               __u8 msg_pending:1;
> > +               __u8 reserved:7;
> > +       } __packed;
> > +};
> 
> For similar reasons, please avoid bit fields and just use a
> bit mask on the first member of the union.

Unfortunately, changing to a bit mask ripples into
architecture independent code and into the x86
implementation.  I'd prefer not to drag that complexity
into this patch set.

> 
> The __packed annotation here is not meaningful,
> as the total size is already only a single byte.

Agreed.

> 
> > +/* Define port identifier type. */
> > +union hv_port_id {
> > +       __u32 asu32;
> > +       struct {
> > +               __u32 id:24;
> > +               __u32 reserved:8;
> > +       }  __packed u;
> > +};
> 
> Here, the __packed annotation is inconsistent with the use
> in the rest of the file: marking only one member of the union
> as __packed means that the union itself is still expected to
> be 4 byte aligned. I would expect that either all of these
> structures have a sensible alignment, or they are all
> completely unaligned.

Agreed.  Looks like it is wrong on the x86 side too.  

> 
> > + * 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))
> 
> In general, we prefer inline functions over macros in header files.

I can change the "set" calls to inline functions.  As you can see, the "get"
functions are coded and used in architecture independent code and on
the x86 side in a way that won't convert to inline functions.

> 
> > +#if IS_ENABLED(CONFIG_HYPERV)
> > +#define hv_enable_stimer0_percpu_irq(irq)      enable_percpu_irq(irq, 0)
> > +#define hv_disable_stimer0_percpu_irq(irq)     disable_percpu_irq(irq)
> > +#endif
> 
> Should there be an #else definition here? It helps readability
> to have the two versions (with and without hyperv support) close
> together rather than in different files. If there is no other
> definition, just drop the #if.

Agreed.  I'll figure out a way to handle this better.

> 
>      Arnd

RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> 
> On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > wrote:
> > >>  /*
> > >> + * Functions for allocating and freeing memory with size and
> > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > >> + * the guest page size may not be the same as the Hyper-V page
> > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > >> + * allocations to the allocation size boundary, so that the
> > >> + * allocated memory appears to Hyper-V as a page of the size
> > >> + * it expects.
> > >> + *
> > >> + * These functions are used by arm64 specific code as well as
> > >> + * arch independent Hyper-V drivers.
> > >> + */
> > >> +
> > >> +void *hv_alloc_hyperv_page(void)
> > >> +{
> > >> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> > >> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > >
> > > I don't think there is any guarantee that kmalloc() returns
> > > page-aligned
> > > allocations in general.
> >
> > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > guarantee
> > natural alignment for kmalloc(power-of-two)").
> >
> > > How about using get_free_pages() to implement this?
> >
> > This would certainly work, at the expense of a lot of wasted memory when
> > PAGE_SIZE isn't 4k.
> 
> I'm sure this is the least of your problems when the guest runs with
> a large base page size, you've already wasted most of your memory
> otherwise then.
> 

I think there's value in keeping these functions.  There are 8 uses in
architecture independent code at the moment, which admittedly saves
only ~1/2 Mbyte of memory with a 64K page size, but we will have
additional uses with more memory savings as we get all of the
Hyper-V synthetic drivers to work with 64K page size.  Furthermore,
there's coming work that will require additional steps to share a page
between a guest and the Hyper-V host.  These functions are the right
place to put the code for the additional sharing steps.  Removing them
now in favor of a bare kmalloc() and then adding them back doesn't
seem worthwhile.

Michael

RE: [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de>
> 
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add ARM64-specific code to set up and handle the interrupts
> > generated by Hyper-V for VMbus messages and for stimer expiration.
> >
> > This code is architecture dependent and is mostly driven by
> > architecture independent code in the VMbus driver and the
> > Hyper-V timer clocksource driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> 
> This looks like it should be a nested irqchip driver instead, so your
> device drivers can use the normal request_irq() functions etc.
> 
> Is anything preventing you from doing that? If so, please describe
> that in the changelog and in a comment in the driver.
> 

As mentioned in my reply on Patch 1, Hyper-V offers a limited synthetic
interrupt controller managed by Linux code that's been around the last
10 years on the x86 side.   For reasons that pre-date me, it was not written
as an irqchip driver.

Modulo the small routines you see in this patch, the code is architecture
independent, and it seems we ought to keep the high degree of commonality.
Re-architecting the arch independent code to model as an irqchip driver seems
to carry some risk to the x86 side that has a lot of real-world usage today, but
I'll take a look and see what the risks look like and if it adds any clarity.

RE: [PATCH v6 06/10] arm64: hyperv: Add kexec and panic handlers

[Michael Kelley]

From: Marc Zyngier <maz@kernel.org>  Sent: Sunday, March 15, 2020 11:16 AM
> 
> On Sat, 14 Mar 2020 15:35:15 +0000,
> Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add functions to set up and remove kexec and panic
> > handlers, and to inform Hyper-V about a guest panic.
> > These functions are called from architecture independent
> > code in the VMbus driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> >  arch/arm64/hyperv/hv_core.c  | 61
> ++++++++++++++++++++++++++++++++++++++++++++
> >  arch/arm64/hyperv/mshyperv.c | 26 +++++++++++++++++++
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
> > index 4aa6b8f..8d6de9f 100644
> > --- a/arch/arm64/hyperv/hv_core.c
> > +++ b/arch/arm64/hyperv/hv_core.c
> > @@ -199,3 +199,64 @@ void hv_get_vpreg_128(u32 msr, struct
> hv_get_vp_register_output *res)
> >  	kfree(output);
> >  }
> >  EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
> > +
> > +void hyperv_report_panic(struct pt_regs *regs, long err)
> > +{
> > +	static bool panic_reported;
> > +	u64 guest_id;
> > +
> > +	/*
> > +	 * We prefer to report panic on 'die' chain as we have proper
> > +	 * registers to report, but if we miss it (e.g. on BUG()) we need
> > +	 * to report it on 'panic'.
> > +	 */
> > +	if (panic_reported)
> > +		return;
> > +	panic_reported = true;
> 
> How does this work when multiple vcpus are crashing at once? Are you
> guaranteed to be single-threaded at this point?

I'll need to go research.  If not, the above should be an atomic
test-and-set.

> 
> > +
> > +	guest_id = hv_get_vpreg(HV_REGISTER_GUEST_OSID);
> > +
> > +	/*
> > +	 * Hyper-V provides the ability to store only 5 values.
> > +	 * Pick the passed in error value, the guest_id, and the PC.
> > +	 * The first two general registers are added arbitrarily.
> > +	 */
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P0, err);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P1, guest_id);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P2, regs->pc);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P3, regs->regs[0]);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P4, regs->regs[1]);
> 
> How about reporting useful information, a pointer to some data
> structure describing the fault? As it is, the usefulness of this is
> pretty dubious.

Yes, it is dubious.  The version with more data describing the fault is
hyperv_report_panic_msg() below, which is provided by newer versions
of Hyper-V, including all versions for ARM64.  So the above function
should never get called on ARM64.  While we could stub it out, I'd
like to keep the notification to Hyper-V just in case
hyperv_report_panic_msg() is not available for some reason I can't
currently anticipate.  The important thing is the notification, and
the register values aren't really important.  I'll add a comment to
that effect.

> 
> > +
> > +	/*
> > +	 * Let Hyper-V know there is crash data available
> > +	 */
> > +	hv_set_vpreg(HV_REGISTER_CRASH_CTL, HV_CRASH_CTL_CRASH_NOTIFY);
> > +}
> > +EXPORT_SYMBOL_GPL(hyperv_report_panic);
> > +
> > +/*
> > + * hyperv_report_panic_msg - report panic message to Hyper-V
> > + * @pa: physical address of the panic page containing the message
> > + * @size: size of the message in the page
> > + */
> > +void hyperv_report_panic_msg(phys_addr_t pa, size_t size)
> > +{
> > +	/*
> > +	 * P3 to contain the physical address of the panic page & P4 to
> > +	 * contain the size of the panic data in that page. Rest of the
> > +	 * registers are no-op when the NOTIFY_MSG flag is set.
> > +	 */
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P0, 0);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P1, 0);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P2, 0);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P3, pa);
> > +	hv_set_vpreg(HV_REGISTER_CRASH_P4, size);
> > +
> > +	/*
> > +	 * Let Hyper-V know there is crash data available along with
> > +	 * the panic message.
> > +	 */
> > +	hv_set_vpreg(HV_REGISTER_CRASH_CTL,
> > +	       (HV_CRASH_CTL_CRASH_NOTIFY | HV_CRASH_CTL_CRASH_NOTIFY_MSG));
> > +}
> > +EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
> > diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> > index ae6ece6..c58940d 100644
> > --- a/arch/arm64/hyperv/mshyperv.c
> > +++ b/arch/arm64/hyperv/mshyperv.c
> > @@ -23,6 +23,8 @@
> >
> >  static void (*vmbus_handler)(void);
> >  static void (*hv_stimer0_handler)(void);
> > +static void (*hv_kexec_handler)(void);
> > +static void (*hv_crash_handler)(struct pt_regs *regs);
> 
> Why is this in the arch-specific code? Yes, it lives in the x86 arch
> code too, but I don't see what prevents it from being moved to the
> vmbus_drv.c code.

OK -- I'll see about moving it to arch independent code.

> 
> >
> >  static int vmbus_irq;
> >  static long __percpu *vmbus_evt;
> > @@ -137,3 +139,27 @@ void hv_remove_stimer0_irq(int irq)
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
> > +
> > +void hv_setup_kexec_handler(void (*handler)(void))
> > +{
> > +	hv_kexec_handler = handler;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_setup_kexec_handler);
> > +
> > +void hv_remove_kexec_handler(void)
> > +{
> > +	hv_kexec_handler = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_remove_kexec_handler);
> > +
> > +void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs))
> > +{
> > +	hv_crash_handler = handler;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
> > +
> > +void hv_remove_crash_handler(void)
> > +{
> > +	hv_crash_handler = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
> > --
> > 1.8.3.1
> >
> >
> 
> Thanks,
> 
> 	M.
> 
> --
> Jazz is not dead, it just smells funny.

RE: [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:30 AM
> 
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add ARM64-specific code to initialize the Hyper-V
> > hypervisor when booting as a guest VM. Provide functions
> > and data structures indicating hypervisor status that
> > are needed by VMbus driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> >  arch/arm64/hyperv/hv_core.c | 156
> ++++++++++++++++++++++++++++++++++++++++++++
> 
> As you are effectively adding a new clocksource driver here, please move the
> code to drivers/clocksource and send the patch to the respective maintainers
> (added to Cc here), splitting it out from the rest of the patch.
> 
> You should also describe why your platform doesn't just use the normal
> architected timer interface.
> 
> > +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);
> 
> This looks like it registers a driver for the same device as the normal
> arch timer. Won't that clash?
> 
>      Arnd

There is a Hyper-V clocksource driver in drivers/clocksource/hyperv_timer.c.
It is architecture independent and works for both x86 and ARM64.

The requirement here is really for a place to hang the general Hyper-V
initialization code.   On the x86 side, there's infrastructure already in place
to do hypervisor initialization, but nothing corresponding on the ARM64 side.
The TIMER_ACPI_DECLARE hook is admittedly a temporary approach, and I'm
happy to hear if someone has a better way to handle this.

FWIW, Hyper-V doesn't currently virtualize the ARM arch counter/timer for
guest VMs.  The Hyper-V synthetic counter/timer in the Hyper-V clocksource
driver is used on both ARM64 and x86.  But this Hyper-V init code doesn't actually
touch the GTDT device, so it won't interfere with the ARM arch counter/timer
when a future Hyper-V version does virtualize it.

Michael

RE: [PATCH v6 09/10] arm64: efi: Export screen_info

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de>
> 
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > The Hyper-V frame buffer driver may be built as a module, and
> > it needs access to screen_info. So export screen_info.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> 
> Is there any chance of using a more modern KMS based driver for the screen
> than the old fbdev subsystem? I had hoped to one day completely remove
> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> architectures.
> 

The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
frame buffer device.  That driver builds and runs on both ARM64 and x86.

I'm not knowledgeable about video/graphics drivers, but when you
say "a more modern KMS based driver", are you meaning one based on
DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
Are there any drivers that would be a good pattern to look at?

Michael

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Arnd Bergmann]

On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > The Hyper-V frame buffer driver may be built as a module, and
> > > it needs access to screen_info. So export screen_info.
> > >
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >
> > Is there any chance of using a more modern KMS based driver for the screen
> > than the old fbdev subsystem? I had hoped to one day completely remove
> > support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> > architectures.
> >
>
> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> frame buffer device.  That driver builds and runs on both ARM64 and x86.
>
> I'm not knowledgeable about video/graphics drivers, but when you
> say "a more modern KMS based driver", are you meaning one based on
> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
> Are there any drivers that would be a good pattern to look at?

It used to be a lot harder to write a DRM driver compared to an fbdev
driver, but this has changed to the opposite over the years.

A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
or anything in drivers/gpu/drm/tiny/, but you may want to look at the
other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.

       Arnd

Re: [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot

[Arnd Bergmann]

On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:30 AM
> >
> > As you are effectively adding a new clocksource driver here, please move the
> > code to drivers/clocksource and send the patch to the respective maintainers
> > (added to Cc here), splitting it out from the rest of the patch.
> >
> > You should also describe why your platform doesn't just use the normal
> > architected timer interface.
> >
> > > +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);
> >
> > This looks like it registers a driver for the same device as the normal
> > arch timer. Won't that clash?
>
> There is a Hyper-V clocksource driver in drivers/clocksource/hyperv_timer.c.
> It is architecture independent and works for both x86 and ARM64.
>
> The requirement here is really for a place to hang the general Hyper-V
> initialization code.   On the x86 side, there's infrastructure already in place
> to do hypervisor initialization, but nothing corresponding on the ARM64 side.
> The TIMER_ACPI_DECLARE hook is admittedly a temporary approach, and I'm
> happy to hear if someone has a better way to handle this.
>
> FWIW, Hyper-V doesn't currently virtualize the ARM arch counter/timer for
> guest VMs.  The Hyper-V synthetic counter/timer in the Hyper-V clocksource
> driver is used on both ARM64 and x86.  But this Hyper-V init code doesn't actually
> touch the GTDT device, so it won't interfere with the ARM arch counter/timer
> when a future Hyper-V version does virtualize it.

I don't have a good idea to solve it, just a few more thoughts:

- if your platform does not actually provide the generic timer, then the
  ACPI tables should not list one either. Instead, create a separate
  description for your custom timer, and have that added to the ACPI
  spec.

- To treat the timer more like a normal driver, better have the
   TIMER_ACPI_DECLARE() function live only in the driver itself,
   and use an early initcall (arch_initcall, subsys_initcall, etc)
   it initialize the rest as late as you can.

- Some of the other code added to arch/arm64/ might be able to
  live in drivers/virt/hyperv in order to be shared between x86 and
  arm64. (No idea how much of it there is).

       Arnd

Re: [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer

[Arnd Bergmann]

On Wed, Mar 18, 2020 at 1:16 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > Add ARM64-specific code to set up and handle the interrupts
> > > generated by Hyper-V for VMbus messages and for stimer expiration.
> > >
> > > This code is architecture dependent and is mostly driven by
> > > architecture independent code in the VMbus driver and the
> > > Hyper-V timer clocksource driver.
> > >
> > > This code is built only when CONFIG_HYPERV is enabled.
> > >
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >
> > This looks like it should be a nested irqchip driver instead, so your
> > device drivers can use the normal request_irq() functions etc.
> >
> > Is anything preventing you from doing that? If so, please describe
> > that in the changelog and in a comment in the driver.
> >
>
> As mentioned in my reply on Patch 1, Hyper-V offers a limited synthetic
> interrupt controller managed by Linux code that's been around the last
> 10 years on the x86 side.   For reasons that pre-date me, it was not written
> as an irqchip driver.

I think the reason is just that 10 years ago, we did not have the concept
of irqchips as device drivers.

> Modulo the small routines you see in this patch, the code is architecture
> independent, and it seems we ought to keep the high degree of commonality.
> Re-architecting the arch independent code to model as an irqchip driver seems
> to carry some risk to the x86 side that has a lot of real-world usage today, but
> I'll take a look and see what the risks look like and if it adds any clarity.

How many drivers link against the custom interface? If it's less than 10,
making it a real driver is probably not too hard to do.

       Arnd

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Arnd Bergmann]

On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> >
> > On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > > wrote:
> > > >>  /*
> > > >> + * Functions for allocating and freeing memory with size and
> > > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > > >> + * the guest page size may not be the same as the Hyper-V page
> > > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > > >> + * allocations to the allocation size boundary, so that the
> > > >> + * allocated memory appears to Hyper-V as a page of the size
> > > >> + * it expects.
> > > >> + *
> > > >> + * These functions are used by arm64 specific code as well as
> > > >> + * arch independent Hyper-V drivers.
> > > >> + */
> > > >> +
> > > >> +void *hv_alloc_hyperv_page(void)
> > > >> +{
> > > >> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> > > >> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > > >> +}
> > > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > > >
> > > > I don't think there is any guarantee that kmalloc() returns
> > > > page-aligned
> > > > allocations in general.
> > >
> > > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > > guarantee
> > > natural alignment for kmalloc(power-of-two)").
> > >
> > > > How about using get_free_pages() to implement this?
> > >
> > > This would certainly work, at the expense of a lot of wasted memory when
> > > PAGE_SIZE isn't 4k.
> >
> > I'm sure this is the least of your problems when the guest runs with
> > a large base page size, you've already wasted most of your memory
> > otherwise then.
> >
>
> I think there's value in keeping these functions.  There are 8 uses in
> architecture independent code at the moment, which admittedly saves
> only ~1/2 Mbyte of memory with a 64K page size, but we will have
> additional uses with more memory savings as we get all of the
> Hyper-V synthetic drivers to work with 64K page size.  Furthermore,
> there's coming work that will require additional steps to share a page
> between a guest and the Hyper-V host.  These functions are the right
> place to put the code for the additional sharing steps.  Removing them
> now in favor of a bare kmalloc() and then adding them back doesn't
> seem worthwhile.

My point was to keep the functions but use alloc_pages() internally,
so you can deal with the hypervisor having a larger page size than
the guest, which seems to be a more important scenario if I correctly
understand the differences between the way Windows and Linux
deal with page cache.

As far as I understand, using 64kb pages on Windows is generally
a win as the VFS code already deals with units of that size, while
on Linux the 4kb page size is too deeply entrenched within the
file system code to mess with it: Whatever you gain in terms of
TLB pressure on workloads that cannot use huge pages is all lost
again through extra I/O and wasted physical memory.

        Arnd

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

[Arnd Bergmann]

On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > > +
> > > +/* 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;
> >
> > Are you sure these need to be made byte-aligned according to the
> > specification? If the structure itself is aligned to 64 bit, better mark only
> > the individual fields that are misaligned as __packed.
> >
> > If the structure is aligned to only 32-bit addresses instead of
> > 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> > generate better code for accessing it.
>
> None of the fields are misaligned and it will always be aligned to 64-bit
> addresses, so there should be no padding needed or added.  There was
> a discussion of __packed and the Hyper-V data structures in general on
> LKML here:  https://lkml.org/lkml/2018/11/30/848.  Adding __packed was
> done as a preventative measure, not because anything was actually
> broken.  Marking as __aligned(8) here would indicate the correct intent,
> though the use of the structure always ensures 64-bit alignment.

Just drop the __packed annotations then, they just confuse the compiler
in this case. In particular, when the compiler thinks that a structure is
misaligned, it tries to avoid using load/store instructions on it that are
inefficient or trap with misaligned code, so having default alignment
produces better object code.

> > Also, in order to write portable code, it would be helpful to mark
> > all the fields as explicitly little-endian, and use __le32_to_cpu()
> > etc for accessing them.
>
> There's an opening comment in this file stating that all data
> structures shared between Hyper-V and a guest VM are little
> endian.  Is there some other marking to consider using?

Yes, device drivers should generally define data structures using
the __le32, __le64 etc types, and use the conversion functions
to access them. Building with 'make C=1' usually tells you when
you have mismatched annotations.

> We have definitely not allowed for the case of Hyper-V running on
> a big endian architecture.  There are a *lot* of messages and data
> structures passed between the guest and Hyper-V, and coding
> to handle either endianness is a big project.  I'm doubtful
> of the value until and unless we actually have a need for it.

In general, the use of big-endian software on Linux is declining, however

- arm64 as an architecture is meant to support both endian types,
  and we still try to ensure it works either way as long as there are
  users that depend on it.

- The remaining users of big-endian software are probably
  more likely to run on virtual machines than on real hardware

- Any device driver should generally be written against portable
  interfaces, even if you think you know how it will be used. As
  driver writers tend to look at existing code for new drivers, it's
  better to have them all be portable. (This is a similar argument
  to the irqchip interface).

Even if you don't convert any of the existing architecture independent
code to run both ways, I see no reason to not do it for new drivers.

> > > +/* Define synthetic interrupt controller message flags. */
> > > +union hv_message_flags {
> > > +       __u8 asu8;
> > > +       struct {
> > > +               __u8 msg_pending:1;
> > > +               __u8 reserved:7;
> > > +       } __packed;
> > > +};
> >
> > For similar reasons, please avoid bit fields and just use a
> > bit mask on the first member of the union.
>
> Unfortunately, changing to a bit mask ripples into
> architecture independent code and into the x86
> implementation.  I'd prefer not to drag that complexity
> into this patch set.

How so? If this file is arm64 specific, there should be no need to make
x86 do the same change.

> > > + * 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))
> >
> > In general, we prefer inline functions over macros in header files.
>
> I can change the "set" calls to inline functions.  As you can see, the "get"
> functions are coded and used in architecture independent code and on
> the x86 side in a way that won't convert to inline functions.

Ok.

        Arnd

RE: [PATCH v6 03/10] arm64: hyperv: Add hypercall and register access functions

[Michael Kelley]

From: Hillf Danton <hdanton@sina.com> Sent: Tuesday, March 17, 2020 8:12 PM
> 
> On Sat, 14 Mar 2020 08:35:12 -0700 Michael Kelley wrote:
> > +/*
> > + * Get the value of a single VP register.  One version
> > + * returns just 64 bits and another returns the full 128 bits.
> > + * The two versions are separate to avoid complicating the
> > + * calling sequence for the more frequently used 64 bit version.
> > + */
> > +
> > +/*
> > + * Input and output memory allocation sizes are rounded up to a power
> > + * of 2 so kmalloc() will guarantee alignment. In turn, the alignment
> > + * ensures that the allocations don't cross a page boundary, which is
> 
> Better to specify kmalloc's current alignment and why it fails to ensure
> (4 * sizeof(u64))-sized allocations wont cross page boundary.
> 

Is your comment referring to ARCH_KMALLOC_MINALIGN?  If so, I see
how that makes sense.  BUILD_BUG_ON (sizeof (*input) >
ARCH_KMALLOC_MINALIGN) would be a cleaner solution. 

Thanks,

Michael

> > + * required by the hypercall interface.
> > + */
> > +#define INPUTSIZE (4 * sizeof(u64))
> > +#define OUTPUTSIZE (2 * sizeof(u64))
> > +
> > +static void __hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
> > +{
> > +	union hv_hypercall_status		status;
> > +	struct hv_get_vp_register_input		*input;
> > +
> > +	BUILD_BUG_ON(sizeof(*input) > INPUTSIZE);
> > +
> > +	input = kzalloc(INPUTSIZE, GFP_ATOMIC);
> > +
> > +	input->partitionid = HV_PARTITION_ID_SELF;
> > +	input->vpindex = HV_VP_INDEX_SELF;
> > +	input->inputvtl = 0;
> > +	input->name0 = msr;
> > +	input->name1 = 0;
> > +
> > +
> > +	status.as_uint64 = hv_do_hypercall(
> > +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COUNT_1,
> > +		input, res);
> > +
> > +	/*
> > +	 * Something is fundamentally broken in the hypervisor if
> > +	 * getting a VP register fails. There's really no way to
> > +	 * continue as a guest VM, so panic.
> > +	 */
> > +	BUG_ON(status.status != HV_STATUS_SUCCESS);
> > +
> > +	kfree(input);
> > +}

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

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
> 
> On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > > +
> > > > +/* 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;
> > >
> > > Are you sure these need to be made byte-aligned according to the
> > > specification? If the structure itself is aligned to 64 bit, better mark only
> > > the individual fields that are misaligned as __packed.
> > >
> > > If the structure is aligned to only 32-bit addresses instead of
> > > 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> > > generate better code for accessing it.
> >
> > None of the fields are misaligned and it will always be aligned to 64-bit
> > addresses, so there should be no padding needed or added.  There was
> > a discussion of __packed and the Hyper-V data structures in general on
> > LKML here:  https://lkml.org/lkml/2018/11/30/848  Adding __packed was
> > done as a preventative measure, not because anything was actually
> > broken.  Marking as __aligned(8) here would indicate the correct intent,
> > though the use of the structure always ensures 64-bit alignment.
> 
> Just drop the __packed annotations then, they just confuse the compiler
> in this case. In particular, when the compiler thinks that a structure is
> misaligned, it tries to avoid using load/store instructions on it that are
> inefficient or trap with misaligned code, so having default alignment
> produces better object code.

So I'm confused a bit.  Were the original concerns in the above LKML
discussion bogus?  Is it legal for the compiler to reorder fields or add
padding, even if the layout of fields in the structure doesn't require it?
If the compiler *could* do such, then it seems like keeping the __packed
would be appropriate per the LKML discussion.

> 
> > > Also, in order to write portable code, it would be helpful to mark
> > > all the fields as explicitly little-endian, and use __le32_to_cpu()
> > > etc for accessing them.
> >
> > There's an opening comment in this file stating that all data
> > structures shared between Hyper-V and a guest VM are little
> > endian.  Is there some other marking to consider using?
> 
> Yes, device drivers should generally define data structures using
> the __le32, __le64 etc types, and use the conversion functions
> to access them. Building with 'make C=1' usually tells you when
> you have mismatched annotations.
> 
> > We have definitely not allowed for the case of Hyper-V running on
> > a big endian architecture.  There are a *lot* of messages and data
> > structures passed between the guest and Hyper-V, and coding
> > to handle either endianness is a big project.  I'm doubtful
> > of the value until and unless we actually have a need for it.
> 
> In general, the use of big-endian software on Linux is declining, however
> 
> - arm64 as an architecture is meant to support both endian types,
>   and we still try to ensure it works either way as long as there are
>   users that depend on it.
> 
> - The remaining users of big-endian software are probably
>   more likely to run on virtual machines than on real hardware
> 
> - Any device driver should generally be written against portable
>   interfaces, even if you think you know how it will be used. As
>   driver writers tend to look at existing code for new drivers, it's
>   better to have them all be portable. (This is a similar argument
>   to the irqchip interface).
> 
> Even if you don't convert any of the existing architecture independent
> code to run both ways, I see no reason to not do it for new drivers.

OK, let me look into this.  Given how the major Linux distros on
ARM64 have all gone little-endian, I'm a bit skeptical of the value
for the big server environments in which Hyper-V would be used.

> 
> > > > +/* Define synthetic interrupt controller message flags. */
> > > > +union hv_message_flags {
> > > > +       __u8 asu8;
> > > > +       struct {
> > > > +               __u8 msg_pending:1;
> > > > +               __u8 reserved:7;
> > > > +       } __packed;
> > > > +};
> > >
> > > For similar reasons, please avoid bit fields and just use a
> > > bit mask on the first member of the union.
> >
> > Unfortunately, changing to a bit mask ripples into
> > architecture independent code and into the x86
> > implementation.  I'd prefer not to drag that complexity
> > into this patch set.
> 
> How so? If this file is arm64 specific, there should be no need to make
> x86 do the same change.

This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
ARM64 sides that are used by arch independent code, and this is one
of those definitions.  I had held off on breaking the file into arch
independent and arch specific portions because the Hyper-V team has
left some gray areas for functionality that isn't yet used on the ARM64
side.  So in some cases, it's hard to know what functionality to put
into the arch independent portion.

But I think I'll go ahead and make the separation with reasonably good
accuracy, and update the x86 side accordingly.  That will reduce the size
of this patch set to contain only the things that we know are ARM64
specific and which are actually used by the ARM64 code.  Things like the
hv_message_flags will go into the arch independent portion so that
they can be used by the arch independent code without cluttering up
the arch specific code.  Making the change will help reduce any
confusion about what is ARM64-specific. The other core #include file,
mshyperv.h, has already been done this way.

Michael

> 
> > > > + * 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))
> > >
> > > In general, we prefer inline functions over macros in header files.
> >
> > I can change the "set" calls to inline functions.  As you can see, the "get"
> > functions are coded and used in architecture independent code and on
> > the x86 side in a way that won't convert to inline functions.
> 
> Ok.
> 
>         Arnd

RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> 
> On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> > >
> > > On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > > > wrote:
> > > > >>  /*
> > > > >> + * Functions for allocating and freeing memory with size and
> > > > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > > > >> + * the guest page size may not be the same as the Hyper-V page
> > > > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > > > >> + * allocations to the allocation size boundary, so that the
> > > > >> + * allocated memory appears to Hyper-V as a page of the size
> > > > >> + * it expects.
> > > > >> + *
> > > > >> + * These functions are used by arm64 specific code as well as
> > > > >> + * arch independent Hyper-V drivers.
> > > > >> + */
> > > > >> +
> > > > >> +void *hv_alloc_hyperv_page(void)
> > > > >> +{
> > > > >> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> > > > >> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > > > >> +}
> > > > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > > > >
> > > > > I don't think there is any guarantee that kmalloc() returns
> > > > > page-aligned
> > > > > allocations in general.
> > > >
> > > > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > > > guarantee
> > > > natural alignment for kmalloc(power-of-two)").
> > > >
> > > > > How about using get_free_pages() to implement this?
> > > >
> > > > This would certainly work, at the expense of a lot of wasted memory when
> > > > PAGE_SIZE isn't 4k.
> > >
> > > I'm sure this is the least of your problems when the guest runs with
> > > a large base page size, you've already wasted most of your memory
> > > otherwise then.
> > >
> >
> > I think there's value in keeping these functions.  There are 8 uses in
> > architecture independent code at the moment, which admittedly saves
> > only ~1/2 Mbyte of memory with a 64K page size, but we will have
> > additional uses with more memory savings as we get all of the
> > Hyper-V synthetic drivers to work with 64K page size.  Furthermore,
> > there's coming work that will require additional steps to share a page
> > between a guest and the Hyper-V host.  These functions are the right
> > place to put the code for the additional sharing steps.  Removing them
> > now in favor of a bare kmalloc() and then adding them back doesn't
> > seem worthwhile.
> 
> My point was to keep the functions but use alloc_pages() internally,
> so you can deal with the hypervisor having a larger page size than
> the guest, which seems to be a more important scenario if I correctly
> understand the differences between the way Windows and Linux
> deal with page cache.

OK, I see now what you are getting at.  I should spell out my
assumption, which is the opposite.  Hyper-V won't have a page
size other than 4K anytime in the foreseeable future.  The
code is too wedded to the x86 4K page size, and the host-guest
interfaces have a lot of implicit assumptions that the page size is
4K (unfortunately).  But the last time I looked, RHEL for ARM64 is
delivered with a 64K page size.  So my assumption is that the only
combination that really matters is the guest page size being larger
than the Hyper-V page size.  The other way around just won't
happen without a major overhaul to Hyper-V, including a rework
of the guest-host interface.

Michael

> 
> As far as I understand, using 64kb pages on Windows is generally
> a win as the VFS code already deals with units of that size, while
> on Linux the 4kb page size is too deeply entrenched within the
> file system code to mess with it: Whatever you gain in terms of
> TLB pressure on workloads that cannot use huge pages is all lost
> again through extra I/O and wasted physical memory.
> 
>         Arnd

RE: [PATCH v6 09/10] arm64: efi: Export screen_info

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
> 
> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > > >
> > > > The Hyper-V frame buffer driver may be built as a module, and
> > > > it needs access to screen_info. So export screen_info.
> > > >
> > > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > >
> > > Is there any chance of using a more modern KMS based driver for the screen
> > > than the old fbdev subsystem? I had hoped to one day completely remove
> > > support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> > > architectures.
> > >
> >
> > The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> > frame buffer device.  That driver builds and runs on both ARM64 and x86.
> >
> > I'm not knowledgeable about video/graphics drivers, but when you
> > say "a more modern KMS based driver", are you meaning one based on
> > DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
> > Are there any drivers that would be a good pattern to look at?
> 
> It used to be a lot harder to write a DRM driver compared to an fbdev
> driver, but this has changed to the opposite over the years.
> 
> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
> 

Thanks for the pointers, especially for the other hypervisors.

Michael

Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Arnd Bergmann]

On Thu, Mar 19, 2020 at 10:43 PM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> > On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > My point was to keep the functions but use alloc_pages() internally,
> > so you can deal with the hypervisor having a larger page size than
> > the guest, which seems to be a more important scenario if I correctly
> > understand the differences between the way Windows and Linux
> > deal with page cache.
>
> OK, I see now what you are getting at.  I should spell out my
> assumption, which is the opposite.  Hyper-V won't have a page
> size other than 4K anytime in the foreseeable future.  The
> code is too wedded to the x86 4K page size, and the host-guest
> interfaces have a lot of implicit assumptions that the page size is
> 4K (unfortunately).  But the last time I looked, RHEL for ARM64 is
> delivered with a 64K page size.  So my assumption is that the only
> combination that really matters is the guest page size being larger
> than the Hyper-V page size.  The other way around just won't
> happen without a major overhaul to Hyper-V, including a rework
> of the guest-host interface.

Ok, got it. We should really ask Red Hat to change the page size,
but as long as you care existing systems, and you expect this to
result in gigabytes of allocation on future systems, having the
wrapper seems reasonable.

Maybe you could fall back to alloc_page when PAGE_SIZE equals
HV_HYP_PAGE_SIZE though? I'm not sure what the tradeoff
between kmalloc and alloc_page is these days, other than the
feeling that alloc_page is the more obvious choice when you know
you always want exactly a page here.

      Arnd

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

[Arnd Bergmann]

On Thu, Mar 19, 2020 at 10:31 PM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
> > Just drop the __packed annotations then, they just confuse the compiler
> > in this case. In particular, when the compiler thinks that a structure is
> > misaligned, it tries to avoid using load/store instructions on it that are
> > inefficient or trap with misaligned code, so having default alignment
> > produces better object code.
>
> So I'm confused a bit.  Were the original concerns in the above LKML
> discussion bogus?  Is it legal for the compiler to reorder fields or add
> padding, even if the layout of fields in the structure doesn't require it?
> If the compiler *could* do such, then it seems like keeping the __packed
> would be appropriate per the LKML discussion.

The padding is defined in the ELF psABI document for a particular
architecture. In theory an architecture might require padding around
smaller members, but they generally don't when you look at the ones
that Linux runs on. The few odd ones are those that require less
padding, only aligning members to 16 or 32 bit rather than natural
alignment, or padding the size of the structure to 32 bit even if it
only contains 8-bit or 16-bit members. When you have structures
in which every member is naturally aligned and the size it a multiple
of 32 bit, better leave out the __packed.

Aside from generating sub-optimal code, the __packed annotation
can also lead to undefined behavior, if you pass a pointer to
an unaligned member into a function call that takes an aligned
pointer. Newer compilers warn about this.

> > > Unfortunately, changing to a bit mask ripples into
> > > architecture independent code and into the x86
> > > implementation.  I'd prefer not to drag that complexity
> > > into this patch set.
> >
> > How so? If this file is arm64 specific, there should be no need to make
> > x86 do the same change.
>
> This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
> ARM64 sides that are used by arch independent code, and this is one
> of those definitions.  I had held off on breaking the file into arch
> independent and arch specific portions because the Hyper-V team has
> left some gray areas for functionality that isn't yet used on the ARM64
> side.  So in some cases, it's hard to know what functionality to put
> into the arch independent portion.
>
> But I think I'll go ahead and make the separation with reasonably good
> accuracy, and update the x86 side accordingly.  That will reduce the size
> of this patch set to contain only the things that we know are ARM64
> specific and which are actually used by the ARM64 code.  Things like the
> hv_message_flags will go into the arch independent portion so that
> they can be used by the arch independent code without cluttering up
> the arch specific code.  Making the change will help reduce any
> confusion about what is ARM64-specific. The other core #include file,
> mshyperv.h, has already been done this way.

Ok, sounds good.

     Arnd

RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Michael Kelley]

From: Arnd Bergmann <arnd@arndb.de> Sent: Friday, March 20, 2020 9:28 AM
> 
> On Thu, Mar 19, 2020 at 10:43 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> > > On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > > My point was to keep the functions but use alloc_pages() internally,
> > > so you can deal with the hypervisor having a larger page size than
> > > the guest, which seems to be a more important scenario if I correctly
> > > understand the differences between the way Windows and Linux
> > > deal with page cache.
> >
> > OK, I see now what you are getting at.  I should spell out my
> > assumption, which is the opposite.  Hyper-V won't have a page
> > size other than 4K anytime in the foreseeable future.  The
> > code is too wedded to the x86 4K page size, and the host-guest
> > interfaces have a lot of implicit assumptions that the page size is
> > 4K (unfortunately).  But the last time I looked, RHEL for ARM64 is
> > delivered with a 64K page size.  So my assumption is that the only
> > combination that really matters is the guest page size being larger
> > than the Hyper-V page size.  The other way around just won't
> > happen without a major overhaul to Hyper-V, including a rework
> > of the guest-host interface.
> 
> Ok, got it. We should really ask Red Hat to change the page size,
> but as long as you care existing systems, and you expect this to
> result in gigabytes of allocation on future systems, having the
> wrapper seems reasonable.
> 
> Maybe you could fall back to alloc_page when PAGE_SIZE equals
> HV_HYP_PAGE_SIZE though? I'm not sure what the tradeoff
> between kmalloc and alloc_page is these days, other than the
> feeling that alloc_page is the more obvious choice when you know
> you always want exactly a page here.
> 

Yes, that works for me.

Michael

Re: RE: [PATCH v6 09/10] arm64: efi: Export screen_info

[Nikhil Mahale]

On 3/20/20 3:16 AM, Michael Kelley wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
>>
>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>
>>>>> The Hyper-V frame buffer driver may be built as a module, and
>>>>> it needs access to screen_info. So export screen_info.
>>>>>
>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>>>>
>>>> Is there any chance of using a more modern KMS based driver for the screen
>>>> than the old fbdev subsystem? I had hoped to one day completely remove
>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
>>>> architectures.
>>>>
>>>
>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
>>> frame buffer device.  That driver builds and runs on both ARM64 and x86.
>>>
>>> I'm not knowledgeable about video/graphics drivers, but when you
>>> say "a more modern KMS based driver", are you meaning one based on
>>> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
>>> Are there any drivers that would be a good pattern to look at?
>>
>> It used to be a lot harder to write a DRM driver compared to an fbdev
>> driver, but this has changed to the opposite over the years.
>>
>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
>>
> 
> Thanks for the pointers, especially for the other hypervisors.
> 
Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
I had configured by email client incorrectly to reply.

screen_info is still useful with a modern KMS-based driver.  It exposes
the mode parameters that the GOP driver chose.  This information is
needed to implement seamless or glitchless boot, by both ensuring that
the scanout parameters don't change and being able to read back the
scanout image to populate the initial contents of the new surface.

This works today on arches which implement (U)EFI and export
screen_info, including x86 and powerpc, but doesn't work on arm or
arm64.  As arm64 systems that implement UEFI with real GOP drivers
become more prevalent, it would be nice to be have these features there
as well.

Thanks,
Nikhil Mahale

> Michael
> 

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Nikhil Mahale]

On 5/13/20 7:56 PM, Nikhil Mahale wrote:
> On 3/20/20 3:16 AM, Michael Kelley wrote:
>> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
>>>
>>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>>
>>>>>> The Hyper-V frame buffer driver may be built as a module, and
>>>>>> it needs access to screen_info. So export screen_info.
>>>>>>
>>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>>>>>
>>>>> Is there any chance of using a more modern KMS based driver for the screen
>>>>> than the old fbdev subsystem? I had hoped to one day completely remove
>>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
>>>>> architectures.
>>>>>
>>>>
>>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
>>>> frame buffer device.  That driver builds and runs on both ARM64 and x86.
>>>>
>>>> I'm not knowledgeable about video/graphics drivers, but when you
>>>> say "a more modern KMS based driver", are you meaning one based on
>>>> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
>>>> Are there any drivers that would be a good pattern to look at?
>>>
>>> It used to be a lot harder to write a DRM driver compared to an fbdev
>>> driver, but this has changed to the opposite over the years.
>>>
>>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
>>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
>>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
>>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
>>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
>>>
>>
>> Thanks for the pointers, especially for the other hypervisors.
>>
> Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
> I had configured by email client incorrectly to reply.
> 
> screen_info is still useful with a modern KMS-based driver.  It exposes
> the mode parameters that the GOP driver chose.  This information is
> needed to implement seamless or glitchless boot, by both ensuring that
> the scanout parameters don't change and being able to read back the
> scanout image to populate the initial contents of the new surface.
> 
> This works today on arches which implement (U)EFI and export
> screen_info, including x86 and powerpc, but doesn't work on arm or
> arm64.  As arm64 systems that implement UEFI with real GOP drivers
> become more prevalent, it would be nice to be have these features there
> as well.

In addition to this, even if a driver doesn't implement a framebuffer
console, or if it does but has an option to disable it, the driver still
needs to know whether the EFI console is using resources on the GPU so
it can avoid clobbering them. For example screen_info provides information
like offset and size of EFI console, using this information driver can
reserve memory used by console and prevent corruption on it.

I think arm64 should export screen_info.

> Thanks,
> Nikhil Mahale
> 
>> Michael
>>

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Ard Biesheuvel]

On Mon, 18 May 2020 at 06:25, Nikhil Mahale <nmahale@nvidia.com> wrote:
>
> On 5/13/20 7:56 PM, Nikhil Mahale wrote:
> > On 3/20/20 3:16 AM, Michael Kelley wrote:
> >> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
> >>>
> >>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>> From: Arnd Bergmann <arnd@arndb.de>
> >>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>>>>
> >>>>>> The Hyper-V frame buffer driver may be built as a module, and
> >>>>>> it needs access to screen_info. So export screen_info.
> >>>>>>
> >>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >>>>>
> >>>>> Is there any chance of using a more modern KMS based driver for the screen
> >>>>> than the old fbdev subsystem? I had hoped to one day completely remove
> >>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> >>>>> architectures.
> >>>>>
> >>>>
> >>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> >>>> frame buffer device.  That driver builds and runs on both ARM64 and x86.
> >>>>
> >>>> I'm not knowledgeable about video/graphics drivers, but when you
> >>>> say "a more modern KMS based driver", are you meaning one based on
> >>>> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
> >>>> Are there any drivers that would be a good pattern to look at?
> >>>
> >>> It used to be a lot harder to write a DRM driver compared to an fbdev
> >>> driver, but this has changed to the opposite over the years.
> >>>
> >>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> >>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> >>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> >>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> >>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
> >>>
> >>
> >> Thanks for the pointers, especially for the other hypervisors.
> >>
> > Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
> > I had configured by email client incorrectly to reply.
> >
> > screen_info is still useful with a modern KMS-based driver.  It exposes
> > the mode parameters that the GOP driver chose.  This information is
> > needed to implement seamless or glitchless boot, by both ensuring that
> > the scanout parameters don't change and being able to read back the
> > scanout image to populate the initial contents of the new surface.
> >
> > This works today on arches which implement (U)EFI and export
> > screen_info, including x86 and powerpc, but doesn't work on arm or
> > arm64.  As arm64 systems that implement UEFI with real GOP drivers
> > become more prevalent, it would be nice to be have these features there
> > as well.
>
> In addition to this, even if a driver doesn't implement a framebuffer
> console, or if it does but has an option to disable it, the driver still
> needs to know whether the EFI console is using resources on the GPU so
> it can avoid clobbering them. For example screen_info provides information
> like offset and size of EFI console, using this information driver can
> reserve memory used by console and prevent corruption on it.
>
> I think arm64 should export screen_info.
>

If there are reasons why KMS or fbdev drivers may need to access the
information in screen_info, it should be exported. I don't think that
is under debate here.

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Nikhil Mahale]

On 5/18/20 6:21 PM, Ard Biesheuvel wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Mon, 18 May 2020 at 06:25, Nikhil Mahale <nmahale@nvidia.com> wrote:
>>
>> On 5/13/20 7:56 PM, Nikhil Mahale wrote:
>>> On 3/20/20 3:16 AM, Michael Kelley wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
>>>>>
>>>>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>>>>>>>>
>>>>>>>> The Hyper-V frame buffer driver may be built as a module, and
>>>>>>>> it needs access to screen_info. So export screen_info.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>>>>>>>
>>>>>>> Is there any chance of using a more modern KMS based driver for the screen
>>>>>>> than the old fbdev subsystem? I had hoped to one day completely remove
>>>>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
>>>>>>> architectures.
>>>>>>>
>>>>>>
>>>>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
>>>>>> frame buffer device.  That driver builds and runs on both ARM64 and x86.
>>>>>>
>>>>>> I'm not knowledgeable about video/graphics drivers, but when you
>>>>>> say "a more modern KMS based driver", are you meaning one based on
>>>>>> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
>>>>>> Are there any drivers that would be a good pattern to look at?
>>>>>
>>>>> It used to be a lot harder to write a DRM driver compared to an fbdev
>>>>> driver, but this has changed to the opposite over the years.
>>>>>
>>>>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
>>>>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
>>>>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
>>>>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
>>>>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
>>>>>
>>>>
>>>> Thanks for the pointers, especially for the other hypervisors.
>>>>
>>> Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
>>> I had configured by email client incorrectly to reply.
>>>
>>> screen_info is still useful with a modern KMS-based driver.  It exposes
>>> the mode parameters that the GOP driver chose.  This information is
>>> needed to implement seamless or glitchless boot, by both ensuring that
>>> the scanout parameters don't change and being able to read back the
>>> scanout image to populate the initial contents of the new surface.
>>>
>>> This works today on arches which implement (U)EFI and export
>>> screen_info, including x86 and powerpc, but doesn't work on arm or
>>> arm64.  As arm64 systems that implement UEFI with real GOP drivers
>>> become more prevalent, it would be nice to be have these features there
>>> as well.
>>
>> In addition to this, even if a driver doesn't implement a framebuffer
>> console, or if it does but has an option to disable it, the driver still
>> needs to know whether the EFI console is using resources on the GPU so
>> it can avoid clobbering them. For example screen_info provides information
>> like offset and size of EFI console, using this information driver can
>> reserve memory used by console and prevent corruption on it.
>>
>> I think arm64 should export screen_info.
>>
> 
> If there are reasons why KMS or fbdev drivers may need to access the
> information in screen_info, it should be exported. I don't think that
> is under debate here.
> 

Hi Ard, thanks for your feedback. If my understanding is correct,
you are agree to export screen_info. Can you provide guidance on how can
we proceed here? are you willing to accept this current patch as-is or
would you like me to re-submit the patch with the additional rationale
provided?

Thanks,
Nikhil Mahale

Re: [PATCH v6 09/10] arm64: efi: Export screen_info

[Ard Biesheuvel]

On Fri, 22 May 2020 at 13:15, Nikhil Mahale <nmahale@nvidia.com> wrote:
>
> On 5/18/20 6:21 PM, Ard Biesheuvel wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, 18 May 2020 at 06:25, Nikhil Mahale <nmahale@nvidia.com> wrote:
> >>
> >> On 5/13/20 7:56 PM, Nikhil Mahale wrote:
> >>> On 3/20/20 3:16 AM, Michael Kelley wrote:
> >>>> From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
> >>>>>
> >>>>> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>>>> From: Arnd Bergmann <arnd@arndb.de>
> >>>>>>> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >>>>>>>>
> >>>>>>>> The Hyper-V frame buffer driver may be built as a module, and
> >>>>>>>> it needs access to screen_info. So export screen_info.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >>>>>>>
> >>>>>>> Is there any chance of using a more modern KMS based driver for the screen
> >>>>>>> than the old fbdev subsystem? I had hoped to one day completely remove
> >>>>>>> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> >>>>>>> architectures.
> >>>>>>>
> >>>>>>
> >>>>>> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> >>>>>> frame buffer device.  That driver builds and runs on both ARM64 and x86.
> >>>>>>
> >>>>>> I'm not knowledgeable about video/graphics drivers, but when you
> >>>>>> say "a more modern KMS based driver", are you meaning one based on
> >>>>>> DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
> >>>>>> Are there any drivers that would be a good pattern to look at?
> >>>>>
> >>>>> It used to be a lot harder to write a DRM driver compared to an fbdev
> >>>>> driver, but this has changed to the opposite over the years.
> >>>>>
> >>>>> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> >>>>> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> >>>>> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> >>>>> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> >>>>> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
> >>>>>
> >>>>
> >>>> Thanks for the pointers, especially for the other hypervisors.
> >>>>
> >>> Sorry if anybody in 'to' or 'cc' is receiving this reply multiple times.
> >>> I had configured by email client incorrectly to reply.
> >>>
> >>> screen_info is still useful with a modern KMS-based driver.  It exposes
> >>> the mode parameters that the GOP driver chose.  This information is
> >>> needed to implement seamless or glitchless boot, by both ensuring that
> >>> the scanout parameters don't change and being able to read back the
> >>> scanout image to populate the initial contents of the new surface.
> >>>
> >>> This works today on arches which implement (U)EFI and export
> >>> screen_info, including x86 and powerpc, but doesn't work on arm or
> >>> arm64.  As arm64 systems that implement UEFI with real GOP drivers
> >>> become more prevalent, it would be nice to be have these features there
> >>> as well.
> >>
> >> In addition to this, even if a driver doesn't implement a framebuffer
> >> console, or if it does but has an option to disable it, the driver still
> >> needs to know whether the EFI console is using resources on the GPU so
> >> it can avoid clobbering them. For example screen_info provides information
> >> like offset and size of EFI console, using this information driver can
> >> reserve memory used by console and prevent corruption on it.
> >>
> >> I think arm64 should export screen_info.
> >>
> >
> > If there are reasons why KMS or fbdev drivers may need to access the
> > information in screen_info, it should be exported. I don't think that
> > is under debate here.
> >
>
> Hi Ard, thanks for your feedback. If my understanding is correct,
> you are agree to export screen_info. Can you provide guidance on how can
> we proceed here? are you willing to accept this current patch as-is or
> would you like me to re-submit the patch with the additional rationale
> provided?
>

Please (re-)submit it along with the code that actually makes use of it.