On 1/6/2025 9:37 AM, Michael Kelley wrote:

From: Roman Kisel <redacted> Sent: Monday, December 30, 2024 10:10 AM

quoted

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 <redacted>
---
 include/hyperv/hvgdk_mini.h | 49 +++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index db3d1aaf7330..e8e3faa78e15 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h

@@ -1068,6 +1068,35 @@ union hv_dispatch_suspend_register {
 	} __packed;
 };

+union hv_arm64_pending_interruption_register {
+	u64 as_uint64;
+	struct {
+		u64 interruption_pending : 1;
+		u64 interruption_type : 1;
+		u64 reserved : 30;
+		u32 error_code;

These bit field definitions don't look right. We want to "fill up"
the field size, so that we're explicit about each bit, and not leave
it to the compiler to add padding (which __packed tells the
compiler not to do). So in aggregate, the "u64" bit fields should
account for all 64 bits, but here you account for only 32 bits.
There are two ways to fix this:

		u32 interruption_pending : 1;
		u32 interruption_type: 1;
		u32 reserved : 30;
		u32 error_code;
Or
		u64 interruption_pending : 1;
		u64 interruption_type: 1;
		u64 reserved : 30;
		u64 error_code : 32;

Agreed. In the spirit of matching the original headers, I'd prefer
the second one. But either will work.

quoted

+	} __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;

Same here. Expand the "reserved" field to 28 bits?  Or maybe
there's a reason to have two separate reserved fields of 4 bits
and 24 bits. I'm not sure what the register layout is supposed to
be. Looking at hv_arm64_pending_synthetic_exception_event
in the OHCL-Linux-Kernel github tree shows the same gap of
24 bits, so that doesn't provide any guidance.

Hmm..these should be u8 bitfields according to the Hyper-V code.
However that leaves a 24 bit gap as you pointed out.

In the Hyper-V code, these structures aren't actually packed,
which means sometimes the explicit padding is left out
(unintentionally).

Please add the 24 bits of padding to make it explicit here. I
suggest making the bitfields u8 as in the original code, and adding
another padding field after, like:

u8 event_pending : 1;
u8 event_type : 3;
u8 reserved : 4;
u8 rsvd[3];

quoted

+		u32 exception_type;
+		u64 context;
+	} __packed;
+};
+
 union hv_x64_interrupt_state_register {
 	u64 as_uint64;
 	struct {

@@ -1103,8 +1132,28 @@ union hv_register_value {
 	union hv_explicit_suspend_register explicit_suspend;
 	union hv_intercept_suspend_register intercept_suspend;
 	union hv_dispatch_suspend_register dispatch_suspend;
+#ifdef CONFIG_ARM64
+	union hv_arm64_interrupt_state_register interrupt_state;
+	union hv_arm64_pending_interruption_register pending_interruption;
+#endif
+#ifdef CONFIG_X86
 	union hv_x64_interrupt_state_register interrupt_state;
 	union hv_x64_pending_interruption_register pending_interruption;
+#endif
+	union hv_arm64_pending_synthetic_exception_event pending_synthetic_exception_event;
+};

Per the previous discussion, I can see that the #ifdef's are needed
here to disambiguate the field names that are the same, but have
different unions on x86 and arm64.

But on the flip side, I wonder if the field names should really be the
same. Because of the different unions, it seems like they couldn't be
accessed by architecture neutral code (unless the access is just using
the "as_uint64" option?). So giving the fields names like
"x86_interrupt_state" and "arm64_interrupt_state" instead of just
"interrupt_state" might be more consistent with how the rest of this
file handles architecture differences. But I don't know all the implications
of making such a change.

Nuno -- your thoughts?

My main preference is to match with the original code unless there are *serious*
clarity, style or incompatibility issues. I don't see a big problem with gating
or not gating these. As you pointed out, it *may* make arch-neutral code a little
more cumbersome. But it's hard to say if that will actually be a problem.

Right now it seems to match the Hyper-V code and seems fine to me!

Michael

quoted

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

See below - you can remove the second part of this comment and just
leave the first line clarifying this is a Linux-only helper.

quoted

+struct hv_output_get_vp_registers {
+	struct {
+		DECLARE_FLEX_ARRAY(union hv_register_value, values);
+		struct {} values_end;
+	};
 };

I missed this change from a previous version - the additional empty struct
isn't needed here.

Michael - The documentation comment you mentioned previously[1] is just
describing how the DECLARE_FLEX_ARRAY() macro works - it actually adds
the empty struct to placate the compiler.

See include/uapi/linux/stddef.h:47:

#define __DECLARE_FLEX_ARRAY(TYPE, NAME)	\
	struct { \
		struct { } __empty_ ## NAME; \
		TYPE NAME[]; \
	}
#endif

So the definition should just look like:

struct hv_output_get_vp_registers {
	DECLARE_FLEX_ARRAY(union hv_register_value, values);
};


Thanks
Nuno


[1] https://lore.kernel.org/linux-hyperv/1bf0ce72-a377-4c3f-b68a-0f890f8b5d09@linux.microsoft.com/ (local)

quoted

 #if defined(CONFIG_ARM64)
--
2.34.1