* [RFC PATCH 0/9] Introduce AMD SVM AVIC
@ 2016-09-19 5:52 Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops Suravee Suthikulpanit
` (11 more replies)
0 siblings, 12 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 4544 bytes --]
GITHUB
======
Latest git tree can be found at:
http://github.com/ssuthiku/xen.git xen_avic_part1_v1
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 injection 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 Xen parameter svm-avic=1.
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.
OVERALL PERFORMANCE
===================
Currently, AVIC is available on the AMD family 15h models 6Xh
(Carrizo) processors and newer. Here, the Carizzo is used to collect
performance data shown below.
Generally, SVM AVIC alone (w/o IOMMU AVIC) should provide overall speed up
for HVM guest since it does not require #vmexit into the hypervisor to
emulate certain guest accesses to local APIC registers.
It should also improve performance when hypervisor wants to inject
interrupts into a running vcpu by setting the corresponded IRR
bit in the vAPIC backing page and trigger AVIC_DOORBELL MSR.
For sending IPI interrupts between running vcpus in Linux guest,
Xen is default to using event channel. However, in case of
non-paravirtualize guest, AVIC can also provide performance
improvements for sending IPI.
BENCHMARK 1: HACKBENCH
======================
For measuring IPI performance used for scheduling workload, I have collected
some performance number on 2 and 3 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 (sec) | 3 vcpus (sec)
--------------------------------------------------------
No AVIC w/o evtchn | 299.57 | 337.779
No AVIC w/ evtchn | 270.37 | 419.6064
AVIC w/ evtchn | 181.46 | 171.7957
AVIC w/o evtchn | 171.81 | 169.0858
Note: In "w/o evtchn" case, the Linux guest is built w/o
Xen guest support.
CURRENT UNTESTED USE-CASES
===========================
- Nested VM
Any feedback and comments are very much appreciated.
Thank you,
Suravee
Suravee Suthikulpanit (9):
x86/HVM: Introduce struct hvm_pi_ops
x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as
non-static
x86/HVM: Call vlapic_destroy after vcpu_destroy
x86/SVM: Modify VMCB fields to add AVIC support
x86/HVM/SVM: Add AVIC initialization code
x86/SVM: Add AVIC vmexit handlers
x86/SVM: Add vcpu scheduling support for AVIC
x86/SVM: Add interrupt management code via AVIC
x86/SVM: Hook up miscellaneous AVIC functions
xen/arch/x86/hvm/hvm.c | 4 +-
xen/arch/x86/hvm/svm/Makefile | 1 +
xen/arch/x86/hvm/svm/avic.c | 609 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/svm/intr.c | 4 +
xen/arch/x86/hvm/svm/svm.c | 57 +++-
xen/arch/x86/hvm/svm/vmcb.c | 9 +-
xen/arch/x86/hvm/vlapic.c | 5 +-
xen/arch/x86/hvm/vmx/vmx.c | 32 +-
xen/include/asm-x86/hvm/domain.h | 63 ++++
xen/include/asm-x86/hvm/hvm.h | 4 +-
xen/include/asm-x86/hvm/svm/avic.h | 49 +++
xen/include/asm-x86/hvm/svm/svm.h | 2 +
xen/include/asm-x86/hvm/svm/vmcb.h | 35 ++-
xen/include/asm-x86/hvm/vlapic.h | 4 +
xen/include/asm-x86/hvm/vmx/vmcs.h | 59 ----
15 files changed, 843 insertions(+), 94 deletions(-)
create mode 100644 xen/arch/x86/hvm/svm/avic.c
create mode 100644 xen/include/asm-x86/hvm/svm/avic.h
--
1.9.1
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-12 17:01 ` Konrad Rzeszutek Wilk
2016-09-19 5:52 ` [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static Suravee Suthikulpanit
` (10 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
The current function pointers for managing hvm posted interrupt
can be used also by SVM AVIC. Therefore, this patch introduces the
struct hvm_pi_ops in the struct hvm_domain to hold them.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/vmx/vmx.c | 32 +++++++++----------
xen/include/asm-x86/hvm/domain.h | 63 ++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/hvm/hvm.h | 4 +--
xen/include/asm-x86/hvm/vmx/vmcs.h | 59 -----------------------------------
4 files changed, 81 insertions(+), 77 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2759e6f..8620697 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -204,12 +204,12 @@ void vmx_pi_hooks_assign(struct domain *d)
if ( !iommu_intpost || !has_hvm_container_domain(d) )
return;
- ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
+ ASSERT(!d->arch.hvm_domain.pi_ops.vcpu_block);
- d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
- d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
- d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
- d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
+ d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
+ d->arch.hvm_domain.pi_ops.pi_switch_from = vmx_pi_switch_from;
+ d->arch.hvm_domain.pi_ops.pi_switch_to = vmx_pi_switch_to;
+ d->arch.hvm_domain.pi_ops.pi_do_resume = vmx_pi_do_resume;
}
/* This function is called when pcidevs_lock is held */
@@ -218,12 +218,12 @@ void vmx_pi_hooks_deassign(struct domain *d)
if ( !iommu_intpost || !has_hvm_container_domain(d) )
return;
- ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
+ ASSERT(d->arch.hvm_domain.pi_ops.vcpu_block);
- d->arch.hvm_domain.vmx.vcpu_block = NULL;
- d->arch.hvm_domain.vmx.pi_switch_from = NULL;
- d->arch.hvm_domain.vmx.pi_switch_to = NULL;
- d->arch.hvm_domain.vmx.pi_do_resume = NULL;
+ d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
+ d->arch.hvm_domain.pi_ops.pi_switch_from = NULL;
+ d->arch.hvm_domain.pi_ops.pi_switch_to = NULL;
+ d->arch.hvm_domain.pi_ops.pi_do_resume = NULL;
}
static int vmx_domain_initialise(struct domain *d)
@@ -901,8 +901,8 @@ static void vmx_ctxt_switch_from(struct vcpu *v)
vmx_restore_host_msrs();
vmx_save_dr(v);
- if ( v->domain->arch.hvm_domain.vmx.pi_switch_from )
- v->domain->arch.hvm_domain.vmx.pi_switch_from(v);
+ if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_from )
+ v->domain->arch.hvm_domain.pi_ops.pi_switch_from(v);
}
static void vmx_ctxt_switch_to(struct vcpu *v)
@@ -916,8 +916,8 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
vmx_restore_guest_msrs(v);
vmx_restore_dr(v);
- if ( v->domain->arch.hvm_domain.vmx.pi_switch_to )
- v->domain->arch.hvm_domain.vmx.pi_switch_to(v);
+ if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_to )
+ v->domain->arch.hvm_domain.pi_ops.pi_switch_to(v);
}
@@ -3914,8 +3914,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
struct hvm_vcpu_asid *p_asid;
bool_t need_flush;
- if ( curr->domain->arch.hvm_domain.vmx.pi_do_resume )
- curr->domain->arch.hvm_domain.vmx.pi_do_resume(curr);
+ if ( curr->domain->arch.hvm_domain.pi_ops.pi_do_resume )
+ curr->domain->arch.hvm_domain.pi_ops.pi_do_resume(curr);
if ( !cpu_has_vmx_vpid )
goto out;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index f34d784..779927b 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -72,6 +72,67 @@ struct hvm_ioreq_server {
bool_t bufioreq_atomic;
};
+struct hvm_pi_ops {
+ /*
+ * To handle posted interrupts correctly, we need to set the following
+ * state:
+ *
+ * * The PI notification vector (NV)
+ * * The PI notification destination processor (NDST)
+ * * The PI "suppress notification" bit (SN)
+ * * The vcpu pi "blocked" list
+ *
+ * If a VM is currently running, we want the PI delivered to the guest vcpu
+ * on the proper pcpu (NDST = v->processor, SN clear).
+ *
+ * If the vm is blocked, we want the PI delivered to Xen so that it can
+ * wake it up (SN clear, NV = pi_wakeup_vector, vcpu on block list).
+ *
+ * If the VM is currently either preempted or offline (i.e., not running
+ * because of some reason other than blocking waiting for an interrupt),
+ * there's nothing Xen can do -- we want the interrupt pending bit set in
+ * the guest, but we don't want to bother Xen with an interrupt (SN clear).
+ *
+ * There's a brief window of time between vmx_intr_assist() and checking
+ * softirqs where if an interrupt comes in it may be lost; so we need Xen
+ * to get an interrupt and raise a softirq so that it will go through the
+ * vmx_intr_assist() path again (SN clear, NV = posted_interrupt).
+ *
+ * The way we implement this now is by looking at what needs to happen on
+ * the following runstate transitions:
+ *
+ * A: runnable -> running
+ * - SN = 0
+ * - NDST = v->processor
+ * B: running -> runnable
+ * - SN = 1
+ * C: running -> blocked
+ * - NV = pi_wakeup_vector
+ * - Add vcpu to blocked list
+ * D: blocked -> runnable
+ * - NV = posted_intr_vector
+ * - Take vcpu off blocked list
+ *
+ * For transitions A and B, we add hooks into vmx_ctxt_switch_{from,to}
+ * paths.
+ *
+ * For transition C, we add a new arch hook, arch_vcpu_block(), which is
+ * called from vcpu_block() and vcpu_do_poll().
+ *
+ * For transition D, rather than add an extra arch hook on vcpu_wake, we
+ * add a hook on the vmentry path which checks to see if either of the two
+ * actions need to be taken.
+ *
+ * These hooks only need to be called when the domain in question actually
+ * has a physical device assigned to it, so we set and clear the callbacks
+ * as appropriate when device assignment changes.
+ */
+ void (*vcpu_block) (struct vcpu *);
+ void (*pi_switch_from) (struct vcpu *v);
+ void (*pi_switch_to) (struct vcpu *v);
+ void (*pi_do_resume) (struct vcpu *v);
+};
+
struct hvm_domain {
/* Guest page range used for non-default ioreq servers */
struct {
@@ -148,6 +209,8 @@ struct hvm_domain {
struct list_head list;
} write_map;
+ struct hvm_pi_ops pi_ops;
+
union {
struct vmx_domain vmx;
struct svm_domain svm;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 81b60d5..c832d9a 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -621,8 +621,8 @@ unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v, bool_t restore);
struct vcpu *v_ = (v); \
struct domain *d_ = v_->domain; \
if ( has_hvm_container_domain(d_) && \
- (cpu_has_vmx && d_->arch.hvm_domain.vmx.vcpu_block) ) \
- d_->arch.hvm_domain.vmx.vcpu_block(v_); \
+ (d_->arch.hvm_domain.pi_ops.vcpu_block) ) \
+ d_->arch.hvm_domain.pi_ops.vcpu_block(v_); \
})
#endif /* __ASM_X86_HVM_HVM_H__ */
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 997f4f5..4ec8b08 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -77,65 +77,6 @@ struct vmx_domain {
unsigned long apic_access_mfn;
/* VMX_DOMAIN_* */
unsigned int status;
-
- /*
- * To handle posted interrupts correctly, we need to set the following
- * state:
- *
- * * The PI notification vector (NV)
- * * The PI notification destination processor (NDST)
- * * The PI "suppress notification" bit (SN)
- * * The vcpu pi "blocked" list
- *
- * If a VM is currently running, we want the PI delivered to the guest vcpu
- * on the proper pcpu (NDST = v->processor, SN clear).
- *
- * If the vm is blocked, we want the PI delivered to Xen so that it can
- * wake it up (SN clear, NV = pi_wakeup_vector, vcpu on block list).
- *
- * If the VM is currently either preempted or offline (i.e., not running
- * because of some reason other than blocking waiting for an interrupt),
- * there's nothing Xen can do -- we want the interrupt pending bit set in
- * the guest, but we don't want to bother Xen with an interrupt (SN clear).
- *
- * There's a brief window of time between vmx_intr_assist() and checking
- * softirqs where if an interrupt comes in it may be lost; so we need Xen
- * to get an interrupt and raise a softirq so that it will go through the
- * vmx_intr_assist() path again (SN clear, NV = posted_interrupt).
- *
- * The way we implement this now is by looking at what needs to happen on
- * the following runstate transitions:
- *
- * A: runnable -> running
- * - SN = 0
- * - NDST = v->processor
- * B: running -> runnable
- * - SN = 1
- * C: running -> blocked
- * - NV = pi_wakeup_vector
- * - Add vcpu to blocked list
- * D: blocked -> runnable
- * - NV = posted_intr_vector
- * - Take vcpu off blocked list
- *
- * For transitions A and B, we add hooks into vmx_ctxt_switch_{from,to}
- * paths.
- *
- * For transition C, we add a new arch hook, arch_vcpu_block(), which is
- * called from vcpu_block() and vcpu_do_poll().
- *
- * For transition D, rather than add an extra arch hook on vcpu_wake, we
- * add a hook on the vmentry path which checks to see if either of the two
- * actions need to be taken.
- *
- * These hooks only need to be called when the domain in question actually
- * has a physical device assigned to it, so we set and clear the callbacks
- * as appropriate when device assignment changes.
- */
- void (*vcpu_block) (struct vcpu *);
- void (*pi_switch_from) (struct vcpu *v);
- void (*pi_switch_to) (struct vcpu *v);
- void (*pi_do_resume) (struct vcpu *v);
};
struct pi_desc {
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-12 19:00 ` Konrad Rzeszutek Wilk
2016-09-19 5:52 ` [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy Suravee Suthikulpanit
` (9 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Expose vlapic_read_aligned and vlapic_reg_write() to be used by AVIC.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/vlapic.c | 5 ++---
xen/include/asm-x86/hvm/vlapic.h | 4 ++++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1d5d287..0f52067 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -562,7 +562,7 @@ static void vlapic_set_tdcr(struct vlapic *vlapic, unsigned int val)
"timer_divisor: %d", vlapic->hw.timer_divisor);
}
-static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
+uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
{
switch ( offset )
{
@@ -680,8 +680,7 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
vcpu_vlapic(v)->hw.tdt_msr = 0;
}
-static void vlapic_reg_write(struct vcpu *v,
- unsigned int offset, uint32_t val)
+void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val)
{
struct vlapic *vlapic = vcpu_vlapic(v);
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 4656293..48ab3a6 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -132,6 +132,10 @@ void vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);
int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
+void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val);
+
+uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset);
+
struct vlapic *vlapic_lowest_prio(
struct domain *d, const struct vlapic *source,
int short_hand, uint32_t dest, bool_t dest_mode);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-12 19:02 ` Konrad Rzeszutek Wilk
2016-12-22 11:09 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support Suravee Suthikulpanit
` (8 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Since vlapic_init() is called before vcpu_initialise().
We should also follow the same order here.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/hvm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7bad845..fb5bf6c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1606,10 +1606,10 @@ void hvm_vcpu_destroy(struct vcpu *v)
tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
hvm_vcpu_cacheattr_destroy(v);
+ hvm_funcs.vcpu_destroy(v);
+
if ( is_hvm_vcpu(v) )
vlapic_destroy(v);
-
- hvm_funcs.vcpu_destroy(v);
}
void hvm_vcpu_down(struct vcpu *v)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (2 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-12 19:07 ` Konrad Rzeszutek Wilk
2016-12-22 11:11 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
` (7 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Introduce AVIC-related VMCB fields.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/include/asm-x86/hvm/svm/vmcb.h | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index bad2382..768e9fb 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -328,14 +328,15 @@ typedef union __packed
struct
{
u64 tpr: 8;
- u64 irq: 1;
+ u64 irq: 1; /* disabled for avic */
u64 rsvd0: 7;
- u64 prio: 4;
- u64 ign_tpr: 1;
+ u64 prio: 4; /* disabled for avic */
+ u64 ign_tpr: 1; /* disabled for avic */
u64 rsvd1: 3;
u64 intr_masking: 1;
- u64 rsvd2: 7;
- u64 vector: 8;
+ u64 rsvd2: 6;
+ u64 avic_enable: 1;
+ u64 vector: 8; /* disabled for avic */
u64 rsvd3: 24;
} fields;
} vintr_t;
@@ -394,7 +395,8 @@ typedef union __packed
uint32_t cr2: 1;
/* debugctlmsr, last{branch,int}{to,from}ip */
uint32_t lbr: 1;
- uint32_t resv: 21;
+ uint32_t avic: 1;
+ uint32_t resv: 20;
} fields;
} vmcbcleanbits_t;
@@ -428,7 +430,8 @@ struct __packed vmcb_struct {
u64 exitinfo2; /* offset 0x80 */
eventinj_t exitintinfo; /* offset 0x88 */
u64 _np_enable; /* offset 0x90 - cleanbit 4 */
- u64 res08[2];
+ u64 avic_vapic_bar; /* offset 0x98 */
+ u64 res08; /* offset 0xA0 */
eventinj_t eventinj; /* offset 0xA8 */
u64 _h_cr3; /* offset 0xB0 - cleanbit 4 */
lbrctrl_t lbr_control; /* offset 0xB8 */
@@ -437,7 +440,11 @@ struct __packed vmcb_struct {
u64 nextrip; /* offset 0xC8 */
u8 guest_ins_len; /* offset 0xD0 */
u8 guest_ins[15]; /* offset 0xD1 */
- u64 res10a[100]; /* offset 0xE0 pad to save area */
+ u64 avic_bk_pg_pa; /* offset 0xE0 */
+ u64 res09a; /* offset 0xE8 */
+ u64 avic_log_apic_id; /* offset 0xF0 */
+ u64 avic_phy_apic_id; /* offset 0xF8 */
+ u64 res09b[96]; /* offset 0x100 pad to save area */
svm_segment_register_t es; /* offset 1024 - cleanbit 8 */
svm_segment_register_t cs; /* cleanbit 8 */
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (3 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
` (2 more replies)
2016-09-19 5:52 ` [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers Suravee Suthikulpanit
` (6 subsequent siblings)
11 siblings, 3 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Introduce AVIC base initialization code. This includes:
* Setting up per-VM data structures.
* Setting up per-vCPU data structure.
* Initializing AVIC-related VMCB bit fields.
This patch also introduces a new Xen parameter (svm-avic),
which can be used to enable/disable AVIC support.
Currently, this svm-avic is disabled by default.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/svm/Makefile | 1 +
xen/arch/x86/hvm/svm/avic.c | 217 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/svm/svm.c | 17 ++-
xen/arch/x86/hvm/svm/vmcb.c | 3 +
xen/include/asm-x86/hvm/svm/avic.h | 40 +++++++
xen/include/asm-x86/hvm/svm/svm.h | 2 +
xen/include/asm-x86/hvm/svm/vmcb.h | 10 ++
7 files changed, 289 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/x86/hvm/svm/avic.c
create mode 100644 xen/include/asm-x86/hvm/svm/avic.h
diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile
index 760d295..e0e4a59 100644
--- a/xen/arch/x86/hvm/svm/Makefile
+++ b/xen/arch/x86/hvm/svm/Makefile
@@ -1,4 +1,5 @@
obj-y += asid.o
+obj-y += avic.o
obj-y += emulate.o
obj-bin-y += entry.o
obj-y += intr.o
diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
new file mode 100644
index 0000000..70bac69
--- /dev/null
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -0,0 +1,217 @@
+#include <xen/domain_page.h>
+#include <xen/sched.h>
+#include <xen/stdbool.h>
+#include <asm/acpi.h>
+#include <asm/apicdef.h>
+#include <asm/event.h>
+#include <asm/p2m.h>
+#include <asm/page.h>
+#include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/svm/avic.h>
+#include <asm/hvm/vlapic.h>
+#include <asm/hvm/emulate.h>
+#include <asm/hvm/support.h>
+
+/* NOTE: Current max index allowed for physical APIC ID table is 255 */
+#define AVIC_PHY_APIC_ID_MAX 0xFF
+
+#define AVIC_DOORBELL 0xc001011b
+#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
+#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
+
+bool_t svm_avic = 0;
+boolean_param("svm-avic", svm_avic);
+
+static struct svm_avic_phy_ait_entry *
+avic_get_phy_ait_entry(struct vcpu *v, int index)
+{
+ struct svm_avic_phy_ait_entry *avic_phy_ait;
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+
+ if ( !d->avic_phy_ait_mfn )
+ return NULL;
+
+ /**
+ * Note: APIC ID = 0xff is used for broadcast.
+ * APIC ID > 0xff is reserved.
+ */
+ if ( index >= 0xff )
+ return NULL;
+
+ avic_phy_ait = mfn_to_virt(d->avic_phy_ait_mfn);
+
+ return &avic_phy_ait[index];
+}
+
+/***************************************************************
+ * AVIC APIs
+ */
+int svm_avic_dom_init(struct domain *d)
+{
+ int ret = 0;
+ struct page_info *pg;
+ unsigned long mfn;
+
+ if ( !svm_avic )
+ return 0;
+
+ /**
+ * Note:
+ * AVIC hardware walks the nested page table to check permissions,
+ * but does not use the SPA address specified in the leaf page
+ * table entry since it uses address in the AVIC_BACKING_PAGE pointer
+ * field of the VMCB. Therefore, we set up a dummy page here _mfn(0).
+ */
+ if ( !d->arch.hvm_domain.svm.avic_access_page_done )
+ {
+ set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
+ _mfn(0), PAGE_ORDER_4K,
+ p2m_get_hostp2m(d)->default_access);
+ d->arch.hvm_domain.svm.avic_access_page_done = true;
+ }
+
+ /* Init AVIC log APIC ID table */
+ pg = alloc_domheap_page(d, MEMF_no_owner);
+ if ( !pg )
+ {
+ dprintk(XENLOG_ERR, "alloc AVIC logical APIC ID table error: %d\n",
+ d->domain_id);
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ mfn = page_to_mfn(pg);
+ clear_domain_page(_mfn(mfn));
+ d->arch.hvm_domain.svm.avic_log_ait_mfn = mfn;
+
+ /* Init AVIC phy APIC ID table */
+ pg = alloc_domheap_page(d, MEMF_no_owner);
+ if ( !pg )
+ {
+ dprintk(XENLOG_ERR, "alloc AVIC physical APIC ID table error: %d\n",
+ d->domain_id);
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ mfn = page_to_mfn(pg);
+ clear_domain_page(_mfn(mfn));
+ d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
+
+ return ret;
+err_out:
+ svm_avic_dom_destroy(d);
+ return ret;
+}
+
+void svm_avic_dom_destroy(struct domain *d)
+{
+ if ( !svm_avic )
+ return;
+
+ if ( d->arch.hvm_domain.svm.avic_phy_ait_mfn )
+ {
+ free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_phy_ait_mfn));
+ d->arch.hvm_domain.svm.avic_phy_ait_mfn = 0;
+ }
+
+ if ( d->arch.hvm_domain.svm.avic_log_ait_mfn )
+ {
+ free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_log_ait_mfn));
+ d->arch.hvm_domain.svm.avic_log_ait_mfn = 0;
+ }
+}
+
+/**
+ * Note: At this point, vlapic->regs_page is already initialized.
+ */
+int svm_avic_init_vcpu(struct vcpu *v)
+{
+ struct vlapic *vlapic = vcpu_vlapic(v);
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+ if ( svm_avic )
+ s->avic_bk_pg = vlapic->regs_page;
+ return 0;
+}
+
+void svm_avic_destroy_vcpu(struct vcpu *v)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+ if ( svm_avic && s->avic_bk_pg )
+ s->avic_bk_pg = NULL;
+}
+
+bool_t svm_avic_vcpu_enabled(struct vcpu *v)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ struct vmcb_struct *vmcb = s->vmcb;
+
+ return ( svm_avic && vmcb->_vintr.fields.avic_enable);
+}
+
+static inline u32 *
+avic_get_bk_page_entry(struct vcpu *v, u32 offset)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ struct page_info *pg = s->avic_bk_pg;
+ char *tmp;
+
+ if ( !pg )
+ return NULL;
+
+ tmp = (char *) page_to_virt(pg);
+ return (u32*)(tmp+offset);
+}
+
+void svm_avic_update_vapic_bar(struct vcpu *v, uint64_t data)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+ s->vmcb->avic_vapic_bar = data & AVIC_APIC_BAR_MASK;
+ s->vmcb->cleanbits.fields.avic = 0;
+}
+
+int svm_avic_init_vmcb(struct vcpu *v)
+{
+ paddr_t ma;
+ u32 apic_id_reg;
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ struct vmcb_struct *vmcb = s->vmcb;
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+ struct svm_avic_phy_ait_entry entry;
+
+ if ( !svm_avic )
+ return 0;
+
+ vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
+ ma = d->avic_log_ait_mfn;
+ vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
+ ma = d->avic_phy_ait_mfn;
+ vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
+ vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
+
+ dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
+ __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
+ (unsigned long long) vmcb->avic_log_apic_id,
+ (unsigned long long) vmcb->avic_phy_apic_id);
+
+
+ apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
+ s->avic_phy_id_cache = avic_get_phy_ait_entry(v, apic_id_reg >> 24);
+ if ( !s->avic_phy_id_cache )
+ return -EINVAL;
+
+ entry = *(s->avic_phy_id_cache);
+ smp_rmb();
+ entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa >> 12) & 0xffffffffff;
+ entry.is_running= 0;
+ entry.valid = 1;
+ *(s->avic_phy_id_cache) = entry;
+ smp_wmb();
+
+ svm_avic_update_vapic_bar(v, APIC_DEFAULT_PHYS_BASE);
+
+ vmcb->_vintr.fields.avic_enable = 1;
+
+ return 0;
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 679e615..bcb7df4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -48,6 +48,7 @@
#include <asm/hvm/svm/asid.h>
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/svm/vmcb.h>
+#include <asm/hvm/svm/avic.h>
#include <asm/hvm/svm/emulate.h>
#include <asm/hvm/svm/intr.h>
#include <asm/hvm/svm/svmdebug.h>
@@ -1164,11 +1165,12 @@ void svm_host_osvw_init()
static int svm_domain_initialise(struct domain *d)
{
- return 0;
+ return svm_avic_dom_init(d);
}
static void svm_domain_destroy(struct domain *d)
{
+ svm_avic_dom_destroy(d);
}
static int svm_vcpu_initialise(struct vcpu *v)
@@ -1181,6 +1183,13 @@ static int svm_vcpu_initialise(struct vcpu *v)
v->arch.hvm_svm.launch_core = -1;
+ if ( (rc = svm_avic_init_vcpu(v)) != 0 )
+ {
+ dprintk(XENLOG_WARNING,
+ "Failed to initiazlize AVIC vcpu.\n");
+ return rc;
+ }
+
if ( (rc = svm_create_vmcb(v)) != 0 )
{
dprintk(XENLOG_WARNING,
@@ -1200,6 +1209,7 @@ static int svm_vcpu_initialise(struct vcpu *v)
static void svm_vcpu_destroy(struct vcpu *v)
{
+ svm_avic_destroy_vcpu(v);
vpmu_destroy(v);
svm_destroy_vmcb(v);
passive_domain_destroy(v);
@@ -1464,6 +1474,7 @@ const struct hvm_function_table * __init start_svm(void)
P(cpu_has_svm_decode, "DecodeAssists");
P(cpu_has_pause_filter, "Pause-Intercept Filter");
P(cpu_has_tsc_ratio, "TSC Rate MSR");
+ P(cpu_has_svm_avic, "AVIC");
#undef P
if ( !printed )
@@ -1809,6 +1820,10 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
switch ( msr )
{
+ case MSR_IA32_APICBASE:
+ if ( svm_avic_vcpu_enabled(v) )
+ svm_avic_update_vapic_bar(v, msr_content);
+ break;
case MSR_IA32_SYSENTER_CS:
case MSR_IA32_SYSENTER_ESP:
case MSR_IA32_SYSENTER_EIP:
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 9ea014f..9ee7fc7 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -28,6 +28,7 @@
#include <asm/msr-index.h>
#include <asm/p2m.h>
#include <asm/hvm/support.h>
+#include <asm/hvm/svm/avic.h>
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/svm/svmdebug.h>
@@ -225,6 +226,8 @@ static int construct_vmcb(struct vcpu *v)
vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
}
+ svm_avic_init_vmcb(v);
+
vmcb->cleanbits.bytes = 0;
return 0;
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
new file mode 100644
index 0000000..9508486
--- /dev/null
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -0,0 +1,40 @@
+#ifndef _SVM_AVIC_H_
+#define _SVM_AVIC_H_
+
+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,
+};
+
+struct __attribute__ ((__packed__))
+svm_avic_log_ait_entry {
+ u32 guest_phy_apic_id : 8;
+ u32 res : 23;
+ u32 valid : 1;
+};
+
+struct __attribute__ ((__packed__))
+svm_avic_phy_ait_entry {
+ u64 host_phy_apic_id : 8;
+ u64 res1 : 4;
+ u64 bk_pg_ptr : 40;
+ u64 res2 : 10;
+ u64 is_running : 1;
+ u64 valid : 1;
+};
+
+extern bool_t svm_avic;
+
+int svm_avic_dom_init(struct domain *d);
+void svm_avic_dom_destroy(struct domain *d);
+
+int svm_avic_init_vcpu(struct vcpu *v);
+void svm_avic_destroy_vcpu(struct vcpu *v);
+bool_t svm_avic_vcpu_enabled(struct vcpu *v);
+
+void svm_avic_update_vapic_bar(struct vcpu *v,uint64_t data);
+int svm_avic_init_vmcb(struct vcpu *v);
+
+#endif /* _SVM_AVIC_H_ */
diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h
index c954b7e..fea61bb 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -81,6 +81,7 @@ extern u32 svm_feature_flags;
#define SVM_FEATURE_FLUSHBYASID 6 /* TLB flush by ASID support */
#define SVM_FEATURE_DECODEASSISTS 7 /* Decode assists support */
#define SVM_FEATURE_PAUSEFILTER 10 /* Pause intercept filter support */
+#define SVM_FEATURE_AVIC 13 /* AVIC support */
#define cpu_has_svm_feature(f) test_bit(f, &svm_feature_flags)
#define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT)
@@ -91,6 +92,7 @@ extern u32 svm_feature_flags;
#define cpu_has_svm_decode cpu_has_svm_feature(SVM_FEATURE_DECODEASSISTS)
#define cpu_has_pause_filter cpu_has_svm_feature(SVM_FEATURE_PAUSEFILTER)
#define cpu_has_tsc_ratio cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR)
+#define cpu_has_svm_avic cpu_has_svm_feature(SVM_FEATURE_AVIC)
#define SVM_PAUSEFILTER_INIT 3000
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 768e9fb..a42c034 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -496,6 +496,12 @@ struct __packed vmcb_struct {
};
struct svm_domain {
+ u32 avic_max_vcpu_id;
+ bool_t avic_access_page_done;
+ paddr_t avic_log_ait_mfn;
+ paddr_t avic_phy_ait_mfn;
+ u32 ldr_reg;
+ u32 ldr_mode;
};
struct arch_svm_struct {
@@ -529,6 +535,10 @@ struct arch_svm_struct {
u64 length;
u64 status;
} osvw;
+
+ /* AVIC APIC backing page */
+ struct page_info *avic_bk_pg;
+ struct svm_avic_phy_ait_entry *avic_phy_id_cache;
};
struct vmcb_struct *alloc_vmcb(void);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (4 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-14 15:20 ` Konrad Rzeszutek Wilk
2016-12-22 11:25 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC Suravee Suthikulpanit
` (5 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
AVIC introduces two #vmexit handlers:
* VMEXIT_INCOMP_IPI
* VMEXIT_DO_NOACCEL
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/svm/avic.c | 279 +++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/svm/svm.c | 8 ++
xen/include/asm-x86/hvm/svm/avic.h | 3 +
xen/include/asm-x86/hvm/svm/vmcb.h | 2 +
4 files changed, 292 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 70bac69..90df181 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -18,6 +18,7 @@
#define AVIC_DOORBELL 0xc001011b
#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
+#define AVIC_UNACCEL_ACCESS_OFFSET_MASK 0xFF0
bool_t svm_avic = 0;
boolean_param("svm-avic", svm_avic);
@@ -215,3 +216,281 @@ int svm_avic_init_vmcb(struct vcpu *v)
return 0;
}
+
+/***************************************************************
+ * AVIC INCOMP IPI VMEXIT
+ */
+void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
+{
+ struct vcpu *v = current;
+ struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ u32 icrh = vmcb->exitinfo1 >> 32;
+ u32 icrl = vmcb->exitinfo1;
+ u32 id = vmcb->exitinfo2 >> 32;
+ u32 index = vmcb->exitinfo2 && 0xFF;
+
+ dprintk(XENLOG_DEBUG, "SVM: %s: cpu=%#x, vcpu=%#x, "
+ "icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
+ __func__, v->processor, v->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.
+ */
+ vlapic_reg_write(v, APIC_ICR2, icrh);
+ vlapic_reg_write(v, APIC_ICR, icrl);
+ break;
+ case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
+ {
+ /*
+ * 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.
+ */
+ struct vcpu *c;
+ struct domain *d = v->domain;
+ uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
+ uint32_t short_hand = icrl & APIC_SHORT_MASK;
+ bool_t dest_mode = !!(icrl & APIC_DEST_MASK);
+
+ for_each_vcpu ( d, c )
+ {
+ if ( vlapic_match_dest(vcpu_vlapic(c), vcpu_vlapic(v),
+ short_hand, dest, dest_mode) )
+ {
+ vcpu_kick(c);
+ break;
+ }
+ }
+ break;
+ }
+ case AVIC_INCMP_IPI_ERR_INV_TARGET:
+ dprintk(XENLOG_ERR,
+ "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
+ __func__, icrh, icrl, index);
+ break;
+ case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
+ dprintk(XENLOG_ERR,
+ "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
+ __func__, icrh, icrl, index);
+ break;
+ default:
+ dprintk(XENLOG_ERR, "SVM: %s: Unknown IPI interception\n", __func__);
+ }
+}
+
+/***************************************************************
+ * AVIC NOACCEL VMEXIT
+ */
+#define GET_APIC_LOGICAL_ID(x) (((x) >> 24) & 0xFFu)
+
+static struct svm_avic_log_ait_entry *
+avic_get_logical_id_entry(struct vcpu *v, u32 ldr, bool flat)
+{
+ int index;
+ struct svm_avic_log_ait_entry *avic_log_ait;
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+ int dlid = GET_APIC_LOGICAL_ID(ldr);
+
+ if ( !dlid )
+ return NULL;
+
+ if ( flat )
+ {
+ index = ffs(dlid) - 1;
+ if ( index > 7 )
+ return NULL;
+ }
+ else
+ {
+ int cluster = (dlid & 0xf0) >> 4;
+ int apic = ffs(dlid & 0x0f) - 1;
+
+ if ((apic < 0) || (apic > 7) || (cluster >= 0xf))
+ return NULL;
+ index = (cluster << 2) + apic;
+ }
+
+ avic_log_ait = mfn_to_virt(d->avic_log_ait_mfn);
+
+ return &avic_log_ait[index];
+}
+
+static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
+{
+ bool flat;
+ struct svm_avic_log_ait_entry *entry, new_entry;
+
+ flat = *avic_get_bk_page_entry(v, APIC_DFR) == APIC_DFR_FLAT;
+ entry = avic_get_logical_id_entry(v, ldr, flat);
+ if (!entry)
+ return -EINVAL;
+
+ new_entry = *entry;
+ smp_rmb();
+ new_entry.guest_phy_apic_id = g_phy_id;
+ new_entry.valid = valid;
+ *entry = new_entry;
+ smp_wmb();
+
+ return 0;
+}
+
+static int avic_handle_ldr_update(struct vcpu *v)
+{
+ int ret = 0;
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+ u32 ldr = *avic_get_bk_page_entry(v, APIC_LDR);
+ u32 apic_id = (*avic_get_bk_page_entry(v, APIC_ID) >> 24);
+
+ if ( !ldr )
+ return 1;
+
+ ret = avic_ldr_write(v, apic_id, ldr, true);
+ if (ret && d->ldr_reg)
+ {
+ avic_ldr_write(v, 0, d->ldr_reg, false);
+ d->ldr_reg = 0;
+ }
+ else
+ {
+ d->ldr_reg = ldr;
+ }
+
+ return ret;
+}
+
+static int avic_handle_apic_id_update(struct vcpu *v, bool init)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+ u32 apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
+ u32 id = (apic_id_reg >> 24) & 0xff;
+ struct svm_avic_phy_ait_entry *old, *new;
+
+ old = s->avic_phy_id_cache;
+ new = avic_get_phy_ait_entry(v, id);
+ if ( !new || !old )
+ return 0;
+
+ /* We need to move physical_id_entry to new offset */
+ *new = *old;
+ *((u64 *)old) = 0ULL;
+ s->avic_phy_id_cache = new;
+
+ /*
+ * Also update the guest physical APIC ID in the logical
+ * APIC ID table entry if already setup the LDR.
+ */
+ if ( d->ldr_reg )
+ avic_handle_ldr_update(v);
+
+ return 0;
+}
+
+static int avic_handle_dfr_update(struct vcpu *v)
+{
+ struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+ u32 dfr = *avic_get_bk_page_entry(v, APIC_DFR);
+ u32 mod = (dfr >> 28) & 0xf;
+
+ /*
+ * We assume that all local APICs are using the same type.
+ * If this changes, we need to flush the AVIC logical
+ * APID id table.
+ */
+ if ( d->ldr_mode == mod )
+ return 0;
+
+ clear_domain_page(_mfn(d->avic_log_ait_mfn));
+ d->ldr_mode = mod;
+ if (d->ldr_reg)
+ avic_handle_ldr_update(v);
+ return 0;
+}
+
+static int avic_unaccel_trap_write(struct vcpu *v)
+{
+ struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ u32 offset = vmcb->exitinfo1 & AVIC_UNACCEL_ACCESS_OFFSET_MASK;
+ u32 reg = *avic_get_bk_page_entry(v, offset);
+
+ switch ( offset ) {
+ case APIC_ID:
+ if ( avic_handle_apic_id_update(v, false) )
+ return 0;
+ break;
+ case APIC_LDR:
+ if ( avic_handle_ldr_update(v) )
+ return 0;
+ break;
+ case APIC_DFR:
+ avic_handle_dfr_update(v);
+ break;
+ default:
+ break;
+ }
+
+ vlapic_reg_write(v, offset, reg);
+
+ return 1;
+}
+
+void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
+{
+ struct vcpu *v = current;
+ struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ u32 offset = vmcb->exitinfo1 & 0xFF0;
+ u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
+ u32 vector = vmcb->exitinfo2 & 0xFFFFFFFF;
+
+ dprintk(XENLOG_DEBUG,
+ "SVM: %s: offset=%#x, rw=%#x, vector=%#x, vcpu_id=%#x, cpu=%#x\n",
+ __func__, offset, rw, vector, v->vcpu_id, v->processor);
+
+ 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();
+ avic_unaccel_trap_write(v);
+ break;
+ default:
+ /* Handling Fault */
+ if ( !rw )
+ *avic_get_bk_page_entry(v, offset) = vlapic_read_aligned(
+ vcpu_vlapic(v), offset);
+
+ hvm_mem_access_emulate_one(EMUL_KIND_NORMAL, TRAP_invalid_op,
+ HVM_DELIVER_NO_ERROR_CODE);
+ }
+
+ return;
+}
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bcb7df4..409096a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2710,6 +2710,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
svm_vmexit_do_pause(regs);
break;
+ case VMEXIT_AVIC_INCOMP_IPI:
+ svm_avic_vmexit_do_incomp_ipi(regs);
+ break;
+
+ case VMEXIT_AVIC_NOACCEL:
+ svm_avic_vmexit_do_noaccel(regs);
+ break;
+
default:
unexpected_exit_type:
gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index 9508486..2c501d4 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -37,4 +37,7 @@ bool_t svm_avic_vcpu_enabled(struct vcpu *v);
void svm_avic_update_vapic_bar(struct vcpu *v,uint64_t data);
int svm_avic_init_vmcb(struct vcpu *v);
+void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
+void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
+
#endif /* _SVM_AVIC_H_ */
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index a42c034..23eb86b 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -302,6 +302,8 @@ enum VMEXIT_EXITCODE
VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
VMEXIT_XSETBV = 141, /* 0x8d */
VMEXIT_NPF = 1024, /* 0x400, nested paging fault */
+ VMEXIT_AVIC_INCOMP_IPI = 1025, /* 0x401 */
+ VMEXIT_AVIC_NOACCEL = 1026, /* 0x402 */
VMEXIT_INVALID = -1
};
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (5 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-14 15:31 ` Konrad Rzeszutek Wilk
2016-12-22 11:32 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC Suravee Suthikulpanit
` (4 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Add hooks to manage AVIC data structure during vcpu scheduling.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/svm/avic.c | 82 +++++++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/svm/svm.c | 10 ++++++
2 files changed, 92 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 90df181..cd8a9d4 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -45,6 +45,83 @@ avic_get_phy_ait_entry(struct vcpu *v, int index)
}
/***************************************************************
+ * AVIC VCPU SCHEDULING
+ */
+static void avic_vcpu_load(struct vcpu *v)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ int h_phy_apic_id;
+ struct svm_avic_phy_ait_entry entry;
+
+ if ( !svm_avic || !s->avic_phy_id_cache )
+ return;
+
+ if ( test_bit(_VPF_blocked, &v->pause_flags) )
+ return;
+
+ /* Note: APIC ID = 0xff is used for broadcast.
+ * APIC ID > 0xff is reserved.
+ */
+ h_phy_apic_id = cpu_data[v->processor].apicid;
+ if ( h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX )
+ return;
+
+ entry = *(s->avic_phy_id_cache);
+ smp_rmb();
+ entry.host_phy_apic_id = h_phy_apic_id;
+ entry.is_running = 1;
+ *(s->avic_phy_id_cache) = entry;
+ smp_wmb();
+}
+
+static void avic_vcpu_put(struct vcpu *v)
+{
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+ struct svm_avic_phy_ait_entry entry;
+
+ if ( !svm_avic || !s->avic_phy_id_cache )
+ return;
+
+ entry = *(s->avic_phy_id_cache);
+ smp_rmb();
+ entry.is_running = 0;
+ *(s->avic_phy_id_cache) = entry;
+ smp_wmb();
+}
+
+static void avic_vcpu_resume(struct vcpu *v)
+{
+ struct svm_avic_phy_ait_entry entry;
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+ if ( !svm_avic_vcpu_enabled(v) || !s->avic_phy_id_cache )
+ return;
+
+ ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
+
+ entry = *(s->avic_phy_id_cache);
+ smp_rmb();
+ entry.is_running = 1;
+ *(s->avic_phy_id_cache)= entry;
+ smp_wmb();
+}
+
+static void avic_vcpu_block(struct vcpu *v)
+{
+ struct svm_avic_phy_ait_entry entry;
+ struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+ if ( !svm_avic_vcpu_enabled(v) || !s->avic_phy_id_cache )
+ return;
+
+ entry = *(s->avic_phy_id_cache);
+ smp_rmb();
+ entry.is_running = 0;
+ *(s->avic_phy_id_cache) = entry;
+ smp_wmb();
+}
+
+/***************************************************************
* AVIC APIs
*/
int svm_avic_dom_init(struct domain *d)
@@ -97,6 +174,11 @@ int svm_avic_dom_init(struct domain *d)
clear_domain_page(_mfn(mfn));
d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
+ d->arch.hvm_domain.pi_ops.pi_switch_to = avic_vcpu_put;
+ d->arch.hvm_domain.pi_ops.pi_switch_from = avic_vcpu_load;
+ d->arch.hvm_domain.pi_ops.vcpu_block = avic_vcpu_block;
+ d->arch.hvm_domain.pi_ops.pi_do_resume = avic_vcpu_resume;
+
return ret;
err_out:
svm_avic_dom_destroy(d);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 409096a..aafbfa1 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1011,6 +1011,10 @@ static void svm_ctxt_switch_from(struct vcpu *v)
svm_tsc_ratio_save(v);
svm_sync_vmcb(v);
+
+ if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_from )
+ v->domain->arch.hvm_domain.pi_ops.pi_switch_from(v);
+
svm_vmload(per_cpu(root_vmcb, cpu));
/* Resume use of ISTs now that the host TR is reinstated. */
@@ -1050,6 +1054,9 @@ static void svm_ctxt_switch_to(struct vcpu *v)
svm_lwp_load(v);
svm_tsc_ratio_load(v);
+ if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_to )
+ v->domain->arch.hvm_domain.pi_ops.pi_switch_to(v);
+
if ( cpu_has_rdtscp )
wrmsrl(MSR_TSC_AUX, hvm_msr_tsc_aux(v));
}
@@ -1095,6 +1102,9 @@ static void noreturn svm_do_resume(struct vcpu *v)
vmcb_set_vintr(vmcb, intr);
}
+ if ( v->domain->arch.hvm_domain.pi_ops.pi_do_resume )
+ v->domain->arch.hvm_domain.pi_ops.pi_do_resume(v);
+
hvm_do_resume(v);
reset_stack_and_jump(svm_asm_do_resume);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (6 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-14 15:44 ` Konrad Rzeszutek Wilk
2016-12-22 11:36 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions Suravee Suthikulpanit
` (3 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Enabling AVIC implicitly disables the V_IRQ, V_INTR_PRIO, V_IGN_TPR,
and V_INTR_VECTOR fields in the VMCB Control Word. Therefore, this patch
introduces new interrupt injection code via AVIC backing page.
Also, the AVIC hardware automatically synchronizes TPR and CR8/vTPR, when
values are updated. Therefore, xen does not need to handle this when enable
AVIC.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/svm/avic.c | 31 +++++++++++++++++++++++++++++++
xen/arch/x86/hvm/svm/intr.c | 4 ++++
xen/arch/x86/hvm/svm/svm.c | 12 ++++++++++--
xen/arch/x86/hvm/svm/vmcb.c | 6 +++++-
xen/include/asm-x86/hvm/svm/avic.h | 1 +
5 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index cd8a9d4..4144223 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -576,3 +576,34 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
return;
}
+
+/***************************************************************
+ * AVIC INTR INJECTION
+ */
+void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec)
+{
+ struct vlapic *vlapic = vcpu_vlapic(v);
+
+ /* Fallback to use non-AVIC if vcpu is not enabled with AVIC */
+ if ( !svm_avic_vcpu_enabled(v) )
+ {
+ if ( !vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
+ vcpu_kick(v);
+ return;
+ }
+
+ if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
+ return;
+
+ if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
+ return;
+
+ /*
+ * If vcpu is running on another cpu, hit the doorbell to signal
+ * it to process interrupt. Otherwise, kick it.
+ */
+ if ( v->is_running && (v != current) )
+ wrmsrl(AVIC_DOORBELL, cpu_data[v->processor].apicid);
+ else
+ vcpu_kick(v);
+}
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index bd94731..876d2ad 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -30,6 +30,7 @@
#include <asm/hvm/io.h>
#include <asm/hvm/support.h>
#include <asm/hvm/vlapic.h>
+#include <asm/hvm/svm/avic.h>
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/svm/intr.h>
#include <asm/hvm/nestedhvm.h> /* for nestedhvm_vcpu_in_guestmode */
@@ -101,6 +102,9 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
HVMTRACE_3D(INTR_WINDOW, intack.vector, intack.source,
vmcb->eventinj.fields.v?vmcb->eventinj.fields.vector:-1);
+ if ( svm_avic_vcpu_enabled(v) )
+ return;
+
/*
* Create a dummy virtual interrupt to intercept as soon as the
* guest can accept the real interrupt.
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index aafbfa1..caf9984 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1091,7 +1091,8 @@ static void noreturn svm_do_resume(struct vcpu *v)
hvm_asid_flush_vcpu(v);
}
- if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) )
+ if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) &&
+ !svm_avic_vcpu_enabled(v) )
{
vintr_t intr;
@@ -2337,7 +2338,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
* NB. We need to preserve the low bits of the TPR to make checked builds
* of Windows work, even though they don't actually do anything.
*/
- if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) )
+ if ( !vcpu_guestmode && !vlapic_hw_disabled(vlapic) &&
+ !svm_avic_vcpu_enabled(v) )
{
intr = vmcb_get_vintr(vmcb);
vlapic_set_reg(vlapic, APIC_TASKPRI,
@@ -2530,6 +2532,12 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
intr = vmcb_get_vintr(vmcb);
+ if ( svm_avic_vcpu_enabled(v) )
+ {
+ gdprintk(XENLOG_ERR, "AVIC VINTR:\n");
+ domain_crash(v->domain);
+ }
+
intr.fields.irq = 0;
general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 9ee7fc7..9ea7627 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -93,10 +93,14 @@ static int construct_vmcb(struct vcpu *v)
vmcb->_dr_intercepts = ~0u;
/* Intercept all control-register accesses except for CR2 and CR8. */
- vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
+ if ( !svm_avic_vcpu_enabled(v) )
+ vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
CR_INTERCEPT_CR2_WRITE |
CR_INTERCEPT_CR8_READ |
CR_INTERCEPT_CR8_WRITE);
+ else
+ vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
+ CR_INTERCEPT_CR2_WRITE );
/* I/O and MSR permission bitmaps. */
arch_svm->msrpm = alloc_xenheap_pages(get_order_from_bytes(MSRPM_SIZE), 0);
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index 2c501d4..e1eb66c 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -40,4 +40,5 @@ int svm_avic_init_vmcb(struct vcpu *v);
void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
+void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vector);
#endif /* _SVM_AVIC_H_ */
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (7 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC Suravee Suthikulpanit
@ 2016-09-19 5:52 ` Suravee Suthikulpanit
2016-10-14 15:46 ` Konrad Rzeszutek Wilk
2016-12-22 11:38 ` Jan Beulich
2016-09-19 13:09 ` [RFC PATCH 0/9] Introduce AMD SVM AVIC Konrad Rzeszutek Wilk
` (2 subsequent siblings)
11 siblings, 2 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 5:52 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, Suravee Suthikulpanit, jbeulich, sherry.hurwitz
Hook up virtual_intr_delivery_enabled and deliver_posted_intr functions
when AVIC is enabled.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/arch/x86/hvm/svm/svm.c | 10 ++++++++++
xen/include/asm-x86/hvm/svm/avic.h | 5 +++++
2 files changed, 15 insertions(+)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index caf9984..a9c09a7 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1495,6 +1495,16 @@ const struct hvm_function_table * __init start_svm(void)
svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
+ if ( !cpu_has_svm_avic )
+ svm_avic = 0;
+
+ if ( svm_avic )
+ {
+ svm_function_table.deliver_posted_intr = svm_avic_deliver_posted_intr,
+ svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled,
+ printk("SVM: AVIC enabled\n");
+ }
+
return &svm_function_table;
}
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index e1eb66c..8411854 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -41,4 +41,9 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vector);
+
+static inline int svm_avic_enabled(void)
+{
+ return svm_avic;
+}
#endif /* _SVM_AVIC_H_ */
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (8 preceding siblings ...)
2016-09-19 5:52 ` [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions Suravee Suthikulpanit
@ 2016-09-19 13:09 ` Konrad Rzeszutek Wilk
2016-09-19 16:21 ` Suravee Suthikulpanit
2016-09-20 14:34 ` Boris Ostrovsky
2016-12-22 11:38 ` Jan Beulich
11 siblings, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-19 13:09 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:39AM -0500, Suravee Suthikulpanit wrote:
> GITHUB
> ======
> Latest git tree can be found at:
> http://github.com/ssuthiku/xen.git xen_avic_part1_v1
>
> 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:
kvm_amd ?, heheh
.. snip..
> Note: In "w/o evtchn" case, the Linux guest is built w/o
> Xen guest support.
You can also just boot Linux with 'xen_nopv' parameter which would
do the same thing.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-09-19 13:09 ` [RFC PATCH 0/9] Introduce AMD SVM AVIC Konrad Rzeszutek Wilk
@ 2016-09-19 16:21 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-09-19 16:21 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On 9/19/16 20:09, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 19, 2016 at 12:52:39AM -0500, Suravee Suthikulpanit wrote:
>> GITHUB
>> ======
>> Latest git tree can be found at:
>> http://github.com/ssuthiku/xen.git xen_avic_part1_v1
>>
>> 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:
>
> kvm_amd ?, heheh
Ooops. hehehe
>
> .. snip..
>> Note: In "w/o evtchn" case, the Linux guest is built w/o
>> Xen guest support.
>
> You can also just boot Linux with 'xen_nopv' parameter which would
> do the same thing.
>
Ok, Thanks for the tips.
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (9 preceding siblings ...)
2016-09-19 13:09 ` [RFC PATCH 0/9] Introduce AMD SVM AVIC Konrad Rzeszutek Wilk
@ 2016-09-20 14:34 ` Boris Ostrovsky
2016-12-04 7:40 ` Suravee Suthikulpanit
2016-12-22 11:38 ` Jan Beulich
11 siblings, 1 reply; 43+ messages in thread
From: Boris Ostrovsky @ 2016-09-20 14:34 UTC (permalink / raw)
To: Suravee Suthikulpanit, xen-devel; +Cc: andrew.cooper3, jbeulich, sherry.hurwitz
On 09/19/2016 01:52 AM, Suravee Suthikulpanit wrote:
> GITHUB
> ======
> Latest git tree can be found at:
> http://github.com/ssuthiku/xen.git xen_avic_part1_v1
>
> 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 injection 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 Xen parameter svm-avic=1.
>
> 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.
>
> OVERALL PERFORMANCE
> ===================
> Currently, AVIC is available on the AMD family 15h models 6Xh
> (Carrizo) processors and newer. Here, the Carizzo is used to collect
> performance data shown below.
>
> Generally, SVM AVIC alone (w/o IOMMU AVIC) should provide overall speed up
> for HVM guest since it does not require #vmexit into the hypervisor to
> emulate certain guest accesses to local APIC registers.
>
> It should also improve performance when hypervisor wants to inject
> interrupts into a running vcpu by setting the corresponded IRR
> bit in the vAPIC backing page and trigger AVIC_DOORBELL MSR.
>
> For sending IPI interrupts between running vcpus in Linux guest,
> Xen is default to using event channel. However, in case of
> non-paravirtualize guest, AVIC can also provide performance
> improvements for sending IPI.
>
> BENCHMARK 1: HACKBENCH
> ======================
>
> For measuring IPI performance used for scheduling workload, I have collected
> some performance number on 2 and 3 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 (sec) | 3 vcpus (sec)
> --------------------------------------------------------
> No AVIC w/o evtchn | 299.57 | 337.779
> No AVIC w/ evtchn | 270.37 | 419.6064
> AVIC w/ evtchn | 181.46 | 171.7957
> AVIC w/o evtchn | 171.81 | 169.0858
>
> Note: In "w/o evtchn" case, the Linux guest is built w/o
> Xen guest support.
Enlightened Linux tries to avoid using event channels for APIC accesses
if XEN_HVM_CPUID_APIC_ACCESS_VIRT or XEN_HVM_CPUID_X2APIC_VIRT is set.
I didn't notice either of these two bits set in the series. Should they
be (probably the first one)? Or is this something you are planning for
the second part?
-boris
>
> CURRENT UNTESTED USE-CASES
> ===========================
> - Nested VM
>
> Any feedback and comments are very much appreciated.
>
> Thank you,
> Suravee
>
> Suravee Suthikulpanit (9):
> x86/HVM: Introduce struct hvm_pi_ops
> x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as
> non-static
> x86/HVM: Call vlapic_destroy after vcpu_destroy
> x86/SVM: Modify VMCB fields to add AVIC support
> x86/HVM/SVM: Add AVIC initialization code
> x86/SVM: Add AVIC vmexit handlers
> x86/SVM: Add vcpu scheduling support for AVIC
> x86/SVM: Add interrupt management code via AVIC
> x86/SVM: Hook up miscellaneous AVIC functions
>
> xen/arch/x86/hvm/hvm.c | 4 +-
> xen/arch/x86/hvm/svm/Makefile | 1 +
> xen/arch/x86/hvm/svm/avic.c | 609 +++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/intr.c | 4 +
> xen/arch/x86/hvm/svm/svm.c | 57 +++-
> xen/arch/x86/hvm/svm/vmcb.c | 9 +-
> xen/arch/x86/hvm/vlapic.c | 5 +-
> xen/arch/x86/hvm/vmx/vmx.c | 32 +-
> xen/include/asm-x86/hvm/domain.h | 63 ++++
> xen/include/asm-x86/hvm/hvm.h | 4 +-
> xen/include/asm-x86/hvm/svm/avic.h | 49 +++
> xen/include/asm-x86/hvm/svm/svm.h | 2 +
> xen/include/asm-x86/hvm/svm/vmcb.h | 35 ++-
> xen/include/asm-x86/hvm/vlapic.h | 4 +
> xen/include/asm-x86/hvm/vmx/vmcs.h | 59 ----
> 15 files changed, 843 insertions(+), 94 deletions(-)
> create mode 100644 xen/arch/x86/hvm/svm/avic.c
> create mode 100644 xen/include/asm-x86/hvm/svm/avic.h
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops
2016-09-19 5:52 ` [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops Suravee Suthikulpanit
@ 2016-10-12 17:01 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-12 17:01 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:40AM -0500, Suravee Suthikulpanit wrote:
> The current function pointers for managing hvm posted interrupt
> can be used also by SVM AVIC. Therefore, this patch introduces the
> struct hvm_pi_ops in the struct hvm_domain to hold them.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
You seemed to have forgotten to CC the VMX maintainers:
INTEL(R) VT FOR X86 (VT-X)
M: Jun Nakajima <jun.nakajima@intel.com>
M: Kevin Tian <kevin.tian@intel.com>
S: Supported
?
Regardlesss of that, Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 32 +++++++++----------
> xen/include/asm-x86/hvm/domain.h | 63 ++++++++++++++++++++++++++++++++++++++
> xen/include/asm-x86/hvm/hvm.h | 4 +--
> xen/include/asm-x86/hvm/vmx/vmcs.h | 59 -----------------------------------
> 4 files changed, 81 insertions(+), 77 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2759e6f..8620697 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -204,12 +204,12 @@ void vmx_pi_hooks_assign(struct domain *d)
> if ( !iommu_intpost || !has_hvm_container_domain(d) )
> return;
>
> - ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
> + ASSERT(!d->arch.hvm_domain.pi_ops.vcpu_block);
>
> - d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
> - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
> - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
> - d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
> + d->arch.hvm_domain.pi_ops.vcpu_block = vmx_vcpu_block;
> + d->arch.hvm_domain.pi_ops.pi_switch_from = vmx_pi_switch_from;
> + d->arch.hvm_domain.pi_ops.pi_switch_to = vmx_pi_switch_to;
> + d->arch.hvm_domain.pi_ops.pi_do_resume = vmx_pi_do_resume;
> }
>
> /* This function is called when pcidevs_lock is held */
> @@ -218,12 +218,12 @@ void vmx_pi_hooks_deassign(struct domain *d)
> if ( !iommu_intpost || !has_hvm_container_domain(d) )
> return;
>
> - ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
> + ASSERT(d->arch.hvm_domain.pi_ops.vcpu_block);
>
> - d->arch.hvm_domain.vmx.vcpu_block = NULL;
> - d->arch.hvm_domain.vmx.pi_switch_from = NULL;
> - d->arch.hvm_domain.vmx.pi_switch_to = NULL;
> - d->arch.hvm_domain.vmx.pi_do_resume = NULL;
> + d->arch.hvm_domain.pi_ops.vcpu_block = NULL;
> + d->arch.hvm_domain.pi_ops.pi_switch_from = NULL;
> + d->arch.hvm_domain.pi_ops.pi_switch_to = NULL;
> + d->arch.hvm_domain.pi_ops.pi_do_resume = NULL;
> }
>
> static int vmx_domain_initialise(struct domain *d)
> @@ -901,8 +901,8 @@ static void vmx_ctxt_switch_from(struct vcpu *v)
> vmx_restore_host_msrs();
> vmx_save_dr(v);
>
> - if ( v->domain->arch.hvm_domain.vmx.pi_switch_from )
> - v->domain->arch.hvm_domain.vmx.pi_switch_from(v);
> + if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_from )
> + v->domain->arch.hvm_domain.pi_ops.pi_switch_from(v);
> }
>
> static void vmx_ctxt_switch_to(struct vcpu *v)
> @@ -916,8 +916,8 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
> vmx_restore_guest_msrs(v);
> vmx_restore_dr(v);
>
> - if ( v->domain->arch.hvm_domain.vmx.pi_switch_to )
> - v->domain->arch.hvm_domain.vmx.pi_switch_to(v);
> + if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_to )
> + v->domain->arch.hvm_domain.pi_ops.pi_switch_to(v);
> }
>
>
> @@ -3914,8 +3914,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> struct hvm_vcpu_asid *p_asid;
> bool_t need_flush;
>
> - if ( curr->domain->arch.hvm_domain.vmx.pi_do_resume )
> - curr->domain->arch.hvm_domain.vmx.pi_do_resume(curr);
> + if ( curr->domain->arch.hvm_domain.pi_ops.pi_do_resume )
> + curr->domain->arch.hvm_domain.pi_ops.pi_do_resume(curr);
>
> if ( !cpu_has_vmx_vpid )
> goto out;
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index f34d784..779927b 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -72,6 +72,67 @@ struct hvm_ioreq_server {
> bool_t bufioreq_atomic;
> };
>
> +struct hvm_pi_ops {
> + /*
> + * To handle posted interrupts correctly, we need to set the following
> + * state:
> + *
> + * * The PI notification vector (NV)
> + * * The PI notification destination processor (NDST)
> + * * The PI "suppress notification" bit (SN)
> + * * The vcpu pi "blocked" list
> + *
> + * If a VM is currently running, we want the PI delivered to the guest vcpu
> + * on the proper pcpu (NDST = v->processor, SN clear).
> + *
> + * If the vm is blocked, we want the PI delivered to Xen so that it can
> + * wake it up (SN clear, NV = pi_wakeup_vector, vcpu on block list).
> + *
> + * If the VM is currently either preempted or offline (i.e., not running
> + * because of some reason other than blocking waiting for an interrupt),
> + * there's nothing Xen can do -- we want the interrupt pending bit set in
> + * the guest, but we don't want to bother Xen with an interrupt (SN clear).
> + *
> + * There's a brief window of time between vmx_intr_assist() and checking
> + * softirqs where if an interrupt comes in it may be lost; so we need Xen
> + * to get an interrupt and raise a softirq so that it will go through the
> + * vmx_intr_assist() path again (SN clear, NV = posted_interrupt).
> + *
> + * The way we implement this now is by looking at what needs to happen on
> + * the following runstate transitions:
> + *
> + * A: runnable -> running
> + * - SN = 0
> + * - NDST = v->processor
> + * B: running -> runnable
> + * - SN = 1
> + * C: running -> blocked
> + * - NV = pi_wakeup_vector
> + * - Add vcpu to blocked list
> + * D: blocked -> runnable
> + * - NV = posted_intr_vector
> + * - Take vcpu off blocked list
> + *
> + * For transitions A and B, we add hooks into vmx_ctxt_switch_{from,to}
> + * paths.
> + *
> + * For transition C, we add a new arch hook, arch_vcpu_block(), which is
> + * called from vcpu_block() and vcpu_do_poll().
> + *
> + * For transition D, rather than add an extra arch hook on vcpu_wake, we
> + * add a hook on the vmentry path which checks to see if either of the two
> + * actions need to be taken.
> + *
> + * These hooks only need to be called when the domain in question actually
> + * has a physical device assigned to it, so we set and clear the callbacks
> + * as appropriate when device assignment changes.
> + */
> + void (*vcpu_block) (struct vcpu *);
> + void (*pi_switch_from) (struct vcpu *v);
> + void (*pi_switch_to) (struct vcpu *v);
> + void (*pi_do_resume) (struct vcpu *v);
> +};
> +
> struct hvm_domain {
> /* Guest page range used for non-default ioreq servers */
> struct {
> @@ -148,6 +209,8 @@ struct hvm_domain {
> struct list_head list;
> } write_map;
>
> + struct hvm_pi_ops pi_ops;
> +
> union {
> struct vmx_domain vmx;
> struct svm_domain svm;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 81b60d5..c832d9a 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -621,8 +621,8 @@ unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v, bool_t restore);
> struct vcpu *v_ = (v); \
> struct domain *d_ = v_->domain; \
> if ( has_hvm_container_domain(d_) && \
> - (cpu_has_vmx && d_->arch.hvm_domain.vmx.vcpu_block) ) \
> - d_->arch.hvm_domain.vmx.vcpu_block(v_); \
> + (d_->arch.hvm_domain.pi_ops.vcpu_block) ) \
> + d_->arch.hvm_domain.pi_ops.vcpu_block(v_); \
> })
>
> #endif /* __ASM_X86_HVM_HVM_H__ */
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 997f4f5..4ec8b08 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -77,65 +77,6 @@ struct vmx_domain {
> unsigned long apic_access_mfn;
> /* VMX_DOMAIN_* */
> unsigned int status;
> -
> - /*
> - * To handle posted interrupts correctly, we need to set the following
> - * state:
> - *
> - * * The PI notification vector (NV)
> - * * The PI notification destination processor (NDST)
> - * * The PI "suppress notification" bit (SN)
> - * * The vcpu pi "blocked" list
> - *
> - * If a VM is currently running, we want the PI delivered to the guest vcpu
> - * on the proper pcpu (NDST = v->processor, SN clear).
> - *
> - * If the vm is blocked, we want the PI delivered to Xen so that it can
> - * wake it up (SN clear, NV = pi_wakeup_vector, vcpu on block list).
> - *
> - * If the VM is currently either preempted or offline (i.e., not running
> - * because of some reason other than blocking waiting for an interrupt),
> - * there's nothing Xen can do -- we want the interrupt pending bit set in
> - * the guest, but we don't want to bother Xen with an interrupt (SN clear).
> - *
> - * There's a brief window of time between vmx_intr_assist() and checking
> - * softirqs where if an interrupt comes in it may be lost; so we need Xen
> - * to get an interrupt and raise a softirq so that it will go through the
> - * vmx_intr_assist() path again (SN clear, NV = posted_interrupt).
> - *
> - * The way we implement this now is by looking at what needs to happen on
> - * the following runstate transitions:
> - *
> - * A: runnable -> running
> - * - SN = 0
> - * - NDST = v->processor
> - * B: running -> runnable
> - * - SN = 1
> - * C: running -> blocked
> - * - NV = pi_wakeup_vector
> - * - Add vcpu to blocked list
> - * D: blocked -> runnable
> - * - NV = posted_intr_vector
> - * - Take vcpu off blocked list
> - *
> - * For transitions A and B, we add hooks into vmx_ctxt_switch_{from,to}
> - * paths.
> - *
> - * For transition C, we add a new arch hook, arch_vcpu_block(), which is
> - * called from vcpu_block() and vcpu_do_poll().
> - *
> - * For transition D, rather than add an extra arch hook on vcpu_wake, we
> - * add a hook on the vmentry path which checks to see if either of the two
> - * actions need to be taken.
> - *
> - * These hooks only need to be called when the domain in question actually
> - * has a physical device assigned to it, so we set and clear the callbacks
> - * as appropriate when device assignment changes.
> - */
> - void (*vcpu_block) (struct vcpu *);
> - void (*pi_switch_from) (struct vcpu *v);
> - void (*pi_switch_to) (struct vcpu *v);
> - void (*pi_do_resume) (struct vcpu *v);
> };
>
> struct pi_desc {
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static
2016-09-19 5:52 ` [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static Suravee Suthikulpanit
@ 2016-10-12 19:00 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-12 19:00 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:41AM -0500, Suravee Suthikulpanit wrote:
> Expose vlapic_read_aligned and vlapic_reg_write() to be used by AVIC.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
.. this was a hard patch to review :-)
> ---
> xen/arch/x86/hvm/vlapic.c | 5 ++---
> xen/include/asm-x86/hvm/vlapic.h | 4 ++++
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 1d5d287..0f52067 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -562,7 +562,7 @@ static void vlapic_set_tdcr(struct vlapic *vlapic, unsigned int val)
> "timer_divisor: %d", vlapic->hw.timer_divisor);
> }
>
> -static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
> +uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
> {
> switch ( offset )
> {
> @@ -680,8 +680,7 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
> vcpu_vlapic(v)->hw.tdt_msr = 0;
> }
>
> -static void vlapic_reg_write(struct vcpu *v,
> - unsigned int offset, uint32_t val)
> +void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val)
> {
> struct vlapic *vlapic = vcpu_vlapic(v);
>
> diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
> index 4656293..48ab3a6 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -132,6 +132,10 @@ void vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);
>
> int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
>
> +void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val);
> +
> +uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset);
> +
> struct vlapic *vlapic_lowest_prio(
> struct domain *d, const struct vlapic *source,
> int short_hand, uint32_t dest, bool_t dest_mode);
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy
2016-09-19 5:52 ` [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy Suravee Suthikulpanit
@ 2016-10-12 19:02 ` Konrad Rzeszutek Wilk
2016-12-22 11:09 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-12 19:02 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:42AM -0500, Suravee Suthikulpanit wrote:
> Since vlapic_init() is called before vcpu_initialise().
> We should also follow the same order here.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
But it would also be good to CC the Intel VMX maintainers in case they
spot something in vmx_vcpu_destroy.
> ---
> xen/arch/x86/hvm/hvm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 7bad845..fb5bf6c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1606,10 +1606,10 @@ void hvm_vcpu_destroy(struct vcpu *v)
> tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
> hvm_vcpu_cacheattr_destroy(v);
>
> + hvm_funcs.vcpu_destroy(v);
> +
> if ( is_hvm_vcpu(v) )
> vlapic_destroy(v);
> -
> - hvm_funcs.vcpu_destroy(v);
> }
>
> void hvm_vcpu_down(struct vcpu *v)
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support
2016-09-19 5:52 ` [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support Suravee Suthikulpanit
@ 2016-10-12 19:07 ` Konrad Rzeszutek Wilk
2016-12-22 11:11 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-12 19:07 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:43AM -0500, Suravee Suthikulpanit wrote:
> Introduce AVIC-related VMCB fields.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/include/asm-x86/hvm/svm/vmcb.h | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
> index bad2382..768e9fb 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -328,14 +328,15 @@ typedef union __packed
> struct
> {
> u64 tpr: 8;
> - u64 irq: 1;
> + u64 irq: 1; /* disabled for avic */
> u64 rsvd0: 7;
> - u64 prio: 4;
> - u64 ign_tpr: 1;
> + u64 prio: 4; /* disabled for avic */
> + u64 ign_tpr: 1; /* disabled for avic */
> u64 rsvd1: 3;
> u64 intr_masking: 1;
> - u64 rsvd2: 7;
> - u64 vector: 8;
> + u64 rsvd2: 6;
> + u64 avic_enable: 1;
> + u64 vector: 8; /* disabled for avic */
Perhaps 'avic implicitly disables this' ?
> u64 rsvd3: 24;
> } fields;
> } vintr_t;
> @@ -394,7 +395,8 @@ typedef union __packed
> uint32_t cr2: 1;
> /* debugctlmsr, last{branch,int}{to,from}ip */
> uint32_t lbr: 1;
> - uint32_t resv: 21;
> + uint32_t avic: 1;
> + uint32_t resv: 20;
> } fields;
> } vmcbcleanbits_t;
>
> @@ -428,7 +430,8 @@ struct __packed vmcb_struct {
> u64 exitinfo2; /* offset 0x80 */
> eventinj_t exitintinfo; /* offset 0x88 */
> u64 _np_enable; /* offset 0x90 - cleanbit 4 */
> - u64 res08[2];
> + u64 avic_vapic_bar; /* offset 0x98 */
> + u64 res08; /* offset 0xA0 */
> eventinj_t eventinj; /* offset 0xA8 */
> u64 _h_cr3; /* offset 0xB0 - cleanbit 4 */
> lbrctrl_t lbr_control; /* offset 0xB8 */
> @@ -437,7 +440,11 @@ struct __packed vmcb_struct {
> u64 nextrip; /* offset 0xC8 */
> u8 guest_ins_len; /* offset 0xD0 */
> u8 guest_ins[15]; /* offset 0xD1 */
> - u64 res10a[100]; /* offset 0xE0 pad to save area */
> + u64 avic_bk_pg_pa; /* offset 0xE0 */
> + u64 res09a; /* offset 0xE8 */
> + u64 avic_log_apic_id; /* offset 0xF0 */
> + u64 avic_phy_apic_id; /* offset 0xF8 */
> + u64 res09b[96]; /* offset 0x100 pad to save area */
>
Otherwise:
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> svm_segment_register_t es; /* offset 1024 - cleanbit 8 */
> svm_segment_register_t cs; /* cleanbit 8 */
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
@ 2016-10-12 20:02 ` Konrad Rzeszutek Wilk
2016-11-17 16:05 ` Suravee Suthikulpanit
2016-11-17 16:55 ` Suravee Suthikulpanit
2016-10-14 14:03 ` Konrad Rzeszutek Wilk
2016-12-22 11:16 ` Jan Beulich
2 siblings, 2 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-12 20:02 UTC (permalink / raw)
To: Suravee Suthikulpanit, boris.ostrovsky
Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:44AM -0500, Suravee Suthikulpanit wrote:
> Introduce AVIC base initialization code. This includes:
> * Setting up per-VM data structures.
> * Setting up per-vCPU data structure.
> * Initializing AVIC-related VMCB bit fields.
>
> This patch also introduces a new Xen parameter (svm-avic),
> which can be used to enable/disable AVIC support.
> Currently, this svm-avic is disabled by default.
Oddly enough you didn't CC the SVM maintainer: Boris Ostrovsky on all
these patches.
Adding him here.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/arch/x86/hvm/svm/Makefile | 1 +
> xen/arch/x86/hvm/svm/avic.c | 217 +++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 17 ++-
> xen/arch/x86/hvm/svm/vmcb.c | 3 +
> xen/include/asm-x86/hvm/svm/avic.h | 40 +++++++
> xen/include/asm-x86/hvm/svm/svm.h | 2 +
> xen/include/asm-x86/hvm/svm/vmcb.h | 10 ++
> 7 files changed, 289 insertions(+), 1 deletion(-)
> create mode 100644 xen/arch/x86/hvm/svm/avic.c
> create mode 100644 xen/include/asm-x86/hvm/svm/avic.h
>
> diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile
> index 760d295..e0e4a59 100644
> --- a/xen/arch/x86/hvm/svm/Makefile
> +++ b/xen/arch/x86/hvm/svm/Makefile
> @@ -1,4 +1,5 @@
> obj-y += asid.o
> +obj-y += avic.o
> obj-y += emulate.o
> obj-bin-y += entry.o
> obj-y += intr.o
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> new file mode 100644
> index 0000000..70bac69
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,217 @@
> +#include <xen/domain_page.h>
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <asm/acpi.h>
> +#include <asm/apicdef.h>
> +#include <asm/event.h>
> +#include <asm/p2m.h>
> +#include <asm/page.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/svm/avic.h>
> +#include <asm/hvm/vlapic.h>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/support.h>
Since this a new file could you kindly sort these? Also do you need a copyright
header at the top?
> +
> +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
> +#define AVIC_PHY_APIC_ID_MAX 0xFF
> +
> +#define AVIC_DOORBELL 0xc001011b
> +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
> +#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
> +
> +bool_t svm_avic = 0;
> +boolean_param("svm-avic", svm_avic);
Why do you want to have it disabled by default?
Also you are missing an patch to the docs/misc/xen-command-line where you would
document what this parameter does.
> +
> +static struct svm_avic_phy_ait_entry *
> +avic_get_phy_ait_entry(struct vcpu *v, int index)
Could I convience you use 'const struct vcpu *v, unsigned int index' ?
> +{
> + struct svm_avic_phy_ait_entry *avic_phy_ait;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> +
> + if ( !d->avic_phy_ait_mfn )
> + return NULL;
> +
> + /**
This '**' is not the standard style. Could you use only one '*' please?
> + * Note: APIC ID = 0xff is used for broadcast.
> + * APIC ID > 0xff is reserved.
> + */
> + if ( index >= 0xff )
> + return NULL;
> +
> + avic_phy_ait = mfn_to_virt(d->avic_phy_ait_mfn);
> +
> + return &avic_phy_ait[index];
> +}
> +
> +/***************************************************************
Ditto
> + * AVIC APIs
> + */
> +int svm_avic_dom_init(struct domain *d)
> +{
> + int ret = 0;
> + struct page_info *pg;
> + unsigned long mfn;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + /**
> + * Note:
> + * AVIC hardware walks the nested page table to check permissions,
> + * but does not use the SPA address specified in the leaf page
> + * table entry since it uses address in the AVIC_BACKING_PAGE pointer
> + * field of the VMCB. Therefore, we set up a dummy page here _mfn(0).
> + */
> + if ( !d->arch.hvm_domain.svm.avic_access_page_done )
> + {
> + set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> + _mfn(0), PAGE_ORDER_4K,
> + p2m_get_hostp2m(d)->default_access);
> + d->arch.hvm_domain.svm.avic_access_page_done = true;
> + }
> +
> + /* Init AVIC log APIC ID table */
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC logical APIC ID table error: %d\n",
How about "%d: AVIC logical .. could not be allocated" ?
> + d->domain_id);
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_log_ait_mfn = mfn;
> +
> + /* Init AVIC phy APIC ID table */
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC physical APIC ID table error: %d\n",
> + d->domain_id);
Ditto.
You could also use gdprintk instead?
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
> +
> + return ret;
> +err_out:
Add an extra space in front of the label please.
> + svm_avic_dom_destroy(d);
> + return ret;
> +}
> +
> +void svm_avic_dom_destroy(struct domain *d)
> +{
> + if ( !svm_avic )
> + return;
> +
> + if ( d->arch.hvm_domain.svm.avic_phy_ait_mfn )
> + {
> + free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_phy_ait_mfn));
> + d->arch.hvm_domain.svm.avic_phy_ait_mfn = 0;
> + }
> +
> + if ( d->arch.hvm_domain.svm.avic_log_ait_mfn )
> + {
> + free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_log_ait_mfn));
> + d->arch.hvm_domain.svm.avic_log_ait_mfn = 0;
> + }
> +}
> +
> +/**
> + * Note: At this point, vlapic->regs_page is already initialized.
> + */
> +int svm_avic_init_vcpu(struct vcpu *v)
> +{
> + struct vlapic *vlapic = vcpu_vlapic(v);
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( svm_avic )
> + s->avic_bk_pg = vlapic->regs_page;
> + return 0;
> +}
> +
> +void svm_avic_destroy_vcpu(struct vcpu *v)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( svm_avic && s->avic_bk_pg )
> + s->avic_bk_pg = NULL;
> +}
> +
> +bool_t svm_avic_vcpu_enabled(struct vcpu *v)
bool please
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> +
> + return ( svm_avic && vmcb->_vintr.fields.avic_enable);
Could you drop the () please?
> +}
> +
> +static inline u32 *
> +avic_get_bk_page_entry(struct vcpu *v, u32 offset)
const on 'struct vcpu' please?
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct page_info *pg = s->avic_bk_pg;
> + char *tmp;
> +
> + if ( !pg )
> + return NULL;
> +
> + tmp = (char *) page_to_virt(pg);
Extra space not needed.
> + return (u32*)(tmp+offset);
Perhaps:
return (u32 *)(tmp + offset);
?
> +}
> +
> +void svm_avic_update_vapic_bar(struct vcpu *v, uint64_t data)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + s->vmcb->avic_vapic_bar = data & AVIC_APIC_BAR_MASK;
> + s->vmcb->cleanbits.fields.avic = 0;
> +}
> +
> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> + paddr_t ma;
> + u32 apic_id_reg;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
> + ma = d->avic_log_ait_mfn;
> + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + ma = d->avic_phy_ait_mfn;
> + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> +
> + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
I think you can drop the 'SVM:' part. The __func__ gives enough details.
> + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
> + (unsigned long long) vmcb->avic_log_apic_id,
> + (unsigned long long) vmcb->avic_phy_apic_id);
Is this also part of the keyboard handler? Perhaps that information should
be presented there?
> +
> +
> + apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
Um, what if it returns NULL? You would derefencing NULL. Perhaps check for that first?
> + s->avic_phy_id_cache = avic_get_phy_ait_entry(v, apic_id_reg >> 24);
> + if ( !s->avic_phy_id_cache )
> + return -EINVAL;
You don't want to unroll the entries that got set earlier? avic_[bk_pg_pa,phy_apic_id,log_apic_id] ?
> +
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa >> 12) & 0xffffffffff;
Should the 0xff.. have some soft of macro?
Also the 12 looks suspicious like some other macro value.
> + entry.is_running= 0;
Missing space.
> + entry.valid = 1;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +
> + svm_avic_update_vapic_bar(v, APIC_DEFAULT_PHYS_BASE);
> +
> + vmcb->_vintr.fields.avic_enable = 1;
> +
> + return 0;
> +}
Please also add:
/*
* Local variables:
* mode: C
* c-file-style: "BSD"
* c-basic-offset: 4
* tab-width: 4
* indent-tabs-mode: nil
* End:
*/
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 679e615..bcb7df4 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -48,6 +48,7 @@
> #include <asm/hvm/svm/asid.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/hvm/svm/vmcb.h>
> +#include <asm/hvm/svm/avic.h>
> #include <asm/hvm/svm/emulate.h>
> #include <asm/hvm/svm/intr.h>
> #include <asm/hvm/svm/svmdebug.h>
> @@ -1164,11 +1165,12 @@ void svm_host_osvw_init()
>
> static int svm_domain_initialise(struct domain *d)
> {
> - return 0;
> + return svm_avic_dom_init(d);
> }
>
> static void svm_domain_destroy(struct domain *d)
> {
> + svm_avic_dom_destroy(d);
> }
>
> static int svm_vcpu_initialise(struct vcpu *v)
> @@ -1181,6 +1183,13 @@ static int svm_vcpu_initialise(struct vcpu *v)
>
> v->arch.hvm_svm.launch_core = -1;
>
> + if ( (rc = svm_avic_init_vcpu(v)) != 0 )
> + {
> + dprintk(XENLOG_WARNING,
> + "Failed to initiazlize AVIC vcpu.\n");
> + return rc;
> + }
> +
> if ( (rc = svm_create_vmcb(v)) != 0 )
> {
> dprintk(XENLOG_WARNING,
> @@ -1200,6 +1209,7 @@ static int svm_vcpu_initialise(struct vcpu *v)
>
> static void svm_vcpu_destroy(struct vcpu *v)
> {
> + svm_avic_destroy_vcpu(v);
> vpmu_destroy(v);
> svm_destroy_vmcb(v);
> passive_domain_destroy(v);
> @@ -1464,6 +1474,7 @@ const struct hvm_function_table * __init start_svm(void)
> P(cpu_has_svm_decode, "DecodeAssists");
> P(cpu_has_pause_filter, "Pause-Intercept Filter");
> P(cpu_has_tsc_ratio, "TSC Rate MSR");
> + P(cpu_has_svm_avic, "AVIC");
> #undef P
>
> if ( !printed )
> @@ -1809,6 +1820,10 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>
> switch ( msr )
> {
> + case MSR_IA32_APICBASE:
> + if ( svm_avic_vcpu_enabled(v) )
> + svm_avic_update_vapic_bar(v, msr_content);
> + break;
> case MSR_IA32_SYSENTER_CS:
> case MSR_IA32_SYSENTER_ESP:
> case MSR_IA32_SYSENTER_EIP:
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 9ea014f..9ee7fc7 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -28,6 +28,7 @@
> #include <asm/msr-index.h>
> #include <asm/p2m.h>
> #include <asm/hvm/support.h>
> +#include <asm/hvm/svm/avic.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/hvm/svm/svmdebug.h>
>
> @@ -225,6 +226,8 @@ static int construct_vmcb(struct vcpu *v)
> vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
> }
>
> + svm_avic_init_vmcb(v);
> +
> vmcb->cleanbits.bytes = 0;
>
> return 0;
> diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
> new file mode 100644
> index 0000000..9508486
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -0,0 +1,40 @@
> +#ifndef _SVM_AVIC_H_
> +#define _SVM_AVIC_H_
> +
> +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,
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_log_ait_entry {
> + u32 guest_phy_apic_id : 8;
> + u32 res : 23;
> + u32 valid : 1;
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_phy_ait_entry {
> + u64 host_phy_apic_id : 8;
> + u64 res1 : 4;
> + u64 bk_pg_ptr : 40;
> + u64 res2 : 10;
> + u64 is_running : 1;
> + u64 valid : 1;
> +};
> +
> +extern bool_t svm_avic;
> +
> +int svm_avic_dom_init(struct domain *d);
> +void svm_avic_dom_destroy(struct domain *d);
> +
> +int svm_avic_init_vcpu(struct vcpu *v);
> +void svm_avic_destroy_vcpu(struct vcpu *v);
> +bool_t svm_avic_vcpu_enabled(struct vcpu *v);
> +
> +void svm_avic_update_vapic_bar(struct vcpu *v,uint64_t data);
> +int svm_avic_init_vmcb(struct vcpu *v);
> +
> +#endif /* _SVM_AVIC_H_ */
> diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h
> index c954b7e..fea61bb 100644
> --- a/xen/include/asm-x86/hvm/svm/svm.h
> +++ b/xen/include/asm-x86/hvm/svm/svm.h
> @@ -81,6 +81,7 @@ extern u32 svm_feature_flags;
> #define SVM_FEATURE_FLUSHBYASID 6 /* TLB flush by ASID support */
> #define SVM_FEATURE_DECODEASSISTS 7 /* Decode assists support */
> #define SVM_FEATURE_PAUSEFILTER 10 /* Pause intercept filter support */
> +#define SVM_FEATURE_AVIC 13 /* AVIC support */
>
> #define cpu_has_svm_feature(f) test_bit(f, &svm_feature_flags)
> #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT)
> @@ -91,6 +92,7 @@ extern u32 svm_feature_flags;
> #define cpu_has_svm_decode cpu_has_svm_feature(SVM_FEATURE_DECODEASSISTS)
> #define cpu_has_pause_filter cpu_has_svm_feature(SVM_FEATURE_PAUSEFILTER)
> #define cpu_has_tsc_ratio cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR)
> +#define cpu_has_svm_avic cpu_has_svm_feature(SVM_FEATURE_AVIC)
>
> #define SVM_PAUSEFILTER_INIT 3000
>
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
> index 768e9fb..a42c034 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -496,6 +496,12 @@ struct __packed vmcb_struct {
> };
>
> struct svm_domain {
> + u32 avic_max_vcpu_id;
> + bool_t avic_access_page_done;
bool pls
> + paddr_t avic_log_ait_mfn;
> + paddr_t avic_phy_ait_mfn;
> + u32 ldr_reg;
> + u32 ldr_mode;
> };
>
> struct arch_svm_struct {
> @@ -529,6 +535,10 @@ struct arch_svm_struct {
> u64 length;
> u64 status;
> } osvw;
> +
> + /* AVIC APIC backing page */
> + struct page_info *avic_bk_pg;
> + struct svm_avic_phy_ait_entry *avic_phy_id_cache;
> };
>
> struct vmcb_struct *alloc_vmcb(void);
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
@ 2016-10-14 14:03 ` Konrad Rzeszutek Wilk
2016-12-22 11:16 ` Jan Beulich
2 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-14 14:03 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
. snip..
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> new file mode 100644
> index 0000000..70bac69
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,217 @@
> +#include <xen/domain_page.h>
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <asm/acpi.h>
> +#include <asm/apicdef.h>
> +#include <asm/event.h>
> +#include <asm/p2m.h>
> +#include <asm/page.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/svm/avic.h>
> +#include <asm/hvm/vlapic.h>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/support.h>
> +
> +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
> +#define AVIC_PHY_APIC_ID_MAX 0xFF
> +
> +#define AVIC_DOORBELL 0xc001011b
> +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
> +#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
> +
> +bool_t svm_avic = 0;
static ? And also make it 'bool'
> +boolean_param("svm-avic", svm_avic);
> +
> +static struct svm_avic_phy_ait_entry *
This name resolves to 'SVM AVIC Physical APIC ID Table Entry'
That 'ait' keeps throwing me off as it sounds like 'eat' to me.
Perhaps 'avic_phy_apic_id_ent' ?
In other words s/ait/apic_id/ and s/svm_avic/avic/ followed by
s/_id_entry/id_ent/ ?
> +avic_get_phy_ait_entry(struct vcpu *v, int index)
> +{
> + struct svm_avic_phy_ait_entry *avic_phy_ait;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> +
> + if ( !d->avic_phy_ait_mfn )
> + return NULL;
> +
> + /**
> + * Note: APIC ID = 0xff is used for broadcast.
> + * APIC ID > 0xff is reserved.
> + */
> + if ( index >= 0xff )
> + return NULL;
> +
> + avic_phy_ait = mfn_to_virt(d->avic_phy_ait_mfn);
> +
> + return &avic_phy_ait[index];
> +}
> +
> +/***************************************************************
> + * AVIC APIs
> + */
> +int svm_avic_dom_init(struct domain *d)
> +{
> + int ret = 0;
> + struct page_info *pg;
> + unsigned long mfn;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + /**
> + * Note:
> + * AVIC hardware walks the nested page table to check permissions,
> + * but does not use the SPA address specified in the leaf page
> + * table entry since it uses address in the AVIC_BACKING_PAGE pointer
> + * field of the VMCB. Therefore, we set up a dummy page here _mfn(0).
s/here/for APIC/
> + */
> + if ( !d->arch.hvm_domain.svm.avic_access_page_done )
> + {
> + set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> + _mfn(0), PAGE_ORDER_4K,
> + p2m_get_hostp2m(d)->default_access);
> + d->arch.hvm_domain.svm.avic_access_page_done = true;
> + }
> +
> + /* Init AVIC log APIC ID table */
s/log/logical/
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC logical APIC ID table error: %d\n",
> + d->domain_id);
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_log_ait_mfn = mfn;
> +
> + /* Init AVIC phy APIC ID table */
physical ?
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC physical APIC ID table error: %d\n",
> + d->domain_id);
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
> +
> + return ret;
> +err_out:
> + svm_avic_dom_destroy(d);
> + return ret;
> +}
> +
.. snip..
> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> + paddr_t ma;
> + u32 apic_id_reg;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
> + ma = d->avic_log_ait_mfn;
> + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + ma = d->avic_phy_ait_mfn;
s/ait/apic_id/ ?
> + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> +
After reading the spec these tables make a bit more sense - one to
translate logical APIC -> physical APIC id, and the last one to
translate to host APIC.
It may be good to include just an simple ASCII flow of how say
IPI for a two vCPU guest would go?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers
2016-09-19 5:52 ` [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers Suravee Suthikulpanit
@ 2016-10-14 15:20 ` Konrad Rzeszutek Wilk
2016-12-12 10:34 ` Suravee Suthikulpanit
2016-12-22 11:25 ` Jan Beulich
1 sibling, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-14 15:20 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:45AM -0500, Suravee Suthikulpanit wrote:
> AVIC introduces two #vmexit handlers:
> * VMEXIT_INCOMP_IPI
> * VMEXIT_DO_NOACCEL
Great.. Can you describe what you are suppose to do with them?
Please keep in mind that the point of the commit description is to
say something about the behavior or what not.
You can also just point to the AMD manual, but it would be better
if you included some explanation of why you wrote the code this way
or such.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/arch/x86/hvm/svm/avic.c | 279 +++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 8 ++
> xen/include/asm-x86/hvm/svm/avic.h | 3 +
> xen/include/asm-x86/hvm/svm/vmcb.h | 2 +
> 4 files changed, 292 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> index 70bac69..90df181 100644
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -18,6 +18,7 @@
> #define AVIC_DOORBELL 0xc001011b
> #define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
> #define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
> +#define AVIC_UNACCEL_ACCESS_OFFSET_MASK 0xFF0
>
> bool_t svm_avic = 0;
> boolean_param("svm-avic", svm_avic);
> @@ -215,3 +216,281 @@ int svm_avic_init_vmcb(struct vcpu *v)
>
> return 0;
> }
> +
> +/***************************************************************
That is a lot of stars. Please shrink to one.
> + * AVIC INCOMP IPI VMEXIT
Hmm, I think I figured that out from the name of the function.
Perhaps you want to say what the function is suppose to do or
under what conditions this would be called instead of bouncing
inside the guest?
Or perhaps just say under what conditions we would _not_
enter here and the guest would run on its merry way?
Or if that is explained in details in the comments in switch statements
then just remove the comment altogether?
> + */
> +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
> +{
> + struct vcpu *v = current;
> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> + u32 icrh = vmcb->exitinfo1 >> 32;
> + u32 icrl = vmcb->exitinfo1;
> + u32 id = vmcb->exitinfo2 >> 32;
> + u32 index = vmcb->exitinfo2 && 0xFF;
> +
> + dprintk(XENLOG_DEBUG, "SVM: %s: cpu=%#x, vcpu=%#x, "
> + "icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
> + __func__, v->processor, v->vcpu_id, icrh, icrl, id, index);
<raises eyebrows>
Please remove this.
> +
> + switch ( id )
> + {
> + case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
> + /*
> + * AVIC hardware handles the generation of
generation? Did you mean 'delivery' ?
> + * 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.
> + */
> + vlapic_reg_write(v, APIC_ICR2, icrh);
> + vlapic_reg_write(v, APIC_ICR, icrl);
> + break;
> + case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
> + {
> + /*
> + * 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.
Is there a pattern to this? Meaning does the CPU go sequentially
through the logical APIC ids - which (if say the guest is using
logical APIC ids which gives you pretty much 1-1 to physical) - means
that this VMEXIT would occur in a sequence of the vCPUs that are not
running? Because if so, then ..
> + */
> + struct vcpu *c;
> + struct domain *d = v->domain;
> + uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
> + uint32_t short_hand = icrl & APIC_SHORT_MASK;
> + bool_t dest_mode = !!(icrl & APIC_DEST_MASK);
bool
> +
> + for_each_vcpu ( d, c )
.. you could optimize this a bit and start at vCPU+1 (since you know
that this current vCPU most certainyl got the vCPU) ?
> + {
> + if ( vlapic_match_dest(vcpu_vlapic(c), vcpu_vlapic(v),
> + short_hand, dest, dest_mode) )
> + {
> + vcpu_kick(c);
> + break;
> + }
> + }
> + break;
> + }
> + case AVIC_INCMP_IPI_ERR_INV_TARGET:
> + dprintk(XENLOG_ERR,
> + "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
> + __func__, icrh, icrl, index);
Please remove this dprintk.
> + break;
> + case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
> + dprintk(XENLOG_ERR,
> + "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> + __func__, icrh, icrl, index);
That should never happen right? If it does that means we messed up the
allocation somehow. If so, should we disable AVIC for this guest
completly and log this error?
Not exactly sure what that means - the manual says that once you enable
AVIC you MUST keep the structures for the life of the guest.
And nothing about what happens if you decide for fun to disable AVIC and
enable it at random times.
> + break;
> + default:
> + dprintk(XENLOG_ERR, "SVM: %s: Unknown IPI interception\n", __func__);
Perhaps include the value as well? But regardless of this -if we do
get this, should we disable AVIC?
> + }
> +}
> +
> +/***************************************************************
Star explosion!
> + * AVIC NOACCEL VMEXIT
I think you know what I will say about that sparse comment :-)
> + */
> +#define GET_APIC_LOGICAL_ID(x) (((x) >> 24) & 0xFFu)
I naively assumed that this macro would be in the code but not so.
Perhaps you can add it in apic.h file? There is an SET_APIC_LOGICAL_ID
so right next to it would be good?
> +
> +static struct svm_avic_log_ait_entry *
> +avic_get_logical_id_entry(struct vcpu *v, u32 ldr, bool flat)
const struct vcpu *v
> +{
> + int index;
unsigned int?
> + struct svm_avic_log_ait_entry *avic_log_ait;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + int dlid = GET_APIC_LOGICAL_ID(ldr);
unsigned int?
'dlid'? How about 'dest_id' ?
> +
> + if ( !dlid )
> + return NULL;
> +
> + if ( flat )
> + {
> + index = ffs(dlid) - 1;
> + if ( index > 7 )
> + return NULL;
> + }
> + else
> + {
> + int cluster = (dlid & 0xf0) >> 4;
unsigned int
> + int apic = ffs(dlid & 0x0f) - 1;
> +
> + if ((apic < 0) || (apic > 7) || (cluster >= 0xf))
Something is off with the (). You need some spaces:
if ( (apic < 0) ..
> + return NULL;
> + index = (cluster << 2) + apic;
> + }
> +
> + avic_log_ait = mfn_to_virt(d->avic_log_ait_mfn);
> +
Would it make sense to add:
ASSERT(index <= 255) ?
> + return &avic_log_ait[index];
> +}
> +
> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
> +{
> + bool flat;
> + struct svm_avic_log_ait_entry *entry, new_entry;
> +
> + flat = *avic_get_bk_page_entry(v, APIC_DFR) == APIC_DFR_FLAT;
If my recollection is right, avic_get_bk_page_entry can return NULL? So
you really don't want to dereference it right away here.
> + entry = avic_get_logical_id_entry(v, ldr, flat);
> + if (!entry)
> + return -EINVAL;
> +
> + new_entry = *entry;
> + smp_rmb();
> + new_entry.guest_phy_apic_id = g_phy_id;
> + new_entry.valid = valid;
> + *entry = new_entry;
> + smp_wmb();
> +
> + return 0;
> +}
> +
> +static int avic_handle_ldr_update(struct vcpu *v)
> +{
> + int ret = 0;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + u32 ldr = *avic_get_bk_page_entry(v, APIC_LDR);
Again, this can be NULL.
> + u32 apic_id = (*avic_get_bk_page_entry(v, APIC_ID) >> 24);
Ditto.
> +
> + if ( !ldr )
> + return 1;
Uh? Perhaps you can put a comment at the start of the function
to talk about the return values?
I would expect this return to be -EINVAL?
> +
> + ret = avic_ldr_write(v, apic_id, ldr, true);
> + if (ret && d->ldr_reg)
You need spaces after ( and before ).
> + {
> + avic_ldr_write(v, 0, d->ldr_reg, false);
Can you add a comment for the 0 and 'false'.
Actually can you explain a bit about the ldr vs ldr_reg values?
It looks like it contains the last LDR value .. but perhaps it is
also set somewhere else?
> + d->ldr_reg = 0;
> + }
> + else
> + {
> + d->ldr_reg = ldr;
> + }
> +
> + return ret;
> +}
> +
> +static int avic_handle_apic_id_update(struct vcpu *v, bool init)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + u32 apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
Again, this can return NULL..
> + u32 id = (apic_id_reg >> 24) & 0xff;
That could use that macro you declared?
> + struct svm_avic_phy_ait_entry *old, *new;
Something is off with the spaces above?
> +
> + old = s->avic_phy_id_cache;
> + new = avic_get_phy_ait_entry(v, id);
> + if ( !new || !old )
Uh, 'old' says cache. That would imply that having it as NULL would be
OK?
> + return 0;
> +
> + /* We need to move physical_id_entry to new offset */
> + *new = *old;
> + *((u64 *)old) = 0ULL;
> + s->avic_phy_id_cache = new;
> +
> + /*
> + * Also update the guest physical APIC ID in the logical
s/Also//
> + * APIC ID table entry if already setup the LDR.
s/already setup the LDR/LDR is already setup/
?
> + */
> + if ( d->ldr_reg )
> + avic_handle_ldr_update(v);
> +
> + return 0;
> +}
> +
> +static int avic_handle_dfr_update(struct vcpu *v)
The return value does not seem to be used? Perhaps make it void?
> +{
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + u32 dfr = *avic_get_bk_page_entry(v, APIC_DFR);
Again, this can be NULL.
> + u32 mod = (dfr >> 28) & 0xf;
> +
> + /*
> + * We assume that all local APICs are using the same type.
> + * If this changes, we need to flush the AVIC logical
> + * APID id table.
> + */
> + if ( d->ldr_mode == mod )
> + return 0;
> +
> + clear_domain_page(_mfn(d->avic_log_ait_mfn));
Whoa. What if another vCPU is using this (aka handling another AVIC
access?)? I see that that the 'V' bit would be cleared so the CPU
would trap .. (thought that depends right - the clear_domain_page is
being done in a sequence so some of the later entries could be valid
while the CPU is clearing it).
Perhaps you should iterate over all the 'V' bit first (to clear them)
and then clear the page using the clear_domain_page?
Or better - turn of AVIC first (for the guest), until the page has been cleared?
Either way I think you also need:
smp_wmb()
here as a memory barrier to flush the changes out.
> + d->ldr_mode = mod;
> + if (d->ldr_reg)
Spaces.
> + avic_handle_ldr_update(v);
> + return 0;
> +}
> +
> +static int avic_unaccel_trap_write(struct vcpu *v)
unsigned int?
> +{
> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> + u32 offset = vmcb->exitinfo1 & AVIC_UNACCEL_ACCESS_OFFSET_MASK;
> + u32 reg = *avic_get_bk_page_entry(v, offset);
Again, please check for NULL?
> +
> + switch ( offset ) {
I think the {
is on its own line.
> + case APIC_ID:
> + if ( avic_handle_apic_id_update(v, false) )
> + return 0;
> + break;
> + case APIC_LDR:
> + if ( avic_handle_ldr_update(v) )
> + return 0;
> + break;
> + case APIC_DFR:
> + avic_handle_dfr_update(v);
You aren't checking its return value?
> + break;
> + default:
> + break;
> + }
> +
> + vlapic_reg_write(v, offset, reg);
> +
> + return 1;
> +}
> +
> +void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
> +{
> + struct vcpu *v = current;
> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> + u32 offset = vmcb->exitinfo1 & 0xFF0;
> + u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
> + u32 vector = vmcb->exitinfo2 & 0xFFFFFFFF;
> +
> + dprintk(XENLOG_DEBUG,
> + "SVM: %s: offset=%#x, rw=%#x, vector=%#x, vcpu_id=%#x, cpu=%#x\n",
> + __func__, offset, rw, vector, v->vcpu_id, v->processor);
> +
Please remove that.
> + switch(offset)
Missing spaces.
> + {
> + 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 */
If it did that would be a truly messed up CPU? You may want to say that:
'If a read trap happens the CPU microcode does not implement the spec.'
> + BUG();
> + avic_unaccel_trap_write(v);
No checking of the function return value?
Say the values are all messed up (the guest provides the wrong
APIC ID).. Shouldn't we inject some type of exception in the guest?
(Like the hardware would do? Actually - would the hardware send any type
of interrupt if one messes up with the APIC? Or is it that it sets an
error bit in the APIC?)
> + break;
> + default:
> + /* Handling Fault */
Which kinds?
> + if ( !rw )
> + *avic_get_bk_page_entry(v, offset) = vlapic_read_aligned(
> + vcpu_vlapic(v), offset);
> +
> + hvm_mem_access_emulate_one(EMUL_KIND_NORMAL, TRAP_invalid_op,
> + HVM_DELIVER_NO_ERROR_CODE);
Odd, something is wrong with the spaces here. The HVM_DELIEV.. is not on
the same line?
So we update the backing page .. and then inject an invalid_op in the
guest? Is that how hardware does it?
> + }
> +
> + return;
> +}
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index bcb7df4..409096a 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2710,6 +2710,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> svm_vmexit_do_pause(regs);
> break;
>
> + case VMEXIT_AVIC_INCOMP_IPI:
> + svm_avic_vmexit_do_incomp_ipi(regs);
> + break;
> +
> + case VMEXIT_AVIC_NOACCEL:
> + svm_avic_vmexit_do_noaccel(regs);
> + break;
> +
> default:
> unexpected_exit_type:
> gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
> diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
> index 9508486..2c501d4 100644
> --- a/xen/include/asm-x86/hvm/svm/avic.h
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -37,4 +37,7 @@ bool_t svm_avic_vcpu_enabled(struct vcpu *v);
> void svm_avic_update_vapic_bar(struct vcpu *v,uint64_t data);
> int svm_avic_init_vmcb(struct vcpu *v);
>
> +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
> +void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
> +
> #endif /* _SVM_AVIC_H_ */
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
> index a42c034..23eb86b 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -302,6 +302,8 @@ enum VMEXIT_EXITCODE
> VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
> VMEXIT_XSETBV = 141, /* 0x8d */
> VMEXIT_NPF = 1024, /* 0x400, nested paging fault */
> + VMEXIT_AVIC_INCOMP_IPI = 1025, /* 0x401 */
> + VMEXIT_AVIC_NOACCEL = 1026, /* 0x402 */
> VMEXIT_INVALID = -1
> };
>
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC
2016-09-19 5:52 ` [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC Suravee Suthikulpanit
@ 2016-10-14 15:31 ` Konrad Rzeszutek Wilk
2016-10-24 11:19 ` Jan Beulich
2016-12-22 11:32 ` Jan Beulich
1 sibling, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-14 15:31 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:46AM -0500, Suravee Suthikulpanit wrote:
> Add hooks to manage AVIC data structure during vcpu scheduling.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/arch/x86/hvm/svm/avic.c | 82 +++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 10 ++++++
> 2 files changed, 92 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> index 90df181..cd8a9d4 100644
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -45,6 +45,83 @@ avic_get_phy_ait_entry(struct vcpu *v, int index)
> }
>
> /***************************************************************
> + * AVIC VCPU SCHEDULING
> + */
> +static void avic_vcpu_load(struct vcpu *v)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + int h_phy_apic_id;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic || !s->avic_phy_id_cache )
> + return;
> +
> + if ( test_bit(_VPF_blocked, &v->pause_flags) )
> + return;
> +
> + /* Note: APIC ID = 0xff is used for broadcast.
Please put the Note: .. on a newline.
So you have it like so:
/*
* Note: ...
> + * APIC ID > 0xff is reserved.
> + */
> + h_phy_apic_id = cpu_data[v->processor].apicid;
> + if ( h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX )
What does that mean to the guest? I presume it means it will always
get an VMEXIT b/c the is_running is not set?
Meaning whatever guest ends up unhappily on an physical CPU with
the APIC ID of 255 is screwed :-(?
Perhaps at bootup time when SVM AVIC is initialized we have a check
for APIC ID of 255 and put a blurb saying:
"You have CPU%u with APIC ID 255. That value for SVM AVIC is reserved
which has the unfortunate consequence that AVIC is disabled on CPU%u."
So that the admin can perhaps schedule/pin dom0 on that CPU?
> + return;
> +
> + entry = *(s->avic_phy_id_cache);
Perhaps instead of '_cache' say '_last' ?
Like 'avic_last_phy_id'?
> + smp_rmb();
> + entry.host_phy_apic_id = h_phy_apic_id;
> + entry.is_running = 1;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +}
> +
> +static void avic_vcpu_put(struct vcpu *v)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic || !s->avic_phy_id_cache )
> + return;
> +
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.is_running = 0;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +}
> +
> +static void avic_vcpu_resume(struct vcpu *v)
> +{
> + struct svm_avic_phy_ait_entry entry;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( !svm_avic_vcpu_enabled(v) || !s->avic_phy_id_cache )
> + return;
> +
> + ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> +
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.is_running = 1;
> + *(s->avic_phy_id_cache)= entry;
^
Please add a space here.
> + smp_wmb();
> +}
> +
> +static void avic_vcpu_block(struct vcpu *v)
> +{
> + struct svm_avic_phy_ait_entry entry;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( !svm_avic_vcpu_enabled(v) || !s->avic_phy_id_cache )
> + return;
> +
Should there be a corresponding ASSERT? Or is that
done after these hooks are called?
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.is_running = 0;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +}
> +
> +/***************************************************************
> * AVIC APIs
> */
> int svm_avic_dom_init(struct domain *d)
> @@ -97,6 +174,11 @@ int svm_avic_dom_init(struct domain *d)
> clear_domain_page(_mfn(mfn));
> d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
>
> + d->arch.hvm_domain.pi_ops.pi_switch_to = avic_vcpu_put;
> + d->arch.hvm_domain.pi_ops.pi_switch_from = avic_vcpu_load;
> + d->arch.hvm_domain.pi_ops.vcpu_block = avic_vcpu_block;
> + d->arch.hvm_domain.pi_ops.pi_do_resume = avic_vcpu_resume;
> +
> return ret;
> err_out:
> svm_avic_dom_destroy(d);
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 409096a..aafbfa1 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1011,6 +1011,10 @@ static void svm_ctxt_switch_from(struct vcpu *v)
> svm_tsc_ratio_save(v);
>
> svm_sync_vmcb(v);
> +
> + if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_from )
> + v->domain->arch.hvm_domain.pi_ops.pi_switch_from(v);
> +
> svm_vmload(per_cpu(root_vmcb, cpu));
>
> /* Resume use of ISTs now that the host TR is reinstated. */
> @@ -1050,6 +1054,9 @@ static void svm_ctxt_switch_to(struct vcpu *v)
> svm_lwp_load(v);
> svm_tsc_ratio_load(v);
>
> + if ( v->domain->arch.hvm_domain.pi_ops.pi_switch_to )
> + v->domain->arch.hvm_domain.pi_ops.pi_switch_to(v);
> +
> if ( cpu_has_rdtscp )
> wrmsrl(MSR_TSC_AUX, hvm_msr_tsc_aux(v));
> }
> @@ -1095,6 +1102,9 @@ static void noreturn svm_do_resume(struct vcpu *v)
> vmcb_set_vintr(vmcb, intr);
> }
>
> + if ( v->domain->arch.hvm_domain.pi_ops.pi_do_resume )
> + v->domain->arch.hvm_domain.pi_ops.pi_do_resume(v);
> +
> hvm_do_resume(v);
>
> reset_stack_and_jump(svm_asm_do_resume);
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC
2016-09-19 5:52 ` [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC Suravee Suthikulpanit
@ 2016-10-14 15:44 ` Konrad Rzeszutek Wilk
2016-12-22 11:36 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-14 15:44 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:47AM -0500, Suravee Suthikulpanit wrote:
> Enabling AVIC implicitly disables the V_IRQ, V_INTR_PRIO, V_IGN_TPR,
> and V_INTR_VECTOR fields in the VMCB Control Word. Therefore, this patch
> introduces new interrupt injection code via AVIC backing page.
>
> Also, the AVIC hardware automatically synchronizes TPR and CR8/vTPR, when
> values are updated. Therefore, xen does not need to handle this when enable
> AVIC.
s/this when enable AVIC/when AVIC is enabled/
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/arch/x86/hvm/svm/avic.c | 31 +++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/intr.c | 4 ++++
> xen/arch/x86/hvm/svm/svm.c | 12 ++++++++++--
> xen/arch/x86/hvm/svm/vmcb.c | 6 +++++-
> xen/include/asm-x86/hvm/svm/avic.h | 1 +
> 5 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> index cd8a9d4..4144223 100644
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -576,3 +576,34 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
>
> return;
> }
> +
> +/***************************************************************
Twinkle twinkle little stars, there are too many of you..
> + * AVIC INTR INJECTION
Also the comment could be deleted as it explains pretty well the flow.
> + */
> +void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec)
> +{
> + struct vlapic *vlapic = vcpu_vlapic(v);
> +
> + /* Fallback to use non-AVIC if vcpu is not enabled with AVIC */
Please add an period at the end.
> + if ( !svm_avic_vcpu_enabled(v) )
> + {
> + if ( !vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
> + vcpu_kick(v);
> + return;
> + }
> +
> + if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
> + return;
> +
> + if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
> + return;
> +
> + /*
> + * If vcpu is running on another cpu, hit the doorbell to signal
> + * it to process interrupt. Otherwise, kick it.
> + */
> + if ( v->is_running && (v != current) )
> + wrmsrl(AVIC_DOORBELL, cpu_data[v->processor].apicid);
What is the consequence if say the destination CPU ends up switching to
a different guest - and the doorball hits at that point?
If the different guest is not AVIC enabled .. what then? The CPU ignores
it? Say the CPU is running at that point without VMCB (it is running
dom0), or with an HVM guest without AVIC? Will we get AVIC IPI delievery
not completed #VMEXIT on the destination CPU? (or on this one?)
And what if this new guest is AVIC enabled and there are no IRR in the
backing page? [I presume nothing will happen]
> + else
> + vcpu_kick(v);
> +}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions
2016-09-19 5:52 ` [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions Suravee Suthikulpanit
@ 2016-10-14 15:46 ` Konrad Rzeszutek Wilk
2016-12-22 11:38 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-10-14 15:46 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Sep 19, 2016 at 12:52:48AM -0500, Suravee Suthikulpanit wrote:
> Hook up virtual_intr_delivery_enabled and deliver_posted_intr functions
> when AVIC is enabled.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> xen/arch/x86/hvm/svm/svm.c | 10 ++++++++++
> xen/include/asm-x86/hvm/svm/avic.h | 5 +++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index caf9984..a9c09a7 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1495,6 +1495,16 @@ const struct hvm_function_table * __init start_svm(void)
> svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
> ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>
> + if ( !cpu_has_svm_avic )
> + svm_avic = 0;
> +
> + if ( svm_avic )
> + {
> + svm_function_table.deliver_posted_intr = svm_avic_deliver_posted_intr,
> + svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled,
> + printk("SVM: AVIC enabled\n");
Is this needed? Doesn't the P(..) macro already give you that
information?
Or you could remove the P(..) call for AVIC and make this a bit more
custom where you print something like "- SVM AVIC (detected and %s)'
where %s is either disabled or enabled ?
> + }
> +
> return &svm_function_table;
> }
>
> diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
> index e1eb66c..8411854 100644
> --- a/xen/include/asm-x86/hvm/svm/avic.h
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -41,4 +41,9 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
> void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
>
> void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vector);
> +
> +static inline int svm_avic_enabled(void)
> +{
> + return svm_avic;
> +}
> #endif /* _SVM_AVIC_H_ */
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC
2016-10-14 15:31 ` Konrad Rzeszutek Wilk
@ 2016-10-24 11:19 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-10-24 11:19 UTC (permalink / raw)
To: Suravee Suthikulpanit, Konrad Rzeszutek Wilk
Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 14.10.16 at 17:31, <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 19, 2016 at 12:52:46AM -0500, Suravee Suthikulpanit wrote:
>> + * APIC ID > 0xff is reserved.
>> + */
>> + h_phy_apic_id = cpu_data[v->processor].apicid;
>> + if ( h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX )
>
> What does that mean to the guest? I presume it means it will always
> get an VMEXIT b/c the is_running is not set?
>
> Meaning whatever guest ends up unhappily on an physical CPU with
> the APIC ID of 255 is screwed :-(?
Such a CPU would be unusable - 0xFF is the "broadcast" destination
specifier.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
@ 2016-11-17 16:05 ` Suravee Suthikulpanit
2016-11-17 17:18 ` Konrad Rzeszutek Wilk
2016-11-17 16:55 ` Suravee Suthikulpanit
1 sibling, 1 reply; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-11-17 16:05 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, boris.ostrovsky
Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
Konrad,
Thanks for the review comments. Got one quick question below.
On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote:
>> +int svm_avic_init_vmcb(struct vcpu *v)
>> > +{
>> > + paddr_t ma;
>> > + u32 apic_id_reg;
>> > + struct arch_svm_struct *s = &v->arch.hvm_svm;
>> > + struct vmcb_struct *vmcb = s->vmcb;
>> > + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
>> > + struct svm_avic_phy_ait_entry entry;
>> > +
>> > + if ( !svm_avic )
>> > + return 0;
>> > +
>> > + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
>> > + ma = d->avic_log_ait_mfn;
>> > + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
>> > + ma = d->avic_phy_ait_mfn;
>> > + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
>> > + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
>> > +
>> > + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
> I think you can drop the 'SVM:' part. The __func__ gives enough details.
>
>> > + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
>> > + (unsigned long long) vmcb->avic_log_apic_id,
>> > + (unsigned long long) vmcb->avic_phy_apic_id);
> Is this also part of the keyboard handler? Perhaps that information should
> be presented there?
I'm not sure what you mean by keyboard handler. I assume you mean the
spacing for the indentation the front should align with above line?
Thanks,
S.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
2016-11-17 16:05 ` Suravee Suthikulpanit
@ 2016-11-17 16:55 ` Suravee Suthikulpanit
2016-11-17 17:19 ` Konrad Rzeszutek Wilk
1 sibling, 1 reply; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-11-17 16:55 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, boris.ostrovsky
Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
Konrad,
On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote:
>> +
>> > +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
>> > +#define AVIC_PHY_APIC_ID_MAX 0xFF
>> > +
>> > +#define AVIC_DOORBELL 0xc001011b
>> > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
>> > +#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
>> > +
>> > +bool_t svm_avic = 0;
>> > +boolean_param("svm-avic", svm_avic);
> Why do you want to have it disabled by default?
svm-avic has not yet fully supported with nested virtualization yet. So,
I didn't want to enable this by default. However, when everything is
stable, I am planning to enable this by default.
Thanks,
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-11-17 16:05 ` Suravee Suthikulpanit
@ 2016-11-17 17:18 ` Konrad Rzeszutek Wilk
2016-11-17 18:32 ` Suravee Suthikulpanit
0 siblings, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-17 17:18 UTC (permalink / raw)
To: Suravee Suthikulpanit
Cc: andrew.cooper3, boris.ostrovsky, sherry.hurwitz, jbeulich, xen-devel
On Thu, Nov 17, 2016 at 10:05:58AM -0600, Suravee Suthikulpanit wrote:
> Konrad,
>
> Thanks for the review comments. Got one quick question below.
>
> On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote:
> > > +int svm_avic_init_vmcb(struct vcpu *v)
> > > > +{
> > > > + paddr_t ma;
> > > > + u32 apic_id_reg;
> > > > + struct arch_svm_struct *s = &v->arch.hvm_svm;
> > > > + struct vmcb_struct *vmcb = s->vmcb;
> > > > + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> > > > + struct svm_avic_phy_ait_entry entry;
> > > > +
> > > > + if ( !svm_avic )
> > > > + return 0;
> > > > +
> > > > + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
> > > > + ma = d->avic_log_ait_mfn;
> > > > + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> > > > + ma = d->avic_phy_ait_mfn;
> > > > + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> > > > + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> > > > +
> > > > + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
> > I think you can drop the 'SVM:' part. The __func__ gives enough details.
> >
> > > > + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
> > > > + (unsigned long long) vmcb->avic_log_apic_id,
> > > > + (unsigned long long) vmcb->avic_phy_apic_id);
> > Is this also part of the keyboard handler? Perhaps that information should
> > be presented there?
>
> I'm not sure what you mean by keyboard handler. I assume you mean the
> spacing for the indentation the front should align with above line?
Well I would ditch them. And if you really want them then make the
keyboard handler, aka svm_vmcb_dump function.
>
> Thanks,
> S.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-11-17 16:55 ` Suravee Suthikulpanit
@ 2016-11-17 17:19 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-17 17:19 UTC (permalink / raw)
To: Suravee Suthikulpanit
Cc: andrew.cooper3, boris.ostrovsky, sherry.hurwitz, jbeulich, xen-devel
On Thu, Nov 17, 2016 at 10:55:52AM -0600, Suravee Suthikulpanit wrote:
> Konrad,
>
> On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote:
> > > +
> > > > +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
> > > > +#define AVIC_PHY_APIC_ID_MAX 0xFF
> > > > +
> > > > +#define AVIC_DOORBELL 0xc001011b
> > > > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
> > > > +#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
> > > > +
> > > > +bool_t svm_avic = 0;
> > > > +boolean_param("svm-avic", svm_avic);
> > Why do you want to have it disabled by default?
>
> svm-avic has not yet fully supported with nested virtualization yet. So, I
> didn't want to enable this by default. However, when everything is stable, I
> am planning to enable this by default.
Ah, could you put an comment above the param to explain this?
Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-11-17 17:18 ` Konrad Rzeszutek Wilk
@ 2016-11-17 18:32 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-11-17 18:32 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: andrew.cooper3, boris.ostrovsky, sherry.hurwitz, jbeulich, xen-devel
On 11/17/16 11:18, Konrad Rzeszutek Wilk wrote:
> On Thu, Nov 17, 2016 at 10:05:58AM -0600, Suravee Suthikulpanit wrote:
>> Konrad,
>>
>> Thanks for the review comments. Got one quick question below.
>>
>> On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote:
>>>> +int svm_avic_init_vmcb(struct vcpu *v)
>>>>> +{
>>>>> + paddr_t ma;
>>>>> + u32 apic_id_reg;
>>>>> + struct arch_svm_struct *s = &v->arch.hvm_svm;
>>>>> + struct vmcb_struct *vmcb = s->vmcb;
>>>>> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
>>>>> + struct svm_avic_phy_ait_entry entry;
>>>>> +
>>>>> + if ( !svm_avic )
>>>>> + return 0;
>>>>> +
>>>>> + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
>>>>> + ma = d->avic_log_ait_mfn;
>>>>> + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
>>>>> + ma = d->avic_phy_ait_mfn;
>>>>> + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
>>>>> + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
>>>>> +
>>>>> + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
>>> I think you can drop the 'SVM:' part. The __func__ gives enough details.
>>>
>>>>> + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
>>>>> + (unsigned long long) vmcb->avic_log_apic_id,
>>>>> + (unsigned long long) vmcb->avic_phy_apic_id);
>>> Is this also part of the keyboard handler? Perhaps that information should
>>> be presented there?
>>
>> I'm not sure what you mean by keyboard handler. I assume you mean the
>> spacing for the indentation the front should align with above line?
>
> Well I would ditch them. And if you really want them then make the
> keyboard handler, aka svm_vmcb_dump function.
>>
>> Thanks,
>> S.
Ahh.. the xl debug-keys stuff. Got it. No problem. I can ditch this.
S
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-09-20 14:34 ` Boris Ostrovsky
@ 2016-12-04 7:40 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-12-04 7:40 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: andrew.cooper3, jbeulich, sherry.hurwitz
Boris,
On 9/20/16 21:34, Boris Ostrovsky wrote:
>> BENCHMARK 1: HACKBENCH
>> > ======================
>> >
>> > For measuring IPI performance used for scheduling workload, I have collected
>> > some performance number on 2 and 3 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 (sec) | 3 vcpus (sec)
>> > --------------------------------------------------------
>> > No AVIC w/o evtchn | 299.57 | 337.779
>> > No AVIC w/ evtchn | 270.37 | 419.6064
>> > AVIC w/ evtchn | 181.46 | 171.7957
>> > AVIC w/o evtchn | 171.81 | 169.0858
>> >
>> > Note: In "w/o evtchn" case, the Linux guest is built w/o
>> > Xen guest support.
> Enlightened Linux tries to avoid using event channels for APIC accesses
> if XEN_HVM_CPUID_APIC_ACCESS_VIRT or XEN_HVM_CPUID_X2APIC_VIRT is set.
>
> I didn't notice either of these two bits set in the series. Should they
> be (probably the first one)? Or is this something you are planning for
> the second part?
>
> -boris
Thanks for pointing this out. The XEN_HVM_CPUID_APIC_ACCESS_VIRT would
likely be needed. Since it is related to MSI, this will be covered in
the second part of this patch series.
Thanks,
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers
2016-10-14 15:20 ` Konrad Rzeszutek Wilk
@ 2016-12-12 10:34 ` Suravee Suthikulpanit
2017-01-07 1:24 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-12-12 10:34 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
Hi Konrad,
Thanks for review comments.
On 10/14/2016 10:20 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 19, 2016 at 12:52:45AM -0500, Suravee Suthikulpanit wrote:
>> AVIC introduces two #vmexit handlers:
>>
>> + * 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.
>> + */
>> + vlapic_reg_write(v, APIC_ICR2, icrh);
>> + vlapic_reg_write(v, APIC_ICR, icrl);
>> + break;
>> + case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
>> + {
>> + /*
>> + * 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.
>
> Is there a pattern to this? Meaning does the CPU go sequentially
> through the logical APIC ids - which (if say the guest is using
> logical APIC ids which gives you pretty much 1-1 to physical) - means
> that this VMEXIT would occur in a sequence of the vCPUs that are not
> running?
Not sure if I follow this part. Typically, the current vcpu (the one
that handles this #vmexit) is the one initiating IPI. Here we check the
destination and try to kick each one of the matching destination.
> Because if so, then ..
>> +
>> + for_each_vcpu ( d, c )
>
> .. you could optimize this a bit and start at vCPU+1 (since you know
> that this current vCPU most certainyl got the vCPU) ?
>
But the current (running) vcpu is not necessary the d->vcpu[0]. Do you
mean checking if "c == current" in this loop and skip the current vcpu?
>> + break;
>> + case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
>> + dprintk(XENLOG_ERR,
>> + "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
>> + __func__, icrh, icrl, index);
>
> That should never happen right? If it does that means we messed up the
> allocation somehow. If so, should we disable AVIC for this guest
> completly and log this error?
I think it depends on when this happens. It might be hard to determine
the state of all VCPUs at that point. Shouldn't we just crash the domain
(probably for AVIC_INCMP_IPI_ERR_INV_TARGET and default case as well)?
>> [... in arch/x86/hvm/svm/avic.c: avic_handle_dfr_update()]
>> + u32 mod = (dfr >> 28) & 0xf;
>> +
>> + /*
>> + * We assume that all local APICs are using the same type.
>> + * If this changes, we need to flush the AVIC logical
>> + * APID id table.
>> + */
>> + if ( d->ldr_mode == mod )
>> + return 0;
>> +
>> + clear_domain_page(_mfn(d->avic_log_ait_mfn));
>
> Whoa. What if another vCPU is using this (aka handling another AVIC
> access?)?
Generally, at least in Linux case, I can see that the kernel switch APIC
routing from cluster to flat early in the boot process prior to setting
LDR and bringing up the rest of the AP cores). Do you know of any cases
where the guest OS could be switching the APIC routing mode while the AP
cores are running?
> I see that that the 'V' bit would be cleared so the CPU
> would trap .. (thought that depends right - the clear_domain_page is
> being done in a sequence so some of the later entries could be valid
> while the CPU is clearing it).
Which "V" bit are you referring to here? And when is it cleared?
> Perhaps you should iterate over all the 'V' bit first (to clear them)
> and then clear the page using the clear_domain_page?
> Or better - turn of AVIC first (for the guest), until the page has been cleared?
What about adding a check to see if DFR is changed after LDR has already
been setup and throw some error or warning message for now?
>> [... in arch/x86/hvm/svm/avic.c: svm_avic_vmexit_do_noaccel()]
>> + BUG();
>> + avic_unaccel_trap_write(v);
>
> Say the values are all messed up (the guest provides the wrong
> APIC ID).. Shouldn't we inject some type of exception in the guest?
> (Like the hardware would do? Actually - would the hardware send any type
> of interrupt if one messes up with the APIC? Or is it that it sets an
> error bit in the APIC?)
IIUC, typically, APIC generates APIC error interrupts and set the APIC
Error Status Register (ESR) when detect errors during interrupt
handling. However, if the APIC registers are not programmed correctly, I
think the system would just fail to boot and go into undefined state.
>> [...]
>> + if ( !rw )
>> + *avic_get_bk_page_entry(v, offset) = vlapic_read_aligned(
>> + vcpu_vlapic(v), offset);
>> +
>> + hvm_mem_access_emulate_one(EMUL_KIND_NORMAL, TRAP_invalid_op,
>> + HVM_DELIVER_NO_ERROR_CODE);
>
> So we update the backing page .. and then inject an invalid_op in the
> guest? Is that how hardware does it?
Looking into the hvm_mem_access_emulate_one(), my understanding is that
this emulates the mem access instruction (e.g. to read/write APIC
register), and inject TRAP_invalid_op when the emulation fails (i.e.
X86EMUL_UNHANDLEABLE). Am I missing something here?
Thanks,
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy
2016-09-19 5:52 ` [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy Suravee Suthikulpanit
2016-10-12 19:02 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:09 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:09 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> Since vlapic_init() is called before vcpu_initialise().
> We should also follow the same order here.
s/same/inverse/?
Also the ordering issue extends to other calls, and I think if at all
possible we should then do all the teardown in reverse order of
init.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support
2016-09-19 5:52 ` [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support Suravee Suthikulpanit
2016-10-12 19:07 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:11 ` Jan Beulich
2016-12-26 5:55 ` Suravee Suthikulpanit
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:11 UTC (permalink / raw)
To: Suravee Suthikulpanit, xen-devel; +Cc: andrew.cooper3, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -328,14 +328,15 @@ typedef union __packed
> struct
> {
> u64 tpr: 8;
> - u64 irq: 1;
> + u64 irq: 1; /* disabled for avic */
> u64 rsvd0: 7;
> - u64 prio: 4;
> - u64 ign_tpr: 1;
> + u64 prio: 4; /* disabled for avic */
> + u64 ign_tpr: 1; /* disabled for avic */
> u64 rsvd1: 3;
> u64 intr_masking: 1;
> - u64 rsvd2: 7;
> - u64 vector: 8;
> + u64 rsvd2: 6;
> + u64 avic_enable: 1;
> + u64 vector: 8; /* disabled for avic */
For all of these, do you perhaps mean "ignored" instead of
"disabled"?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
2016-10-14 14:03 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:16 ` Jan Beulich
2016-12-28 3:36 ` Suravee Suthikulpanit
2 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:16 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> +int svm_avic_init_vcpu(struct vcpu *v)
> +{
> + struct vlapic *vlapic = vcpu_vlapic(v);
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( svm_avic )
> + s->avic_bk_pg = vlapic->regs_page;
Why this copying? Can't consuming code not use vlapic->regs_page?
> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> + paddr_t ma;
> + u32 apic_id_reg;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
> + ma = d->avic_log_ait_mfn;
> + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + ma = d->avic_phy_ait_mfn;
> + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> +
> + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
> + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
> + (unsigned long long) vmcb->avic_log_apic_id,
> + (unsigned long long) vmcb->avic_phy_apic_id);
> +
> +
> + apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
> + s->avic_phy_id_cache = avic_get_phy_ait_entry(v, apic_id_reg >> 24);
> + if ( !s->avic_phy_id_cache )
> + return -EINVAL;
> +
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa >> 12) & 0xffffffffff;
Please avoid such open coded constants.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers
2016-09-19 5:52 ` [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers Suravee Suthikulpanit
2016-10-14 15:20 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:25 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:25 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
> +{
> + struct vcpu *v = current;
Please name such variables "curr", which at once avoids the need for
the unusual name ...
> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> + u32 icrh = vmcb->exitinfo1 >> 32;
> + u32 icrl = vmcb->exitinfo1;
> + u32 id = vmcb->exitinfo2 >> 32;
> + u32 index = vmcb->exitinfo2 && 0xFF;
> +
> + dprintk(XENLOG_DEBUG, "SVM: %s: cpu=%#x, vcpu=%#x, "
> + "icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
> + __func__, v->processor, v->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.
> + */
> + vlapic_reg_write(v, APIC_ICR2, icrh);
> + vlapic_reg_write(v, APIC_ICR, icrl);
> + break;
> + case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
> + {
> + /*
> + * 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.
> + */
> + struct vcpu *c;
here.
> + struct domain *d = v->domain;
(This would then become currd.)
> +/***************************************************************
> + * AVIC NOACCEL VMEXIT
> + */
> +#define GET_APIC_LOGICAL_ID(x) (((x) >> 24) & 0xFFu)
GET_xAPIC_ID()?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC
2016-09-19 5:52 ` [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC Suravee Suthikulpanit
2016-10-14 15:31 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:32 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:32 UTC (permalink / raw)
To: Suravee Suthikulpanit, xen-devel; +Cc: andrew.cooper3, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -45,6 +45,83 @@ avic_get_phy_ait_entry(struct vcpu *v, int index)
> }
>
> /***************************************************************
> + * AVIC VCPU SCHEDULING
> + */
> +static void avic_vcpu_load(struct vcpu *v)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + int h_phy_apic_id;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic || !s->avic_phy_id_cache )
Instead of adding checks of svm_avic inside the functions, please don't
install them in the first place when !svm_avic.
> + return;
> +
> + if ( test_bit(_VPF_blocked, &v->pause_flags) )
> + return;
> +
> + /* Note: APIC ID = 0xff is used for broadcast.
> + * APIC ID > 0xff is reserved.
> + */
> + h_phy_apic_id = cpu_data[v->processor].apicid;
> + if ( h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX )
> + return;
Wouldn't such better be an ASSERT(), if at all? Without x2APIC
support I can't see how such a processor could be in use.
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.host_phy_apic_id = h_phy_apic_id;
> + entry.is_running = 1;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +}
> +
> +static void avic_vcpu_put(struct vcpu *v)
May I recommend giving this and its counterpart a consistent pair of
names (put not really being the opposite of load)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC
2016-09-19 5:52 ` [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC Suravee Suthikulpanit
2016-10-14 15:44 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:36 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:36 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> Also, the AVIC hardware automatically synchronizes TPR and CR8/vTPR, when
> values are updated. Therefore, xen does not need to handle this when enable
> AVIC.
I'm having trouble matching this up with ...
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -93,10 +93,14 @@ static int construct_vmcb(struct vcpu *v)
> vmcb->_dr_intercepts = ~0u;
>
> /* Intercept all control-register accesses except for CR2 and CR8. */
> - vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
> + if ( !svm_avic_vcpu_enabled(v) )
> + vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
> CR_INTERCEPT_CR2_WRITE |
> CR_INTERCEPT_CR8_READ |
> CR_INTERCEPT_CR8_WRITE);
> + else
> + vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
> + CR_INTERCEPT_CR2_WRITE );
... this change, enabling CR8 intercepts in AVIC mode.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions
2016-09-19 5:52 ` [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions Suravee Suthikulpanit
2016-10-14 15:46 ` Konrad Rzeszutek Wilk
@ 2016-12-22 11:38 ` Jan Beulich
1 sibling, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:38 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1495,6 +1495,16 @@ const struct hvm_function_table * __init start_svm(void)
> svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
> ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>
> + if ( !cpu_has_svm_avic )
> + svm_avic = 0;
> +
> + if ( svm_avic )
> + {
> + svm_function_table.deliver_posted_intr = svm_avic_deliver_posted_intr,
> + svm_function_table.virtual_intr_delivery_enabled = svm_avic_enabled,
> + printk("SVM: AVIC enabled\n");
> + }
> +
> return &svm_function_table;
> }
>
> --- a/xen/include/asm-x86/hvm/svm/avic.h
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -41,4 +41,9 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs);
> void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs);
>
> void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vector);
> +
> +static inline int svm_avic_enabled(void)
> +{
> + return svm_avic;
> +}
Why is this an inline function (and in a header) when its only use is
for setting a function pointer?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
` (10 preceding siblings ...)
2016-09-20 14:34 ` Boris Ostrovsky
@ 2016-12-22 11:38 ` Jan Beulich
2016-12-28 6:30 ` Suravee Suthikulpanit
11 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2016-12-22 11:38 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
> Suravee Suthikulpanit (9):
> x86/HVM: Introduce struct hvm_pi_ops
> x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as
> non-static
> x86/HVM: Call vlapic_destroy after vcpu_destroy
> x86/SVM: Modify VMCB fields to add AVIC support
> x86/HVM/SVM: Add AVIC initialization code
> x86/SVM: Add AVIC vmexit handlers
> x86/SVM: Add vcpu scheduling support for AVIC
> x86/SVM: Add interrupt management code via AVIC
> x86/SVM: Hook up miscellaneous AVIC functions
Apart from the small comments on the individual patches I've just
given, there are a number of cosmetic issues which will need dealing
with, like coding style, use of plain int when unsigned int is meant,
use of bool_t (or even plain int) when elsewhere you already properly
use bool, and perhaps some constification.
And I'm sorry for it having taken so long to get to look over this
series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support
2016-12-22 11:11 ` Jan Beulich
@ 2016-12-26 5:55 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-12-26 5:55 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: andrew.cooper3, sherry.hurwitz
On 12/22/16 18:11, Jan Beulich wrote:
>>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
>> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
>> @@ -328,14 +328,15 @@ typedef union __packed
>> struct
>> {
>> u64 tpr: 8;
>> - u64 irq: 1;
>> + u64 irq: 1; /* disabled for avic */
>> u64 rsvd0: 7;
>> - u64 prio: 4;
>> - u64 ign_tpr: 1;
>> + u64 prio: 4; /* disabled for avic */
>> + u64 ign_tpr: 1; /* disabled for avic */
>> u64 rsvd1: 3;
>> u64 intr_masking: 1;
>> - u64 rsvd2: 7;
>> - u64 vector: 8;
>> + u64 rsvd2: 6;
>> + u64 avic_enable: 1;
>> + u64 vector: 8; /* disabled for avic */
>
> For all of these, do you perhaps mean "ignored" instead of
> "disabled"?
>
> Jan
>
Yes, I'm fixing the comment here in my next patch.
S
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
2016-12-22 11:16 ` Jan Beulich
@ 2016-12-28 3:36 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-12-28 3:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
On 12/22/16 18:16, Jan Beulich wrote:
>>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
>> +int svm_avic_init_vcpu(struct vcpu *v)
>> +{
>> + struct vlapic *vlapic = vcpu_vlapic(v);
>> + struct arch_svm_struct *s = &v->arch.hvm_svm;
>> +
>> + if ( svm_avic )
>> + s->avic_bk_pg = vlapic->regs_page;
> Why this copying? Can't consuming code not use vlapic->regs_page?
>
Hm.. good point. I'll get rid of avic_bk_pg and just use the regs_page.
Thanks,
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 0/9] Introduce AMD SVM AVIC
2016-12-22 11:38 ` Jan Beulich
@ 2016-12-28 6:30 ` Suravee Suthikulpanit
0 siblings, 0 replies; 43+ messages in thread
From: Suravee Suthikulpanit @ 2016-12-28 6:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, sherry.hurwitz
On 12/22/16 18:38, Jan Beulich wrote:
>>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@amd.com> wrote:
>> Suravee Suthikulpanit (9):
>> x86/HVM: Introduce struct hvm_pi_ops
>> x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as
>> non-static
>> x86/HVM: Call vlapic_destroy after vcpu_destroy
>> x86/SVM: Modify VMCB fields to add AVIC support
>> x86/HVM/SVM: Add AVIC initialization code
>> x86/SVM: Add AVIC vmexit handlers
>> x86/SVM: Add vcpu scheduling support for AVIC
>> x86/SVM: Add interrupt management code via AVIC
>> x86/SVM: Hook up miscellaneous AVIC functions
>
> Apart from the small comments on the individual patches I've just
> given, there are a number of cosmetic issues which will need dealing
> with, like coding style, use of plain int when unsigned int is meant,
> use of bool_t (or even plain int) when elsewhere you already properly
> use bool, and perhaps some constification.
>
> And I'm sorry for it having taken so long to get to look over this
> series.
>
> Jan
>
Thanks for your review comment. I know you are busy. Your time is
greatly appreciated. Konrad has also pointed out several styling issue
which I have been making changes.
I'll test and send out the V2.
S
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers
2016-12-12 10:34 ` Suravee Suthikulpanit
@ 2017-01-07 1:24 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-01-07 1:24 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: andrew.cooper3, sherry.hurwitz, jbeulich, xen-devel
On Mon, Dec 12, 2016 at 05:34:29PM +0700, Suravee Suthikulpanit wrote:
> Hi Konrad,
>
> Thanks for review comments.
>
> On 10/14/2016 10:20 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Sep 19, 2016 at 12:52:45AM -0500, Suravee Suthikulpanit wrote:
> > > AVIC introduces two #vmexit handlers:
> > >
> > > + * 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.
> > > + */
> > > + vlapic_reg_write(v, APIC_ICR2, icrh);
> > > + vlapic_reg_write(v, APIC_ICR, icrl);
> > > + break;
> > > + case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
> > > + {
> > > + /*
> > > + * 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.
> >
> > Is there a pattern to this? Meaning does the CPU go sequentially
> > through the logical APIC ids - which (if say the guest is using
> > logical APIC ids which gives you pretty much 1-1 to physical) - means
> > that this VMEXIT would occur in a sequence of the vCPUs that are not
> > running?
>
> Not sure if I follow this part. Typically, the current vcpu (the one that
> handles this #vmexit) is the one initiating IPI. Here we check the
> destination and try to kick each one of the matching destination.
>
> > Because if so, then ..
> > > +
> > > + for_each_vcpu ( d, c )
> >
> > .. you could optimize this a bit and start at vCPU+1 (since you know
> > that this current vCPU most certainyl got the vCPU) ?
> >
>
> But the current (running) vcpu is not necessary the d->vcpu[0]. Do you mean
> checking if "c == current" in this loop and skip the current vcpu?
Yes.
>
> > > + break;
> > > + case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
> > > + dprintk(XENLOG_ERR,
> > > + "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> > > + __func__, icrh, icrl, index);
> >
> > That should never happen right? If it does that means we messed up the
> > allocation somehow. If so, should we disable AVIC for this guest
> > completly and log this error?
>
> I think it depends on when this happens. It might be hard to determine the
> state of all VCPUs at that point. Shouldn't we just crash the domain
> (probably for AVIC_INCMP_IPI_ERR_INV_TARGET and default case as well)?
That would be much better
>
> > > [... in arch/x86/hvm/svm/avic.c: avic_handle_dfr_update()]
> > > + u32 mod = (dfr >> 28) & 0xf;
> > > +
> > > + /*
> > > + * We assume that all local APICs are using the same type.
> > > + * If this changes, we need to flush the AVIC logical
> > > + * APID id table.
> > > + */
> > > + if ( d->ldr_mode == mod )
> > > + return 0;
> > > +
> > > + clear_domain_page(_mfn(d->avic_log_ait_mfn));
> >
> > Whoa. What if another vCPU is using this (aka handling another AVIC
> > access?)?
>
> Generally, at least in Linux case, I can see that the kernel switch APIC
> routing from cluster to flat early in the boot process prior to setting LDR
> and bringing up the rest of the AP cores). Do you know of any cases where
> the guest OS could be switching the APIC routing mode while the AP cores are
> running?
Put yourself in the shoes of an attacker. IF there is some way we can
screw up the hypervisor the better. Not sure if clearing this page
would lead us in the hypervisor to crash or such.
>
> > I see that that the 'V' bit would be cleared so the CPU
> > would trap .. (thought that depends right - the clear_domain_page is
> > being done in a sequence so some of the later entries could be valid
> > while the CPU is clearing it).
>
> Which "V" bit are you referring to here? And when is it cleared?
I sadly don't remember :-(
>
> > Perhaps you should iterate over all the 'V' bit first (to clear them)
> > and then clear the page using the clear_domain_page?
> > Or better - turn of AVIC first (for the guest), until the page has been cleared?
>
> What about adding a check to see if DFR is changed after LDR has already
> been setup and throw some error or warning message for now?
Sure.
>
> > > [... in arch/x86/hvm/svm/avic.c: svm_avic_vmexit_do_noaccel()]
> > > + BUG();
> > > + avic_unaccel_trap_write(v);
> >
> > Say the values are all messed up (the guest provides the wrong
> > APIC ID).. Shouldn't we inject some type of exception in the guest?
> > (Like the hardware would do? Actually - would the hardware send any type
> > of interrupt if one messes up with the APIC? Or is it that it sets an
> > error bit in the APIC?)
>
> IIUC, typically, APIC generates APIC error interrupts and set the APIC Error
> Status Register (ESR) when detect errors during interrupt handling. However,
> if the APIC registers are not programmed correctly, I think the system would
> just fail to boot and go into undefined state.
As long as we don't crash the hypervisor I suppose that is OK.
>
> > > [...]
> > > + if ( !rw )
> > > + *avic_get_bk_page_entry(v, offset) = vlapic_read_aligned(
> > > + vcpu_vlapic(v), offset);
> > > +
> > > + hvm_mem_access_emulate_one(EMUL_KIND_NORMAL, TRAP_invalid_op,
> > > + HVM_DELIVER_NO_ERROR_CODE);
> >
> > So we update the backing page .. and then inject an invalid_op in the
> > guest? Is that how hardware does it?
>
> Looking into the hvm_mem_access_emulate_one(), my understanding is that this
> emulates the mem access instruction (e.g. to read/write APIC register), and
> inject TRAP_invalid_op when the emulation fails (i.e. X86EMUL_UNHANDLEABLE).
> Am I missing something here?
Nope. Just me misremembering it.
>
> Thanks,
> Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2017-01-07 1:24 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-19 5:52 [RFC PATCH 0/9] Introduce AMD SVM AVIC Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 1/9] x86/HVM: Introduce struct hvm_pi_ops Suravee Suthikulpanit
2016-10-12 17:01 ` Konrad Rzeszutek Wilk
2016-09-19 5:52 ` [RFC PATCH 2/9] x86/vLAPIC: Declare vlapic_read_aligned() and vlapic_reg_write() as non-static Suravee Suthikulpanit
2016-10-12 19:00 ` Konrad Rzeszutek Wilk
2016-09-19 5:52 ` [RFC PATCH 3/9] x86/HVM: Call vlapic_destroy after vcpu_destroy Suravee Suthikulpanit
2016-10-12 19:02 ` Konrad Rzeszutek Wilk
2016-12-22 11:09 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 4/9] x86/SVM: Modify VMCB fields to add AVIC support Suravee Suthikulpanit
2016-10-12 19:07 ` Konrad Rzeszutek Wilk
2016-12-22 11:11 ` Jan Beulich
2016-12-26 5:55 ` Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code Suravee Suthikulpanit
2016-10-12 20:02 ` Konrad Rzeszutek Wilk
2016-11-17 16:05 ` Suravee Suthikulpanit
2016-11-17 17:18 ` Konrad Rzeszutek Wilk
2016-11-17 18:32 ` Suravee Suthikulpanit
2016-11-17 16:55 ` Suravee Suthikulpanit
2016-11-17 17:19 ` Konrad Rzeszutek Wilk
2016-10-14 14:03 ` Konrad Rzeszutek Wilk
2016-12-22 11:16 ` Jan Beulich
2016-12-28 3:36 ` Suravee Suthikulpanit
2016-09-19 5:52 ` [RFC PATCH 6/9] x86/SVM: Add AVIC vmexit handlers Suravee Suthikulpanit
2016-10-14 15:20 ` Konrad Rzeszutek Wilk
2016-12-12 10:34 ` Suravee Suthikulpanit
2017-01-07 1:24 ` Konrad Rzeszutek Wilk
2016-12-22 11:25 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC Suravee Suthikulpanit
2016-10-14 15:31 ` Konrad Rzeszutek Wilk
2016-10-24 11:19 ` Jan Beulich
2016-12-22 11:32 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 8/9] x86/SVM: Add interrupt management code via AVIC Suravee Suthikulpanit
2016-10-14 15:44 ` Konrad Rzeszutek Wilk
2016-12-22 11:36 ` Jan Beulich
2016-09-19 5:52 ` [RFC PATCH 9/9] x86/SVM: Hook up miscellaneous AVIC functions Suravee Suthikulpanit
2016-10-14 15:46 ` Konrad Rzeszutek Wilk
2016-12-22 11:38 ` Jan Beulich
2016-09-19 13:09 ` [RFC PATCH 0/9] Introduce AMD SVM AVIC Konrad Rzeszutek Wilk
2016-09-19 16:21 ` Suravee Suthikulpanit
2016-09-20 14:34 ` Boris Ostrovsky
2016-12-04 7:40 ` Suravee Suthikulpanit
2016-12-22 11:38 ` Jan Beulich
2016-12-28 6:30 ` Suravee Suthikulpanit
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.