Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Peter Xu <peterx@redhat.com>
Cc: "Vitaly Kuznetsov" <vkuznets@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Paul Mackerras" <paulus@ozlabs.org>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"Janosch Frank" <frankja@linux.ibm.com>,
	"David Hildenbrand" <david@redhat.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	"Wanpeng Li" <wanpengli@tencent.com>,
	"Jim Mattson" <jmattson@google.com>,
	"Joerg Roedel" <joro@8bytes.org>, "Marc Zyngier" <maz@kernel.org>,
	"James Morse" <james.morse@arm.com>,
	"Julien Thierry" <julien.thierry.kdev@gmail.com>,
	"Suzuki K Poulose" <suzuki.poulose@arm.com>,
	linux-mips@vger.kernel.org, kvm@vger.kernel.org,
	kvm-ppc@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
	"Christoffer Dall" <christoffer.dall@arm.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PATCH v5 15/19] KVM: Provide common implementation for generic dirty log functions
Date: Fri, 7 Feb 2020 17:29:38 -0800
Message-ID: <20200208012938.GC15581@linux.intel.com> (raw)
In-Reply-To: <20200208005334.GB823968@xz-x1>

On Fri, Feb 07, 2020 at 07:53:34PM -0500, Peter Xu wrote:
> On Fri, Feb 07, 2020 at 04:42:33PM -0800, Sean Christopherson wrote:
> > On Fri, Feb 07, 2020 at 07:18:32PM -0500, Peter Xu wrote:
> > > On Fri, Feb 07, 2020 at 11:45:32AM -0800, Sean Christopherson wrote:
> > > > +Vitaly for HyperV
> > > > 
> > > > On Thu, Feb 06, 2020 at 04:41:06PM -0500, Peter Xu wrote:
> > > > > On Thu, Feb 06, 2020 at 01:21:20PM -0800, Sean Christopherson wrote:
> > > > > > On Thu, Feb 06, 2020 at 03:02:00PM -0500, Peter Xu wrote:
> > > > > > > But that matters to this patch because if MIPS can use
> > > > > > > kvm_flush_remote_tlbs(), then we probably don't need this
> > > > > > > arch-specific hook any more and we can directly call
> > > > > > > kvm_flush_remote_tlbs() after sync dirty log when flush==true.
> > > > > > 
> > > > > > Ya, the asid_flush_mask in kvm_vz_flush_shadow_all() is the only thing
> > > > > > that prevents calling kvm_flush_remote_tlbs() directly, but I have no
> > > > > > clue as to the important of that code.
> > > > > 
> > > > > As said above I think the x86 lockdep is really not necessary, then
> > > > > considering MIPS could be the only one that will use the new hook
> > > > > introduced in this patch...  Shall we figure that out first?
> > > > 
> > > > So I prepped a follow-up patch to make kvm_arch_dirty_log_tlb_flush() a
> > > > MIPS-only hook and use kvm_flush_remote_tlbs() directly for arm and x86,
> > > > but then I realized x86 *has* a hook to do a precise remote TLB flush.
> > > > There's even an existing kvm_flush_remote_tlbs_with_address() call on a
> > > > memslot, i.e. this exact scenario.  So arguably, x86 should be using the
> > > > more precise flush and should keep kvm_arch_dirty_log_tlb_flush().
> > > > 
> > > > But, the hook is only used when KVM is running as an L1 on top of HyperV,
> > > > and I assume dirty logging isn't used much, if at all, for L1 KVM on
> > > > HyperV?
> > > > 
> > > > I see three options:
> > > > 
> > > >   1. Make kvm_arch_dirty_log_tlb_flush() MIPS-only and call
> > > >      kvm_flush_remote_tlbs() directly for arm and x86.  Add comments to
> > > >      explain when an arch should implement kvm_arch_dirty_log_tlb_flush().
> > > > 
> > > >   2. Change x86 to use kvm_flush_remote_tlbs_with_address() when flushing
> > > >      a memslot after the dirty log is grabbed by userspace.
> > > > 
> > > >   3. Keep the resulting code as is, but add a comment in x86's
> > > >      kvm_arch_dirty_log_tlb_flush() to explain why it uses
> > > >      kvm_flush_remote_tlbs() instead of the with_address() variant.
> > > > 
> > > > I strongly prefer to (2) or (3), but I'll defer to Vitaly as to which of
> > > > those is preferable.
> > > > 
> > > > I don't like (1) because (a) it requires more lines code (well comments),
> > > > to explain why kvm_flush_remote_tlbs() is the default, and (b) it would
> > > > require even more comments, which would be x86-specific in generic KVM,
> > > > to explain why x86 doesn't use its with_address() flush, or we'd lost that
> > > > info altogether.
> > > > 
> > > 
> > > I proposed the 4th solution here:
> > > 
> > > https://lore.kernel.org/kvm/20200207223520.735523-1-peterx@redhat.com/
> > > 
> > > I'm not sure whether that's acceptable, but if it can, then we can
> > > drop the kvm_arch_dirty_log_tlb_flush() hook, or even move on to
> > > per-slot tlb flushing.
> > 
> > This effectively is per-slot TLB flushing, it just has a different name.
> > I.e. s/kvm_arch_dirty_log_tlb_flush/kvm_arch_flush_remote_tlbs_memslot.
> > I'm not opposed to that name change.  And on second and third glance, I
> > probably prefer it.  That would more or less follow the naming of
> > kvm_arch_flush_shadow_all() and kvm_arch_flush_shadow_memslot().
> 
> Note that the major point of the above patchset is not about doing tlb
> flush per-memslot or globally.  It's more about whether we can provide
> a common entrance for TLB flushing.  Say, after that series, we should
> be able to flush TLB on all archs (majorly, including MIPS) as:
> 
>   kvm_flush_remote_tlbs(kvm);
> 
> And with the same idea we can also introduce the ranged version.
> 
> > 
> > I don't want to go straight to kvm_arch_flush_remote_tlb_with_address()
> > because that loses the important distinction (on x86) that slots_lock is
> > expected to be held.
> 
> Sorry I'm still puzzled on why that lockdep is so important and
> special for x86...  For example, what if we move that lockdep to the
> callers of the kvm_arch_dirty_log_tlb_flush() calls so it protects
> even more arch (where we do get/clear dirty log)?  IMHO the callers
> must be with the slots_lock held anyways no matter for x86 or not.


