Re: [PATCH v2 4/9] x86/hyperv: Parse the ACPI wakeup mailbox

On Wed, Sep 04, 2024 at 02:56:49PM +0000, Michael Kelley wrote:

From: Yunhong Jiang <redacted> Sent: Tuesday, September 3, 2024 1:19 PM

quoted

On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote:

quoted

From: Yunhong Jiang <redacted>

quoted

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?

Sure, will try to put it in hyperv_init() before it's needed.

quoted

quoted

Signed-off-by: Yunhong Jiang <redacted>
---
 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.

I think your comment includes two points, the duplicated variables and the
incosistency in the testing.

Thank you for pointing out the duplication of wakeup_mailbox_addr with
acpi_mp_wake_mailbox_paddr. I didn't realize it. Yes, such duplication should be
avoided and will fix it in next submission.

Agree the inconsistency in testing wakeup_mailbox_addr and
hv_isolation_type_tdx() is not good. IMHO, the wakeup_mailbox_addr (or the new
function you proposed) is better than hv_isolation_type_tdx(), since the
wakeup_mailbox_addr is more directly related.  But hv_vtl_init_platform()
happens before DT parse, thus I have to use the hv_isolation_type_tdx() in it. I
don't have a good idea on how to fix this.

Thanks
--jyh

I don't think there's a requirement to set the "is_private_mmio"
function in hv_vtl_init_platform(). It just needs to be set before
acpi_wakeup_cpu() is called, which does the memremap() that will
invoke the "is_private_mmio" function to decide whether to map
as encrypted or decrypted.

So maybe setting the "is_private_mmio" function could be
done after dtb_parse_mp_wake() is called in its new location, and
you know you have a valid wake mailbox address? Again, I haven't
worked out all the details, so this approach might be just as messy,
but in a different way. Use your judgment ... :-)

Sorry that I didn't explain clearly. The testing in hv_vtl_init_platform() is
not only for the is_private_mmio, but also for the realmode_reserve(), which
happens before the DT parse.

BTW, I don't know why the trampoline_64.S is put into the real mode blob. I
don't find any specific requirement in the code, but I'm not sure if I missed
anything. If this dependency is removed, all the TDX guest will benefit.

Thank you
--jyh

Michael