KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Wanpeng Li" <wanpengli@tencent.com>,
	kvm@vger.kernel.org, "David Hildenbrand" <david@redhat.com>,
	"James Hogan" <jhogan@kernel.org>,
	linux-mips@vger.kernel.org, "Paul Mackerras" <paulus@ozlabs.org>,
	kvmarm@lists.cs.columbia.edu,
	"Janosch Frank" <frankja@linux.ibm.com>,
	"Marc Zyngier" <maz@kernel.org>, "Joerg Roedel" <joro@8bytes.org>,
	kvm-ppc@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	"Jim Mattson" <jmattson@google.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	linux-kernel@vger.kernel.org,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Vitaly Kuznetsov" <vkuznets@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PATCH v4 19/19] KVM: selftests: Add test for KVM_SET_USER_MEMORY_REGION
Date: Tue, 24 Mar 2020 11:20:48 -0700
Message-ID: <20200324182048.GF5998@linux.intel.com> (raw)
In-Reply-To: <78b21097-52e4-b851-fc78-da3442fd0904@de.ibm.com>

On Tue, Mar 24, 2020 at 10:43:07AM +0100, Christian Borntraeger wrote:
> 
> On 18.12.19 17:39, Sean Christopherson wrote:
> > On Wed, Dec 18, 2019 at 12:39:43PM +0100, Christian Borntraeger wrote:
> >>
> I started looking into this what it would cost to implement this on s390.
> s390 is also returning EFAULT if no memory slot is available.
> 
> According to the doc this is not documented at all. So this part of the test
>         vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
>         vm_vcpu_add(vm, VCPU_ID);
>         /* Fails with ENOSPC because the MMU can't create pages (no slots). */
>         TEST_ASSERT(_vcpu_run(vm, VCPU_ID) == -1 && errno == ENOSPC,
>                     "Unexpected error code = %d", errno);
>         kvm_vm_free(vm);
> 
> is actually just testing that the implementation for x86 does not change the error
> from ENOSPC to something else.

It's even worse than that.  There error isn't directly due to no having
a memslots, it occurs because the limit on number of pages in the MMU is
zero.  On x86, that limit is automatically derived from the total size of
memslots.

The selftest could add an explicit ioctl() call to manually override the
number of allowed MMU pages, but that didn't seem any cleaner as it'd still
rely on undocumented internal KVM behavior.

TL;DR: I'm not a huge fan of the code either.

> The question is: do we want to document the error for the "no memslot" case
> and then change all existing platforms?

At first blush, I like the idea of adding an explicit check in KVM_RUN to
return an error if there isn't at least one usable memslot.  But, it'd be a
little weird/kludgy on x86/VMX due to the existence of "private" memslots,
i.e. the check should really fire on "no public memslots".  At that point,
I'm not sure whether the well defined errno would be worth the extra code.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 40+ 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 21:56   ` Peter Xu
2019-12-17 22:20     ` Sean Christopherson
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
2020-03-24  9:43       ` Christian Borntraeger
2020-03-24 18:20         ` Sean Christopherson [this message]
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 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=20200324182048.GF5998@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --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=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 ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/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 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


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