From: Paul Durrant pdurrant@amazon.com

There is a pfncache unmap helper but mapping is open-coded. Arguably this is fine because mapping is done in only one place, hva_to_pfn_retry(), but adding the helper does make that function more readable.

No functional change intended.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: David Woodhouse dwmw2@infradead.org Cc: Paolo Bonzini pbonzini@redhat.com

v8: - Re-work commit comment. - Fix CONFIG_HAS_IOMEM=n build. --- virt/kvm/pfncache.c | 47 ++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 2d6aba677830..10842f1eeeae 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -96,17 +96,32 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) } EXPORT_SYMBOL_GPL(kvm_gpc_check);

-static void gpc_unmap_khva(kvm_pfn_t pfn, void *khva) +static void *gpc_map(kvm_pfn_t pfn) { - /* Unmap the old pfn/page if it was mapped before. */ - if (!is_error_noslot_pfn(pfn) && khva) { - if (pfn_valid(pfn)) - kunmap(pfn_to_page(pfn)); + if (pfn_valid(pfn)) + return kmap(pfn_to_page(pfn)); + #ifdef CONFIG_HAS_IOMEM - else - memunmap(khva); + return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); +#else + return NULL; #endif +} + +static void gpc_unmap(kvm_pfn_t pfn, void *khva) +{ + /* Unmap the old pfn/page if it was mapped before. */ + if (is_error_noslot_pfn(pfn) || !khva) + return; + + if (pfn_valid(pfn)) { + kunmap(pfn_to_page(pfn)); + return; } + +#ifdef CONFIG_HAS_IOMEM + memunmap(khva); +#endif }

static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq) @@ -175,7 +190,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * the existing mapping and didn't create a new one. */ if (new_khva != old_khva) - gpc_unmap_khva(new_pfn, new_khva); + gpc_unmap(new_pfn, new_khva);

kvm_release_pfn_clean(new_pfn);

@@ -193,15 +208,11 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * too must be done outside of gpc->lock! */ if (gpc->usage & KVM_HOST_USES_PFN) { - if (new_pfn == gpc->pfn) { + if (new_pfn == gpc->pfn) new_khva = old_khva; - } else if (pfn_valid(new_pfn)) { - new_khva = kmap(pfn_to_page(new_pfn)); -#ifdef CONFIG_HAS_IOMEM - } else { - new_khva = memremap(pfn_to_hpa(new_pfn), PAGE_SIZE, MEMREMAP_WB); -#endif - } + else + new_khva = gpc_map(new_pfn); + if (!new_khva) { kvm_release_pfn_clean(new_pfn); goto out_error; @@ -326,7 +337,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, mutex_unlock(&gpc->refresh_lock);

if (unmap_old) - gpc_unmap_khva(old_pfn, old_khva); + gpc_unmap(old_pfn, old_khva);

return ret; } @@ -412,7 +423,7 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc) list_del(&gpc->list); spin_unlock(&kvm->gpc_lock);

- gpc_unmap_khva(old_pfn, old_khva); + gpc_unmap(old_pfn, old_khva); } } EXPORT_SYMBOL_GPL(kvm_gpc_deactivate);

From: Paul Durrant pdurrant@amazon.com

Sampling gpa and memslot from an unlocked pfncache may yield inconsistent values so, since there is no problem with calling mark_page_dirty_in_slot() with the pfncache lock held, relocate the calls in kvm_xen_update_runstate_guest() and kvm_xen_inject_pending_events() accordingly.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: David Woodhouse dwmw2@infradead.org Cc: x86@kernel.org

v13: - Patch title change.

v8: - New in this version. --- arch/x86/kvm/xen.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 4b4e738c6f1b..f3327508ae41 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -452,14 +452,13 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic) smp_wmb(); }

- if (user_len2) + if (user_len2) { + mark_page_dirty_in_slot(v->kvm, gpc2->memslot, gpc2->gpa >> PAGE_SHIFT); read_unlock(&gpc2->lock); - - read_unlock_irqrestore(&gpc1->lock, flags); + }

mark_page_dirty_in_slot(v->kvm, gpc1->memslot, gpc1->gpa >> PAGE_SHIFT); - if (user_len2) - mark_page_dirty_in_slot(v->kvm, gpc2->memslot, gpc2->gpa >> PAGE_SHIFT); + read_unlock_irqrestore(&gpc1->lock, flags); }

void kvm_xen_update_runstate(struct kvm_vcpu *v, int state) @@ -565,13 +564,13 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v) : "0" (evtchn_pending_sel32)); WRITE_ONCE(vi->evtchn_upcall_pending, 1); } + + mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT); read_unlock_irqrestore(&gpc->lock, flags);

/* For the per-vCPU lapic vector, deliver it as MSI. */ if (v->arch.xen.upcall_vector) kvm_xen_inject_vcpu_vector(v); - - mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT); }

int __kvm_xen_has_interrupt(struct kvm_vcpu *v)

From: Paul Durrant pdurrant@amazon.com

As noted in [1] the KVM_GUEST_USES_PFN usage flag is never set by any callers of kvm_gpc_init(), which also makes the 'vcpu' argument redundant. Moreover, all existing callers specify KVM_HOST_USES_PFN so the usage check in hva_to_pfn_retry() and hence the 'usage' argument to kvm_gpc_init() are also redundant. Remove the pfn_cache_usage enumeration and remove the redundant arguments, fields of struct gfn_to_hva_cache, and all the related code.

[1] https://lore.kernel.org/all/ZQiR8IpqOZrOpzHC@google.com/

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: David Woodhouse dwmw2@infradead.org Cc: x86@kernel.org

v8: - New in this version. --- arch/x86/kvm/x86.c | 2 +- arch/x86/kvm/xen.c | 14 ++++----- include/linux/kvm_host.h | 11 +------ include/linux/kvm_types.h | 8 ----- virt/kvm/pfncache.c | 61 ++++++--------------------------------- 5 files changed, 16 insertions(+), 80 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 16269430006f..31cd5d803dae 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12042,7 +12042,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) vcpu->arch.regs_avail = ~0; vcpu->arch.regs_dirty = ~0;

- kvm_gpc_init(&vcpu->arch.pv_time, vcpu->kvm, vcpu, KVM_HOST_USES_PFN); + kvm_gpc_init(&vcpu->arch.pv_time, vcpu->kvm);

if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu)) vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE; diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 2d001a9c6378..e90464225467 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -2108,14 +2108,10 @@ void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)

timer_setup(&vcpu->arch.xen.poll_timer, cancel_evtchn_poll, 0);

