Re: [Xen-devel] [PATCH 2/8] x86/HVM/SVM: Add AVIC initialization code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/18 00:01, Janakarajan Natarajan wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>
> Introduce AVIC base initialization code. This includes:
>     * Setting up per-VM data structures.
>     * Setting up per-vCPU data structure.
>     * Initializing AVIC-related VMCB bit fields.
>
> This patch also introduces a new Xen parameter (svm-avic),
> which can be used to enable/disable AVIC support.
> Currently, this svm-avic is disabled by default.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@xxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/Makefile      |   1 +
>  xen/arch/x86/hvm/svm/avic.c        | 191 
> +++++++++++++++++++++++++++++++++++++
>  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 |  36 +++++++
>  xen/include/asm-x86/hvm/svm/svm.h  |   2 +
>  xen/include/asm-x86/hvm/svm/vmcb.h |  17 ++++
>  8 files changed, 261 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..8108698911
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,191 @@
> +/*
> + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) 
> support
> + * Copyright (c) 2016, 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    0xFF
> +
> +#define AVIC_VAPIC_BAR_MASK     (((1ULL << 40) - 1) << PAGE_SHIFT)

This constant should hopefully be stale (following the debugging), and
should be dropped.

> +
> +/*
> + * 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 = 0;
> +
> +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);

Please also have:

if ( index >= AVIC_PHY_APIC_ID_MAX )
    return NULL; /* Avoid out-of-bounds access in release builds. */

> +
> +    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 _mfn(0).
> +     */
> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> +                       _mfn(0), PAGE_ORDER_4K,
> +                       p2m_get_hostp2m(d)->default_access);

Is default access right here, rather than blanket R/W?  IIRC, the frame
is magic in the pipeline and causes redirections to the APIC handling,
so the permissions of the page don't take effect.

> +
> +    /* Init AVIC logical APIC ID table */
> +    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    if ( !pg )
> +    {
> +        ret = -ENOMEM;
> +        goto err_out;
> +    }
> +    clear_domain_page(_mfn(page_to_mfn(pg)));

You'll need to rebase your changes onto the latest staging.  Julien
finished the tree-wide cleanup of page_to_mfn() and changed its type, so
you'll need to drop _mfn() wrappers, or insert mfn_x() wrappers.

> +    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(_mfn(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 = 0;
> +        d->arch.hvm_domain.svm.avic_physical_id_table = 0;
> +    }
> +
> +    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 = 0;
> +        d->arch.hvm_domain.svm.avic_logical_id_table = 0;
> +    }
> +}
> +
> +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;
> +    unsigned long tmp;
> +    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 = (struct avic_physical_id_entry 
> *)&tmp;

This is still broken.  Anywhere where type punning like this is going on
needs fixing.

> +
> +    if ( !svm_avic || !has_vlapic(v->domain) )
> +        return 0;
> +
> +    if ( !vlapic || !vlapic->regs_page )
> +        return -EINVAL;
> +
> +    apic_id = vlapic_read_aligned(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 = 
> domain_page_map_to_mfn(d->avic_logical_id_table) << PAGE_SHIFT;
> +    vmcb->avic_physical_id_table_pa = 
> domain_page_map_to_mfn(d->avic_physical_id_table) << PAGE_SHIFT;
> +
> +    /* Set Physical ID Table Pointer [7:0] to max apic id of the domain */
> +    vmcb->avic_logical_id_table_pa &= ~AVIC_PHY_APIC_ID_MAX;
> +    vmcb->avic_physical_id_table_pa |= (v->domain->max_vcpus * 2) & 0xFF;
> +
> +    tmp = read_atomic((u64*)(s->avic_last_phy_id));
> +    entry->bk_pg_ptr_mfn = (vmcb->avic_bk_pg_pa) >> PAGE_SHIFT;
> +    entry->is_running = 0;
> +    entry->valid = 1;
> +    write_atomic((u64*)(s->avic_last_phy_id), tmp);

This is an init function, and it will be called sequentially for each
allocated vcpu.  The guest is guaranteed not to be executing, so don't
need to have any special interlocking for s->avic_last_phy_id

> +
> +    vmcb->avic_vapic_bar = APIC_DEFAULT_PHYS_BASE & AVIC_VAPIC_BAR_MASK;
> +    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 8538232f68..b445f59ada 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>
> @@ -1225,11 +1226,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)
> @@ -1694,6 +1696,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");
> @@ -1705,6 +1710,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 1b9f00a0e4..a4472200a6 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..32bb9a91e8
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -0,0 +1,36 @@
> +#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,
> +};
> +
> +struct __packed avic_logical_id_entry {
> +    u32 guest_phy_apic_id : 8;
> +    u32 res               : 23;
> +    u32 valid             : 1;
> +};
> +
> +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;
> +};

For help with your type-punning fixes in later patches, you'll want
these as a union.

typedef union avic_logical_id_entry {
    uint32_t raw;
    struct {
        ...
    };
} avic_logical_id_entry_t;

~Andrew

> +
> +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..386aad2260 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -500,6 +500,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.
> +     */
> +    struct avic_logical_id_entry *avic_logical_id_table;
> +    struct page_info *avic_logical_id_table_pg;
>  };
>  
>  struct arch_svm_struct {
> @@ -533,6 +548,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);


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