[PATCH v2 00/10] Introduce AMD SVM AVIC

[PATCH v2 00/10] Introduce AMD SVM AVIC

[Janakarajan Natarajan]

OVERVIEW
========
This patchset is the first of a two-part patch series to introduce
the AMD Advanced Virtual Interrupt Controller (AVIC) support.

The AVIC hardware virtualizes local APIC registers of each vCPU via
the virtual APIC (vAPIC) backing page. This allows the guest to access
certain APIC registers without the need for emulation of hardware
behaviour in the hypervisor. More information about AVIC can be found in

* AMD64 Architecture Programmers Manual Volume 2 - System Programming
  https://support.amd.com/TechDocs/24593.pdf

For SVM AVIC, we extend the existing SVM 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.

This patchset does not enable AVIC by default since it does not
yet support nested VMs. Until then, users can enable SVM AVIC by
specifying Xen parameter svm=avic.

Later, in Part 2, we will introduce the IOMMU AVIC support, which
provides speed-up for the PCI device passthrough use case by allowing
the IOMMU hardware to inject interrupts directly into the guest via
the vAPIC backing page.

OVERALL PERFORMANCE
===================
AVIC is available on AMD Family 15h models 6Xh (Carrizo) processors
and newer. An AMD Family 17h Epyc processor is used to collect the
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 the hypervisor wants to
inject interrupts into a running vcpu. It can do this by setting the 
corresponding IRR bit in the vAPIC backing page and triggering the
AVIC_DOORBELL.

For sending IPI interrupts between running vcpus in a Linux guest,
Xen defaults to using event channels. However, in case of
non-paravirtualized guests, AVIC can also provide performance
improvements for sending IPIs.

BENCHMARK: HACKBENCH
====================
For measuring IPI performance used for scheduling workload, some
performance numbers are collected using hackbench.

    # 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
                          |      3 vcpus (sec)        |
    ---------------------------------------------------
    No AVIC w/ xen_nopv   |           517             |
    AVIC w/ xen_nopv      |           238             |
    No AVIC w/o xen_nopv  |           141             |
    AVIC w/o xen_nopv     |           135             |

Each benchmark test was averaged over 10 runs.

CURRENT UNTESTED USE_CASES
==========================
* Nested VM

Any feedback and comments are very much appreciated.

v1->v2:
* Remove use of MFN 0 as dummy page for AVIC as suggested by
  Jan and Andrew.
* Rename vlapic_read_aligned().
* Moved AVIC stats dump from new keyhandler to IRQ keyhandler.
* Changed avic_logical_id_entry from struct to union.
* Updated cover letter and patch descriptions.
* Miscellaneous fixes based on feedback.

Janakarajan Natarajan (2):
  x86/HVM: Rename vlapic_read_aligned() to vlapic_reg_read()
  x86/HVM: Make vlapic_reg_read/write() non-static

Suravee Suthikulpanit (8):
  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/HVM: Hook up miscellaneous AVIC functions
  x86/SVM: Introduce svm command line option
  x86/SVM: Append AMD AVIC related data to IRQ keyhandler 'i'

 docs/misc/xen-command-line.markdown |  16 +
 xen/arch/x86/hvm/svm/Makefile       |   1 +
 xen/arch/x86/hvm/svm/avic.c         | 614 ++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/intr.c         |   4 +
 xen/arch/x86/hvm/svm/svm.c          |  76 ++++-
 xen/arch/x86/hvm/svm/vmcb.c         |   3 +
 xen/arch/x86/hvm/vlapic.c           |  28 +-
 xen/arch/x86/hvm/vmx/vmx.c          |   8 +-
 xen/arch/x86/irq.c                  |   2 +
 xen/include/asm-x86/hvm/hvm.h       |   4 +-
 xen/include/asm-x86/hvm/svm/avic.h  |  46 +++
 xen/include/asm-x86/hvm/svm/svm.h   |   2 +
 xen/include/asm-x86/hvm/svm/vmcb.h  |  53 +++-
 xen/include/asm-x86/hvm/vlapic.h    |   4 +
 xen/include/asm-x86/msr-index.h     |   1 +
 15 files changed, 828 insertions(+), 34 deletions(-)
 create mode 100644 xen/arch/x86/hvm/svm/avic.c
 create mode 100644 xen/include/asm-x86/hvm/svm/avic.h

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 01/10] x86/SVM: Modify VMCB fields to add AVIC support

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Introduce AVIC-related VMCB fields.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@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 de07429dff..591d98fc8c 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -324,16 +324,17 @@ typedef union
     struct
     {
         u64 tpr:          8;
-        u64 irq:          1;
+        u64 irq:          1; /* ignored in avic mode */
         u64 vgif:         1;
         u64 rsvd0:        6;
-        u64 prio:         4;
-        u64 ign_tpr:      1;
+        u64 prio:         4; /* ignored in avic mode */
+        u64 ign_tpr:      1; /* ignored in avic mode */
         u64 rsvd1:        3;
         u64 intr_masking: 1;
         u64 vgif_enable:  1;
-        u64 rsvd2:        6;
-        u64 vector:       8;
+        u64 rsvd2:        5;
+        u64 avic_enable:  1;
+        u64 vector:       8; /* ignored in avic mode */
         u64 rsvd3:       24;
     } fields;
 } vintr_t;
@@ -393,7 +394,8 @@ typedef union
         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;
 
@@ -427,7 +429,8 @@ struct 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 */
     virt_ext_t virt_ext;        /* offset 0xB8 */
@@ -436,7 +439,11 @@ struct 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_logical_id_table_pa;  /* offset 0xF0 */
+    u64 avic_physical_id_table_pa; /* offset 0xF8 */
+    u64 res10a[96];             /* offset 0x100 pad to save area */
 
     union {
         struct segment_register sreg[6];
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 02/10] x86/HVM: Rename vlapic_read_aligned() to vlapic_reg_read()

[Janakarajan Natarajan]