- kvm_gpc_init(&vcpu->arch.xen.runstate_cache, vcpu->kvm, NULL, - KVM_HOST_USES_PFN); - kvm_gpc_init(&vcpu->arch.xen.runstate2_cache, vcpu->kvm, NULL, - KVM_HOST_USES_PFN); - kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache, vcpu->kvm, NULL, - KVM_HOST_USES_PFN); - kvm_gpc_init(&vcpu->arch.xen.vcpu_time_info_cache, vcpu->kvm, NULL, - KVM_HOST_USES_PFN); + kvm_gpc_init(&vcpu->arch.xen.runstate_cache, vcpu->kvm); + kvm_gpc_init(&vcpu->arch.xen.runstate2_cache, vcpu->kvm); + kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache, vcpu->kvm); + kvm_gpc_init(&vcpu->arch.xen.vcpu_time_info_cache, vcpu->kvm); }

void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu) @@ -2158,7 +2154,7 @@ void kvm_xen_init_vm(struct kvm *kvm) { mutex_init(&kvm->arch.xen.xen_lock); idr_init(&kvm->arch.xen.evtchn_ports); - kvm_gpc_init(&kvm->arch.xen.shinfo_cache, kvm, NULL, KVM_HOST_USES_PFN); + kvm_gpc_init(&kvm->arch.xen.shinfo_cache, kvm); }

void kvm_xen_destroy_vm(struct kvm *kvm) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 5a27b4389d32..da20b7018cc8 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1318,21 +1318,12 @@ void kvm_vcpu_mark_page_dirty(struct kvm_vcpu *vcpu, gfn_t gfn); * * @gpc: struct gfn_to_pfn_cache object. * @kvm: pointer to kvm instance. - * @vcpu: vCPU to be used for marking pages dirty and to be woken on - * invalidation. - * @usage: indicates if the resulting host physical PFN is used while - * the @vcpu is IN_GUEST_MODE (in which case invalidation of - * the cache from MMU notifiers---but not for KVM memslot - * changes!---will also force @vcpu to exit the guest and - * refresh the cache); and/or if the PFN used directly - * by KVM (and thus needs a kernel virtual mapping). * * This sets up a gfn_to_pfn_cache by initializing locks and assigning the * immutable attributes. Note, the cache must be zero-allocated (or zeroed by * the caller before init). */ -void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm, - struct kvm_vcpu *vcpu, enum pfn_cache_usage usage); +void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm);

/** * kvm_gpc_activate - prepare a cached kernel mapping and HPA for a given guest diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h index 9d1f7835d8c1..d93f6522b2c3 100644 --- a/include/linux/kvm_types.h +++ b/include/linux/kvm_types.h @@ -49,12 +49,6 @@ typedef u64 hfn_t;

typedef hfn_t kvm_pfn_t;

-enum pfn_cache_usage { - KVM_GUEST_USES_PFN = BIT(0), - KVM_HOST_USES_PFN = BIT(1), - KVM_GUEST_AND_HOST_USE_PFN = KVM_GUEST_USES_PFN | KVM_HOST_USES_PFN, -}; - struct gfn_to_hva_cache { u64 generation; gpa_t gpa; @@ -69,13 +63,11 @@ struct gfn_to_pfn_cache { unsigned long uhva; struct kvm_memory_slot *memslot; struct kvm *kvm; - struct kvm_vcpu *vcpu; struct list_head list; rwlock_t lock; struct mutex refresh_lock; void *khva; kvm_pfn_t pfn; - enum pfn_cache_usage usage; bool active; bool valid; }; diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index f3571f44d9af..6f4b537eb25b 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -25,9 +25,7 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start, unsigned long end, bool may_block) { - DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); struct gfn_to_pfn_cache *gpc; - bool evict_vcpus = false;

spin_lock(&kvm->gpc_lock); list_for_each_entry(gpc, &kvm->gpc_list, list) { @@ -37,43 +35,10 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start, if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) && gpc->uhva >= start && gpc->uhva < end) { gpc->valid = false; - - /* - * If a guest vCPU could be using the physical address, - * it needs to be forced out of guest mode. - */ - if (gpc->usage & KVM_GUEST_USES_PFN) { - if (!evict_vcpus) { - evict_vcpus = true; - bitmap_zero(vcpu_bitmap, KVM_MAX_VCPUS); - } - __set_bit(gpc->vcpu->vcpu_idx, vcpu_bitmap); - } } write_unlock_irq(&gpc->lock); } spin_unlock(&kvm->gpc_lock); - - if (evict_vcpus) { - /* - * KVM needs to ensure the vCPU is fully out of guest context - * before allowing the invalidation to continue. - */ - unsigned int req = KVM_REQ_OUTSIDE_GUEST_MODE; - bool called; - - /* - * If the OOM reaper is active, then all vCPUs should have - * been stopped already, so perform the request without - * KVM_REQUEST_WAIT and be sad if any needed to be IPI'd. - */ - if (!may_block) - req &= ~KVM_REQUEST_WAIT; - - called = kvm_make_vcpus_request_mask(kvm, req, vcpu_bitmap); - - WARN_ON_ONCE(called && !may_block); - } }

bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) @@ -206,16 +171,14 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * pfn. Note, kmap() and memremap() can both sleep, so this * too must be done outside of gpc->lock! */ - if (gpc->usage & KVM_HOST_USES_PFN) { - if (new_pfn == gpc->pfn) - new_khva = old_khva; - else - new_khva = gpc_map(new_pfn); - - if (!new_khva) { - kvm_release_pfn_clean(new_pfn); - goto out_error; - } + if (new_pfn == gpc->pfn) + new_khva = old_khva; + else + new_khva = gpc_map(new_pfn); + + if (!new_khva) { + kvm_release_pfn_clean(new_pfn); + goto out_error; }

write_lock_irq(&gpc->lock); @@ -346,18 +309,12 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len) return __kvm_gpc_refresh(gpc, gpc->gpa, len); }

-void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm, - struct kvm_vcpu *vcpu, enum pfn_cache_usage usage) +void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm) { - WARN_ON_ONCE(!usage || (usage & KVM_GUEST_AND_HOST_USE_PFN) != usage); - WARN_ON_ONCE((usage & KVM_GUEST_USES_PFN) && !vcpu); - rwlock_init(&gpc->lock); mutex_init(&gpc->refresh_lock);

gpc->kvm = kvm; - gpc->vcpu = vcpu; - gpc->usage = usage; gpc->pfn = KVM_PFN_ERR_FAULT; gpc->uhva = KVM_HVA_ERR_BAD; }

