Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On Fri, 2021-12-10 at 15:53 +0100, Paolo Bonzini wrote: > On 12/10/21 13:25, David Woodhouse wrote: > > On Thu, 2021-12-09 at 23:34 +0100, Paolo Bonzini wrote: > > > Compared to the review it's missing this hunk: > > > > > > @@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, > > > struct gfn_to_pfn_cache *gpc) > > > > > > gpc->valid = false; > > > > > > - old_khva = gpc->khva; > > > + old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK); > > > old_dirty = gpc->dirty; > > > > Do you know what? I couldn't bring myself to add that a second time. I > > managed it once, but it made me sad. > > > > Did it like this instead: > > > > - old_khva = gpc->khva; > > + old_khva = gpc->khva - offset_in_page(gpc->khva); > > Very nice, and it would have deserved a macro in include/linux if there > wasn't a decent way to write it. Indeed. I actually went hunting for such, which is where I found offset_in_page(). > > I checked that for me at least, GCC is clever enough to just do the > > mask. > > > > old_khva = gpc->khva - offset_in_page(gpc->khva); > > 131: 48 8b 43 78 mov0x78(%rbx),%rax > > 135: 48 25 00 f0 ff ff and$0xf000,%rax > > > > > > I still don't see the previous patches in kvm/next — is that an > > automatic push after testing has passed, or is the kernel.org > > infrastructure just *really* slow? > > No, it's me really wanting to send out the -rc5 pull request before the > weekend. Just wait five more minutes. Ack. Thanks. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On 12/10/21 13:25, David Woodhouse wrote: On Thu, 2021-12-09 at 23:34 +0100, Paolo Bonzini wrote: Compared to the review it's missing this hunk: @@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) gpc->valid = false; - old_khva = gpc->khva; + old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK); old_dirty = gpc->dirty; Do you know what? I couldn't bring myself to add that a second time. I managed it once, but it made me sad. Did it like this instead: - old_khva = gpc->khva; + old_khva = gpc->khva - offset_in_page(gpc->khva); Very nice, and it would have deserved a macro in include/linux if there wasn't a decent way to write it. I checked that for me at least, GCC is clever enough to just do the mask. old_khva = gpc->khva - offset_in_page(gpc->khva); 131: 48 8b 43 78 mov0x78(%rbx),%rax 135: 48 25 00 f0 ff ff and$0xf000,%rax I still don't see the previous patches in kvm/next — is that an automatic push after testing has passed, or is the kernel.org infrastructure just *really* slow? No, it's me really wanting to send out the -rc5 pull request before the weekend. Just wait five more minutes. Paolo I've pushed based on the currently-visible kvm/next to https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xen-evtchn and can resend when the tree finally surfaces.
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On Thu, 2021-12-09 at 23:34 +0100, Paolo Bonzini wrote: > > Compared to the review it's missing this hunk: > > @@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, struct > gfn_to_pfn_cache *gpc) > > gpc->valid = false; > > - old_khva = gpc->khva; > + old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK); > old_dirty = gpc->dirty; Do you know what? I couldn't bring myself to add that a second time. I managed it once, but it made me sad. Did it like this instead: - old_khva = gpc->khva; + old_khva = gpc->khva - offset_in_page(gpc->khva); I checked that for me at least, GCC is clever enough to just do the mask. old_khva = gpc->khva - offset_in_page(gpc->khva); 131: 48 8b 43 78 mov0x78(%rbx),%rax 135: 48 25 00 f0 ff ff and$0xf000,%rax I still don't see the previous patches in kvm/next — is that an automatic push after testing has passed, or is the kernel.org infrastructure just *really* slow? I've pushed based on the currently-visible kvm/next to https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/xen-evtchn and can resend when the tree finally surfaces. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On Thu, 2021-12-09 at 23:34 +0100, Paolo Bonzini wrote: > Compared to the review it's missing this hunk: > > @@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, struct > gfn_to_pfn_cache *gpc) > > gpc->valid = false; > > - old_khva = gpc->khva; > + old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK); > old_dirty = gpc->dirty; > old_gpa = gpc->gpa; > old_pfn = gpc->pfn; Ah right, there were two of those. Will fix; thanks. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On 12/9/21 21:40, David Woodhouse wrote: NP, very useful fixes. Thanks. Incremental patch looks like this. It passes the xen_shinfo_test self-test; will test it with real Xen guests tomorrow and repost based on your kvm/next tree once it shows up. Compared to the review it's missing this hunk: @@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) gpc->valid = false; - old_khva = gpc->khva; + old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK); old_dirty = gpc->dirty; old_gpa = gpc->gpa; old_pfn = gpc->pfn; Otherwise looks good, thanks! Paolo
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
On Thu, 2021-12-09 at 19:34 +0100, Paolo Bonzini wrote:
> Sorry for the late review...
NP, very useful fixes. Thanks. Incremental patch looks like this. It
passes the xen_shinfo_test self-test; will test it with real Xen guests
tomorrow and repost based on your kvm/next tree once it shows up.
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index d8c6e1d4a647..9e3c662f815f 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -124,6 +124,33 @@ static void __release_gpc(struct kvm *kvm, kvm_pfn_t pfn,
void *khva,
}
}
+static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, unsigned long uhva)
+{
+ unsigned long mmu_seq;
+ kvm_pfn_t new_pfn;
+ int retry;
+
+ do {
+ mmu_seq = kvm->mmu_notifier_seq;
+ smp_rmb();
+
+ /* We always request a writeable mapping */
+ new_pfn = hva_to_pfn(uhva, false, NULL, true, NULL);
+ if (is_error_noslot_pfn(new_pfn))
+ break;
+
+ KVM_MMU_READ_LOCK(kvm);
+ retry = mmu_notifier_retry_hva(kvm, mmu_seq, uhva);
+ KVM_MMU_READ_UNLOCK(kvm);
+ if (!retry)
+ break;
+
+ cond_resched();
+ } while (1);
+
+ return new_pfn;
+}
+
int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct gfn_to_pfn_cache *gpc,
gpa_t gpa, unsigned long len, bool dirty)
{
@@ -147,7 +174,7 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
old_gpa = gpc->gpa;
old_pfn = gpc->pfn;
- old_khva = gpc->khva;
+ old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK);
old_uhva = gpc->uhva;
old_valid = gpc->valid;
old_dirty = gpc->dirty;
@@ -178,8 +205,6 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
if (!old_valid || old_uhva != gpc->uhva) {
unsigned long uhva = gpc->uhva;
void *new_khva = NULL;
- unsigned long mmu_seq;
- int retry;
/* Placeholders for "hva is valid but not yet mapped" */
gpc->pfn = KVM_PFN_ERR_FAULT;
@@ -188,28 +213,15 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
write_unlock_irq(>lock);
- retry_map:
- mmu_seq = kvm->mmu_notifier_seq;
- smp_rmb();
-
- /* We always request a writeable mapping */
- new_pfn = hva_to_pfn(uhva, false, NULL, true, NULL);
+ new_pfn = hva_to_pfn_retry(kvm, uhva);
if (is_error_noslot_pfn(new_pfn)) {
ret = -EFAULT;
goto map_done;
}
- KVM_MMU_READ_LOCK(kvm);
- retry = mmu_notifier_retry_hva(kvm, mmu_seq, uhva);
- KVM_MMU_READ_UNLOCK(kvm);
- if (retry) {
- cond_resched();
- goto retry_map;
- }
-
if (gpc->kernel_map) {
if (new_pfn == old_pfn) {
- new_khva = (void *)((unsigned long)old_khva -
page_offset);
+ new_khva = old_khva;
old_pfn = KVM_PFN_ERR_FAULT;
old_khva = NULL;
} else if (pfn_valid(new_pfn)) {
@@ -219,7 +231,9 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
new_khva = memremap(pfn_to_hpa(new_pfn),
PAGE_SIZE, MEMREMAP_WB);
#endif
}
- if (!new_khva)
+ if (new_khva)
+ new_khva += page_offset;
+ else
ret = -EFAULT;
}
@@ -232,7 +246,7 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
} else {
/* At this point, gpc->valid may already have been
cleared */
gpc->pfn = new_pfn;
- gpc->khva = new_khva + page_offset;
+ gpc->khva = new_khva;
}
} else {
/* If the HVA→PFN mapping was already valid, don't unmap it. */
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
Sorry for the late review...
On 11/21/21 13:54, David Woodhouse wrote:
+EXPORT_SYMBOL_GPL(kvm_gfn_to_pfn_cache_check);
+
+static void __release_gpc(struct kvm *kvm, kvm_pfn_t pfn, void *khva,
+ gpa_t gpa, bool dirty)
+{
+ /* Unmap the old page if it was mapped before, and release it */
+ if (!is_error_noslot_pfn(pfn)) {
+ if (khva) {
+ if (pfn_valid(pfn))
+ kunmap(pfn_to_page(pfn));
+#ifdef CONFIG_HAS_IOMEM
+ else
+ memunmap(khva);
+#endif
+ }
Considering that the khva is passed directly to memunmap, perhaps it's
cleaner to ensure it's page-aligned:
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 13cae72d39e9..267477bd2972 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -147,7 +147,7 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
old_gpa = gpc->gpa;
old_pfn = gpc->pfn;
- old_khva = gpc->khva;
+ old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK);
old_uhva = gpc->uhva;
old_valid = gpc->valid;
old_dirty = gpc->dirty;
@@ -209,7 +209,7 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc,
if (gpc->kernel_map) {
if (new_pfn == old_pfn) {
- new_khva = (void *)((unsigned long)old_khva -
page_offset);
+ new_khva = old_khva;
old_pfn = KVM_PFN_ERR_FAULT;
old_khva = NULL;
} else if (pfn_valid(new_pfn)) {
@@ -265,7 +265,7 @@ void kvm_gfn_to_pfn_cache_unmap(struct kvm *kvm, struct
gfn_to_pfn_cache *gpc)
gpc->valid = false;
- old_khva = gpc->khva;
+ old_khva = (void *)((unsigned long)gpc->khva & ~PAGE_MASK);
old_dirty = gpc->dirty;
old_gpa = gpc->gpa;
old_pfn = gpc->pfn;
+ retry_map:
+ mmu_seq = kvm->mmu_notifier_seq;
+ smp_rmb();
+
+ /* We always request a writeable mapping */
+ new_pfn = hva_to_pfn(uhva, false, NULL, true, NULL);
+ if (is_error_noslot_pfn(new_pfn)) {
+ ret = -EFAULT;
+ goto map_done;
+ }
+
+ KVM_MMU_READ_LOCK(kvm);
+ retry = mmu_notifier_retry_hva(kvm, mmu_seq, uhva);
+ KVM_MMU_READ_UNLOCK(kvm);
+ if (retry) {
+ cond_resched();
+ goto retry_map;
+ }
+
This should also be a separate function, like
static kvm_pfn_t hva_to_pfn_retry(unsigned long uhva)
{
kvm_pfn_t new_pfn
unsigned long mmu_seq;
int retry;
retry_map:
mmu_seq = kvm->mmu_notifier_seq;
smp_rmb();
/* We always request a writeable mapping */
new_pfn = hva_to_pfn(uhva, false, NULL, true, NULL);
if (is_error_noslot_pfn(new_pfn))
return new_pfn;
KVM_MMU_READ_LOCK(kvm);
retry = mmu_notifier_retry_hva(kvm, mmu_seq, uhva);
KVM_MMU_READ_UNLOCK(kvm);
if (retry) {
cond_resched();
goto retry_map;
}
return new_pfn;
}
+ write_lock_irq(>lock);
+ if (ret) {
+ gpc->valid = false;
+ gpc->pfn = KVM_PFN_ERR_FAULT;
+ gpc->khva = NULL;
+ } else {
+ /* At this point, gpc->valid may already have been
cleared */
+ gpc->pfn = new_pfn;
+ gpc->khva = new_khva + page_offset;
+ }
Should set gpc->khva only if new_khva != NULL (i.e. only if gpc->kernel_map
is true).
Paolo
[PATCH v5 08/12] KVM: Reinstate gfn_to_pfn_cache with invalidation support
From: David Woodhouse
This can be used in two modes. There is an atomic mode where the cached
mapping is accessed while holding the rwlock, and a mode where the
physical address is used by a vCPU in guest mode.
For the latter case, an invalidation will wake the vCPU with the new
KVM_REQ_GPC_INVALIDATE, and the architecture will need to refresh any
caches it still needs to access before entering guest mode again.
Only one vCPU can be targeted by the wake requests; it's simple enough
to make it wake all vCPUs or even a mask but I don't see a use case for
that additional complexity right now.
Invalidation happens from the invalidate_range_start MMU notifier, which
needs to be able to sleep in order to wake the vCPU and wait for it.
This means that revalidation potentially needs to "wait" for the MMU
operation to complete and the invalidate_range_end notifier to be
invoked. Like the vCPU when it takes a page fault in that period, we
just spin — fixing that in a future patch by implementing an actual
*wait* may be another part of shaving this particularly hirsute yak.
As noted in the comments in the function itself, the only case where
the invalidate_range_start notifier is expected to be called *without*
being able to sleep is when the OOM reaper is killing the process. In
that case, we expect the vCPU threads already to have exited, and thus
there will be nothing to wake, and no reason to wait. So we clear the
KVM_REQUEST_WAIT bit and send the request anyway, then complain loudly
if there actually *was* anything to wake up.
Signed-off-by: David Woodhouse
---
arch/x86/kvm/Kconfig | 1 +
include/linux/kvm_host.h | 103
include/linux/kvm_types.h | 18 +++
virt/kvm/Kconfig | 3 +
virt/kvm/Makefile.kvm | 1 +
virt/kvm/dirty_ring.c | 2 +-
virt/kvm/kvm_main.c | 12 +-
virt/kvm/kvm_mm.h | 44 ++
virt/kvm/mmu_lock.h | 23 ---
virt/kvm/pfncache.c | 323 ++
10 files changed, 503 insertions(+), 27 deletions(-)
create mode 100644 virt/kvm/kvm_mm.h
delete mode 100644 virt/kvm/mmu_lock.h
create mode 100644 virt/kvm/pfncache.c
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index d7fa0a42ac25..af351107d47f 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -26,6 +26,7 @@ config KVM
select PREEMPT_NOTIFIERS
select MMU_NOTIFIER
select HAVE_KVM_IRQCHIP
+ select HAVE_KVM_PFNCACHE
select HAVE_KVM_IRQFD
select HAVE_KVM_DIRTY_RING
select IRQ_BYPASS_MANAGER
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c310648cc8f1..457c38d75913 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -151,6 +151,7 @@ static inline bool is_error_page(struct page *page)
#define KVM_REQ_UNBLOCK 2
#define KVM_REQ_UNHALT3
#define KVM_REQ_VM_DEAD (4 | KVM_REQUEST_WAIT |
KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_GPC_INVALIDATE(5 | KVM_REQUEST_WAIT |
KVM_REQUEST_NO_WAKEUP)
#define KVM_REQUEST_ARCH_BASE 8
#define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
@@ -559,6 +560,10 @@ struct kvm {
unsigned long mn_active_invalidate_count;
struct rcuwait mn_memslots_update_rcuwait;
+ /* For management / invalidation of gfn_to_pfn_caches */
+ spinlock_t gpc_lock;
+ struct list_head gpc_list;
+
/*
* created_vcpus is protected by kvm->lock, and is incremented
* at the beginning of KVM_CREATE_VCPU. online_vcpus is only
@@ -966,6 +971,104 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t
gpa, const void *data,
unsigned long len);
void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn);
+/**
+ * kvm_gfn_to_pfn_cache_init - prepare a cached kernel mapping and HPA for a
+ * given guest physical address.
+ *
+ * @kvm: pointer to kvm instance.
+ * @gpc: struct gfn_to_pfn_cache object.
+ * @vcpu: vCPU to be used for marking pages dirty and to be woken on
+ *invalidation.
+ * @guest_uses_pa: indicates that the resulting host physical PFN is used while
+ *@vcpu is IN_GUEST_MODE so invalidations should wake it.
+ * @kernel_map:requests a kernel virtual mapping (kmap / memremap).
+ * @gpa: guest physical address to map.
+ * @len: sanity check; the range being access must fit a single page.
+ * @dirty: mark the cache dirty immediately.
+ *
+ * @return: 0 for success.
+ *-EINVAL for a mapping which would cross a page boundary.
+ * -EFAULT for an untranslatable guest physical address.
+ *
+ * This primes a gfn_to_pfn_cache and links it into the @kvm's list for
+ * invalidations to be processed. Invalidation callbacks to @vcpu using
+ * %KVM_REQ_GPC_INVALIDATE will occur only for MMU notifiers, not for KVM
+ * memslot changes. Callers are required to
