Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Andrew Cooper]

On 30/04/2018 22:23, Natarajan, Janakarajan wrote:
>>> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr,
>>> bool valid)
>>> +{
>>> +    struct avic_logical_id_entry *entry, new_entry;
>>> +    u32 dfr = vlapic_read_aligned(vcpu_vlapic(v), APIC_DFR);
>>> +
>>> +    entry = avic_get_logical_id_entry(>domain->arch.hvm_domain.svm,
>>> +  ldr, (dfr == APIC_DFR_FLAT));
>>> +    if (!entry)
>> if ( !entry )
>>
>>> +    return -EINVAL;
>>> +
>>> +    new_entry = *entry;
>>> +    smp_rmb();
>>> +    new_entry.guest_phy_apic_id = g_phy_id;
>>> +    new_entry.valid = valid;
>>> +    *entry = new_entry;
>>> +    smp_wmb();
>> These barriers don't do what you want.  The pattern you are looking for
>> would require an smp_mb() between setting valid and writing things
>> back.  However, that is overkill - all that matters is that the compiler
>> doesn't generate multiple partial updates.
>>
>> 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;
>
> Since it was decided to not use
>
> union ... {
>     uint64_t raw;
>     struct avic_logical_table_entry {
>     
>     
>     };
> };
>
> would smp_mb() make sense here?

Nothing has been decided yet.  I've made an argument for why the
minimalistic approach (as suggested in in the thread hanging off patch
4, which supersedes this) would be correct (and best, IMO) but the
decision as to what code to use is ultimately up to you as the submitter
(subject to it being functionally correct, which is the purpose of review).

In the ACCESS_ONCE() case, all the assignments are strictly dependent on
previous reads, which forces the compiler and pipeline to issue them in
order, so I don't see any reason for an mfence instruction.

~Andrew

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

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Natarajan, Janakarajan]

On 4/13/2018 12:48 PM, Andrew Cooper wrote:

On 04/04/18 00:01, Janakarajan Natarajan wrote:

From: Suravee Suthikulpanit 

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.

This patch also declare vlapic_read_aligned() and vlapic_reg_write()
as non-static to expose them to be used by AVIC.

Signed-off-by: Suravee Suthikulpanit 
Signed-off-by: Janakarajan Natarajan 
---
  xen/arch/x86/hvm/svm/avic.c| 296 +
  xen/arch/x86/hvm/svm/svm.c |   8 +
  xen/arch/x86/hvm/vlapic.c  |   4 +-
  xen/include/asm-x86/hvm/svm/avic.h |   3 +
  xen/include/asm-x86/hvm/svm/vmcb.h |   6 +
  xen/include/asm-x86/hvm/vlapic.h   |   4 +
  6 files changed, 319 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 8108698911..e112469774 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -37,6 +38,8 @@
  
  #define AVIC_VAPIC_BAR_MASK (((1ULL << 40) - 1) << PAGE_SHIFT)
  
+#define AVIC_UNACCEL_ACCESS_OFFSET_MASK0xFF0