From: Paul Durrant pdurrant@amazon.com

Currently the pfncache page offset is sometimes determined using the gpa and sometimes the khva, whilst the uhva is always page-aligned. After a subsequent patch is applied the gpa will not always be valid so adjust the code to include the page offset in the uhva and use it consistently as the source of truth.

Also, where a page-aligned address is required, use PAGE_ALIGN_DOWN() for clarity.

No functional change intended.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: David Woodhouse dwmw2@infradead.org

v8: - New in this version. --- virt/kvm/pfncache.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 0eeb034d0674..97eec8ee3449 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -48,10 +48,10 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) if (!gpc->active) return false;

- if (offset_in_page(gpc->gpa) + len > PAGE_SIZE) + if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva)) return false;

- if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva)) + if (offset_in_page(gpc->uhva) + len > PAGE_SIZE) return false;

if (!gpc->valid) @@ -119,7 +119,7 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) { /* Note, the new page offset may be different than the old! */ - void *old_khva = gpc->khva - offset_in_page(gpc->khva); + void *old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva); kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT; void *new_khva = NULL; unsigned long mmu_seq; @@ -192,7 +192,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)

gpc->valid = true; gpc->pfn = new_pfn; - gpc->khva = new_khva + offset_in_page(gpc->gpa); + gpc->khva = new_khva + offset_in_page(gpc->uhva);

/* * Put the reference to the _new_ pfn. The pfn is now tracked by the @@ -217,6 +217,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, bool unmap_old = false; unsigned long old_uhva; kvm_pfn_t old_pfn; + bool hva_change = false; void *old_khva; int ret;

@@ -242,10 +243,10 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, }

old_pfn = gpc->pfn; - old_khva = gpc->khva - offset_in_page(gpc->khva); - old_uhva = gpc->uhva; + old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva); + old_uhva = PAGE_ALIGN_DOWN(gpc->uhva);

- /* If the userspace HVA is invalid, refresh that first */ + /* Refresh the userspace HVA if necessary */ if (gpc->gpa != gpa || gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva)) { gfn_t gfn = gpa_to_gfn(gpa); @@ -259,13 +260,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, ret = -EFAULT; goto out; } + + /* + * Even if the GPA and/or the memslot generation changed, the + * HVA may still be the same. + */ + if (gpc->uhva != old_uhva) + hva_change = true; + } else { + gpc->uhva = old_uhva; }

+ /* Note: the offset must be correct before calling hva_to_pfn_retry() */ + gpc->uhva += page_offset; + /* * If the userspace HVA changed or the PFN was already invalid, * drop the lock and do the HVA to PFN lookup again. */ - if (!gpc->valid || old_uhva != gpc->uhva) { + if (!gpc->valid || hva_change) { ret = hva_to_pfn_retry(gpc); } else { /*

From: Paul Durrant pdurrant@amazon.com

Some pfncache pages may actually be overlays on guest memory that have a fixed HVA within the VMM. It's pointless to invalidate such cached mappings if the overlay is moved so allow a cache to be activated directly with the HVA to cater for such cases. A subsequent patch will make use of this facility.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: David Woodhouse dwmw2@infradead.org

v13: - Define kvm_is_error_gpa() to check GPA validity. - Add an either/or address check to __kvm_gpc_refresh() as requested. - Make sure memslot is NULL if the cache is activated with an HVA.

v11: - Fixed kvm_gpc_check() to ignore memslot generation if the cache is not activated with a GPA. (This breakage occured during the re-work for v8).

v9: - Pass both GPA and HVA into __kvm_gpc_refresh() rather than overloading the address paraneter and using a bool flag to indicated what it is.

v8: - Re-worked to avoid messing with struct gfn_to_pfn_cache. --- include/linux/kvm_host.h | 21 +++++++++ virt/kvm/pfncache.c | 94 +++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 26 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 41ee515b304e..043cc824d55a 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -148,6 +148,11 @@ static inline bool kvm_is_error_hva(unsigned long addr)

#endif

+static inline bool kvm_is_error_gpa(gpa_t gpa) +{ + return gpa == INVALID_GPA; +} + #define KVM_ERR_PTR_BAD_PAGE (ERR_PTR(-ENOENT))

static inline bool is_error_page(struct page *page) @@ -1343,6 +1348,22 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm); */ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len);

+/** + * kvm_gpc_activate_hva - prepare a cached kernel mapping and HPA for a given HVA. + * + * @gpc: struct gfn_to_pfn_cache object. + * @hva: userspace virtual address to map. + * @len: sanity check; the range being access must fit a single page. + * + * @return: 0 for success. + * -EINVAL for a mapping which would cross a page boundary. + * -EFAULT for an untranslatable guest physical address. + * + * The semantics of this function are the same as those of kvm_gpc_activate(). It + * merely bypasses a layer of address translation. + */ +int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long hva, unsigned long len); + /** * kvm_gpc_check - check validity of a gfn_to_pfn_cache. * diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 97eec8ee3449..4e64d349b2f7 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -48,7 +48,14 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) if (!gpc->active) return false;

- if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva)) + /* + * If the page was cached from a memslot, make sure the memslots have + * not been re-configured. + */ + if (!kvm_is_error_gpa(gpc->gpa) && gpc->generation != slots->generation) + return false; + + if (kvm_is_error_hva(gpc->uhva)) return false;

if (offset_in_page(gpc->uhva) + len > PAGE_SIZE) @@ -209,11 +216,11 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) return -EFAULT; }

