From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: pbonzini@redhat.com, joro@8bytes.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, tglx@linutronix.de,
mingo@redhat.com, hpa@zytor.com, x86@kernel.org,
Thomas.Lendacky@amd.com
Subject: Re: [PATCH] x86: kvm: Avoid guest page table walk when gpa_available is set
Date: Wed, 26 Apr 2017 22:44:28 +0200 [thread overview]
Message-ID: <20170426204427.GA7135@potion> (raw)
In-Reply-To: <6e453f35-cd26-1df8-5f8e-68fa09c6a1a3@amd.com>
2017-04-25 17:02-0500, Brijesh Singh:
> > > I also wanted to avoid adding yet another variable but we can't depend on
> > > cr2 parameters passed into x86_emulate_instruction().
> > >
> > > The x86_emulate_instruction() function is called from two places:
> > >
> > > 1) handling the page-fault.
> > > pf_interception [svm.c]
> > > kvm_mmu_page_fault [mmu.c]
> > > x86_emulate_instruction [x86.c]
> > >
> > > 2) completing the IO/MMIO's from previous instruction decode
> > > kvm_arch_vcpu_ioctl_run
> > > complete_emulated_io
> > > emulate_instruction
> > > x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0)
> > >
> > > In #1, we are guaranteed that cr2 variable will contain a valid GPA but
> > > in #2, CR2 is set to zero.
> >
> > We are setting up the completion in #1 x86_emulate_instruction(), where
> > the gpa (cr2) is available, so we could store the value while arming
> > vcpu->arch.complete_userspace_io.
> >
> > emulator_read_write_onepage() already saves gpa in frag->gpa, which is
> > then passed into complete_emulated_mmio -- isn't that mechanism
> > sufficient?
> >
>
> I see that complete_emulated_mmio() saves the frag>gpa into run->mmio.phys_addr,
> so based on the exit_reason we should be able to get the saved gpa.
I mean, we could pass the frag->gpa into x86_emulate_instruction().
I think that exit_reason is related just accidentally and relying on it
inside x86_emulate_instruction() is bad.
> In my debug patch below, I tried doing something similar to verify that frag->gpa
> contains the valid CR2 value but I saw a bunch of mismatch. So it seems like we
> may not able to use frag->gpa mechanism. Additionally we also need to handle the
> PIO cases (e.g what if we are called from complete_emulated_pio), which also takes
> similar code path
Yes, but PIO should trigger a NPF exit relatively rarely and
generalizing the method from MMIO will be easy.
> complete_emulated_pio
> completed_emulated_io
> emulate_instruction
> x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0)
>
> @@ -5682,13 +5686,20 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> restart:
> /*
> - * Save the faulting GPA (cr2) in the address field
> - * NOTE: If gpa_available is set then gpa_val will contain a valid GPA
> + * if previous exit was due to userspace mmio completion then actual
> + * cr2 is stored in mmio.phys_addr.
> */
> - if (vcpu->arch.gpa_available)
> - ctxt->exception.address = vcpu->arch.gpa_val;
> - else
> - ctxt->exception.address = cr2;
> + if (vcpu->run->exit_reason == KVM_EXIT_MMIO) {
> + cr2 = vcpu->run->mmio.phys_addr;
> + if (cr2 != vcpu->arch.gpa_val)
> + pr_err("** mismatch %llx %llx\n",
> + vcpu->run->mmio.phys_addr, vcpu->arch.gpa_val);
This will probably return false negatives when then vcpu->arch.gpa_val
couldn't be used anyway (possibly after a VM exits), so it is hard to
draw a conclusion.
I would really like if we had a solution that passed the gpa inside
function parameters.
(Btw. cr2 can also be a virtual address, so we can call it gpa directly)
> > If at all possible, I'd like to have the gpa passed with other relevant
> > data, instead of having it isolated like this ... and we can't manage
> > that, then at least good benchmark results to excuse the bad code.
> >
>
> I ran two tests to see how many times we walk guest page table.
>
> Test1: run kvm-unit-test
> Test2: launch Ubuntu 16.06 guest, run a stressapptest for 20 seconds, shutdown the VM
>
> Before patch
> * Test1: 10419
> * Test2: 243365
>
> After patch:
> * Test1: 1259
> * Test2: 1221
>
> Please let me know if you want me to run other other benchmark and capture the data.
Looks convincing, thanks.
next prev parent reply other threads:[~2017-04-26 20:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-24 15:52 [PATCH] x86: kvm: Avoid guest page table walk when gpa_available is set Brijesh Singh
2017-04-24 16:16 ` Brijesh Singh
2017-04-24 20:52 ` Radim Krčmář
2017-04-24 22:14 ` Brijesh Singh
2017-04-25 14:03 ` Radim Krčmář
2017-04-25 22:02 ` Brijesh Singh
2017-04-26 20:44 ` Radim Krčmář [this message]
2017-04-28 19:15 ` Brijesh Singh
2017-05-02 16:24 ` Paolo Bonzini
2017-05-02 21:44 ` Brijesh Singh
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=20170426204427.GA7135@potion \
--to=rkrcmar@redhat.com \
--cc=Thomas.Lendacky@amd.com \
--cc=brijesh.singh@amd.com \
--cc=hpa@zytor.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).