KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Yang Weijiang <weijiang.yang@intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Yang Weijiang <weijiang.yang@intel.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	pbonzini@redhat.com, jmattson@google.com,
	yu.c.zhang@linux.intel.com, alazar@bitdefender.com,
	edwin.zhai@intel.com
Subject: Re: [RESEND PATCH v10 06/10] vmx: spp: Set up SPP paging table at vmentry/vmexit
Date: Wed, 15 Jan 2020 09:36:31 +0800
Message-ID: <20200115013631.GA5975@local-michael-cet-test.sh.intel.com> (raw)
In-Reply-To: <20200114185808.GI16784@linux.intel.com>

On Tue, Jan 14, 2020 at 10:58:08AM -0800, Sean Christopherson wrote:
> On Tue, Jan 14, 2020 at 11:08:20AM +0800, Yang Weijiang wrote:
> > On Mon, Jan 13, 2020 at 09:33:58AM -0800, Sean Christopherson wrote:
> > > On Mon, Jan 13, 2020 at 04:10:50PM +0800, Yang Weijiang wrote:
> > > > On Fri, Jan 10, 2020 at 10:04:59AM -0800, Sean Christopherson wrote:
> > > > > On Thu, Jan 02, 2020 at 02:13:15PM +0800, Yang Weijiang wrote:
> > > > > > @@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
> > > > > >  		if ((error_code & PFERR_WRITE_MASK) &&
> > > > > >  		    spte_can_locklessly_be_made_writable(spte))
> > > > > >  		{
> > > > > > -			new_spte |= PT_WRITABLE_MASK;
> > > > > > +			/*
> > > > > > +			 * Record write protect fault caused by
> > > > > > +			 * Sub-page Protection, let VMI decide
> > > > > > +			 * the next step.
> > > > > > +			 */
> > > > > > +			if (spte & PT_SPP_MASK) {
> > > > > > +				int len = kvm_x86_ops->get_inst_len(vcpu);
> > > > > 
> > > > > There's got to be a better way to handle SPP exits than adding a helper
> > > > > to retrieve the instruction length.
> > > > >
> > > > The fault instruction was skipped by kvm_skip_emulated_instruction()
> > > > before, but Paolo suggested leave the re-do or skip option to user-space
> > > > to make it flexible for write protection or write tracking, so return
> > > > length to user-space.
> > > 
> > > Sorry, my comment was unclear.  I have no objection to punting the fault
> > > to userspace, it's the mechanics of how it's done that I dislike.
> > > 
> > > Specifically, (a) using run->exit_reason to propagate the SPP exit up the
> > > stack, e.g. instead of modifying affected call stacks to play nice with
> > > any exit to userspace, (b) assuming ->get_insn_len() will always be
> > > accurate, e.g. see the various caveats in skip_emulated_instruction() for
> > > both VMX and SVM, and (c) duplicating the state capture code in every
> > > location that can encounter a SPP fault.
> >
> > How about calling skip_emulated_instruction() in KVM before exit to
> 
> I'm confused.  It sounds like KVM_EXIT_SPP provides the instruction length
> because it skips an instruction before exiting to userspace.  But if KVM
> is is emulating an instruction, it shouldn't be doing
> {kvm_}skip_emulated_instruction(), e.g. if emulation fails due to a SPP
> violation (returns KVM_EXIT_SPP) then GUEST_RIP should still point at the
> exiting instruction.  Ditto for the fast_page_fault() case, RIP shouldn't
> be advanced.
There're two SPP usages, one is for write-protection the other is for
write-tracking. If the first case is being used, KVM ignores the write
, i.e., write to the memory is discarded. The second case is, if
userspace is tracking memory write through SPP, then it's notified via
KVM_EXIT_SPP but still let the write take effect by unprotecting the
subpage, i.e., like generic 4KB access-tracking.

In the first case, no necessity to re-try the faulted instruction,
the second case, a re-try is necessary, so I would skip current instruction
first, then if it's actually the second case, userspace should take action
based on the instruction lenght returned.
> 
> What am I missing?
>

> > userspace, but still return the skipped instruction length, if userspace
> > would like to re-execute the instruction, it can unwind RIP or simply
> > rely on KVM?
> 
> I'm not convinced the instruction length needs to be provided to userspace
> for this case.  Obviously it's not difficult to provide the info, I just
> don't understand the value added by doing so.  As above, RIP shouldn't
> need to be unwound, and blindly skipping an instruction seems like an odd
> thing for a VMI engine to do.
In the last review by Paolo, he mentioned SPP could be used in
access-tracing manner, it's flexible to provide instruction length to
userspace, so I removed instruction-skip in KVM but let userspace to
decide.
> 
> > > What I'm hoping is that it's possible to modify the call stacks to
> > > explicitly propagate an exit to userspace and/or SPP fault, and shove all
> > > the state capture into a common location, e.g. handle_ept_violation().
> > >
> > The problem is, the state capture code in fast_page_fault() and
> > emulation case share different causes, the former is generic occurence
> > of SPP induced EPT violation, the latter is atually a "faked" one while
> > detecting emulation instruction is writing some SPP protected area, so I
> > seperated them.
> 
> Can we make SPP dependent on unrestricted guest so that the only entry
> point to the emulator is through handle_ept_violation()?  And thus the
> only path to triggering KVM_EXIT_SPP would also be through
> handle_ept_violation(); (I think, might be forgetting a different emulation
> path).
> 
I don't got your point, from my understanding, instruction emulation is
used in several cases regarless of guest working mode. As long as any memory write happens
e.g., string ops, port ops etc, SPP write-protection
check should be applied to let the userspace capture the event.

> >
> > > Side topic, assuming the userspace VMI is going to be instrospecting the
> > > faulting instruction, won't it decode the instruction?  I.e. calculate
> > > the instruction length anyways?

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-02  6:13 [RESEND PATCH v10 00/10] Enable Sub-Page Write Protection Support Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 01/10] Documentation: Add EPT based Subpage Protection and related APIs Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 02/10] vmx: spp: Add control flags for Sub-Page Protection(SPP) Yang Weijiang
2020-01-10 16:58   ` Sean Christopherson
2020-01-13  5:44     ` Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 03/10] mmu: spp: Add SPP Table setup functions Yang Weijiang
2020-01-10 17:26   ` Sean Christopherson
2020-01-13  6:00     ` Yang Weijiang
2020-01-10 17:40   ` Sean Christopherson
2020-01-13  6:04     ` Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 04/10] mmu: spp: Add functions to operate SPP access bitmap Yang Weijiang
2020-01-10 17:38   ` Sean Christopherson
2020-01-13  6:15     ` Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 05/10] x86: spp: Introduce user-space SPP IOCTLs Yang Weijiang
2020-01-10 18:10   ` Sean Christopherson
2020-01-13  8:21     ` Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 06/10] vmx: spp: Set up SPP paging table at vmentry/vmexit Yang Weijiang
2020-01-10 17:55   ` Sean Christopherson
2020-01-13  6:50     ` Yang Weijiang
2020-01-21 14:01     ` Paolo Bonzini
2020-01-10 18:04   ` Sean Christopherson
2020-01-13  8:10     ` Yang Weijiang
2020-01-13 17:33       ` Sean Christopherson
2020-01-13 18:55         ` Adalbert Lazăr
2020-01-13 21:47           ` Sean Christopherson
2020-01-14  3:08         ` Yang Weijiang
2020-01-14 18:58           ` Sean Christopherson
2020-01-15  1:36             ` Yang Weijiang [this message]
2020-01-21 14:14             ` Paolo Bonzini
2020-01-02  6:13 ` [RESEND PATCH v10 07/10] mmu: spp: Enable Lazy mode SPP protection Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 08/10] mmu: spp: Handle SPP protected pages when VM memory changes Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 09/10] x86: spp: Add SPP protection check in emulation Yang Weijiang
2020-01-02  6:13 ` [RESEND PATCH v10 10/10] kvm: selftests: selftest for Sub-Page protection Yang Weijiang
  -- strict thread matches above, loose matches on Subject: below --
2020-01-02  5:18 [RESEND PATCH v10 00/10] Enable Sub-Page Write Protection Support Yang Weijiang
2020-01-02  5:19 ` [RESEND PATCH v10 06/10] vmx: spp: Set up SPP paging table at vmentry/vmexit Yang Weijiang

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=20200115013631.GA5975@local-michael-cet-test.sh.intel.com \
    --to=weijiang.yang@intel.com \
    --cc=alazar@bitdefender.com \
    --cc=edwin.zhai@intel.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=yu.c.zhang@linux.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
	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