Message-ID: <SA1PR21MB1335945301F8E8FBC2CA15B5BF1CA@SA1PR21MB1335.namprd21.prod.outlook.com>
Date:   Wed, 23 Aug 2023 04:23:50 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "arnd@...db.de" <arnd@...db.de>, "bp@...en8.de" <bp@...en8.de>,
        "brijesh.singh@....com" <brijesh.singh@....com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "dave.hansen@...el.com" <dave.hansen@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "jane.chu@...cle.com" <jane.chu@...cle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        KY Srinivasan <kys@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>, jason <jason@...c4.com>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>
CC:     "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "rick.p.edgecombe@...el.com" <rick.p.edgecombe@...el.com>,
        Anthony Davis <andavis@...hat.com>,
        Mark Heslin <mheslin@...hat.com>,
        vkuznets <vkuznets@...hat.com>,
        "xiaoyao.li@...el.com" <xiaoyao.li@...el.com>
Subject: RE: [PATCH v2 6/9] x86/hyperv: Introduce a global variable
 hyperv_paravisor_present

> From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> Sent: Monday, August 21, 2023 12:33 PM
>  [...]
> From: Dexuan Cui <decui@...rosoft.com> Sent: Sunday, August 20, 2023
> >
> > The new variable hyperv_paravisor_present is set only when the VM
> > is a SNP/TDX with the paravisor running: see ms_hyperv_init_platform().
> >
> > In many places, hyperv_paravisor_present can replace
> 
> You said "In many places".  Are there places where it can't replace
> ms_hyperv.paravisor_present?  It looks like all the uses are gone
> after this patch.

Sorry for the inaccuracy. I meant to say "everywhere" rather than
"In many places".  I think hyperv_paravisor_present and
ms_hyperv.paravisor_present can be used interchangeably except that
I need to use hyperv_paravisor_present in arch/x86/include/asm/mshyperv.h
to avoid the header file dependency issue.

As we discussed offline, we'll add some hypercall function structure in future
for different VM types, and then hyperv_paravisor_present and
ms_hyperv.paravisor_present would go away when we make hypercalls.

So let me use hyperv_paravisor_present only in
arch/x86/include/asm/mshyperv.h to make the patch smaller.

> > ms_hyperv.paravisor_present, and it's also used to replace
> > hv_isolation_type_snp() in drivers/hv/hv.c.
> >
> > Call hv_vtom_init() when it's a VBS VM or when hyperv_paravisor_present
> > is true (i.e. the VM is a SNP/TDX VM with the paravisor).
> >
> > Enhance hv_vtom_init() for a TDX VM with the paravisor.
> >
> > The biggest motive to introduce hyperv_paravisor_present is that we
> > can not use ms_hyperv.paravisor_present in
> arch/x86/include/asm/mshyperv.h:
> > that would introduce a complicated header file dependency issue.
> 
> The discussion in this commit messages about hyperv_paravisor_present
> is a bit scattered and confusing.  I think you are introducing the global
> variable
> to solve the header file dependency issue.  Otherwise, the ms_hyperv field
> would be equivalent.  Then you are using hyperv_paravisor_present for
> several purposes, including to decide whether to call hv_vtom_init() and
> to simplify the logic in drivers/hv/hv.c.  Maybe you could reorganize
> the commit message a bit to be more direct regarding the purpose.

