From: "Raslan, KarimAllah" <karahmed@amazon.de>
To: "konrad.wilk@oracle.com" <konrad.wilk@oracle.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"rkrcmar@redhat.com" <rkrcmar@redhat.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>
Subject: Re: [PATCH v5 07/13] KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page
Date: Fri, 25 Jan 2019 18:14:33 +0000 [thread overview]
Message-ID: <1548440073.17444.21.camel@amazon.de> (raw)
In-Reply-To: <20190123175711.GL19289@Konrads-MacBook-Pro.local>
On Wed, 2019-01-23 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Jan 09, 2019 at 10:42:07AM +0100, KarimAllah Ahmed wrote:
> >
> > Use kvm_vcpu_map when mapping the virtual APIC page since using
> > kvm_vcpu_gpa_to_page() and kmap() will only work for guest memory that has
> > a "struct page".
> >
> > One additional semantic change is that the virtual host mapping lifecycle
> > has changed a bit. It now has the same lifetime of the pinning of the
> > virtual APIC page on the host side.
>
> Could you expand a bit on the 'same lifetime .. on the host side' to be
> obvious for folks what exactly the semantic is?
>
> And how does this ring with this comment:
> >
> >
> > Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> > ---
> > v4 -> v5:
> > - unmap with dirty flag
> >
> > v1 -> v2:
> > - Do not change the lifecycle of the mapping (pbonzini)
>
> .. Where Paolo does not want the semantics of the mapping to be changed?
>
> Code wise feel free to smack my Reviewed-by on it, but obviously the
> question on the above comment needs to be resolved.
Ah, right. So there was two life-cycle changes:
1- Lazily unmap the mapping and only do it on: a) release b) if the gfn is
different. This was done as an optimization (i.e. cache of a single entry).
This is the first life-cycle change that Paolo asked me to do seperately and
suggested to create a PFN cache instead. This has indeed been dropped
between v1 -> v2 switch.
2- The life-cycle change now is the fact that the kvm_vcpu_map interface does:
a) map to a virtual address b) translate a gfn to a pfn.
The original code was doing the kmap in one location and the gfn_to_page in
another. Using kvm_vcpu_map means that both kmap+gfn_to_page will be tied
together and will not be done seperately. So far no one complained about
this one so I kept it :D
>
> Thank you.
>
> >
> > - Use pfn_to_hpa instead of gfn_to_gpa
> > ---
> > arch/x86/kvm/vmx/nested.c | 32 +++++++++++---------------------
> > arch/x86/kvm/vmx/vmx.c | 5 ++---
> > arch/x86/kvm/vmx/vmx.h | 2 +-
> > 3 files changed, 14 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 4127ad9..dcff99d 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -229,10 +229,7 @@ static void free_nested(struct kvm_vcpu *vcpu)
> > kvm_release_page_dirty(vmx->nested.apic_access_page);
> > vmx->nested.apic_access_page = NULL;
> > }
> > - if (vmx->nested.virtual_apic_page) {
> > - kvm_release_page_dirty(vmx->nested.virtual_apic_page);
> > - vmx->nested.virtual_apic_page = NULL;
> > - }
> > + kvm_vcpu_unmap(&vmx->nested.virtual_apic_map, true);
> > if (vmx->nested.pi_desc_page) {
> > kunmap(vmx->nested.pi_desc_page);
> > kvm_release_page_dirty(vmx->nested.pi_desc_page);
> > @@ -2817,6 +2814,7 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > {
> > struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > + struct kvm_host_map *map;
> > struct page *page;
> > u64 hpa;
> >
> > @@ -2849,11 +2847,7 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > }
> >
> > if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) {
> > - if (vmx->nested.virtual_apic_page) { /* shouldn't happen */
> > - kvm_release_page_dirty(vmx->nested.virtual_apic_page);
> > - vmx->nested.virtual_apic_page = NULL;
> > - }
> > - page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->virtual_apic_page_addr);
> > + map = &vmx->nested.virtual_apic_map;
> >
> > /*
> > * If translation failed, VM entry will fail because
> > @@ -2868,11 +2862,9 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > * control. But such a configuration is useless, so
> > * let's keep the code simple.
> > */
> > - if (!is_error_page(page)) {
> > - vmx->nested.virtual_apic_page = page;
> > - hpa = page_to_phys(vmx->nested.virtual_apic_page);
> > - vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, hpa);
> > - }
> > + if (!kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->virtual_apic_page_addr), map))
> > + vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, pfn_to_hpa(map->pfn));
> > +
> > }
> >
> > if (nested_cpu_has_posted_intr(vmcs12)) {
> > @@ -3279,11 +3271,12 @@ static void vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
> >
> > max_irr = find_last_bit((unsigned long *)vmx->nested.pi_desc->pir, 256);
> > if (max_irr != 256) {
> > - vapic_page = kmap(vmx->nested.virtual_apic_page);
> > + vapic_page = vmx->nested.virtual_apic_map.hva;
> > + if (!vapic_page)
> > + return;
> > +
> > __kvm_apic_update_irr(vmx->nested.pi_desc->pir,
> > vapic_page, &max_irr);
> > - kunmap(vmx->nested.virtual_apic_page);
> > -
> > status = vmcs_read16(GUEST_INTR_STATUS);
> > if ((u8)max_irr > ((u8)status & 0xff)) {
> > status &= ~0xff;
> > @@ -3917,10 +3910,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> > kvm_release_page_dirty(vmx->nested.apic_access_page);
> > vmx->nested.apic_access_page = NULL;
> > }
> > - if (vmx->nested.virtual_apic_page) {
> > - kvm_release_page_dirty(vmx->nested.virtual_apic_page);
> > - vmx->nested.virtual_apic_page = NULL;
> > - }
> > + kvm_vcpu_unmap(&vmx->nested.virtual_apic_map, true);
> > if (vmx->nested.pi_desc_page) {
> > kunmap(vmx->nested.pi_desc_page);
> > kvm_release_page_dirty(vmx->nested.pi_desc_page);
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 71d88df..e13308e 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -3627,14 +3627,13 @@ static bool vmx_guest_apic_has_interrupt(struct kvm_vcpu *vcpu)
> >
> > if (WARN_ON_ONCE(!is_guest_mode(vcpu)) ||
> > !nested_cpu_has_vid(get_vmcs12(vcpu)) ||
> > - WARN_ON_ONCE(!vmx->nested.virtual_apic_page))
> > + WARN_ON_ONCE(!vmx->nested.virtual_apic_map.gfn))
> > return false;
> >
> > rvi = vmx_get_rvi();
> >
> > - vapic_page = kmap(vmx->nested.virtual_apic_page);
> > + vapic_page = vmx->nested.virtual_apic_map.hva;
> > vppr = *((u32 *)(vapic_page + APIC_PROCPRI));
> > - kunmap(vmx->nested.virtual_apic_page);
> >
> > return ((rvi & 0xf0) > (vppr & 0xf0));
> > }
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 6fb69d8..f618f52 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -142,7 +142,7 @@ struct nested_vmx {
> > * pointers, so we must keep them pinned while L2 runs.
> > */
> > struct page *apic_access_page;
> > - struct page *virtual_apic_page;
> > + struct kvm_host_map virtual_apic_map;
> > struct page *pi_desc_page;
> >
> > struct kvm_host_map msr_bitmap_map;
> > --
> > 2.7.4
> >
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
next prev parent reply other threads:[~2019-01-25 18:14 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-09 9:42 [PATCH v5 00/13] KVM/X86: Introduce a new guest mapping interface KarimAllah Ahmed
2019-01-09 9:42 ` [PATCH v5 01/13] X86/nVMX: handle_vmon: Read 4 bytes from guest memory KarimAllah Ahmed
2019-01-23 16:59 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 02/13] X86/nVMX: Update the PML table without mapping and unmapping the page KarimAllah Ahmed
2019-01-23 17:17 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 03/13] X86/KVM: Handle PFNs outside of kernel reach when touching GPTEs KarimAllah Ahmed
2019-01-23 17:31 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 04/13] KVM: Introduce a new guest mapping API KarimAllah Ahmed
2019-01-10 13:07 ` David Hildenbrand
2019-01-25 18:01 ` Raslan, KarimAllah
2019-01-23 17:41 ` Konrad Rzeszutek Wilk
2019-01-23 17:50 ` Konrad Rzeszutek Wilk
2019-01-25 18:09 ` Raslan, KarimAllah
2019-01-30 17:08 ` Paolo Bonzini
2019-01-09 9:42 ` [PATCH v5 05/13] X86/nVMX: handle_vmptrld: Use kvm_vcpu_map when copying VMCS12 from guest memory KarimAllah Ahmed
2019-01-23 17:44 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 06/13] KVM/nVMX: Use kvm_vcpu_map when mapping the L1 MSR bitmap KarimAllah Ahmed
2019-01-09 9:42 ` [PATCH v5 07/13] KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page KarimAllah Ahmed
2019-01-23 17:57 ` Konrad Rzeszutek Wilk
2019-01-25 18:14 ` Raslan, KarimAllah [this message]
2019-01-09 9:42 ` [PATCH v5 08/13] KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt descriptor table KarimAllah Ahmed
2019-01-23 18:03 ` Konrad Rzeszutek Wilk
2019-01-25 18:15 ` Raslan, KarimAllah
2019-01-09 9:42 ` [PATCH v5 09/13] KVM/X86: Use kvm_vcpu_map in emulator_cmpxchg_emulated KarimAllah Ahmed
2019-01-23 18:04 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 10/13] KVM/nSVM: Use the new mapping API for mapping guest memory KarimAllah Ahmed
2019-01-23 18:08 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 11/13] KVM/nVMX: Use kvm_vcpu_map for accessing the shadow VMCS KarimAllah Ahmed
2019-01-23 18:10 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 12/13] KVM/nVMX: Use kvm_vcpu_map for accessing the enlightened VMCS KarimAllah Ahmed
2019-01-23 18:11 ` Konrad Rzeszutek Wilk
2019-01-09 9:42 ` [PATCH v5 13/13] KVM/nVMX: Use page_address_valid in a few more locations KarimAllah Ahmed
2019-01-23 18:18 ` Konrad Rzeszutek Wilk
2019-01-25 18:17 ` Raslan, KarimAllah
2019-01-23 18:16 ` [PATCH v5 00/13] KVM/X86: Introduce a new guest mapping interface Konrad Rzeszutek Wilk
2019-01-25 18:28 ` Raslan, KarimAllah
2019-01-30 17:14 ` Paolo Bonzini
2019-01-31 13:03 ` Raslan, KarimAllah
2019-01-31 13:10 ` Paolo Bonzini
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=1548440073.17444.21.camel@amazon.de \
--to=karahmed@amazon.de \
--cc=konrad.wilk@oracle.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=rkrcmar@redhat.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.