diff mbox series

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

Message ID 20221214194056.161492-3-michael.roth@amd.com
State New
Headers
Series Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support |

Commit Message

Michael Roth Dec. 14, 2022, 7:39 p.m. UTC 
  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.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 10 ++++++++++
 include/uapi/linux/kvm.h        |  1 +
 3 files changed, 14 insertions(+)

Comments

Borislav Petkov Dec. 22, 2022, 12:26 p.m. UTC | #1 
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 ?
Jarkko Sakkinen Jan. 4, 2023, 12:03 p.m. UTC | #2 
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.
> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 +++
>  arch/x86/kvm/x86.c              | 10 ++++++++++
>  include/uapi/linux/kvm.h        |  1 +
>  3 files changed, 14 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 27ef31133352..2b6244525107 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1438,6 +1438,9 @@ struct kvm_arch {
>  	 */
>  #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
>  	struct kvm_mmu_memory_cache split_desc_cache;
> +
> +	/* Use/enforce unmapped private memory. */
> +	bool upm_mode;
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c67e22f3e2ee..99ecf99bc4d2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4421,6 +4421,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_EXIT_HYPERCALL:
>  		r = KVM_EXIT_HYPERCALL_VALID_MASK;
>  		break;
> +#ifdef CONFIG_HAVE_KVM_MEMORY_ATTRIBUTES
> +	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
> +		r = 1;
> +		break;
> +#endif
>  	case KVM_CAP_SET_GUEST_DEBUG2:
>  		return KVM_GUESTDBG_VALID_MASK;
>  #ifdef CONFIG_KVM_XEN
> @@ -6382,6 +6387,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		}
>  		mutex_unlock(&kvm->lock);
>  		break;
> +	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
> +		kvm->arch.upm_mode = true;
> +		r = 0;
> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;
> @@ -12128,6 +12137,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.default_tsc_khz = max_tsc_khz ? : tsc_khz;
>  	kvm->arch.guest_can_read_msr_platform_info = true;
>  	kvm->arch.enable_pmu = enable_pmu;
> +	kvm->arch.upm_mode = false;
>  
>  #if IS_ENABLED(CONFIG_HYPERV)
>  	spin_lock_init(&kvm->arch.hv_root_tdp_lock);
> 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
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.25.1
> 

Why we want to carry non-UPM support still?

BR, Jarkko
Michael Roth Jan. 4, 2023, 5:47 p.m. UTC | #3 
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@mail.gmail.com/

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.

-Mike

> 
> -- 
> 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%7Cb019ddcb34a74fae1e3e08dae417c322%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638073087997837943%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=QRyV96y3drt%2BqwxfifWzJ6UF6te8NOKWAFuGAYOdKcg%3D&reserved=0
Michael Roth Jan. 4, 2023, 5:56 p.m. UTC | #4 
On Wed, Jan 04, 2023 at 12:03:44PM +0000, Jarkko Sakkinen 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.
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  3 +++
> >  arch/x86/kvm/x86.c              | 10 ++++++++++
> >  include/uapi/linux/kvm.h        |  1 +
> >  3 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 27ef31133352..2b6244525107 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1438,6 +1438,9 @@ struct kvm_arch {
> >  	 */
> >  #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
> >  	struct kvm_mmu_memory_cache split_desc_cache;
> > +
> > +	/* Use/enforce unmapped private memory. */
> > +	bool upm_mode;
> >  };
> >  
> >  struct kvm_vm_stat {
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index c67e22f3e2ee..99ecf99bc4d2 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4421,6 +4421,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  	case KVM_CAP_EXIT_HYPERCALL:
> >  		r = KVM_EXIT_HYPERCALL_VALID_MASK;
> >  		break;
> > +#ifdef CONFIG_HAVE_KVM_MEMORY_ATTRIBUTES
> > +	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
> > +		r = 1;
> > +		break;
> > +#endif
> >  	case KVM_CAP_SET_GUEST_DEBUG2:
> >  		return KVM_GUESTDBG_VALID_MASK;
> >  #ifdef CONFIG_KVM_XEN
> > @@ -6382,6 +6387,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> >  		}
> >  		mutex_unlock(&kvm->lock);
> >  		break;
> > +	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
> > +		kvm->arch.upm_mode = true;
> > +		r = 0;
> > +		break;
> >  	default:
> >  		r = -EINVAL;
> >  		break;
> > @@ -12128,6 +12137,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >  	kvm->arch.default_tsc_khz = max_tsc_khz ? : tsc_khz;
> >  	kvm->arch.guest_can_read_msr_platform_info = true;
> >  	kvm->arch.enable_pmu = enable_pmu;
> > +	kvm->arch.upm_mode = false;
> >  
> >  #if IS_ENABLED(CONFIG_HYPERV)
> >  	spin_lock_init(&kvm->arch.hv_root_tdp_lock);
> > 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
> >  
> >  #ifdef KVM_CAP_IRQ_ROUTING
> >  
> > -- 
> > 2.25.1
> > 
> 
> Why we want to carry non-UPM support still?

For SNP, non-UPM support is no longer included in this patchset.

However, this patchset also adds support for UPM-based SEV (for lazy-pinning
support). So we still need a way to let userspace switch between those 2
modes.

-Mike

> 
> BR, Jarkko
>
Borislav Petkov Jan. 5, 2023, 11:57 a.m. UTC | #5 
On Wed, Jan 04, 2023 at 11:47:21AM -0600, Michael Roth wrote:
> 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.

Makes sense.

AFAICT, ofc.
Jarkko Sakkinen Jan. 19, 2023, 1:03 p.m. UTC | #6 
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@mail.gmail.com/
> 
> 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@kernel.org/

BR, Jarkko
diff mbox series

Patch

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 27ef31133352..2b6244525107 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1438,6 +1438,9 @@  struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/* Use/enforce unmapped private memory. */
+	bool upm_mode;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c67e22f3e2ee..99ecf99bc4d2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4421,6 +4421,11 @@  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_EXIT_HYPERCALL:
 		r = KVM_EXIT_HYPERCALL_VALID_MASK;
 		break;
+#ifdef CONFIG_HAVE_KVM_MEMORY_ATTRIBUTES
+	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
+		r = 1;
+		break;
+#endif
 	case KVM_CAP_SET_GUEST_DEBUG2:
 		return KVM_GUESTDBG_VALID_MASK;
 #ifdef CONFIG_KVM_XEN
@@ -6382,6 +6387,10 @@  int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		}
 		mutex_unlock(&kvm->lock);
 		break;
+	case KVM_CAP_UNMAPPED_PRIVATE_MEM:
+		kvm->arch.upm_mode = true;
+		r = 0;
+		break;
 	default:
 		r = -EINVAL;
 		break;
@@ -12128,6 +12137,7 @@  int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.default_tsc_khz = max_tsc_khz ? : tsc_khz;
 	kvm->arch.guest_can_read_msr_platform_info = true;
 	kvm->arch.enable_pmu = enable_pmu;
+	kvm->arch.upm_mode = false;
 
 #if IS_ENABLED(CONFIG_HYPERV)
 	spin_lock_init(&kvm->arch.hv_root_tdp_lock);
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
 
 #ifdef KVM_CAP_IRQ_ROUTING