This is a follow-up to the RFC implementation [1] that incorporates
review feedback and bug fixes. See the "RFC v1" section below for a
list of changes.
SEV guest requires the guest's pages to be pinned in host physical
memory as migration of encrypted pages is not supported. The memory
encryption scheme uses the physical address of the memory being
encrypted. If guest pages are moved by the host, content decrypted in
the guest would be incorrect thereby corrupting guest's memory.
For SEV/SEV-ES guests, the hypervisor doesn't know which pages are
encrypted and when the guest is done using those pages. Hypervisor
should treat all the guest pages as encrypted until they are
deallocated or the guest is destroyed.
While provision a pfn, make KVM aware that guest pages need to be
pinned for long-term and use appropriate pin_user_pages API for these
special encrypted memory regions. KVM takes the first reference and
holds it until a mapping is done. Take an extra reference before KVM
releases the pfn.
Actual pinning management is handled by vendor code via new
kvm_x86_ops hooks. MMU calls in to vendor code to pin the page on
demand. Metadata of the pinning is stored in architecture specific
memslot area. During the memslot freeing path and deallocation path
guest pages are unpinned.
Guest boot time comparison:
+---------------+----------------+-------------------+
| Guest Memory | baseline | Demand Pinning + |
| Size (GB) | v5.17-rc6(secs)| v5.17-rc6(secs) |
+---------------+----------------+-------------------+
| 4 | 6.16 | 5.71 |
+---------------+----------------+-------------------+
| 16 | 7.38 | 5.91 |
+---------------+----------------+-------------------+
| 64 | 12.17 | 6.16 |
+---------------+----------------+-------------------+
| 128 | 18.20 | 6.50 |
+---------------+----------------+-------------------+
| 192 | 24.56 | 6.80 |
+---------------+----------------+-------------------+
Changelog:
RFC v1:
* Use pin_user_pages API with FOLL_LONGTERM flag for pinning the
encrypted guest pages. [David Hildenbrand]
* Use new api kvm_for_each_memslot_in_hva_range to walk the memslot.
[Maciej S. Szmigiero]
* Maintain the non-mmu pinned memory and free them on destruction.
[Peter Gonda]
* Handle non-mmu pinned memory for intra host migration. [Peter Gonda]
* Add the missing RLIMIT_MEMLOCK check. [David Hildenbrand]
* Use pin_user_pages API for long term pinning of pages.
[David Hildenbrand]
* Flush the page before releasing it to the host system.
[Mingwei Zhang]
[1] https://lore.kernel.org/kvm/[email protected]/
Nikunj A Dadhania (7):
KVM: Introduce pinning flag to hva_to_pfn*
KVM: x86/mmu: Move hugepage adjust to direct_page_fault
KVM: x86/mmu: Add hook to pin PFNs on demand in MMU
KVM: SVM: Add pinning metadata in the arch memslot
KVM: SVM: Implement demand page pinning
KVM: SEV: Carve out routine for allocation of pages
KVM: Move kvm_for_each_memslot_in_hva_range() to be used in SVM
Sean Christopherson (2):
KVM: x86/mmu: Introduce kvm_mmu_map_tdp_page() for use by SEV/TDX
KVM: SVM: Pin SEV pages in MMU during sev_launch_update_data()
arch/x86/include/asm/kvm-x86-ops.h | 3 +
arch/x86/include/asm/kvm_host.h | 10 +
arch/x86/kvm/mmu.h | 3 +
arch/x86/kvm/mmu/mmu.c | 57 +++-
arch/x86/kvm/mmu/tdp_mmu.c | 2 -
arch/x86/kvm/svm/sev.c | 531 ++++++++++++++++++++++-------
arch/x86/kvm/svm/svm.c | 4 +
arch/x86/kvm/svm/svm.h | 12 +-
arch/x86/kvm/x86.c | 11 +-
include/linux/kvm_host.h | 12 +
virt/kvm/kvm_main.c | 69 ++--
virt/kvm/kvm_mm.h | 2 +-
virt/kvm/pfncache.c | 2 +-
13 files changed, 556 insertions(+), 162 deletions(-)
--
2.32.0
AMD SEV guest requires the guest's pages to be pinned in host
physical memory. The memory encryption scheme uses the physical
address of the memory being encrypted. If guest pages are moved,
content decrypted would be incorrect, corrupting guest's memory.
For SEV/SEV-ES guests, the hypervisor doesn't know which pages are
encrypted and when the guest is done using those pages. Hypervisor
should treat all the guest pages as encrypted until they are
deallocated or the guest is destroyed.
The KVM MMU needs to track the pages that are pinned and the
corresponding pfns for unpinning them during the guest destroy path
and deallocation path.
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
arch/x86/include/asm/kvm-x86-ops.h | 2 ++
arch/x86/include/asm/kvm_host.h | 7 +++++
arch/x86/kvm/svm/sev.c | 49 ++++++++++++++++++++++++++++++
arch/x86/kvm/svm/svm.c | 3 ++
arch/x86/kvm/svm/svm.h | 6 ++++
arch/x86/kvm/x86.c | 11 ++++++-
6 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 8efb43d92eef..61ff8a636db6 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -89,6 +89,8 @@ KVM_X86_OP(set_identity_map_addr)
KVM_X86_OP(get_mt_mask)
KVM_X86_OP(load_mmu_pgd)
KVM_X86_OP(pin_pfn)
+KVM_X86_OP(alloc_memslot_metadata)
+KVM_X86_OP(free_memslot)
KVM_X86_OP_NULL(has_wbinvd_exit)
KVM_X86_OP(get_l2_tsc_offset)
KVM_X86_OP(get_l2_tsc_multiplier)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index df11f1fb76de..eeb2c799b59f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -926,6 +926,8 @@ struct kvm_arch_memory_slot {
struct kvm_rmap_head *rmap[KVM_NR_PAGE_SIZES];
struct kvm_lpage_info *lpage_info[KVM_NR_PAGE_SIZES - 1];
unsigned short *gfn_track[KVM_PAGE_TRACK_MAX];
+ unsigned long *pinned_bitmap;
+ kvm_pfn_t *pfns;
};
/*
@@ -1421,6 +1423,11 @@ struct kvm_x86_ops {
bool (*pin_pfn)(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot,
kvm_pfn_t pfn, hva_t hva, bool write,
enum pg_level level);
+ int (*alloc_memslot_metadata)(struct kvm *kvm,
+ const struct kvm_memory_slot *old,
+ struct kvm_memory_slot *new);
+ void (*free_memslot)(struct kvm *kvm,
+ struct kvm_memory_slot *slot);
bool (*has_wbinvd_exit)(void);
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 17b53457d866..bd7572517c99 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2950,3 +2950,52 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 1);
}
+
+void sev_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+ struct kvm_arch_memory_slot *aslot = &slot->arch;
+
+ if (!sev_guest(kvm))
+ return;
+
+ if (aslot->pinned_bitmap) {
+ kvfree(aslot->pinned_bitmap);
+ aslot->pinned_bitmap = NULL;
+ }
+
+ if (aslot->pfns) {
+ kvfree(aslot->pfns);
+ aslot->pfns = NULL;
+ }
+}
+
+int sev_alloc_memslot_metadata(struct kvm *kvm,
+ const struct kvm_memory_slot *old,
+ struct kvm_memory_slot *new)
+{
+ struct kvm_arch_memory_slot *aslot = &new->arch;
+ unsigned long pinned_bytes = new->npages * sizeof(kvm_pfn_t);
+
+ if (!sev_guest(kvm))
+ return 0;
+
+ if (old && old->arch.pinned_bitmap && old->arch.pfns) {
+ WARN_ON(old->npages != new->npages);
+ aslot->pinned_bitmap = old->arch.pinned_bitmap;
+ aslot->pfns = old->arch.pfns;
+ return 0;
+ }
+
+ aslot->pfns = kvcalloc(new->npages, sizeof(*aslot->pfns),
+ GFP_KERNEL_ACCOUNT);
+ if (!aslot->pfns)
+ return -ENOMEM;
+
+ aslot->pinned_bitmap = kvzalloc(pinned_bytes, GFP_KERNEL_ACCOUNT);
+ if (!aslot->pinned_bitmap) {
+ kvfree(aslot->pfns);
+ aslot->pfns = NULL;
+ return -ENOMEM;
+ }
+ return 0;
+}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index fd3a00c892c7..ec06421cb532 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4658,6 +4658,9 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
.complete_emulated_msr = svm_complete_emulated_msr,
.vcpu_deliver_sipi_vector = svm_vcpu_deliver_sipi_vector,
+
+ .alloc_memslot_metadata = sev_alloc_memslot_metadata,
+ .free_memslot = sev_free_memslot,
};
/*
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index fa98d6844728..f00364020d7e 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -616,4 +616,10 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm);
void __svm_sev_es_vcpu_run(unsigned long vmcb_pa);
void __svm_vcpu_run(unsigned long vmcb_pa, unsigned long *regs);
+int sev_alloc_memslot_metadata(struct kvm *kvm,
+ const struct kvm_memory_slot *old,
+ struct kvm_memory_slot *new);
+void sev_free_memslot(struct kvm *kvm,
+ struct kvm_memory_slot *slot);
+
#endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 82a9dcd8c67f..95070aaa1636 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11796,6 +11796,7 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
}
kvm_page_track_free_memslot(slot);
+ static_call_cond(kvm_x86_free_memslot)(kvm, slot);
}
int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages)
@@ -11821,6 +11822,7 @@ int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages)
}
static int kvm_alloc_memslot_metadata(struct kvm *kvm,
+ const struct kvm_memory_slot *old,
struct kvm_memory_slot *slot)
{
unsigned long npages = slot->npages;
@@ -11873,8 +11875,15 @@ static int kvm_alloc_memslot_metadata(struct kvm *kvm,
if (kvm_page_track_create_memslot(kvm, slot, npages))
goto out_free;
+ if (kvm_x86_ops.alloc_memslot_metadata &&
+ static_call(kvm_x86_alloc_memslot_metadata)(kvm, old, slot))
+ goto out_free_page_track;
+
return 0;
+out_free_page_track:
+ kvm_page_track_free_memslot(slot);
+
out_free:
memslot_rmap_free(slot);
@@ -11907,7 +11916,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
enum kvm_mr_change change)
{
if (change == KVM_MR_CREATE || change == KVM_MR_MOVE)
- return kvm_alloc_memslot_metadata(kvm, new);
+ return kvm_alloc_memslot_metadata(kvm, old, new);
if (change == KVM_MR_FLAGS_ONLY)
memcpy(&new->arch, &old->arch, sizeof(old->arch));
--
2.32.0
Use vendor code via kvm_x86_ops hooks for pinning.
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
arch/x86/include/asm/kvm-x86-ops.h | 1 +
arch/x86/include/asm/kvm_host.h | 3 +++
arch/x86/kvm/mmu/mmu.c | 15 +++++++++++++++
3 files changed, 19 insertions(+)
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index d39e0de06be2..8efb43d92eef 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -88,6 +88,7 @@ KVM_X86_OP(set_tss_addr)
KVM_X86_OP(set_identity_map_addr)
KVM_X86_OP(get_mt_mask)
KVM_X86_OP(load_mmu_pgd)
+KVM_X86_OP(pin_pfn)
KVM_X86_OP_NULL(has_wbinvd_exit)
KVM_X86_OP(get_l2_tsc_offset)
KVM_X86_OP(get_l2_tsc_multiplier)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ec9830d2aabf..df11f1fb76de 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1418,6 +1418,9 @@ struct kvm_x86_ops {
void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
int root_level);
+ bool (*pin_pfn)(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot,
+ kvm_pfn_t pfn, hva_t hva, bool write,
+ enum pg_level level);
bool (*has_wbinvd_exit)(void);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index db1feecd6fed..b94e5e71653e 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4001,6 +4001,16 @@ static bool is_page_fault_stale(struct kvm_vcpu *vcpu,
mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva);
}
+static bool kvm_pin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
+{
+ if (is_error_noslot_pfn(fault->pfn) || kvm_is_reserved_pfn(fault->pfn) ||
+ !kvm_x86_ops.pin_pfn)
+ return true;
+
+ return kvm_x86_ops.pin_pfn(vcpu, fault->slot, fault->pfn, fault->hva,
+ fault->write, fault->goal_level);
+}
+
static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
{
bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu);
@@ -4035,6 +4045,9 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
kvm_mmu_hugepage_adjust(vcpu, fault);
+ if (memslot_is_encrypted(fault->slot) && !kvm_pin_pfn(vcpu, fault))
+ goto out_release;
+
if (is_tdp_mmu_fault)
read_lock(&vcpu->kvm->mmu_lock);
else
@@ -4057,6 +4070,8 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
read_unlock(&vcpu->kvm->mmu_lock);
else
write_unlock(&vcpu->kvm->mmu_lock);
+
+out_release:
kvm_release_pfn_clean(fault->pfn);
return r;
}
--
2.32.0
Move the macro to kvm_host.h and make if visible for SVM to use.
No functional change intended.
Suggested-by: Maciej S. Szmigiero <[email protected]>
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
include/linux/kvm_host.h | 6 ++++++
virt/kvm/kvm_main.c | 6 ------
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c23022960d51..d72f692725d2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1063,6 +1063,12 @@ static inline bool kvm_memslot_iter_is_valid(struct kvm_memslot_iter *iter, gfn_
kvm_memslot_iter_is_valid(iter, end); \
kvm_memslot_iter_next(iter))
+/* Iterate over each memslot intersecting [start, last] (inclusive) range */
+#define kvm_for_each_memslot_in_hva_range(node, slots, start, last) \
+ for (node = interval_tree_iter_first(&slots->hva_tree, start, last); \
+ node; \
+ node = interval_tree_iter_next(node, start, last))
+
/*
* KVM_SET_USER_MEMORY_REGION ioctl allows the following operations:
* - create a new memory slot
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c035fe6b39ec..ff890f41c7ce 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -511,12 +511,6 @@ static void kvm_null_fn(void)
}
#define IS_KVM_NULL_FN(fn) ((fn) == (void *)kvm_null_fn)
-/* Iterate over each memslot intersecting [start, last] (inclusive) range */
-#define kvm_for_each_memslot_in_hva_range(node, slots, start, last) \
- for (node = interval_tree_iter_first(&slots->hva_tree, start, last); \
- node; \
- node = interval_tree_iter_next(node, start, last)) \
-
static __always_inline int __kvm_handle_hva_range(struct kvm *kvm,
const struct kvm_hva_range *range)
{
--
2.32.0
Both TDP MMU and legacy MMU do hugepage adjust in the mapping routine.
Adjust the pfn early in the common code. This will be used by the
following patches for pinning the pages.
No functional change intended.
Signed-off-by: Nikunj A Dadhania <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 4 ++--
arch/x86/kvm/mmu/tdp_mmu.c | 2 --
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 8e24f73bf60b..db1feecd6fed 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2940,8 +2940,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
int ret;
gfn_t base_gfn = fault->gfn;
- kvm_mmu_hugepage_adjust(vcpu, fault);
-
trace_kvm_mmu_spte_requested(fault);
for_each_shadow_entry(vcpu, fault->addr, it) {
/*
@@ -4035,6 +4033,8 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
r = RET_PF_RETRY;
+ kvm_mmu_hugepage_adjust(vcpu, fault);
+
if (is_tdp_mmu_fault)
read_lock(&vcpu->kvm->mmu_lock);
else
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index bc9e3553fba2..e03bf59b2f81 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -959,8 +959,6 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
u64 new_spte;
int ret;
- kvm_mmu_hugepage_adjust(vcpu, fault);
-
trace_kvm_mmu_spte_requested(fault);
rcu_read_lock();
--
2.32.0
On Tue, Mar 08, 2022, Nikunj A Dadhania wrote:
> Both TDP MMU and legacy MMU do hugepage adjust in the mapping routine.
> Adjust the pfn early in the common code. This will be used by the
> following patches for pinning the pages.
>
> No functional change intended.
There is a functional change here, as kvm_mmu_hugepage_adjust() is now called
without mmu_lock being held. That really shouldn't be problematic, but sadly KVM
very, very subtly relies on calling lookup_address_in_mm() while holding mmu_lock
_and_ after checking mmu_notifier_retry_hva().
https://lore.kernel.org/all/CAL715WL7ejOBjzXy9vbS_M2LmvXcC-CxmNr+oQtCZW0kciozHA@mail.gmail.com
> Signed-off-by: Nikunj A Dadhania <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 4 ++--
> arch/x86/kvm/mmu/tdp_mmu.c | 2 --
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 8e24f73bf60b..db1feecd6fed 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2940,8 +2940,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> int ret;
> gfn_t base_gfn = fault->gfn;
>
> - kvm_mmu_hugepage_adjust(vcpu, fault);
> -
> trace_kvm_mmu_spte_requested(fault);
> for_each_shadow_entry(vcpu, fault->addr, it) {
> /*
> @@ -4035,6 +4033,8 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>
> r = RET_PF_RETRY;
>
> + kvm_mmu_hugepage_adjust(vcpu, fault);
> +
> if (is_tdp_mmu_fault)
> read_lock(&vcpu->kvm->mmu_lock);
> else
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index bc9e3553fba2..e03bf59b2f81 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -959,8 +959,6 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> u64 new_spte;
> int ret;
>
> - kvm_mmu_hugepage_adjust(vcpu, fault);
> -
> trace_kvm_mmu_spte_requested(fault);
>
> rcu_read_lock();
> --
> 2.32.0
>
On Tue, Mar 08, 2022, Nikunj A Dadhania wrote:
> This is a follow-up to the RFC implementation [1] that incorporates
> review feedback and bug fixes. See the "RFC v1" section below for a
> list of changes.
Heh, for future reference, the initial posting of a series/patch/RFC is implicitly
v1, i.e. this should be RFC v2.
> SEV guest requires the guest's pages to be pinned in host physical
> memory as migration of encrypted pages is not supported. The memory
> encryption scheme uses the physical address of the memory being
> encrypted. If guest pages are moved by the host, content decrypted in
> the guest would be incorrect thereby corrupting guest's memory.
>
> For SEV/SEV-ES guests, the hypervisor doesn't know which pages are
> encrypted and when the guest is done using those pages. Hypervisor
> should treat all the guest pages as encrypted until they are
> deallocated or the guest is destroyed.
>
> While provision a pfn, make KVM aware that guest pages need to be
> pinned for long-term and use appropriate pin_user_pages API for these
> special encrypted memory regions. KVM takes the first reference and
> holds it until a mapping is done. Take an extra reference before KVM
> releases the pfn.
>
> Actual pinning management is handled by vendor code via new
> kvm_x86_ops hooks. MMU calls in to vendor code to pin the page on
> demand. Metadata of the pinning is stored in architecture specific
> memslot area. During the memslot freeing path and deallocation path
> guest pages are unpinned.
>
> Guest boot time comparison:
> +---------------+----------------+-------------------+
> | Guest Memory | baseline | Demand Pinning + |
> | Size (GB) | v5.17-rc6(secs)| v5.17-rc6(secs) |
> +---------------+----------------+-------------------+
> | 4 | 6.16 | 5.71 |
> +---------------+----------------+-------------------+
> | 16 | 7.38 | 5.91 |
> +---------------+----------------+-------------------+
> | 64 | 12.17 | 6.16 |
> +---------------+----------------+-------------------+
> | 128 | 18.20 | 6.50 |
> +---------------+----------------+-------------------+
> | 192 | 24.56 | 6.80 |
> +---------------+----------------+-------------------+
Let me preface this by saying I generally like the idea and especially the
performance, but...
I think we should abandon this approach in favor of committing all our resources
to fd-based private memory[*], which (if done right) will provide on-demand pinning
for "free". I would much rather get that support merged sooner than later, and use
it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
memory. That would require guest kernel support to communicate private vs. shared,
but SEV guests already "need" to do that to play nice with live migration, so it's
not a big ask, just another carrot to entice guests/customers to update their kernel
(and possibly users to update their guest firmware).
This series isn't awful by any means, but it requires poking into core flows and
further complicates paths that are already anything but simple. And things like
conditionally grabbing vCPU0 to pin pages in its MMU make me flinch. And I think
the situation would only get worse by the time all the bugs and corner cases are
ironed out. E.g. this code is wrong:
void kvm_release_pfn_clean(kvm_pfn_t pfn)
{
if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) {
struct page *page = pfn_to_page(pfn);
if (page_maybe_dma_pinned(page))
unpin_user_page(page);
else
put_page(page);
}
}
EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
Because (a) page_maybe_dma_pinned() is susceptible to false positives (clearly
documented), and (b) even if it didn't get false positives, there's no guarantee
that _KVM_ owns a pin of the page.
It's not an impossible problem to solve, but I suspect any solution will require
either touching a lot of code or will be fragile and difficult to maintain, e.g.
by auditing all users to understand which need to pin and which don't. Even if
we _always_ pin memory for SEV guests, we'd still need to plumb the "is SEV guest"
info around.
And FWIW, my years-old idea of using a software-available SPTE bit to track pinned
pages is plagued by the same underlying issue: KVM's current management (or lack
thereof) of SEV guest memory just isn't viable long term. In all honesty, it
probably should never have been merged. We can't change the past, but we can,
and IMO should, avoid piling on more code to an approach that is fundamentally flawed.
[*] https://lore.kernel.org/all/[email protected]
On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
> > Let me preface this by saying I generally like the idea and especially the
> > performance, but...
> >
> > I think we should abandon this approach in favor of committing all our resources
> > to fd-based private memory[*], which (if done right) will provide on-demand pinning
> > for "free".
>
> I will give this a try for SEV, was on my todo list.
>
> > I would much rather get that support merged sooner than later, and use
> > it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
> > term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
> > memory.
>
> > That would require guest kernel support to communicate private vs. shared,
>
> Could you explain this in more detail? This is required for punching hole for shared pages?
Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
don't provide private vs. shared information to the host (KVM) on page fault. And
it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
properly enlightened beyond what is required architecturally.
On 3/29/2022 2:30 AM, Sean Christopherson wrote:
> On Tue, Mar 08, 2022, Nikunj A Dadhania wrote:
>> This is a follow-up to the RFC implementation [1] that incorporates
>> review feedback and bug fixes. See the "RFC v1" section below for a
>> list of changes.
>
> Heh, for future reference, the initial posting of a series/patch/RFC is implicitly
> v1, i.e. this should be RFC v2.
Sure.
>
>> SEV guest requires the guest's pages to be pinned in host physical
>> memory as migration of encrypted pages is not supported. The memory
>> encryption scheme uses the physical address of the memory being
>> encrypted. If guest pages are moved by the host, content decrypted in
>> the guest would be incorrect thereby corrupting guest's memory.
>>
>> For SEV/SEV-ES guests, the hypervisor doesn't know which pages are
>> encrypted and when the guest is done using those pages. Hypervisor
>> should treat all the guest pages as encrypted until they are
>> deallocated or the guest is destroyed.
>>
>> While provision a pfn, make KVM aware that guest pages need to be
>> pinned for long-term and use appropriate pin_user_pages API for these
>> special encrypted memory regions. KVM takes the first reference and
>> holds it until a mapping is done. Take an extra reference before KVM
>> releases the pfn.
>>
>> Actual pinning management is handled by vendor code via new
>> kvm_x86_ops hooks. MMU calls in to vendor code to pin the page on
>> demand. Metadata of the pinning is stored in architecture specific
>> memslot area. During the memslot freeing path and deallocation path
>> guest pages are unpinned.
>>
>> Guest boot time comparison:
>> +---------------+----------------+-------------------+
>> | Guest Memory | baseline | Demand Pinning + |
>> | Size (GB) | v5.17-rc6(secs)| v5.17-rc6(secs) |
>> +---------------+----------------+-------------------+
>> | 4 | 6.16 | 5.71 |
>> +---------------+----------------+-------------------+
>> | 16 | 7.38 | 5.91 |
>> +---------------+----------------+-------------------+
>> | 64 | 12.17 | 6.16 |
>> +---------------+----------------+-------------------+
>> | 128 | 18.20 | 6.50 |
>> +---------------+----------------+-------------------+
>> | 192 | 24.56 | 6.80 |
>> +---------------+----------------+-------------------+
>
> Let me preface this by saying I generally like the idea and especially the
> performance, but...
>
> I think we should abandon this approach in favor of committing all our resources
> to fd-based private memory[*], which (if done right) will provide on-demand pinning
> for "free".
I will give this a try for SEV, was on my todo list.
> I would much rather get that support merged sooner than later, and use
> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
> memory.
> That would require guest kernel support to communicate private vs. shared,
Could you explain this in more detail? This is required for punching hole for shared pages?
> but SEV guests already "need" to do that to play nice with live migration, so it's
> not a big ask, just another carrot to entice guests/customers to update their kernel
> (and possibly users to update their guest firmware).
>
> This series isn't awful by any means, but it requires poking into core flows and
> further complicates paths that are already anything but simple. And things like
> conditionally grabbing vCPU0 to pin pages in its MMU make me flinch. And I think
> the situation would only get worse by the time all the bugs and corner cases are
> ironed out. E.g. this code is wrong:
>
> void kvm_release_pfn_clean(kvm_pfn_t pfn)
> {
> if (!is_error_noslot_pfn(pfn) && !kvm_is_reserved_pfn(pfn)) {
> struct page *page = pfn_to_page(pfn);
>
> if (page_maybe_dma_pinned(page))
> unpin_user_page(page);
> else
> put_page(page);
> }
> }
> EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
>
> Because (a) page_maybe_dma_pinned() is susceptible to false positives (clearly
> documented), and (b) even if it didn't get false positives, there's no guarantee
> that _KVM_ owns a pin of the page.
Right, the pinning could have been done by some other subsystem.
>
> It's not an impossible problem to solve, but I suspect any solution will require
> either touching a lot of code or will be fragile and difficult to maintain, e.g.
> by auditing all users to understand which need to pin and which don't. Even if
> we _always_ pin memory for SEV guests, we'd still need to plumb the "is SEV guest"
> info around.
>
> And FWIW, my years-old idea of using a software-available SPTE bit to track pinned
> pages is plagued by the same underlying issue: KVM's current management (or lack
> thereof) of SEV guest memory just isn't viable long term. In all honesty, it
> probably should never have been merged. We can't change the past, but we can,
> and IMO should, avoid piling on more code to an approach that is fundamentally flawed.
>
> [*] https://lore.kernel.org/all/[email protected]
>
Thanks for the valuable feedback.
Regards
Nikunj
On 3/31/2022 1:17 AM, Sean Christopherson wrote:
> On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
>> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
>>> Let me preface this by saying I generally like the idea and especially the
>>> performance, but...
>>>
>>> I think we should abandon this approach in favor of committing all our resources
>>> to fd-based private memory[*], which (if done right) will provide on-demand pinning
>>> for "free".
>>
>> I will give this a try for SEV, was on my todo list.
>>
>>> I would much rather get that support merged sooner than later, and use
>>> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
>>> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
>>> memory.
>>
>>> That would require guest kernel support to communicate private vs. shared,
>>
>> Could you explain this in more detail? This is required for punching hole for shared pages?
>
> Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
> don't provide private vs. shared information to the host (KVM) on page fault. And
> it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
> accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
>
> That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
> hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
> ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
> running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
> properly enlightened beyond what is required architecturally.
>
So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
pages to the hypervisor, this information can be used to mark page shared/private.
Regards,
Nikunj
On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
>
>
>
> On 3/31/2022 1:17 AM, Sean Christopherson wrote:
> > On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
> >> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
> >>> Let me preface this by saying I generally like the idea and especially the
> >>> performance, but...
> >>>
> >>> I think we should abandon this approach in favor of committing all our resources
> >>> to fd-based private memory[*], which (if done right) will provide on-demand pinning
> >>> for "free".
> >>
> >> I will give this a try for SEV, was on my todo list.
> >>
> >>> I would much rather get that support merged sooner than later, and use
> >>> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
> >>> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
> >>> memory.
> >>
> >>> That would require guest kernel support to communicate private vs. shared,
> >>
> >> Could you explain this in more detail? This is required for punching hole for shared pages?
> >
> > Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
> > don't provide private vs. shared information to the host (KVM) on page fault. And
> > it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
> > accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
> >
> > That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
> > hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
> > ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
> > running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
> > properly enlightened beyond what is required architecturally.
> >
>
> So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
> KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
> pages to the hypervisor, this information can be used to mark page shared/private.
One concern here may be that the VMM doesn't know which guests have
KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
guest boots does the guest tell KVM that it supports
KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
the memory before we run the guest to be safe to be safe.
On Thu, Mar 31, 2022, Peter Gonda wrote:
> On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
> > On 3/31/2022 1:17 AM, Sean Christopherson wrote:
> > > On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
> > >> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
> > >>> Let me preface this by saying I generally like the idea and especially the
> > >>> performance, but...
> > >>>
> > >>> I think we should abandon this approach in favor of committing all our resources
> > >>> to fd-based private memory[*], which (if done right) will provide on-demand pinning
> > >>> for "free".
> > >>
> > >> I will give this a try for SEV, was on my todo list.
> > >>
> > >>> I would much rather get that support merged sooner than later, and use
> > >>> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
> > >>> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
> > >>> memory.
> > >>
> > >>> That would require guest kernel support to communicate private vs. shared,
> > >>
> > >> Could you explain this in more detail? This is required for punching hole for shared pages?
> > >
> > > Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
> > > don't provide private vs. shared information to the host (KVM) on page fault. And
> > > it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
> > > accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
> > >
> > > That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
> > > hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
> > > ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
> > > running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
> > > properly enlightened beyond what is required architecturally.
> > >
> >
> > So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
> > KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
> > pages to the hypervisor, this information can be used to mark page shared/private.
>
> One concern here may be that the VMM doesn't know which guests have
> KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
> guest boots does the guest tell KVM that it supports
> KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
> the memory before we run the guest to be safe to be safe.
Yep, that's a big reason why I view purging the existing SEV memory management as
a long term goal. The other being that userspace obviously needs to be updated to
support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
would be to restrict it to new VM types that have a disclaimer regarding additional
requirements.
[*] I believe Peter coined the UPM acronym for "Unmapping guest Private Memory". We've
been using it iternally for discussion and it rolls off the tongue a lot easier than
the full phrase, and is much more precise/descriptive than just "private fd".
On 4/1/2022 12:30 AM, Sean Christopherson wrote:
> On Thu, Mar 31, 2022, Peter Gonda wrote:
>> On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
>>> On 3/31/2022 1:17 AM, Sean Christopherson wrote:
>>>> On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
>>>>> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
>>>>>> Let me preface this by saying I generally like the idea and especially the
>>>>>> performance, but...
>>>>>>
>>>>>> I think we should abandon this approach in favor of committing all our resources
>>>>>> to fd-based private memory[*], which (if done right) will provide on-demand pinning
>>>>>> for "free".
>>>>>
>>>>> I will give this a try for SEV, was on my todo list.
>>>>>
>>>>>> I would much rather get that support merged sooner than later, and use
>>>>>> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
>>>>>> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
>>>>>> memory.
>>>>>
>>>>>> That would require guest kernel support to communicate private vs. shared,
>>>>>
>>>>> Could you explain this in more detail? This is required for punching hole for shared pages?
>>>>
>>>> Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
>>>> don't provide private vs. shared information to the host (KVM) on page fault. And
>>>> it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
>>>> accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
>>>>
>>>> That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
>>>> hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
>>>> ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
>>>> running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
>>>> properly enlightened beyond what is required architecturally.
>>>>
>>>
>>> So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
>>> KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
>>> pages to the hypervisor, this information can be used to mark page shared/private.
>>
>> One concern here may be that the VMM doesn't know which guests have
>> KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
>> guest boots does the guest tell KVM that it supports
>> KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
>> the memory before we run the guest to be safe to be safe.
>
> Yep, that's a big reason why I view purging the existing SEV memory management as
> a long term goal. The other being that userspace obviously needs to be updated to
> support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
> would be to restrict it to new VM types that have a disclaimer regarding additional
> requirements.
For SEV/SEV-ES could we base demand pinning on my first RFC[*]. Those patches does not touch
the core KVM flow. Moreover, it does not expect any guest/firmware changes.
[*] https://lore.kernel.org/kvm/[email protected]/
On Fri, Apr 01, 2022, Nikunj A. Dadhania wrote:
>
> On 4/1/2022 12:30 AM, Sean Christopherson wrote:
> > On Thu, Mar 31, 2022, Peter Gonda wrote:
> >> On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
> >>> So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
> >>> KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
> >>> pages to the hypervisor, this information can be used to mark page shared/private.
> >>
> >> One concern here may be that the VMM doesn't know which guests have
> >> KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
> >> guest boots does the guest tell KVM that it supports
> >> KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
> >> the memory before we run the guest to be safe to be safe.
> >
> > Yep, that's a big reason why I view purging the existing SEV memory management as
> > a long term goal. The other being that userspace obviously needs to be updated to
> > support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
> > would be to restrict it to new VM types that have a disclaimer regarding additional
> > requirements.
>
> For SEV/SEV-ES could we base demand pinning on my first RFC[*].
No, because as David pointed out, elevating the refcount is not the same as actually
pinning the page. Things like NUMA balancing will still try to migrate the page,
and even go so far as to zap the PTE, before bailing due to the outstanding reference.
In other words, not actually pinning makes the mm subsystem less efficient. Would it
functionally work? Yes. Is it acceptable KVM behavior? No.
> Those patches does not touch the core KVM flow.
I don't mind touching core KVM code. If this goes forward, I actually strongly
prefer having the x86 MMU code handle the pinning as opposed to burying it in SEV
via kvm_x86_ops. The reason I don't think it's worth pursuing this approach is
because (a) we know that the current SEV/SEV-ES memory management scheme is flawed
and is a deadend, and (b) this is not so trivial as we (or at least I) originally
thought/hoped it would be. In other words, it's not that I think demand pinning
is a bad idea, nor do I think the issues are unsolvable, it's that I think the
cost of getting a workable solution, e.g. code churn, ongoing maintenance, reviewer
time, etc..., far outweighs the benefits.
> Moreover, it does not expect any guest/firmware changes.
>
> [*] https://lore.kernel.org/kvm/[email protected]/
On Thu, Mar 31, 2022 at 12:01 PM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Mar 31, 2022, Peter Gonda wrote:
> > On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
> > > On 3/31/2022 1:17 AM, Sean Christopherson wrote:
> > > > On Wed, Mar 30, 2022, Nikunj A. Dadhania wrote:
> > > >> On 3/29/2022 2:30 AM, Sean Christopherson wrote:
> > > >>> Let me preface this by saying I generally like the idea and especially the
> > > >>> performance, but...
> > > >>>
> > > >>> I think we should abandon this approach in favor of committing all our resources
> > > >>> to fd-based private memory[*], which (if done right) will provide on-demand pinning
> > > >>> for "free".
> > > >>
> > > >> I will give this a try for SEV, was on my todo list.
> > > >>
> > > >>> I would much rather get that support merged sooner than later, and use
> > > >>> it as a carrot for legacy SEV to get users to move over to its new APIs, with a long
> > > >>> term goal of deprecating and disallowing SEV/SEV-ES guests without fd-based private
> > > >>> memory.
> > > >>
> > > >>> That would require guest kernel support to communicate private vs. shared,
> > > >>
> > > >> Could you explain this in more detail? This is required for punching hole for shared pages?
> > > >
> > > > Unlike SEV-SNP, which enumerates private vs. shared in the error code, SEV and SEV-ES
> > > > don't provide private vs. shared information to the host (KVM) on page fault. And
> > > > it's even more fundamental then that, as SEV/SEV-ES won't even fault if the guest
> > > > accesses the "wrong" GPA variant, they'll silent consume/corrupt data.
> > > >
> > > > That means KVM can't support implicit conversions for SEV/SEV-ES, and so an explicit
> > > > hypercall is mandatory. SEV doesn't even have a vendor-agnostic guest/host paravirt
> > > > ABI, and IIRC SEV-ES doesn't provide a conversion/map hypercall in the GHCB spec, so
> > > > running a SEV/SEV-ES guest under UPM would require the guest firmware+kernel to be
> > > > properly enlightened beyond what is required architecturally.
> > > >
> > >
> > > So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
> > > KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
> > > pages to the hypervisor, this information can be used to mark page shared/private.
> >
> > One concern here may be that the VMM doesn't know which guests have
> > KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
> > guest boots does the guest tell KVM that it supports
> > KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
> > the memory before we run the guest to be safe to be safe.
>
> Yep, that's a big reason why I view purging the existing SEV memory management as
> a long term goal. The other being that userspace obviously needs to be updated to
> support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
> would be to restrict it to new VM types that have a disclaimer regarding additional
> requirements.
>
> [*] I believe Peter coined the UPM acronym for "Unmapping guest Private Memory". We've
> been using it iternally for discussion and it rolls off the tongue a lot easier than
> the full phrase, and is much more precise/descriptive than just "private fd".
Can we really "purge the existing SEV memory management"? This seems
like a non-starter because it violates userspace API (i.e., the
ability for the userspace VMM to run a guest without
KVM_FEATURE_HC_MAP_GPA_RANGE). Or maybe I'm not quite following what
you mean by purge.
Assuming that UPM-based lazy pinning comes together via a new VM type
that only supports new images based on a minimum kernel version with
KVM_FEATURE_HC_MAP_GPA_RANGE, then I think this would like as follows:
1. Userspace VMM: Check SEV VM type. If type is legacy SEV type then
do upfront pinning. Else, skip up front pinning.
2. KVM: I'm not sure anything special needs to happen here. For the
legacy VM types, it can be configured to use legacy memslots,
presumably the same as non-CVMs will be configured. For the new VM
type, it should be configured to use UPM.
3. Control plane (thing creating VMs): Responsible for not allowing
legacy SEV images (i.e., images without KVM_FEATURE_HC_MAP_GPA_RANGE)
with the new SEV VM types that use UPM and have support for demand
pinning.
Sean: Did I get this right?
On Fri, Apr 1, 2022 at 11:02 AM Sean Christopherson <[email protected]> wrote:
>
> On Fri, Apr 01, 2022, Marc Orr wrote:
> > On Thu, Mar 31, 2022 at 12:01 PM Sean Christopherson <[email protected]> wrote:
> > > Yep, that's a big reason why I view purging the existing SEV memory management as
> > > a long term goal. The other being that userspace obviously needs to be updated to
> > > support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
> > > would be to restrict it to new VM types that have a disclaimer regarding additional
> > > requirements.
> > >
> > > [*] I believe Peter coined the UPM acronym for "Unmapping guest Private Memory". We've
> > > been using it iternally for discussion and it rolls off the tongue a lot easier than
> > > the full phrase, and is much more precise/descriptive than just "private fd".
> >
> > Can we really "purge the existing SEV memory management"? This seems
> > like a non-starter because it violates userspace API (i.e., the
> > ability for the userspace VMM to run a guest without
> > KVM_FEATURE_HC_MAP_GPA_RANGE). Or maybe I'm not quite following what
> > you mean by purge.
>
> I really do mean purge, but I also really do mean "long term", as in 5+ years
> (probably 10+ if I'm being realistic).
>
> Removing support is completely ok, as is changing the uABI, the rule is that we
> can't break userspace. If all users are migrated to private-fd, e.g. by carrots
> and/or sticks such as putting the code into maintenance-only mode, then at some
> point in the future there will be no users left to break and we can drop the
> current code and make use of private-fd mandatory for SEV/SEV-ES guests.
Ah, it makes sense now. Thanks!
> > Assuming that UPM-based lazy pinning comes together via a new VM type
> > that only supports new images based on a minimum kernel version with
> > KVM_FEATURE_HC_MAP_GPA_RANGE, then I think this would like as follows:
> >
> > 1. Userspace VMM: Check SEV VM type. If type is legacy SEV type then
> > do upfront pinning. Else, skip up front pinning.
>
> Yep, if by legacy "SEV type" you mean "SEV/SEV-ES guest that isn't required to
> use MAP_GPA_RANGE", which I'm pretty sure you do based on #3.
Yeah, that's exactly what I meant.
> > 2. KVM: I'm not sure anything special needs to happen here. For the
> > legacy VM types, it can be configured to use legacy memslots,
> > presumably the same as non-CVMs will be configured. For the new VM
> > type, it should be configured to use UPM.
>
> Correct, for now, KVM does nothing different for SEV/SEV-ES guests.
>
> > 3. Control plane (thing creating VMs): Responsible for not allowing
> > legacy SEV images (i.e., images without KVM_FEATURE_HC_MAP_GPA_RANGE)
> > with the new SEV VM types that use UPM and have support for demand
> > pinning.
> >
> > Sean: Did I get this right?
>
> Yep.
Thank you for verifying.
On Fri, Apr 01, 2022, Marc Orr wrote:
> On Thu, Mar 31, 2022 at 12:01 PM Sean Christopherson <[email protected]> wrote:
> > Yep, that's a big reason why I view purging the existing SEV memory management as
> > a long term goal. The other being that userspace obviously needs to be updated to
> > support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
> > would be to restrict it to new VM types that have a disclaimer regarding additional
> > requirements.
> >
> > [*] I believe Peter coined the UPM acronym for "Unmapping guest Private Memory". We've
> > been using it iternally for discussion and it rolls off the tongue a lot easier than
> > the full phrase, and is much more precise/descriptive than just "private fd".
>
> Can we really "purge the existing SEV memory management"? This seems
> like a non-starter because it violates userspace API (i.e., the
> ability for the userspace VMM to run a guest without
> KVM_FEATURE_HC_MAP_GPA_RANGE). Or maybe I'm not quite following what
> you mean by purge.
I really do mean purge, but I also really do mean "long term", as in 5+ years
(probably 10+ if I'm being realistic).
Removing support is completely ok, as is changing the uABI, the rule is that we
can't break userspace. If all users are migrated to private-fd, e.g. by carrots
and/or sticks such as putting the code into maintenance-only mode, then at some
point in the future there will be no users left to break and we can drop the
current code and make use of private-fd mandatory for SEV/SEV-ES guests.
> Assuming that UPM-based lazy pinning comes together via a new VM type
> that only supports new images based on a minimum kernel version with
> KVM_FEATURE_HC_MAP_GPA_RANGE, then I think this would like as follows:
>
> 1. Userspace VMM: Check SEV VM type. If type is legacy SEV type then
> do upfront pinning. Else, skip up front pinning.
Yep, if by legacy "SEV type" you mean "SEV/SEV-ES guest that isn't required to
use MAP_GPA_RANGE", which I'm pretty sure you do based on #3.
> 2. KVM: I'm not sure anything special needs to happen here. For the
> legacy VM types, it can be configured to use legacy memslots,
> presumably the same as non-CVMs will be configured. For the new VM
> type, it should be configured to use UPM.
Correct, for now, KVM does nothing different for SEV/SEV-ES guests.
> 3. Control plane (thing creating VMs): Responsible for not allowing
> legacy SEV images (i.e., images without KVM_FEATURE_HC_MAP_GPA_RANGE)
> with the new SEV VM types that use UPM and have support for demand
> pinning.
>
> Sean: Did I get this right?
Yep.
On 4/1/2022 8:24 PM, Sean Christopherson wrote:
> On Fri, Apr 01, 2022, Nikunj A. Dadhania wrote:
>>
>> On 4/1/2022 12:30 AM, Sean Christopherson wrote:
>>> On Thu, Mar 31, 2022, Peter Gonda wrote:
>>>> On Wed, Mar 30, 2022 at 10:48 PM Nikunj A. Dadhania <[email protected]> wrote:
>>>>> So with guest supporting KVM_FEATURE_HC_MAP_GPA_RANGE and host (KVM) supporting
>>>>> KVM_HC_MAP_GPA_RANGE hypercall, SEV/SEV-ES guest should communicate private/shared
>>>>> pages to the hypervisor, this information can be used to mark page shared/private.
>>>>
>>>> One concern here may be that the VMM doesn't know which guests have
>>>> KVM_FEATURE_HC_MAP_GPA_RANGE support and which don't. Only once the
>>>> guest boots does the guest tell KVM that it supports
>>>> KVM_FEATURE_HC_MAP_GPA_RANGE. If the guest doesn't we need to pin all
>>>> the memory before we run the guest to be safe to be safe.
>>>
>>> Yep, that's a big reason why I view purging the existing SEV memory management as
>>> a long term goal. The other being that userspace obviously needs to be updated to
>>> support UPM[*]. I suspect the only feasible way to enable this for SEV/SEV-ES
>>> would be to restrict it to new VM types that have a disclaimer regarding additional
>>> requirements.
>>
>> For SEV/SEV-ES could we base demand pinning on my first RFC[*].
>
> No, because as David pointed out, elevating the refcount is not the same as actually
> pinning the page. Things like NUMA balancing will still try to migrate the page,
> and even go so far as to zap the PTE, before bailing due to the outstanding reference.
> In other words, not actually pinning makes the mm subsystem less efficient. Would it
> functionally work? Yes. Is it acceptable KVM behavior? No.
>
>> Those patches does not touch the core KVM flow.
>
> I don't mind touching core KVM code. If this goes forward, I actually strongly
> prefer having the x86 MMU code handle the pinning as opposed to burying it in SEV
> via kvm_x86_ops. The reason I don't think it's worth pursuing this approach is
> because (a) we know that the current SEV/SEV-ES memory management scheme is flawed
> and is a deadend, and (b) this is not so trivial as we (or at least I) originally
> thought/hoped it would be. In other words, it's not that I think demand pinning
> is a bad idea, nor do I think the issues are unsolvable, it's that I think the
> cost of getting a workable solution, e.g. code churn, ongoing maintenance, reviewer
> time, etc..., far outweighs the benefits.
Point noted Sean, will focus on the UPM effort.
Regards
Nikunj