Message-ID:
 <BN7PR02MB41482C89094DBC3E811C5918D4122@BN7PR02MB4148.namprd02.prod.outlook.com>
Date: Wed, 8 Jan 2025 18:22:01 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>, Roman Kisel
	<romank@...ux.microsoft.com>, "hpa@...or.com" <hpa@...or.com>,
	"kys@...rosoft.com" <kys@...rosoft.com>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "eahariha@...ux.microsoft.com"
	<eahariha@...ux.microsoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "mingo@...hat.com" <mingo@...hat.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "tiala@...rosoft.com"
	<tiala@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
	<benhill@...rosoft.com>, "ssengar@...rosoft.com" <ssengar@...rosoft.com>,
	"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>, "vdso@...bites.dev"
	<vdso@...bites.dev>
Subject: RE: [PATCH v5 1/5] hyperv: Define struct hv_output_get_vp_registers

From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Wednesday, January 8, 2025 9:58 AM
> 
> On 1/6/2025 9:37 AM, Michael Kelley wrote:
> > From: Roman Kisel <romank@...ux.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@...ux.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.

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

Powered by blists - more mailing lists