linux-kernel - RE: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <BN7PR02MB4148CC3F9091BC2604E457CFD4922@BN7PR02MB4148.namprd02.prod.outlook.com>
Date: Mon, 2 Sep 2024 03:35:13 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Yunhong Jiang <yunhong.jiang@...ux.intel.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>, "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"kys@...rosoft.com" <kys@...rosoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "rafael@...nel.org"
	<rafael@...nel.org>, "lenb@...nel.org" <lenb@...nel.org>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: RE: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

From: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
> 
> Parse the wakeup mailbox VTL2 TDX guest. Put it to the guest_late_init, so
> that it will be invoked before hyperv_init() where the mailbox address is
> checked.

Could you elaborate on the choice to set the wakeup_mailbox_address
in ms_hyperv_late_init()? The code in hv_common.c is intended to be
code that is architecture neutral (see the comment at the top of the module),
so it's a red flag to see #ifdef CONFIG_X86_64. Couldn't the
wakeup_mailbox_address be set in the x86 version of hyperv_init()
before it is needed?

> 
> Signed-off-by: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
> ---
>  arch/x86/include/asm/mshyperv.h | 3 +++
>  arch/x86/kernel/cpu/mshyperv.c  | 2 ++
>  drivers/hv/hv_common.c          | 8 ++++++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 390c4d13956d..5178b96c7fc9 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -10,6 +10,7 @@
>  #include <asm/nospec-branch.h>
>  #include <asm/paravirt.h>
>  #include <asm/mshyperv.h>
> +#include <asm/madt_wakeup.h>
> 
>  /*
>   * Hyper-V always provides a single IO-APIC at this MMIO address.
> @@ -49,6 +50,8 @@ extern u64 hv_current_partition_id;
> 
>  extern union hv_ghcb * __percpu *hv_ghcb_pg;
> 
> +extern u64 wakeup_mailbox_addr;
> +
>  bool hv_isolation_type_snp(void);
>  bool hv_isolation_type_tdx(void);
>  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 3d4237f27569..f6b727b4bd0b 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -43,6 +43,8 @@ struct ms_hyperv_info ms_hyperv;
>  bool hyperv_paravisor_present __ro_after_init;
>  EXPORT_SYMBOL_GPL(hyperv_paravisor_present);
> 
> +u64 wakeup_mailbox_addr;

This value duplicates acpi_mp_wake_mailbox_paddr in
madt_wakeup.c. It looks like the duplicate value is used
for two things:

1) In hv_is_private_mmio_tdx() to control the encrypted
vs. decrypted mapping (Patch 5 of this series)

2) As a boolean in hv_vtl_early_init() to avoid overwriting
the wakeup_secondary_cpu_64 value when
dtb_parse_mp_wake() has set it to acpi_wakeup_cpu().
(Patch 9 of this series).

Having a duplicate value is messy, and I'm wondering if
it can be avoided. For (1), hv_private_mmio_tdx() could call
into a function added to madt_wakeup.c to make the
check.  For (2), the check should probably be based on
hv_isolation_type_tdx() instead of whether the wakeup
mailbox address is set.  I'll note that Patch 5 of this series
is using hv_isolation_type_tdx(), so there's a bit of an
inconsistency in testing the wakeup_mailbox_addr in
Patch 9.

This is just a suggestion, as I haven't worked out all
the details. If you think it ends up being messier than
the duplicate value, then I'm OK with it.

Michael

> +
>  #if IS_ENABLED(CONFIG_HYPERV)
>  static inline unsigned int hv_get_nested_msr(unsigned int reg)
>  {
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 9c452bfbd571..14b005b6270f 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -365,6 +365,14 @@ void __init ms_hyperv_late_init(void)
>  	u8 *randomdata;
>  	u32 length, i;
> 
> +	/*
> +	 * Parse the ACPI wakeup structure information from device tree.
> +	 * Currently VTL2 TDX guest only.
> +	 */
> +#ifdef CONFIG_X86_64
> +	wakeup_mailbox_addr = dtb_parse_mp_wake();
> +#endif
> +
>  	/*
>  	 * Seed the Linux random number generator with entropy provided by
>  	 * the Hyper-V host in ACPI table OEM0.
> --
> 2.25.1
>