Rename vlapic_read_aligned() to vlapic_reg_read() to make it a pair of
vlapic_reg_write().

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/vlapic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 1b9f00a0e4..c9b6461cbf 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -592,7 +592,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(const struct vlapic *vlapic,
+static uint32_t vlapic_reg_read(const struct vlapic *vlapic,
                                     unsigned int offset)
 {
     switch ( offset )
@@ -627,7 +627,7 @@ static int vlapic_read(
     if ( offset > (APIC_TDCR + 0x3) )
         goto out;
 
-    tmp = vlapic_read_aligned(vlapic, offset & ~3);
+    tmp = vlapic_reg_read(vlapic, offset & ~3);
 
     switch ( len )
     {
@@ -691,10 +691,10 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
         return X86EMUL_UNHANDLEABLE;
 
     if ( offset == APIC_ICR )
-        high = vlapic_read_aligned(vlapic, APIC_ICR2);
+        high = vlapic_reg_read(vlapic, APIC_ICR2);
 
     *msr_content = ((uint64_t)high << 32) |
-                   vlapic_read_aligned(vlapic, offset);
+                   vlapic_reg_read(vlapic, offset);
 
     return X86EMUL_OKAY;
 }
@@ -926,7 +926,7 @@ static int vlapic_write(struct vcpu *v, unsigned long address,
      */
     if ( unlikely(len != 4) )
     {
-        unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
+        unsigned int tmp = vlapic_reg_read(vlapic, offset & ~3);
         unsigned char alignment = (offset & 3) * 8;
 
         switch ( len )
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 03/10] x86/HVM: Make vlapic_reg_read/write() non-static

[Janakarajan Natarajan]

AMD AVIC code makes use of vlapic_reg_read() and vlapic_reg_write(). To
do this make the functions non-static.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/vlapic.c        | 4 ++--
 xen/include/asm-x86/hvm/vlapic.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index c9b6461cbf..60d1f7e748 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -592,7 +592,7 @@ static void vlapic_set_tdcr(struct vlapic *vlapic, unsigned int val)
                 "timer_divisor: %d", vlapic->hw.timer_divisor);
 }
 
-static uint32_t vlapic_reg_read(const struct vlapic *vlapic,
+uint32_t vlapic_reg_read(const struct vlapic *vlapic,
                                     unsigned int offset)
 {
     switch ( offset )
@@ -788,7 +788,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
     }
 }
 
-static void vlapic_reg_write(struct vcpu *v,
+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 212c36b5c2..692e34ad4c 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -137,6 +137,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_reg_read(const 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);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

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.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/svm/Makefile      |   1 +
 xen/arch/x86/hvm/svm/avic.c        | 190 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c         |   8 +-
 xen/arch/x86/hvm/svm/vmcb.c        |   3 +
 xen/arch/x86/hvm/vlapic.c          |   4 +
 xen/include/asm-x86/hvm/svm/avic.h |  39 ++++++++
 xen/include/asm-x86/hvm/svm/svm.h  |   2 +
 xen/include/asm-x86/hvm/svm/vmcb.h |  18 ++++
 8 files changed, 264 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 760d2954da..e0e4a59f7d 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 0000000000..d6b8638bab
--- /dev/null
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -0,0 +1,190 @@
+/*
+ * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) support
+ * Copyright (c) 2018, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/domain_page.h>
+#include <xen/sched.h>
+#include <xen/stdbool.h>
+#include <asm/acpi.h>
+#include <asm/apicdef.h>
+#include <asm/atomic.h>
+#include <asm/event.h>
+#include <asm/hvm/emulate.h>
+#include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/support.h>
+#include <asm/hvm/svm/avic.h>
+#include <asm/hvm/vlapic.h>
+#include <asm/p2m.h>
+#include <asm/page.h>
+
+/* Note: Current max index allowed for physical APIC ID table is 255. */
+#define AVIC_PHY_APIC_ID_MAX    GET_xAPIC_ID(APIC_ID_MASK)
+
+/*
+ * Note:
+ * Currently, svm-avic mode is not supported with nested virtualization.
+ * Therefore, it is not yet currently enabled by default. Once the support
+ * is in-place, this should be enabled by default.
+ */
+bool svm_avic = false;
+
+static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
+    avic_backing_page[PAGE_SIZE];
+
+static struct avic_physical_id_entry*
+avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)
+{
+    if ( !d->avic_physical_id_table )
+        return NULL;
+
+    /*
+    * Note: APIC ID = 0xFF is used for broadcast.
+    *       APIC ID > 0xFF is reserved.
+    */
+    ASSERT(index < AVIC_PHY_APIC_ID_MAX);
+
+    if ( index >= AVIC_PHY_APIC_ID_MAX )
+        return NULL;
+
+    return &d->avic_physical_id_table[index];
+}
+
+int svm_avic_dom_init(struct domain *d)
+{
+    int ret = 0;
+    struct page_info *pg;
+
+    if ( !svm_avic || !has_vlapic(d) )
+        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 for APIC.
+     */
+    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
+                       _mfn(virt_to_mfn(avic_backing_page)), PAGE_ORDER_4K,
+                       p2m_access_rw);
+
+    /* Init AVIC logical APIC ID table */
+    pg = alloc_domheap_page(d, MEMF_no_owner);
+    if ( !pg )
+    {
+        ret = -ENOMEM;
+        goto err_out;
+    }
+    clear_domain_page(page_to_mfn(pg));
+    d->arch.hvm_domain.svm.avic_logical_id_table_pg = pg;
+    d->arch.hvm_domain.svm.avic_logical_id_table = __map_domain_page_global(pg);
+
+    /* Init AVIC physical APIC ID table */
+    pg = alloc_domheap_page(d, MEMF_no_owner);
+    if ( !pg )
+    {
+        ret = -ENOMEM;
+        goto err_out;
+    }
+    clear_domain_page(page_to_mfn(pg));
+    d->arch.hvm_domain.svm.avic_physical_id_table_pg = pg;
+    d->arch.hvm_domain.svm.avic_physical_id_table = __map_domain_page_global(pg);
+
+    return ret;
+ err_out:
+    svm_avic_dom_destroy(d);
+    return ret;
+}
+
+void svm_avic_dom_destroy(struct domain *d)
+{
+    if ( !svm_avic || !has_vlapic(d) )
+        return;
+
+    if ( d->arch.hvm_domain.svm.avic_physical_id_table )
+    {
+        unmap_domain_page_global(d->arch.hvm_domain.svm.avic_physical_id_table);
+        free_domheap_page(d->arch.hvm_domain.svm.avic_physical_id_table_pg);
+        d->arch.hvm_domain.svm.avic_physical_id_table_pg = NULL;
+        d->arch.hvm_domain.svm.avic_physical_id_table = NULL;
+    }
+
+    if ( d->arch.hvm_domain.svm.avic_logical_id_table)
+    {
+        unmap_domain_page_global(d->arch.hvm_domain.svm.avic_logical_id_table);
+        free_domheap_page(d->arch.hvm_domain.svm.avic_logical_id_table_pg);
+        d->arch.hvm_domain.svm.avic_logical_id_table_pg = NULL;
+        d->arch.hvm_domain.svm.avic_logical_id_table = NULL;
+    }
+}
+
+bool svm_avic_vcpu_enabled(const struct vcpu *v)
+{
+    const struct arch_svm_struct *s = &v->arch.hvm_svm;
+    const struct vmcb_struct *vmcb = s->vmcb;
+
+    return vmcb->_vintr.fields.avic_enable;
+}
+
+int svm_avic_init_vmcb(struct vcpu *v)
+{
+    u32 apic_id;
+    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;
+    const struct vlapic *vlapic = vcpu_vlapic(v);
+    struct avic_physical_id_entry *entry;
+
+    if ( !svm_avic || !has_vlapic(v->domain) )
+        return 0;
+
+    if ( !vlapic || !vlapic->regs_page )
+        return -EINVAL;
+
+    apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
+    s->avic_last_phy_id = avic_get_physical_id_entry(d, GET_xAPIC_ID(apic_id));
+    if ( !s->avic_last_phy_id )
+        return -EINVAL;
+
+    vmcb->avic_bk_pg_pa = page_to_maddr(vlapic->regs_page);
+    vmcb->avic_logical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_logical_id_table));
+    vmcb->avic_physical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_physical_id_table));
+
+    /* Set Physical ID Table Pointer [7:0] to max apic id of the domain */
+    vmcb->avic_physical_id_table_pa |= (v->domain->max_vcpus * 2) & 0xFF;
+
+    entry = s->avic_last_phy_id;
+    entry->bk_pg_ptr_mfn = (vmcb->avic_bk_pg_pa) >> PAGE_SHIFT;
+    entry->is_running = 0;
+    entry->valid = 1;
+
+    vmcb->avic_vapic_bar = APIC_DEFAULT_PHYS_BASE;
+    vmcb->cleanbits.fields.avic = 0;
+
+    vmcb->_vintr.fields.avic_enable = 1;
+
+    return 0;
+}
+
+/*
+ * 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 c7616456dd..c264353e69 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -47,6 +47,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>
@@ -1244,11 +1245,12 @@ static int svm_domain_initialise(struct domain *d)
 
     d->arch.ctxt_switch = &csw;
 
-    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)
@@ -1713,6 +1715,9 @@ const struct hvm_function_table * __init start_svm(void)
     if ( cpu_has_tsc_ratio )
         svm_function_table.tsc_scaling.ratio_frac_bits = 32;
 
+    if ( !cpu_has_svm_avic )
+        svm_avic = 0;
+
 #define P(p,s) if ( p ) { printk(" - %s\n", s); printed = 1; }
     P(cpu_has_svm_npt, "Nested Page Tables (NPT)");
     P(cpu_has_svm_lbrv, "Last Branch Record (LBR) Virtualisation");
@@ -1724,6 +1729,7 @@ const struct hvm_function_table * __init start_svm(void)
     P(cpu_has_pause_filter, "Pause-Intercept Filter");
     P(cpu_has_pause_thresh, "Pause-Intercept Filter Threshold");
     P(cpu_has_tsc_ratio, "TSC Rate MSR");
+    P(cpu_has_svm_avic, svm_avic ? "AVIC (enabled)" : "AVIC (disabled)");
 #undef P
 
     if ( !printed )
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index ae60d8dc1c..7ade023cfe 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -27,6 +27,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>
 
@@ -215,6 +216,8 @@ static int construct_vmcb(struct vcpu *v)
             vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
     }
 
+    svm_avic_init_vmcb(v);
+
     vmcb->cleanbits.bytes = 0;
 
     return 0;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 60d1f7e748..eb71cc21ed 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1597,6 +1597,10 @@ int vlapic_init(struct vcpu *v)
 
     if (vlapic->regs_page == NULL)
     {
+        /*
+         * SVM AVIC depends on the vlapic->regs_page being a full
+         * page allocation as it is also used for vAPIC backing page.
+         */
         vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
         if ( vlapic->regs_page == NULL )
         {
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 0000000000..0c01a40ff5
--- /dev/null
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -0,0 +1,39 @@
+#ifndef _SVM_AVIC_H_
+#define _SVM_AVIC_H_
+
+#include <xen/compiler.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,
+};
+
+typedef union avic_logical_id_entry {
+    u32 raw;
+    struct __packed {
+        u32 guest_phy_apic_id : 8;
+        u32 res               : 23;
+        u32 valid             : 1;
+    };
+} avic_logical_id_entry_t;
+
+struct __packed avic_physical_id_entry {
+        u64 host_phy_apic_id  : 8;
+        u64 res1              : 4;
+        u64 bk_pg_ptr_mfn     : 40;
+        u64 res2              : 10;
+        u64 is_running        : 1;
+        u64 valid             : 1;
+};
+
+extern bool svm_avic;
+
+int svm_avic_dom_init(struct domain *d);
+void svm_avic_dom_destroy(struct domain *d);
+
+bool svm_avic_vcpu_enabled(const struct vcpu *v);
+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 4e5e142910..983750fee9 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -65,6 +65,7 @@ extern u32 svm_feature_flags;
 #define SVM_FEATURE_DECODEASSISTS  7 /* Decode assists support */
 #define SVM_FEATURE_PAUSEFILTER   10 /* Pause intercept filter support */
 #define SVM_FEATURE_PAUSETHRESH   12 /* Pause intercept filter support */
+#define SVM_FEATURE_AVIC          13 /* AVIC Support */
 #define SVM_FEATURE_VLOADSAVE     15 /* virtual vmload/vmsave */
 #define SVM_FEATURE_VGIF          16 /* Virtual GIF */
 
@@ -79,6 +80,7 @@ extern u32 svm_feature_flags;
 #define cpu_has_pause_filter  cpu_has_svm_feature(SVM_FEATURE_PAUSEFILTER)
 #define cpu_has_pause_thresh  cpu_has_svm_feature(SVM_FEATURE_PAUSETHRESH)
 #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 cpu_has_svm_vloadsave cpu_has_svm_feature(SVM_FEATURE_VLOADSAVE)
 
 #define SVM_PAUSEFILTER_INIT    4000
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 591d98fc8c..e625884c8b 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -21,6 +21,7 @@
 
 #include <xen/types.h>
 #include <asm/hvm/emulate.h>
+#include <asm/hvm/svm/avic.h>
 
 
 /* general 1 intercepts */
@@ -500,6 +501,21 @@ struct vmcb_struct {
 };
 
 struct svm_domain {
+    /*
+     * This per-domain table is used by the hardware to locate
+     * the vAPIC backing page to be used to deliver interrupts
+     * based on the guest physical APIC ID.
+     */
+    struct avic_physical_id_entry *avic_physical_id_table;
+    struct page_info *avic_physical_id_table_pg;
+
+    /*
+     * This per-domain table is used by the hardware to map
+     * logically addressed interrupt requests (w/ guest logical APIC id)
+     * to the guest physical APIC ID.
+     */
+    avic_logical_id_entry_t *avic_logical_id_table;
+    struct page_info *avic_logical_id_table_pg;
 };
 
 struct arch_svm_struct {
@@ -533,6 +549,8 @@ struct arch_svm_struct {
         u64 length;
         u64 status;
     } osvw;
+
+    struct avic_physical_id_entry *avic_last_phy_id;
 };
 
 struct vmcb_struct *alloc_vmcb(void);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

AVIC introduces two new #vmexit handlers:

VMEXIT_INCOMP_IPI:
This occurs when an IPI could not be delivered to all targeted guest
virtual processors because at least one guest virtual processor
was not allocated to a physical core at the time. In this case,
Xen would try to emulate IPI.

VMEXIT_DO_NOACCEL:
This occurs when a guest access to an APIC register that cannot be
accelerated by AVIC. In this case, Xen tries to emulate register accesses.

This fault is also generated if an EOI is attempted when the highest priority
in-service interrupt is set for level-triggered mode.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/svm/avic.c        | 292 +++++++++++++++++++++++++++++++++++++
 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 |   6 +
 4 files changed, 309 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index d6b8638bab..2fba35fe47 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -19,6 +19,7 @@
 #include <xen/sched.h>
 #include <xen/stdbool.h>
 #include <asm/acpi.h>
+#include <asm/apic.h>
 #include <asm/apicdef.h>
 #include <asm/atomic.h>
 #include <asm/event.h>
@@ -33,6 +34,8 @@
 /* Note: Current max index allowed for physical APIC ID table is 255. */
 #define AVIC_PHY_APIC_ID_MAX    GET_xAPIC_ID(APIC_ID_MASK)
 
