Re: [PATCH RFC v7 02/64] KVM: x86: Add KVM_CAP_UNMAPPED_PRIVATE_MEMORY

[Jarkko Sakkinen]

On Wed, Jan 04, 2023 at 11:47:21AM -0600, Michael Roth wrote:
> On Thu, Dec 22, 2022 at 01:26:25PM +0100, Borislav Petkov wrote:
> > On Wed, Dec 14, 2022 at 01:39:54PM -0600, Michael Roth wrote:
> > > This mainly indicates to KVM that it should expect all private guest
> > > memory to be backed by private memslots. Ideally this would work
> > > similarly for others archs, give or take a few additional flags, but
> > > for now it's a simple boolean indicator for x86.
> > 
> > ...
> > 
> > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > index c7e9d375a902..cc9424ccf9b2 100644
> > > --- a/include/uapi/linux/kvm.h
> > > +++ b/include/uapi/linux/kvm.h
> > > @@ -1219,6 +1219,7 @@ struct kvm_ppc_resize_hpt {
> > >  #define KVM_CAP_DIRTY_LOG_RING_ACQ_REL 223
> > >  #define KVM_CAP_S390_PROTECTED_ASYNC_DISABLE 224
> > >  #define KVM_CAP_MEMORY_ATTRIBUTES 225
> > > +#define KVM_CAP_UNMAPPED_PRIVATE_MEM 240
> > 
> > Isn't this new cap supposed to be documented somewhere in
> > Documentation/virt/kvm/api.rst ?
> 
> It should, but this is sort of a placeholder for now. Ideally we'd
> re-use the capabilities introduced by UPM patchset rather than introduce
> a new one. Originally the UPM patchset had a KVM_CAP_PRIVATE_MEM which
> we planned to use to switch between legacy SEV and UPM-based SEV (for
> lazy-pinning support) by making it writeable, but that was removed in v10
> in favor of KVM_CAP_MEMORY_ATTRIBUTES, which is tied to the new
> KVM_GET_SUPPORTED_MEMORY_ATTRIBUTES/KVM_SET_MEMORY_ATTRIBUTES ioctls:
> 
>   https://lore.kernel.org/lkml/CA+EHjTxXOdzcP25F57Mtmnb1NWyG5DcyqeDPqzjEOzRUrqH8FQ@xxxxxxxxxxxxxx/
> 
> It wasn't clear at the time if that was the right interface to use for
> this particular case, so we stuck with the more general
> 'use-upm/dont-use-upm' semantics originally provided by making
> KVM_CAP_UNMAPPED_PRIVATE_MEM/KVM_CAP_PRIVATE_MEM writeable.
> 
> But maybe it's okay to just make KVM_CAP_MEMORY_ATTRIBUTES writeable and
> require userspace to negotiate it rather than just tying it to
> CONFIG_HAVE_KVM_MEMORY_ATTRIBUTES. Or maybe introducing a new
> KVM_SET_SUPPORTED_MEMORY_ATTRIBUTES ioctl to pair with
> KVM_GET_SUPPORTED_MEMORY_ATTRIBUTES. It sort of makes sense, since userspace
> needs to be prepared to deal with KVM_EXIT_MEMORY_FAULTs relating to these
> attributes.

Doesn't upm patch set imply that user space should negotiate the memory
attributes with the ioctl? 

For me it looks like that the problem is introduced by conflicting usage
pattern in the SNP code [*].

Perhaps sev_launch_update_gfn_handler() should not set memory attributes
but instead expect user space to do it before the call?

[*] https://lore.kernel.org/all/Y8cydYUfTUFwCh4K@xxxxxxxxxx/

BR, Jarkko