On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote:
> From: Yunhong Jiang <yunhong.jiang@xxxxxxxxxxxxxxx>
> >
> > 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.
>
> >
> > Signed-off-by: Yunhong Jiang <yunhong.jiang@xxxxxxxxxxxxxxx>
> > ---
> > 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
>
> 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
> >
>
