Re: [Xen-devel] [RFC PATCH 5/9] x86/HVM/SVM: Add AVIC initialization code

2016-12-27 Thread Suravee Suthikulpanit



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

2016-12-22 Thread Jan Beulich
>>> 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

2016-11-17 Thread Suravee Suthikulpanit



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

2016-11-17 Thread Konrad Rzeszutek Wilk
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

2016-11-17 Thread Konrad Rzeszutek Wilk
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

2016-11-17 Thread Suravee Suthikulpanit

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

2016-11-17 Thread Suravee Suthikulpanit

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

2016-10-14 Thread Konrad Rzeszutek Wilk
. 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

2016-10-12 Thread Konrad Rzeszutek Wilk
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));
> +