+
  /*
   * Note:
   * Currently, svm-avic mode is not supported with nested virtualization.
@@ -101,6 +104,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(>arch.hvm_domain.svm.avic_dfr_mode_lock);

+
  return ret;
   err_out:
  svm_avic_dom_destroy(d);
@@ -181,6 +186,297 @@ 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);

No need for !!.  It is the explicit behaviour of the bool type.


+
+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);
+ 

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Jan Beulich]

>>> On 20.04.18 at 22:02,  wrote:
> On 4/17/2018 7:58 AM, Jan Beulich wrote:
> On 04.04.18 at 01:01,  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_read_aligned(const struct vlapic *vlapic, unsigned int 
>>> offset);
>> If making these non-static is really necessary, they should (name-wise) 
>> become
>> proper pairs of one another, e.g. renamed the former to vlapic_reg_read().
>>
>> Also while here you properly use uint32_t, almost everywhere you use u32.
>> Please switch this throughout the series, and of course for all other fixed
>> width integer types.
> 
> In the case of vmcb.h, where the existing variables use u32, u64 would 
> you want me to have uint32_t,
> uint64_t for the new avic variables?

Ideally, yes (and even more ideally you'd find the time to switch the others
around), but I wouldn't make this a strict requirement (even less so as I'm
not a maintainer of those files anyway) - I can see the consistency
argument.

Jan



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

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Natarajan, Janakarajan]

On 4/17/2018 7:58 AM, Jan Beulich wrote:

On 04.04.18 at 01:01,  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_read_aligned(const struct vlapic *vlapic, unsigned int offset);

If making these non-static is really necessary, they should (name-wise) become
proper pairs of one another, e.g. renamed the former to vlapic_reg_read().

Also while here you properly use uint32_t, almost everywhere you use u32.
Please switch this throughout the series, and of course for all other fixed
width integer types.


In the case of vmcb.h, where the existing variables use u32, u64 would 
you want me to have uint32_t,

uint64_t for the new avic variables?



Jan





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

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Jan Beulich]

>>> On 04.04.18 at 01:01,  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_read_aligned(const struct vlapic *vlapic, unsigned int 
> offset);

If making these non-static is really necessary, they should (name-wise) become
proper pairs of one another, e.g. renamed the former to vlapic_reg_read().

Also while here you properly use uint32_t, almost everywhere you use u32.
Please switch this throughout the series, and of course for all other fixed
width integer types.

Jan



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

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Andrew Cooper]

On 04/04/18 00:01, Janakarajan Natarajan wrote:
> From: Suravee Suthikulpanit 
>
> 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.
>
> This patch also declare vlapic_read_aligned() and vlapic_reg_write()
> as non-static to expose them to be used by AVIC.
>
> Signed-off-by: Suravee Suthikulpanit 
> Signed-off-by: Janakarajan Natarajan 
> ---
>  xen/arch/x86/hvm/svm/avic.c| 296 
> +
>  xen/arch/x86/hvm/svm/svm.c |   8 +
>  xen/arch/x86/hvm/vlapic.c  |   4 +-
>  xen/include/asm-x86/hvm/svm/avic.h |   3 +
>  xen/include/asm-x86/hvm/svm/vmcb.h |   6 +
>  xen/include/asm-x86/hvm/vlapic.h   |   4 +
>  6 files changed, 319 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> index 8108698911..e112469774 100644
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -37,6 +38,8 @@
>  
>  #define AVIC_VAPIC_BAR_MASK (((1ULL << 40) - 1) << PAGE_SHIFT)
>  
> +#define AVIC_UNACCEL_ACCESS_OFFSET_MASK0xFF0
> +
>  /*
>   * Note:
>   * Currently, svm-avic mode is not supported with nested virtualization.
> @@ -101,6 +104,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(>arch.hvm_domain.svm.avic_dfr_mode_lock);
> +
>  return ret;
>   err_out:
>  svm_avic_dom_destroy(d);
> @@ -181,6 +186,297 @@ 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);

No need for !!.  It is the explicit behaviour of the bool type.

> +
> +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:
> 

[Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Janakarajan Natarajan]

From: Suravee Suthikulpanit 

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.

This patch also declare vlapic_read_aligned() and vlapic_reg_write()
as non-static to expose them to be used by AVIC.

Signed-off-by: Suravee Suthikulpanit 
Signed-off-by: Janakarajan Natarajan 
---
 xen/arch/x86/hvm/svm/avic.c| 296 +
 xen/arch/x86/hvm/svm/svm.c |   8 +
 xen/arch/x86/hvm/vlapic.c  |   4 +-
 xen/include/asm-x86/hvm/svm/avic.h |   3 +
 xen/include/asm-x86/hvm/svm/vmcb.h |   6 +
 xen/include/asm-x86/hvm/vlapic.h   |   4 +
 6 files changed, 319 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 8108698911..e112469774 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -37,6 +38,8 @@
 
 #define AVIC_VAPIC_BAR_MASK (((1ULL << 40) - 1) << PAGE_SHIFT)
 
+#define AVIC_UNACCEL_ACCESS_OFFSET_MASK0xFF0
+
 /*
  * Note:
  * Currently, svm-avic mode is not supported with nested virtualization.
@@ -101,6 +104,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(>arch.hvm_domain.svm.avic_dfr_mode_lock);
+
 return ret;
  err_out:
 svm_avic_dom_destroy(d);
@@ -181,6 +186,297 @@ 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 struct