+#define AVIC_UNACCEL_ACCESS_OFFSET_MASK    0xFF0
+
 /*
  * Note:
  * Currently, svm-avic mode is not supported with nested virtualization.
@@ -103,6 +106,8 @@ int svm_avic_dom_init(struct domain *d)
     d->arch.hvm_domain.svm.avic_physical_id_table_pg = pg;
     d->arch.hvm_domain.svm.avic_physical_id_table = __map_domain_page_global(pg);
 
+    spin_lock_init(&d->arch.hvm_domain.svm.avic_dfr_mode_lock);
+
     return ret;
  err_out:
     svm_avic_dom_destroy(d);
@@ -180,6 +185,293 @@ int svm_avic_init_vmcb(struct vcpu *v)
 }
 
 /*
+ * Note:
+ * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
+ * The hardware generates this fault when an IPI could not be delivered
+ * to all targeted guest virtual processors because at least one guest
+ * virtual processor was not allocated to a physical core at the time.
+ */
+void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
+{
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
+    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+    u32 icrh = vmcb->exitinfo1 >> 32;
+    u32 icrl = vmcb->exitinfo1;
+    u32 id = vmcb->exitinfo2 >> 32;
+    u32 index = vmcb->exitinfo2 && 0xFF;
+
+    switch ( id )
+    {
+    case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
+        /*
+         * AVIC hardware handles the delivery 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(curr, APIC_ICR2, icrh);
+        vlapic_reg_write(curr, 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 *v;
+        uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
+        uint32_t short_hand = icrl & APIC_SHORT_MASK;
+        bool dest_mode = icrl & APIC_DEST_MASK;
+
+        for_each_vcpu ( currd,  v )
+        {
+            if ( v != curr &&
+                 vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
+                                   short_hand, dest, dest_mode) )
+            {
+                vcpu_kick(v);
+                break;
+            }
+        }
+        break;
+    }
+
+    case AVIC_INCMP_IPI_ERR_INV_TARGET:
+        gprintk(XENLOG_ERR,
+                "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
+                __func__, icrh, icrl, index);
+        domain_crash(currd);
+        break;
+
+    case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
+        gprintk(XENLOG_ERR,
+                "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
+                __func__, icrh, icrl, index);
+        domain_crash(currd);
+        break;
+
+    default:
+        gprintk(XENLOG_ERR, "SVM: %s: Unknown IPI interception (%#x)\n",
+                __func__, id);
+        domain_crash(currd);
+    }
+}
+
+static avic_logical_id_entry_t *
+avic_get_logical_id_entry(struct svm_domain *d, u32 ldr, bool flat)
+{
+    unsigned int index;
+    unsigned int dest_id = GET_xAPIC_LOGICAL_ID(ldr);
+
+    if ( !dest_id )
+        return NULL;
+
+    if ( flat )
+    {
+        index = ffs(dest_id) - 1;
+        if ( index > 7 )
+            return NULL;
+    }
+    else
+    {
+        unsigned int cluster = (dest_id & 0xf0) >> 4;
+        int apic = ffs(dest_id & 0x0f) - 1;
+
+        if ( (apic < 0) || (apic > 7) || (cluster >= 0xf) )
+            return NULL;
+        index = (cluster << 2) + apic;
+    }
+
+    ASSERT(index <= 255);
+
+    return &d->avic_logical_id_table[index];
+}
+
+static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
+{
+    avic_logical_id_entry_t *entry, new_entry;
+    u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);
+
+    entry = avic_get_logical_id_entry(&v->domain->arch.hvm_domain.svm,
+                                      ldr, (dfr == APIC_DFR_FLAT));
+    if ( !entry )
+        return -EINVAL;
+
+    new_entry.raw = ACCESS_ONCE(entry->raw);
+    new_entry.guest_phy_apic_id = g_phy_id;
+    new_entry.valid = valid;
+    ACCESS_ONCE(entry->raw) = new_entry.raw;
+
+    return 0;
+}
+
+static int avic_handle_ldr_update(struct vcpu *v)
+{
+    int ret = 0;
+    u32 ldr = vlapic_reg_read(vcpu_vlapic(v), APIC_LDR);
+    u32 apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
+
+    if ( !ldr )
+        return -EINVAL;
+
+    ret = avic_ldr_write(v, GET_xAPIC_ID(apic_id), ldr, true);
+    if ( ret && v->arch.hvm_svm.avic_last_ldr )
+    {
+        /*
+         * Note:
+         * In case of failure to update LDR register,
+         * we set the guest physical APIC ID to 0,
+         * and set the entry logical APID ID entry
+         * to invalid (false).
+         */
+        avic_ldr_write(v, 0, v->arch.hvm_svm.avic_last_ldr, false);
+        v->arch.hvm_svm.avic_last_ldr = 0;
+    }
+    else
+    {
+        /*
+         * Note:
+         * This saves the last valid LDR so that we
+         * know which entry in the local APIC ID
+         * to clean up when the LDR is updated.
+         */
+        v->arch.hvm_svm.avic_last_ldr = ldr;
+    }
+
+    return ret;
+}
+
+static int avic_handle_dfr_update(struct vcpu *v)
+{
+    u32 mod;
+    struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
+    u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);
+
+    mod = (dfr >> 28) & 0xFu;
+
+    spin_lock(&d->avic_dfr_mode_lock);
+    if ( d->avic_dfr_mode != mod )
+    {
+        /*
+         * We assume that all local APICs are using the same type.
+         * If DFR mode changes, we need to flush the domain AVIC logical
+         * APIC id table.
+         */
+        clear_domain_page(page_to_mfn(d->avic_logical_id_table_pg));
+        d->avic_dfr_mode = mod;
+    }
+    spin_unlock(&d->avic_dfr_mode_lock);
+
+    if ( v->arch.hvm_svm.avic_last_ldr )
+        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 = vlapic_reg_read(vcpu_vlapic(v), offset);
+
+    switch ( offset )
+    {
+    case APIC_ID:
+        /*
+         * Currently, we do not support APIC_ID update while
+         * the vcpus are running, which might require updating
+         * AVIC max APIC ID in all VMCBs. This would require
+         * synchronize update on all running VCPUs.
+         */
+        return X86EMUL_UNHANDLEABLE;
+
+    case APIC_LDR:
+        if ( avic_handle_ldr_update(v) )
+            return X86EMUL_UNHANDLEABLE;
+        break;
+
+    case APIC_DFR:
+        if ( avic_handle_dfr_update(v) )
+            return X86EMUL_UNHANDLEABLE;
+        break;
+
+    default:
+        break;
+    }
+
+    vlapic_reg_write(v, offset, reg);
+
+    return X86EMUL_OKAY;
+}
+
+static inline bool avic_is_trap(u32 offset)
+{
+    u32 pos = offset >> 4;
+    static const unsigned long avic_trap[] = {
+#define REG(x) (1UL << (APIC_ ## x >> 4))
+    REG(ID)   | REG(EOI)     | REG(RRR)   | REG(LDR)  |
+    REG(DFR)  | REG(SPIV)    | REG(ESR)   | REG(ICR)  |
+    REG(LVTT) | REG(LVTTHMR) | REG(LVTPC) | REG(LVT0) |
+    REG(LVT1) | REG(LVTERR)  | REG(TMICT) | REG(TDCR)
+#undef REG
+    };
+
+    return pos < (sizeof(avic_trap) * 8) && test_bit(pos, avic_trap);
+}
+
+/*
+ * Note:
+ * This function handles the AVIC_NOACCEL #vmexit when AVIC is enabled.
+ * The hardware generates this fault when :
+ * - A guest access to an APIC register that is not accelerated
+ *   by AVIC hardware.
+ * - EOI is attempted when the highest priority in-service interrupt
+ *   is level-triggered.
+ */
+void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
+{
+    struct vcpu *curr = current;
+    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+    u32 offset = vmcb->exitinfo1 & 0xFF0;
+    u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
+
+    if ( avic_is_trap(offset) )
+    {
+        /* Handling AVIC Trap (intercept right after the access). */
+        if ( !rw )
+        {
+            /*
+             * If a read trap happens, the CPU microcode does not
+             * implement the spec.
+             */
+            gprintk(XENLOG_ERR, "%s: Invalid #VMEXIT due to trap read (%#x)\n",
+                    __func__, offset);
+            domain_crash(curr->domain);
+        }
+
+        if ( avic_unaccel_trap_write(curr) != X86EMUL_OKAY )
+        {
+            gprintk(XENLOG_ERR, "%s: Failed to handle trap write (%#x)\n",
+                    __func__, offset);
+            domain_crash(curr->domain);
+        }
+    }
+    else
+        /* Handling AVIC Fault (intercept before the access). */
+        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
+                                 X86_EVENT_NO_EC);
+    return;
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c264353e69..249059625c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -3009,6 +3009,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             VM_EVENT_DESC_TR, exit_reason == VMEXIT_TR_WRITE);
         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:
         gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", "
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index 0c01a40ff5..1961daa578 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -36,4 +36,7 @@ void svm_avic_dom_destroy(struct domain *d);
 bool svm_avic_vcpu_enabled(const struct vcpu *v);
 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 e625884c8b..f27bdbd83d 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
 };
 
@@ -516,6 +518,9 @@ struct svm_domain {
      */
     avic_logical_id_entry_t *avic_logical_id_table;
     struct page_info *avic_logical_id_table_pg;
+
+    u32 avic_dfr_mode;
+    spinlock_t avic_dfr_mode_lock;
 };
 
 struct arch_svm_struct {
@@ -551,6 +556,7 @@ struct arch_svm_struct {
     } osvw;
 
     struct avic_physical_id_entry *avic_last_phy_id;
+    u32 avic_last_ldr;
 };
 
 struct vmcb_struct *alloc_vmcb(void);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 06/10] x86/SVM: Add vcpu scheduling support for AVIC

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Add hooks to manage AVIC data structure during vcpu scheduling.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/svm/avic.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c  | 10 +++++++++
 2 files changed, 61 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 2fba35fe47..7cc10c313a 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -36,6 +36,7 @@
 
 #define AVIC_UNACCEL_ACCESS_OFFSET_MASK    0xFF0
 
+#define IS_RUNNING_BIT        62
 /*
  * Note:
  * Currently, svm-avic mode is not supported with nested virtualization.
@@ -65,6 +66,51 @@ avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)
     return &d->avic_physical_id_table[index];
 }
 
+static void avic_vcpu_load(struct vcpu *v)
+{
+    struct arch_svm_struct *s = &v->arch.hvm_svm;
+    int h_phy_apic_id;
+
+    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
+
+    /*
+     * Note: APIC ID = 0xff is used for broadcast.
+     *       APIC ID > 0xff is reserved.
+     */
+    h_phy_apic_id = cpu_data[v->processor].apicid;
+    ASSERT(h_phy_apic_id < AVIC_PHY_APIC_ID_MAX);
+
+    s->avic_last_phy_id->host_phy_apic_id = h_phy_apic_id;
+    smp_wmb();
+    set_bit(IS_RUNNING_BIT, (u64*)(s->avic_last_phy_id));
+}
+
+static void avic_vcpu_unload(struct vcpu *v)
+{
+    struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+    clear_bit(IS_RUNNING_BIT, (u64*)(s->avic_last_phy_id));
+}
+
+static void avic_vcpu_resume(struct vcpu *v)
+{
+    struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+    ASSERT(svm_avic_vcpu_enabled(v));
+    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
+
+    set_bit(IS_RUNNING_BIT, (u64*)(s->avic_last_phy_id));
+}
+
+static void avic_vcpu_block(struct vcpu *v)
+{
+    struct arch_svm_struct *s = &v->arch.hvm_svm;
+
+    ASSERT(svm_avic_vcpu_enabled(v));
+
+    clear_bit(IS_RUNNING_BIT, (u64*)(s->avic_last_phy_id));
+}
+
 int svm_avic_dom_init(struct domain *d)
 {
     int ret = 0;
@@ -108,6 +154,11 @@ int svm_avic_dom_init(struct domain *d)
 
     spin_lock_init(&d->arch.hvm_domain.svm.avic_dfr_mode_lock);
 
+    d->arch.hvm_domain.pi_ops.switch_from = avic_vcpu_unload;
+    d->arch.hvm_domain.pi_ops.switch_to = avic_vcpu_load;
+    d->arch.hvm_domain.pi_ops.vcpu_block = avic_vcpu_block;
+    d->arch.hvm_domain.pi_ops.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 249059625c..b3e3c84175 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1088,6 +1088,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.switch_from )
+        v->domain->arch.hvm_domain.pi_ops.switch_from(v);
+
     svm_vmload_pa(per_cpu(host_vmcb, cpu));
 
     /* Resume use of ISTs now that the host TR is reinstated. */
