From: Paul Durrant <[email protected]>
Some cached 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 <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---
Cc: Sean Christopherson <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: David Woodhouse <[email protected]>
---
include/linux/kvm_host.h | 29 ++++++++++++++++
include/linux/kvm_types.h | 3 +-
virt/kvm/pfncache.c | 73 ++++++++++++++++++++++++++++-----------
3 files changed, 84 insertions(+), 21 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4d8027fe9928..6823bae5c66c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1321,6 +1321,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.
*
@@ -1378,9 +1394,22 @@ void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc);
* kvm_gpc_gpa - retrieve the guest physical address of a cached mapping
*
* @gpc: struct gfn_to_pfn_cache object.
+ *
+ * @return: If the cache was activated with a fixed HVA then INVALID_GPA
+ * will be returned.
*/
gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc);
+/**
+ * kvm_gpc_hva - retrieve the fixed host physical address of a cached mapping
+ *
+ * @gpc: struct gfn_to_pfn_cache object.
+ *
+ * @return: If the cache was activated with a guest physical address then
+ * 0 will be returned.
+ */
+unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc);
+
void kvm_sigset_activate(struct kvm_vcpu *vcpu);
void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index 6f4737d5046a..d49946ee7ae3 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -64,7 +64,7 @@ struct gfn_to_hva_cache {
struct gfn_to_pfn_cache {
u64 generation;
- gpa_t gpa;
+ u64 addr;
unsigned long uhva;
struct kvm_memory_slot *memslot;
struct kvm *kvm;
@@ -77,6 +77,7 @@ struct gfn_to_pfn_cache {
enum pfn_cache_usage usage;
bool active;
bool valid;
+ bool addr_is_gpa;
};
#ifdef KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 37bcb4399780..b3e3f7e38410 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -83,7 +83,7 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
if (!gpc->active)
return false;
- if (gpc->generation != slots->generation)
+ if (gpc->addr_is_gpa && gpc->generation != slots->generation)
return false;
if (kvm_is_error_hva(gpc->uhva))
@@ -229,7 +229,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 + (gpc->gpa & ~PAGE_MASK);
+ gpc->khva = new_khva + (gpc->addr & ~PAGE_MASK);
/*
* Put the reference to the _new_ pfn. The pfn is now tracked by the
@@ -246,11 +246,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,
- unsigned long len)
+static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, u64 addr,
+ unsigned long len, bool addr_is_gpa)
{
struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
- unsigned long page_offset = gpa & ~PAGE_MASK;
+ unsigned long page_offset = addr & ~PAGE_MASK;
bool unmap_old = false;
unsigned long old_uhva;
kvm_pfn_t old_pfn;
@@ -282,22 +282,34 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
old_khva = gpc->khva - offset_in_page(gpc->khva);
old_uhva = gpc->uhva;
- /* If the userspace HVA is invalid, refresh that first */
- if (gpc->gpa != gpa || gpc->generation != slots->generation ||
+ /*
+ * If the address has changed, switched from guest to host (or vice
+ * versa), or it's a guest address and the memory slots have been
+ * updated, we need to refresh the userspace HVA.
+ */
+ if (gpc->addr != addr ||
+ gpc->addr_is_gpa != addr_is_gpa ||
+ (addr_is_gpa && gpc->generation != slots->generation) ||
kvm_is_error_hva(gpc->uhva)) {
- gfn_t gfn = gpa_to_gfn(gpa);
+ gpc->addr = addr;
+ gpc->addr_is_gpa = addr_is_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 (addr_is_gpa) {
+ gfn_t gfn = gpa_to_gfn(addr);
- if (kvm_is_error_hva(gpc->uhva)) {
- ret = -EFAULT;
- goto out;
+ gpc->generation = slots->generation;
+ gpc->memslot = __gfn_to_memslot(slots, gfn);
+ gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn);
+ } else {
+ gpc->uhva = addr & PAGE_MASK;
}
}
+ if (kvm_is_error_hva(gpc->uhva)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
/*
* If the userspace HVA changed or the PFN was already invalid,
* drop the lock and do the HVA to PFN lookup again.
@@ -343,7 +355,7 @@ 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);
+ return __kvm_gpc_refresh(gpc, gpc->addr, len, gpc->addr_is_gpa);
}
EXPORT_SYMBOL_GPL(kvm_gpc_refresh);
@@ -364,7 +376,8 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm,
}
EXPORT_SYMBOL_GPL(kvm_gpc_init);
-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, u64 addr, unsigned long len,
+ bool addr_is_gpa)
{
struct kvm *kvm = gpc->kvm;
@@ -385,19 +398,39 @@ 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, addr, len, addr_is_gpa);
+}
+
+int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
+{
+ return __kvm_gpc_activate(gpc, gpa, len, true);
}
EXPORT_SYMBOL_GPL(kvm_gpc_activate);
gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc)
{
- return gpc->gpa;
+ return gpc->addr_is_gpa ? gpc->addr : INVALID_GPA;
}
EXPORT_SYMBOL_GPL(kvm_gpc_gpa);
+int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long hva, unsigned long len)
+{
+ return __kvm_gpc_activate(gpc, hva, len, false);
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_activate_hva);
+
+unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc)
+{
+ return !gpc->addr_is_gpa ? gpc->addr : 0;
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_hva);
+
void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
{
- mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+ if (!gpc->addr_is_gpa)
+ return;
+
+ mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->addr >> PAGE_SHIFT);
}
EXPORT_SYMBOL_GPL(kvm_gpc_mark_dirty);
--
2.39.2
On Mon, Oct 02, 2023, Paul Durrant wrote:
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index 6f4737d5046a..d49946ee7ae3 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -64,7 +64,7 @@ struct gfn_to_hva_cache {
>
> struct gfn_to_pfn_cache {
> u64 generation;
> - gpa_t gpa;
> + u64 addr;
Holy moly, we have unions for exactly this reason.
union {
gpa_t gpa;
unsigned long addr;
};
But that's also weird and silly because it's basically the exact same thing as
"uhva". If "uhva" stores the full address instead of the page-aligned address,
then I don't see a need for unionizing the gpa and uhva.
kvm_xen_vcpu_get_attr() should darn well explicitly check that the gpc stores
the correct type and not bleed ABI into the gfn_to_pfn_cache implementation.
If there's a true need for a union, the helpers should WARN.
> +unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc)
> +{
> + return !gpc->addr_is_gpa ? gpc->addr : 0;
'0' is a perfectly valid address. Yeah, practically speaking '0' can't be used
these days, but we already have KVM_HVA_ERR_BAD. If y'all want to use the for the
Xen ABI, then so be it. But the common helpers need to use a sane value.
On 31/10/2023 23:49, Sean Christopherson wrote:
> On Mon, Oct 02, 2023, Paul Durrant wrote:
>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>> index 6f4737d5046a..d49946ee7ae3 100644
>> --- a/include/linux/kvm_types.h
>> +++ b/include/linux/kvm_types.h
>> @@ -64,7 +64,7 @@ struct gfn_to_hva_cache {
>>
>> struct gfn_to_pfn_cache {
>> u64 generation;
>> - gpa_t gpa;
>> + u64 addr;
>
> Holy moly, we have unions for exactly this reason.
>
> union {
> gpa_t gpa;
> unsigned long addr;
> };
>
> But that's also weird and silly because it's basically the exact same thing as
> "uhva". If "uhva" stores the full address instead of the page-aligned address,
> then I don't see a need for unionizing the gpa and uhva.
>
Ok, I think that'll be more invasive but I'll see how it looks.
> kvm_xen_vcpu_get_attr() should darn well explicitly check that the gpc stores
> the correct type and not bleed ABI into the gfn_to_pfn_cache implementation.
>
I guess if we leave gpa alone and make it INVALID_GPA for caches
initialized using an HVA then that can be checked. Is that what you mean
here?
> If there's a true need for a union, the helpers should WARN.
>
>> +unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc)
>> +{
>> + return !gpc->addr_is_gpa ? gpc->addr : 0;
>
> '0' is a perfectly valid address. Yeah, practically speaking '0' can't be used
> these days, but we already have KVM_HVA_ERR_BAD. If y'all want to use the for the
> Xen ABI, then so be it. But the common helpers need to use a sane value.
Ok.
Paul
On Thu, Nov 02, 2023, Paul Durrant wrote:
> On 31/10/2023 23:49, Sean Christopherson wrote:
> > On Mon, Oct 02, 2023, Paul Durrant wrote:
> > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > index 6f4737d5046a..d49946ee7ae3 100644
> > > --- a/include/linux/kvm_types.h
> > > +++ b/include/linux/kvm_types.h
> > > @@ -64,7 +64,7 @@ struct gfn_to_hva_cache {
> > > struct gfn_to_pfn_cache {
> > > u64 generation;
> > > - gpa_t gpa;
> > > + u64 addr;
> >
> > Holy moly, we have unions for exactly this reason.
> >
> > union {
> > gpa_t gpa;
> > unsigned long addr;
> > };
> >
> > But that's also weird and silly because it's basically the exact same thing as
> > "uhva". If "uhva" stores the full address instead of the page-aligned address,
> > then I don't see a need for unionizing the gpa and uhva.
>
> Ok, I think that'll be more invasive but I'll see how it looks.
Invasive is fine. Not ideal, but fine. If the resulting code is a mess, then
that's a problem, but churn in and of itself isn't awful if the end result is a
net positive.
> > kvm_xen_vcpu_get_attr() should darn well explicitly check that the gpc stores
> > the correct type and not bleed ABI into the gfn_to_pfn_cache implementation.
>
> I guess if we leave gpa alone and make it INVALID_GPA for caches initialized
> using an HVA then that can be checked. Is that what you mean here?
Yep, that should work.