On 1/6/2025 9:37 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx/
>>
>> #if defined(CONFIG_ARM64)
>> --
>> 2.34.1