@@ -1120,6 +1124,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.switch_to )
+        v->domain->arch.hvm_domain.pi_ops.switch_to(v);
+
     if ( cpu_has_rdtscp )
         wrmsr_tsc_aux(hvm_msr_tsc_aux(v));
 }
@@ -1167,6 +1174,9 @@ static void noreturn svm_do_resume(struct vcpu *v)
         vmcb_set_vintr(vmcb, intr);
     }
 
+    if ( v->domain->arch.hvm_domain.pi_ops.do_resume )
+        v->domain->arch.hvm_domain.pi_ops.do_resume(v);
+
     hvm_do_resume(v);
 
     reset_stack_and_jump(svm_asm_do_resume);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 07/10] x86/SVM: Add interrupt management code via AVIC

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

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.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/svm/avic.c        | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/hvm/svm/intr.c        |  4 ++++
 xen/arch/x86/hvm/svm/svm.c         | 15 +++++++++++++--
 xen/include/asm-x86/hvm/svm/avic.h |  1 +
 xen/include/asm-x86/msr-index.h    |  1 +
 5 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 7cc10c313a..19caaeda53 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -522,6 +522,35 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
     return;
 }
 
+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 interrupt is disabled, do not ignore the interrupt */
+    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(MSR_AMD_AVIC_DOORBELL, cpu_data[v->processor].apicid);
+    else
+        vcpu_kick(v);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 8511ff0b70..8c5a2eb2cd 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -29,6 +29,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 */
@@ -100,6 +101,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 b3e3c84175..15744a16a7 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1163,7 +1163,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;
 
@@ -1728,6 +1729,9 @@ const struct hvm_function_table * __init start_svm(void)
     if ( !cpu_has_svm_avic )
         svm_avic = 0;
 
+    if ( svm_avic )
+        svm_function_table.deliver_posted_intr  = svm_avic_deliver_posted_intr;
+
 #define P(p,s) if ( p ) { printk(" - %s\n", s); printed = 1; }
     P(cpu_has_svm_npt, "Nested Page Tables (NPT)");
     P(cpu_has_svm_lbrv, "Last Branch Record (LBR) Virtualisation");
@@ -2559,7 +2563,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,
@@ -2792,6 +2797,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/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index 1961daa578..8e8c4c9422 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -39,4 +39,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_ */
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 8416756f02..f37b08bf83 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -189,6 +189,7 @@
 #define MSR_K8_ENABLE_C1E		0xc0010055
 #define MSR_K8_VM_CR			0xc0010114
 #define MSR_K8_VM_HSAVE_PA		0xc0010117
+#define MSR_AMD_AVIC_DOORBELL		0xc001011b
 
 #define MSR_AMD_FAM15H_EVNTSEL0		0xc0010200
 #define MSR_AMD_FAM15H_PERFCTR0		0xc0010201
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 08/10] x86/HVM: Hook up miscellaneous AVIC functions

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch modifies the hvm_function_table.virtual_intr_delivery_enabled()
to become a bool variable as both VMX and SVM simply return static value.

Also, this patch hooks 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    |  3 +++
 xen/arch/x86/hvm/vlapic.c     | 12 ++----------
 xen/arch/x86/hvm/vmx/vmx.c    |  8 ++------
 xen/include/asm-x86/hvm/hvm.h |  4 +++-
 4 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 15744a16a7..3057d232bc 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1730,7 +1730,10 @@ const struct hvm_function_table * __init start_svm(void)
         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;
+    }
 
 #define P(p,s) if ( p ) { printk(" - %s\n", s); printed = 1; }
     P(cpu_has_svm_npt, "Nested Page Tables (NPT)");
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index eb71cc21ed..7fccfb4f06 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1258,14 +1258,6 @@ void vlapic_adjust_i8259_target(struct domain *d)
     pt_adjust_global_vcpu_target(v);
 }
 
-int vlapic_virtual_intr_delivery_enabled(void)
-{
-    if ( hvm_funcs.virtual_intr_delivery_enabled )
-        return hvm_funcs.virtual_intr_delivery_enabled();
-    else
-        return 0;
-}
-
 int vlapic_has_pending_irq(struct vcpu *v)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
@@ -1278,7 +1270,7 @@ int vlapic_has_pending_irq(struct vcpu *v)
     if ( irr == -1 )
         return -1;
 
-    if ( vlapic_virtual_intr_delivery_enabled() &&
+    if ( hvm_funcs.virtual_intr_delivery_enabled &&
          !nestedhvm_vcpu_in_guestmode(v) )
         return irr;
 
@@ -1316,7 +1308,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool_t force_ack)
     int isr;
 
     if ( !force_ack &&
-         vlapic_virtual_intr_delivery_enabled() )
+         hvm_funcs.virtual_intr_delivery_enabled )
         return 1;
 
     /* If there's no chance of using APIC assist then bail now. */
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 970751494c..8c228c4bab 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1900,11 +1900,6 @@ static void vmx_update_eoi_exit_bitmap(struct vcpu *v, u8 vector, u8 trig)
         vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
-static int vmx_virtual_intr_delivery_enabled(void)
-{
-    return cpu_has_vmx_virtual_intr_delivery;
-}
-
 static void vmx_process_isr(int isr, struct vcpu *v)
 {
     unsigned long status;
@@ -2283,7 +2278,6 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .nhvm_intr_blocked    = nvmx_intr_blocked,
     .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
     .update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap,
-    .virtual_intr_delivery_enabled = vmx_virtual_intr_delivery_enabled,
     .process_isr          = vmx_process_isr,
     .deliver_posted_intr  = vmx_deliver_posted_intr,
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
@@ -2423,6 +2417,8 @@ const struct hvm_function_table * __init start_vmx(void)
         vmx_function_table.process_isr = NULL;
         vmx_function_table.handle_eoi = NULL;
     }
+    else
+        vmx_function_table.virtual_intr_delivery_enabled = true;
 
     if ( cpu_has_vmx_posted_intr_processing )
     {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index ef5e198ebd..8d2f0c1acc 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -97,6 +97,9 @@ struct hvm_function_table {
     /* Necessary hardware support for alternate p2m's? */
     bool altp2m_supported;
 
+    /* Hardware virtual interrupt delivery enable? */
+    bool virtual_intr_delivery_enabled;
+
     /* Indicate HAP capabilities. */
     unsigned int hap_capabilities;
 
@@ -195,7 +198,6 @@ struct hvm_function_table {
 
     /* Virtual interrupt delivery */
     void (*update_eoi_exit_bitmap)(struct vcpu *v, u8 vector, u8 trig);
-    int (*virtual_intr_delivery_enabled)(void);
     void (*process_isr)(int isr, struct vcpu *v);
     void (*deliver_posted_intr)(struct vcpu *v, u8 vector);
     void (*sync_pir_to_irr)(struct vcpu *v);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 09/10] x86/SVM: Introduce svm command line option

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch introduces a new Xen command line option to enable/disable
SVM sub-options. Currently, it supports sub-option "avic", which can
be used to enable/disable SVM AVIC feature.

Signed-off-by: Suavee Suthikulpant <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 docs/misc/xen-command-line.markdown | 16 ++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c          | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index b353352adf..60a1005c42 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1730,6 +1730,22 @@ enforces the maximum theoretically necessary timeout of 670ms. Any number
 is being interpreted as a custom timeout in milliseconds. Zero or boolean
 false disable the quirk workaround, which is also the default.
 
+### svm
+> `= List of [ avic ]`
+
+> Sub-options:
+
+> All sub-options are of boolean kind and can be prefixed with `no-` to
+> effect the inverse meaning.
+
+> `avic`
+
+> Default: `false`
+
+>> This option enables Advanced Virtual Interrupt Controller (AVIC),
+>> which is an extension of AMD Secure Virtual Machine (SVM) to virtualize
+>> local APIC for guest VM.
+
 ### sync\_console
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 3057d232bc..bf851f83a1 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -64,6 +64,16 @@
 #include <asm/monitor.h>
 #include <asm/xstate.h>
 
+static int parse_svm_param(const char *s);
+
+/*
+ * The 'svm' parameter en/dis-ables various SVM features.
+ * Optional comma separated value may contain:
+ *
+ *   avic - Enable SVM Advanced Virtual Interrupt Controller (AVIC)
+ */
+custom_param("svm", parse_svm_param);
+
 void svm_asm_do_resume(void);
 
 u32 svm_feature_flags;
@@ -89,6 +99,28 @@ static bool_t amd_erratum383_found __read_mostly;
 static uint64_t osvw_length, osvw_status;
 static DEFINE_SPINLOCK(osvw_lock);
 
+static int __init parse_svm_param(const char *s)
+{
+    char *ss;
+    int val;
+
+    do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "avic") )
+            svm_avic = val;
+
+        s = ss + 1;
+    } while ( ss );
+
+    return 0;
+}
 /* Only crash the guest if the problem originates in kernel mode. */
 static void svm_crash_or_fault(struct vcpu *v)
 {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 10/10] x86/SVM: Append AMD AVIC related data to IRQ keyhandler 'i'

[Janakarajan Natarajan]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Append AVIC related data when IRQ information is dumped with key 'i'.
Here is an example of per-domain statistics being dumped.

    *********** SVM AVIC Statistics **************
    >>> Domain 1 <<<
        VCPU 0
        * incomp_ipi = 3110
        * noaccel    = 236475
        * post_intr  = 116176
        * doorbell   = 715
        VCPU 1
        * incomp_ipi = 2565
        * noaccel    = 233061
        * post_intr  = 115765
        * doorbell   = 771
    **************************************

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 xen/arch/x86/hvm/svm/avic.c        | 54 +++++++++++++++++++++++++++++++++++++-
 xen/arch/x86/irq.c                 |  2 ++
 xen/include/asm-x86/hvm/svm/avic.h |  3 +++
 xen/include/asm-x86/hvm/svm/vmcb.h |  6 +++++
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 19caaeda53..fd86c578db 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -48,6 +48,11 @@ bool svm_avic = false;
 static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
     avic_backing_page[PAGE_SIZE];
 