The new changelog will be:

    x86/hyperv: Introduce a global variable hyperv_paravisor_present

    The new variable hyperv_paravisor_present is set only when the VM
    is a SNP/TDX VM with the paravisor running: see ms_hyperv_init_platform().

    We introduce hyperv_paravisor_present because we can not use
    ms_hyperv.paravisor_present in arch/x86/include/asm/mshyperv.h:

    struct ms_hyperv_info is defined in include/asm-generic/mshyperv.h, which
    is included at the end of arch/x86/include/asm/mshyperv.h, but at the
    beginning of arch/x86/include/asm/mshyperv.h, we would already need to use
    struct ms_hyperv_info in hv_do_hypercall().

    We use hyperv_paravisor_present only in include/asm-generic/mshyperv.h,
    and use ms_hyperv.paravisor_present elsewhere. In the future, we'll
    introduce a hypercall function structure for different VM types, and
    at boot time, the right function pointers would be written into the
    structure so that runtime testing of TDX vs. SNP vs. normal will be
    avoided and hyperv_paravisor_present will no longer be needed.

    Call hv_vtom_init() when it's a VBS VM or when ms_hyperv.paravisor_present
    is true, i.e. the VM is a SNP VM or TDX VM with the paravisor.

    Enhance hv_vtom_init() for a TDX VM with the paravisor.

    In hv_common_cpu_init(), don't decrypt the hyperv_pcpu_input_arg
    for a TDX VM with the paravisor, just like we don't decrypt the page
    for a SNP VM with the paravisor.

BTW, please refer to the link for the v3 version of this patch (WIP):

> > In arch/x86/include/asm/mshyperv.h, _hv_do_fast_hypercall8()
> > is changed to specially handle HVCALL_SIGNAL_EVENT for a TDX VM with
> the
> > paravisor, because such a VM must use TDX GHCI (see hv_tdx_hypercall())
> > for this hypercall. See vmbus_set_event() -> hv_do_fast_hypercall8() ->
> > _hv_do_fast_hypercall8() -> hv_tdx_hypercall().
> 
> Embedding the special case for HVCALL_SIGNAL_EVENT within
> hv_do_fast_hypercall8() is not consistent with how this special case
> is handled for SNP.  For SNP, the special case is coded directly into
> vmbus_set_event().  Any reason not to do the same for TDX + paravisor?

Ok, will handle it directly in vmbus_set_event().

> > @@ -497,13 +497,29 @@ int hv_snp_boot_ap(int cpu, unsigned long
> start_ip)
> >
> >  void __init hv_vtom_init(void)
> >  {
> > +	enum hv_isolation_type type = hv_get_isolation_type();
> >  	/*
> >  	 * By design, a VM using vTOM doesn't see the SEV setting,
> >  	 * so SEV initialization is bypassed and sev_status isn't set.
> >  	 * Set it here to indicate a vTOM VM.
> >  	 */
> 
> The above comment applies just to the case HV_ISOLATION_TYPE_SNP,
> not to the entire switch statement, so it should be moved under the
> case.

Will fix.

> > [...]
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
[...]
> > @@ -134,7 +135,9 @@ static inline u64 _hv_do_fast_hypercall8(u64
> control, u64 input1)
> >  	u64 hv_status;
> >
> >  #ifdef CONFIG_X86_64
> > -	if (hv_isolation_type_tdx())
> > +	if (hv_isolation_type_tdx() &&
> > +		(!hyperv_paravisor_present ||
> > +		 control == (HVCALL_SIGNAL_EVENT |
> HV_HYPERCALL_FAST_BIT)))
> 
> See comment above.  This would be more consistent with SNP if it were
> handled directly in vmbus_set_event().

Will fix.

> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -484,7 +484,7 @@ void vmbus_set_event(struct vmbus_channel
> *channel)
> >
> >  	++channel->sig_events;
> >
> > -	if (hv_isolation_type_snp())
> > +	if (hv_isolation_type_snp() && hyperv_paravisor_present)
> 
> This code change seems to be more properly part of Patch 9 in the
> series when hv_isolation_type_en_snp() goes away.

The change here is part of the efforts to correctly support hypercalls for
a TDX VM with the paravisor. Patch 9 is just a clean-up patch, which is
not really required for a TDX VM (with or with the paravisor) to run on
Hyper-V, so IMO it's better to keep the change here in this patch.

BTW, please refer to the link for the v3 version of this patch (WIP):

Powered by blists - more mailing lists