| Message ID | 20240115125707.1183-5-paul@xen.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: xen: update shared_info and vcpu_info handling | expand |
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.
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);