From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, January 8, 2025 9:58 AM
>
> 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.
Matching the original headers by using the second one is
fine with me.
>
> >> + } __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];
I'm good with that. For the ABI between the host and guest, we
*do* want to make all the padding explicit.
>
> >> + 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!
OK by me as well.
>
> >
> > 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);
> };
It was actually Easwar who mentioned this. But regardless, I'm glad
the simpler definition works!
Michael