Following the breadcrumbs leads to the comment in
kvm_mmu_slot_remove_write_access(), which says:

        /*
         * kvm_mmu_slot_remove_write_access() and kvm_vm_ioctl_get_dirty_log()
         * which do tlb flush out of mmu-lock should be serialized by
         * kvm->slots_lock otherwise tlb flush would be missed.
         */

I.e. write-protecting a memslot and grabbing the dirty log for the memslot
need to be serialized.  It's quite obvious *now* that get_dirty_log() holds
slots_lock, but the purpose of lockdep assertions isn't just to verify the
current functionality, it's to help ensure the correctness for future code
and to document assumptions in the code.

Digging deeper, there are four functions, all related to dirty logging, in
the x86 mmu that basically open code what x86's
kvm_arch_flush_remote_tlbs_memslot() would look like if it uses the range
based flushing.

Unless it's functionally incorrect (Vitaly?), going with option (2) and
naming the hook kvm_arch_flush_remote_tlbs_memslot() seems like the obvious
choice, e.g. the final cleanup gives this diff stat:

 arch/x86/kvm/mmu/mmu.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

  reply index

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-21 22:31 [PATCH v5 00/19] KVM: Dynamically size memslot arrays Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 01/19] KVM: x86: Allocate new rmap and large page tracking when moving memslot Sean Christopherson
2020-02-05 21:49   ` Peter Xu
2020-02-05 23:55     ` Sean Christopherson
2020-02-06  2:00       ` Peter Xu
2020-02-06  2:17         ` Sean Christopherson
2020-02-06  2:58           ` Peter Xu
2020-02-06  5:05             ` Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 02/19] KVM: Reinstall old memslots if arch preparation fails Sean Christopherson
2020-02-05 22:08   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 03/19] KVM: Don't free new memslot if allocation of said memslot fails Sean Christopherson
2020-02-05 22:28   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 04/19] KVM: PPC: Move memslot memory allocation into prepare_memory_region() Sean Christopherson
2020-02-05 22:41   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 05/19] KVM: x86: Allocate memslot resources during prepare_memory_region() Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 06/19] KVM: Drop kvm_arch_create_memslot() Sean Christopherson
2020-02-05 22:45   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 07/19] KVM: Explicitly free allocated-but-unused dirty bitmap Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 08/19] KVM: Refactor error handling for setting memory region Sean Christopherson
2020-02-05 22:48   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 09/19] KVM: Move setting of memslot into helper routine Sean Christopherson
2020-02-06 16:26   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 10/19] KVM: Drop "const" attribute from old memslot in commit_memory_region() Sean Christopherson
2020-02-06 16:26   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 11/19] KVM: x86: Free arrays for old memslot when moving memslot's base gfn Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 12/19] KVM: Move memslot deletion to helper function Sean Christopherson
2020-02-06 16:14   ` Peter Xu
2020-02-06 16:28     ` Sean Christopherson
2020-02-06 16:51       ` Peter Xu
2020-02-07 17:59         ` Sean Christopherson
2020-02-07 18:07           ` Sean Christopherson
2020-02-07 18:17           ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 13/19] KVM: Simplify kvm_free_memslot() and all its descendents Sean Christopherson
2020-02-06 16:29   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 14/19] KVM: Clean up local variable usage in __kvm_set_memory_region() Sean Christopherson
2020-02-06 19:06   ` Peter Xu
2020-02-06 19:22     ` Sean Christopherson
2020-02-06 19:36       ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 15/19] KVM: Provide common implementation for generic dirty log functions Sean Christopherson
2020-02-06 20:02   ` Peter Xu
2020-02-06 21:21     ` Sean Christopherson
2020-02-06 21:41       ` Peter Xu
2020-02-07 19:45         ` Sean Christopherson
2020-02-08  0:18           ` Peter Xu
2020-02-08  0:42             ` Sean Christopherson
2020-02-08  0:53               ` Peter Xu
2020-02-08  1:29                 ` Sean Christopherson [this message]
2020-02-17 15:39                   ` Vitaly Kuznetsov
2020-02-18 17:10                     ` Sean Christopherson
2020-02-17 15:35           ` Vitaly Kuznetsov
2020-02-06 21:24   ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 16/19] KVM: Ensure validity of memslot with respect to kvm_get_dirty_log() Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 17/19] KVM: Terminate memslot walks via used_slots Sean Christopherson
2020-02-06 21:09   ` Peter Xu
2020-02-07 18:33     ` Sean Christopherson
2020-02-07 20:39       ` Peter Xu
2020-02-07 21:10         ` Sean Christopherson
2020-02-07 21:46           ` Peter Xu
2020-02-07 22:03             ` Sean Christopherson
2020-02-07 22:24               ` Peter Xu
2020-01-21 22:31 ` [PATCH v5 18/19] KVM: Dynamically size memslot array based on number of used slots Sean Christopherson
2020-02-06 22:12   ` Peter Xu
2020-02-07 15:38     ` Sean Christopherson
2020-02-07 16:05       ` Peter Xu
2020-02-07 16:15         ` Sean Christopherson
2020-02-07 16:37           ` Peter Xu
2020-02-07 16:47             ` Sean Christopherson
2020-01-21 22:31 ` [PATCH v5 19/19] KVM: selftests: Add test for KVM_SET_USER_MEMORY_REGION Sean Christopherson
2020-02-06 22:30   ` Peter Xu
2020-02-06 23:11     ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200208012938.GC15581@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=borntraeger@de.ibm.com \
    --cc=christoffer.dall@arm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=f4bug@amsat.org \
    --cc=frankja@linux.ibm.com \
    --cc=james.morse@arm.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=julien.thierry.kdev@gmail.com \
    --cc=kvm-ppc@vger.kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=paulus@ozlabs.org \
    --cc=pbonzini@redhat.com \
    --cc=peterx@redhat.com \
    --cc=suzuki.poulose@arm.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git