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: "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>,
	"Vitaly Kuznetsov" <vkuznets@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 17/19] KVM: Terminate memslot walks via used_slots
Date: Fri, 7 Feb 2020 13:10:16 -0800
Message-ID: <20200207211016.GN2401@linux.intel.com> (raw)
In-Reply-To: <20200207203909.GE720553@xz-x1>

On Fri, Feb 07, 2020 at 03:39:09PM -0500, Peter Xu wrote:
> On Fri, Feb 07, 2020 at 10:33:25AM -0800, Sean Christopherson wrote:
> > On Thu, Feb 06, 2020 at 04:09:44PM -0500, Peter Xu wrote:
> > > On Tue, Jan 21, 2020 at 02:31:55PM -0800, Sean Christopherson wrote:
> > > > @@ -9652,13 +9652,13 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
> > > >  		if (IS_ERR((void *)hva))
> > > >  			return PTR_ERR((void *)hva);
> > > >  	} else {
> > > > -		if (!slot->npages)
> > > > +		if (!slot || !slot->npages)
> > > >  			return 0;
> > > >  
> > > > -		hva = 0;
> > > > +		hva = slot->userspace_addr;
> > > 
> > > Is this intended?
> > 
> > Yes.  It's possible to allow VA=0 for userspace mappings.  It's extremely
> > uncommon, but possible.  Therefore "hva == 0" shouldn't be used to
> > indicate an invalid slot.
> 
> Note that this is the deletion path in __x86_set_memory_region() not
> allocation.  IIUC userspace_addr won't even be used in follow up code
> path so it shouldn't really matter.  Or am I misunderstood somewhere?

No, but that's precisely why I don't want to zero out @hva, as doing so
implies that '0' indicates an invalid hva, which is wrong.

What if I change this to 

			hva = 0xdeadull << 48;

and add a blurb in the changelog about stuff hva with a non-canonical value
to indicate it's being destroyed.

> > > > +		old_npages = slot->npages;
> > > >  	}
> > > >  
> > > > -	old = *slot;
> > > >  	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
> > > >  		struct kvm_userspace_memory_region m;
> > > >  

...

> > > > +{
> > > > +	struct kvm_memory_slot *mslots = slots->memslots;
> > > > +	int i;
> > > > +
> > > > +	if (WARN_ON_ONCE(slots->id_to_index[memslot->id] == -1) ||
> > > > +	    WARN_ON_ONCE(!slots->used_slots))
> > > > +		return -1;
> > > > +
> > > > +	/*
> > > > +	 * Move the target memslot backward in the array by shifting existing
> > > > +	 * memslots with a higher GFN (than the target memslot) towards the
> > > > +	 * front of the array.
> > > > +	 */
> > > > +	for (i = slots->id_to_index[memslot->id]; i < slots->used_slots - 1; i++) {
> > > > +		if (memslot->base_gfn > mslots[i + 1].base_gfn)
> > > > +			break;
> > > > +
> > > > +		WARN_ON_ONCE(memslot->base_gfn == mslots[i + 1].base_gfn);
> > > 
> > > Will this trigger?  Note that in __kvm_set_memory_region() we have
> > > already checked overlap of memslots.
> > 
> > If you screw up the code it will :-)  In a perfect world, no WARN() will
> > *ever* trigger.  All of the added WARN_ON_ONCE() are to help the next poor
> > soul that wants to modify this code.
> 
> I normally won't keep WARN_ON if it is 100% not triggering (100% here
> I mean when e.g. it is checked twice so the 1st one will definitely
> trigger first).  My question is more like a pure question in case I
> overlooked something.  Please also feel free to keep it if you want.

Ah.  The WARNs here as much to concisely document the assumptions and
conditions of the code as they are there to enforce those conditions.

> > > > +
> > > > +		/* Shift the next memslot forward one and update its index. */
> > > > +		mslots[i] = mslots[i + 1];
s> > > > +		slots->id_to_index[mslots[i].id] = i;
> > > > +	}
> > > > +	return i;
> > > > +}
> > > > @@ -1104,8 +1203,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > 
> > ...
> > 
> > > >  	 * when the memslots are re-sorted by update_memslots().
> > > >  	 */
> > > >  	tmp = id_to_memslot(__kvm_memslots(kvm, as_id), id);
> > > > -	old = *tmp;
> > > > -	tmp = NULL;
> > > 
> > > I was confused in that patch, then...
> > > 
> > > > +	if (tmp) {
> > > > +		old = *tmp;
> > > > +		tmp = NULL;
> > > 
> > > ... now I still don't know why it needs to set to NULL?
> > 
> > To make it abundantly clear that though shall not use @tmp, i.e. to force
> > using the copy and not the pointer.  Note, @tmp is also reused as an
> > iterator below.
> 
> OK it still feels a bit strange, say, we can comment on that if you
> wants to warn the others.  The difference is probably no useless
> instruction executed.  But this is also trivial, I'll leave to the
> others to judge.

After having suffered through deciphering this code and blundering into
nasty gotchas more than once, I'd really like to keep the nullification.
I'll add a comment to explain that the sole purpose is to kill @tmp so it
can't be used incorrectly and thus cause silent failure.

This is also another reason I'd like to keep the WARN_ONs.  When this code
goes awry, the result is usually silent corruption and delayed explosions,
i.e. failures that absolutely suck to debug.

  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
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 [this message]
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=20200207211016.GN2401@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