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

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 1/6/2025 9:37 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, December 30, 2024 10:10 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 | 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.

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

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

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

>>
>>  #if defined(CONFIG_ARM64)
>> --
>> 2.34.1