KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Peter Xu <peterx@redhat.com>
Cc: "James Hogan" <jhogan@kernel.org>,
	"Paul Mackerras" <paulus@ozlabs.org>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"Janosch Frank" <frankja@linux.ibm.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc Zyngier" <maz@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	"Wanpeng Li" <wanpengli@tencent.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	kvm@vger.kernel.org, "David Hildenbrand" <david@redhat.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	"Cornelia Huck" <cohuck@redhat.com>,
	linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm-ppc@vger.kernel.org, "Vitaly Kuznetsov" <vkuznets@redhat.com>,
	kvmarm@lists.cs.columbia.edu, "Jim Mattson" <jmattson@google.com>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v4 01/19] KVM: x86: Allocate new rmap and large page tracking when moving memslot
Date: Tue, 17 Dec 2019 14:20:59 -0800
Message-ID: <20191217222058.GD11771@linux.intel.com> (raw)
In-Reply-To: <20191217215640.GI7258@xz-x1>

On Tue, Dec 17, 2019 at 04:56:40PM -0500, Peter Xu wrote:
> On Tue, Dec 17, 2019 at 12:40:23PM -0800, Sean Christopherson wrote:
> > Reallocate a rmap array and recalcuate large page compatibility when
> > moving an existing memslot to correctly handle the alignment properties
> > of the new memslot.  The number of rmap entries required at each level
> > is dependent on the alignment of the memslot's base gfn with respect to
> > that level, e.g. moving a large-page aligned memslot so that it becomes
> > unaligned will increase the number of rmap entries needed at the now
> > unaligned level.

...

> I think the error-prone part is:
> 
> 	new = old = *slot;

Lol, IMO the error-prone part is the entire memslot mess :-)

> Where IMHO it would be better if we only copy pointers explicitly when
> under control, rather than blindly copying all the pointers in the
> structure which even contains sub-structures.

Long term, yes, that would be ideal.  For the immediate bug fix, reworking
common KVM and other arch code would be unnecessarily dangerous and would
make it more difficult to backport the fix to stable branches.

I actually briefly considered moving the slot->arch handling into arch
code as part of the bug fix, but the memslot code has many subtle
dependencies, e.g. PPC and x86 rely on common KVM code to copy slot->arch
when flags are being changed.

I'll happily clean up the slot->arch code once this series is merged.
There is refactoring in this series that will make it a lot easier to do
additional clean up.

> For example, I see PPC has this:
> 
> struct kvm_arch_memory_slot {
> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> 	unsigned long *rmap;
> #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
> };
> 
> I started to look into HV code of it a bit, then I see...
> 
>  - kvm_arch_create_memslot(kvmppc_core_create_memslot_hv) init slot->arch.rmap,
>  - kvm_arch_flush_shadow_memslot(kvmppc_core_flush_memslot_hv) didn't free it,
>  - kvm_arch_prepare_memory_region(kvmppc_core_prepare_memory_region_hv) is nop.
> 
> So Does it have similar issue?

No, KVM doesn't allow a memslot's size to be changed, and PPC's rmap
allocation is directly tied to the size of the memslot.  The x86 bug exists
because the size of its metadata arrays varies based on the alignment of
the base gfn.

  reply index

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

Reply instructions:

You may reply publically 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=20191217222058.GD11771@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=david@redhat.com \
    --cc=f4bug@amsat.org \
    --cc=frankja@linux.ibm.com \
    --cc=jhogan@kernel.org \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --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=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

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/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 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


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