+static inline const bool svm_is_avic_domain(struct domain *d)
+{
+    return d->arch.hvm_domain.svm.avic_physical_id_table != 0;
+}
+
 static struct avic_physical_id_entry*
 avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)
 {
@@ -252,6 +257,8 @@ void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
     u32 id = vmcb->exitinfo2 >> 32;
     u32 index = vmcb->exitinfo2 && 0xFF;
 
+    curr->arch.hvm_svm.cnt_avic_incomp_ipi++;
+
     switch ( id )
     {
     case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
@@ -494,6 +501,8 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
     u32 offset = vmcb->exitinfo1 & 0xFF0;
     u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
 
+    curr->arch.hvm_svm.cnt_avic_noaccel++;
+
     if ( avic_is_trap(offset) )
     {
         /* Handling AVIC Trap (intercept right after the access). */
@@ -541,14 +550,57 @@ void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec)
     if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
         return;
 
+    v->arch.hvm_svm.cnt_avic_post_intr++;
     /*
      * 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(MSR_AMD_AVIC_DOORBELL, cpu_data[v->processor].apicid);
-    else
+        v->arch.hvm_svm.cnt_avic_doorbell++;
+    }
+    else {
         vcpu_kick(v);
+    }
+}
+
+void dump_avic_info()
+{
+    struct domain *d;
+    struct vcpu *v;
+
+    if ( !svm_avic )
+        return;
+
+    printk("*********** SVM AVIC Statistics **************\n");
+
+    rcu_read_lock(&domlist_read_lock);
+
+    for_each_domain ( d )
+    {
+        if ( !svm_is_avic_domain(d) )
+            continue;
+
+        printk(">>> Domain %d <<<\n", d->domain_id);
+        for_each_vcpu ( d, v )
+        {
+            printk("\tVCPU %d\n", v->vcpu_id);
+            printk("\t* incomp_ipi = %u\n",
+                   v->arch.hvm_svm.cnt_avic_incomp_ipi);
+            printk("\t* noaccel    = %u\n",
+                   v->arch.hvm_svm.cnt_avic_noaccel);
+            printk("\t* post_intr  = %u\n",
+                   v->arch.hvm_svm.cnt_avic_post_intr);
+            printk("\t* doorbell   = %u\n",
+                   v->arch.hvm_svm.cnt_avic_doorbell);
+        }
+    }
+
+    rcu_read_unlock(&domlist_read_lock);
+
+    printk("**************************************\n");
+
 }
 
 /*
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 87ef2e801f..62fe9c3177 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -23,6 +23,7 @@
 #include <asm/msi.h>
 #include <asm/current.h>
 #include <asm/flushtlb.h>
+#include <asm/hvm/svm/avic.h>
 #include <asm/mach-generic/mach_apic.h>
 #include <public/physdev.h>
 
@@ -2351,6 +2352,7 @@ static void dump_irqs(unsigned char key)
             printk("   %#02x -> %ps()\n", i, direct_apic_vector[i]);
 
     dump_ioapic_irq_info();
+    dump_avic_info();
 }
 
 static int __init setup_dump_irqs(void)
diff --git a/xen/include/asm-x86/hvm/svm/avic.h b/xen/include/asm-x86/hvm/svm/avic.h
index 8e8c4c9422..92326a77a3 100644
--- a/xen/include/asm-x86/hvm/svm/avic.h
+++ b/xen/include/asm-x86/hvm/svm/avic.h
@@ -40,4 +40,7 @@ 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);
+
+void dump_avic_info(void);
+
 #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 f27bdbd83d..e902d51b25 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -557,6 +557,12 @@ struct arch_svm_struct {
 
     struct avic_physical_id_entry *avic_last_phy_id;
     u32 avic_last_ldr;
+
+    /* AVIC Statistics */
+    u32 cnt_avic_incomp_ipi;
+    u32 cnt_avic_noaccel;
+    u32 cnt_avic_post_intr;
+    u32 cnt_avic_doorbell;
 };
 
 struct vmcb_struct *alloc_vmcb(void);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 02/10] x86/HVM: Rename vlapic_read_aligned() to vlapic_reg_read()

[Wei Liu]

On Mon, May 07, 2018 at 04:07:45PM -0500, Janakarajan Natarajan wrote:
> Rename vlapic_read_aligned() to vlapic_reg_read() to make it a pair of
> vlapic_reg_write().
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  xen/arch/x86/hvm/vlapic.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 1b9f00a0e4..c9b6461cbf 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -592,7 +592,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(const struct vlapic *vlapic,
> +static uint32_t vlapic_reg_read(const struct vlapic *vlapic,
>                                      unsigned int offset)

Minor issue: indentation needs to be adjusted.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 02/10] x86/HVM: Rename vlapic_read_aligned() to vlapic_reg_read()

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> Rename vlapic_read_aligned() to vlapic_reg_read() to make it a pair of
> vlapic_reg_write().
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/10] x86/HVM: Make vlapic_reg_read/write() non-static

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> AMD AVIC code makes use of vlapic_reg_read() and vlapic_reg_write(). To
> do this make the functions non-static.

To be honest I'd prefer if each of the two functions was made non-static in
the patch actually needing this to be the case. This allows better judgment
on whether that's really an appropriate thing to do.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 03/10] x86/HVM: Make vlapic_reg_read/write() non-static

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -137,6 +137,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_reg_read(const struct vlapic *vlapic, unsigned int offset);

For them to be made non-static, they should really be counterparts of one
another. This (to me) includes suitably matching parameter types (i.e. both
should take struct vlapic * or struct vcpu *; the read one having it const is
of course fine).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,190 @@
> +/*
> + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) support
> + * Copyright (c) 2018, Advanced Micro Devices, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/domain_page.h>
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <asm/acpi.h>
> +#include <asm/apicdef.h>
> +#include <asm/atomic.h>
> +#include <asm/event.h>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/support.h>
> +#include <asm/hvm/svm/avic.h>
> +#include <asm/hvm/vlapic.h>
> +#include <asm/p2m.h>
> +#include <asm/page.h>

Are all of these really needed? For example, xen/stdbool.h isn't commonly
included by non-header files, but is instead obtained from xen/types.h. That
header, in turn, is rarely required to be included explicitly by non-headers
because almost every header already includes it anyway.

In some cases I'm also not convinced you really mean asm/ (rather than
xen/).

> +/* Note: Current max index allowed for physical APIC ID table is 255. */
> +#define AVIC_PHY_APIC_ID_MAX    GET_xAPIC_ID(APIC_ID_MASK)

I think it was pointed out before that "max" generally means the last valid
value, rather than the first invalid one.

> +/*
> + * Note:
> + * Currently, svm-avic mode is not supported with nested virtualization.
> + * Therefore, it is not yet currently enabled by default. Once the support
> + * is in-place, this should be enabled by default.
> + */
> +bool svm_avic = false;
> +
> +static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
> +    avic_backing_page[PAGE_SIZE];

So nothing ever writes to this page? I think it would be misleading if CPU side
writes were possible, yet this was marked const.

Also - does this really need allocating statically (rather than just on systems
actually needing it)?

> +static struct avic_physical_id_entry*
> +avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)

I think the first parameter could be const.

> +int svm_avic_dom_init(struct domain *d)
> +{
> +    int ret = 0;
> +    struct page_info *pg;
> +
> +    if ( !svm_avic || !has_vlapic(d) )
> +        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 for APIC.
> +     */
> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> +                       _mfn(virt_to_mfn(avic_backing_page)), PAGE_ORDER_4K,
> +                       p2m_access_rw);
> +
> +    /* Init AVIC logical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_owner);

Do you really mean d here (and below) rather than NULL?

> +    if ( !pg )
> +    {
> +        ret = -ENOMEM;
> +        goto err_out;
> +    }
> +    clear_domain_page(page_to_mfn(pg));
> +    d->arch.hvm_domain.svm.avic_logical_id_table_pg = pg;
> +    d->arch.hvm_domain.svm.avic_logical_id_table = __map_domain_page_global(pg);

I think I have said before that I don't think you need to store both
virtual and physical address here, unless both are used frequently.
You establishing a global mapping suggests to me that it's the
virtual address you want to store (MFN and hence struct page_info
can be derived from the mapping via domain_page_map_to_mfn(),
like you already do further down).

> +bool svm_avic_vcpu_enabled(const struct vcpu *v)
> +{
> +    const struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    const struct vmcb_struct *vmcb = s->vmcb;
> +
> +    return vmcb->_vintr.fields.avic_enable;

Please don't use excess local variables (both of them are used just once,
and I'm sure you could get away with just one of the two [or none at all]
without breaking the line length limit).

Also shouldn't this be vmcb_get_vintr()?

> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> +    u32 apic_id;
> +    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;
> +    const struct vlapic *vlapic = vcpu_vlapic(v);
> +    struct avic_physical_id_entry *entry;
> +
> +    if ( !svm_avic || !has_vlapic(v->domain) )
> +        return 0;
> +
> +    if ( !vlapic || !vlapic->regs_page )
> +        return -EINVAL;
> +
> +    apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);

Why can't this be vlapic_get_reg()?

> +    s->avic_last_phy_id = avic_get_physical_id_entry(d, GET_xAPIC_ID(apic_id));

You don't appear to read this value outside of this function. Please store
values in struct domain / struct vcpu only if you in fact read them, and
if their calculation isn't trivial.

I also don't appear to understand the purpose of the "last" in the name.

> +    if ( !s->avic_last_phy_id )
> +        return -EINVAL;
> +
> +    vmcb->avic_bk_pg_pa = page_to_maddr(vlapic->regs_page);
> +    vmcb->avic_logical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_logical_id_table));
> +    vmcb->avic_physical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_physical_id_table));
> +
> +    /* Set Physical ID Table Pointer [7:0] to max apic id of the domain */
> +    vmcb->avic_physical_id_table_pa |= (v->domain->max_vcpus * 2) & 0xFF;
> +
> +    entry = s->avic_last_phy_id;
> +    entry->bk_pg_ptr_mfn = (vmcb->avic_bk_pg_pa) >> PAGE_SHIFT;

Please don't open-code paddr_to_pfn() / maddr_to_mfn().

> @@ -215,6 +216,8 @@ static int construct_vmcb(struct vcpu *v)
>              vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
>      }
>  
> +    svm_avic_init_vmcb(v);

This function may fail.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1597,6 +1597,10 @@ int vlapic_init(struct vcpu *v)
>  
>      if (vlapic->regs_page == NULL)
>      {
> +        /*
> +         * SVM AVIC depends on the vlapic->regs_page being a full
> +         * page allocation as it is also used for vAPIC backing page.
> +         */
>          vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);

I'm not convinced of the utility of this comment - iirc the same is true on the
VMX side (and there was no similar comment added here at the time).

> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -0,0 +1,39 @@
> +#ifndef _SVM_AVIC_H_
> +#define _SVM_AVIC_H_
> +
> +#include <xen/compiler.h>

You mean xen/types.h here, or else ...

> +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,
> +};
> +
> +typedef union avic_logical_id_entry {
> +    u32 raw;

... u32 (which really should be uint32_t - please replace thoughout the series)
may not be available here.

> +    struct __packed {
> +        u32 guest_phy_apic_id : 8;
> +        u32 res               : 23;
> +        u32 valid             : 1;
> +    };
> +} avic_logical_id_entry_t;
> +
> +struct __packed avic_physical_id_entry {
> +        u64 host_phy_apic_id  : 8;
> +        u64 res1              : 4;
> +        u64 bk_pg_ptr_mfn     : 40;
> +        u64 res2              : 10;
> +        u64 is_running        : 1;
> +        u64 valid             : 1;
> +};
> +
> +extern bool svm_avic;
> +
> +int svm_avic_dom_init(struct domain *d);
> +void svm_avic_dom_destroy(struct domain *d);
> +
> +bool svm_avic_vcpu_enabled(const struct vcpu *v);
> +int svm_avic_init_vmcb(struct vcpu *v);

