Message-ID: <20230105181741.q6mq37gvcjk5nbjg@amd.com>
Date:   Thu, 5 Jan 2023 12:17:41 -0600
From:   Michael Roth <michael.roth@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     <kvm@...r.kernel.org>, <linux-coco@...ts.linux.dev>,
        <linux-mm@...ck.org>, <linux-crypto@...r.kernel.org>,
        <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <jroedel@...e.de>,
        <thomas.lendacky@....com>, <hpa@...or.com>, <ardb@...nel.org>,
        <pbonzini@...hat.com>, <seanjc@...gle.com>, <vkuznets@...hat.com>,
        <wanpengli@...cent.com>, <jmattson@...gle.com>, <luto@...nel.org>,
        <dave.hansen@...ux.intel.com>, <slp@...hat.com>,
        <pgonda@...gle.com>, <peterz@...radead.org>,
        <srinivas.pandruvada@...ux.intel.com>, <rientjes@...gle.com>,
        <dovmurik@...ux.ibm.com>, <tobin@....com>, <vbabka@...e.cz>,
        <kirill@...temov.name>, <ak@...ux.intel.com>,
        <tony.luck@...el.com>, <marcorr@...gle.com>,
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        <alpergun@...gle.com>, <dgilbert@...hat.com>, <jarkko@...nel.org>,
        <ashish.kalra@....com>, <harald@...fian.com>,
        Nikunj A Dadhania <nikunj@....com>,
        <chao.p.peng@...ux.intel.com>
Subject: Re: [PATCH RFC v7 03/64] KVM: SVM: Advertise private memory support
 to KVM

On Thu, Jan 05, 2023 at 04:04:51PM +0100, Borislav Petkov wrote:
> On Wed, Jan 04, 2023 at 08:14:19PM -0600, Michael Roth wrote:
> > Maybe that's not actually enforced, by it seems awkward to try to use a
> > bool return instead. At least for KVM_X86_OP_OPTIONAL_RET0().
> 
> I don't see there being a problem/restriction for bool functions, see
> 
> 5be2226f417d ("KVM: x86: allow defining return-0 static calls")
> 
> and __static_call_return0() returns a long which, if you wanna interpret as
> bool, works too as "false".
> 
> I still need to disassemble and single-step through a static_call to see what
> all that magic does in detail, to be sure.

Hmm, yah, looking at that patch it seems pretty clear (at least at the
time) that bool returns are expected to work fine and there are still
existing cases that do things that way.

> 
> > However, we could just use KVM_X86_OP() to declare it so we can cleanly
> > use a function that returns bool, and then we just need to do:
> > 
> >   bool kvm_arch_has_private_mem(struct kvm *kvm)
> >   {
> >           if (kvm_x86_ops.private_mem_enabled)
> >                   return static_call(kvm_x86_private_mem_enabled)(kvm);
> 
> That would be defeating the whole purpose of static calls, AFAICT, as you're
> testing the pointer. Might as well leave it be a normal function pointer then.

That's true, we should just use the function pointer for those cases.
But given the above maybe KVM_X86_OP_OPTIONAL_RET0() is fine after all
so we can continue using static_call().

> 
> > On a separate topic though, at a high level, this hook is basically a way
> > for platform-specific code to tell generic KVM code that private memslots
> > are supported by overriding the kvm_arch_has_private_mem() weak
> > reference. In this case the AMD platform is using using kvm->arch.upm_mode
> > flag to convey that, which is in turn set by the
> > KVM_CAP_UNMAPPED_PRIVATE_MEMORY introduced in this series.
> > 
> > But if, as I suggested in response to your PATCH 2 comments, we drop
> > KVM_CAP_UNAMMPED_PRIVATE_MEMORY in favor of
> > KVM_SET_SUPPORTED_MEMORY_ATTRIBUTES ioctl to enable "UPM mode" in SEV/SNP
> > code, then we need to rethink things a bit, since KVM_SET_MEMORY_ATTRIBUTES
> > in-part relies on kvm_arch_has_private_mem() to determine what flags are
> > supported, whereas SEV/SNP code would be using what was set by
> > KVM_SET_MEMORY_ATTRIBUTES to determine the return value in
> > kvm_arch_has_private_mem().
> > 
> > So, for AMD, the return value of kvm_arch_has_private_mem() needs to rely
> > on something else. Maybe the logic can just be:
> > 
> >   bool svm_private_mem_enabled(struct kvm *kvm)
> >   {
> >     return sev_enabled(kvm) || sev_snp_enabled(kvm)
> 
> I haven't followed the whole discussion in detail but this means that SEV/SNP
> *means* UPM. I.e., no SEV/SNP without UPM, correct? I guess that's the final
> thing you guys decided to do ...

It's more about reporting whether UPM is *possible*. In the case of
this patchset there is support for using UPM in conjunction with SEV
or SEV-SNP, and that's all the above check is conveying. This is
basically what KVM_SET_SUPPORTED_MEMORY_ATTRIBUTES ioctl will rely on to
determine what attributes it reports to the guest as being supported,
which is basically what tells userspace whether or not it can make use
of UPM features.

In the case of SEV, it would still be up to userspace whether or not it
actually wants to make use of UPM functionality like KVM_SET_MEMORY_ATTRIBUTES
and private memslots. Otherwise, to maintain backward-compatibility, 
userspace can do things as it has always done and continue running SEV without
relying on private memslots/KVM_SET_MEMORY_ATTRIBUTES or any of the new ioctls.

For SNP however it is required that userspace uses/implements UPM
functionality.

-Mike

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=05%7C01%7Cmichael.roth%40amd.com%7C4bfcf0c3d1e54046703008daef2e35d4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638085279100077750%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oCjQx7LV1paTYdMA4JpFjIexf5UkiZEf2pYOWTvpkZ0%3D&reserved=0

Powered by blists - more mailing lists