From: Paul Durrant <[email protected]>
This series has one small fix to what was in v11 [1]:
* KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
The v11 patch failed to set the return code of the ioctl if the mode
was not actually changed, leading to a spurious failure.
This version of the series also contains a new bug-fix to the pfncache
code from David Woodhouse.
[1] https://lore.kernel.org/kvm/[email protected]/
David Woodhouse (1):
KVM: pfncache: rework __kvm_gpc_refresh() to fix locking issues
Paul Durrant (19):
KVM: pfncache: Add a map helper function
KVM: pfncache: remove unnecessary exports
KVM: xen: mark guest pages dirty with the pfncache lock held
KVM: pfncache: add a mark-dirty helper
KVM: pfncache: remove KVM_GUEST_USES_PFN usage
KVM: pfncache: stop open-coding offset_in_page()
KVM: pfncache: include page offset in uhva and use it consistently
KVM: pfncache: allow a cache to be activated with a fixed (userspace)
HVA
KVM: xen: separate initialization of shared_info cache and content
KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
KVM: xen: allow shared_info to be mapped by fixed HVA
KVM: xen: allow vcpu_info to be mapped by fixed HVA
KVM: selftests / xen: map shared_info using HVA rather than GFN
KVM: selftests / xen: re-map vcpu_info using HVA rather than GPA
KVM: xen: advertize the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA capability
KVM: xen: split up kvm_xen_set_evtchn_fast()
KVM: xen: don't block on pfncache locks in kvm_xen_set_evtchn_fast()
KVM: pfncache: check the need for invalidation under read lock first
KVM: xen: allow vcpu_info content to be 'safely' copied
Documentation/virt/kvm/api.rst | 53 ++-
arch/x86/kvm/x86.c | 7 +-
arch/x86/kvm/xen.c | 356 +++++++++++-------
include/linux/kvm_host.h | 40 +-
include/linux/kvm_types.h | 8 -
include/uapi/linux/kvm.h | 9 +-
.../selftests/kvm/x86_64/xen_shinfo_test.c | 59 ++-
virt/kvm/pfncache.c | 340 +++++++++--------
8 files changed, 535 insertions(+), 337 deletions(-)
base-commit: 1c6d984f523f67ecfad1083bb04c55d91977bb15
--
2.39.2
From: Paul Durrant <[email protected]>
Some code in pfncache uses offset_in_page() but in other places it is open-
coded. Use offset_in_page() consistently everywhere.
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]>
v8:
- New in this version.
---
virt/kvm/pfncache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 6f4b537eb25b..0eeb034d0674 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -48,7 +48,7 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
if (!gpc->active)
return false;
- if ((gpc->gpa & ~PAGE_MASK) + len > PAGE_SIZE)
+ if (offset_in_page(gpc->gpa) + len > PAGE_SIZE)
return false;
if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva))
@@ -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 + (gpc->gpa & ~PAGE_MASK);
+ gpc->khva = new_khva + offset_in_page(gpc->gpa);
/*
* Put the reference to the _new_ pfn. The pfn is now tracked by the
@@ -213,7 +213,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
unsigned long len)
{
struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
- unsigned long page_offset = gpa & ~PAGE_MASK;
+ unsigned long page_offset = offset_in_page(gpa);
bool unmap_old = false;
unsigned long old_uhva;
kvm_pfn_t old_pfn;
--
2.39.2
From: Paul Durrant <[email protected]>
At the moment pages are marked dirty by open-coded calls to
mark_page_dirty_in_slot(), directly deferefencing the gpa and memslot
from the cache. After a subsequent patch these may not always be set
so add a helper now so that caller will protected from the need to know
about this detail.
Signed-off-by: Paul Durrant <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---
Cc: David Woodhouse <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
v8:
- Make the helper a static inline.
---
arch/x86/kvm/x86.c | 2 +-
arch/x86/kvm/xen.c | 6 +++---
include/linux/kvm_host.h | 11 +++++++++++
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 27e23714e960..0a0ac91a494f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3156,7 +3156,7 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
guest_hv_clock->version = ++vcpu->hv_clock.version;
- mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+ kvm_gpc_mark_dirty(gpc);
read_unlock_irqrestore(&gpc->lock, flags);
trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index b63bf54bb376..34c48d0029c1 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -453,11 +453,11 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
}
if (user_len2) {
- mark_page_dirty_in_slot(v->kvm, gpc2->memslot, gpc2->gpa >> PAGE_SHIFT);
+ kvm_gpc_mark_dirty(gpc2);
read_unlock(&gpc2->lock);
}
- mark_page_dirty_in_slot(v->kvm, gpc1->memslot, gpc1->gpa >> PAGE_SHIFT);
+ kvm_gpc_mark_dirty(gpc1);
read_unlock_irqrestore(&gpc1->lock, flags);
}
@@ -565,7 +565,7 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
WRITE_ONCE(vi->evtchn_upcall_pending, 1);
}
- mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+ kvm_gpc_mark_dirty(gpc);
read_unlock_irqrestore(&gpc->lock, flags);
/* For the per-vCPU lapic vector, deliver it as MSI. */
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7e7fd25b09b3..f3bb9e0a81fe 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1399,6 +1399,17 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
*/
void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
+/**
+ * kvm_gpc_mark_dirty - mark a cached page as dirty.
+ *
+ * @gpc: struct gfn_to_pfn_cache object.
+ */
+static inline void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
+{
+ lockdep_assert_held(&gpc->lock);
+ mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+}
+
void kvm_sigset_activate(struct kvm_vcpu *vcpu);
void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
--
2.39.2
On 15/01/2024 12:56, Paul Durrant wrote:
> From: Paul Durrant <[email protected]>
>
> This series has one small fix to what was in v11 [1]:
>
> * KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
>
> The v11 patch failed to set the return code of the ioctl if the mode
> was not actually changed, leading to a spurious failure.
>
> This version of the series also contains a new bug-fix to the pfncache
> code from David Woodhouse.
>
> [1] https://lore.kernel.org/kvm/[email protected]/
>
Ping?
On Thu, Jan 25, 2024, Paul Durrant wrote:
> On 15/01/2024 12:56, Paul Durrant wrote:
> > From: Paul Durrant <[email protected]>
> >
> > This series has one small fix to what was in v11 [1]:
> >
> > * KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
> >
> > The v11 patch failed to set the return code of the ioctl if the mode
> > was not actually changed, leading to a spurious failure.
> >
> > This version of the series also contains a new bug-fix to the pfncache
> > code from David Woodhouse.
> >
> > [1] https://lore.kernel.org/kvm/[email protected]/
> >
>
> Ping?
Sorry, I have done basically zero upstream reviews over the last few weeks, for
a variety of reasons. Unless yet another thing pops up, I expect to dive into
upstream reviews tomorrow and spend a good long while there.
On Thu, 2024-01-25 at 15:03 +0000, Paul Durrant wrote:
> On 15/01/2024 12:56, Paul Durrant wrote:
> > From: Paul Durrant <[email protected]>
> >
> > This series has one small fix to what was in v11 [1]:
> >
> > * KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
> >
> > The v11 patch failed to set the return code of the ioctl if the mode
> > was not actually changed, leading to a spurious failure.
> >
> > This version of the series also contains a new bug-fix to the pfncache
> > code from David Woodhouse.
> >
> > [1] https://lore.kernel.org/kvm/[email protected]/
> >
>
> Ping?
>
I think it's only the final patch which is controversial, isn't it? We
can drop that for now and I can submit it under separate cover.
It'll be basically the same patch, just won't claim to be a bug fix:
https://lore.kernel.org/kvm/[email protected]/
Although I'll eat my hat if that "should never happen" bug actually
does keep happening after the locking is less baroque.
On 26/01/2024 01:19, Sean Christopherson wrote:
> On Thu, Jan 25, 2024, Paul Durrant wrote:
>> On 15/01/2024 12:56, Paul Durrant wrote:
>>> From: Paul Durrant <[email protected]>
>>>
>>> This series has one small fix to what was in v11 [1]:
>>>
>>> * KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
>>>
>>> The v11 patch failed to set the return code of the ioctl if the mode
>>> was not actually changed, leading to a spurious failure.
>>>
>>> This version of the series also contains a new bug-fix to the pfncache
>>> code from David Woodhouse.
>>>
>>> [1] https://lore.kernel.org/kvm/[email protected]/
>>>
>>
>> Ping?
>
> Sorry, I have done basically zero upstream reviews over the last few weeks, for
> a variety of reasons. Unless yet another thing pops up, I expect to dive into
> upstream reviews tomorrow and spend a good long while there.
Hi Sean,
Have you had any time to take a look at this?
Paul
On Fri, Feb 02, 2024, Paul Durrant wrote:
> On 26/01/2024 01:19, Sean Christopherson wrote:
> > On Thu, Jan 25, 2024, Paul Durrant wrote:
> > > On 15/01/2024 12:56, Paul Durrant wrote:
> > > > From: Paul Durrant <[email protected]>
> > > >
> > > > This series has one small fix to what was in v11 [1]:
> > > >
> > > > * KVM: xen: re-initialize shared_info if guest (32/64-bit) mode is set
> > > >
> > > > The v11 patch failed to set the return code of the ioctl if the mode
> > > > was not actually changed, leading to a spurious failure.
> > > >
> > > > This version of the series also contains a new bug-fix to the pfncache
> > > > code from David Woodhouse.
> > > >
> > > > [1] https://lore.kernel.org/kvm/[email protected]/
> > > >
> > >
> > > Ping?
> >
> > Sorry, I have done basically zero upstream reviews over the last few weeks, for
> > a variety of reasons. Unless yet another thing pops up, I expect to dive into
> > upstream reviews tomorrow and spend a good long while there.
>
> Hi Sean,
>
> Have you had any time to take a look at this?
No, I was hoping to get to it today, but that isn't happening. It's next in my
queue after David Steven's "KVM: allow mapping non-refcounted pages" serie", so
hopefully Monday or Tuesday will be the day.
On Mon, Jan 15, 2024, Paul Durrant wrote:
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7e7fd25b09b3..f3bb9e0a81fe 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1399,6 +1399,17 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
> */
> void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
>
> +/**
> + * kvm_gpc_mark_dirty - mark a cached page as dirty.
> + *
> + * @gpc: struct gfn_to_pfn_cache object.
> + */
> +static inline void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
Any objection to kvm_gpc_mark_dirty_in_slot()? I want to make it clear this only
marks the gfn dirty in the memslot, i.e. that it doesn't mark the underlying page
as dirty.
On 07/02/2024 03:20, Sean Christopherson wrote:
> On Mon, Jan 15, 2024, Paul Durrant wrote:
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7e7fd25b09b3..f3bb9e0a81fe 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1399,6 +1399,17 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
>> */
>> void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
>>
>> +/**
>> + * kvm_gpc_mark_dirty - mark a cached page as dirty.
>> + *
>> + * @gpc: struct gfn_to_pfn_cache object.
>> + */
>> +static inline void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
>
> Any objection to kvm_gpc_mark_dirty_in_slot()? I want to make it clear this only
> marks the gfn dirty in the memslot, i.e. that it doesn't mark the underlying page
> as dirty.
Ok, that sounds fair.
Paul
On Mon, Jan 15, 2024, Paul Durrant wrote:
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7e7fd25b09b3..f3bb9e0a81fe 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1399,6 +1399,17 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
> */
> void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
>
> +/**
> + * kvm_gpc_mark_dirty - mark a cached page as dirty.
> + *
> + * @gpc: struct gfn_to_pfn_cache object.
> + */
> +static inline void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
> +{
> + lockdep_assert_held(&gpc->lock);
> + mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
Can you opportunistically have this pre-check gpc->memslot? __kvm_gpc_refresh()
should nullify gpc->memslot when using an hva. That way, you don't need to
explicitly check for the "invalid gfn" case here (or you could, but WARN_ON_ONCE()
if the memslot is non-NULL and the gfn is invalid?).
On 09/02/2024 15:58, Sean Christopherson wrote:
> On Mon, Jan 15, 2024, Paul Durrant wrote:
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7e7fd25b09b3..f3bb9e0a81fe 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1399,6 +1399,17 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
>> */
>> void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
>>
>> +/**
>> + * kvm_gpc_mark_dirty - mark a cached page as dirty.
>> + *
>> + * @gpc: struct gfn_to_pfn_cache object.
>> + */
>> +static inline void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
>> +{
>> + lockdep_assert_held(&gpc->lock);
>> + mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
>
> Can you opportunistically have this pre-check gpc->memslot? __kvm_gpc_refresh()
> should nullify gpc->memslot when using an hva. That way, you don't need to
> explicitly check for the "invalid gfn" case here (or you could, but WARN_ON_ONCE()
> if the memslot is non-NULL and the gfn is invalid?).
Ok, sure.