-static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, +static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva, unsigned long len) { - struct kvm_memslots *slots = kvm_memslots(gpc->kvm); - unsigned long page_offset = offset_in_page(gpa); + unsigned long page_offset = kvm_is_error_gpa(gpa) ? + offset_in_page(uhva) : offset_in_page(gpa); bool unmap_old = false; unsigned long old_uhva; kvm_pfn_t old_pfn; @@ -221,6 +228,10 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, void *old_khva; int ret;

+ /* Either gpa or uhva must be valid, but not both */ + if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva))) + return -EINVAL; + /* * If must fit within a single page. The 'len' argument is * only to enforce that. @@ -246,29 +257,39 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva); old_uhva = PAGE_ALIGN_DOWN(gpc->uhva);

- /* Refresh the userspace HVA if necessary */ - if (gpc->gpa != gpa || gpc->generation != slots->generation || - kvm_is_error_hva(gpc->uhva)) { - gfn_t gfn = gpa_to_gfn(gpa); - - gpc->gpa = gpa; - gpc->generation = slots->generation; - gpc->memslot = __gfn_to_memslot(slots, gfn); - gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn); + if (kvm_is_error_gpa(gpa)) { + gpc->gpa = INVALID_GPA; + gpc->memslot = NULL; + gpc->uhva = PAGE_ALIGN_DOWN(uhva);

- if (kvm_is_error_hva(gpc->uhva)) { - ret = -EFAULT; - goto out; - } - - /* - * Even if the GPA and/or the memslot generation changed, the - * HVA may still be the same. - */ if (gpc->uhva != old_uhva) hva_change = true; } else { - gpc->uhva = old_uhva; + struct kvm_memslots *slots = kvm_memslots(gpc->kvm); + + if (gpc->gpa != gpa || gpc->generation != slots->generation || + kvm_is_error_hva(gpc->uhva)) { + gfn_t gfn = gpa_to_gfn(gpa); + + gpc->gpa = gpa; + gpc->generation = slots->generation; + gpc->memslot = __gfn_to_memslot(slots, gfn); + gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn); + + if (kvm_is_error_hva(gpc->uhva)) { + ret = -EFAULT; + goto out; + } + + /* + * Even if the GPA and/or the memslot generation changed, the + * HVA may still be the same. + */ + if (gpc->uhva != old_uhva) + hva_change = true; + } else { + gpc->uhva = old_uhva; + } }

/* Note: the offset must be correct before calling hva_to_pfn_retry() */ @@ -319,7 +340,16 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,

int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len) { - return __kvm_gpc_refresh(gpc, gpc->gpa, len); + unsigned long uhva = gpc->uhva; + + /* + * If the GPA is valid then invalidate the HVA, otherwise + * __kvm_gpc_refresh() will fail its strict either/or address check. + */ + if (!kvm_is_error_gpa(gpc->gpa)) + uhva = KVM_HVA_ERR_BAD; + + return __kvm_gpc_refresh(gpc, gpc->gpa, uhva, len); }

void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm) @@ -329,10 +359,12 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)

gpc->kvm = kvm; gpc->pfn = KVM_PFN_ERR_FAULT; + gpc->gpa = INVALID_GPA; gpc->uhva = KVM_HVA_ERR_BAD; }

-int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len) +static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva, + unsigned long len) { struct kvm *kvm = gpc->kvm;

@@ -353,7 +385,17 @@ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len) gpc->active = true; write_unlock_irq(&gpc->lock); } - return __kvm_gpc_refresh(gpc, gpa, len); + return __kvm_gpc_refresh(gpc, gpa, uhva, len); +} + +int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len) +{ + return __kvm_gpc_activate(gpc, gpa, KVM_HVA_ERR_BAD, len); +} + +int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long uhva, unsigned long len) +{ + return __kvm_gpc_activate(gpc, INVALID_GPA, uhva, len); }

void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)

From: Paul Durrant pdurrant@amazon.com

If the shared_info PFN cache has already been initialized then the content of the shared_info page needs to be re-initialized whenever the guest mode is (re)set. Setting the guest mode is either done explicitly by the VMM via the KVM_XEN_ATTR_TYPE_LONG_MODE attribute, or implicitly when the guest writes the MSR to set up the hypercall page.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: David Woodhouse dwmw2@infradead.org Cc: x86@kernel.org

v13: - Patch title change.

v12: - Fix missing update of return value if mode is not actually changed.

v11: - Drop the hunk removing the call to kvm_xen_shared_info_init() when KVM_XEN_ATTR_TYPE_SHARED_INFO is set; it was a mistake and causes self- test failures.

v10: - New in this version. --- arch/x86/kvm/xen.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 031e98d88ba2..52edf676c471 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -625,8 +625,16 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) } else { mutex_lock(&kvm->arch.xen.xen_lock); kvm->arch.xen.long_mode = !!data->u.long_mode; + + /* + * Re-initialize shared_info to put the wallclock in the + * correct place. Whilst it's not necessary to do this + * unless the mode is actually changed, it does no harm + * to make the call anyway. + */ + r = kvm->arch.xen.shinfo_cache.active ? + kvm_xen_shared_info_init(kvm) : 0; mutex_unlock(&kvm->arch.xen.xen_lock); - r = 0; } break;

@@ -1101,9 +1109,24 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data) u32 page_num = data & ~PAGE_MASK; u64 page_addr = data & PAGE_MASK; bool lm = is_long_mode(vcpu); + int r = 0; + + mutex_lock(&kvm->arch.xen.xen_lock); + if (kvm->arch.xen.long_mode != lm) { + kvm->arch.xen.long_mode = lm; + + /* + * Re-initialize shared_info to put the wallclock in the + * correct place. + */ + if (kvm->arch.xen.shinfo_cache.active && + kvm_xen_shared_info_init(kvm)) + r = 1; + } + mutex_unlock(&kvm->arch.xen.xen_lock);

- /* Latch long_mode for shared_info pages etc. */ - vcpu->kvm->arch.xen.long_mode = lm; + if (r) + return r;

/* * If Xen hypercall intercept is enabled, fill the hypercall

From: Paul Durrant pdurrant@amazon.com

If the guest does not explicitly set the GPA of vcpu_info structure in memory then, for guests with 32 vCPUs or fewer, the vcpu_info embedded in the shared_info page may be used. As described in a previous commit, the shared_info page is an overlay at a fixed HVA within the VMM, so in this case it also more optimal to activate the vcpu_info cache with a fixed HVA to avoid unnecessary invalidation if the guest memory layout is modified.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: David Woodhouse dwmw2@infradead.org Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: x86@kernel.org

v13: - Patch title change.

v8: - Re-base.

v5: - New in this version. --- Documentation/virt/kvm/api.rst | 26 +++++++++++++++++++++----- arch/x86/kvm/xen.c | 34 ++++++++++++++++++++++++++++------ include/uapi/linux/kvm.h | 3 +++ 3 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 3372be85b335..bd93cafd3e4e 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -5523,11 +5523,12 @@ KVM_XEN_ATTR_TYPE_SHARED_INFO Sets the guest physical frame number at which the Xen shared_info page resides. Note that although Xen places vcpu_info for the first 32 vCPUs in the shared_info page, KVM does not automatically do so - and instead requires that KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO be used - explicitly even when the vcpu_info for a given vCPU resides at the - "default" location in the shared_info page. This is because KVM may - not be aware of the Xen CPU id which is used as the index into the - vcpu_info[] array, so may know the correct default location. + and instead requires that KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO or + KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA be used explicitly even when + the vcpu_info for a given vCPU resides at the "default" location + in the shared_info page. This is because KVM may not be aware of + the Xen CPU id which is used as the index into the vcpu_info[] + array, so may know the correct default location.

Note that the shared_info page may be constantly written to by KVM; it contains the event channel bitmap used to deliver interrupts to @@ -5649,6 +5650,21 @@ KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO on dirty logging. Setting the gpa to KVM_XEN_INVALID_GPA will disable the vcpu_info.

+KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA + If the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA flag is also set in the + Xen capabilities, then this attribute may be used to set the + userspace address of the vcpu_info for a given vCPU. It should + only be used when the vcpu_info resides at the "default" location + in the shared_info page. In this case it is safe to assume the + userspace address will not change, because the shared_info page is + an overlay on guest memory and remains at a fixed host address + regardless of where it is mapped in guest physical address space + and hence unnecessary invalidation of an internal cache may be + avoided if the guest memory layout is modified. + If the vcpu_info does not reside at the "default" location then + it is not guaranteed to remain at the same host address and + hence the aforementioned cache invalidation is required. + KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO Sets the guest physical address of an additional pvclock structure for a given vCPU. This is typically used for guest vsyscall support. diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 6fb268e424fa..4e79cc68e0a9 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -784,20 +784,33 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)

switch (data->type) { case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO: + case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA: /* No compat necessary here. */ BUILD_BUG_ON(sizeof(struct vcpu_info) != sizeof(struct compat_vcpu_info)); BUILD_BUG_ON(offsetof(struct vcpu_info, time) != offsetof(struct compat_vcpu_info, time));

