Received-SPF: pass (google.com: domain of linux-crypto+bounces-4531-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Date: Thu, 30 May 2024 19:47:41 +0300
From: Zhi Wang <zhiw@nvidia.com>
To: Michael Roth <michael.roth@amd.com>
CC: Binbin Wu <binbin.wu@linux.intel.com>, Paolo Bonzini
	<pbonzini@redhat.com>, <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
	<linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, <tglx@linutronix.de>, <mingo@redhat.com>,
	<jroedel@suse.de>, <thomas.lendacky@amd.com>, <hpa@zytor.com>,
	<ardb@kernel.org>, <seanjc@google.com>, <vkuznets@redhat.com>,
	<jmattson@google.com>, <luto@kernel.org>, <dave.hansen@linux.intel.com>,
	<slp@redhat.com>, <pgonda@google.com>, <peterz@infradead.org>,
	<srinivas.pandruvada@linux.intel.com>, <rientjes@google.com>,
	<dovmurik@linux.ibm.com>, <tobin@ibm.com>, <bp@alien8.de>, <vbabka@suse.cz>,
	<kirill@shutemov.name>, <ak@linux.intel.com>, <tony.luck@intel.com>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>, <alpergun@google.com>,
	<jarkko@kernel.org>, <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
	<pankaj.gupta@amd.com>, <liam.merwick@oracle.com>, Brijesh Singh
	<brijesh.singh@amd.com>, "Yamahata, Isaku" <isaku.yamahata@intel.com>
Subject: Re: [PATCH v15 09/20] KVM: SEV: Add support to handle MSR based
 Page State Change VMGEXIT
Message-ID: <20240530194606.00003f3a.zhiw@nvidia.com>
In-Reply-To: <rczrxq3lhqguarwh4cwxwa35j5riiagbilcw32oaxd7aqpyaq7@6bqrqn6ontba>
References: <20240501085210.2213060-1-michael.roth@amd.com>
	<20240501085210.2213060-10-michael.roth@amd.com>
	<84e8460d-f8e7-46d7-a274-90ea7aec2203@linux.intel.com>
	<CABgObfaXmMUYHEuK+D+2E9pybKMJqGZsKB033X1aOSQHSEqqVA@mail.gmail.com>
	<7d6a4320-89f5-48ce-95ff-54b00e7e9597@linux.intel.com>
	<rczrxq3lhqguarwh4cwxwa35j5riiagbilcw32oaxd7aqpyaq7@6bqrqn6ontba>
Organization: NVIDIA
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 16:48:23.8589
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: c12050d6-8097-4c92-eb3d-08dc80c85228
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	CH3PEPF00000017.namprd21.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8585

On Tue, 21 May 2024 16:49:52 -0500
Michael Roth <michael.roth@amd.com> wrote:

> On Tue, May 21, 2024 at 08:49:59AM +0800, Binbin Wu wrote:
> >=20
> >=20
> > On 5/17/2024 1:23 AM, Paolo Bonzini wrote:
> > > On Thu, May 16, 2024 at 10:29=E2=80=AFAM Binbin Wu
> > > <binbin.wu@linux.intel.com> wrote:
> > > >=20
> > > >=20
> > > > On 5/1/2024 4:51 PM, Michael Roth wrote:
> > > > > SEV-SNP VMs can ask the hypervisor to change the page state
> > > > > in the RMP table to be private or shared using the Page State
> > > > > Change MSR protocol as defined in the GHCB specification.
> > > > >=20
> > > > > When using gmem, private/shared memory is allocated through
> > > > > separate pools, and KVM relies on userspace issuing a
> > > > > KVM_SET_MEMORY_ATTRIBUTES KVM ioctl to tell the KVM MMU
> > > > > whether or not a particular GFN should be backed by private
> > > > > memory or not.
> > > > >=20
> > > > > Forward these page state change requests to userspace so that
> > > > > it can issue the expected KVM ioctls. The KVM MMU will handle
> > > > > updating the RMP entries when it is ready to map a private
> > > > > page into a guest.
> > > > >=20
> > > > > Use the existing KVM_HC_MAP_GPA_RANGE hypercall format to
> > > > > deliver these requests to userspace via KVM_EXIT_HYPERCALL.
> > > > >=20
> > > > > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > > > > Co-developed-by: Brijesh Singh <brijesh.singh@amd.com>
> > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > > > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > > > > ---
> > > > >    arch/x86/include/asm/sev-common.h |  6 ++++
> > > > >    arch/x86/kvm/svm/sev.c            | 48
> > > > > +++++++++++++++++++++++++++++++ 2 files changed, 54
> > > > > insertions(+)
> > > > >=20
> > > > > diff --git a/arch/x86/include/asm/sev-common.h
> > > > > b/arch/x86/include/asm/sev-common.h index
> > > > > 1006bfffe07a..6d68db812de1 100644 ---
> > > > > a/arch/x86/include/asm/sev-common.h +++
> > > > > b/arch/x86/include/asm/sev-common.h @@ -101,11 +101,17 @@
> > > > > enum psc_op { /* GHCBData[11:0] */
> > > > > \ GHCB_MSR_PSC_REQ)
> > > > >=20
> > > > > +#define GHCB_MSR_PSC_REQ_TO_GFN(msr) (((msr) &
> > > > > GENMASK_ULL(51, 12)) >> 12) +#define
> > > > > GHCB_MSR_PSC_REQ_TO_OP(msr) (((msr) & GENMASK_ULL(55, 52)) >>
> > > > > 52) + #define GHCB_MSR_PSC_RESP           0x015
> > > > >    #define GHCB_MSR_PSC_RESP_VAL(val)                  \
> > > > >        /* GHCBData[63:32] */                           \
> > > > >        (((u64)(val) & GENMASK_ULL(63, 32)) >> 32)
> > > > >=20
> > > > > +/* Set highest bit as a generic error response */
> > > > > +#define GHCB_MSR_PSC_RESP_ERROR (BIT_ULL(63) |
> > > > > GHCB_MSR_PSC_RESP) +
> > > > >    /* GHCB Hypervisor Feature Request/Response */
> > > > >    #define GHCB_MSR_HV_FT_REQ          0x080
> > > > >    #define GHCB_MSR_HV_FT_RESP         0x081
> > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > > index e1ac5af4cb74..720775c9d0b8 100644
> > > > > --- a/arch/x86/kvm/svm/sev.c
> > > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > > @@ -3461,6 +3461,48 @@ static void set_ghcb_msr(struct
> > > > > vcpu_svm *svm, u64 value) svm->vmcb->control.ghcb_gpa =3D value;
> > > > >    }
> > > > >=20
> > > > > +static int snp_complete_psc_msr(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +     struct vcpu_svm *svm =3D to_svm(vcpu);
> > > > > +
> > > > > +     if (vcpu->run->hypercall.ret)
> > > > Do we have definition of ret? I didn't find clear documentation
> > > > about it. According to the code, 0 means succssful. Is there
> > > > any other error codes need to or can be interpreted?
> > > They are defined in include/uapi/linux/kvm_para.h
> > >=20
> > > #define KVM_ENOSYS        1000
> > > #define KVM_EFAULT        EFAULT /* 14 */
> > > #define KVM_EINVAL        EINVAL /* 22 */
> > > #define KVM_E2BIG        E2BIG /* 7 */
> > > #define KVM_EPERM        EPERM /* 1*/
> > > #define KVM_EOPNOTSUPP        95
> > >=20
> > > Linux however does not expect the hypercall to fail for
> > > SEV/SEV-ES; and it will terminate the guest if the PSC operation
> > > fails for SEV-SNP.  So it's best for userspace if the hypercall
> > > always succeeds. :)
> > Thanks for the info.
> >=20
> > For TDX, it wants to restrict the size of memory range for
> > conversion in one hypercall to avoid a too long latency.
> > Previously, in TDX QEMU patchset v5, the limitation is in userspace
> > and=C2=A0 if the size is too big, the status_code will set to
> > TDG_VP_VMCALL_RETRY and the failed GPA for guest to retry is
> > updated.
> > https://lore.kernel.org/all/20240229063726.610065-51-xiaoyao.li@intel.c=
om/
> >=20
> > When TDX converts TDVMCALL_MAP_GPA to KVM_HC_MAP_GPA_RANGE, do you
> > think which is more reasonable to set the restriction? In KVM (TDX
> > specific code) or userspace?
> > If userspace is preferred, then the interface needs to=C2=A0 be extended
> > to support it.
>=20
> With SNP we might get a batch of requests in a single GHCB request,
> and potentially each of those requests need to get set out to
> userspace as a single KVM_HC_MAP_GPA_RANGE. The subsequent patch here
> handles that in a loop by issuing a new KVM_HC_MAP_GPA_RANGE via the
> completion handler. So we also sort of need to split large requests
> into multiple userspace requests in some cases.
>=20
> It seems like TDX should be able to do something similar by limiting
> the size of each KVM_HC_MAP_GPA_RANGE to TDX_MAP_GPA_MAX_LEN, and then
> returning TDG_VP_VMCALL_RETRY to guest if the original size was
> greater than TDX_MAP_GPA_MAX_LEN. But at that point you're
> effectively done with the entire request and can return to guest, so
> it actually seems a little more straightforward than the SNP case
> above. E.g. TDX has a 1:1 mapping between TDG_VP_VMCALL_MAP_GPA and
> KVM_HC_MAP_GPA_RANGE events. (And even similar names :))
>=20
> So doesn't seem like there's a good reason to expose any of these
> throttling details to userspace, in which case existing
> KVM_HC_MAP_GPA_RANGE interface seems like it should be sufficient.
>=20

Is there any rough data about the latency of private-shared and
shared-private page conversion?

Thanks,
Zhi.=20
> -Mike
>=20
> >=20
> >=20
> > >=20
> > > > For TDX, it may also want to use KVM_HC_MAP_GPA_RANGE hypercall
> > > >  to userspace via KVM_EXIT_HYPERCALL.
> > > Yes, definitely.
> > >=20
> > > Paolo
> > >=20
> >=20
>=20