[PATCH v4 0/5] hyperv: Fixes for get_vtl(), hv_vtl_apicid_to_vp_id()

[PATCH v4 0/5] hyperv: Fixes for get_vtl(), hv_vtl_apicid_to_vp_id()

[Roman Kisel]

The get_vtl(void) function

* has got one bug when the code started using a wrong pointer type after
  refactoring, and also
* the both function in question don't adhere to the requirements of
  the Hypervisor Top-Level Funactional Specification[1, 2] as the code overlaps
  the input and output areas for a hypercall.

The first issue leads to a wrong 100% reproducible computation due to reading
a byte worth of data at a wrong offset. That in turn leads to using a nonsensical
value ("fortunately", could catch it easily!) for the current VTL when initiating
VMBus communications. As a repercussion from that, the system wouldn't boot. The
fix is straightforward: use the correct pointer type.

The second issue doesn't seem to lead to any reproducible breakage just yet. It is
fixed with using the output hypercall pages allocated per-CPU, and that isn't the
only or the most obvious choice so let me elaborate why that fix appears to be the
best one in my opinion out of the options I could conceive of.

The approach chosen for fixing the second issue makes two things shine through:

* these functions just get a vCPU register, no special treatment needs to be
  involved,
* VTLs and dom0 can and should share code as both exist to provide services to
  a guest(s), be that from within the partition or from outside of it.

The projected benefits include replacing the functions in question with a future
`hv_get_vp_registers` one shared between dom0 and VTLs to allow for a better test
coverage.

I have validated the fixes by booting the fixed kernel in VTL2 up using OpenVMM and
OpenHCL[3, 4].