- if (data->u.gpa == KVM_XEN_INVALID_GPA) { - kvm_gpc_deactivate(&vcpu->arch.xen.vcpu_info_cache); - r = 0; - break; + if (data->type == KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO) { + if (data->u.gpa == KVM_XEN_INVALID_GPA) { + kvm_gpc_deactivate(&vcpu->arch.xen.vcpu_info_cache); + r = 0; + break; + } + + r = kvm_gpc_activate(&vcpu->arch.xen.vcpu_info_cache, + data->u.gpa, sizeof(struct vcpu_info)); + } else { + if (data->u.hva == 0) { + kvm_gpc_deactivate(&vcpu->arch.xen.vcpu_info_cache); + r = 0; + break; + } + + r = kvm_gpc_activate_hva(&vcpu->arch.xen.vcpu_info_cache, + data->u.hva, sizeof(struct vcpu_info)); }

- r = kvm_gpc_activate(&vcpu->arch.xen.vcpu_info_cache, - data->u.gpa, sizeof(struct vcpu_info)); if (!r) kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

@@ -1026,6 +1039,15 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) r = 0; break;

+ case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA: + if (vcpu->arch.xen.vcpu_info_cache.active && + vcpu->arch.xen.vcpu_info_cache.gpa == KVM_XEN_INVALID_GPA) + data->u.hva = vcpu->arch.xen.vcpu_info_cache.uhva; + else + data->u.hva = 0; + r = 0; + break; + case KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO: if (vcpu->arch.xen.vcpu_time_info_cache.active) data->u.gpa = vcpu->arch.xen.vcpu_time_info_cache.gpa; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index ac5caba313d1..d2665319db6e 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1809,6 +1809,7 @@ struct kvm_xen_vcpu_attr { union { __u64 gpa; #define KVM_XEN_INVALID_GPA ((__u64)-1) + __u64 hva; __u64 pad[8]; struct { __u64 state; @@ -1839,6 +1840,8 @@ struct kvm_xen_vcpu_attr { #define KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID 0x6 #define KVM_XEN_VCPU_ATTR_TYPE_TIMER 0x7 #define KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR 0x8 +/* Available with KVM_CAP_XEN_HVM / KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA */ +#define KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA 0x9

/* Secure Encrypted Virtualization command */ enum sev_cmd_id {

From: Paul Durrant pdurrant@amazon.com

If the relevant capability (KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA) is present then re-map vcpu_info using the HVA part way through the tests to make sure then there is no functional change.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: David Woodhouse dwmw2@infradead.org

v13: - Patch title change.

v5: - New in this version. --- .../selftests/kvm/x86_64/xen_shinfo_test.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c index a61500ff0822..d2ea0435f4f7 100644 --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c @@ -62,6 +62,7 @@ enum { TEST_POLL_TIMEOUT, TEST_POLL_MASKED, TEST_POLL_WAKE, + SET_VCPU_INFO, TEST_TIMER_PAST, TEST_LOCKING_SEND_RACE, TEST_LOCKING_POLL_RACE, @@ -321,6 +322,10 @@ static void guest_code(void)

GUEST_SYNC(TEST_POLL_WAKE);

+ /* Set the vcpu_info to point at exactly the place it already is to + * make sure the attribute is functional. */ + GUEST_SYNC(SET_VCPU_INFO); + /* A timer wake an *unmasked* port which should wake us with an * actual interrupt, while we're polling on a different port. */ ports[0]++; @@ -888,6 +893,16 @@ int main(int argc, char *argv[]) alarm(1); break;

+ case SET_VCPU_INFO: + if (has_shinfo_hva) { + struct kvm_xen_vcpu_attr vih = { + .type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA, + .u.hva = (unsigned long)vinfo + }; + vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vih); + } + break; + case TEST_TIMER_PAST: TEST_ASSERT(!evtchn_irq_expected, "Expected event channel IRQ but it didn't happen");

From: Paul Durrant pdurrant@amazon.com

The implementation of kvm_xen_set_evtchn_fast() is a rather lengthy piece of code that performs two operations: updating of the shared_info evtchn_pending mask, and updating of the vcpu_info evtchn_pending_sel mask. Introduce a separate function to perform each of those operations and re-work kvm_xen_set_evtchn_fast() to use them.

No functional change intended.

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: David Woodhouse dwmw2@infradead.org Cc: x86@kernel.org

v13: - Patch title change.

v11: - Fixed /64 vs /32 switcheroo and changed type of port_word_bit back to int.

v10: - Updated in this version. Dropped David'd R-b since the updates are non-trivial.

v8: - New in this version. --- arch/x86/kvm/xen.c | 175 ++++++++++++++++++++++++++------------------- 1 file changed, 100 insertions(+), 75 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 4e79cc68e0a9..59073642c078 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -1670,6 +1670,101 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port) } }

