Messages in this thread | | | From | Paul Durrant <> | Date | Thu, 2 Nov 2023 17:52:02 +0000 | Subject | Re: [PATCH v7 02/11] KVM: pfncache: add a mark-dirty helper |
| |
On 31/10/2023 23:28, Sean Christopherson wrote: > On Mon, Oct 02, 2023, Paul Durrant wrote: >> From: Paul Durrant <pdurrant@amazon.com> >> >> 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. >> >> NOTE: Pages are now marked dirty while the cache lock is held. This is >> to ensure that gpa and memslot are mutually consistent. > > This absolutely belongs in a separate patch. It sounds like a bug fix (haven't > spent the time to figure out if it actually is), and even if it doesn't fix > anything, burying something like this in a "add a helper" patch is just mean. >
Ok, I can split it out. It's a pretty minor fix so didn't seem worth it.
> >> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c >> index 0f36acdf577f..b68ed7fa56a2 100644 >> --- a/virt/kvm/pfncache.c >> +++ b/virt/kvm/pfncache.c >> @@ -386,6 +386,12 @@ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len) >> } >> EXPORT_SYMBOL_GPL(kvm_gpc_activate); >> >> +void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc) >> +{ > > If there's actually a reason to call mark_page_dirty_in_slot() while holding @gpc's > lock, then this should have a lockdep. If there's no good reason, then don't move > the invocation. > >> + mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT); >> +} >> +EXPORT_SYMBOL_GPL(kvm_gpc_mark_dirty); > > This doesn't need to be exported. Hrm, none of the exports in this file are > necessary, they likely all got added when we were thinking this stuff would be > used for nVMX. I think we should remove them, not because I'm worried about > sub-modules doing bad things, but just because we should avoid polluting exported > symbols as much as possible.
That in a separate clean-up patch too, I assume?
Paul
| |