[1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
[2] https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/main/tlfs
[3] https://openvmm.dev/guide/user_guide/openhcl.html
[4] https://github.com/microsoft/OpenVMM

[v4]
  - Wrapped DECLARE_FLEX_ARRAY into a struct and added one more
    member as the documentation requires,
  - Removed superfluous type coercion,
  - Fixed tags,
  - Rebased onto the latest hyperv-next branch.

[v3]: https://lore.kernel.org/lkml/20241226213110.899497-1-romank@linux.microsoft.com/
  - Added a fix for hv_vtl_apicid_to_vp_id(),
  - Split out the patch for enabling the hypercall output page,
  - Updated the title of the patch series,

[v2]: https://lore.kernel.org/lkml/20241226203050.800524-1-romank@linux.microsoft.com/
  - Used the suggestions to define an additional structure to improve code readability,
  - Split out the patch with that definition.

[v1]: https://lore.kernel.org/lkml/20241218205421.319969-1-romank@linux.microsoft.com/

Roman Kisel (5):
  hyperv: Define struct hv_output_get_vp_registers
  hyperv: Fix pointer type in get_vtl(void)
  hyperv: Enable the hypercall output page for the VTL mode
  hyperv: Do not overlap the hvcall IO areas in get_vtl()
  hyperv: Do not overlap the hvcall IO areas in hv_vtl_apicid_to_vp_id()

 arch/x86/hyperv/hv_init.c   |  6 ++--
 arch/x86/hyperv/hv_vtl.c    |  2 +-
 drivers/hv/hv_common.c      |  6 ++--
 include/hyperv/hvgdk_mini.h | 65 +++++++++++++++++++++++++++++++++++--
 4 files changed, 70 insertions(+), 9 deletions(-)


base-commit: 26e1b813fcd02984b1cac5f3decdf4b0bb56fe02
-- 
2.34.1

[PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers

[Roman Kisel]

There is no definition of the output structure for the
GetVpRegisters hypercall. Hence, using the hypercall
is not possible when the output value has some structure
to it. Even getting a datum of a primitive type reads
as ad-hoc without that definition.

Define struct hv_output_get_vp_registers to enable using
the GetVpRegisters hypercall. Make provisions for all
supported architectures. No functional changes.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 include/hyperv/hvgdk_mini.h | 65 +++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index db3d1aaf7330..c2f5cc231dce 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h
@@ -781,6 +781,8 @@ struct hv_timer_message_payload {
 	__u64 delivery_time;	/* When the message was delivered */
 } __packed;
 
+#if defined(CONFIG_X86)
+
 struct hv_x64_segment_register {
 	u64 base;
 	u32 limit;
@@ -807,6 +809,8 @@ struct hv_x64_table_register {
 	u64 base;
 } __packed;
 
+#endif
+
 union hv_input_vtl {
 	u8 as_uint8;
 	struct {
@@ -1068,6 +1072,41 @@ union hv_dispatch_suspend_register {
 	} __packed;
 };
 
+#if defined(CONFIG_ARM64)
+
+union hv_arm64_pending_interruption_register {
+	u64 as_uint64;
+	struct {
+		u64 interruption_pending : 1;
+		u64 interruption_type : 1;
+		u64 reserved : 30;
+		u32 error_code;
+	} __packed;
+};
+
+union hv_arm64_interrupt_state_register {
+	u64 as_uint64;
+	struct {
+		u64 interrupt_shadow : 1;
+		u64 reserved : 63;
+	} __packed;
+};
+
+union hv_arm64_pending_synthetic_exception_event {
+	u64 as_uint64[2];
+	struct {
+		u32 event_pending : 1;
+		u32 event_type : 3;
+		u32 reserved : 4;
+		u32 exception_type;
+		u64 context;
+	} __packed;
+};
+
+#endif
+
+#if defined(CONFIG_X86)
+
 union hv_x64_interrupt_state_register {
 	u64 as_uint64;
 	struct {
@@ -1091,6 +1130,8 @@ union hv_x64_pending_interruption_register {
 	} __packed;
 };
 
+#endif
+
 union hv_register_value {
 	struct hv_u128 reg128;
 	u64 reg64;
@@ -1098,13 +1139,33 @@ union hv_register_value {
 	u16 reg16;
 	u8 reg8;
 
-	struct hv_x64_segment_register segment;
-	struct hv_x64_table_register table;
 	union hv_explicit_suspend_register explicit_suspend;
 	union hv_intercept_suspend_register intercept_suspend;
 	union hv_dispatch_suspend_register dispatch_suspend;
+#if defined(CONFIG_X86)
+	struct hv_x64_segment_register segment;
+	struct hv_x64_table_register table;
 	union hv_x64_interrupt_state_register interrupt_state;
 	union hv_x64_pending_interruption_register pending_interruption;
+#endif
+#if defined(CONFIG_ARM64)
+	union hv_arm64_pending_interruption_register pending_interruption;
+	union hv_arm64_interrupt_state_register interrupt_state;
+	union hv_arm64_pending_synthetic_exception_event pending_synthetic_exception_event;
+#endif
+};
+
+/*
+ * NOTE: Linux helper struct - NOT from Hyper-V code.
+ * DECLARE_FLEX_ARRAY() needs to be wrapped into
+ * a structure and have at least one more member besides
+ * DECLARE_FLEX_ARRAY.
+ */
+struct hv_output_get_vp_registers {
+	struct {
+		DECLARE_FLEX_ARRAY(union hv_register_value, values);
+		struct {} values_end;
+	};
 };
 
 #if defined(CONFIG_ARM64)
-- 
2.34.1

[PATCH v4 2/5] hyperv: Fix pointer type in get_vtl(void)

[Roman Kisel]

Commit bc905fa8b633 ("hyperv: Switch from hyperv-tlfs.h to hyperv/hvhdk.h")
changed the type of the output pointer to `struct hv_register_assoc` from
`struct hv_get_vp_registers_output`. That leads to an incorrect computation,
and leaves the system broken.

Use the correct pointer type for the output of the GetVpRegisters hypercall.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 9e5e8328df6b..f82d1aefaa8a 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -416,13 +416,13 @@ static u8 __init get_vtl(void)
 {
 	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
 	struct hv_input_get_vp_registers *input;
-	struct hv_register_assoc *output;
+	struct hv_output_get_vp_registers *output;
 	unsigned long flags;
 	u64 ret;
 
 	local_irq_save(flags);
 	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-	output = (struct hv_register_assoc *)input;
+	output = (struct hv_output_get_vp_registers *)input;
 
 	memset(input, 0, struct_size(input, names, 1));
 	input->partition_id = HV_PARTITION_ID_SELF;
@@ -432,7 +432,7 @@ static u8 __init get_vtl(void)
 
 	ret = hv_do_hypercall(control, input, output);
 	if (hv_result_success(ret)) {
-		ret = output->value.reg8 & HV_X64_VTL_MASK;
+		ret = output->values[0].reg8 & HV_X64_VTL_MASK;
 	} else {
 		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
 		BUG();
-- 
2.34.1

[PATCH v4 3/5] hyperv: Enable the hypercall output page for the VTL mode

[Roman Kisel]

Due to the hypercall page not being allocated in the VTL mode,
the code resorts to using a part of the input page.

Allocate the hypercall output page in the VTL mode thus enabling
it to use it for output and share code with dom0.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 drivers/hv/hv_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index c6ed3ba4bf61..c983cfd4d6c0 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -340,7 +340,7 @@ int __init hv_common_init(void)
 	BUG_ON(!hyperv_pcpu_input_arg);
 
 	/* Allocate the per-CPU state for output arg for root */
-	if (hv_root_partition) {
+	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
 		hyperv_pcpu_output_arg = alloc_percpu(void *);
 		BUG_ON(!hyperv_pcpu_output_arg);
 	}
@@ -435,7 +435,7 @@ int hv_common_cpu_init(unsigned int cpu)
 	void **inputarg, **outputarg;
 	u64 msr_vp_index;
 	gfp_t flags;
-	int pgcount = hv_root_partition ? 2 : 1;
+	const int pgcount = (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) ? 2 : 1;
 	void *mem;
 	int ret;
 
@@ -453,7 +453,7 @@ int hv_common_cpu_init(unsigned int cpu)
 		if (!mem)
 			return -ENOMEM;
 
-		if (hv_root_partition) {
+		if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
 			outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
 			*outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
 		}
-- 
2.34.1

[PATCH v4 4/5] hyperv: Do not overlap the hvcall IO areas in get_vtl()

[Roman Kisel]

The Top-Level Functional Specification for Hyper-V, Section 3.6 [1, 2],
disallows overlapping of the input and output hypercall areas, and
get_vtl(void) does overlap them.

Use the output hypercall page of the current vCPU for the hypercall.

[1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
[2] https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/main/tlfs

Fixes: 8387ce06d70b ("x86/hyperv: Set Virtual Trust Level in VMBus init message")
Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index f82d1aefaa8a..173005e6a95d 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -422,7 +422,7 @@ static u8 __init get_vtl(void)
 
 	local_irq_save(flags);
 	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
-	output = (struct hv_output_get_vp_registers *)input;
+	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
 
 	memset(input, 0, struct_size(input, names, 1));
 	input->partition_id = HV_PARTITION_ID_SELF;
-- 
2.34.1

[PATCH v4 5/5] hyperv: Do not overlap the hvcall IO areas in hv_vtl_apicid_to_vp_id()

[Roman Kisel]

The Top-Level Functional Specification for Hyper-V, Section 3.6 [1, 2],
disallows overlapping of the input and output hypercall areas, and
hv_vtl_apicid_to_vp_id() overlaps them.

Use the output hypercall page of the current vCPU for the hypercall.

[1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
[2] https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/main/tlfs

Reported-by: Michael Kelley <mhklinux@outlook.com>
Closes: https://lore.kernel.org/lkml/SN6PR02MB4157B98CD34781CC87A9D921D40D2@SN6PR02MB4157.namprd02.prod.outlook.com/
Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_vtl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 04775346369c..4e1b1e3b5658 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -189,7 +189,7 @@ static int hv_vtl_apicid_to_vp_id(u32 apic_id)
 	input->partition_id = HV_PARTITION_ID_SELF;
 	input->apic_ids[0] = apic_id;
 
-	output = (u32 *)input;
+	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
 
 	control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_ID_FROM_APIC_ID;
 	status = hv_do_hypercall(control, input, output);
-- 
2.34.1

Re: [PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers

[Easwar Hariharan]

On 12/27/2024 10:31 AM, Roman Kisel wrote:
> There is no definition of the output structure for the
> GetVpRegisters hypercall. Hence, using the hypercall
> is not possible when the output value has some structure
> to it. Even getting a datum of a primitive type reads
> as ad-hoc without that definition.
> 
> Define struct hv_output_get_vp_registers to enable using
> the GetVpRegisters hypercall. Make provisions for all
> supported architectures. No functional changes.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  include/hyperv/hvgdk_mini.h | 65 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 

Looks good to me.

Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>

Re: [PATCH v4 0/5] hyperv: Fixes for get_vtl(), hv_vtl_apicid_to_vp_id()

[Easwar Hariharan]

On 12/27/2024 10:31 AM, Roman Kisel wrote:
> The get_vtl(void) function
> 
> * has got one bug when the code started using a wrong pointer type after
>   refactoring, and also
> * the both function in question don't adhere to the requirements of
>   the Hypervisor Top-Level Funactional Specification[1, 2] as the code overlaps
>   the input and output areas for a hypercall.
> 
> The first issue leads to a wrong 100% reproducible computation due to reading
> a byte worth of data at a wrong offset. That in turn leads to using a nonsensical
> value ("fortunately", could catch it easily!) for the current VTL when initiating
> VMBus communications. As a repercussion from that, the system wouldn't boot. The
> fix is straightforward: use the correct pointer type.
> 
> The second issue doesn't seem to lead to any reproducible breakage just yet. It is
> fixed with using the output hypercall pages allocated per-CPU, and that isn't the
> only or the most obvious choice so let me elaborate why that fix appears to be the
> best one in my opinion out of the options I could conceive of.
> 
> The approach chosen for fixing the second issue makes two things shine through:
> 
> * these functions just get a vCPU register, no special treatment needs to be
>   involved,
> * VTLs and dom0 can and should share code as both exist to provide services to
>   a guest(s), be that from within the partition or from outside of it.
> 
> The projected benefits include replacing the functions in question with a future
> `hv_get_vp_registers` one shared between dom0 and VTLs to allow for a better test
> coverage.
> 
> I have validated the fixes by booting the fixed kernel in VTL2 up using OpenVMM and
> OpenHCL[3, 4].
> 
> [1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
> [2] https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/main/tlfs
> [3] https://openvmm.dev/guide/user_guide/openhcl.html
> [4] https://github.com/microsoft/OpenVMM
> 
> [v4]
>   - Wrapped DECLARE_FLEX_ARRAY into a struct and added one more
>     member as the documentation requires,
>   - Removed superfluous type coercion,
>   - Fixed tags,
>   - Rebased onto the latest hyperv-next branch.
> 
> [v3]: https://lore.kernel.org/lkml/20241226213110.899497-1-romank@linux.microsoft.com/
>   - Added a fix for hv_vtl_apicid_to_vp_id(),
>   - Split out the patch for enabling the hypercall output page,
>   - Updated the title of the patch series,
> 
> [v2]: https://lore.kernel.org/lkml/20241226203050.800524-1-romank@linux.microsoft.com/
>   - Used the suggestions to define an additional structure to improve code readability,
>   - Split out the patch with that definition.
> 
> [v1]: https://lore.kernel.org/lkml/20241218205421.319969-1-romank@linux.microsoft.com/
> 
> Roman Kisel (5):
>   hyperv: Define struct hv_output_get_vp_registers
>   hyperv: Fix pointer type in get_vtl(void)
>   hyperv: Enable the hypercall output page for the VTL mode
>   hyperv: Do not overlap the hvcall IO areas in get_vtl()
>   hyperv: Do not overlap the hvcall IO areas in hv_vtl_apicid_to_vp_id()
> 
>  arch/x86/hyperv/hv_init.c   |  6 ++--
>  arch/x86/hyperv/hv_vtl.c    |  2 +-
>  drivers/hv/hv_common.c      |  6 ++--
>  include/hyperv/hvgdk_mini.h | 65 +++++++++++++++++++++++++++++++++++--
>  4 files changed, 70 insertions(+), 9 deletions(-)
> 
> 
> base-commit: 26e1b813fcd02984b1cac5f3decdf4b0bb56fe02

Thank you for the persistence!

For the series,

Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>

Re: [PATCH v4 0/5] hyperv: Fixes for get_vtl(), hv_vtl_apicid_to_vp_id()

[Roman Kisel]

On 12/27/2024 10:42 AM, Easwar Hariharan wrote:
> On 12/27/2024 10:31 AM, Roman Kisel wrote:

[...]

> 
> Thank you for the persistence!
I feel most fortunate learning from you, Michael, and Nuno :)
Thank you!

> 
> For the series,
> 
> Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>

-- 
Thank you,
Roman

Re: [PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers

[kernel test robot]

Hi Roman,

kernel test robot noticed the following build errors:

[auto build test ERROR on 26e1b813fcd02984b1cac5f3decdf4b0bb56fe02]

url:    https://github.com/intel-lab-lkp/linux/commits/Roman-Kisel/hyperv-Define-struct-hv_output_get_vp_registers/20241228-023454
base:   26e1b813fcd02984b1cac5f3decdf4b0bb56fe02
patch link:    https://lore.kernel.org/r/20241227183155.122827-2-romank%40linux.microsoft.com
patch subject: [PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers
config: arm64-randconfig-001-20241228 (https://download.01.org/0day-ci/archive/20241228/202412280604.2Fvp7M4m-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241228/202412280604.2Fvp7M4m-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412280604.2Fvp7M4m-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/hyperv/hvhdk_mini.h:8,
                    from include/hyperv/hvhdk.h:10,
                    from arch/arm64/hyperv/hv_core.c:17:
>> include/hyperv/hvgdk_mini.h:828:40: error: field 'cs' has incomplete type
     828 |         struct hv_x64_segment_register cs;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:829:40: error: field 'ds' has incomplete type
     829 |         struct hv_x64_segment_register ds;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:830:40: error: field 'es' has incomplete type
     830 |         struct hv_x64_segment_register es;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:831:40: error: field 'fs' has incomplete type
     831 |         struct hv_x64_segment_register fs;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:832:40: error: field 'gs' has incomplete type
     832 |         struct hv_x64_segment_register gs;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:833:40: error: field 'ss' has incomplete type
     833 |         struct hv_x64_segment_register ss;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:834:40: error: field 'tr' has incomplete type
     834 |         struct hv_x64_segment_register tr;
         |                                        ^~
>> include/hyperv/hvgdk_mini.h:835:40: error: field 'ldtr' has incomplete type
     835 |         struct hv_x64_segment_register ldtr;
         |                                        ^~~~
>> include/hyperv/hvgdk_mini.h:837:38: error: field 'idtr' has incomplete type
     837 |         struct hv_x64_table_register idtr;
         |                                      ^~~~
>> include/hyperv/hvgdk_mini.h:838:38: error: field 'gdtr' has incomplete type
     838 |         struct hv_x64_table_register gdtr;
         |                                      ^~~~
>> include/hyperv/hvhdk.h:78:56: error: field 'es' has incomplete type
      78 |                         struct hv_x64_segment_register es;
         |                                                        ^~
>> include/hyperv/hvhdk.h:79:56: error: field 'cs' has incomplete type
      79 |                         struct hv_x64_segment_register cs;
         |                                                        ^~
>> include/hyperv/hvhdk.h:80:56: error: field 'ss' has incomplete type
      80 |                         struct hv_x64_segment_register ss;
         |                                                        ^~
>> include/hyperv/hvhdk.h:81:56: error: field 'ds' has incomplete type
      81 |                         struct hv_x64_segment_register ds;
         |                                                        ^~
>> include/hyperv/hvhdk.h:82:56: error: field 'fs' has incomplete type
      82 |                         struct hv_x64_segment_register fs;
         |                                                        ^~
>> include/hyperv/hvhdk.h:83:56: error: field 'gs' has incomplete type
      83 |                         struct hv_x64_segment_register gs;
         |                                                        ^~
>> include/hyperv/hvhdk.h:86:48: error: array type has incomplete element type 'struct hv_x64_segment_register'
      86 |                 struct hv_x64_segment_register segment_registers[6];
         |                                                ^~~~~~~~~~~~~~~~~
>> include/hyperv/hvhdk.h:95:52: error: field 'pending_interruption' has incomplete type
      95 |         union hv_x64_pending_interruption_register pending_interruption;
         |                                                    ^~~~~~~~~~~~~~~~~~~~
>> include/hyperv/hvhdk.h:96:47: error: field 'interrupt_state' has incomplete type
      96 |         union hv_x64_interrupt_state_register interrupt_state;
         |                                               ^~~~~~~~~~~~~~~


vim +/cs +828 include/hyperv/hvgdk_mini.h

dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  822  
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  823  struct hv_init_vp_context {
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  824  	u64 rip;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  825  	u64 rsp;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  826  	u64 rflags;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  827  
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @828  	struct hv_x64_segment_register cs;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @829  	struct hv_x64_segment_register ds;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @830  	struct hv_x64_segment_register es;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @831  	struct hv_x64_segment_register fs;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @832  	struct hv_x64_segment_register gs;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @833  	struct hv_x64_segment_register ss;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @834  	struct hv_x64_segment_register tr;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @835  	struct hv_x64_segment_register ldtr;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  836  
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @837  	struct hv_x64_table_register idtr;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25 @838  	struct hv_x64_table_register gdtr;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  839  
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  840  	u64 efer;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  841  	u64 cr0;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  842  	u64 cr3;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  843  	u64 cr4;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  844  	u64 msr_cr_pat;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  845  } __packed;
dd4f3a2b21ac14 Nuno Das Neves 2024-11-25  846  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, December 27, 2024 10:32 AM
> 
> There is no definition of the output structure for the
> GetVpRegisters hypercall. Hence, using the hypercall
> is not possible when the output value has some structure
> to it. Even getting a datum of a primitive type reads
> as ad-hoc without that definition.
> 
> Define struct hv_output_get_vp_registers to enable using
> the GetVpRegisters hypercall. Make provisions for all
> supported architectures. No functional changes.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  include/hyperv/hvgdk_mini.h | 65 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index db3d1aaf7330..c2f5cc231dce 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -781,6 +781,8 @@ struct hv_timer_message_payload {
>  	__u64 delivery_time;	/* When the message was delivered */
>  } __packed;
> 
> +#if defined(CONFIG_X86)
> +

From a thread [1] with Nuno, my understanding is that the
include/hyperv/* files should *not* use #ifdef <arch> unless
strictly necessary because a structure or symbol is used in
arch neutral code and has a different definition for each arch.

When the structure or symbol definition name contains "x64"
or "arm64", such conflicts won't occur, and the #ifdef isn't
needed. This does means that when compiling for either
x86 or arm64, the symbols from both architectures will be
visible in the symbol namespace, but that shouldn't cause any 
problems.

So it looks to me like none of the #ifdef's in this patch are
needed. Nuno -- please jump in if I have misunderstood.

Michael

[1] https://lore.kernel.org/linux-hyperv/SN6PR02MB4157AA30A9F27ECCAE202BC2D4582@SN6PR02MB4157.namprd02.prod.outlook.com/

>  struct hv_x64_segment_register {
>  	u64 base;
>  	u32 limit;
> @@ -807,6 +809,8 @@ struct hv_x64_table_register {
>  	u64 base;
>  } __packed;
> 
> +#endif
> +
>  union hv_input_vtl {
>  	u8 as_uint8;
>  	struct {
> @@ -1068,6 +1072,41 @@ union hv_dispatch_suspend_register {
>  	} __packed;
>  };
> 
> +#if defined(CONFIG_ARM64)
> +
> +union hv_arm64_pending_interruption_register {
> +	u64 as_uint64;
> +	struct {
> +		u64 interruption_pending : 1;
> +		u64 interruption_type : 1;
> +		u64 reserved : 30;
> +		u32 error_code;
> +	} __packed;
> +};
> +
> +union hv_arm64_interrupt_state_register {
> +	u64 as_uint64;
> +	struct {
> +		u64 interrupt_shadow : 1;
> +		u64 reserved : 63;
> +	} __packed;
> +};
> +
> +union hv_arm64_pending_synthetic_exception_event {
> +	u64 as_uint64[2];
> +	struct {
> +		u32 event_pending : 1;
> +		u32 event_type : 3;
> +		u32 reserved : 4;
> +		u32 exception_type;
> +		u64 context;
> +	} __packed;
> +};
> +
> +#endif
> +
> +#if defined(CONFIG_X86)
> +
>  union hv_x64_interrupt_state_register {
>  	u64 as_uint64;
>  	struct {
> @@ -1091,6 +1130,8 @@ union hv_x64_pending_interruption_register {
>  	} __packed;
>  };
> 
> +#endif
> +
>  union hv_register_value {
>  	struct hv_u128 reg128;
>  	u64 reg64;
> @@ -1098,13 +1139,33 @@ union hv_register_value {
>  	u16 reg16;
>  	u8 reg8;
> 
> -	struct hv_x64_segment_register segment;
> -	struct hv_x64_table_register table;
>  	union hv_explicit_suspend_register explicit_suspend;
>  	union hv_intercept_suspend_register intercept_suspend;
>  	union hv_dispatch_suspend_register dispatch_suspend;
> +#if defined(CONFIG_X86)
> +	struct hv_x64_segment_register segment;
> +	struct hv_x64_table_register table;
>  	union hv_x64_interrupt_state_register interrupt_state;
>  	union hv_x64_pending_interruption_register pending_interruption;
> +#endif
> +#if defined(CONFIG_ARM64)
> +	union hv_arm64_pending_interruption_register pending_interruption;
> +	union hv_arm64_interrupt_state_register interrupt_state;
> +	union hv_arm64_pending_synthetic_exception_event
> pending_synthetic_exception_event;
> +#endif
> +};
> +
> +/*
> + * NOTE: Linux helper struct - NOT from Hyper-V code.
> + * DECLARE_FLEX_ARRAY() needs to be wrapped into
> + * a structure and have at least one more member besides
> + * DECLARE_FLEX_ARRAY.
> + */
> +struct hv_output_get_vp_registers {
> +	struct {
> +		DECLARE_FLEX_ARRAY(union hv_register_value, values);
> +		struct {} values_end;
> +	};
>  };
> 
>  #if defined(CONFIG_ARM64)
> --
> 2.34.1

Re: [PATCH v4 1/5] hyperv: Define struct hv_output_get_vp_registers

[Roman Kisel]

On 12/29/2024 9:46 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, December 27, 2024 10:32 AM

[...]

>>From a thread [1] with Nuno, my understanding is that the
> include/hyperv/* files should *not* use #ifdef <arch> unless
> strictly necessary because a structure or symbol is used in
> arch neutral code and has a different definition for each arch.
> 
> When the structure or symbol definition name contains "x64"
> or "arm64", such conflicts won't occur, and the #ifdef isn't
> needed. This does means that when compiling for either
> x86 or arm64, the symbols from both architectures will be
> visible in the symbol namespace, but that shouldn't cause any
> problems.
> 
> So it looks to me like none of the #ifdef's in this patch are
> needed. Nuno -- please jump in if I have misunderstood.
> 
Understood, thank you, Michael! From the thread you linked,
it appears that there were other approaches with other trade-offs
to be had, and this route was considered to be the most beneficial
one.

Let me spin up another version with these guards removed.
Thanks you all for bearing with me so far!

> Michael
> 
> [1] https://lore.kernel.org/linux-hyperv/SN6PR02MB4157AA30A9F27ECCAE202BC2D4582@SN6PR02MB4157.namprd02.prod.outlook.com/
> 
>>   struct hv_x64_segment_register {
>>   	u64 base;
>>   	u32 limit;
>> @@ -807,6 +809,8 @@ struct hv_x64_table_register {
>>   	u64 base;
>>   } __packed;
>>
>> +#endif
>> +
>>   union hv_input_vtl {
>>   	u8 as_uint8;
>>   	struct {
>> @@ -1068,6 +1072,41 @@ union hv_dispatch_suspend_register {
>>   	} __packed;
>>   };
>>
>> +#if defined(CONFIG_ARM64)
>> +
>> +union hv_arm64_pending_interruption_register {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 interruption_pending : 1;
>> +		u64 interruption_type : 1;
>> +		u64 reserved : 30;
>> +		u32 error_code;
>> +	} __packed;
>> +};
>> +
>> +union hv_arm64_interrupt_state_register {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 interrupt_shadow : 1;
>> +		u64 reserved : 63;
>> +	} __packed;
>> +};
>> +
>> +union hv_arm64_pending_synthetic_exception_event {
>> +	u64 as_uint64[2];
>> +	struct {
>> +		u32 event_pending : 1;
>> +		u32 event_type : 3;
>> +		u32 reserved : 4;
>> +		u32 exception_type;
>> +		u64 context;
>> +	} __packed;
>> +};
>> +
>> +#endif
>> +
>> +#if defined(CONFIG_X86)
>> +
>>   union hv_x64_interrupt_state_register {
>>   	u64 as_uint64;
>>   	struct {
>> @@ -1091,6 +1130,8 @@ union hv_x64_pending_interruption_register {
>>   	} __packed;
>>   };
>>
>> +#endif
>> +
>>   union hv_register_value {
>>   	struct hv_u128 reg128;
>>   	u64 reg64;
>> @@ -1098,13 +1139,33 @@ union hv_register_value {
>>   	u16 reg16;
>>   	u8 reg8;
>>
>> -	struct hv_x64_segment_register segment;
>> -	struct hv_x64_table_register table;
>>   	union hv_explicit_suspend_register explicit_suspend;
>>   	union hv_intercept_suspend_register intercept_suspend;
>>   	union hv_dispatch_suspend_register dispatch_suspend;
>> +#if defined(CONFIG_X86)
>> +	struct hv_x64_segment_register segment;
>> +	struct hv_x64_table_register table;
>>   	union hv_x64_interrupt_state_register interrupt_state;
>>   	union hv_x64_pending_interruption_register pending_interruption;
>> +#endif
>> +#if defined(CONFIG_ARM64)
>> +	union hv_arm64_pending_interruption_register pending_interruption;
>> +	union hv_arm64_interrupt_state_register interrupt_state;
>> +	union hv_arm64_pending_synthetic_exception_event
>> pending_synthetic_exception_event;
>> +#endif
>> +};
>> +
>> +/*
>> + * NOTE: Linux helper struct - NOT from Hyper-V code.
>> + * DECLARE_FLEX_ARRAY() needs to be wrapped into
>> + * a structure and have at least one more member besides
>> + * DECLARE_FLEX_ARRAY.
>> + */
>> +struct hv_output_get_vp_registers {
>> +	struct {
>> +		DECLARE_FLEX_ARRAY(union hv_register_value, values);
>> +		struct {} values_end;
>> +	};
>>   };
>>
>>   #if defined(CONFIG_ARM64)
>> --
>> 2.34.1

-- 
Thank you,
Roman