From: Michael Roth <michael.roth@amd.com>
To: Sean Christopherson <seanjc@google.com>
Cc: <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>,
<pbonzini@redhat.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>
Subject: Re: [PATCH v14 09/22] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT
Date: Fri, 26 Apr 2024 17:48:26 -0500 [thread overview]
Message-ID: <20240426224826.q53obbzjzhp6lrme@amd.com> (raw)
In-Reply-To: <20240426222457.7yn66athor2jxsrj@amd.com>
On Fri, Apr 26, 2024 at 05:24:57PM -0500, Michael Roth wrote:
> On Fri, Apr 26, 2024 at 01:14:32PM -0700, Sean Christopherson wrote:
> > On Fri, Apr 26, 2024, Michael Roth wrote:
> > > On Thu, Apr 25, 2024 at 03:13:40PM -0700, Sean Christopherson wrote:
> > > > On Thu, Apr 25, 2024, Michael Roth wrote:
> > > > > On Wed, Apr 24, 2024 at 01:59:48PM -0700, Sean Christopherson wrote:
> > > > > > On Sun, Apr 21, 2024, Michael Roth wrote:
> > > > > > > +static int snp_begin_psc_msr(struct kvm_vcpu *vcpu, u64 ghcb_msr)
> > > > > > > +{
> > > > > > > + u64 gpa = gfn_to_gpa(GHCB_MSR_PSC_REQ_TO_GFN(ghcb_msr));
> > > > > > > + u8 op = GHCB_MSR_PSC_REQ_TO_OP(ghcb_msr);
> > > > > > > + struct vcpu_svm *svm = to_svm(vcpu);
> > > > > > > +
> > > > > > > + if (op != SNP_PAGE_STATE_PRIVATE && op != SNP_PAGE_STATE_SHARED) {
> > > > > > > + set_ghcb_msr(svm, GHCB_MSR_PSC_RESP_ERROR);
> > > > > > > + return 1; /* resume guest */
> > > > > > > + }
> > > > > > > +
> > > > > > > + vcpu->run->exit_reason = KVM_EXIT_VMGEXIT;
> > > > > > > + vcpu->run->vmgexit.type = KVM_USER_VMGEXIT_PSC_MSR;
> > > > > > > + vcpu->run->vmgexit.psc_msr.gpa = gpa;
> > > > > > > + vcpu->run->vmgexit.psc_msr.op = op;
> > > > > >
> > > > > > Argh, no.
> > > > > >
> > > > > > This is the same crud that TDX tried to push[*]. Use KVM's existing user exits,
> > > > > > and extend as *needed*. There is no good reason page state change requests need
> > > > > > *two* exit reasons. The *only* thing KVM supports right now is private<=>shared
> > > > > > conversions, and that can be handled with either KVM_HC_MAP_GPA_RANGE or
> > > > > > KVM_EXIT_MEMORY_FAULT.
> > > > > >
> > > > > > The non-MSR flavor can batch requests, but I'm willing to bet that the overwhelming
> > > > > > majority of requests are contiguous, i.e. can be combined into a range by KVM,
> > > > > > and that handling any outliers by performing multiple exits to userspace will
> > > > > > provide sufficient performance.
> > > > >
> > > > > That does tend to be the case. We won't have as much granularity with
> > > > > the per-entry error codes, but KVM_SET_MEMORY_ATTRIBUTES would be
> > > > > expected to be for the entire range anyway, and if that fails for
> > > > > whatever reason then we KVM_BUG_ON() anyway. We do have to have handling
> > > > > for cases where the entries aren't contiguous however, which would
> > > > > involve multiple KVM_EXIT_HYPERCALLs until everything is satisfied. But
> > > > > not a huge deal since it doesn't seem to be a common case.
> > > >
> > > > If it was less complex overall, I wouldn't be opposed to KVM marshalling everything
> > > > into a buffer, but I suspect it will be simpler to just have KVM loop until the
> > > > PSC request is complete.
> > >
> > > Agreed. But *if* we decided to introduce a buffer, where would you
> > > suggest adding it? The kvm_run union fields are set to 256 bytes, and
> > > we'd need close to 4K to handle a full GHCB PSC buffer in 1 go. Would
> > > additional storage at the end of struct kvm_run be acceptable?
> >
> > Don't even need more memory, just use vcpu->arch.pio_data, which is always
> > allocated and is mmap()able by userspace via KVM_PIO_PAGE_OFFSET.
>
> Nice, that seems like a good option if needed.
>
> >
> > > > > KVM_HC_MAP_GPA_RANGE seems like a nice option because we'd also have the
> > > > > flexibility to just issue that directly within a guest rather than
> > > > > relying on SNP/TDX specific hcalls. I don't know if that approach is
> > > > > practical for a real guest, but it could be useful for having re-usable
> > > > > guest code in KVM selftests that "just works" for all variants of
> > > > > SNP/TDX/sw-protected. (though we'd still want stuff that exercises
> > > > > SNP/TDX->KVM_HC_MAP_GPA_RANGE translation).
> > > > >
> > > > > I think we'd there is some potential baggage there with the previous SEV
> > > > > live migration use cases. There's some potential that existing guest kernels
> > > > > will use it once it gets advertised and issue them alongside GHCB-based
> > > > > page-state changes. It might make sense to use one of the reserved bits
> > > > > to denote this flavor of KVM_HC_MAP_GPA_RANGE as being for
> > > > > hardware/software-protected VMs and not interchangeable with calls that
> > > > > were used for SEV live migration stuff.
> > > >
> > > > I don't think I follow, what exactly wouldn't be interchangeable, and why?
> > >
> > > For instance, if KVM_FEATURE_MIGRATION_CONTROL is advertised, then when
> > > amd_enc_status_change_finish() is triggered as a result of
> > > set_memory_encrypted(), we'd see
> > >
> > > 1) a GHCB PSC for SNP, which will get forwarded to userspace via
> > > KVM_HC_MAP_GPA_RANGE
> > > 2) KVM_HC_MAP_GPA_RANGE issued directly by the guest.
> > >
> > > In that case, we'd be duplicating PSCs but it wouldn't necessarily hurt
> > > anything. But ideally we'd be able to distinguish the 2 cases so we
> > > could rightly treat 1) as only being expected for SNP, and 2) as only
> > > being expected for SEV/SEV-ES.
> >
> > Why would the guest issue both? That's a guest bug. Or if supressing the second
> > hypercall is an issue, simply don't enumerate MIGRATION_CONTROL for SNP guests.
>
> At the time of its inception, KVM_HC_MAP_GPA_RANGE was simply
> KVM_HC_PAGE_ENC_STATUS and got a more generic name over the course of
> development. But its purpose never changed: to inform the hypervisor of
> the current encryption status of a GPA range so VMMs could build up a
> list of shared guest regions that don't need to go through firmware for
> migration.. And it was and still is asynchronous to a degree, since the
> the migration control MSRs signals when that list of shared pages is
> usable.
>
> These are very different semantics the proposal to use KVM_HC_MAP_GPA_RANGE
> as a means to set memory attributes via KVM_SET_MEMORY_ATTRIBUTES, and
> the 2 purposes aren't necessarily mutually exclusive to one another. It
> only really becomes a bug if we begin to interpret the original use-case
> as something other than it's initial intent in the case of SNP.
>
> But at the same time, it's hard to imagine this older SEV live migration
> use-case being useful for SNP, since userspace will necessarily have all
> the information it needs to determine what is/isn't shared with relying
> on an additional hypercall.
>
> So treating the older use case as specific to non-SNP and disallowing the
> use of MIGRATION_CONTROL does seems reasonable. But it's really the CPUID
> bit that advertises it, SEV just happens to only use it for when
> MIGRATION_CONTROL is also advertised. So we could disable that as well,
> but I did like the idea of being able to handle guest-issued
> KVM_HC_MAP_GPA_RANGE calls even with SNP/TDX enabled, which is less of an
> option if we can't advertised KVM_HC_MAP_GPA_RANGE via cpuid. But I
> suppose we could do that with KVM selftests which is probably where
> that's more likely to be useful.
Hmm, well...assuming SNP/TDX guest agree to make those vCPU registers
available via VMSA/etc in those cases... So i suppose we'd need some
additional handling to support advertising KVM_HC_MAP_GPA_RANGE via cpuid
either way and it is best to disallow guest-issued KVM_HC_MAP_GPA_RANGE
from being advertised to guests until there's support and a solid use-case
for it.
-Mike
>
> -Mike
>
next prev parent reply other threads:[~2024-04-26 22:48 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-21 18:01 [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-21 18:01 ` [PATCH v14 01/22] KVM: SEV: Select KVM_GENERIC_PRIVATE_MEM when CONFIG_KVM_AMD_SEV=y Michael Roth
2024-04-21 18:01 ` [PATCH v14 02/22] KVM: SEV: Add support to handle AP reset MSR protocol Michael Roth
2024-04-21 18:01 ` [PATCH v14 03/22] KVM: SEV: Add GHCB handling for Hypervisor Feature Support requests Michael Roth
2024-04-24 20:21 ` Sean Christopherson
2024-04-25 20:52 ` Michael Roth
2024-04-25 21:55 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 04/22] KVM: SEV: Add initial SEV-SNP support Michael Roth
2024-04-21 18:01 ` [PATCH v14 05/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command Michael Roth
2024-04-24 23:26 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 06/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command Michael Roth
2024-04-24 23:58 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 07/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_FINISH command Michael Roth
2024-04-21 18:01 ` [PATCH v14 08/22] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-21 18:01 ` [PATCH v14 09/22] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-24 20:59 ` Sean Christopherson
2024-04-25 22:00 ` Michael Roth
2024-04-25 22:13 ` Sean Christopherson
2024-04-26 17:16 ` Michael Roth
2024-04-26 20:14 ` Sean Christopherson
2024-04-26 22:24 ` Michael Roth
2024-04-26 22:48 ` Michael Roth [this message]
2024-04-21 18:01 ` [PATCH v14 10/22] KVM: SEV: Add support to handle " Michael Roth
2024-04-21 18:01 ` [PATCH v14 11/22] KVM: SEV: Add support to handle RMP nested page faults Michael Roth
2024-04-21 18:01 ` [PATCH v14 12/22] KVM: SEV: Support SEV-SNP AP Creation NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 13/22] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-21 18:01 ` [PATCH v14 14/22] KVM: SEV: Implement gmem hook for initializing private pages Michael Roth
2024-04-21 18:01 ` [PATCH v14 15/22] KVM: SEV: Implement gmem hook for invalidating " Michael Roth
2024-04-21 18:01 ` [PATCH v14 16/22] KVM: x86: Implement gmem hook for determining max NPT mapping level Michael Roth
2024-04-25 0:45 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 17/22] KVM: SEV: Avoid WBINVD for HVA-based MMU notifications for SNP Michael Roth
2024-04-25 0:17 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 18/22] KVM: SVM: Add module parameter to enable SEV-SNP Michael Roth
2024-04-21 18:01 ` [PATCH v14 19/22] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 20/22] crypto: ccp: Add the SNP_VLEK_LOAD command Michael Roth
2024-04-21 18:01 ` [PATCH v14 21/22] crypto: ccp: Add the SNP_{PAUSE,RESUME}_ATTESTATION commands Michael Roth
2024-04-25 0:15 ` Sean Christopherson
2024-04-26 17:35 ` Michael Roth
2024-04-26 19:57 ` Sean Christopherson
2024-04-26 21:46 ` Michael Roth
2024-04-27 0:10 ` Sean Christopherson
2024-04-27 1:32 ` Michael Roth
2024-04-29 14:27 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 22/22] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event Michael Roth
2024-04-25 0:10 ` Sean Christopherson
2024-04-26 17:57 ` Michael Roth
2024-04-23 16:21 ` [PATCH v14 23/22] [SQUASH] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 24/22] [SQUASH] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 25/22] [SQUASH] KVM: SEV: Add support to handle " Michael Roth
2024-04-23 16:21 ` [PATCH v14 26/22] [SQUASH] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-23 16:21 ` [PATCH v14 27/22] [SQUASH] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-23 16:21 ` [PATCH v14 28/22] [SQUASH] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST " Michael Roth
2024-04-23 21:36 ` Jarkko Sakkinen
2024-04-23 16:21 ` [PATCH v14 29/22] [SQUASH] KVM: SEV: Support SEV-SNP AP Creation " Michael Roth
2024-04-23 16:31 ` [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-24 16:51 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240426224826.q53obbzjzhp6lrme@amd.com \
--to=michael.roth@amd.com \
--cc=ak@linux.intel.com \
--cc=alpergun@google.com \
--cc=ardb@kernel.org \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=dave.hansen@linux.intel.com \
--cc=dovmurik@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=jmattson@google.com \
--cc=jroedel@suse.de \
--cc=kirill@shutemov.name \
--cc=kvm@vger.kernel.org \
--cc=liam.merwick@oracle.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=nikunj.dadhania@amd.com \
--cc=pankaj.gupta@amd.com \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=pgonda@google.com \
--cc=rientjes@google.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=slp@redhat.com \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=tobin@ibm.com \
--cc=tony.luck@intel.com \
--cc=vbabka@suse.cz \
--cc=vkuznets@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.