| Message ID | 20240823232327.2408869-5-yunhong.jiang@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | x86/hyperv: Support wakeup mailbox for VTL2 TDX guest | expand |
From: Yunhong Jiang <yunhong.jiang@linux.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@linux.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 >
On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote: > From: Yunhong Jiang <yunhong.jiang@linux.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? Sure, will try to put it in hyperv_init() before it's needed. > > > > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.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. 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 > > >
From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Tuesday, September 3, 2024 1:19 PM > > On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote: > > From: Yunhong Jiang <yunhong.jiang@linux.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? > > Sure, will try to put it in hyperv_init() before it's needed. > > > > > > > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.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. > > 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 ... :-) Michael
On Wed, Sep 04, 2024 at 02:56:49PM +0000, Michael Kelley wrote: > From: Yunhong Jiang <yunhong.jiang@linux.intel.com> Sent: Tuesday, September 3, 2024 1:19 PM > > > > On Mon, Sep 02, 2024 at 03:35:13AM +0000, Michael Kelley wrote: > > > From: Yunhong Jiang <yunhong.jiang@linux.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? > > > > Sure, will try to put it in hyperv_init() before it's needed. > > > > > > > > > > > Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.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. > > > > 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
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; + #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.
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. Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.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(+)