| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<SN6PR02MB4157F8C28BD92F36B27C9032D4102@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 6 Jan 2025 17:37:21 +0000
From: Michael Kelley <mhklinux@...look.com>
To: 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>,
"nunodasneves@...ux.microsoft.com" <nunodasneves@...ux.microsoft.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: 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;
> + } __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.
> + 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?
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.
> + */
> +struct hv_output_get_vp_registers {
> + struct {
> + DECLARE_FLEX_ARRAY(union hv_register_value, values);
> + struct {} values_end;
> + };
> };
>
> #if defined(CONFIG_ARM64)
> --
> 2.34.1
Powered by blists - more mailing lists