+static int set_shinfo_evtchn_pending(struct kvm_vcpu *vcpu, u32 port) +{ + struct kvm *kvm = vcpu->kvm; + struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache; + unsigned long *pending_bits, *mask_bits; + unsigned long flags; + int rc = -EWOULDBLOCK; + + read_lock_irqsave(&gpc->lock, flags); + if (!kvm_gpc_check(gpc, PAGE_SIZE)) + goto out; + + if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) { + struct shared_info *shinfo = gpc->khva; + + pending_bits = (unsigned long *)&shinfo->evtchn_pending; + mask_bits = (unsigned long *)&shinfo->evtchn_mask; + } else { + struct compat_shared_info *shinfo = gpc->khva; + + pending_bits = (unsigned long *)&shinfo->evtchn_pending; + mask_bits = (unsigned long *)&shinfo->evtchn_mask; + } + + if (test_and_set_bit(port, pending_bits)) { + rc = 0; /* It was already raised */ + } else if (test_bit(port, mask_bits)) { + rc = -ENOTCONN; /* It is masked */ + kvm_xen_check_poller(vcpu, port); + } else { + rc = 1; /* It is newly raised */ + } + + out: + read_unlock_irqrestore(&gpc->lock, flags); + return rc; +} + +static bool set_vcpu_info_evtchn_pending(struct kvm_vcpu *vcpu, u32 port) +{ + struct kvm *kvm = vcpu->kvm; + struct gfn_to_pfn_cache *gpc = &vcpu->arch.xen.vcpu_info_cache; + unsigned long flags; + bool kick_vcpu = false; + + read_lock_irqsave(&gpc->lock, flags); + + /* + * Try to deliver the event directly to the vcpu_info. If successful and + * the guest is using upcall_vector delivery, send the MSI. + * If the pfncache is invalid, set the shadow. In this case, or if the + * guest is using another form of event delivery, the vCPU must be + * kicked to complete the delivery. + */ + if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) { + struct vcpu_info *vcpu_info = gpc->khva; + int port_word_bit = port / 64; + + if (!kvm_gpc_check(gpc, sizeof(*vcpu_info))) { + if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel)) + kick_vcpu = true; + goto out; + } + + if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) { + WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1); + kick_vcpu = true; + } + } else { + struct compat_vcpu_info *vcpu_info = gpc->khva; + int port_word_bit = port / 32; + + if (!kvm_gpc_check(gpc, sizeof(*vcpu_info))) { + if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel)) + kick_vcpu = true; + goto out; + } + + if (!test_and_set_bit(port_word_bit, + (unsigned long *)&vcpu_info->evtchn_pending_sel)) { + WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1); + kick_vcpu = true; + } + } + + if (kick_vcpu && vcpu->arch.xen.upcall_vector) { + kvm_xen_inject_vcpu_vector(vcpu); + kick_vcpu = false; + } + + out: + read_unlock_irqrestore(&gpc->lock, flags); + return kick_vcpu; +} + /* * The return value from this function is propagated to kvm_set_irq() API, * so it returns: @@ -1678,15 +1773,12 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port) * > 0 Number of CPUs interrupt was delivered to * * It is also called directly from kvm_arch_set_irq_inatomic(), where the - * only check on its return value is a comparison with -EWOULDBLOCK'. + * only check on its return value is a comparison with -EWOULDBLOCK + * (which may be returned by set_shinfo_evtchn_pending()). */ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm) { - struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache; struct kvm_vcpu *vcpu; - unsigned long *pending_bits, *mask_bits; - unsigned long flags; - int port_word_bit; bool kick_vcpu = false; int vcpu_idx, idx, rc;

@@ -1706,79 +1798,12 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm) if (xe->port >= max_evtchn_port(kvm)) return -EINVAL;

- rc = -EWOULDBLOCK; - idx = srcu_read_lock(&kvm->srcu);

- read_lock_irqsave(&gpc->lock, flags); - if (!kvm_gpc_check(gpc, PAGE_SIZE)) - goto out_rcu; - - if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) { - struct shared_info *shinfo = gpc->khva; - pending_bits = (unsigned long *)&shinfo->evtchn_pending; - mask_bits = (unsigned long *)&shinfo->evtchn_mask; - port_word_bit = xe->port / 64; - } else { - struct compat_shared_info *shinfo = gpc->khva; - pending_bits = (unsigned long *)&shinfo->evtchn_pending; - mask_bits = (unsigned long *)&shinfo->evtchn_mask; - port_word_bit = xe->port / 32; - } + rc = set_shinfo_evtchn_pending(vcpu, xe->port); + if (rc == 1) /* Delivered to the bitmap in shared_info */ + kick_vcpu = set_vcpu_info_evtchn_pending(vcpu, xe->port);

- /* - * If this port wasn't already set, and if it isn't masked, then - * we try to set the corresponding bit in the in-kernel shadow of - * evtchn_pending_sel for the target vCPU. And if *that* wasn't - * already set, then we kick the vCPU in question to write to the - * *real* evtchn_pending_sel in its own guest vcpu_info struct. - */ - if (test_and_set_bit(xe->port, pending_bits)) { - rc = 0; /* It was already raised */ - } else if (test_bit(xe->port, mask_bits)) { - rc = -ENOTCONN; /* Masked */ - kvm_xen_check_poller(vcpu, xe->port); - } else { - rc = 1; /* Delivered to the bitmap in shared_info. */ - /* Now switch to the vCPU's vcpu_info to set the index and pending_sel */ - read_unlock_irqrestore(&gpc->lock, flags); - gpc = &vcpu->arch.xen.vcpu_info_cache; - - read_lock_irqsave(&gpc->lock, flags); - if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) { - /* - * Could not access the vcpu_info. Set the bit in-kernel - * and prod the vCPU to deliver it for itself. - */ - if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel)) - kick_vcpu = true; - goto out_rcu; - } - - if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) { - struct vcpu_info *vcpu_info = gpc->khva; - if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) { - WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1); - kick_vcpu = true; - } - } else { - struct compat_vcpu_info *vcpu_info = gpc->khva; - if (!test_and_set_bit(port_word_bit, - (unsigned long *)&vcpu_info->evtchn_pending_sel)) { - WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1); - kick_vcpu = true; - } - } - - /* For the per-vCPU lapic vector, deliver it as MSI. */ - if (kick_vcpu && vcpu->arch.xen.upcall_vector) { - kvm_xen_inject_vcpu_vector(vcpu); - kick_vcpu = false; - } - } - - out_rcu: - read_unlock_irqrestore(&gpc->lock, flags); srcu_read_unlock(&kvm->srcu, idx);

