Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Tue, 31 Oct 2023 16:28:16 -0700
In-Reply-To: <20231002095740.1472907-3-paul@xen.org>
Mime-Version: 1.0
References: <20231002095740.1472907-1-paul@xen.org> <20231002095740.1472907-3-paul@xen.org>
Message-ID: <ZUGNkCljRm5VXcGg@google.com>
Subject: Re: [PATCH v7 02/11] KVM: pfncache: add a mark-dirty helper
From:   Sean Christopherson <seanjc@google.com>
To:     Paul Durrant <paul@xen.org>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paul Durrant <pdurrant@amazon.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        David Woodhouse <dwmw2@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.


> 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.