| Message ID | 20221214194056.161492-3-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
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 ?
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
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
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 >
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.
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 --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
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(+)