|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
On Mon, Sep 19, 2016 at 12:52:44AM -0500, Suravee Suthikulpanit wrote:
> Introduce AVIC base initialization code. This includes:
> * Setting up per-VM data structures.
> * Setting up per-vCPU data structure.
> * Initializing AVIC-related VMCB bit fields.
>
> This patch also introduces a new Xen parameter (svm-avic),
> which can be used to enable/disable AVIC support.
> Currently, this svm-avic is disabled by default.
Oddly enough you didn't CC the SVM maintainer: Boris Ostrovsky on all
these patches.
Adding him here.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
> xen/arch/x86/hvm/svm/Makefile | 1 +
> xen/arch/x86/hvm/svm/avic.c | 217
> +++++++++++++++++++++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 17 ++-
> xen/arch/x86/hvm/svm/vmcb.c | 3 +
> xen/include/asm-x86/hvm/svm/avic.h | 40 +++++++
> xen/include/asm-x86/hvm/svm/svm.h | 2 +
> xen/include/asm-x86/hvm/svm/vmcb.h | 10 ++
> 7 files changed, 289 insertions(+), 1 deletion(-)
> create mode 100644 xen/arch/x86/hvm/svm/avic.c
> create mode 100644 xen/include/asm-x86/hvm/svm/avic.h
>
> diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile
> index 760d295..e0e4a59 100644
> --- a/xen/arch/x86/hvm/svm/Makefile
> +++ b/xen/arch/x86/hvm/svm/Makefile
> @@ -1,4 +1,5 @@
> obj-y += asid.o
> +obj-y += avic.o
> obj-y += emulate.o
> obj-bin-y += entry.o
> obj-y += intr.o
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> new file mode 100644
> index 0000000..70bac69
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -0,0 +1,217 @@
> +#include <xen/domain_page.h>
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <asm/acpi.h>
> +#include <asm/apicdef.h>
> +#include <asm/event.h>
> +#include <asm/p2m.h>
> +#include <asm/page.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/svm/avic.h>
> +#include <asm/hvm/vlapic.h>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/support.h>
Since this a new file could you kindly sort these? Also do you need a copyright
header at the top?
> +
> +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
> +#define AVIC_PHY_APIC_ID_MAX 0xFF
> +
> +#define AVIC_DOORBELL 0xc001011b
> +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF)
> +#define AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
> +
> +bool_t svm_avic = 0;
> +boolean_param("svm-avic", svm_avic);
Why do you want to have it disabled by default?
Also you are missing an patch to the docs/misc/xen-command-line where you would
document what this parameter does.
> +
> +static struct svm_avic_phy_ait_entry *
> +avic_get_phy_ait_entry(struct vcpu *v, int index)
Could I convience you use 'const struct vcpu *v, unsigned int index' ?
> +{
> + struct svm_avic_phy_ait_entry *avic_phy_ait;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> +
> + if ( !d->avic_phy_ait_mfn )
> + return NULL;
> +
> + /**
This '**' is not the standard style. Could you use only one '*' please?
> + * Note: APIC ID = 0xff is used for broadcast.
> + * APIC ID > 0xff is reserved.
> + */
> + if ( index >= 0xff )
> + return NULL;
> +
> + avic_phy_ait = mfn_to_virt(d->avic_phy_ait_mfn);
> +
> + return &avic_phy_ait[index];
> +}
> +
> +/***************************************************************
Ditto
> + * AVIC APIs
> + */
> +int svm_avic_dom_init(struct domain *d)
> +{
> + int ret = 0;
> + struct page_info *pg;
> + unsigned long mfn;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + /**
> + * Note:
> + * AVIC hardware walks the nested page table to check permissions,
> + * but does not use the SPA address specified in the leaf page
> + * table entry since it uses address in the AVIC_BACKING_PAGE pointer
> + * field of the VMCB. Therefore, we set up a dummy page here _mfn(0).
> + */
> + if ( !d->arch.hvm_domain.svm.avic_access_page_done )
> + {
> + set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> + _mfn(0), PAGE_ORDER_4K,
> + p2m_get_hostp2m(d)->default_access);
> + d->arch.hvm_domain.svm.avic_access_page_done = true;
> + }
> +
> + /* Init AVIC log APIC ID table */
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC logical APIC ID table error: %d\n",
How about "%d: AVIC logical .. could not be allocated" ?
> + d->domain_id);
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_log_ait_mfn = mfn;
> +
> + /* Init AVIC phy APIC ID table */
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + if ( !pg )
> + {
> + dprintk(XENLOG_ERR, "alloc AVIC physical APIC ID table error: %d\n",
> + d->domain_id);
Ditto.
You could also use gdprintk instead?
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + mfn = page_to_mfn(pg);
> + clear_domain_page(_mfn(mfn));
> + d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn;
> +
> + return ret;
> +err_out:
Add an extra space in front of the label please.
> + svm_avic_dom_destroy(d);
> + return ret;
> +}
> +
> +void svm_avic_dom_destroy(struct domain *d)
> +{
> + if ( !svm_avic )
> + return;
> +
> + if ( d->arch.hvm_domain.svm.avic_phy_ait_mfn )
> + {
> +
> free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_phy_ait_mfn));
> + d->arch.hvm_domain.svm.avic_phy_ait_mfn = 0;
> + }
> +
> + if ( d->arch.hvm_domain.svm.avic_log_ait_mfn )
> + {
> +
> free_domheap_page(mfn_to_page(d->arch.hvm_domain.svm.avic_log_ait_mfn));
> + d->arch.hvm_domain.svm.avic_log_ait_mfn = 0;
> + }
> +}
> +
> +/**
> + * Note: At this point, vlapic->regs_page is already initialized.
> + */
> +int svm_avic_init_vcpu(struct vcpu *v)
> +{
> + struct vlapic *vlapic = vcpu_vlapic(v);
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( svm_avic )
> + s->avic_bk_pg = vlapic->regs_page;
> + return 0;
> +}
> +
> +void svm_avic_destroy_vcpu(struct vcpu *v)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + if ( svm_avic && s->avic_bk_pg )
> + s->avic_bk_pg = NULL;
> +}
> +
> +bool_t svm_avic_vcpu_enabled(struct vcpu *v)
bool please
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> +
> + return ( svm_avic && vmcb->_vintr.fields.avic_enable);
Could you drop the () please?
> +}
> +
> +static inline u32 *
> +avic_get_bk_page_entry(struct vcpu *v, u32 offset)
const on 'struct vcpu' please?
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct page_info *pg = s->avic_bk_pg;
> + char *tmp;
> +
> + if ( !pg )
> + return NULL;
> +
> + tmp = (char *) page_to_virt(pg);
Extra space not needed.
> + return (u32*)(tmp+offset);
Perhaps:
return (u32 *)(tmp + offset);
?
> +}
> +
> +void svm_avic_update_vapic_bar(struct vcpu *v, uint64_t data)
> +{
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> +
> + s->vmcb->avic_vapic_bar = data & AVIC_APIC_BAR_MASK;
> + s->vmcb->cleanbits.fields.avic = 0;
> +}
> +
> +int svm_avic_init_vmcb(struct vcpu *v)
> +{
> + paddr_t ma;
> + u32 apic_id_reg;
> + struct arch_svm_struct *s = &v->arch.hvm_svm;
> + struct vmcb_struct *vmcb = s->vmcb;
> + struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> + struct svm_avic_phy_ait_entry entry;
> +
> + if ( !svm_avic )
> + return 0;
> +
> + vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK;
> + ma = d->avic_log_ait_mfn;
> + vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + ma = d->avic_phy_ait_mfn;
> + vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK;
> + vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX;
> +
> + dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n",
I think you can drop the 'SVM:' part. The __func__ gives enough details.
> + __func__, (unsigned long long)vmcb->avic_bk_pg_pa,
> + (unsigned long long) vmcb->avic_log_apic_id,
> + (unsigned long long) vmcb->avic_phy_apic_id);
Is this also part of the keyboard handler? Perhaps that information should
be presented there?
> +
> +
> + apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID);
Um, what if it returns NULL? You would derefencing NULL. Perhaps check for that
first?
> + s->avic_phy_id_cache = avic_get_phy_ait_entry(v, apic_id_reg >> 24);
> + if ( !s->avic_phy_id_cache )
> + return -EINVAL;
You don't want to unroll the entries that got set earlier?
avic_[bk_pg_pa,phy_apic_id,log_apic_id] ?
> +
> + entry = *(s->avic_phy_id_cache);
> + smp_rmb();
> + entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa >> 12) & 0xffffffffff;
Should the 0xff.. have some soft of macro?
Also the 12 looks suspicious like some other macro value.
> + entry.is_running= 0;
Missing space.
> + entry.valid = 1;
> + *(s->avic_phy_id_cache) = entry;
> + smp_wmb();
> +
> + svm_avic_update_vapic_bar(v, APIC_DEFAULT_PHYS_BASE);
> +
> + vmcb->_vintr.fields.avic_enable = 1;
> +
> + return 0;
> +}
Please also add:
/*
* Local variables:
* mode: C
* c-file-style: "BSD"
* c-basic-offset: 4
* tab-width: 4
* indent-tabs-mode: nil
* End:
*/
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 679e615..bcb7df4 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -48,6 +48,7 @@
> #include <asm/hvm/svm/asid.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/hvm/svm/vmcb.h>
> +#include <asm/hvm/svm/avic.h>
> #include <asm/hvm/svm/emulate.h>
> #include <asm/hvm/svm/intr.h>
> #include <asm/hvm/svm/svmdebug.h>
> @@ -1164,11 +1165,12 @@ void svm_host_osvw_init()
>
> static int svm_domain_initialise(struct domain *d)
> {
> - return 0;
> + return svm_avic_dom_init(d);
> }
>
> static void svm_domain_destroy(struct domain *d)
> {
> + svm_avic_dom_destroy(d);
> }
>
> static int svm_vcpu_initialise(struct vcpu *v)
> @@ -1181,6 +1183,13 @@ static int svm_vcpu_initialise(struct vcpu *v)
>
> v->arch.hvm_svm.launch_core = -1;
>
> + if ( (rc = svm_avic_init_vcpu(v)) != 0 )
> + {
> + dprintk(XENLOG_WARNING,
> + "Failed to initiazlize AVIC vcpu.\n");
> + return rc;
> + }
> +
> if ( (rc = svm_create_vmcb(v)) != 0 )
> {
> dprintk(XENLOG_WARNING,
> @@ -1200,6 +1209,7 @@ static int svm_vcpu_initialise(struct vcpu *v)
>
> static void svm_vcpu_destroy(struct vcpu *v)
> {
> + svm_avic_destroy_vcpu(v);
> vpmu_destroy(v);
> svm_destroy_vmcb(v);
> passive_domain_destroy(v);
> @@ -1464,6 +1474,7 @@ const struct hvm_function_table * __init start_svm(void)
> P(cpu_has_svm_decode, "DecodeAssists");
> P(cpu_has_pause_filter, "Pause-Intercept Filter");
> P(cpu_has_tsc_ratio, "TSC Rate MSR");
> + P(cpu_has_svm_avic, "AVIC");
> #undef P
>
> if ( !printed )
> @@ -1809,6 +1820,10 @@ static int svm_msr_write_intercept(unsigned int msr,
> uint64_t msr_content)
>
> switch ( msr )
> {
> + case MSR_IA32_APICBASE:
> + if ( svm_avic_vcpu_enabled(v) )
> + svm_avic_update_vapic_bar(v, msr_content);
> + break;
> case MSR_IA32_SYSENTER_CS:
> case MSR_IA32_SYSENTER_ESP:
> case MSR_IA32_SYSENTER_EIP:
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 9ea014f..9ee7fc7 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -28,6 +28,7 @@
> #include <asm/msr-index.h>
> #include <asm/p2m.h>
> #include <asm/hvm/support.h>
> +#include <asm/hvm/svm/avic.h>
> #include <asm/hvm/svm/svm.h>
> #include <asm/hvm/svm/svmdebug.h>
>
> @@ -225,6 +226,8 @@ static int construct_vmcb(struct vcpu *v)
> vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_PAUSE;
> }
>
> + svm_avic_init_vmcb(v);
> +
> vmcb->cleanbits.bytes = 0;
>
> return 0;
> diff --git a/xen/include/asm-x86/hvm/svm/avic.h
> b/xen/include/asm-x86/hvm/svm/avic.h
> new file mode 100644
> index 0000000..9508486
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/svm/avic.h
> @@ -0,0 +1,40 @@
> +#ifndef _SVM_AVIC_H_
> +#define _SVM_AVIC_H_
> +
> +enum avic_incmp_ipi_err_code {
> + AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE,
> + AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN,
> + AVIC_INCMP_IPI_ERR_INV_TARGET,
> + AVIC_INCMP_IPI_ERR_INV_BK_PAGE,
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_log_ait_entry {
> + u32 guest_phy_apic_id : 8;
> + u32 res : 23;
> + u32 valid : 1;
> +};
> +
> +struct __attribute__ ((__packed__))
> +svm_avic_phy_ait_entry {
> + u64 host_phy_apic_id : 8;
> + u64 res1 : 4;
> + u64 bk_pg_ptr : 40;
> + u64 res2 : 10;
> + u64 is_running : 1;
> + u64 valid : 1;
> +};
> +
> +extern bool_t svm_avic;
> +
> +int svm_avic_dom_init(struct domain *d);
> +void svm_avic_dom_destroy(struct domain *d);
> +
> +int svm_avic_init_vcpu(struct vcpu *v);
> +void svm_avic_destroy_vcpu(struct vcpu *v);
> +bool_t svm_avic_vcpu_enabled(struct vcpu *v);
> +
> +void svm_avic_update_vapic_bar(struct vcpu *v,uint64_t data);
> +int svm_avic_init_vmcb(struct vcpu *v);
> +
> +#endif /* _SVM_AVIC_H_ */
> diff --git a/xen/include/asm-x86/hvm/svm/svm.h
> b/xen/include/asm-x86/hvm/svm/svm.h
> index c954b7e..fea61bb 100644
> --- a/xen/include/asm-x86/hvm/svm/svm.h
> +++ b/xen/include/asm-x86/hvm/svm/svm.h
> @@ -81,6 +81,7 @@ extern u32 svm_feature_flags;
> #define SVM_FEATURE_FLUSHBYASID 6 /* TLB flush by ASID support */
> #define SVM_FEATURE_DECODEASSISTS 7 /* Decode assists support */
> #define SVM_FEATURE_PAUSEFILTER 10 /* Pause intercept filter support */
> +#define SVM_FEATURE_AVIC 13 /* AVIC support */
>
> #define cpu_has_svm_feature(f) test_bit(f, &svm_feature_flags)
> #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT)
> @@ -91,6 +92,7 @@ extern u32 svm_feature_flags;
> #define cpu_has_svm_decode cpu_has_svm_feature(SVM_FEATURE_DECODEASSISTS)
> #define cpu_has_pause_filter cpu_has_svm_feature(SVM_FEATURE_PAUSEFILTER)
> #define cpu_has_tsc_ratio cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR)
> +#define cpu_has_svm_avic cpu_has_svm_feature(SVM_FEATURE_AVIC)
>
> #define SVM_PAUSEFILTER_INIT 3000
>
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
> b/xen/include/asm-x86/hvm/svm/vmcb.h
> index 768e9fb..a42c034 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -496,6 +496,12 @@ struct __packed vmcb_struct {
> };
>
> struct svm_domain {
> + u32 avic_max_vcpu_id;
> + bool_t avic_access_page_done;
bool pls
> + paddr_t avic_log_ait_mfn;
> + paddr_t avic_phy_ait_mfn;
> + u32 ldr_reg;
> + u32 ldr_mode;
> };
>
> struct arch_svm_struct {
> @@ -529,6 +535,10 @@ struct arch_svm_struct {
> u64 length;
> u64 status;
> } osvw;
> +
> + /* AVIC APIC backing page */
> + struct page_info *avic_bk_pg;
> + struct svm_avic_phy_ait_entry *avic_phy_id_cache;
> };
>
> struct vmcb_struct *alloc_vmcb(void);
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |