KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Orr <marcorr@google.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: kvm@vger.kernel.org, Jim Mattson <jmattson@google.com>,
	Peter Shier <pshier@google.com>
Subject: Re: [kvm-unit-tests PATCH] x86: nvmx: test max atomic switch MSRs
Date: Fri, 13 Sep 2019 14:55:56 -0700
Message-ID: <CAA03e5E28QaDAHjCg5J0_aPoY8pNnUiUQVvrZSHsEj0dq6-q7w@mail.gmail.com> (raw)
In-Reply-To: <20190913183040.GA8904@linux.intel.com>

Thanks for the review! I'll get to work on v2 now.

> > I'll happily apply what you've suggested in v2. But I don't see why
> > it's so terrible to over-allocate here. Leveraging a generic 2 MB page
> > allocator can be reused going forward, and encourages uniformity
> > across tests.
>
> My main concern is avoiding setting 6mb+ of memory.  I like to run the
> tests in L1 and L2 and would prefer to keep overhead at a minimum.
>
> As for the allocation itself, being precise in the allocation size is a
> form of documentation, e.g. it conveys that the size/order was chosen to
> ensure enough space for the maximum theoretical list size.  A purely
> arbitrary size, especially one that corresponds with a large page size,
> can lead to people looking for things that don't exist, e.g. the 2mb size
> is partially what led me to believe that this test was deliberately
> exceeding the limit, otherwise why allocate such a large amount of memory?
> I also didn't know if 2mb was sufficient to handle the maximum theoretical
> list size.

SGTM. I'll make this change in v2.

> > > Distilling things down to the bare minimum yields something like the
> > > following.
> >
> > Looks excellent overall. Still not clear what the consensus is on
> > whether or not to test the VM-entry failure. I think a flag seems like
> > a reasonable compromise. I've never added a flag to a kvm-unit-test,
> > so I'll see if I can figure that out.
>
> No need for a flag if you want to go that route, just put it in a separate
> VMX subtest and exclude said test from the [vmx] config, i.e. make the
> test opt-in.

SGTM, thanks!

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 18:09 Marc Orr
2019-09-13 15:24 ` Sean Christopherson
2019-09-13 16:26   ` Jim Mattson
2019-09-13 17:15     ` Sean Christopherson
2019-09-13 17:21       ` Jim Mattson
2019-09-13 18:02   ` Marc Orr
2019-09-13 18:30     ` Sean Christopherson
2019-09-13 21:55       ` Marc Orr [this message]
2019-09-14  0:49   ` Marc Orr

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=CAA03e5E28QaDAHjCg5J0_aPoY8pNnUiUQVvrZSHsEj0dq6-q7w@mail.gmail.com \
    --to=marcorr@google.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=pshier@google.com \
    --cc=sean.j.christopherson@intel.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 kvm@archiver.kernel.org
	public-inbox-index kvm


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