if (kick_vcpu) {

From: Paul Durrant pdurrant@amazon.com

When processing mmu_notifier invalidations for gpc caches, pre-check for overlap with the invalidation event while holding gpc->lock for read, and only take gpc->lock for write if the cache needs to be invalidated. Doing a pre-check without taking gpc->lock for write avoids unnecessarily contending the lock for unrelated invalidations, which is very beneficial for caches that are heavily used (but rarely subjected to mmu_notifier invalidations).

Signed-off-by: Paul Durrant pdurrant@amazon.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com Cc: David Woodhouse dwmw2@infradead.org

v13: - Use Sean's preferred wording for the commit comment.

v10: - New in this version. --- virt/kvm/pfncache.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 4e64d349b2f7..a60d8f906896 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -29,14 +29,30 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,

spin_lock(&kvm->gpc_lock); list_for_each_entry(gpc, &kvm->gpc_list, list) { - write_lock_irq(&gpc->lock); + read_lock_irq(&gpc->lock);

/* Only a single page so no need to care about length */ if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) && gpc->uhva >= start && gpc->uhva < end) { - gpc->valid = false; + read_unlock_irq(&gpc->lock); + + /* + * There is a small window here where the cache could + * be modified, and invalidation would no longer be + * necessary. Hence check again whether invalidation + * is still necessary once the write lock has been + * acquired. + */ + + write_lock_irq(&gpc->lock); + if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) && + gpc->uhva >= start && gpc->uhva < end) + gpc->valid = false; + write_unlock_irq(&gpc->lock); + continue; } - write_unlock_irq(&gpc->lock); + + read_unlock_irq(&gpc->lock); } spin_unlock(&kvm->gpc_lock); }

From: David Woodhouse dwmw@amazon.co.uk

This function can race with kvm_gpc_deactivate(), which does not take the ->refresh_lock. This means kvm_gpc_deactivate() can wipe the ->pfn and ->khva fields, and unmap the latter, while hva_to_pfn_retry() has temporarily dropped its write lock on gpc->lock.

Then if hva_to_pfn_retry() determines that the PFN hasn't changed and that the original pfn and khva can be reused, they get assigned back to gpc->pfn and gpc->khva even though the khva was already unmapped by kvm_gpc_deactivate(). This leaves the cache in an apparently valid state but with ->khva pointing to an address which has been unmapped. Which in turn leads to oopses in e.g. __kvm_xen_has_interrupt() and set_shinfo_evtchn_pending() when they dereference said khva.

It may be possible to fix this just by making kvm_gpc_deactivate() take the ->refresh_lock, but that still leaves ->refresh_lock being basically redundant with the write lock on ->lock, which frankly makes my skin itch, with the way that pfn_to_hva_retry() operates on fields in the gpc without holding a write lock on ->lock.

Instead, fix it by cleaning up the semantics of hva_to_pfn_retry(). It now no longer does locking gymnastics because it no longer operates on the gpc object at all. I's called with a uhva and simply returns the corresponding pfn (pinned), and a mapped khva for it.

Its caller __kvm_gpc_refresh() now sets gpc->uhva and clears gpc->valid before dropping ->lock, calling hva_to_pfn_retry() and retaking ->lock for write.

If hva_to_pfn_retry() fails, *or* if the ->uhva or ->active fields in the gpc changed while the lock was dropped, the new mapping is discarded and the gpc is not modified. On success with an unchanged gpc, the new mapping is installed and the current ->pfn and ->uhva are taken into the local old_pfn and old_khva variables to be unmapped once the locks are all released.

This simplification means that ->refresh_lock is no longer needed for correctness, but it does still provide a minor optimisation because it will prevent two concurrent __kvm_gpc_refresh() calls from mapping a given PFN, only for one of them to lose the race and discard its mapping.

The optimisation in hva_to_pfn_retry() where it attempts to use the old mapping if the pfn doesn't change is dropped, since it makes the pinning more complex. It's a pointless optimisation anyway, since the odds of the pfn ending up the same when the uhva has changed (i.e. the odds of the two userspace addresses both pointing to the same underlying physical page) are negligible,

The 'hva_changed' local variable in __kvm_gpc_refresh() is also removed, since it's simpler just to clear gpc->valid if the uhva changed. Likewise the unmap_old variable is dropped because it's just as easy to check the old_pfn variable for KVM_PFN_ERR_FAULT.

I remain slightly confused because although this is clearly a race in the gfn_to_pfn_cache code, I don't quite know how the Xen support code actually managed to trigger it. We've seen oopses from dereferencing a valid-looking ->khva in both __kvm_xen_has_interrupt() (the vcpu_info) and in set_shinfo_evtchn_pending() (the shared_info). But surely the race shouldn't happen for the vcpu_info gpc because all calls to both refresh and deactivate hold the vcpu mutex, and it shouldn't happen for the shared_info gpc because all calls to both will hold the kvm->arch.xen.xen_lock mutex.

Signed-off-by: David Woodhouse dwmw@amazon.co.uk Reviewed-by: Paul Durrant paul@xen.org --- Cc: Sean Christopherson seanjc@google.com Cc: Paolo Bonzini pbonzini@redhat.com

v12: - New in this version. --- virt/kvm/pfncache.c | 184 +++++++++++++++++++++----------------------- 1 file changed, 88 insertions(+), 96 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index a60d8f906896..d1c73b452570 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -139,108 +139,65 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s return kvm->mmu_invalidate_seq != mmu_seq; }

-static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) +/* + * Given a user virtual address, obtain a pinned host PFN and kernel mapping + * for it. The caller will release the PFN after installing it into the GPC + * so that the MMU notifier invalidation mechanism is active. + */ +static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, unsigned long uhva, + kvm_pfn_t *pfn, void **khva) { /* Note, the new page offset may be different than the old! */ - void *old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva); kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT; void *new_khva = NULL; unsigned long mmu_seq;

