Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1350575 > unrolled thread
| Started by | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| First post | 2016-03-04 21:50 +0100 |
| Last post | 2016-03-17 10:40 +0100 |
| Articles | 20 on this page of 27 — 3 participants |
Back to article view | Back to linux.kernel
[PART1 RFC v2 00/10] KVM: x86: Introduce SVM AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
[PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 16:50 +0100
Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-14 08:50 +0100
Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Paolo Bonzini <pbonzini@redhat.com> - 2016-03-14 13:30 +0100
Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-15 14:00 +0100
[PART1 RFC v2 08/10] svm: Do not expose x2APIC when enable AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
[PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 21:50 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 17:50 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-15 18:20 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-15 18:30 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-16 07:30 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-16 08:30 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-16 09:30 +0100
Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Paolo Bonzini <pbonzini@redhat.com> - 2016-03-16 12:20 +0100
[PART1 RFC v2 01/10] KVM: x86: Misc LAPIC changes to exposes helper functions Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
[PART1 RFC v2 04/10] svm: clean up V_TPR, V_IRQ, V_INTR_PRIO, and V_INTR_MASKING Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
[PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-04 22:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-07 17:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-08 23:10 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-09 12:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-09 22:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-10 20:40 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-10 21:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Radim Krčmář <rkrcmar@redhat.com> - 2016-03-10 21:50 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-17 05:00 +0100
Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Paolo Bonzini <pbonzini@redhat.com> - 2016-03-17 10:40 +0100
Page 1 of 2 [1] 2 Next page →
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 21:50 +0100 |
| Subject | [PART1 RFC v2 00/10] KVM: x86: Introduce SVM AVIC support |
| Message-ID | <r94PD-3Vt-3@gated-at.bofh.it> |
CHANGES FROM RFCv1:
==================
* Rebased from tip.git
* Use the vAPIC backing page as the emulated LAPIC register page.
* Clarified with HW engineer that Avic sets the IRR of all the cores
targeted whether they are running or not. It sends doorbells to the
ones that are running to get them to evaluate their new state.
If one or more weren't running it does the VMEXIT to indicate that
the host needs to go run the not running guests. It gives it the
ID of one of the guests that it observed not running which should
be enough of a hint for the hypervisor to track them all down.
* Rewrite the logic for handling AVIC_INCOMPLETE_IPI #vmexit for
IPI target not running case based on information above.
* Rewrite the logic in avic_vcpu_load() and avic_set_running().
* Rewrite the interrupt injection to remove the avic_pending_cnt
variable.
GITHUB
======
Latest git tree can be found at:
http://github.com/ssuthiku/linux.git avic_part1_rfc_v2
OVERVIEW
========
This patch set is the first of the two-part patch series to introduce
the new AMD Advance Virtual Interrupt Controller (AVIC) support.
Basically, SVM AVIC hardware virtualizes local APIC registers of each
vCPU via the virtual APIC (vAPIC) backing page. This allows guest access
to certain APIC registers without the need to emulate the hardware behavior
in the hypervisor. More information about AVIC can be found in the
AMD64 Architecture Programmer’s Manual Volume 2 - System Programming.
http://support.amd.com/TechDocs/24593.pdf
For SVM AVIC, we extend the existing kvm_amd driver to:
* Check CPUID to detect AVIC support in the processor
* Program new fields in VMCB to enable AVIC
* Introduce new AVIC data structures and add code to manage them
* Handle two new AVIC #VMEXITs
* Add new interrupt intjection code using vAPIC backing page
instead of the existing V_IRQ, V_INTR_PRIO, V_INTR_VECTOR,
and V_IGN_TPR fields
Currently, this patch series does not enable AVIC by default.
Users can enable SVM AVIC by specifying avic=1 during insmod kvm-amd.
Later, in part 2, we will introduce the IOMMU AVIC support, which
provides speed up for PCI device pass-through use case by allowing
the IOMMU hardware to inject interrupt directly into the guest via
the vAPIC backing page.
PERFORMANCE RESULTS
===================
Currently, AVIC is supported in the AMD family 15h models 6Xh
(Carrizo) processors. Therefore, it is used to collect the
perforamance data shown below.
Generaly, SVM AVIC alone (w/o IOMMU AVIC) should provide speedup for
IPI interrupt since hypervisor does not require VMEXIT to inject
these interrupts. Also, it should speed up the case when hypervisor
wants to inject an interrupt into a running guest by setting the
corresponded IRR bit in the vAPIC backing page and trigger
AVIC_DOORBELL MSR.
IPI PERFORMANCE
===============
* BENCHMARK 1: HACKBENCH
For IPI, I have collected some performance number on 2 and 4 CPU running
hackbech with the following detail:
hackbench -p -l 100000
Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
Each sender will pass 100000 messages of 100 bytes
| 2 vcpus | 4 vcpus
------------------------------------------------
Vanila | 273.76 | 190.21
AVIC disabled | 260.51 (~5%) | 184.40 (~5%)
AVIC | 248.53 (~10%) | 155.01 (~20%)
OVERALL PERFORMANCE
===================
Enabling AVIC should helps speeding up workloads, which generate
large amount of interrupts. However, it requires additional logics to
maintain AVIC-specific data structures during vCPU load/unload
due to vcpu scheduling.
The goal is to minimize the overhead of AVIC in most cases, so that
we can achieve equivalent or improvement in overall performance when
enabling AVIC.
* BENCHMARK 1: TAR DECOMPRESSION
This test measures the average running time (of 10 runs) of the following
tar decompression command with 1, 2, and 4 vcpus.
tar xf linux-4.3.3.tar.xz
| 4 vcpus
---------------------------------
Vanila | 10.26
AVIC disabled | 10.10 (~1.5%)
AVIC | 10.07 (~1.8%)
Note: The unit of result below is in seconds (lower is better).
* BENCHMARK 2: NETPERF w/ virtual network
This test creates a virtual network by setting up bridge and tap device
on the host and pass it into the VM as virtio-net-pci device w/ vhost.
Then it sets up netserver in the host machine, and run netperf
in the VM with following option:
netperf -H <netserver ip> -l 60 -t TCP_RR -D 2
| 1 vcpu
------------------------------------
Vanila | 21623.887
AVIC disabled | 21538.09 (~-.4%)
AVIC | 21712.68 (~0.4%)
Note: The unit of result below is trans/sec (higher is better).
Preliminary result of both benchmarks show AVIC performance are slightly
better than the other two cases.
CURRENT UNTESTED USE-CASES
===========================
- VM Migration (work in progress)
- Nested VM
Any feedback and comments are very much appreciated.
Thank you,
Suravee
Suravee Suthikulpanit (10):
KVM: x86: Misc LAPIC changes to exposes helper functions
KVM: x86: Introducing kvm_x86_ops VCPU blocking/unblocking
svm: Introduce new AVIC VMCB registers
svm: clean up V_TPR, V_IRQ, V_INTR_PRIO, and V_INTR_MASKING
KVM: x86: Detect and Initialize AVIC support
svm: Add interrupt injection via AVIC
svm: Add VMEXIT handlers for AVIC
svm: Do not expose x2APIC when enable AVIC
svm: Do not intercept CR8 when enable AVIC
svm: Manage vcpu load/unload when enable AVIC
arch/x86/include/asm/kvm_host.h | 19 +-
arch/x86/include/asm/svm.h | 38 +-
arch/x86/include/uapi/asm/svm.h | 9 +-
arch/x86/kvm/lapic.c | 51 ++-
arch/x86/kvm/lapic.h | 7 +
arch/x86/kvm/svm.c | 937 +++++++++++++++++++++++++++++++++++++++-
6 files changed, 1003 insertions(+), 58 deletions(-)
--
1.9.1
[toc] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 21:50 +0100 |
| Subject | [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers |
| Message-ID | <r94PE-3Vt-21@gated-at.bofh.it> |
| In reply to | #1350575 |
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Introduce new AVIC VMCB registers. Also breakdown int_ctl register
into bit-field for ease of use.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/include/asm/svm.h | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 6136d99..db5d7af 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -67,10 +67,24 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
u32 asid;
u8 tlb_ctl;
u8 reserved_2[3];
- u32 int_ctl;
+ union { /* Offset 0x60 */
+ u32 int_ctl;
+
+ struct __attribute__ ((__packed__)) {
+ u32 v_tpr : 8,
+ v_irq : 1,
+ reserved_3 : 7,
+ v_intr_prio : 4,
+ v_ign_tpr : 1,
+ reserved_4 : 3,
+ v_intr_masking : 1,
+ reserved_5 : 6,
+ avic_enable : 1;
+ };
+ };
u32 int_vector;
u32 int_state;
- u8 reserved_3[4];
+ u8 reserved_6[4];
u32 exit_code;
u32 exit_code_hi;
u64 exit_info_1;
@@ -78,17 +92,22 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
u32 exit_int_info;
u32 exit_int_info_err;
u64 nested_ctl;
- u8 reserved_4[16];
+ u64 avic_vapic_bar;
+ u8 reserved_7[8];
u32 event_inj;
u32 event_inj_err;
u64 nested_cr3;
u64 lbr_ctl;
u32 clean;
- u32 reserved_5;
+ u32 reserved_8;
u64 next_rip;
u8 insn_len;
u8 insn_bytes[15];
- u8 reserved_6[800];
+ u64 avic_bk_page; /* Offset 0xe0 */
+ u8 reserved_9[8]; /* Offset 0xe8 */
+ u64 avic_log_apic_id; /* Offset 0xf0 */
+ u64 avic_phy_apic_id; /* Offset 0xf8 */
+ u8 reserved_10[768];
};
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-07 16:50 +0100 |
| Subject | Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers |
| Message-ID | <ra5zY-4I3-21@gated-at.bofh.it> |
| In reply to | #1350576 |
On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> Introduce new AVIC VMCB registers. Also breakdown int_ctl register
> into bit-field for ease of use.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> arch/x86/include/asm/svm.h | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 6136d99..db5d7af 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -67,10 +67,24 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> u32 asid;
> u8 tlb_ctl;
> u8 reserved_2[3];
> - u32 int_ctl;
> + union { /* Offset 0x60 */
> + u32 int_ctl;
> +
> + struct __attribute__ ((__packed__)) {
> + u32 v_tpr : 8,
> + v_irq : 1,
> + reserved_3 : 7,
> + v_intr_prio : 4,
> + v_ign_tpr : 1,
> + reserved_4 : 3,
> + v_intr_masking : 1,
> + reserved_5 : 6,
> + avic_enable : 1;
Please do not introduce bitfields and drop patch 4.
Thanks,
Paolo
> + };
> + };
> u32 int_vector;
> u32 int_state;
> - u8 reserved_3[4];
> + u8 reserved_6[4];
> u32 exit_code;
> u32 exit_code_hi;
> u64 exit_info_1;
> @@ -78,17 +92,22 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> u32 exit_int_info;
> u32 exit_int_info_err;
> u64 nested_ctl;
> - u8 reserved_4[16];
> + u64 avic_vapic_bar;
> + u8 reserved_7[8];
> u32 event_inj;
> u32 event_inj_err;
> u64 nested_cr3;
> u64 lbr_ctl;
> u32 clean;
> - u32 reserved_5;
> + u32 reserved_8;
> u64 next_rip;
> u8 insn_len;
> u8 insn_bytes[15];
> - u8 reserved_6[800];
> + u64 avic_bk_page; /* Offset 0xe0 */
> + u8 reserved_9[8]; /* Offset 0xe8 */
> + u64 avic_log_apic_id; /* Offset 0xf0 */
> + u64 avic_phy_apic_id; /* Offset 0xf8 */
> + u8 reserved_10[768];
> };
>
>
>
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-14 08:50 +0100 |
| Subject | Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers |
| Message-ID | <rcvqi-KB-25@gated-at.bofh.it> |
| In reply to | #1351728 |
Hi,
On 03/07/2016 10:44 PM, Paolo Bonzini wrote:
>
> On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
>> >From: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> >
>> >Introduce new AVIC VMCB registers. Also breakdown int_ctl register
>> >into bit-field for ease of use.
>> >
>> >Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> >---
>> > arch/x86/include/asm/svm.h | 29 ++++++++++++++++++++++++-----
>> > 1 file changed, 24 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
>> >index 6136d99..db5d7af 100644
>> >--- a/arch/x86/include/asm/svm.h
>> >+++ b/arch/x86/include/asm/svm.h
>> >@@ -67,10 +67,24 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>> > u32 asid;
>> > u8 tlb_ctl;
>> > u8 reserved_2[3];
>> >- u32 int_ctl;
>> >+ union { /* Offset 0x60 */
>> >+ u32 int_ctl;
>> >+
>> >+ struct __attribute__ ((__packed__)) {
>> >+ u32 v_tpr : 8,
>> >+ v_irq : 1,
>> >+ reserved_3 : 7,
>> >+ v_intr_prio : 4,
>> >+ v_ign_tpr : 1,
>> >+ reserved_4 : 3,
>> >+ v_intr_masking : 1,
>> >+ reserved_5 : 6,
>> >+ avic_enable : 1;
> Please do not introduce bitfields and drop patch 4.
>
> Thanks,
>
> Paolo
>
Any particular reason why you do not recommend the use of bit field?
Thanks,
Suravee
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-14 13:30 +0100 |
| Subject | Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers |
| Message-ID | <rczNg-3Fk-11@gated-at.bofh.it> |
| In reply to | #1357019 |
On 14/03/2016 08:41, Suravee Suthikulpanit wrote: > Any particular reason why you do not recommend the use of bit field? 1) The current coding style is generally not using bitfields 2) Having to review patches that change working code unrelated to AVIC 3) Most of the fields are not even used when AVIC is enabled, so the benefit of the conversion is small. Paolo
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-15 14:00 +0100 |
| Subject | Re: [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers |
| Message-ID | <rcWJQ-2cW-5@gated-at.bofh.it> |
| In reply to | #1357209 |
On 03/14/2016 07:25 PM, Paolo Bonzini wrote: > > > On 14/03/2016 08:41, Suravee Suthikulpanit wrote: >> Any particular reason why you do not recommend the use of bit field? > > 1) The current coding style is generally not using bitfields > > 2) Having to review patches that change working code unrelated to AVIC > > 3) Most of the fields are not even used when AVIC is enabled, so the > benefit of the conversion is small. > > Paolo > Ok I'll remove the bit-field stuff from patch 3 and will get rid off patch 4. Thanks, Suravee
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 21:50 +0100 |
| Subject | [PART1 RFC v2 08/10] svm: Do not expose x2APIC when enable AVIC |
| Message-ID | <r94PF-3Vt-25@gated-at.bofh.it> |
| In reply to | #1350575 |
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Since AVIC only virtualizes xAPIC hardware for the guest, we need to:
* Intercept APIC BAR msr accesses to disable x2APIC
* Intercept CPUID access to not advertise x2APIC support
* Hide x2APIC support when checking via KVM ioctl
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/kvm/svm.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a177781..02cd8d0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -228,6 +228,7 @@ static const struct svm_direct_access_msrs {
{ .index = MSR_IA32_LASTBRANCHTOIP, .always = false },
{ .index = MSR_IA32_LASTINTFROMIP, .always = false },
{ .index = MSR_IA32_LASTINTTOIP, .always = false },
+ { .index = MSR_IA32_APICBASE, .always = false },
{ .index = MSR_INVALID, .always = false },
};
@@ -855,6 +856,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm)
set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1);
}
+
+ if (avic)
+ set_msr_interception(msrpm, MSR_IA32_APICBASE, 1, 1);
}
static void add_msr_offset(u32 offset)
@@ -3490,6 +3494,18 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
msr_info->data = 0x1E;
}
break;
+ case MSR_IA32_APICBASE:
+ if (avic) {
+ /* Note:
+ * For AVIC, we need to disable X2APIC
+ * and enable XAPIC
+ */
+ kvm_get_msr_common(vcpu, msr_info);
+ msr_info->data &= ~X2APIC_ENABLE;
+ msr_info->data |= XAPIC_ENABLE;
+ break;
+ }
+ /* Follow through if not AVIC */
default:
return kvm_get_msr_common(vcpu, msr_info);
}
@@ -3618,6 +3634,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
case MSR_VM_IGNNE:
vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data);
break;
+ case MSR_IA32_APICBASE:
+ if (avic)
+ avic_update_vapic_bar(to_svm(vcpu), data);
+ /* Follow through */
default:
return kvm_set_msr_common(vcpu, msr);
}
@@ -4754,11 +4774,26 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
/* Update nrips enabled cache */
svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
+
+ /* Do not support X2APIC when enable AVIC */
+ if (avic) {
+ int i;
+
+ for (i = 0 ; i < vcpu->arch.cpuid_nent ; i++) {
+ if (vcpu->arch.cpuid_entries[i].function == 1)
+ vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21);
+ }
+ }
}
static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
{
switch (func) {
+ case 0x00000001:
+ /* Do not support X2APIC when enable AVIC */
+ if (avic)
+ entry->ecx &= ~(1 << 21);
+ break;
case 0x80000001:
if (nested)
entry->ecx |= (1 << 2); /* Set SVM bit */
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 21:50 +0100 |
| Subject | [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <r94PF-3Vt-23@gated-at.bofh.it> |
| In reply to | #1350575 |
This patch introduces AVIC-related data structure, and AVIC
initialization code.
There are three main data structures for AVIC:
* Virtual APIC (vAPIC) backing page (per-VCPU)
* Physical APIC ID table (per-VM)
* Logical APIC ID table (per-VM)
In order to accommodate the new per-VM tables, we introduce
a new per-VM arch-specific void pointer, struct kvm_arch.arch_data.
This will point to the newly introduced struct svm_vm_data.
Currently, AVIC is disabled by default. Users can manually
enable AVIC via kernel boot option kvm-amd.avic=1 or during
kvm-amd module loading with parameter avic=1.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/include/asm/kvm_host.h | 2 +
arch/x86/kvm/svm.c | 416 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 417 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9a61669..15f6cd8 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -754,6 +754,8 @@ struct kvm_arch {
bool irqchip_split;
u8 nr_reserved_ioapic_pins;
+
+ void *arch_data;
};
struct kvm_vm_stat {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6ab66d0..c703149 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -14,6 +14,9 @@
* the COPYING file in the top-level directory.
*
*/
+
+#define pr_fmt(fmt) "SVM: " fmt
+
#include <linux/kvm_host.h>
#include "irq.h"
@@ -78,6 +81,11 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
#define TSC_RATIO_MIN 0x0000000000000001ULL
#define TSC_RATIO_MAX 0x000000ffffffffffULL
+#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
+
+/* NOTE: Current max index allowed for physical APIC ID table is 255 */
+#define AVIC_PHY_APIC_ID_MAX 0xFF
+
static bool erratum_383_found __read_mostly;
static const u32 host_save_user_msrs[] = {
@@ -162,6 +170,37 @@ struct vcpu_svm {
/* cached guest cpuid flags for faster access */
bool nrips_enabled : 1;
+
+ struct page *avic_bk_page;
+ void *in_kernel_lapic_regs;
+};
+
+struct __attribute__ ((__packed__))
+svm_avic_log_ait_entry {
+ u32 guest_phy_apic_id : 8,
+ res : 23,
+ valid : 1;
+};
+
+struct __attribute__ ((__packed__))
+svm_avic_phy_ait_entry {
+ u64 host_phy_apic_id : 8,
+ res1 : 4,
+ bk_pg_ptr : 40,
+ res2 : 10,
+ is_running : 1,
+ valid : 1;
+};
+
+/* Note: This structure is per VM */
+struct svm_vm_data {
+ atomic_t count;
+ u32 ldr_mode;
+ u32 avic_max_vcpu_id;
+ u32 avic_tag;
+
+ struct page *avic_log_ait_page;
+ struct page *avic_phy_ait_page;
};
static DEFINE_PER_CPU(u64, current_tsc_ratio);
@@ -205,6 +244,10 @@ module_param(npt, int, S_IRUGO);
static int nested = true;
module_param(nested, int, S_IRUGO);
+/* enable / disable AVIC */
+static int avic;
+module_param(avic, int, S_IRUGO);
+
static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
static void svm_flush_tlb(struct kvm_vcpu *vcpu);
static void svm_complete_interrupts(struct vcpu_svm *svm);
@@ -234,6 +277,13 @@ enum {
/* TPR and CR2 are always written before VMRUN */
#define VMCB_ALWAYS_DIRTY_MASK ((1U << VMCB_INTR) | (1U << VMCB_CR2))
+#define VMCB_AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
+
+static inline void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
+{
+ svm->vmcb->control.avic_vapic_bar = data & VMCB_AVIC_APIC_BAR_MASK;
+}
+
static inline void mark_all_dirty(struct vmcb *vmcb)
{
vmcb->control.clean = 0;
@@ -923,6 +973,13 @@ static __init int svm_hardware_setup(void)
} else
kvm_disable_tdp();
+ if (avic && (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)))
+ avic = false;
+
+ if (avic) {
+ printk(KERN_INFO "kvm: AVIC enabled\n");
+ }
+
return 0;
err:
@@ -1000,6 +1057,41 @@ static void svm_adjust_tsc_offset_guest(struct kvm_vcpu *vcpu, s64 adjustment)
mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
}
+static void avic_init_vmcb(struct vcpu_svm *svm)
+{
+ void *vapic_bkpg;
+ struct vmcb *vmcb = svm->vmcb;
+ struct svm_vm_data *vm_data = svm->vcpu.kvm->arch.arch_data;
+ phys_addr_t bpa = PFN_PHYS(page_to_pfn(svm->avic_bk_page));
+ phys_addr_t lpa = PFN_PHYS(page_to_pfn(vm_data->avic_log_ait_page));
+ phys_addr_t ppa = PFN_PHYS(page_to_pfn(vm_data->avic_phy_ait_page));
+
+ if (!vmcb)
+ return;
+
+ pr_debug("%s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
+ __func__, (unsigned long long)bpa,
+ (unsigned long long)lpa, (unsigned long long)ppa);
+
+
+ /*
+ * Here we copy the value from the emulated LAPIC register
+ * to the vAPIC backign page, and then flip regs frame.
+ */
+ if (!svm->in_kernel_lapic_regs)
+ svm->in_kernel_lapic_regs = svm->vcpu.arch.apic->regs;
+
+ vapic_bkpg = pfn_to_kaddr(page_to_pfn(svm->avic_bk_page));
+ memcpy(vapic_bkpg, svm->in_kernel_lapic_regs, PAGE_SIZE);
+ svm->vcpu.arch.apic->regs = vapic_bkpg;
+
+ vmcb->control.avic_bk_page = bpa & AVIC_HPA_MASK;
+ vmcb->control.avic_log_apic_id = lpa & AVIC_HPA_MASK;
+ vmcb->control.avic_phy_apic_id = ppa & AVIC_HPA_MASK;
+ vmcb->control.avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
+ vmcb->control.avic_enable = 1;
+}
+
static void init_vmcb(struct vcpu_svm *svm)
{
struct vmcb_control_area *control = &svm->vmcb->control;
@@ -1113,6 +1205,293 @@ static void init_vmcb(struct vcpu_svm *svm)
mark_all_dirty(svm->vmcb);
enable_gif(svm);
+
+ if (avic)
+ avic_init_vmcb(svm);
+}
+
+static struct svm_avic_phy_ait_entry *
+avic_get_phy_ait_entry(struct kvm_vcpu *vcpu, int index)
+{
+ struct svm_avic_phy_ait_entry *avic_phy_ait;
+ struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
+
+ if (!vm_data)
+ return NULL;
+
+ /* Note: APIC ID = 0xff is used for broadcast.
+ * APIC ID > 0xff is reserved.
+ */
+ if (index >= 0xff)
+ return NULL;
+
+ avic_phy_ait = page_address(vm_data->avic_phy_ait_page);
+
+ return &avic_phy_ait[index];
+}
+
+struct svm_avic_log_ait_entry *
+avic_get_log_ait_entry(struct kvm_vcpu *vcpu, u8 mda, bool is_flat)
+{
+ struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
+ int index;
+ struct svm_avic_log_ait_entry *avic_log_ait;
+
+ if (!vm_data)
+ return NULL;
+
+ if (is_flat) { /* flat */
+ if (mda > 7)
+ return NULL;
+ index = mda;
+ } else { /* cluster */
+ int apic_id = mda & 0xf;
+ int cluster_id = (mda & 0xf0) >> 8;
+
+ if (apic_id > 4 || cluster_id >= 0xf)
+ return NULL;
+ index = (cluster_id << 2) + apic_id;
+ }
+ avic_log_ait = (struct svm_avic_log_ait_entry *)
+ page_address(vm_data->avic_log_ait_page);
+
+ return &avic_log_ait[index];
+}
+
+static inline void
+avic_set_bk_page_entry(struct vcpu_svm *svm, int reg_off, u32 val)
+{
+ void *avic_bk = page_address(svm->avic_bk_page);
+
+ *((u32 *) (avic_bk + reg_off)) = val;
+}
+
+static inline u32 *avic_get_bk_page_entry(struct vcpu_svm *svm, u32 offset)
+{
+ char *tmp = (char *)page_address(svm->avic_bk_page);
+
+ return (u32 *)(tmp+offset);
+}
+
+static int avic_init_log_apic_entry(struct kvm_vcpu *vcpu, u8 g_phy_apic_id,
+ u8 log_apic_id)
+{
+ u32 mod;
+ struct svm_avic_log_ait_entry *entry;
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ if (!svm)
+ return -EINVAL;
+
+ mod = (*avic_get_bk_page_entry(svm, APIC_DFR) >> 28) & 0xf;
+ entry = avic_get_log_ait_entry(vcpu, log_apic_id, (mod == 0xf));
+ if (!entry)
+ return -EINVAL;
+ entry->guest_phy_apic_id = g_phy_apic_id;
+ entry->valid = 1;
+
+ return 0;
+}
+
+static int avic_init_bk_page(struct kvm_vcpu *vcpu)
+{
+ u64 addr;
+ struct page *page;
+ int id = vcpu->vcpu_id;
+ struct kvm *kvm = vcpu->kvm;
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ addr = APIC_DEFAULT_PHYS_BASE + (id * PAGE_SIZE);
+ page = gfn_to_page(kvm, addr >> PAGE_SHIFT);
+ if (is_error_page(page))
+ return -EFAULT;
+
+ /*
+ * Do not pin the page in memory, so that memory hot-unplug
+ * is able to migrate it.
+ */
+ put_page(page);
+
+ /* Setting up AVIC Backing Page */
+ svm->avic_bk_page = page;
+
+ clear_page(kmap(page));
+ pr_debug("%s: vAPIC bk page: cpu=%u, addr=%#llx, pa=%#llx\n",
+ __func__, id, addr,
+ (unsigned long long) PFN_PHYS(page_to_pfn(page)));
+
+ avic_init_vmcb(svm);
+
+ return 0;
+}
+
+static inline void avic_unalloc_bk_page(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ if (svm->avic_bk_page)
+ kunmap(svm->avic_bk_page);
+}
+
+static int avic_alloc_bk_page(struct vcpu_svm *svm, int id)
+{
+ int ret = 0, i;
+ bool realloc = false;
+ struct kvm_vcpu *vcpu;
+ struct kvm *kvm = svm->vcpu.kvm;
+ struct svm_vm_data *vm_data = kvm->arch.arch_data;
+
+ mutex_lock(&kvm->slots_lock);
+
+ /* Check if we have already allocated vAPIC backing
+ * page for this vCPU. If not, we need to realloc
+ * a new one and re-assign all other vCPU.
+ */
+ if (kvm->arch.apic_access_page_done &&
+ (id > vm_data->avic_max_vcpu_id)) {
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ avic_unalloc_bk_page(vcpu);
+
+ __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
+ 0, 0);
+ realloc = true;
+ vm_data->avic_max_vcpu_id = 0;
+ }
+
+ /*
+ * We are allocating vAPIC backing page
+ * upto the max vCPU ID
+ */
+ if (id >= vm_data->avic_max_vcpu_id) {
+ ret = __x86_set_memory_region(kvm,
+ APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
+ APIC_DEFAULT_PHYS_BASE,
+ PAGE_SIZE * (id + 1));
+ if (ret)
+ goto out;
+
+ vm_data->avic_max_vcpu_id = id;
+ }
+
+ /* Reinit vAPIC backing page for exisinting vcpus */
+ if (realloc)
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ avic_init_bk_page(vcpu);
+
+ avic_init_bk_page(&svm->vcpu);
+
+ kvm->arch.apic_access_page_done = true;
+
+out:
+ mutex_unlock(&kvm->slots_lock);
+ return ret;
+}
+
+static void avic_vm_uninit(struct kvm *kvm)
+{
+ struct svm_vm_data *vm_data = kvm->arch.arch_data;
+
+ if (!vm_data)
+ return;
+
+ if (vm_data->avic_log_ait_page)
+ __free_page(vm_data->avic_log_ait_page);
+ if (vm_data->avic_phy_ait_page)
+ __free_page(vm_data->avic_phy_ait_page);
+ kfree(vm_data);
+ kvm->arch.arch_data = NULL;
+}
+
+static void avic_vcpu_uninit(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+ struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
+
+ if (!avic)
+ return;
+
+ avic_unalloc_bk_page(vcpu);
+ svm->vcpu.arch.apic->regs = svm->in_kernel_lapic_regs;
+ svm->in_kernel_lapic_regs = NULL;
+
+ if (vm_data &&
+ (atomic_read(&vm_data->count) == 0 ||
+ atomic_dec_and_test(&vm_data->count)))
+ avic_vm_uninit(vcpu->kvm);
+}
+
+static atomic_t avic_tag_gen = ATOMIC_INIT(1);
+
+static inline u32 avic_get_next_tag(void)
+{
+ u32 tag = atomic_read(&avic_tag_gen);
+
+ atomic_inc(&avic_tag_gen);
+ return tag;
+}
+
+static int avic_vm_init(struct kvm *kvm)
+{
+ int err = -ENOMEM;
+ struct svm_vm_data *vm_data;
+ struct page *avic_phy_ait_page;
+ struct page *avic_log_ait_page;
+
+ vm_data = kzalloc(sizeof(struct svm_vm_data),
+ GFP_KERNEL);
+ if (!vm_data)
+ return err;
+
+ kvm->arch.arch_data = vm_data;
+ atomic_set(&vm_data->count, 0);
+
+ /* Allocating physical APIC ID table (4KB) */
+ avic_phy_ait_page = alloc_page(GFP_KERNEL);
+ if (!avic_phy_ait_page)
+ goto free_avic;
+
+ vm_data->avic_phy_ait_page = avic_phy_ait_page;
+ clear_page(page_address(avic_phy_ait_page));
+
+ /* Allocating logical APIC ID table (4KB) */
+ avic_log_ait_page = alloc_page(GFP_KERNEL);
+ if (!avic_log_ait_page)
+ goto free_avic;
+
+ vm_data->avic_log_ait_page = avic_log_ait_page;
+ clear_page(page_address(avic_log_ait_page));
+
+ vm_data->avic_tag = avic_get_next_tag();
+
+ return 0;
+
+free_avic:
+ avic_vm_uninit(kvm);
+ return err;
+}
+
+static int avic_vcpu_init(struct kvm *kvm, struct vcpu_svm *svm, int id)
+{
+ int err;
+ struct svm_vm_data *vm_data = NULL;
+
+ /* Note: svm_vm_data is per VM */
+ if (!kvm->arch.arch_data) {
+ err = avic_vm_init(kvm);
+ if (err)
+ return err;
+ }
+
+ err = avic_alloc_bk_page(svm, id);
+ if (err) {
+ avic_vcpu_uninit(&svm->vcpu);
+ return err;
+ }
+
+ vm_data = kvm->arch.arch_data;
+ atomic_inc(&vm_data->count);
+
+ return 0;
}
static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
@@ -1131,6 +1510,9 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy);
kvm_register_write(vcpu, VCPU_REGS_RDX, eax);
+
+ if (avic && !init_event)
+ avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
}
static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
@@ -1169,6 +1551,12 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
if (!hsave_page)
goto free_page3;
+ if (avic) {
+ err = avic_vcpu_init(kvm, svm, id);
+ if (err)
+ goto free_page4;
+ }
+
svm->nested.hsave = page_address(hsave_page);
svm->msrpm = page_address(msrpm_pages);
@@ -1187,6 +1575,8 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
return &svm->vcpu;
+free_page4:
+ __free_page(hsave_page);
free_page3:
__free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER);
free_page2:
@@ -1209,6 +1599,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
__free_page(virt_to_page(svm->nested.hsave));
__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
+ avic_vcpu_uninit(vcpu);
kvm_vcpu_uninit(vcpu);
kmem_cache_free(kvm_vcpu_cache, svm);
}
@@ -3382,6 +3773,7 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
pr_err("%-20s%08x\n", "exit_int_info_err:", control->exit_int_info_err);
pr_err("%-20s%lld\n", "nested_ctl:", control->nested_ctl);
pr_err("%-20s%016llx\n", "nested_cr3:", control->nested_cr3);
+ pr_err("%-20s%016llx\n", "avic_vapic_bar:", control->avic_vapic_bar);
pr_err("%-20s%08x\n", "event_inj:", control->event_inj);
pr_err("%-20s%08x\n", "event_inj_err:", control->event_inj_err);
pr_err("%-20s%lld\n", "lbr_ctl:", control->lbr_ctl);
@@ -3613,11 +4005,31 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
static bool svm_get_enable_apicv(void)
{
- return false;
+ return avic;
+}
+
+static void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
+{
}
+static void svm_hwapic_isr_update(struct kvm *kvm, int isr)
+{
+}
+
+/* Note: Currently only used by Hyper-V. */
static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
{
+ struct vcpu_svm *svm = to_svm(vcpu);
+ struct vmcb *vmcb = svm->vmcb;
+
+ if (!avic)
+ return;
+
+ if (!svm->in_kernel_lapic_regs)
+ return;
+
+ svm->vcpu.arch.apic->regs = svm->in_kernel_lapic_regs;
+ vmcb->control.avic_enable = 0;
}
static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
@@ -4387,6 +4799,8 @@ static struct kvm_x86_ops svm_x86_ops = {
.refresh_apicv_exec_ctrl = svm_refresh_apicv_exec_ctrl,
.load_eoi_exitmap = svm_load_eoi_exitmap,
.sync_pir_to_irr = svm_sync_pir_to_irr,
+ .hwapic_irr_update = svm_hwapic_irr_update,
+ .hwapic_isr_update = svm_hwapic_isr_update,
.set_tss_addr = svm_set_tss_addr,
.get_tdp_level = get_npt_level,
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-07 17:50 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <ra6w2-5kV-13@gated-at.bofh.it> |
| In reply to | #1350580 |
On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
> @@ -162,6 +170,37 @@ struct vcpu_svm {
>
> /* cached guest cpuid flags for faster access */
> bool nrips_enabled : 1;
> +
> + struct page *avic_bk_page;
> + void *in_kernel_lapic_regs;
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_log_ait_entry {
> + u32 guest_phy_apic_id : 8,
> + res : 23,
> + valid : 1;
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_phy_ait_entry {
> + u64 host_phy_apic_id : 8,
> + res1 : 4,
> + bk_pg_ptr : 40,
> + res2 : 10,
> + is_running : 1,
> + valid : 1;
> +};
Please don't use bitfields.
> +/* Note: This structure is per VM */
> +struct svm_vm_data {
> + atomic_t count;
> + u32 ldr_mode;
> + u32 avic_max_vcpu_id;
> + u32 avic_tag;
> +
> + struct page *avic_log_ait_page;
> + struct page *avic_phy_ait_page;
You can put these directly in kvm_arch. Do not use abbreviations:
struct page *avic_logical_apic_id_table_page;
struct page *avic_physical_apic_id_table_page;
> @@ -1000,6 +1057,41 @@ static void svm_adjust_tsc_offset_guest(struct kvm_vcpu *vcpu, s64 adjustment)
> mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> }
>
> +static void avic_init_vmcb(struct vcpu_svm *svm)
> +{
> + void *vapic_bkpg;
> + struct vmcb *vmcb = svm->vmcb;
> + struct svm_vm_data *vm_data = svm->vcpu.kvm->arch.arch_data;
> + phys_addr_t bpa = PFN_PHYS(page_to_pfn(svm->avic_bk_page));
> + phys_addr_t lpa = PFN_PHYS(page_to_pfn(vm_data->avic_log_ait_page));
> + phys_addr_t ppa = PFN_PHYS(page_to_pfn(vm_data->avic_phy_ait_page));
Please use page_to_phys.
> + if (!vmcb)
> + return;
> +
> + pr_debug("%s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
> + __func__, (unsigned long long)bpa,
> + (unsigned long long)lpa, (unsigned long long)ppa);
No pr_debug.
> +
> + /*
> + * Here we copy the value from the emulated LAPIC register
> + * to the vAPIC backign page, and then flip regs frame.
> + */
> + if (!svm->in_kernel_lapic_regs)
> + svm->in_kernel_lapic_regs = svm->vcpu.arch.apic->regs;
> +
> + vapic_bkpg = pfn_to_kaddr(page_to_pfn(svm->avic_bk_page));
Please use kmap instead.
> + memcpy(vapic_bkpg, svm->in_kernel_lapic_regs, PAGE_SIZE);
> + svm->vcpu.arch.apic->regs = vapic_bkpg;
Can you explain the flipping logic, and why you cannot just use the
existing apic.regs?
> + vmcb->control.avic_bk_page = bpa & AVIC_HPA_MASK;
No abbreviations ("avic_backing_page").
> + vmcb->control.avic_log_apic_id = lpa & AVIC_HPA_MASK;
> + vmcb->control.avic_phy_apic_id = ppa & AVIC_HPA_MASK;
> + vmcb->control.avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> + vmcb->control.avic_enable = 1;
> +}
> +
> static void init_vmcb(struct vcpu_svm *svm)
> {
> struct vmcb_control_area *control = &svm->vmcb->control;
> @@ -1113,6 +1205,293 @@ static void init_vmcb(struct vcpu_svm *svm)
> mark_all_dirty(svm->vmcb);
>
> enable_gif(svm);
> +
> + if (avic)
> + avic_init_vmcb(svm);
> +}
> +
> +static struct svm_avic_phy_ait_entry *
> +avic_get_phy_ait_entry(struct kvm_vcpu *vcpu, int index)
> +{
> + struct svm_avic_phy_ait_entry *avic_phy_ait;
> + struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
> +
> + if (!vm_data)
> + return NULL;
> +
> + /* Note: APIC ID = 0xff is used for broadcast.
> + * APIC ID > 0xff is reserved.
> + */
> + if (index >= 0xff)
> + return NULL;
> +
> + avic_phy_ait = page_address(vm_data->avic_phy_ait_page);
> +
> + return &avic_phy_ait[index];
> +}
> +
> +struct svm_avic_log_ait_entry *
> +avic_get_log_ait_entry(struct kvm_vcpu *vcpu, u8 mda, bool is_flat)
> +{
> + struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
> + int index;
> + struct svm_avic_log_ait_entry *avic_log_ait;
> +
> + if (!vm_data)
> + return NULL;
> +
> + if (is_flat) { /* flat */
> + if (mda > 7)
> + return NULL;
> + index = mda;
> + } else { /* cluster */
> + int apic_id = mda & 0xf;
> + int cluster_id = (mda & 0xf0) >> 8;
> +
> + if (apic_id > 4 || cluster_id >= 0xf)
> + return NULL;
> + index = (cluster_id << 2) + apic_id;
> + }
> + avic_log_ait = (struct svm_avic_log_ait_entry *)
> + page_address(vm_data->avic_log_ait_page);
> +
> + return &avic_log_ait[index];
> +}
Instead of these functions, create a complete function to handle APIC_ID
and APIC_LDR writes. Then use kmap/kunmap instead of page_address.
> +static inline void
> +avic_set_bk_page_entry(struct vcpu_svm *svm, int reg_off, u32 val)
> +{
> + void *avic_bk = page_address(svm->avic_bk_page);
> +
> + *((u32 *) (avic_bk + reg_off)) = val;
> +}
Unused function.
> +static inline u32 *avic_get_bk_page_entry(struct vcpu_svm *svm, u32 offset)
> +{
> + char *tmp = (char *)page_address(svm->avic_bk_page);
> +
> + return (u32 *)(tmp+offset);
> +}
I think you should be able to use kvm_apic_get_reg instead.
> +static int avic_init_bk_page(struct kvm_vcpu *vcpu)
No abbreviations (but again, please try reusing apic.regs).
> +static int avic_alloc_bk_page(struct vcpu_svm *svm, int id)
> +{
> + int ret = 0, i;
> + bool realloc = false;
> + struct kvm_vcpu *vcpu;
> + struct kvm *kvm = svm->vcpu.kvm;
> + struct svm_vm_data *vm_data = kvm->arch.arch_data;
> +
> + mutex_lock(&kvm->slots_lock);
> +
> + /* Check if we have already allocated vAPIC backing
> + * page for this vCPU. If not, we need to realloc
> + * a new one and re-assign all other vCPU.
> + */
> + if (kvm->arch.apic_access_page_done &&
> + (id > vm_data->avic_max_vcpu_id)) {
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + avic_unalloc_bk_page(vcpu);
> +
> + __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
> + 0, 0);
> + realloc = true;
> + vm_data->avic_max_vcpu_id = 0;
> + }
> +
> + /*
> + * We are allocating vAPIC backing page
> + * upto the max vCPU ID
> + */
> + if (id >= vm_data->avic_max_vcpu_id) {
> + ret = __x86_set_memory_region(kvm,
> + APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
> + APIC_DEFAULT_PHYS_BASE,
> + PAGE_SIZE * (id + 1));
Why is this necessary? The APIC access page is a peculiarity of Intel
processors (and the special memslot for only needs to map 0xfee00000 to
0xfee00fff; after that there is the MSI area).
> + if (ret)
> + goto out;
> +
> + vm_data->avic_max_vcpu_id = id;
> + }
> +
> + /* Reinit vAPIC backing page for exisinting vcpus */
> + if (realloc)
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + avic_init_bk_page(vcpu);
Why is this necessary?
> + avic_init_bk_page(&svm->vcpu);
> +
> + kvm->arch.apic_access_page_done = true;
> +
> +out:
> + mutex_unlock(&kvm->slots_lock);
> + return ret;
> +}
> +
> +static void avic_vm_uninit(struct kvm *kvm)
> +{
> + struct svm_vm_data *vm_data = kvm->arch.arch_data;
> +
> + if (!vm_data)
> + return;
> +
> + if (vm_data->avic_log_ait_page)
> + __free_page(vm_data->avic_log_ait_page);
> + if (vm_data->avic_phy_ait_page)
> + __free_page(vm_data->avic_phy_ait_page);
> + kfree(vm_data);
> + kvm->arch.arch_data = NULL;
> +}
> +
> +static void avic_vcpu_uninit(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> + struct svm_vm_data *vm_data = vcpu->kvm->arch.arch_data;
> +
> + if (!avic)
> + return;
> +
> + avic_unalloc_bk_page(vcpu);
> + svm->vcpu.arch.apic->regs = svm->in_kernel_lapic_regs;
> + svm->in_kernel_lapic_regs = NULL;
> +
> + if (vm_data &&
> + (atomic_read(&vm_data->count) == 0 ||
> + atomic_dec_and_test(&vm_data->count)))
> + avic_vm_uninit(vcpu->kvm);
Add a new kvm_x86_ops callback .free_vcpus and call it from
kvm_free_vcpus; then count is not necessary.
We probably should use it for Intel too. The calls to
x86_set_memory_region in kvm_arch_destroy_vm are hideous.
> +}
> +
> +static atomic_t avic_tag_gen = ATOMIC_INIT(1);
> +
> +static inline u32 avic_get_next_tag(void)
> +{
> + u32 tag = atomic_read(&avic_tag_gen);
> +
> + atomic_inc(&avic_tag_gen);
> + return tag;
> +}
> +
> +static int avic_vm_init(struct kvm *kvm)
> +{
> + int err = -ENOMEM;
> + struct svm_vm_data *vm_data;
> + struct page *avic_phy_ait_page;
> + struct page *avic_log_ait_page;
This is probably also best moved to a new callback .init_vm (called from
kvm_arch_init_vm).
> + vm_data = kzalloc(sizeof(struct svm_vm_data),
> + GFP_KERNEL);
> + if (!vm_data)
> + return err;
> +
> + kvm->arch.arch_data = vm_data;
> + atomic_set(&vm_data->count, 0);
> +
> + /* Allocating physical APIC ID table (4KB) */
> + avic_phy_ait_page = alloc_page(GFP_KERNEL);
> + if (!avic_phy_ait_page)
> + goto free_avic;
> +
> + vm_data->avic_phy_ait_page = avic_phy_ait_page;
> + clear_page(page_address(avic_phy_ait_page));
You can use get_zeroed_page and free_page, instead of
alloc_page/clear_page/__free_page.
Thanks,
Paolo
> + /* Allocating logical APIC ID table (4KB) */
> + avic_log_ait_page = alloc_page(GFP_KERNEL);
> + if (!avic_log_ait_page)
> + goto free_avic;
> +
> + vm_data->avic_log_ait_page = avic_log_ait_page;
> + clear_page(page_address(avic_log_ait_page));
> +
> + vm_data->avic_tag = avic_get_next_tag();
> +
> + return 0;
> +
> +free_avic:
> + avic_vm_uninit(kvm);
> + return err;
> +}
> +
> +static int avic_vcpu_init(struct kvm *kvm, struct vcpu_svm *svm, int id)
> +{
> + int err;
> + struct svm_vm_data *vm_data = NULL;
> +
> + /* Note: svm_vm_data is per VM */
> + if (!kvm->arch.arch_data) {
> + err = avic_vm_init(kvm);
> + if (err)
> + return err;
> + }
> +
> + err = avic_alloc_bk_page(svm, id);
> + if (err) {
> + avic_vcpu_uninit(&svm->vcpu);
> + return err;
> + }
> +
> + vm_data = kvm->arch.arch_data;
> + atomic_inc(&vm_data->count);
> +
> + return 0;
> }
>
> static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> @@ -1131,6 +1510,9 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>
> kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy);
> kvm_register_write(vcpu, VCPU_REGS_RDX, eax);
> +
> + if (avic && !init_event)
> + avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
> }
>
> static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
> @@ -1169,6 +1551,12 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
> if (!hsave_page)
> goto free_page3;
>
> + if (avic) {
> + err = avic_vcpu_init(kvm, svm, id);
> + if (err)
> + goto free_page4;
> + }
> +
> svm->nested.hsave = page_address(hsave_page);
>
> svm->msrpm = page_address(msrpm_pages);
> @@ -1187,6 +1575,8 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
>
> return &svm->vcpu;
>
> +free_page4:
> + __free_page(hsave_page);
> free_page3:
> __free_pages(nested_msrpm_pages, MSRPM_ALLOC_ORDER);
> free_page2:
> @@ -1209,6 +1599,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
> __free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
> __free_page(virt_to_page(svm->nested.hsave));
> __free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
> + avic_vcpu_uninit(vcpu);
> kvm_vcpu_uninit(vcpu);
> kmem_cache_free(kvm_vcpu_cache, svm);
> }
> @@ -3382,6 +3773,7 @@ static void dump_vmcb(struct kvm_vcpu *vcpu)
> pr_err("%-20s%08x\n", "exit_int_info_err:", control->exit_int_info_err);
> pr_err("%-20s%lld\n", "nested_ctl:", control->nested_ctl);
> pr_err("%-20s%016llx\n", "nested_cr3:", control->nested_cr3);
> + pr_err("%-20s%016llx\n", "avic_vapic_bar:", control->avic_vapic_bar);
> pr_err("%-20s%08x\n", "event_inj:", control->event_inj);
> pr_err("%-20s%08x\n", "event_inj_err:", control->event_inj_err);
> pr_err("%-20s%lld\n", "lbr_ctl:", control->lbr_ctl);
> @@ -3613,11 +4005,31 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
>
> static bool svm_get_enable_apicv(void)
> {
> - return false;
> + return avic;
> +}
> +
> +static void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
> +{
> }
>
> +static void svm_hwapic_isr_update(struct kvm *kvm, int isr)
> +{
> +}
> +
> +/* Note: Currently only used by Hyper-V. */
> static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> {
> + struct vcpu_svm *svm = to_svm(vcpu);
> + struct vmcb *vmcb = svm->vmcb;
> +
> + if (!avic)
> + return;
> +
> + if (!svm->in_kernel_lapic_regs)
> + return;
> +
> + svm->vcpu.arch.apic->regs = svm->in_kernel_lapic_regs;
> + vmcb->control.avic_enable = 0;
> }
>
> static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
> @@ -4387,6 +4799,8 @@ static struct kvm_x86_ops svm_x86_ops = {
> .refresh_apicv_exec_ctrl = svm_refresh_apicv_exec_ctrl,
> .load_eoi_exitmap = svm_load_eoi_exitmap,
> .sync_pir_to_irr = svm_sync_pir_to_irr,
> + .hwapic_irr_update = svm_hwapic_irr_update,
> + .hwapic_isr_update = svm_hwapic_isr_update,
>
> .set_tss_addr = svm_set_tss_addr,
> .get_tdp_level = get_npt_level,
>
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-15 18:20 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rd0Nt-51D-13@gated-at.bofh.it> |
| In reply to | #1351787 |
Hi
On 03/07/2016 11:41 PM, Paolo Bonzini wrote:
> On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
> > [....]
>> +/* Note: This structure is per VM */
>> +struct svm_vm_data {
>> + atomic_t count;
>> + u32 ldr_mode;
>> + u32 avic_max_vcpu_id;
>> + u32 avic_tag;
>> +
>> + struct page *avic_log_ait_page;
>> + struct page *avic_phy_ait_page;
>
> You can put these directly in kvm_arch. Do not use abbreviations:
>
> struct page *avic_logical_apic_id_table_page;
> struct page *avic_physical_apic_id_table_page;
>
Actually, the reason I would like to introduce this per-arch specific
structure is because I feel that it is easier to manage these
processor-specific variable/data-structure. If we add all these directly
into kvm_arch, which is shared b/w SVM and VMX, it is more difficult to
tell which one is used in the different code base.
>> [...]
>> + memcpy(vapic_bkpg, svm->in_kernel_lapic_regs, PAGE_SIZE);
>> + svm->vcpu.arch.apic->regs = vapic_bkpg;
>
> Can you explain the flipping logic, and why you cannot just use the
> existing apic.regs?
Please see "explanation 1" below.
>> [...]
>> +static struct svm_avic_phy_ait_entry *
>> +avic_get_phy_ait_entry(struct kvm_vcpu *vcpu, int index)
>> +{
>> + [.....]
>> +}
>> +
>> +struct svm_avic_log_ait_entry *
>> +avic_get_log_ait_entry(struct kvm_vcpu *vcpu, u8 mda, bool is_flat)
>> +{
>> + [.....]
>> +}
>
> Instead of these functions, create a complete function to handle APIC_ID
> and APIC_LDR writes. Then use kmap/kunmap instead of page_address.
>
Ok. May I ask why we are against using page_address? I have see that
used in several places in the code.
>> [...]
>> +static int avic_alloc_bk_page(struct vcpu_svm *svm, int id)
>> +{
>> + int ret = 0, i;
>> + bool realloc = false;
>> + struct kvm_vcpu *vcpu;
>> + struct kvm *kvm = svm->vcpu.kvm;
>> + struct svm_vm_data *vm_data = kvm->arch.arch_data;
>> +
>> + mutex_lock(&kvm->slots_lock);
>> +
>> + /* Check if we have already allocated vAPIC backing
>> + * page for this vCPU. If not, we need to realloc
>> + * a new one and re-assign all other vCPU.
>> + */
>> + if (kvm->arch.apic_access_page_done &&
>> + (id > vm_data->avic_max_vcpu_id)) {
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + avic_unalloc_bk_page(vcpu);
>> +
>> + __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
>> + 0, 0);
>> + realloc = true;
>> + vm_data->avic_max_vcpu_id = 0;
>> + }
>> +
>> + /*
>> + * We are allocating vAPIC backing page
>> + * upto the max vCPU ID
>> + */
>> + if (id >= vm_data->avic_max_vcpu_id) {
>> + ret = __x86_set_memory_region(kvm,
>> + APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
>> + APIC_DEFAULT_PHYS_BASE,
>> + PAGE_SIZE * (id + 1));
>
> Why is this necessary? The APIC access page is a peculiarity of Intel
> processors (and the special memslot for only needs to map 0xfee00000 to
> 0xfee00fff; after that there is the MSI area).
>
Please see "explanation 1" below.
>> [...]
>> + if (ret)
>> + goto out;
>> +
>> + vm_data->avic_max_vcpu_id = id;
>> + }
>> +
>> + /* Reinit vAPIC backing page for exisinting vcpus */
>> + if (realloc)
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + avic_init_bk_page(vcpu);
>
> Why is this necessary?
Explanation 1:
The current lapic regs page is allocated using get_zeroed_page(), which
can be paged out. If I use these pages for AVIC backing pages, it seems
to cause VM to slow down quite a bit due to a lot of page faults.
Currently, the AVIC backing pages are acquired from __x86_set_memory
region() with APIC_ACCESS_PAGE_PRIVATE_MEMSLOT, which maps the pages for
address 0xfee00000 and above for VM to use. I mostly grab this from the
VMX implementation in alloc_apic_access_page().
However, the memslot requires specification of the size at the time when
calling __x86_set_memory_region(). However, I can't seem to figure out
where I can get the number of vcpus at the time when we creating VM.
Therefore, I have to track the vcpu creation, and re-acquire larger
memslot every time vcpu_create() is called.
I was not sure if this is the right approach, any suggestion for this part.
Thanks,
Suravee
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-15 18:30 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rd0X8-566-11@gated-at.bofh.it> |
| In reply to | #1358159 |
On 15/03/2016 18:09, Suravee Suthikulpanit wrote:
> Hi
>
> On 03/07/2016 11:41 PM, Paolo Bonzini wrote:
>> On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
>> > [....]
>>> +/* Note: This structure is per VM */
>>> +struct svm_vm_data {
>>> + atomic_t count;
>>> + u32 ldr_mode;
>>> + u32 avic_max_vcpu_id;
>>> + u32 avic_tag;
>>> +
>>> + struct page *avic_log_ait_page;
>>> + struct page *avic_phy_ait_page;
>>
>> You can put these directly in kvm_arch. Do not use abbreviations:
>>
>> struct page *avic_logical_apic_id_table_page;
>> struct page *avic_physical_apic_id_table_page;
>>
>
> Actually, the reason I would like to introduce this per-arch specific
> structure is because I feel that it is easier to manage these
> processor-specific variable/data-structure. If we add all these directly
> into kvm_arch, which is shared b/w SVM and VMX, it is more difficult to
> tell which one is used in the different code base.
You're right, but adding a pointer makes things slower and larger.
Using an anonymous union would work. For now, I prefer to have the
fields directly in kvm_arch.
>>> [...]
>>> +static struct svm_avic_phy_ait_entry *
>>> +avic_get_phy_ait_entry(struct kvm_vcpu *vcpu, int index)
>>> +{
>>> + [.....]
>>> +}
>>> +
>>> +struct svm_avic_log_ait_entry *
>>> +avic_get_log_ait_entry(struct kvm_vcpu *vcpu, u8 mda, bool is_flat)
>>> +{
>>> + [.....]
>>> +}
>>
>> Instead of these functions, create a complete function to handle APIC_ID
>> and APIC_LDR writes. Then use kmap/kunmap instead of page_address.
>>
>
> Ok. May I ask why we are against using page_address? I have see that
> used in several places in the code.
You're right, I guess page_address is okay for pages that were allocated
with alloc_page().
>> Why is this necessary? The APIC access page is a peculiarity of Intel
>> processors (and the special memslot for only needs to map 0xfee00000 to
>> 0xfee00fff; after that there is the MSI area).
>
> The current lapic regs page is allocated using get_zeroed_page(), which
> can be paged out. If I use these pages for AVIC backing pages, it seems
> to cause VM to slow down quite a bit due to a lot of page faults.
What causes the lapic regs page to be paged out?
> Currently, the AVIC backing pages are acquired from __x86_set_memory
> region() with APIC_ACCESS_PAGE_PRIVATE_MEMSLOT, which maps the pages for
> address 0xfee00000 and above for VM to use. I mostly grab this from the
> VMX implementation in alloc_apic_access_page().
>
> However, the memslot requires specification of the size at the time when
> calling __x86_set_memory_region(). However, I can't seem to figure out
> where I can get the number of vcpus at the time when we creating VM.
> Therefore, I have to track the vcpu creation, and re-acquire larger
> memslot every time vcpu_create() is called.
The purpose of the APIC_ACCESS_PAGE_PRIVATE_MEMSLOT is very specific:
it is there to provide a mapping for 0xfee00000 because Intel processors
trap writes between 0xfee00000 and 0xfee00fff, but otherwise ignore the
contents of the page you map there. Intel processors only need
something to compare the physical address with, they don't care about
the data in the page, so they have one page per VM (they could really
use a single page in the whole system---one per VM is just how KVM works
right now). It is a peculiar design, and one that you should probably
ignore in your AVIC patches.
The AVIC backing page is more similar to Intel's "virtual-APIC page".
You can see that vmx.c just uses lapic->regs for it.
if (cpu_has_vmx_tpr_shadow() && !init_event) {
vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, 0);
if (cpu_need_tpr_shadow(vcpu))
vmcs_write64(VIRTUAL_APIC_PAGE_ADDR,
__pa(vcpu->arch.apic->regs));
vmcs_write32(TPR_THRESHOLD, 0);
}
Paolo
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-16 07:30 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rdd7X-4Wr-3@gated-at.bofh.it> |
| In reply to | #1358163 |
Hi,
On 03/16/2016 12:22 AM, Paolo Bonzini wrote:
>>> Why is this necessary? The APIC access page is a peculiarity of Intel
>>> >>processors (and the special memslot for only needs to map 0xfee00000 to
>>> >>0xfee00fff; after that there is the MSI area).
>> >
>> >The current lapic regs page is allocated using get_zeroed_page(), which
>> >can be paged out. If I use these pages for AVIC backing pages, it seems
>> >to cause VM to slow down quite a bit due to a lot of page faults.
> What causes the lapic regs page to be paged out?
>
This is mainly causing a large number of VMEXIT due to NPF. In my test
running hackbench in the guest. The following are perf result profiling
for 10 seconds in the host in two cases:
CASE1: Using x86_set_memory_region() for AVIC backing page
# ./perf-vmexit.sh 10
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.813 MB perf.data.guest (30356 samples) ]
Analyze events for all VMs, all VCPUs:
VM-EXIT Samples Samples% Time% Min Time Max
Time Avg time
interrupt 10042 66.30% 81.33% 0.43us
202.50us 7.43us ( +- 1.20% )
msr 5004 33.04% 15.76% 0.73us
12.21us 2.89us ( +- 0.43% )
pause 58 0.38% 0.18% 0.56us
5.88us 2.92us ( +- 6.43% )
npf 35 0.23% 2.01% 6.41us
207.78us 52.70us ( +- 23.67% )
nmi 4 0.03% 0.02% 2.31us
4.67us 3.49us ( +- 14.26% )
io 3 0.02% 0.70% 82.75us
360.90us 214.28us ( +- 37.64% )
avic_incomp_ipi 1 0.01% 0.00% 2.17us
2.17us 2.17us ( +- 0.00% )
Total Samples:15147, Total events handled time:91715.78us.
CASE2: Using the lapic regs page for AVIC backing page.
# ./perf-vmexit.sh 10
[ perf record: Woken up 255 times to write data ]
[ perf record: Captured and wrote 509.202 MB perf.data.guest (5718856
samples) ]
Analyze events for all VMs, all VCPUs:
VM-EXIT Samples Samples% Time% Min Time Max
Time Avg time
npf 1897710 99.33% 98.08% 1.09us
243.22us 1.67us ( +- 0.04% )
interrupt 7818 0.41% 1.44% 0.44us
216.55us 5.97us ( +- 1.92% )
msr 5001 0.26% 0.45% 0.68us
12.58us 2.89us ( +- 0.50% )
pause 25 0.00% 0.00% 0.71us
4.23us 2.03us ( +- 10.76% )
io 4 0.00% 0.03% 73.91us
337.29us 206.74us ( +- 26.38% )
nmi 1 0.00% 0.00% 5.92us
5.92us 5.92us ( +- 0.00% )
Total Samples:1910559, Total events handled time:3229214.64us.
Thanks,
Suravee
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-16 08:30 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rde42-5FY-7@gated-at.bofh.it> |
| In reply to | #1358550 |
On 16/03/2016 07:22, Suravee Suthikulpanit wrote: > This is mainly causing a large number of VMEXIT due to NPF. Got it, it's here in the manual: "System software is responsible for setting up a translation in the nested page table granting guest read and write permissions for accesses to the vAPIC Backing Page in SPA space. AVIC hardware walks the nested page table to check permissions, but does not use the SPA address specified in the leaf page table entry. Instead, AVIC hardware finds this address in the AVIC_BACKING_PAGE pointer field of the VMCB". Strictly speaking the address of the 0xFEE00000 translation is unnecessary and it could be all zeroes, but I suggest that you set up an APIC access page like Intel does (4k only), using the special memslot. The AVIC backing page can then point to lapic->regs. Thanks for the explanation! Paolo > CASE1: Using x86_set_memory_region() for AVIC backing page > > # ./perf-vmexit.sh 10 > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 2.813 MB perf.data.guest (30356 > samples) ] > > > Analyze events for all VMs, all VCPUs: > > VM-EXIT Samples Samples% Time% Min Time Max > Time Avg time > > interrupt 10042 66.30% 81.33% 0.43us > 202.50us 7.43us ( +- 1.20% ) > msr 5004 33.04% 15.76% 0.73us > 12.21us 2.89us ( +- 0.43% ) > pause 58 0.38% 0.18% 0.56us > 5.88us 2.92us ( +- 6.43% ) > npf 35 0.23% 2.01% 6.41us > 207.78us 52.70us ( +- 23.67% ) > nmi 4 0.03% 0.02% 2.31us > 4.67us 3.49us ( +- 14.26% ) > io 3 0.02% 0.70% 82.75us > 360.90us 214.28us ( +- 37.64% ) > avic_incomp_ipi 1 0.01% 0.00% 2.17us > 2.17us 2.17us ( +- 0.00% ) > > Total Samples:15147, Total events handled time:91715.78us. > > > CASE2: Using the lapic regs page for AVIC backing page. > > # ./perf-vmexit.sh 10 > [ perf record: Woken up 255 times to write data ] > [ perf record: Captured and wrote 509.202 MB perf.data.guest (5718856 > samples) ] > > > Analyze events for all VMs, all VCPUs: > > VM-EXIT Samples Samples% Time% Min Time Max > Time Avg time > > npf 1897710 99.33% 98.08% 1.09us > 243.22us 1.67us ( +- 0.04% ) > interrupt 7818 0.41% 1.44% 0.44us > 216.55us 5.97us ( +- 1.92% ) > msr 5001 0.26% 0.45% 0.68us > 12.58us 2.89us ( +- 0.50% ) > pause 25 0.00% 0.00% 0.71us > 4.23us 2.03us ( +- 10.76% ) > io 4 0.00% 0.03% 73.91us > 337.29us 206.74us ( +- 26.38% ) > nmi 1 0.00% 0.00% 5.92us > 5.92us 5.92us ( +- 0.00% ) > > Total Samples:1910559, Total events handled time:3229214.64us. > > Thanks, > Suravee > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-16 09:30 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rdf08-6ju-69@gated-at.bofh.it> |
| In reply to | #1358573 |
Hi, On 03/16/2016 02:20 PM, Paolo Bonzini wrote: > > On 16/03/2016 07:22, Suravee Suthikulpanit wrote: >> >This is mainly causing a large number of VMEXIT due to NPF. > Got it, it's here in the manual: "System software is responsible for > setting up a translation in the nested page table granting guest read > and write permissions for accesses to the vAPIC Backing Page in SPA > space. AVIC hardware walks the nested page table to check permissions, > but does not use the SPA address specified in the leaf page table entry. > Instead, AVIC hardware finds this address in the AVIC_BACKING_PAGE > pointer field of the VMCB". > > Strictly speaking the address of the 0xFEE00000 translation is > unnecessary and it could be all zeroes, but I suggest that you set up an > APIC access page like Intel does (4k only), using the special memslot. > The AVIC backing page can then point to lapic->regs. > > Thanks for the explanation! > > Paolo > Ahh... you are right, this works also. Thanks for the pointer. I'm fixing this, doing some more testing, and cleaning up the code. This has simplify the init logic quite a bit. Thanks for suggestion, Suravee
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-16 12:20 +0100 |
| Subject | Re: [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support |
| Message-ID | <rdhEC-8cQ-5@gated-at.bofh.it> |
| In reply to | #1358691 |
On 16/03/2016 09:21, Suravee Suthikulpanit wrote: >> Strictly speaking the address of the 0xFEE00000 translation is >> unnecessary and it could be all zeroes, but I suggest that you set up an >> APIC access page like Intel does (4k only), using the special memslot. >> The AVIC backing page can then point to lapic->regs. > > Ahh... you are right, this works also. Thanks for the pointer. I'm > fixing this, doing some more testing, and cleaning up the code. This has > simplify the init logic quite a bit. Awesome, thanks. Paolo
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 22:00 +0100 |
| Subject | [PART1 RFC v2 01/10] KVM: x86: Misc LAPIC changes to exposes helper functions |
| Message-ID | <r94Zk-40K-13@gated-at.bofh.it> |
| In reply to | #1350575 |
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Exporting LAPIC utility functions and macros to reuse.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/kvm/lapic.c | 51 +++++++++++++++++++++++++++------------------------
arch/x86/kvm/lapic.h | 7 +++++++
2 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3a045f3..a8d91b89 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -59,9 +59,8 @@
/* #define apic_debug(fmt,arg...) printk(KERN_WARNING fmt,##arg) */
#define apic_debug(fmt, arg...)
-#define APIC_LVT_NUM 6
/* 14 is the version for Xeon and Pentium 8.4.8*/
-#define APIC_VERSION (0x14UL | ((APIC_LVT_NUM - 1) << 16))
+#define APIC_VERSION (0x14UL | ((KVM_APIC_LVT_NUM - 1) << 16))
#define LAPIC_MMIO_LENGTH (1 << 12)
/* followed define is not in apicdef.h */
#define APIC_SHORT_MASK 0xc0000
@@ -94,10 +93,11 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
apic_test_vector(vector, apic->regs + APIC_IRR);
}
-static inline void apic_set_vector(int vec, void *bitmap)
+void kvm_lapic_set_vector(int vec, void *bitmap)
{
set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
}
+EXPORT_SYMBOL_GPL(kvm_lapic_set_vector);
static inline void apic_clear_vector(int vec, void *bitmap)
{
@@ -290,7 +290,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
apic_set_reg(apic, APIC_LVR, v);
}
-static const unsigned int apic_lvt_mask[APIC_LVT_NUM] = {
+static const unsigned int apic_lvt_mask[KVM_APIC_LVT_NUM] = {
LVT_MASK , /* part LVTT mask, timer mode mask added at runtime */
LVT_MASK | APIC_MODE_MASK, /* LVTTHMR */
LVT_MASK | APIC_MODE_MASK, /* LVTPC */
@@ -351,7 +351,7 @@ EXPORT_SYMBOL_GPL(kvm_apic_update_irr);
static inline void apic_set_irr(int vec, struct kvm_lapic *apic)
{
- apic_set_vector(vec, apic->regs + APIC_IRR);
+ kvm_lapic_set_vector(vec, apic->regs + APIC_IRR);
/*
* irr_pending must be true if any interrupt is pending; set it after
* APIC_IRR to avoid race with apic_clear_irr
@@ -674,6 +674,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
return false;
}
}
+EXPORT_SYMBOL_GPL(kvm_apic_match_dest);
bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
@@ -844,7 +845,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
if (trig_mode)
- apic_set_vector(vector, apic->regs + APIC_TMR);
+ kvm_lapic_set_vector(vector, apic->regs + APIC_TMR);
else
apic_clear_vector(vector, apic->regs + APIC_TMR);
}
@@ -1109,7 +1110,7 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
return container_of(dev, struct kvm_lapic, dev);
}
-static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
+int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
void *data)
{
unsigned char alignment = offset & 0xf;
@@ -1146,6 +1147,7 @@ static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
}
return 0;
}
+EXPORT_SYMBOL_GPL(kvm_lapic_reg_read);
static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
{
@@ -1163,7 +1165,7 @@ static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
if (!apic_mmio_in_range(apic, address))
return -EOPNOTSUPP;
- apic_reg_read(apic, offset, len, data);
+ kvm_lapic_reg_read(apic, offset, len, data);
return 0;
}
@@ -1348,7 +1350,7 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
}
}
-static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
+int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
{
int ret = 0;
@@ -1395,7 +1397,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
int i;
u32 lvt_val;
- for (i = 0; i < APIC_LVT_NUM; i++) {
+ for (i = 0; i < KVM_APIC_LVT_NUM; i++) {
lvt_val = kvm_apic_get_reg(apic,
APIC_LVTT + 0x10 * i);
apic_set_reg(apic, APIC_LVTT + 0x10 * i,
@@ -1467,7 +1469,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
case APIC_SELF_IPI:
if (apic_x2apic_mode(apic)) {
- apic_reg_write(apic, APIC_ICR, 0x40000 | (val & 0xff));
+ kvm_lapic_reg_write(apic, APIC_ICR, 0x40000 | (val & 0xff));
} else
ret = 1;
break;
@@ -1479,6 +1481,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
apic_debug("Local APIC Write to read-only register %x\n", reg);
return ret;
}
+EXPORT_SYMBOL_GPL(kvm_lapic_reg_write);
static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
gpa_t address, int len, const void *data)
@@ -1508,7 +1511,7 @@ static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
apic_debug("%s: offset 0x%x with length 0x%x, and value is "
"0x%x\n", __func__, offset, len, val);
- apic_reg_write(apic, offset & 0xff0, val);
+ kvm_lapic_reg_write(apic, offset & 0xff0, val);
return 0;
}
@@ -1516,7 +1519,7 @@ static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
{
if (kvm_vcpu_has_lapic(vcpu))
- apic_reg_write(vcpu->arch.apic, APIC_EOI, 0);
+ kvm_lapic_reg_write(vcpu->arch.apic, APIC_EOI, 0);
}
EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
@@ -1528,10 +1531,10 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
/* hw has done the conditional check and inst decode */
offset &= 0xff0;
- apic_reg_read(vcpu->arch.apic, offset, 4, &val);
+ kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
/* TODO: optimize to just emulate side effect w/o one more write */
- apic_reg_write(vcpu->arch.apic, offset, val);
+ kvm_lapic_reg_write(vcpu->arch.apic, offset, val);
}
EXPORT_SYMBOL_GPL(kvm_apic_write_nodecode);
@@ -1670,7 +1673,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
kvm_apic_set_id(apic, vcpu->vcpu_id);
kvm_apic_set_version(apic->vcpu);
- for (i = 0; i < APIC_LVT_NUM; i++)
+ for (i = 0; i < KVM_APIC_LVT_NUM; i++)
apic_set_reg(apic, APIC_LVTT + 0x10 * i, APIC_LVT_MASKED);
apic_update_lvtt(apic);
if (kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_LINT0_REENABLED))
@@ -2073,8 +2076,8 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
/* if this is ICR write vector before command */
if (reg == APIC_ICR)
- apic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
- return apic_reg_write(apic, reg, (u32)data);
+ kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+ return kvm_lapic_reg_write(apic, reg, (u32)data);
}
int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
@@ -2091,10 +2094,10 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
return 1;
}
- if (apic_reg_read(apic, reg, 4, &low))
+ if (kvm_lapic_reg_read(apic, reg, 4, &low))
return 1;
if (reg == APIC_ICR)
- apic_reg_read(apic, APIC_ICR2, 4, &high);
+ kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high);
*data = (((u64)high) << 32) | low;
@@ -2110,8 +2113,8 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
/* if this is ICR write vector before command */
if (reg == APIC_ICR)
- apic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
- return apic_reg_write(apic, reg, (u32)data);
+ kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+ return kvm_lapic_reg_write(apic, reg, (u32)data);
}
int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
@@ -2122,10 +2125,10 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
if (!kvm_vcpu_has_lapic(vcpu))
return 1;
- if (apic_reg_read(apic, reg, 4, &low))
+ if (kvm_lapic_reg_read(apic, reg, 4, &low))
return 1;
if (reg == APIC_ICR)
- apic_reg_read(apic, APIC_ICR2, 4, &high);
+ kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high);
*data = (((u64)high) << 32) | low;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 41bdb35..936fd10 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -7,6 +7,7 @@
#define KVM_APIC_INIT 0
#define KVM_APIC_SIPI 1
+#define KVM_APIC_LVT_NUM 6
struct kvm_timer {
struct hrtimer timer;
@@ -56,6 +57,12 @@ void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
void kvm_apic_set_version(struct kvm_vcpu *vcpu);
+void kvm_lapic_set_vector(int vec, void *bitmap);
+int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val);
+int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
+ void *data);
+bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
+ int short_hand, unsigned int dest, int dest_mode);
void __kvm_apic_update_irr(u32 *pir, void *regs);
void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 22:00 +0100 |
| Subject | [PART1 RFC v2 04/10] svm: clean up V_TPR, V_IRQ, V_INTR_PRIO, and V_INTR_MASKING |
| Message-ID | <r94Zk-40K-17@gated-at.bofh.it> |
| In reply to | #1350575 |
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Now that we have defined the bit field, use them to replace existing
macros. This patch should not have functional change.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/include/asm/svm.h | 9 ---------
arch/x86/kvm/svm.c | 24 ++++++++++++------------
2 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index db5d7af..7bb34c9 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -118,18 +118,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
#define V_TPR_MASK 0x0f
-#define V_IRQ_SHIFT 8
-#define V_IRQ_MASK (1 << V_IRQ_SHIFT)
-
-#define V_INTR_PRIO_SHIFT 16
-#define V_INTR_PRIO_MASK (0x0f << V_INTR_PRIO_SHIFT)
-
#define V_IGN_TPR_SHIFT 20
#define V_IGN_TPR_MASK (1 << V_IGN_TPR_SHIFT)
-#define V_INTR_MASKING_SHIFT 24
-#define V_INTR_MASKING_MASK (1 << V_INTR_MASKING_SHIFT)
-
#define SVM_INTERRUPT_SHADOW_MASK 1
#define SVM_IOIO_STR_SHIFT 2
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 28f8618..6ab66d0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1052,7 +1052,7 @@ static void init_vmcb(struct vcpu_svm *svm)
control->iopm_base_pa = iopm_base;
control->msrpm_base_pa = __pa(svm->msrpm);
- control->int_ctl = V_INTR_MASKING_MASK;
+ control->v_intr_masking = 1;
init_seg(&save->es);
init_seg(&save->ss);
@@ -2316,7 +2316,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
/* We always set V_INTR_MASKING and remember the old value in hflags */
if (!(svm->vcpu.arch.hflags & HF_VINTR_MASK))
- nested_vmcb->control.int_ctl &= ~V_INTR_MASKING_MASK;
+ nested_vmcb->control.v_intr_masking = 0;
/* Restore the original control entries */
copy_vmcb_control_area(vmcb, hsave);
@@ -2526,8 +2526,9 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
svm->nested.intercept = nested_vmcb->control.intercept;
svm_flush_tlb(&svm->vcpu);
- svm->vmcb->control.int_ctl = nested_vmcb->control.int_ctl | V_INTR_MASKING_MASK;
- if (nested_vmcb->control.int_ctl & V_INTR_MASKING_MASK)
+ svm->vmcb->control.int_ctl = nested_vmcb->control.int_ctl;
+ svm->vmcb->control.v_intr_masking = 1;
+ if (nested_vmcb->control.v_intr_masking)
svm->vcpu.arch.hflags |= HF_VINTR_MASK;
else
svm->vcpu.arch.hflags &= ~HF_VINTR_MASK;
@@ -2680,7 +2681,7 @@ static int clgi_interception(struct vcpu_svm *svm)
/* After a CLGI no interrupts should come */
svm_clear_vintr(svm);
- svm->vmcb->control.int_ctl &= ~V_IRQ_MASK;
+ svm->vmcb->control.v_irq = 0;
mark_dirty(svm->vmcb, VMCB_INTR);
@@ -3257,7 +3258,7 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
{
kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
svm_clear_vintr(svm);
- svm->vmcb->control.int_ctl &= ~V_IRQ_MASK;
+ svm->vmcb->control.v_irq = 0;
mark_dirty(svm->vmcb, VMCB_INTR);
++svm->vcpu.stat.irq_window_exits;
return 1;
@@ -3568,11 +3569,11 @@ static inline void svm_inject_irq(struct vcpu_svm *svm, int irq)
{
struct vmcb_control_area *control;
+
control = &svm->vmcb->control;
control->int_vector = irq;
- control->int_ctl &= ~V_INTR_PRIO_MASK;
- control->int_ctl |= V_IRQ_MASK |
- ((/*control->int_vector >> 4*/ 0xf) << V_INTR_PRIO_SHIFT);
+ control->v_intr_prio = 0xf;
+ control->v_irq = 1;
mark_dirty(svm->vmcb, VMCB_INTR);
}
@@ -3738,7 +3739,7 @@ static inline void sync_cr8_to_lapic(struct kvm_vcpu *vcpu)
return;
if (!is_cr_intercept(svm, INTERCEPT_CR8_WRITE)) {
- int cr8 = svm->vmcb->control.int_ctl & V_TPR_MASK;
+ int cr8 = svm->vmcb->control.v_tpr & V_TPR_MASK;
kvm_set_cr8(vcpu, cr8);
}
}
@@ -3752,8 +3753,7 @@ static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
return;
cr8 = kvm_get_cr8(vcpu);
- svm->vmcb->control.int_ctl &= ~V_TPR_MASK;
- svm->vmcb->control.int_ctl |= cr8 & V_TPR_MASK;
+ svm->vmcb->control.v_tpr = cr8 & V_TPR_MASK;
}
static void svm_complete_interrupts(struct vcpu_svm *svm)
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> |
|---|---|
| Date | 2016-03-04 22:00 +0100 |
| Subject | [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC |
| Message-ID | <r94Zl-40K-23@gated-at.bofh.it> |
| In reply to | #1350575 |
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Introduce VMEXIT handlers, avic_incp_ipi_interception() and
avic_noaccel_interception().
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
arch/x86/include/uapi/asm/svm.h | 9 +-
arch/x86/kvm/svm.c | 260 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 268 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
index 8a4add8..ebfdf8d 100644
--- a/arch/x86/include/uapi/asm/svm.h
+++ b/arch/x86/include/uapi/asm/svm.h
@@ -73,6 +73,8 @@
#define SVM_EXIT_MWAIT_COND 0x08c
#define SVM_EXIT_XSETBV 0x08d
#define SVM_EXIT_NPF 0x400
+#define SVM_EXIT_AVIC_INCMP_IPI 0x401
+#define SVM_EXIT_AVIC_NOACCEL 0x402
#define SVM_EXIT_ERR -1
@@ -107,8 +109,10 @@
{ SVM_EXIT_SMI, "smi" }, \
{ SVM_EXIT_INIT, "init" }, \
{ SVM_EXIT_VINTR, "vintr" }, \
+ { SVM_EXIT_CR0_SEL_WRITE, "cr0_sec_write" }, \
{ SVM_EXIT_CPUID, "cpuid" }, \
{ SVM_EXIT_INVD, "invd" }, \
+ { SVM_EXIT_PAUSE, "pause" }, \
{ SVM_EXIT_HLT, "hlt" }, \
{ SVM_EXIT_INVLPG, "invlpg" }, \
{ SVM_EXIT_INVLPGA, "invlpga" }, \
@@ -127,7 +131,10 @@
{ SVM_EXIT_MONITOR, "monitor" }, \
{ SVM_EXIT_MWAIT, "mwait" }, \
{ SVM_EXIT_XSETBV, "xsetbv" }, \
- { SVM_EXIT_NPF, "npf" }
+ { SVM_EXIT_NPF, "npf" }, \
+ { SVM_EXIT_RSM, "rsm" }, \
+ { SVM_EXIT_AVIC_INCMP_IPI, "avic_incomp_ipi" }, \
+ { SVM_EXIT_AVIC_NOACCEL, "avic_noaccel" }
#endif /* _UAPI__SVM_H */
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8f11200..a177781 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3690,6 +3690,264 @@ static int mwait_interception(struct vcpu_svm *svm)
return nop_interception(svm);
}
+enum avic_incmp_ipi_err_code {
+ AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE,
+ AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN,
+ AVIC_INCMP_IPI_ERR_INV_TARGET,
+ AVIC_INCMP_IPI_ERR_INV_BK_PAGE,
+};
+
+#define APIC_SHORT_MASK 0xc0000
+#define APIC_DEST_MASK 0x800
+static int avic_incomp_ipi_interception(struct vcpu_svm *svm)
+{
+ u32 icrh = svm->vmcb->control.exit_info_1 >> 32;
+ u32 icrl = svm->vmcb->control.exit_info_1;
+ u32 id = svm->vmcb->control.exit_info_2 >> 32;
+ u32 index = svm->vmcb->control.exit_info_2 && 0xFF;
+ struct kvm_lapic *apic = svm->vcpu.arch.apic;
+
+ pr_debug("%s: cpu=%#x, vcpu=%#x, icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
+ __func__, svm->vcpu.cpu, svm->vcpu.vcpu_id,
+ icrh, icrl, id, index);
+
+ switch (id) {
+ case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
+ /*
+ * AVIC hardware handles the generation of
+ * IPIs when the specified Message Type is Fixed
+ * (also known as fixed delivery mode) and
+ * the Trigger Mode is edge-triggered. The hardware
+ * also supports self and broadcast delivery modes
+ * specified via the Destination Shorthand(DSH)
+ * field of the ICRL. Logical and physical APIC ID
+ * formats are supported. All other IPI types cause
+ * a #VMEXIT, which needs to emulated.
+ */
+ kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
+ kvm_lapic_reg_write(apic, APIC_ICR, icrl);
+ break;
+ case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN: {
+ int i;
+ struct kvm_vcpu *vcpu;
+ struct kvm *kvm = svm->vcpu.kvm;
+ struct kvm_lapic *apic = svm->vcpu.arch.apic;
+
+ /*
+ * At this point, we expect that the AVIC HW has already
+ * set the appropriate IRR bits on the valid target
+ * vcpus. So, we just need to kick the appropriate vcpu.
+ */
+ kvm_for_each_vcpu(i, vcpu, kvm) {
+ if (!kvm_apic_match_dest(vcpu, apic,
+ icrl & APIC_SHORT_MASK,
+ GET_APIC_DEST_FIELD(icrh),
+ icrl & APIC_DEST_MASK))
+ continue;
+
+ kvm_vcpu_kick(vcpu);
+ }
+ break;
+ }
+ case AVIC_INCMP_IPI_ERR_INV_TARGET:
+ pr_err("%s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
+ __func__, icrh, icrl, index);
+ BUG();
+ break;
+ case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
+ pr_err("%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
+ __func__, icrh, icrl, index);
+ BUG();
+ break;
+ default:
+ pr_err("Unknown IPI interception\n");
+ }
+
+ return 1;
+}
+
+static int avic_noaccel_trap_write(struct vcpu_svm *svm)
+{
+ u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
+ struct svm_vm_data *vm_data = svm->vcpu.kvm->arch.arch_data;
+ struct kvm_lapic *apic = svm->vcpu.arch.apic;
+ u32 reg = *avic_get_bk_page_entry(svm, offset);
+
+ pr_debug("%s: offset=%#x, val=%#x, (cpu=%x) (vcpu_id=%x)\n",
+ __func__, offset, reg, svm->vcpu.cpu, svm->vcpu.vcpu_id);
+
+ switch (offset) {
+ case APIC_ID: {
+ u32 aid = (reg >> 24) & 0xff;
+ struct svm_avic_phy_ait_entry *o_ent =
+ avic_get_phy_ait_entry(&svm->vcpu, svm->vcpu.vcpu_id);
+ struct svm_avic_phy_ait_entry *n_ent =
+ avic_get_phy_ait_entry(&svm->vcpu, aid);
+
+ if (!n_ent || !o_ent)
+ return 0;
+
+ pr_debug("%s: APIC_ID=%#x (id=%x)\n", __func__, reg, aid);
+
+ /* We need to move phy_apic_entry to new offset */
+ *n_ent = *o_ent;
+ *((u64 *)o_ent) = 0ULL;
+ break;
+ }
+ case APIC_LDR: {
+ int ret, lid;
+ int dlid = (reg >> 24) & 0xff;
+
+ if (!dlid)
+ return 0;
+
+ lid = ffs(dlid) - 1;
+ pr_debug("%s: LDR=%0#10x (lid=%x)\n", __func__, reg, lid);
+ ret = avic_init_log_apic_entry(&svm->vcpu, svm->vcpu.vcpu_id,
+ lid);
+ if (ret)
+ return 0;
+
+ break;
+ }
+ case APIC_DFR: {
+ u32 mod = (*avic_get_bk_page_entry(svm, offset) >> 28) & 0xf;
+
+ pr_debug("%s: DFR=%#x (%s)\n", __func__,
+ mod, (mod == 0xf) ? "flat" : "cluster");
+
+ /*
+ * We assume that all local APICs are using the same type.
+ * If this changes, we need to rebuild the AVIC logical
+ * APID id table with subsequent write to APIC_LDR.
+ */
+ if (vm_data->ldr_mode != mod) {
+ clear_page(page_address(vm_data->avic_log_ait_page));
+ vm_data->ldr_mode = mod;
+ }
+ break;
+ }
+ case APIC_TMICT: {
+ u32 val = kvm_apic_get_reg(apic, APIC_TMICT);
+
+ pr_debug("%s: TMICT=%#x,%#x\n", __func__, val, reg);
+ break;
+ }
+ case APIC_ESR: {
+ u32 val = kvm_apic_get_reg(apic, APIC_ESR);
+
+ pr_debug("%s: ESR=%#x,%#x\n", __func__, val, reg);
+ break;
+ }
+ case APIC_LVTERR: {
+ u32 val = kvm_apic_get_reg(apic, APIC_LVTERR);
+
+ pr_debug("%s: LVTERR=%#x,%#x\n", __func__, val, reg);
+ break;
+ }
+ default:
+ break;
+ }
+
+ kvm_lapic_reg_write(apic, offset, reg);
+
+ return 1;
+}
+
+static int avic_noaccel_fault_read(struct vcpu_svm *svm)
+{
+ u32 val;
+ u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
+ struct kvm_lapic *apic = svm->vcpu.arch.apic;
+
+ pr_debug("%s: offset=%x\n", __func__, offset);
+
+ switch (offset) {
+ case APIC_TMCCT: {
+ if (kvm_lapic_reg_read(apic, offset, 4, &val))
+ return 0;
+
+ pr_debug("%s: TMCCT: rip=%#lx, next_rip=%#llx, val=%#x)\n",
+ __func__, kvm_rip_read(&svm->vcpu), svm->next_rip, val);
+
+ *avic_get_bk_page_entry(svm, offset) = val;
+ break;
+ }
+ default:
+ pr_debug("%s: (rip=%#lx), offset=%#x\n", __func__,
+ kvm_rip_read(&svm->vcpu), offset);
+ break;
+ }
+
+ return 1;
+}
+
+static int avic_noaccel_fault_write(struct vcpu_svm *svm)
+{
+ u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
+
+ pr_debug("%s: offset=%x\n", __func__, offset);
+
+ switch (offset) {
+ case APIC_ARBPRI: /* APR: Arbitration Priority Register */
+ case APIC_TMCCT: /* Timer Current Count */
+ /* TODO */
+ break;
+ default:
+ BUG();
+ }
+
+ return 1;
+}
+
+static int avic_noaccel_interception(struct vcpu_svm *svm)
+{
+ int ret = 0;
+ u32 offset = svm->vmcb->control.exit_info_1 & 0xFF0;
+ u32 rw = (svm->vmcb->control.exit_info_1 >> 32) & 0x1;
+ u32 vector = svm->vmcb->control.exit_info_2 & 0xFFFFFFFF;
+
+ pr_debug("%s: offset=%#x, rw=%#x, vector=%#x, vcpu_id=%#x, cpu=%#x\n",
+ __func__, offset, rw, vector, svm->vcpu.vcpu_id, svm->vcpu.cpu);
+
+ BUG_ON(offset >= 0x400);
+
+ switch (offset) {
+ case APIC_ID:
+ case APIC_EOI:
+ case APIC_RRR:
+ case APIC_LDR:
+ case APIC_DFR:
+ case APIC_SPIV:
+ case APIC_ESR:
+ case APIC_ICR:
+ case APIC_LVTT:
+ case APIC_LVTTHMR:
+ case APIC_LVTPC:
+ case APIC_LVT0:
+ case APIC_LVT1:
+ case APIC_LVTERR:
+ case APIC_TMICT:
+ case APIC_TDCR: {
+ /* Handling Trap */
+ if (!rw) /* Trap read should never happens */
+ BUG();
+ ret = avic_noaccel_trap_write(svm);
+ break;
+ }
+ default: {
+ /* Handling Fault */
+ if (rw)
+ ret = avic_noaccel_fault_write(svm);
+ else
+ ret = avic_noaccel_fault_read(svm);
+ skip_emulated_instruction(&svm->vcpu);
+ }
+ }
+
+ return ret;
+}
+
static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
[SVM_EXIT_READ_CR0] = cr_interception,
[SVM_EXIT_READ_CR3] = cr_interception,
@@ -3753,6 +4011,8 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
[SVM_EXIT_XSETBV] = xsetbv_interception,
[SVM_EXIT_NPF] = pf_interception,
[SVM_EXIT_RSM] = emulate_on_interception,
+ [SVM_EXIT_AVIC_INCMP_IPI] = avic_incomp_ipi_interception,
+ [SVM_EXIT_AVIC_NOACCEL] = avic_noaccel_interception,
};
static void dump_vmcb(struct kvm_vcpu *vcpu)
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-03-07 17:00 +0100 |
| Subject | Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC |
| Message-ID | <ra5JF-4Li-15@gated-at.bofh.it> |
| In reply to | #1350586 |
On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
> +#define SVM_EXIT_AVIC_INCMP_IPI 0x401
> +#define SVM_EXIT_AVIC_NOACCEL 0x402
>
> +enum avic_incmp_ipi_err_code {
> + AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE,
> + AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN,
> + AVIC_INCMP_IPI_ERR_INV_TARGET,
> + AVIC_INCMP_IPI_ERR_INV_BK_PAGE,
> +};
> +
Please do not abbreviate names and use the same definition as the
manual; these are "IPI delivery failure causes", which we can shorten to
"IPI failure causes". Putting it together gives:
#define SVM_EXIT_AVIC_INCOMPLETE_IPI 0x401
#define SVM_EXIT_AVIC_UNACCELERATED_ACCESS 0x402
enum avic_ipi_failure_cause {
AVIC_IPI_FAILURE_INVALID_INT_TYPE,
AVIC_IPI_FAILURE_TARGET_NOT_RUNNING,
AVIC_IPI_FAILURE_INVALID_TARGET,
AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
};
Likewise, do not abbreviate function names.
> +#define APIC_SHORT_MASK 0xc0000
> +#define APIC_DEST_MASK 0x800
Please move these to lapic.h too in patch 1.
> + case AVIC_INCMP_IPI_ERR_INV_TARGET:
> + pr_err("%s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
> + __func__, icrh, icrl, index);
> + BUG();
> + break;
> + case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
> + pr_err("%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> + __func__, icrh, icrl, index);
> + BUG();
> + break;
Please use WARN(1, "%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
__func__, icrh, icrl, index) (and likewise for invalid target) instead
of BUG().
>
> + pr_debug("%s: offset=%#x, val=%#x, (cpu=%x) (vcpu_id=%x)\n",
> + __func__, offset, reg, svm->vcpu.cpu, svm->vcpu.vcpu_id);
> +
> + switch (offset) {
> + case APIC_ID: {
> + u32 aid = (reg >> 24) & 0xff;
> + struct svm_avic_phy_ait_entry *o_ent =
> + avic_get_phy_ait_entry(&svm->vcpu, svm->vcpu.vcpu_id);
> + struct svm_avic_phy_ait_entry *n_ent =
> + avic_get_phy_ait_entry(&svm->vcpu, aid);
> +
> + if (!n_ent || !o_ent)
> + return 0;
> +
> + pr_debug("%s: APIC_ID=%#x (id=%x)\n", __func__, reg, aid);
> +
> + /* We need to move phy_apic_entry to new offset */
> + *n_ent = *o_ent;
> + *((u64 *)o_ent) = 0ULL;
> + break;
> + }
> + case APIC_LDR: {
> + int ret, lid;
> + int dlid = (reg >> 24) & 0xff;
> +
> + if (!dlid)
> + return 0;
> +
> + lid = ffs(dlid) - 1;
> + pr_debug("%s: LDR=%0#10x (lid=%x)\n", __func__, reg, lid);
> + ret = avic_init_log_apic_entry(&svm->vcpu, svm->vcpu.vcpu_id,
> + lid);
> + if (ret)
> + return 0;
> +
> + break;
> + }
> + case APIC_DFR: {
> + u32 mod = (*avic_get_bk_page_entry(svm, offset) >> 28) & 0xf;
> +
> + pr_debug("%s: DFR=%#x (%s)\n", __func__,
> + mod, (mod == 0xf) ? "flat" : "cluster");
> +
> + /*
> + * We assume that all local APICs are using the same type.
> + * If this changes, we need to rebuild the AVIC logical
> + * APID id table with subsequent write to APIC_LDR.
> + */
> + if (vm_data->ldr_mode != mod) {
> + clear_page(page_address(vm_data->avic_log_ait_page));
> + vm_data->ldr_mode = mod;
> + }
> + break;
> + }
> + case APIC_TMICT: {
> + u32 val = kvm_apic_get_reg(apic, APIC_TMICT);
> +
> + pr_debug("%s: TMICT=%#x,%#x\n", __func__, val, reg);
> + break;
> + }
> + case APIC_ESR: {
> + u32 val = kvm_apic_get_reg(apic, APIC_ESR);
> +
> + pr_debug("%s: ESR=%#x,%#x\n", __func__, val, reg);
> + break;
> + }
> + case APIC_LVTERR: {
> + u32 val = kvm_apic_get_reg(apic, APIC_LVTERR);
> +
> + pr_debug("%s: LVTERR=%#x,%#x\n", __func__, val, reg);
> + break;
> + }
> + default:
> + break;
Please use a single tracepoint instead of all these pr_debug statements.
The tracepoint can convert APIC register offsets to APIC register
names, like the existing kvm_apic tracepoint. Also please remove the
TMICT/ESR/LVTERR cases, since they do nothing but debugging.
Notice how a single tracepoint can be used for multiple functions, the
example being kvm_avic as well.
Existing tracepoints in fact make most of the pr_debug statements
unnecessary.
Paolo
[toc] | [prev] | [next] | [standalone]
| From | Radim Krčmář <rkrcmar@redhat.com> |
|---|---|
| Date | 2016-03-08 23:10 +0100 |
| Subject | Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC |
| Message-ID | <raxZg-730-9@gated-at.bofh.it> |
| In reply to | #1351743 |
2016-03-07 16:58+0100, Paolo Bonzini:
>> + case AVIC_INCMP_IPI_ERR_INV_TARGET:
>> + pr_err("%s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
>> + __func__, icrh, icrl, index);
>> + BUG();
>> + break;
>> + case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
>> + pr_err("%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
>> + __func__, icrh, icrl, index);
>> + BUG();
>> + break;
>
> Please use WARN(1, "%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> __func__, icrh, icrl, index) (and likewise for invalid target) instead
> of BUG().
I think that if we hit one of these, then WARNs would just flood the
log. I'd prefer WARN_ONCE on AVIC_INCMP_IPI_ERR_INV_BK_PAGE.
(Btw. aren't icr and idx are pointless on this error? and the function
name should be printed by WARN.)
Invalid target is triggerable by the guest (by sending IPI to a
non-existent LAPIC), so warning log level seems too severe.
pr_info_ratelimited() or nothing would be better.
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web