Message-ID:
 <SN6PR02MB4157963DE55041D0631188A4D49C2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 4 Sep 2024 14:56:49 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
CC: "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>, "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> 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@...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?
> 
> Sure, will try to put it in hyperv_init() before it's 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.
> 
> 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

Powered by blists - more mailing lists