These declarations even need xen/sched.h in place of (or together with)
xen/types.h.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

[Andrew Cooper]

On 16/05/18 16:29, Jan Beulich wrote:
>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
>>>>
>> +    s->avic_last_phy_id = avic_get_physical_id_entry(d, GET_xAPIC_ID(apic_id));
> You don't appear to read this value outside of this function. Please store
> values in struct domain / struct vcpu only if you in fact read them, and
> if their calculation isn't trivial.
>
> I also don't appear to understand the purpose of the "last" in the name.

s->avic_last_phy_id is not needed.  It is a cached unchanging pointer
into the physid table.

Removing it will help clean up some of the later patches.

Strictly speaking, I'm not sure this is true if we decide to permit a
guest to update its local APIC ID, but we don't currently allow this and
I see no benefit from supporting it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> @@ -180,6 +185,293 @@ int svm_avic_init_vmcb(struct vcpu *v)
>  }
>  
>  /*
> + * Note:
> + * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
> + * The hardware generates this fault when an IPI could not be delivered
> + * to all targeted guest virtual processors because at least one guest
> + * virtual processor was not allocated to a physical core at the time.
> + */
> +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
> +    u32 icrh = vmcb->exitinfo1 >> 32;
> +    u32 icrl = vmcb->exitinfo1;
> +    u32 id = vmcb->exitinfo2 >> 32;
> +    u32 index = vmcb->exitinfo2 && 0xFF;
> +
> +    switch ( id )
> +    {
> +    case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
> +        /*
> +         * AVIC hardware handles the delivery 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.

Please utilize the permitted line length (also elsewhere).

> +         */
> +        vlapic_reg_write(curr, APIC_ICR2, icrh);
> +        vlapic_reg_write(curr, 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 *v;
> +        uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
> +        uint32_t short_hand = icrl & APIC_SHORT_MASK;
> +        bool dest_mode = icrl & APIC_DEST_MASK;
> +
> +        for_each_vcpu ( currd,  v )
> +        {
> +            if ( v != curr &&
> +                 vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
> +                                   short_hand, dest, dest_mode) )
> +            {
> +                vcpu_kick(v);
> +                break;

Why do you break out of the loop here? With a shorthand more than
one vCPU might be the target.

> +            }
> +        }
> +        break;
> +    }
> +
> +    case AVIC_INCMP_IPI_ERR_INV_TARGET:
> +        gprintk(XENLOG_ERR,
> +                "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",

%#08x produces something like 0x012345, rather than a full eight digits.
Preferably drop the #, or if you really think it's needed replace it be an
explicit 0x.

> +                __func__, icrh, icrl, index);

Please use __func__ only when a log message really can't be disambiguated
another way.

For both of these - same further down.

> +static avic_logical_id_entry_t *
> +avic_get_logical_id_entry(struct svm_domain *d, u32 ldr, bool flat)
> +{
> +    unsigned int index;
> +    unsigned int dest_id = GET_xAPIC_LOGICAL_ID(ldr);
> +
> +    if ( !dest_id )
> +        return NULL;
> +
> +    if ( flat )
> +    {
> +        index = ffs(dest_id) - 1;
> +        if ( index > 7 )
> +            return NULL;
> +    }
> +    else
> +    {
> +        unsigned int cluster = (dest_id & 0xf0) >> 4;
> +        int apic = ffs(dest_id & 0x0f) - 1;
> +
> +        if ( (apic < 0) || (apic > 7) || (cluster >= 0xf) )

I can't see a way for apic to be larger than 3 with the calculation above.

> +            return NULL;
> +        index = (cluster << 2) + apic;
> +    }
> +
> +    ASSERT(index <= 255);

Which of the many possible meanings of 255 is this?

> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
> +{
> +    avic_logical_id_entry_t *entry, new_entry;
> +    u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);

Just to give another example - looks like this too could be vlapic_get_reg().

> +static int avic_handle_ldr_update(struct vcpu *v)
> +{
> +    int ret = 0;

Pointless initializer.

> +    u32 ldr = vlapic_reg_read(vcpu_vlapic(v), APIC_LDR);
> +    u32 apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
> +
> +    if ( !ldr )
> +        return -EINVAL;
> +
> +    ret = avic_ldr_write(v, GET_xAPIC_ID(apic_id), ldr, true);
> +    if ( ret && v->arch.hvm_svm.avic_last_ldr )
> +    {
> +        /*
> +         * Note:
> +         * In case of failure to update LDR register,
> +         * we set the guest physical APIC ID to 0,
> +         * and set the entry logical APID ID entry
> +         * to invalid (false).
> +         */
> +        avic_ldr_write(v, 0, v->arch.hvm_svm.avic_last_ldr, false);
> +        v->arch.hvm_svm.avic_last_ldr = 0;
> +    }
> +    else
> +    {
> +        /*
> +         * Note:
> +         * This saves the last valid LDR so that we
> +         * know which entry in the local APIC ID
> +         * to clean up when the LDR is updated.
> +         */
> +        v->arch.hvm_svm.avic_last_ldr = ldr;

The comment says "last valid", but you may get here also when the
first avic_ldr_write() failed. I think you mean

    if ( !ret )
    ...
    else if ( v->arch.hvm_svm.avic_last_ldr )
    ...

> +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 = vlapic_reg_read(vcpu_vlapic(v), offset);
> +
> +    switch ( offset )
> +    {
> +    case APIC_ID:
> +        /*
> +         * Currently, we do not support APIC_ID update while
> +         * the vcpus are running, which might require updating
> +         * AVIC max APIC ID in all VMCBs. This would require
> +         * synchronize update on all running VCPUs.
> +         */
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    case APIC_LDR:
> +        if ( avic_handle_ldr_update(v) )
> +            return X86EMUL_UNHANDLEABLE;
> +        break;
> +
> +    case APIC_DFR:
> +        if ( avic_handle_dfr_update(v) )
> +            return X86EMUL_UNHANDLEABLE;
> +        break;
> +
> +    default:
> +        break;

This default case is unnecessary.

> +void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *curr = current;
> +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
> +    u32 offset = vmcb->exitinfo1 & 0xFF0;
> +    u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;

bool?

> +    if ( avic_is_trap(offset) )
> +    {
> +        /* Handling AVIC Trap (intercept right after the access). */
> +        if ( !rw )
> +        {
> +            /*
> +             * If a read trap happens, the CPU microcode does not
> +             * implement the spec.
> +             */
> +            gprintk(XENLOG_ERR, "%s: Invalid #VMEXIT due to trap read (%#x)\n",
> +                    __func__, offset);
> +            domain_crash(curr->domain);
> +        }
> +
> +        if ( avic_unaccel_trap_write(curr) != X86EMUL_OKAY )

ITYM "else if" here.

> +        {
> +            gprintk(XENLOG_ERR, "%s: Failed to handle trap write (%#x)\n",
> +                    __func__, offset);
> +            domain_crash(curr->domain);
> +        }
> +    }
> +    else
> +        /* Handling AVIC Fault (intercept before the access). */
> +        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
> +                                 X86_EVENT_NO_EC);

What's the rationale behind having chosen this function? I don't think it is
supposed to be called from outside the VM event code.

> +    return;
> +}

Please omit such redundant return statements.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 06/10] x86/SVM: Add vcpu scheduling support for AVIC

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> @@ -65,6 +66,51 @@ avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)
>      return &d->avic_physical_id_table[index];
>  }
>  
> +static void avic_vcpu_load(struct vcpu *v)
> +{
> +    struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    int h_phy_apic_id;

APIC IDs are of unsigned type.

> +    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> +
> +    /*
> +     * Note: APIC ID = 0xff is used for broadcast.
> +     *       APIC ID > 0xff is reserved.
> +     */
> +    h_phy_apic_id = cpu_data[v->processor].apicid;
> +    ASSERT(h_phy_apic_id < AVIC_PHY_APIC_ID_MAX);
> +
> +    s->avic_last_phy_id->host_phy_apic_id = h_phy_apic_id;
> +    smp_wmb();
> +    set_bit(IS_RUNNING_BIT, (u64*)(s->avic_last_phy_id));

You have a struct defined for this - please avoid such bogus casting.
I can see why you may not want to use the bitfield here - make the
struct a union with a "raw" field, define IS_RUNNING_BIT right there (so
one can easily see the correlation; may require renaming the constant),
and do the operation on &s->raw.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 07/10] x86/SVM: Add interrupt management code via AVIC

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> +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 interrupt is disabled, do not ignore the interrupt */
> +    if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
> +        return;

It seems to me that I did comment on this before - I don't think EFLAGS.IF
should be considered here:

> +    if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
> +        return;

Latching the interrupt into IRR ought to keep it pending until the guest sets
EFLAGS.IF again.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 09/10] x86/SVM: Introduce svm command line option

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -64,6 +64,16 @@
>  #include <asm/monitor.h>
>  #include <asm/xstate.h>
>  
> +static int parse_svm_param(const char *s);

This is unnecessary if you move ...

> +/*
> + * The 'svm' parameter en/dis-ables various SVM features.
> + * Optional comma separated value may contain:
> + *
> + *   avic - Enable SVM Advanced Virtual Interrupt Controller (AVIC)
> + */
> +custom_param("svm", parse_svm_param);

... this after the function definition.

> @@ -89,6 +99,28 @@ static bool_t amd_erratum383_found __read_mostly;
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> +static int __init parse_svm_param(const char *s)
> +{
> +    char *ss;
> +    int val;
> +
> +    do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;

Please use parse_boolean().

> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        if ( !strcmp(s, "avic") )
> +            svm_avic = val;

    else
        ret = -EINVAL;

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 10/10] x86/SVM: Append AMD AVIC related data to IRQ keyhandler 'i'

[Jan Beulich]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
> @@ -2351,6 +2352,7 @@ static void dump_irqs(unsigned char key)
>              printk("   %#02x -> %ps()\n", i, direct_apic_vector[i]);
>  
>      dump_ioapic_irq_info();
> +    dump_avic_info();
>  }

While this is better than a separate key, I still don't like it. These statistics
are unrelated to the purpose of 'i'. Why can't this be connected to e.g. 'v',
provided these statistics are all that relevant in the first place?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

[Natarajan, Janakarajan]