- lockdep_assert_held(&gpc->refresh_lock); - - lockdep_assert_held_write(&gpc->lock); - - /* - * Invalidate the cache prior to dropping gpc->lock, the gpa=>uhva - * assets have already been updated and so a concurrent check() from a - * different task may not fail the gpa/uhva/generation checks. - */ - gpc->valid = false; - - do { - mmu_seq = gpc->kvm->mmu_invalidate_seq; + for (;;) { + mmu_seq = kvm->mmu_invalidate_seq; smp_rmb();

- write_unlock_irq(&gpc->lock); - - /* - * If the previous iteration "failed" due to an mmu_notifier - * event, release the pfn and unmap the kernel virtual address - * from the previous attempt. Unmapping might sleep, so this - * needs to be done after dropping the lock. Opportunistically - * check for resched while the lock isn't held. - */ - if (new_pfn != KVM_PFN_ERR_FAULT) { - /* - * Keep the mapping if the previous iteration reused - * the existing mapping and didn't create a new one. - */ - if (new_khva != old_khva) - gpc_unmap(new_pfn, new_khva); - - kvm_release_pfn_clean(new_pfn); - - cond_resched(); - } - /* We always request a writeable mapping */ - new_pfn = hva_to_pfn(gpc->uhva, false, false, NULL, true, NULL); + new_pfn = hva_to_pfn(uhva, false, false, NULL, true, NULL); if (is_error_noslot_pfn(new_pfn)) - goto out_error; + return -EFAULT;

/* - * Obtain a new kernel mapping if KVM itself will access the - * pfn. Note, kmap() and memremap() can both sleep, so this - * too must be done outside of gpc->lock! + * Always obtain a new kernel mapping. Trying to reuse an + * existing one is more complex than it's worth. */ - if (new_pfn == gpc->pfn) - new_khva = old_khva; - else - new_khva = gpc_map(new_pfn); - + new_khva = gpc_map(new_pfn); if (!new_khva) { kvm_release_pfn_clean(new_pfn); - goto out_error; + return -EFAULT; }

- write_lock_irq(&gpc->lock); + if (!mmu_notifier_retry_cache(kvm, mmu_seq)) + break;

/* - * Other tasks must wait for _this_ refresh to complete before - * attempting to refresh. + * If this iteration "failed" due to an mmu_notifier event, + * release the pfn and unmap the kernel virtual address, and + * loop around again. */ - WARN_ON_ONCE(gpc->valid); - } while (mmu_notifier_retry_cache(gpc->kvm, mmu_seq)); - - gpc->valid = true; - gpc->pfn = new_pfn; - gpc->khva = new_khva + offset_in_page(gpc->uhva); + if (new_pfn != KVM_PFN_ERR_FAULT) { + gpc_unmap(new_pfn, new_khva); + kvm_release_pfn_clean(new_pfn); + } + }

- /* - * Put the reference to the _new_ pfn. The pfn is now tracked by the - * cache and can be safely migrated, swapped, etc... as the cache will - * invalidate any mappings in response to relevant mmu_notifier events. - */ - kvm_release_pfn_clean(new_pfn); + *pfn = new_pfn; + *khva = new_khva;

return 0; - -out_error: - write_lock_irq(&gpc->lock); - - return -EFAULT; }

-static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva, - unsigned long len) +static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, + unsigned long uhva, unsigned long len) { unsigned long page_offset = kvm_is_error_gpa(gpa) ? offset_in_page(uhva) : offset_in_page(gpa); - bool unmap_old = false; unsigned long old_uhva; - kvm_pfn_t old_pfn; - bool hva_change = false; + kvm_pfn_t old_pfn = KVM_PFN_ERR_FAULT; void *old_khva; int ret;

@@ -257,8 +214,9 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l

/* * If another task is refreshing the cache, wait for it to complete. - * There is no guarantee that concurrent refreshes will see the same - * gpa, memslots generation, etc..., so they must be fully serialized. + * This is purely an optimisation, to avoid concurrent mappings from + * hva_to_pfn_retry(), all but one of which will be discarded after + * losing a race to install them in the GPC. */ mutex_lock(&gpc->refresh_lock);

@@ -279,7 +237,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l gpc->uhva = PAGE_ALIGN_DOWN(uhva);

if (gpc->uhva != old_uhva) - hva_change = true; + gpc->valid = false; } else { struct kvm_memslots *slots = kvm_memslots(gpc->kvm);

@@ -294,7 +252,11 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l

if (kvm_is_error_hva(gpc->uhva)) { ret = -EFAULT; - goto out; + + gpc->valid = false; + gpc->pfn = KVM_PFN_ERR_FAULT; + gpc->khva = NULL; + goto out_unlock; }

/* @@ -302,7 +264,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l * HVA may still be the same. */ if (gpc->uhva != old_uhva) - hva_change = true; + gpc->valid = false; } else { gpc->uhva = old_uhva; } @@ -315,9 +277,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l * If the userspace HVA changed or the PFN was already invalid, * drop the lock and do the HVA to PFN lookup again. */ - if (!gpc->valid || hva_change) { - ret = hva_to_pfn_retry(gpc); - } else { + if (gpc->valid) { /* * If the HVA→PFN mapping was already valid, don't unmap it. * But do update gpc->khva because the offset within the page @@ -325,30 +285,62 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l */ gpc->khva = old_khva + page_offset; ret = 0; - goto out_unlock; - }

- out: - /* - * Invalidate the cache and purge the pfn/khva if the refresh failed. - * Some/all of the uhva, gpa, and memslot generation info may still be - * valid, leave it as is. - */ - if (ret) { + /* old_pfn must not be unmapped because it was reused. */ + old_pfn = KVM_PFN_ERR_FAULT; + } else { + kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT; + unsigned long new_uhva = gpc->uhva; + void *new_khva = NULL; + + /* + * Invalidate the cache prior to dropping gpc->lock; the + * gpa=>uhva assets have already been updated and so a + * concurrent check() from a different task may not fail + * the gpa/uhva/generation checks as it should. + */ gpc->valid = false; - gpc->pfn = KVM_PFN_ERR_FAULT; - gpc->khva = NULL; - }

- /* Detect a pfn change before dropping the lock! */ - unmap_old = (old_pfn != gpc->pfn); + write_unlock_irq(&gpc->lock); + + ret = hva_to_pfn_retry(gpc->kvm, new_uhva, &new_pfn, &new_khva); + + write_lock_irq(&gpc->lock); + + WARN_ON_ONCE(gpc->valid); + + if (ret || !gpc->active || gpc->uhva != new_uhva) { + /* + * On failure or if another change occurred while the + * lock was dropped, just purge the new mapping. + */ + old_pfn = new_pfn; + old_khva = new_khva; + } else { + old_pfn = gpc->pfn; + old_khva = gpc->khva; + + gpc->pfn = new_pfn; + gpc->khva = new_khva + offset_in_page(gpc->uhva); + gpc->valid = true; + } + + /* + * Put the reference to the _new_ pfn. On success, the + * pfn is now tracked by the cache and can safely be + * migrated, swapped, etc. as the cache will invalidate + * any mappings in response to relevant mmu_notifier + * events. + */ + kvm_release_pfn_clean(new_pfn); + }

out_unlock: write_unlock_irq(&gpc->lock);

mutex_unlock(&gpc->refresh_lock);

- if (unmap_old) + if (old_pfn != KVM_PFN_ERR_FAULT) gpc_unmap(old_pfn, old_khva);

return ret;