Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
On 12/22/16 18:16, Jan Beulich wrote: On 19.09.16 at 07:52,wrote: +int svm_avic_init_vcpu(struct vcpu *v) +{ +struct vlapic *vlapic = vcpu_vlapic(v); +struct arch_svm_struct *s = >arch.hvm_svm; + +if ( svm_avic ) +s->avic_bk_pg = vlapic->regs_page; Why this copying? Can't consuming code not use vlapic->regs_page? Hm.. good point. I'll get rid of avic_bk_pg and just use the regs_page. Thanks, Suravee ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
>>> On 19.09.16 at 07:52,wrote: > +int svm_avic_init_vcpu(struct vcpu *v) > +{ > +struct vlapic *vlapic = vcpu_vlapic(v); > +struct arch_svm_struct *s = >arch.hvm_svm; > + > +if ( svm_avic ) > +s->avic_bk_pg = vlapic->regs_page; Why this copying? Can't consuming code not use vlapic->regs_page? > +int svm_avic_init_vmcb(struct vcpu *v) > +{ > +paddr_t ma; > +u32 apic_id_reg; > +struct arch_svm_struct *s = >arch.hvm_svm; > +struct vmcb_struct *vmcb = s->vmcb; > +struct svm_domain *d = >domain->arch.hvm_domain.svm; > +struct svm_avic_phy_ait_entry entry; > + > +if ( !svm_avic ) > +return 0; > + > +vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK; > +ma = d->avic_log_ait_mfn; > +vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +ma = d->avic_phy_ait_mfn; > +vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX; > + > +dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n", > + __func__, (unsigned long long)vmcb->avic_bk_pg_pa, > + (unsigned long long) vmcb->avic_log_apic_id, > + (unsigned long long) vmcb->avic_phy_apic_id); > + > + > +apic_id_reg = *avic_get_bk_page_entry(v, APIC_ID); > +s->avic_phy_id_cache = avic_get_phy_ait_entry(v, apic_id_reg >> 24); > +if ( !s->avic_phy_id_cache ) > +return -EINVAL; > + > +entry = *(s->avic_phy_id_cache); > +smp_rmb(); > +entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa >> 12) & 0xff; Please avoid such open coded constants. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
On 11/17/16 11:18, Konrad Rzeszutek Wilk wrote: On Thu, Nov 17, 2016 at 10:05:58AM -0600, Suravee Suthikulpanit wrote: Konrad, Thanks for the review comments. Got one quick question below. On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote: +int svm_avic_init_vmcb(struct vcpu *v) +{ +paddr_t ma; +u32 apic_id_reg; +struct arch_svm_struct *s = >arch.hvm_svm; +struct vmcb_struct *vmcb = s->vmcb; +struct svm_domain *d = >domain->arch.hvm_domain.svm; +struct svm_avic_phy_ait_entry entry; + +if ( !svm_avic ) +return 0; + +vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK; +ma = d->avic_log_ait_mfn; +vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; +ma = d->avic_phy_ait_mfn; +vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; +vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX; + +dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n", I think you can drop the 'SVM:' part. The __func__ gives enough details. + __func__, (unsigned long long)vmcb->avic_bk_pg_pa, + (unsigned long long) vmcb->avic_log_apic_id, + (unsigned long long) vmcb->avic_phy_apic_id); Is this also part of the keyboard handler? Perhaps that information should be presented there? I'm not sure what you mean by keyboard handler. I assume you mean the spacing for the indentation the front should align with above line? Well I would ditch them. And if you really want them then make the keyboard handler, aka svm_vmcb_dump function. Thanks, S. Ahh.. the xl debug-keys stuff. Got it. No problem. I can ditch this. S ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
On Thu, Nov 17, 2016 at 10:55:52AM -0600, Suravee Suthikulpanit wrote: > Konrad, > > On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote: > > > + > > > > +/* NOTE: Current max index allowed for physical APIC ID table is 255 */ > > > > +#define AVIC_PHY_APIC_ID_MAX0xFF > > > > + > > > > +#define AVIC_DOORBELL 0xc001011b > > > > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF) > > > > +#define AVIC_APIC_BAR_MASK 0xFF000ULL > > > > + > > > > +bool_t svm_avic = 0; > > > > +boolean_param("svm-avic", svm_avic); > > Why do you want to have it disabled by default? > > svm-avic has not yet fully supported with nested virtualization yet. So, I > didn't want to enable this by default. However, when everything is stable, I > am planning to enable this by default. Ah, could you put an comment above the param to explain this? Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
On Thu, Nov 17, 2016 at 10:05:58AM -0600, Suravee Suthikulpanit wrote: > Konrad, > > Thanks for the review comments. Got one quick question below. > > On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote: > > > +int svm_avic_init_vmcb(struct vcpu *v) > > > > +{ > > > > +paddr_t ma; > > > > +u32 apic_id_reg; > > > > +struct arch_svm_struct *s = >arch.hvm_svm; > > > > +struct vmcb_struct *vmcb = s->vmcb; > > > > +struct svm_domain *d = >domain->arch.hvm_domain.svm; > > > > +struct svm_avic_phy_ait_entry entry; > > > > + > > > > +if ( !svm_avic ) > > > > +return 0; > > > > + > > > > +vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK; > > > > +ma = d->avic_log_ait_mfn; > > > > +vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > > > > +ma = d->avic_phy_ait_mfn; > > > > +vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > > > > +vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX; > > > > + > > > > +dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n", > > I think you can drop the 'SVM:' part. The __func__ gives enough details. > > > > > > + __func__, (unsigned long long)vmcb->avic_bk_pg_pa, > > > > + (unsigned long long) vmcb->avic_log_apic_id, > > > > + (unsigned long long) vmcb->avic_phy_apic_id); > > Is this also part of the keyboard handler? Perhaps that information should > > be presented there? > > I'm not sure what you mean by keyboard handler. I assume you mean the > spacing for the indentation the front should align with above line? Well I would ditch them. And if you really want them then make the keyboard handler, aka svm_vmcb_dump function. > > Thanks, > S. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
Konrad, On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote: + > +/* NOTE: Current max index allowed for physical APIC ID table is 255 */ > +#define AVIC_PHY_APIC_ID_MAX0xFF > + > +#define AVIC_DOORBELL 0xc001011b > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF) > +#define AVIC_APIC_BAR_MASK 0xFF000ULL > + > +bool_t svm_avic = 0; > +boolean_param("svm-avic", svm_avic); Why do you want to have it disabled by default? svm-avic has not yet fully supported with nested virtualization yet. So, I didn't want to enable this by default. However, when everything is stable, I am planning to enable this by default. Thanks, Suravee ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
Konrad, Thanks for the review comments. Got one quick question below. On 10/12/16 15:02, Konrad Rzeszutek Wilk wrote: +int svm_avic_init_vmcb(struct vcpu *v) > +{ > +paddr_t ma; > +u32 apic_id_reg; > +struct arch_svm_struct *s = >arch.hvm_svm; > +struct vmcb_struct *vmcb = s->vmcb; > +struct svm_domain *d = >domain->arch.hvm_domain.svm; > +struct svm_avic_phy_ait_entry entry; > + > +if ( !svm_avic ) > +return 0; > + > +vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK; > +ma = d->avic_log_ait_mfn; > +vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +ma = d->avic_phy_ait_mfn; > +vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX; > + > +dprintk(XENLOG_DEBUG, "SVM: %s: bpa=%#llx, lpa=%#llx, ppa=%#llx\n", I think you can drop the 'SVM:' part. The __func__ gives enough details. > + __func__, (unsigned long long)vmcb->avic_bk_pg_pa, > + (unsigned long long) vmcb->avic_log_apic_id, > + (unsigned long long) vmcb->avic_phy_apic_id); Is this also part of the keyboard handler? Perhaps that information should be presented there? I'm not sure what you mean by keyboard handler. I assume you mean the spacing for the indentation the front should align with above line? Thanks, S. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code
. snip.. > diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c > new file mode 100644 > index 000..70bac69 > --- /dev/null > +++ b/xen/arch/x86/hvm/svm/avic.c > @@ -0,0 +1,217 @@ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* NOTE: Current max index allowed for physical APIC ID table is 255 */ > +#define AVIC_PHY_APIC_ID_MAX0xFF > + > +#define AVIC_DOORBELL 0xc001011b > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF) > +#define AVIC_APIC_BAR_MASK 0xFF000ULL > + > +bool_t svm_avic = 0; static ? And also make it 'bool' > +boolean_param("svm-avic", svm_avic); > + > +static struct svm_avic_phy_ait_entry * This name resolves to 'SVM AVIC Physical APIC ID Table Entry' That 'ait' keeps throwing me off as it sounds like 'eat' to me. Perhaps 'avic_phy_apic_id_ent' ? In other words s/ait/apic_id/ and s/svm_avic/avic/ followed by s/_id_entry/id_ent/ ? > +avic_get_phy_ait_entry(struct vcpu *v, int index) > +{ > +struct svm_avic_phy_ait_entry *avic_phy_ait; > +struct svm_domain *d = >domain->arch.hvm_domain.svm; > + > +if ( !d->avic_phy_ait_mfn ) > +return NULL; > + > +/** > +* Note: APIC ID = 0xff is used for broadcast. > +* APIC ID > 0xff is reserved. > +*/ > +if ( index >= 0xff ) > +return NULL; > + > +avic_phy_ait = mfn_to_virt(d->avic_phy_ait_mfn); > + > +return _phy_ait[index]; > +} > + > +/*** > + * AVIC APIs > + */ > +int svm_avic_dom_init(struct domain *d) > +{ > +int ret = 0; > +struct page_info *pg; > +unsigned long mfn; > + > +if ( !svm_avic ) > +return 0; > + > +/** > + * Note: > + * AVIC hardware walks the nested page table to check permissions, > + * but does not use the SPA address specified in the leaf page > + * table entry since it uses address in the AVIC_BACKING_PAGE pointer > + * field of the VMCB. Therefore, we set up a dummy page here _mfn(0). s/here/for APIC/ > + */ > +if ( !d->arch.hvm_domain.svm.avic_access_page_done ) > +{ > +set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), > + _mfn(0), PAGE_ORDER_4K, > + p2m_get_hostp2m(d)->default_access); > +d->arch.hvm_domain.svm.avic_access_page_done = true; > +} > + > +/* Init AVIC log APIC ID table */ s/log/logical/ > +pg = alloc_domheap_page(d, MEMF_no_owner); > +if ( !pg ) > +{ > +dprintk(XENLOG_ERR, "alloc AVIC logical APIC ID table error: %d\n", > +d->domain_id); > +ret = -ENOMEM; > +goto err_out; > +} > +mfn = page_to_mfn(pg); > +clear_domain_page(_mfn(mfn)); > +d->arch.hvm_domain.svm.avic_log_ait_mfn = mfn; > + > +/* Init AVIC phy APIC ID table */ physical ? > +pg = alloc_domheap_page(d, MEMF_no_owner); > +if ( !pg ) > +{ > +dprintk(XENLOG_ERR, "alloc AVIC physical APIC ID table error: %d\n", > +d->domain_id); > +ret = -ENOMEM; > +goto err_out; > +} > +mfn = page_to_mfn(pg); > +clear_domain_page(_mfn(mfn)); > +d->arch.hvm_domain.svm.avic_phy_ait_mfn = mfn; > + > +return ret; > +err_out: > +svm_avic_dom_destroy(d); > +return ret; > +} > + .. snip.. > +int svm_avic_init_vmcb(struct vcpu *v) > +{ > +paddr_t ma; > +u32 apic_id_reg; > +struct arch_svm_struct *s = >arch.hvm_svm; > +struct vmcb_struct *vmcb = s->vmcb; > +struct svm_domain *d = >domain->arch.hvm_domain.svm; > +struct svm_avic_phy_ait_entry entry; > + > +if ( !svm_avic ) > +return 0; > + > +vmcb->avic_bk_pg_pa = page_to_maddr(s->avic_bk_pg) & AVIC_HPA_MASK; > +ma = d->avic_log_ait_mfn; > +vmcb->avic_log_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +ma = d->avic_phy_ait_mfn; s/ait/apic_id/ ? > +vmcb->avic_phy_apic_id = (ma << PAGE_SHIFT) & AVIC_HPA_MASK; > +vmcb->avic_phy_apic_id |= AVIC_PHY_APIC_ID_MAX; > + After reading the spec these tables make a bit more sense - one to translate logical APIC -> physical APIC id, and the last one to translate to host APIC. It may be good to include just an simple ASCII flow of how say IPI for a two vCPU guest would go? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
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> --- > 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 000..70bac69 > --- /dev/null > +++ b/xen/arch/x86/hvm/svm/avic.c > @@ -0,0 +1,217 @@ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include 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_MAX0xFF > + > +#define AVIC_DOORBELL 0xc001011b > +#define AVIC_HPA_MASK ~((0xFFFULL << 52) || 0xFFF) > +#define AVIC_APIC_BAR_MASK 0xFF000ULL > + > +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 = >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 _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)); > +