On 5/16/2018 10:29 AM, Jan Beulich wrote:
>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/svm/avic.c
>> @@ -0,0 +1,190 @@
>> +/*
>> + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) support
>> + * Copyright (c) 2018, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/domain_page.h>
>> +#include <xen/sched.h>
>> +#include <xen/stdbool.h>
>> +#include <asm/acpi.h>
>> +#include <asm/apicdef.h>
>> +#include <asm/atomic.h>
>> +#include <asm/event.h>
>> +#include <asm/hvm/emulate.h>
>> +#include <asm/hvm/nestedhvm.h>
>> +#include <asm/hvm/support.h>
>> +#include <asm/hvm/svm/avic.h>
>> +#include <asm/hvm/vlapic.h>
>> +#include <asm/p2m.h>
>> +#include <asm/page.h>
> Are all of these really needed? For example, xen/stdbool.h isn't commonly
> included by non-header files, but is instead obtained from xen/types.h. That
> header, in turn, is rarely required to be included explicitly by non-headers
> because almost every header already includes it anyway.
>
> In some cases I'm also not convinced you really mean asm/ (rather than
> xen/).
>
>> +/* Note: Current max index allowed for physical APIC ID table is 255. */
>> +#define AVIC_PHY_APIC_ID_MAX    GET_xAPIC_ID(APIC_ID_MASK)
> I think it was pointed out before that "max" generally means the last valid
> value, rather than the first invalid one.
>
>> +/*
>> + * Note:
>> + * Currently, svm-avic mode is not supported with nested virtualization.
>> + * Therefore, it is not yet currently enabled by default. Once the support
>> + * is in-place, this should be enabled by default.
>> + */
>> +bool svm_avic = false;
>> +
>> +static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
>> +    avic_backing_page[PAGE_SIZE];
> So nothing ever writes to this page? I think it would be misleading if CPU side
> writes were possible, yet this was marked const.


AVIC hardware uses this page to look at permission bits. AFAIK, nothing 
writes to this page.


>
> Also - does this really need allocating statically (rather than just on systems
> actually needing it)?


I'm not aware of systems that don't have permission bits.


>
>> +static struct avic_physical_id_entry*
>> +avic_get_physical_id_entry(struct svm_domain *d, unsigned int index)
> I think the first parameter could be const.
>
>> +int svm_avic_dom_init(struct domain *d)
>> +{
>> +    int ret = 0;
>> +    struct page_info *pg;
>> +
>> +    if ( !svm_avic || !has_vlapic(d) )
>> +        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 for APIC.
>> +     */
>> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
>> +                       _mfn(virt_to_mfn(avic_backing_page)), PAGE_ORDER_4K,
>> +                       p2m_access_rw);
>> +
>> +    /* Init AVIC logical APIC ID table */
>> +    pg = alloc_domheap_page(d, MEMF_no_owner);
> Do you really mean d here (and below) rather than NULL?


Wouldn't the logical and physical APIC ID table pages be connected to 
the domain and its heap?


>
>> +    if ( !pg )
>> +    {
>> +        ret = -ENOMEM;
>> +        goto err_out;
>> +    }
>> +    clear_domain_page(page_to_mfn(pg));
>> +    d->arch.hvm_domain.svm.avic_logical_id_table_pg = pg;
>> +    d->arch.hvm_domain.svm.avic_logical_id_table = __map_domain_page_global(pg);
> I think I have said before that I don't think you need to store both
> virtual and physical address here, unless both are used frequently.
> You establishing a global mapping suggests to me that it's the
> virtual address you want to store (MFN and hence struct page_info
> can be derived from the mapping via domain_page_map_to_mfn(),
> like you already do further down).

Okay.


>
>> +bool svm_avic_vcpu_enabled(const struct vcpu *v)
>> +{
>> +    const struct arch_svm_struct *s = &v->arch.hvm_svm;
>> +    const struct vmcb_struct *vmcb = s->vmcb;
>> +
>> +    return vmcb->_vintr.fields.avic_enable;
> Please don't use excess local variables (both of them are used just once,
> and I'm sure you could get away with just one of the two [or none at all]
> without breaking the line length limit).
>
> Also shouldn't this be vmcb_get_vintr()?

Yes. That should be vmcb_get_vintr().


>
>> +int svm_avic_init_vmcb(struct vcpu *v)
>> +{
>> +    u32 apic_id;
>> +    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;
>> +    const struct vlapic *vlapic = vcpu_vlapic(v);
>> +    struct avic_physical_id_entry *entry;
>> +
>> +    if ( !svm_avic || !has_vlapic(v->domain) )
>> +        return 0;
>> +
>> +    if ( !vlapic || !vlapic->regs_page )
>> +        return -EINVAL;
>> +
>> +    apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
> Why can't this be vlapic_get_reg()?
>
>> +    s->avic_last_phy_id = avic_get_physical_id_entry(d, GET_xAPIC_ID(apic_id));
> You don't appear to read this value outside of this function. Please store
> values in struct domain / struct vcpu only if you in fact read them, and
> if their calculation isn't trivial.
>
> I also don't appear to understand the purpose of the "last" in the name.

I can remove this from the struct.


>
>> +    if ( !s->avic_last_phy_id )
>> +        return -EINVAL;
>> +
>> +    vmcb->avic_bk_pg_pa = page_to_maddr(vlapic->regs_page);
>> +    vmcb->avic_logical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_logical_id_table));
>> +    vmcb->avic_physical_id_table_pa = mfn_to_maddr(domain_page_map_to_mfn(d->avic_physical_id_table));
>> +
>> +    /* Set Physical ID Table Pointer [7:0] to max apic id of the domain */
>> +    vmcb->avic_physical_id_table_pa |= (v->domain->max_vcpus * 2) & 0xFF;
>> +
>> +    entry = s->avic_last_phy_id;
>> +    entry->bk_pg_ptr_mfn = (vmcb->avic_bk_pg_pa) >> PAGE_SHIFT;
> Please don't open-code paddr_to_pfn() / maddr_to_mfn().
>
>> @@ -215,6 +216,8 @@ static int construct_vmcb(struct vcpu *v)
>>               vmcb->_pause_filter_thresh = SVM_PAUSETHRESH_INIT;
>>       }
>>   
>> +    svm_avic_init_vmcb(v);
> This function may fail.
>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1597,6 +1597,10 @@ int vlapic_init(struct vcpu *v)
>>   
>>       if (vlapic->regs_page == NULL)
>>       {
>> +        /*
>> +         * SVM AVIC depends on the vlapic->regs_page being a full
>> +         * page allocation as it is also used for vAPIC backing page.
>> +         */
>>           vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
> I'm not convinced of the utility of this comment - iirc the same is true on the
> VMX side (and there was no similar comment added here at the time).
>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/hvm/svm/avic.h
>> @@ -0,0 +1,39 @@
>> +#ifndef _SVM_AVIC_H_
>> +#define _SVM_AVIC_H_
>> +
>> +#include <xen/compiler.h>
> You mean xen/types.h here, or else ...
>
>> +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,
>> +};
>> +
>> +typedef union avic_logical_id_entry {
>> +    u32 raw;
> ... u32 (which really should be uint32_t - please replace thoughout the series)
> may not be available here.

Okay. I can change it.

Thanks,
Janak

>
>> +    struct __packed {
>> +        u32 guest_phy_apic_id : 8;
>> +        u32 res               : 23;
>> +        u32 valid             : 1;
>> +    };
>> +} avic_logical_id_entry_t;
>> +
>> +struct __packed avic_physical_id_entry {
>> +        u64 host_phy_apic_id  : 8;
>> +        u64 res1              : 4;
>> +        u64 bk_pg_ptr_mfn     : 40;
>> +        u64 res2              : 10;
>> +        u64 is_running        : 1;
>> +        u64 valid             : 1;
>> +};
>> +
>> +extern bool svm_avic;
>> +
>> +int svm_avic_dom_init(struct domain *d);
>> +void svm_avic_dom_destroy(struct domain *d);
>> +
>> +bool svm_avic_vcpu_enabled(const struct vcpu *v);
>> +int svm_avic_init_vmcb(struct vcpu *v);
> These declarations even need xen/sched.h in place of (or together with)
> xen/types.h.
>
> Jan
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 04/10] x86/HVM/SVM: Add AVIC initialization code

[Jan Beulich]

>>> On 21.05.18 at 20:41, <jnataraj@amd.com> wrote:
> On 5/16/2018 10:29 AM, Jan Beulich wrote:
>>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
>>> +/*
>>> + * Note:
>>> + * Currently, svm-avic mode is not supported with nested virtualization.
>>> + * Therefore, it is not yet currently enabled by default. Once the support
>>> + * is in-place, this should be enabled by default.
>>> + */
>>> +bool svm_avic = false;
>>> +
>>> +static const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
>>> +    avic_backing_page[PAGE_SIZE];
>> So nothing ever writes to this page? I think it would be misleading if CPU side
>> writes were possible, yet this was marked const.
> 
> 
> AVIC hardware uses this page to look at permission bits. AFAIK, nothing 
> writes to this page.
> 
> 
>>
>> Also - does this really need allocating statically (rather than just on 
> systems
>> actually needing it)?
> 
> 
> I'm not aware of systems that don't have permission bits.

Older AMD ones? Intel ones?

>>> +int svm_avic_dom_init(struct domain *d)
>>> +{
>>> +    int ret = 0;
>>> +    struct page_info *pg;
>>> +
>>> +    if ( !svm_avic || !has_vlapic(d) )
>>> +        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 for APIC.
>>> +     */
>>> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
>>> +                       _mfn(virt_to_mfn(avic_backing_page)), PAGE_ORDER_4K,
>>> +                       p2m_access_rw);
>>> +
>>> +    /* Init AVIC logical APIC ID table */
>>> +    pg = alloc_domheap_page(d, MEMF_no_owner);
>> Do you really mean d here (and below) rather than NULL?
> 
> 
> Wouldn't the logical and physical APIC ID table pages be connected to 
> the domain and its heap?

Not with MEMF_no_owner afaict.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Natarajan, Janakarajan]

On 5/16/2018 10:56 AM, Jan Beulich wrote:
>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
>> @@ -180,6 +185,293 @@ int svm_avic_init_vmcb(struct vcpu *v)
>>   }
>>   
>>   /*
>> + * Note:
>> + * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
>> + * The hardware generates this fault when an IPI could not be delivered
>> + * to all targeted guest virtual processors because at least one guest
>> + * virtual processor was not allocated to a physical core at the time.
>> + */
>> +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct domain *currd = curr->domain;
>> +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>> +    u32 icrh = vmcb->exitinfo1 >> 32;
>> +    u32 icrl = vmcb->exitinfo1;
>> +    u32 id = vmcb->exitinfo2 >> 32;
>> +    u32 index = vmcb->exitinfo2 && 0xFF;
>> +
>> +    switch ( id )
>> +    {
>> +    case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
>> +        /*
>> +         * AVIC hardware handles the delivery 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.
> Please utilize the permitted line length (also elsewhere).
>
>> +         */
>> +        vlapic_reg_write(curr, APIC_ICR2, icrh);
>> +        vlapic_reg_write(curr, 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 *v;
>> +        uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
>> +        uint32_t short_hand = icrl & APIC_SHORT_MASK;
>> +        bool dest_mode = icrl & APIC_DEST_MASK;
>> +
>> +        for_each_vcpu ( currd,  v )
>> +        {
>> +            if ( v != curr &&
>> +                 vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
>> +                                   short_hand, dest, dest_mode) )
>> +            {
>> +                vcpu_kick(v);
>> +                break;
> Why do you break out of the loop here? With a shorthand more than
> one vCPU might be the target.
>
>> +            }
>> +        }
>> +        break;
>> +    }
>> +
>> +    case AVIC_INCMP_IPI_ERR_INV_TARGET:
>> +        gprintk(XENLOG_ERR,
>> +                "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
> %#08x produces something like 0x012345, rather than a full eight digits.
> Preferably drop the #, or if you really think it's needed replace it be an
> explicit 0x.
>
>> +                __func__, icrh, icrl, index);
> Please use __func__ only when a log message really can't be disambiguated
> another way.
>
> For both of these - same further down.
>
>> +static avic_logical_id_entry_t *
>> +avic_get_logical_id_entry(struct svm_domain *d, u32 ldr, bool flat)
>> +{
>> +    unsigned int index;
>> +    unsigned int dest_id = GET_xAPIC_LOGICAL_ID(ldr);
>> +
>> +    if ( !dest_id )
>> +        return NULL;
>> +
>> +    if ( flat )
>> +    {
>> +        index = ffs(dest_id) - 1;
>> +        if ( index > 7 )
>> +            return NULL;
>> +    }
>> +    else
>> +    {
>> +        unsigned int cluster = (dest_id & 0xf0) >> 4;
>> +        int apic = ffs(dest_id & 0x0f) - 1;
>> +
>> +        if ( (apic < 0) || (apic > 7) || (cluster >= 0xf) )
> I can't see a way for apic to be larger than 3 with the calculation above.
>
>> +            return NULL;
>> +        index = (cluster << 2) + apic;
>> +    }
>> +
>> +    ASSERT(index <= 255);
> Which of the many possible meanings of 255 is this?
>
>> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
>> +{
>> +    avic_logical_id_entry_t *entry, new_entry;
>> +    u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);
> Just to give another example - looks like this too could be vlapic_get_reg().
>
>> +static int avic_handle_ldr_update(struct vcpu *v)
>> +{
>> +    int ret = 0;
> Pointless initializer.
>
>> +    u32 ldr = vlapic_reg_read(vcpu_vlapic(v), APIC_LDR);
>> +    u32 apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
>> +
>> +    if ( !ldr )
>> +        return -EINVAL;
>> +
>> +    ret = avic_ldr_write(v, GET_xAPIC_ID(apic_id), ldr, true);
>> +    if ( ret && v->arch.hvm_svm.avic_last_ldr )
>> +    {
>> +        /*
>> +         * Note:
>> +         * In case of failure to update LDR register,
>> +         * we set the guest physical APIC ID to 0,
>> +         * and set the entry logical APID ID entry
>> +         * to invalid (false).
>> +         */
>> +        avic_ldr_write(v, 0, v->arch.hvm_svm.avic_last_ldr, false);
>> +        v->arch.hvm_svm.avic_last_ldr = 0;
>> +    }
>> +    else
>> +    {
>> +        /*
>> +         * Note:
>> +         * This saves the last valid LDR so that we
>> +         * know which entry in the local APIC ID
>> +         * to clean up when the LDR is updated.
>> +         */
>> +        v->arch.hvm_svm.avic_last_ldr = ldr;
> The comment says "last valid", but you may get here also when the
> first avic_ldr_write() failed. I think you mean
>
>      if ( !ret )
>      ...
>      else if ( v->arch.hvm_svm.avic_last_ldr )
>      ...
>
>> +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 = vlapic_reg_read(vcpu_vlapic(v), offset);
>> +
>> +    switch ( offset )
>> +    {
>> +    case APIC_ID:
>> +        /*
>> +         * Currently, we do not support APIC_ID update while
>> +         * the vcpus are running, which might require updating
>> +         * AVIC max APIC ID in all VMCBs. This would require
>> +         * synchronize update on all running VCPUs.
>> +         */
>> +        return X86EMUL_UNHANDLEABLE;
>> +
>> +    case APIC_LDR:
>> +        if ( avic_handle_ldr_update(v) )
>> +            return X86EMUL_UNHANDLEABLE;
>> +        break;
>> +
>> +    case APIC_DFR:
>> +        if ( avic_handle_dfr_update(v) )
>> +            return X86EMUL_UNHANDLEABLE;
>> +        break;
>> +
>> +    default:
>> +        break;
> This default case is unnecessary.
>
>> +void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
>> +{
>> +    struct vcpu *curr = current;
>> +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>> +    u32 offset = vmcb->exitinfo1 & 0xFF0;
>> +    u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
> bool?
>
>> +    if ( avic_is_trap(offset) )
>> +    {
>> +        /* Handling AVIC Trap (intercept right after the access). */
>> +        if ( !rw )
>> +        {
>> +            /*
>> +             * If a read trap happens, the CPU microcode does not
>> +             * implement the spec.
>> +             */
>> +            gprintk(XENLOG_ERR, "%s: Invalid #VMEXIT due to trap read (%#x)\n",
>> +                    __func__, offset);
>> +            domain_crash(curr->domain);
>> +        }
>> +
>> +        if ( avic_unaccel_trap_write(curr) != X86EMUL_OKAY )
> ITYM "else if" here.
>
>> +        {
>> +            gprintk(XENLOG_ERR, "%s: Failed to handle trap write (%#x)\n",
>> +                    __func__, offset);
>> +            domain_crash(curr->domain);
>> +        }
>> +    }
>> +    else
>> +        /* Handling AVIC Fault (intercept before the access). */
>> +        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
>> +                                 X86_EVENT_NO_EC);
> What's the rationale behind having chosen this function? I don't think it is
> supposed to be called from outside the VM event code.

We wanted to handle the AVIC fault by emulating the instruction causing it.
Would this be better suited ?

         struct hvm_emulate_ctxt ctx;

         hvm_emulate_init_once(&ctx, NULL, guest_cpu_user_regs());
         rc = hvm_emulate_one(&ctx);

         switch( rc )
         {
         case X86EMUL_UNHANDLABLE:
         case X86EMUL_UNRECOGNIZED:
                 hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
                 break;

         case X86EMUL_EXCEPTION:
                 hvm_inject_event(&ctxt.ctxt.event);
                 break;
         }

         hvm_emulate_writeback(&ctxt);

Thanks,
Janak

>
>> +    return;
>> +}
> Please omit such redundant return statements.
>
> Jan
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Andrew Cooper]

>>> +        {
>>> +            gprintk(XENLOG_ERR, "%s: Failed to handle trap write
>>> (%#x)\n",
>>> +                    __func__, offset);
>>> +            domain_crash(curr->domain);
>>> +        }
>>> +    }
>>> +    else
>>> +        /* Handling AVIC Fault (intercept before the access). */
>>> +        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
>>> +                                 X86_EVENT_NO_EC);
>> What's the rationale behind having chosen this function? I don't
>> think it is
>> supposed to be called from outside the VM event code.
>
> We wanted to handle the AVIC fault by emulating the instruction
> causing it.
> Would this be better suited ?

Almost.

The purpose of the validate function is to fix an inherent race
condition which occurs with a vmexit.

After a vmexit, rereading the instruction for emulation is inherently
racy, and a malicious VM could rewrite the instruction stream while the
vmexit is occuring.  As a result, we provide a validate function to
check that the instruction decoded matches one which plausibly broke for
emulation here.

Therefore, you want a validate function which checks that the
instruction has a memory operand, and that it falls within the 4k region
which maps the APIC registers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Jan Beulich]

>>> On 30.05.18 at 01:33, <andrew.cooper3@citrix.com> wrote:
>> Would this be better suited ?
> 
> Almost.
> 
> The purpose of the validate function is to fix an inherent race
> condition which occurs with a vmexit.
> 
> After a vmexit, rereading the instruction for emulation is inherently
> racy, and a malicious VM could rewrite the instruction stream while the
> vmexit is occuring.  As a result, we provide a validate function to
> check that the instruction decoded matches one which plausibly broke for
> emulation here.
> 
> Therefore, you want a validate function which checks that the
> instruction has a memory operand, and that it falls within the 4k region
> which maps the APIC registers.

The validate hook is called right after decode, i.e. before operands have
been evaluated, so the latter part of what you suggest cannot be done.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Natarajan, Janakarajan]

On 5/30/2018 2:24 AM, Jan Beulich wrote:
>>>> On 30.05.18 at 01:33, <andrew.cooper3@citrix.com> wrote:
>>> Would this be better suited ?
>> Almost.
>>
>> The purpose of the validate function is to fix an inherent race
>> condition which occurs with a vmexit.
>>
>> After a vmexit, rereading the instruction for emulation is inherently
>> racy, and a malicious VM could rewrite the instruction stream while the
>> vmexit is occuring.  As a result, we provide a validate function to
>> check that the instruction decoded matches one which plausibly broke for
>> emulation here.
>>
>> Therefore, you want a validate function which checks that the
>> instruction has a memory operand, and that it falls within the 4k region
>> which maps the APIC registers.
> The validate hook is called right after decode, i.e. before operands have
> been evaluated, so the latter part of what you suggest cannot be done.

So check to see if there is a memory operand. Something along the lines of:

bool has_memory_operand(const struct x86_emulate_state *state,
                                                    const struct 
x86_emulate_ctxt *ctxt)
{
         if ( (state->desc & DstMask) == DstMem )
                 return true;

         return false;
}

Thanks,
Janak



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 07/10] x86/SVM: Add interrupt management code via AVIC

[Natarajan, Janakarajan]

On 5/17/2018 9:50 AM, Jan Beulich wrote:
>>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@amd.com> wrote:
>> +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 interrupt is disabled, do not ignore the interrupt */
>> +    if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
>> +        return;
> It seems to me that I did comment on this before - I don't think EFLAGS.IF
> should be considered here:
>
>> +    if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
>> +        return;
> Latching the interrupt into IRR ought to keep it pending until the guest sets
> EFLAGS.IF again.

My apologies. I thought I removed that if check. I'll remove it.

Thanks,
Janak



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 05/10] x86/SVM: Add AVIC vmexit handlers

[Andrew Cooper]

On 30/05/2018 19:30, Natarajan, Janakarajan wrote:
> On 5/30/2018 2:24 AM, Jan Beulich wrote:
>>>>> On 30.05.18 at 01:33, <andrew.cooper3@citrix.com> wrote:
>>>> Would this be better suited ?
>>> Almost.
>>>
>>> The purpose of the validate function is to fix an inherent race
>>> condition which occurs with a vmexit.
>>>
>>> After a vmexit, rereading the instruction for emulation is inherently
>>> racy, and a malicious VM could rewrite the instruction stream while the
>>> vmexit is occuring.  As a result, we provide a validate function to
>>> check that the instruction decoded matches one which plausibly broke
>>> for
>>> emulation here.
>>>
>>> Therefore, you want a validate function which checks that the
>>> instruction has a memory operand, and that it falls within the 4k
>>> region
>>> which maps the APIC registers.
>> The validate hook is called right after decode, i.e. before operands
>> have
>> been evaluated, so the latter part of what you suggest cannot be done.
>
> So check to see if there is a memory operand. Something along the
> lines of:
>
> bool has_memory_operand(const struct x86_emulate_state *state,
>                                                    const struct
> x86_emulate_ctxt *ctxt)
> {
>         if ( (state->desc & DstMask) == DstMem )
>                 return true;
>
>         return false;
> }

x86_insn_is_mem_access() or x86_insn_is_mem_write() (depending on
whether we ever need to emulate a read).

Here are several we prepared earlier, and they are sadly more
complicated than you'd imagine.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel