[PATCH v4 0/6] VMX: Properly handle pi descriptor and per-cpu blocking list

[PATCH v4 0/6] VMX: Properly handle pi descriptor and per-cpu blocking list

[Feng Wu]

The current VT-d PI related code may operate incorrectly in the
following scenarios:
1. When the last assigned device is dettached from the domain, all
the PI related hooks are removed then, however, the vCPU can be
blocked, switched to another pCPU, etc, all without the aware of
PI. After the next time we attach another device to the domain,
which makes the PI realted hooks avaliable again, the status
of the pi descriptor is not true. Beside that, the blocking vcpu
may still remain in the per-cpu blocking in this case. Patch [1/6]
and [2/6] handle this.

2. After the domain is destroyed, the the blocking vcpu may also
remain in the per-cpu blocking. Handled in patch [3/6].

3. When IRTE is in posted mode, we don't need to set the irq
affinity for it, since the destination of these interrupts is
vCPU and the vCPU affinity is set during vCPU scheduling. Patch
[5/6] handles this.

4. When a pCPU is unplugged, and there might be vCPUs on its
list. Since the pCPU is offline, those vCPUs might not be woken
up again. [6/6] addresses it.

Feng Wu (6):
  VMX: Statically assign two PI hooks
  VMX: Properly handle pi when all the assigned devices are removed
  VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed
  VMX: Make sure PI is in proper state before install the hooks
  VT-d: No need to set irq affinity for posted format IRTE
  VMX: Fixup PI descritpor when cpu is offline

 xen/arch/x86/hvm/vmx/vmcs.c            |  14 ++---
 xen/arch/x86/hvm/vmx/vmx.c             | 105 ++++++++++++++++++++++++++++++---
 xen/drivers/passthrough/vtd/intremap.c |   9 ++-
 xen/include/asm-x86/hvm/vmx/vmx.h      |   1 +
 4 files changed, 111 insertions(+), 18 deletions(-)

-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 1/6] VMX: Statically assign two PI hooks

[Feng Wu]

PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
needed even when any previously assigned device is detached
from the domain. Since 'SN' bit is also used to control the
CPU side PI and we change the state of SN bit in these two
functions, then evaluate this bit in vmx_deliver_posted_intr()
when trying to deliver the interrupt in posted way via software.
The problem is if we deassign the hooks while the vCPU is runnable
in the runqueue with 'SN' set, all the furture notificaton event
will be suppressed. This patch makes these two hooks statically
assigned.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- Don't zap vmx_pi_switch_from() and vmx_pi_switch_to() when
any previously assigned device is detached from the domain.
- Comments changes.

 xen/arch/x86/hvm/vmx/vmx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3d330b6..355936a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -221,8 +221,6 @@ void vmx_pi_hooks_deassign(struct domain *d)
     ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
 
     d->arch.hvm_domain.vmx.vcpu_block = NULL;
-    d->arch.hvm_domain.vmx.pi_switch_from = NULL;
-    d->arch.hvm_domain.vmx.pi_switch_to = NULL;
     d->arch.hvm_domain.vmx.pi_do_resume = NULL;
 }
 
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

[Feng Wu]

This patch handles some concern cases when the last assigned device
is removed from the domain. In this case we should carefully handle
pi descriptor and the per-cpu blocking list, to make sure:
- all the PI descriptor are in the right state when next time a
devices is assigned to the domain again.
- No remaining vcpus of the domain in the per-cpu blocking list.

Basically, we pause the domain before zapping the PI hooks and
removing the vCPU from the blocking list, then unpause it after
that.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- Rename some functions:
	vmx_pi_remove_vcpu_from_blocking_list() -> vmx_pi_list_remove()
	vmx_pi_blocking_cleanup() -> vmx_pi_list_cleanup()
- Remove the check in vmx_pi_list_cleanup()
- Comments adjustment

 xen/arch/x86/hvm/vmx/vmx.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 355936a..7305f40 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -158,14 +158,12 @@ static void vmx_pi_switch_to(struct vcpu *v)
     pi_clear_sn(pi_desc);
 }
 
-static void vmx_pi_do_resume(struct vcpu *v)
+static void vmx_pi_list_remove(struct vcpu *v)
 {
     unsigned long flags;
     spinlock_t *pi_blocking_list_lock;
     struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
 
-    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
-
     /*
      * Set 'NV' field back to posted_intr_vector, so the
      * Posted-Interrupts can be delivered to the vCPU when
@@ -173,12 +171,12 @@ static void vmx_pi_do_resume(struct vcpu *v)
      */
     write_atomic(&pi_desc->nv, posted_intr_vector);
 
-    /* The vCPU is not on any blocking list. */
     pi_blocking_list_lock = v->arch.hvm_vmx.pi_blocking.lock;
 
     /* Prevent the compiler from eliminating the local variable.*/
     smp_rmb();
 
+    /* The vCPU is not on any blocking list. */
     if ( pi_blocking_list_lock == NULL )
         return;
 
@@ -198,6 +196,18 @@ static void vmx_pi_do_resume(struct vcpu *v)
     spin_unlock_irqrestore(pi_blocking_list_lock, flags);
 }
 
+static void vmx_pi_do_resume(struct vcpu *v)
+{
+    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
+
+    vmx_pi_list_remove(v);
+}
+
+static void vmx_pi_list_cleanup(struct vcpu *v)
+{
+    vmx_pi_list_remove(v);
+}
+
 /* This function is called when pcidevs_lock is held */
 void vmx_pi_hooks_assign(struct domain *d)
 {
@@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d)
 /* This function is called when pcidevs_lock is held */
 void vmx_pi_hooks_deassign(struct domain *d)
 {
+    struct vcpu *v;
+
     if ( !iommu_intpost || !has_hvm_container_domain(d) )
         return;
 
     ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
 
+    /*
+     * Pausing the domain can make sure the vCPU is not
+     * running and hence calling the hooks simultaneously
+     * when deassigning the PI hooks and removing the vCPU
+     * from the blocking list.
+     */
+    domain_pause(d);
+
     d->arch.hvm_domain.vmx.vcpu_block = NULL;
     d->arch.hvm_domain.vmx.pi_do_resume = NULL;
+
+    for_each_vcpu ( d, v )
+        vmx_pi_list_cleanup(v);
+
+    domain_unpause(d);
 }
 
 static int vmx_domain_initialise(struct domain *d)
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 3/6] VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed

[Feng Wu]

We should remove the vCPU from the per-cpu blocking list
if it is going to be destroyed.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- Call vmx_pi_list_cleanup() before vmx_destroy_vmcs()

 xen/arch/x86/hvm/vmx/vmx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7305f40..821cba2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -336,6 +336,7 @@ static void vmx_vcpu_destroy(struct vcpu *v)
      * separately here.
      */
     vmx_vcpu_disable_pml(v);
+    vmx_pi_list_cleanup(v);
     vmx_destroy_vmcs(v);
     vpmu_destroy(v);
     passive_domain_destroy(v);
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks

[Feng Wu]

We may hit the ASSERT() in vmx_vcpu_block in the current code,
since vmx_vcpu_block() may get called before vmx_pi_switch_to()
has been installed or executed. Here We use cmpxchg to update
the NDST field, this can make sure we only update the NDST when
vmx_pi_switch_to() has not been called. So the NDST is in a
proper state in vmx_vcpu_block().

Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- This patch is previously called "Pause/Unpause the domain before/after assigning PI hooks"
- Remove the pause/unpause method
- Use cmpxchg to update NDST

 xen/arch/x86/hvm/vmx/vmcs.c | 13 +++++--------
 xen/arch/x86/hvm/vmx/vmx.c  | 27 ++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 1bd875a..e733753 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -956,16 +956,13 @@ void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val)
  */
 static void pi_desc_init(struct vcpu *v)
 {
-    uint32_t dest;
-
     v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector;
 
-    dest = cpu_physical_id(v->processor);
-
-    if ( x2apic_enabled )
-        v->arch.hvm_vmx.pi_desc.ndst = dest;
-    else
-        v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, PI_xAPIC_NDST_MASK);
+    /*
+     * Mark NDST as invalid, then we can use this invalid value as a
+     * marker to whether update NDST or not in vmx_pi_hooks_assign().
+     */
+    v->arch.hvm_vmx.pi_desc.ndst = 0xff;
 }
 
 static int construct_vmcs(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 821cba2..09262d5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(struct vcpu *v)
 /* This function is called when pcidevs_lock is held */
 void vmx_pi_hooks_assign(struct domain *d)
 {
+    struct vcpu *v;
+
     if ( !iommu_intpost || !has_hvm_container_domain(d) )
         return;
 
     ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
 
-    d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
+    /*
+     * We carefully handle the timing here:
+     * - Install the context switch first
+     * - Then set the NDST field
+     * - Install the block and resume hooks in the end
+     *
+     * This can make sure the PI (especially the NDST feild) is
+     * in proper state when we call vmx_vcpu_block().
+     */
     d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
     d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
+
+    for_each_vcpu ( d, v )
+    {
+        unsigned int dest = cpu_physical_id(v->processor);
+        struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
+
+	/*
+	 * We don't need to update NDST if 'arch.hvm_domain.vmx.pi_switch_to'
+	 * already gets called
+	 */
+        (void)cmpxchg(&pi_desc->ndst, 0xff,
+                x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));
+    }
+
+    d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
     d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
 }
 
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Feng Wu]

We don't set the affinity for posted format IRTE, since the
destination of these interrupts is vCPU and the vCPU affinity
is set during vCPU scheduling.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- Keep the construction of new_ire and only modify the hardware
IRTE when it is not in posted mode.

 xen/drivers/passthrough/vtd/intremap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index bfd468b..4537714 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
     remap_rte->address_hi = 0;
     remap_rte->data = index - i;
 
-    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
-    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
-    iommu_flush_iec_index(iommu, 0, index);
+    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
+    {
+        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
+        iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
+        iommu_flush_iec_index(iommu, 0, index);
+    }
 
     unmap_vtd_domain_page(iremap_entries);
     spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline

[Feng Wu]

When cpu is offline, we need to move all the vcpus in its blocking
list to another online cpu, this patch handles it.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v4:
- Remove the pointless check since we are in machine stop
context and no other cpus go down in parallel.

 xen/arch/x86/hvm/vmx/vmcs.c       |  1 +
 xen/arch/x86/hvm/vmx/vmx.c        | 42 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
 3 files changed, 44 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index e733753..9f56c7c 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -578,6 +578,7 @@ void vmx_cpu_dead(unsigned int cpu)
     vmx_free_vmcs(per_cpu(vmxon_region, cpu));
     per_cpu(vmxon_region, cpu) = 0;
     nvmx_cpu_dead(cpu);
+    vmx_pi_desc_fixup(cpu);
 }
 
 int vmx_cpu_up(void)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 09262d5..ff87444 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -208,6 +208,48 @@ static void vmx_pi_list_cleanup(struct vcpu *v)
     vmx_pi_list_remove(v);
 }
 
+void vmx_pi_desc_fixup(int cpu)
+{
+    unsigned int new_cpu, dest;
+    unsigned long flags;
+    struct arch_vmx_struct *vmx, *tmp;
+    spinlock_t *new_lock, *old_lock = &per_cpu(vmx_pi_blocking, cpu).lock;
+    struct list_head *blocked_vcpus = &per_cpu(vmx_pi_blocking, cpu).list;
+
+    if ( !iommu_intpost )
+        return;
+
+    spin_lock_irqsave(old_lock, flags);
+
+    list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
+    {
+        /*
+         * We need to find an online cpu as the NDST of the PI descriptor, it
+         * doesn't matter whether it is within the cpupool of the domain or
+         * not. As long as it is online, the vCPU will be woken up once the
+         * notification event arrives.
+         */
+        new_cpu = cpumask_any(&cpu_online_map);
+        new_lock = &per_cpu(vmx_pi_blocking, new_cpu).lock;
+
+        spin_lock(new_lock);
+
+        ASSERT(vmx->pi_blocking.lock == old_lock);
+
+        dest = cpu_physical_id(new_cpu);
+        write_atomic(&vmx->pi_desc.ndst,
+                     x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));
+
+        list_move(&vmx->pi_blocking.list,
+                  &per_cpu(vmx_pi_blocking, new_cpu).list);
+        vmx->pi_blocking.lock = new_lock;
+
+        spin_unlock(new_lock);
+    }
+
+    spin_unlock_irqrestore(old_lock, flags);
+}
+
 /* This function is called when pcidevs_lock is held */
 void vmx_pi_hooks_assign(struct domain *d)
 {
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 4cdd9b1..9783c70 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -569,6 +569,7 @@ void free_p2m_hap_data(struct p2m_domain *p2m);
 void p2m_init_hap_data(struct p2m_domain *p2m);
 
 void vmx_pi_per_cpu_init(unsigned int cpu);
+void vmx_pi_desc_fixup(int cpu);
 
 void vmx_pi_hooks_assign(struct domain *d);
 void vmx_pi_hooks_deassign(struct domain *d);
-- 
2.1.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Jan Beulich]

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
> needed even when any previously assigned device is detached
> from the domain. Since 'SN' bit is also used to control the
> CPU side PI and we change the state of SN bit in these two
> functions, then evaluate this bit in vmx_deliver_posted_intr()
> when trying to deliver the interrupt in posted way via software.
> The problem is if we deassign the hooks while the vCPU is runnable
> in the runqueue with 'SN' set, all the furture notificaton event
> will be suppressed. This patch makes these two hooks statically
> assigned.

So if only SN left set is a problem, why do you need to also keep
vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
the bit, and vmx_deliver_posted_intr() doesn't actively change it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

[Jan Beulich]

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> +static void vmx_pi_list_cleanup(struct vcpu *v)
> +{
> +    vmx_pi_list_remove(v);
> +}

Please avoid such a no-op wrapper - the caller can easily call
vmx_pi_list_remove() directly.

> @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d)
>  /* This function is called when pcidevs_lock is held */
>  void vmx_pi_hooks_deassign(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>      if ( !iommu_intpost || !has_hvm_container_domain(d) )
>          return;
>  
>      ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
>  
> +    /*
> +     * Pausing the domain can make sure the vCPU is not
> +     * running and hence calling the hooks simultaneously
> +     * when deassigning the PI hooks and removing the vCPU
> +     * from the blocking list.
> +     */
> +    domain_pause(d);
> +
>      d->arch.hvm_domain.vmx.vcpu_block = NULL;
>      d->arch.hvm_domain.vmx.pi_do_resume = NULL;
> +
> +    for_each_vcpu ( d, v )
> +        vmx_pi_list_cleanup(v);
> +
> +    domain_unpause(d);
>  }

So you continue using pausing, and I continue to miss the argumentation
of why you can't do without (even if previously the discussion was for
patch 4, but it obviously applies here as well).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Jan Beulich]

>>> On 26.09.16 at 13:37, <JBeulich@suse.com> wrote:
>>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
>> needed even when any previously assigned device is detached
>> from the domain. Since 'SN' bit is also used to control the
>> CPU side PI and we change the state of SN bit in these two
>> functions, then evaluate this bit in vmx_deliver_posted_intr()
>> when trying to deliver the interrupt in posted way via software.
>> The problem is if we deassign the hooks while the vCPU is runnable
>> in the runqueue with 'SN' set, all the furture notificaton event
>> will be suppressed. This patch makes these two hooks statically
>> assigned.
> 
> So if only SN left set is a problem, why do you need to also keep
> vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
> the bit, and vmx_deliver_posted_intr() doesn't actively change it.

And it doesn't appear completely unreasonable for
vmx_pi_switch_to() to remove itself (when it gets run with
the "from" hook still NULL and no new device being in the
process of getting assigned).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks

[Jan Beulich]

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> We may hit the ASSERT() in vmx_vcpu_block in the current code,

There are three of them - please be explicit which one is what gets
dealt with here.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -956,16 +956,13 @@ void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val)
>   */
>  static void pi_desc_init(struct vcpu *v)
>  {
> -    uint32_t dest;
> -
>      v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector;
>  
> -    dest = cpu_physical_id(v->processor);
> -
> -    if ( x2apic_enabled )
> -        v->arch.hvm_vmx.pi_desc.ndst = dest;
> -    else
> -        v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, PI_xAPIC_NDST_MASK);
> +    /*
> +     * Mark NDST as invalid, then we can use this invalid value as a
> +     * marker to whether update NDST or not in vmx_pi_hooks_assign().
> +     */
> +    v->arch.hvm_vmx.pi_desc.ndst = 0xff;

Is this a proper "invalid" indicator in the x2APIC case? I would have
assumed you want 0xffffffff in that case.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(struct vcpu *v)
>  /* This function is called when pcidevs_lock is held */
>  void vmx_pi_hooks_assign(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>      if ( !iommu_intpost || !has_hvm_container_domain(d) )
>          return;
>  
>      ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
>  
> -    d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
> +    /*
> +     * We carefully handle the timing here:
> +     * - Install the context switch first
> +     * - Then set the NDST field
> +     * - Install the block and resume hooks in the end
> +     *
> +     * This can make sure the PI (especially the NDST feild) is
> +     * in proper state when we call vmx_vcpu_block().
> +     */
>      d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
>      d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        unsigned int dest = cpu_physical_id(v->processor);
> +        struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> +	/*
> +	 * We don't need to update NDST if 'arch.hvm_domain.vmx.pi_switch_to'
> +	 * already gets called
> +	 */

Stray tabs.

> +        (void)cmpxchg(&pi_desc->ndst, 0xff,
> +                x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));

Here even more than above I think you want to derive the xAPIC
value from the wider x2APIC one not just for the value to write, but
also for the value to compare against. If you did so in pi_desc_init()
as well as here, I don't see a strict need for introducing a suitable
#define. If you don't, however, you definitely want a pair of
#define-s to one can associate both places with one another.

But overall I think this is much better than the previous version, so
thanks for doing (and testing) this.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Jan Beulich]

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> We don't set the affinity for posted format IRTE, since the
> destination of these interrupts is vCPU and the vCPU affinity
> is set during vCPU scheduling.

I'm quite sure I did point out before that you talk about just affinity
changes here, yet ...

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
>      remap_rte->address_hi = 0;
>      remap_rte->data = index - i;
>  
> -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> -    iommu_flush_iec_index(iommu, 0, index);
> +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
> +    {
> +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> +        iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> +        iommu_flush_iec_index(iommu, 0, index);
> +    }

... you suppress the update also in other cases. This _may_ be safe
at present, but you're digging a hole for someone else to fall into
down the road. Hence at the very least you should, in a to be added
"else" path, ASSERT() that nothing in the descriptor changed except
the bits representing affinity. Even better would be if in fact you
suppressed the update+flush only when nothing other than dst
changed.

Also, since you already touch this, please consider switching from the
type-unsafe memcpy() to type-safe structure assignment. And please
in any event change the sizeof()-s to sizeof(*iremap_entry).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline

[Jan Beulich]

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> +void vmx_pi_desc_fixup(int cpu)
> +{
> +    unsigned int new_cpu, dest;
> +    unsigned long flags;
> +    struct arch_vmx_struct *vmx, *tmp;
> +    spinlock_t *new_lock, *old_lock = &per_cpu(vmx_pi_blocking, cpu).lock;
> +    struct list_head *blocked_vcpus = &per_cpu(vmx_pi_blocking, cpu).list;
> +
> +    if ( !iommu_intpost )
> +        return;
> +
> +    spin_lock_irqsave(old_lock, flags);
> +
> +    list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
> +    {
> +        /*
> +         * We need to find an online cpu as the NDST of the PI descriptor, it
> +         * doesn't matter whether it is within the cpupool of the domain or
> +         * not. As long as it is online, the vCPU will be woken up once the
> +         * notification event arrives.
> +         */
> +        new_cpu = cpumask_any(&cpu_online_map);
> +        new_lock = &per_cpu(vmx_pi_blocking, new_cpu).lock;
> +
> +        spin_lock(new_lock);

Without extra consideration this is a classical ABBA deadlock
scenario. Please add a comment (perhaps at the first lock location
above) at least briefly explaining why there can't be any deadlock
here.

Apart from that the patch looks fine to me now.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 26, 2016 8:10 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
> 
> >>> On 26.09.16 at 13:37, <JBeulich@suse.com> wrote:
> >>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> >> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
> >> needed even when any previously assigned device is detached
> >> from the domain. Since 'SN' bit is also used to control the
> >> CPU side PI and we change the state of SN bit in these two
> >> functions, then evaluate this bit in vmx_deliver_posted_intr()
> >> when trying to deliver the interrupt in posted way via software.
> >> The problem is if we deassign the hooks while the vCPU is runnable
> >> in the runqueue with 'SN' set, all the furture notificaton event
> >> will be suppressed. This patch makes these two hooks statically
> >> assigned.
> >
> > So if only SN left set is a problem, why do you need to also keep
> > vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
> > the bit, and vmx_deliver_posted_intr() doesn't actively change it.
> 
> And it doesn't appear completely unreasonable for
> vmx_pi_switch_to() to remove itself (when it gets run with
> the "from" hook still NULL and no new device being in the
> process of getting assigned).

I think this may introduce extra complex to the situation:
1. Especially for "no new device being in the process of getting assigned",
since device assignment can be happened simultaneous when this function
gets called, so does it mean we need to use a lock to protect it?
2. vmx_pi_switch_to() will update the 'NDST' field to the current pCPU,
so if we zap vmx_pi_switch_to() here, that means, the 'NDST' filed may
contain some valid pCPU information. So the method you suggested
for [4/6] of the series doesn't work anymore, since we cannot use
an invalid value for the judgement.

Thanks,
Feng

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 26, 2016 7:47 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned
> devices are removed
> 
> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> > +static void vmx_pi_list_cleanup(struct vcpu *v)
> > +{
> > +    vmx_pi_list_remove(v);
> > +}
> 
> Please avoid such a no-op wrapper - the caller can easily call
> vmx_pi_list_remove() directly.
> 
> > @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d)
> >  /* This function is called when pcidevs_lock is held */
> >  void vmx_pi_hooks_deassign(struct domain *d)
> >  {
> > +    struct vcpu *v;
> > +
> >      if ( !iommu_intpost || !has_hvm_container_domain(d) )
> >          return;
> >
> >      ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
> >
> > +    /*
> > +     * Pausing the domain can make sure the vCPU is not
> > +     * running and hence calling the hooks simultaneously
> > +     * when deassigning the PI hooks and removing the vCPU
> > +     * from the blocking list.
> > +     */
> > +    domain_pause(d);
> > +
> >      d->arch.hvm_domain.vmx.vcpu_block = NULL;
> >      d->arch.hvm_domain.vmx.pi_do_resume = NULL;
> > +
> > +    for_each_vcpu ( d, v )
> > +        vmx_pi_list_cleanup(v);
> > +
> > +    domain_unpause(d);
> >  }
> 
> So you continue using pausing, and I continue to miss the argumentation
> of why you can't do without (even if previously the discussion was for
> patch 4, but it obviously applies here as well).

I think this case is slightly different. Here we need to call vmx_pi_list_cleanup()
to remove the vCPU from the blocking list if it is on the list. However, this
can be happened when vmx_vcpu_block() is called, hence we might incorrectly
add the vcpu to the blocking list while the last device is detached from the domain.
In fact, v2 gave some trick methods to handle this, and that was considered as
hard to maintain, so George suggested to use pause/unpause for this case, and I
also think it is easy and acceptable consider that devices detaching is not a
frequent action.

Thanks,
Feng

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 26, 2016 8:46 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: Re: [PATCH v4 4/6] VMX: Make sure PI is in proper state before install
> the hooks
> 
> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> > We may hit the ASSERT() in vmx_vcpu_block in the current code,
> 
> There are three of them - please be explicit which one is what gets
> dealt with here.
> 
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -956,16 +956,13 @@ void virtual_vmcs_vmwrite(const struct vcpu *v,
> u32 vmcs_encoding, u64 val)
> >   */
> >  static void pi_desc_init(struct vcpu *v)
> >  {
> > -    uint32_t dest;
> > -
> >      v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector;
> >
> > -    dest = cpu_physical_id(v->processor);
> > -
> > -    if ( x2apic_enabled )
> > -        v->arch.hvm_vmx.pi_desc.ndst = dest;
> > -    else
> > -        v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest,
> PI_xAPIC_NDST_MASK);
> > +    /*
> > +     * Mark NDST as invalid, then we can use this invalid value as a
> > +     * marker to whether update NDST or not in vmx_pi_hooks_assign().
> > +     */
> > +    v->arch.hvm_vmx.pi_desc.ndst = 0xff;
> 
> Is this a proper "invalid" indicator in the x2APIC case? I would have
> assumed you want 0xffffffff in that case.

Right. We should use 0xffffffff here. Thanks a lot!

Thanks,
Feng


> 
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(struct vcpu *v)
> >  /* This function is called when pcidevs_lock is held */
> >  void vmx_pi_hooks_assign(struct domain *d)
> >  {
> > +    struct vcpu *v;
> > +
> >      if ( !iommu_intpost || !has_hvm_container_domain(d) )
> >          return;
> >
> >      ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
> >
> > -    d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
> > +    /*
> > +     * We carefully handle the timing here:
> > +     * - Install the context switch first
> > +     * - Then set the NDST field
> > +     * - Install the block and resume hooks in the end
> > +     *
> > +     * This can make sure the PI (especially the NDST feild) is
> > +     * in proper state when we call vmx_vcpu_block().
> > +     */
> >      d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
> >      d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
> > +
> > +    for_each_vcpu ( d, v )
> > +    {
> > +        unsigned int dest = cpu_physical_id(v->processor);
> > +        struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> > +
> > +	/*
> > +	 * We don't need to update NDST if
> 'arch.hvm_domain.vmx.pi_switch_to'
> > +	 * already gets called
> > +	 */
> 
> Stray tabs.
> 
> > +        (void)cmpxchg(&pi_desc->ndst, 0xff,
> > +                x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));
> 
> Here even more than above I think you want to derive the xAPIC
> value from the wider x2APIC one not just for the value to write, but
> also for the value to compare against. If you did so in pi_desc_init()
> as well as here, I don't see a strict need for introducing a suitable
> #define. If you don't, however, you definitely want a pair of
> #define-s to one can associate both places with one another.
> 
> But overall I think this is much better than the previous version, so
> thanks for doing (and testing) this.
> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 26, 2016 8:58 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format
> IRTE
> 
> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> > We don't set the affinity for posted format IRTE, since the
> > destination of these interrupts is vCPU and the vCPU affinity
> > is set during vCPU scheduling.
> 
> I'm quite sure I did point out before that you talk about just affinity
> changes here, yet ...
> 
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
> >      remap_rte->address_hi = 0;
> >      remap_rte->data = index - i;
> >
> > -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> > -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> > -    iommu_flush_iec_index(iommu, 0, index);
> > +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
> > +    {
> > +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> > +        iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> > +        iommu_flush_iec_index(iommu, 0, index);
> > +    }
> 
> ... you suppress the update also in other cases. This _may_ be safe
> at present, but you're digging a hole for someone else to fall into
> down the road. Hence at the very least you should, in a to be added
> "else" path, ASSERT() that nothing in the descriptor changed except
> the bits representing affinity. Even better would be if in fact you
> suppressed the update+flush only when nothing other than dst
> changed.

I am a little confused about the above comments, Posted IRTE and
Remapped IRTE has different format, and when the IRTE is in posted
format, typically, the updated information (delivery mode, dest mode,
vector, dest, etc) has no meaning for posted IRTE. However, there are
indeed some shared part between the two format as below:
- p
- fpd
- im
- sid
- sq
- svt

Bits like sid, sq, and svt are not changed in this function, I am not sure
this is what you mean by " you suppress the update also in other cases.",
and I am not quite clear about what is your requirement, it would be
appreciated if you can describe a bit more! Thanks!

Thanks,
Feng

> 
> Also, since you already touch this, please consider switching from the
> type-unsafe memcpy() to type-safe structure assignment. And please
> in any event change the sizeof()-s to sizeof(*iremap_entry).
> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 26, 2016 9:03 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: Re: [PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline
> 
> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> > +void vmx_pi_desc_fixup(int cpu)
> > +{
> > +    unsigned int new_cpu, dest;
> > +    unsigned long flags;
> > +    struct arch_vmx_struct *vmx, *tmp;
> > +    spinlock_t *new_lock, *old_lock = &per_cpu(vmx_pi_blocking, cpu).lock;
> > +    struct list_head *blocked_vcpus = &per_cpu(vmx_pi_blocking, cpu).list;
> > +
> > +    if ( !iommu_intpost )
> > +        return;
> > +
> > +    spin_lock_irqsave(old_lock, flags);
> > +
> > +    list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
> > +    {
> > +        /*
> > +         * We need to find an online cpu as the NDST of the PI descriptor, it
> > +         * doesn't matter whether it is within the cpupool of the domain or
> > +         * not. As long as it is online, the vCPU will be woken up once the
> > +         * notification event arrives.
> > +         */
> > +        new_cpu = cpumask_any(&cpu_online_map);
> > +        new_lock = &per_cpu(vmx_pi_blocking, new_cpu).lock;
> > +
> > +        spin_lock(new_lock);
> 
> Without extra consideration this is a classical ABBA deadlock
> scenario. Please add a comment (perhaps at the first lock location
> above) at least briefly explaining why there can't be any deadlock
> here.

Yes, I also noted this. I will add a comment here, which is really needed.
Thanks a lot!

Thanks,
Feg

> 
> Apart from that the patch looks fine to me now.
> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Jan Beulich]

>>> On 28.09.16 at 08:48, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, September 26, 2016 8:10 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org 
>> Subject: Re: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
>> 
>> >>> On 26.09.16 at 13:37, <JBeulich@suse.com> wrote:
>> >>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> >> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
>> >> needed even when any previously assigned device is detached
>> >> from the domain. Since 'SN' bit is also used to control the
>> >> CPU side PI and we change the state of SN bit in these two
>> >> functions, then evaluate this bit in vmx_deliver_posted_intr()
>> >> when trying to deliver the interrupt in posted way via software.
>> >> The problem is if we deassign the hooks while the vCPU is runnable
>> >> in the runqueue with 'SN' set, all the furture notificaton event
>> >> will be suppressed. This patch makes these two hooks statically
>> >> assigned.
>> >
>> > So if only SN left set is a problem, why do you need to also keep
>> > vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
>> > the bit, and vmx_deliver_posted_intr() doesn't actively change it.
>> 
>> And it doesn't appear completely unreasonable for
>> vmx_pi_switch_to() to remove itself (when it gets run with
>> the "from" hook still NULL and no new device being in the
>> process of getting assigned).
> 
> I think this may introduce extra complex to the situation:
> 1. Especially for "no new device being in the process of getting assigned",
> since device assignment can be happened simultaneous when this function
> gets called, so does it mean we need to use a lock to protect it?

Since device addition/removal is already protected by a lock, this
would at least seem not impossible to do without causing lock
conflicts (and would certainly be required, yes).

> 2. vmx_pi_switch_to() will update the 'NDST' field to the current pCPU,
> so if we zap vmx_pi_switch_to() here, that means, the 'NDST' filed may
> contain some valid pCPU information. So the method you suggested
> for [4/6] of the series doesn't work anymore, since we cannot use
> an invalid value for the judgement.

But why does the function need to continue to do these adjustments
when there's no assigned device anymore? I.e. couldn't it reset the
field to the "invalid" value along with removing itself?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

[Jan Beulich]

>>> On 28.09.16 at 08:50, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, September 26, 2016 7:47 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org 
>> Subject: Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned
>> devices are removed
>> 
>> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> > +static void vmx_pi_list_cleanup(struct vcpu *v)
>> > +{
>> > +    vmx_pi_list_remove(v);
>> > +}
>> 
>> Please avoid such a no-op wrapper - the caller can easily call
>> vmx_pi_list_remove() directly.
>> 
>> > @@ -215,13 +225,28 @@ void vmx_pi_hooks_assign(struct domain *d)
>> >  /* This function is called when pcidevs_lock is held */
>> >  void vmx_pi_hooks_deassign(struct domain *d)
>> >  {
>> > +    struct vcpu *v;
>> > +
>> >      if ( !iommu_intpost || !has_hvm_container_domain(d) )
>> >          return;
>> >
>> >      ASSERT(d->arch.hvm_domain.vmx.vcpu_block);
>> >
>> > +    /*
>> > +     * Pausing the domain can make sure the vCPU is not
>> > +     * running and hence calling the hooks simultaneously
>> > +     * when deassigning the PI hooks and removing the vCPU
>> > +     * from the blocking list.
>> > +     */
>> > +    domain_pause(d);
>> > +
>> >      d->arch.hvm_domain.vmx.vcpu_block = NULL;
>> >      d->arch.hvm_domain.vmx.pi_do_resume = NULL;
>> > +
>> > +    for_each_vcpu ( d, v )
>> > +        vmx_pi_list_cleanup(v);
>> > +
>> > +    domain_unpause(d);
>> >  }
>> 
>> So you continue using pausing, and I continue to miss the argumentation
>> of why you can't do without (even if previously the discussion was for
>> patch 4, but it obviously applies here as well).
> 
> I think this case is slightly different. Here we need to call 
> vmx_pi_list_cleanup()
> to remove the vCPU from the blocking list if it is on the list. However, this
> can be happened when vmx_vcpu_block() is called, hence we might incorrectly
> add the vcpu to the blocking list while the last device is detached from the domain.
> In fact, v2 gave some trick methods to handle this, and that was considered as
> hard to maintain, so George suggested to use pause/unpause for this case, and I
> also think it is easy and acceptable consider that devices detaching is not a
> frequent action.

Note how I said "I continue to miss the argumentation of why you
can't do without" - I'm not opposed to pausing getting used here, but
it needs to be at least briefly explained in the commit message. That's
among other things so that (see that other thread) people can't later
come and say "Hey, pausing is done in all sorts of situations, why
won't you let me add some more pausing?"

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Jan Beulich]

>>> On 28.09.16 at 08:51, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, September 26, 2016 8:58 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org 
>> Subject: Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted 
> format
>> IRTE
>> 
>> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> > We don't set the affinity for posted format IRTE, since the
>> > destination of these interrupts is vCPU and the vCPU affinity
>> > is set during vCPU scheduling.
>> 
>> I'm quite sure I did point out before that you talk about just affinity
>> changes here, yet ...
>> 
>> > --- a/xen/drivers/passthrough/vtd/intremap.c
>> > +++ b/xen/drivers/passthrough/vtd/intremap.c
>> > @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
>> >      remap_rte->address_hi = 0;
>> >      remap_rte->data = index - i;
>> >
>> > -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> > -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>> > -    iommu_flush_iec_index(iommu, 0, index);
>> > +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
>> > +    {
>> > +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> > +        iommu_flush_cache_entry(iremap_entry, sizeof(struct 
> iremap_entry));
>> > +        iommu_flush_iec_index(iommu, 0, index);
>> > +    }
>> 
>> ... you suppress the update also in other cases. This _may_ be safe
>> at present, but you're digging a hole for someone else to fall into
>> down the road. Hence at the very least you should, in a to be added
>> "else" path, ASSERT() that nothing in the descriptor changed except
>> the bits representing affinity. Even better would be if in fact you
>> suppressed the update+flush only when nothing other than dst
>> changed.
> 
> I am a little confused about the above comments, Posted IRTE and
> Remapped IRTE has different format, and when the IRTE is in posted
> format, typically, the updated information (delivery mode, dest mode,
> vector, dest, etc) has no meaning for posted IRTE. However, there are
> indeed some shared part between the two format as below:
> - p
> - fpd
> - im
> - sid
> - sq
> - svt
> 
> Bits like sid, sq, and svt are not changed in this function,

How do you know? Judging just from current callers is - as said
before - calling for trouble down the road. And the function clearly
creates a brand new IRTE, which fully replaces the previous one.

> I am not sure
> this is what you mean by " you suppress the update also in other cases.",
> and I am not quite clear about what is your requirement, it would be
> appreciated if you can describe a bit more! Thanks!

My requirement is for the function to not get changed in ways
making (hidden) assumptions about its callers. If you want to
suppress the actual update, you should conditionalize this in a
way which can be proven correct by looking at just this one
function. That'll presumably entail comparing fields of the
currently in place and the new IRTE.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/6] VMX: Properly handle pi when all the assigned devices are removed

[Wu, Feng]

> >>
> >> So you continue using pausing, and I continue to miss the argumentation
> >> of why you can't do without (even if previously the discussion was for
> >> patch 4, but it obviously applies here as well).
> >
> > I think this case is slightly different. Here we need to call
> > vmx_pi_list_cleanup()
> > to remove the vCPU from the blocking list if it is on the list. However, this
> > can be happened when vmx_vcpu_block() is called, hence we might incorrectly
> > add the vcpu to the blocking list while the last device is detached from the
> domain.
> > In fact, v2 gave some trick methods to handle this, and that was considered as
> > hard to maintain, so George suggested to use pause/unpause for this case,
> and I
> > also think it is easy and acceptable consider that devices detaching is not a
> > frequent action.
> 
> Note how I said "I continue to miss the argumentation of why you
> can't do without" - I'm not opposed to pausing getting used here, but
> it needs to be at least briefly explained in the commit message. That's
> among other things so that (see that other thread) people can't later
> come and say "Hey, pausing is done in all sorts of situations, why
> won't you let me add some more pausing?"

Fair enough, I will elaborate a bit more on it.

Thanks,
Feng

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, September 28, 2016 5:59 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: RE: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format
> IRTE
> 
> >>> On 28.09.16 at 08:51, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, September 26, 2016 8:58 PM
> >> To: Wu, Feng <feng.wu@intel.com>
> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> >> devel@lists.xen.org
> >> Subject: Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted
> > format
> >> IRTE
> >>
> >> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> >> > We don't set the affinity for posted format IRTE, since the
> >> > destination of these interrupts is vCPU and the vCPU affinity
> >> > is set during vCPU scheduling.
> >>
> >> I'm quite sure I did point out before that you talk about just affinity
> >> changes here, yet ...
> >>
> >> > --- a/xen/drivers/passthrough/vtd/intremap.c
> >> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> >> > @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
> >> >      remap_rte->address_hi = 0;
> >> >      remap_rte->data = index - i;
> >> >
> >> > -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> >> > -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> >> > -    iommu_flush_iec_index(iommu, 0, index);
> >> > +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
> >> > +    {
> >> > +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> >> > +        iommu_flush_cache_entry(iremap_entry, sizeof(struct
> > iremap_entry));
> >> > +        iommu_flush_iec_index(iommu, 0, index);
> >> > +    }
> >>
> >> ... you suppress the update also in other cases. This _may_ be safe
> >> at present, but you're digging a hole for someone else to fall into
> >> down the road. Hence at the very least you should, in a to be added
> >> "else" path, ASSERT() that nothing in the descriptor changed except
> >> the bits representing affinity. Even better would be if in fact you
> >> suppressed the update+flush only when nothing other than dst
> >> changed.
> >
> > I am a little confused about the above comments, Posted IRTE and
> > Remapped IRTE has different format, and when the IRTE is in posted
> > format, typically, the updated information (delivery mode, dest mode,
> > vector, dest, etc) has no meaning for posted IRTE. However, there are
> > indeed some shared part between the two format as below:
> > - p
> > - fpd
> > - im
> > - sid
> > - sq
> > - svt
> >
> > Bits like sid, sq, and svt are not changed in this function,
> 
> How do you know? Judging just from current callers is - as said
> before - calling for trouble down the road. And the function clearly
> creates a brand new IRTE, which fully replaces the previous one.

This function copy the old IRTE to 'new_ire' and it doesn't touch
fields like 'sid', 'sq', and 'svt', so how could these bits get changed?

> 
> > I am not sure
> > this is what you mean by " you suppress the update also in other cases.",
> > and I am not quite clear about what is your requirement, it would be
> > appreciated if you can describe a bit more! Thanks!
> 
> My requirement is for the function to not get changed in ways
> making (hidden) assumptions about its callers. 

As mentioned above, I don't think I made such assumptions about its caller.
Even without my patch, this function only changes the affinity related fields,
such as, destination, delivery mode, destination mode, etc, which are not
needed for posted mode.

Thanks,
Feng

> If you want to
> suppress the actual update, you should conditionalize this in a
> way which can be proven correct by looking at just this one
> function. That'll presumably entail comparing fields of the
> currently in place and the new IRTE.
> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, September 28, 2016 5:39 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: RE: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
> 
> >>> On 28.09.16 at 08:48, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, September 26, 2016 8:10 PM
> >> To: Wu, Feng <feng.wu@intel.com>
> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> >> devel@lists.xen.org
> >> Subject: Re: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
> >>
> >> >>> On 26.09.16 at 13:37, <JBeulich@suse.com> wrote:
> >> >>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> >> >> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
> >> >> needed even when any previously assigned device is detached
> >> >> from the domain. Since 'SN' bit is also used to control the
> >> >> CPU side PI and we change the state of SN bit in these two
> >> >> functions, then evaluate this bit in vmx_deliver_posted_intr()
> >> >> when trying to deliver the interrupt in posted way via software.
> >> >> The problem is if we deassign the hooks while the vCPU is runnable
> >> >> in the runqueue with 'SN' set, all the furture notificaton event
> >> >> will be suppressed. This patch makes these two hooks statically
> >> >> assigned.
> >> >
> >> > So if only SN left set is a problem, why do you need to also keep
> >> > vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
> >> > the bit, and vmx_deliver_posted_intr() doesn't actively change it.
> >>
> >> And it doesn't appear completely unreasonable for
> >> vmx_pi_switch_to() to remove itself (when it gets run with
> >> the "from" hook still NULL and no new device being in the
> >> process of getting assigned).
> >
> > I think this may introduce extra complex to the situation:
> > 1. Especially for "no new device being in the process of getting assigned",
> > since device assignment can be happened simultaneous when this function
> > gets called, so does it mean we need to use a lock to protect it?
> 
> Since device addition/removal is already protected by a lock, this
> would at least seem not impossible to do without causing lock
> conflicts (and would certainly be required, yes).
> 

I use pcidevs_lock()/pcidevs_unlock() in vmx_pi_switch_to() since this lock
is held while device assignment. However, I got the following call trace during
booting the guest. From the message, seems we cannot acquire that lock
in this function.

Back to the original question, maybe it is worth remain the "to" hooks, and
we might go too far if we really want to zap it.

(XEN) CPU:    7
(XEN) RIP:    e008:[<ffff82d0801309a4>] spinlock.c#check_lock+0x3c/0x40
(XEN) RFLAGS: 0000000000010046   CONTEXT: hypervisor (d1v0)
(XEN) rax: 0000000000000000   rbx: ffff82d0802fea48   rcx: 0000000000000000
(XEN) rdx: 0000000000000001   rsi: 0000000000000003   rdi: ffff82d0802fea4e
(XEN) rbp: ffff8301713efca8   rsp: ffff8301713efca8   r8:  0000000000000003
(XEN) r9:  0000ffff0000ffff   r10: 00ff00ff00ff00ff   r11: 0f0f0f0f0f0f0f0f
(XEN) r12: 0000000000000007   r13: ffff83005da89000   r14: ffff83007baf9000
(XEN) r15: 0000000000000003   cr0: 000000008005003b   cr4: 00000000003526e0
(XEN) cr3: 00000001432c6000   cr2: ffff88016a3e6ee8
(XEN) ds: 002b   es: 002b   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d0801309a4> (spinlock.c#check_lock+0x3c/0x40):
(XEN)  98 83 f1 01 39 c8 75 02 <0f> 0b 5d c3 55 48 89 e5 f0 ff 05 19 8e 1b 00 5d

(XEN) Xen stack trace from rsp=ffff8301713efca8:
(XEN)    ffff8301713efcc0 ffff82d0801309d3 ffff82d0802fea48 ffff8301713efce0
(XEN)    ffff82d080130c13 ffff83005da89000 ffff8301713f4fe0 ffff8301713efcf0
(XEN)    ffff82d08014da5f ffff8301713efd10 ffff82d0801f24d7 ffff8301713efd40
(XEN)    0000000000000d01 ffff8301713efd50 ffff82d0801f4413 ffff8301713f0068
(XEN)    0000000000000000 ffff83005da89000 0000000000000007 ffff83017e11b000
(XEN)    ffff83007baf9000 ffff8301713efdb0 ffff82d080165ea2 ffff8301713eff18
(XEN)    ffff83017137a000 00000000ffffffff ffff83007baf9060 000000000000004d
(XEN)    ffff83005da89000 ffff83007bada000 ffff83017e11b000 0000000000000007
(XEN)    ffff8301714c3000 ffff8301713efe20 ffff82d080169fb6 ffff82d0801309d3
(XEN)    ffff8301713efde0 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 ffff8301713efe20 ffff83007bada000 ffff83005da89000
(XEN)    0000001531ccf815 ffff8301713f4148 0000000000000001 ffff8301713efeb0
(XEN)    ffff82d08012cf82 b2c99dc400000001 ffff8301713f4160 00000007003efe60
(XEN)    ffff8301713f4140 ffff8301713efe60 0000000700010000 0000000000000001
(XEN)    ffff83005da89000 0000000001c9c380 ffff82d0801bc200 000000007bada000
(XEN)    ffff82d08031ae00 ffff82d08031aa80 ffffffffffffffff ffff8301713effff
(XEN)    ffff83017137a000 ffff8301713efee0 ffff82d080130180 ffff8301713effff
(XEN)    ffff83007baf9000 ffff8301714c3000 00000000ffffffff ffff8301713efef0
(XEN)    ffff82d0801301d5 ffff8301713eff10 ffff82d0801656b2 ffff82d0801301d5
(XEN)    ffff83007bada000 ffff8301713efdc8 0000000000000000 0000000000000000

(XEN) Xen call trace:
(XEN)    [<ffff82d0801309a4>] spinlock.c#check_lock+0x3c/0x40
(XEN)    [<ffff82d0801309d3>] _spin_lock+0x11/0x4f
(XEN)    [<ffff82d080130c13>] _spin_lock_recursive+0x2a/0x56
(XEN)    [<ffff82d08014da5f>] pcidevs_lock+0x10/0x12
(XEN)    [<ffff82d0801f24d7>] vmx.c#vmx_pi_switch_to+0x3f/0x6f
(XEN)    [<ffff82d0801f4413>] vmx.c#vmx_ctxt_switch_to+0x1d8/0x1e5
(XEN)    [<ffff82d080165ea2>] domain.c#__context_switch+0x191/0x3d2
(XEN)    [<ffff82d080169fb6>] context_switch+0x147/0xee7
(XEN)    [<ffff82d08012cf82>] schedule.c#schedule+0x5ae/0x609
(XEN)    [<ffff82d080130180>] softirq.c#__do_softirq+0x7f/0x8a
(XEN)    [<ffff82d0801301d5>] do_softirq+0x13/0x15
(XEN)    [<ffff82d0801656b2>] domain.c#idle_loop+0x55/0x62
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 7:
(XEN) Xen BUG at spinlock.c:48
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...
(XEN) Assertion 'current == idle_vcpu[smp_processor_id()]' failed at domain.c:2168
(XEN) ----[ Xen-4.8-unstable  x86_64  debug=y  Not tainted ]----
(XEN) CPU:    7
(XEN) RIP:    e008:[<ffff82d08016ada9>] __sync_local_execstate+0x44/0x67
(XEN) RFLAGS: 0000000000010087   CONTEXT: hypervisor (d1v0)
(XEN) rax: ffff82d080344290   rbx: 0000000000000002   rcx: 0000000000000007
(XEN) rdx: ffff82d0802e6080   rsi: ffff83005da89000   rdi: ffff83007bada000
(XEN) rbp: ffff8301713ef988   rsp: ffff8301713ef978   r8:  ffff8301714e0000
(XEN) r9:  0000000000000000   r10: 0000000000000007   r11: 0000000000000001
(XEN) r12: 0000000000000001   r13: 00000000000000fd   r14: 0000000000000030
(XEN) r15: 0000000080000000   cr0: 000000008005003b   cr4: 00000000003526e0
(XEN) cr3: 00000001432c6000   cr2: ffff88016a3e6ee8
(XEN) ds: 002b   es: 002b   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d08016ada9> (__sync_local_execstate+0x44/0x67):
(XEN)  8b 3c ca 48 39 fe 74 02 <0f> 0b e8 61 af ff ff 81 e3 00 02 00 00 9c 48 81
(XEN) Xen stack trace from rsp=ffff8301713ef978:
(XEN)    0000000000000100 ffff8301713efa28 ffff8301713ef9a8 ffff82d080194a5c
(XEN)    ffff8301713ef9a8 0000000000000086 ffff8301713efa18 ffff82d080174293
(XEN)    0000000000000046 ffff8301713ef9d8 0000000000000000 0000000000000096
(XEN)    ffff8301713ef9f0 ffff82d0801309d3 ffff82d0802ffc00 0000000000000086
(XEN)    ffff82d08027997d ffff82d080275e11 0000000000000030 ffff82d080279d20
(XEN)    00007cfe8ec105b7 ffff82d080243167 ffff82d080279d20 0000000000000030
(XEN)    ffff82d080275e11 ffff82d08027997d ffff8301713efb18 0000000000000086
(XEN)    0000000000000001 0000000000000007 0000000000000000 ffff8301714e0000
(XEN)    ffff8301713f4028 0000000000000001 ffff8301713effff 0000000000000046
(XEN)    ffff82d0802fe880 000000fd00000000 ffff82d080194593 000000000000e008
(XEN)    0000000000000202 ffff8301713efad8 000000000000e010 ffff82d080194578
(XEN)    ffff8301713efb28 00001388713efae8 000082d08027997d 0000000000000000
(XEN)    0000000000000086 ffff82d08027997d ffff82d080275e11 0000000000000030
(XEN)    ffff8301713efb88 ffff82d080146f25 00000000713efb88 0000000000000020
(XEN)    ffff8301713efb98 ffff8301713efb48 e5894855c35d0b0f ffff82d080279d20
(XEN)    ffff82d080275e11 0000000000000030 ffff8301714e0000 0000000000000002
(XEN)    ffff8301713efbf8 ffff82d080248518 ffff8301713efbe8 ffff82d08019c47d
(XEN)    ffff8301713efbf8 ffff82d0801309a6 0000000000000040 0b0f000000000086
(XEN)    000000fc713efc08 ffff83005da89000 0000000000000007 ffff83005da89000
(XEN)    ffff83007baf9000 0000000000000003 00007cfe8ec103e7 ffff82d080243268

(XEN) Xen call trace:
(XEN)    [<ffff82d08016ada9>] __sync_local_execstate+0x44/0x67
(XEN)    [<ffff82d080194a5c>] invalidate_interrupt+0x40/0x7d
(XEN)    [<ffff82d080174293>] do_IRQ+0x8c/0x60c
(XEN)    [<ffff82d080243167>] common_interrupt+0x67/0x70
(XEN)    [<ffff82d080194593>] machine_restart+0x4a/0x257
(XEN)    [<ffff82d080146f25>] console_suspend+0/0x28
(XEN)    [<ffff82d08019c47d>] do_invalid_op+0x39b/0x4a1
(XEN)    [<ffff82d080243268>] entry.o#handle_exception_saved+0x66/0xa4
(XEN)    [<ffff82d0801309a4>] spinlock.c#check_lock+0x3c/0x40
(XEN)    [<ffff82d0801309d3>] _spin_lock+0x11/0x4f
(XEN)    [<ffff82d080130c13>] _spin_lock_recursive+0x2a/0x56
(XEN)    [<ffff82d08014da5f>] pcidevs_lock+0x10/0x12
(XEN)    [<ffff82d0801f24d7>] vmx.c#vmx_pi_switch_to+0x3f/0x6f
(XEN)    [<ffff82d0801f4413>] vmx.c#vmx_ctxt_switch_to+0x1d8/0x1e5
(XEN)    [<ffff82d080165ea2>] domain.c#__context_switch+0x191/0x3d2
(XEN)    [<ffff82d080169fb6>] context_switch+0x147/0xee7
(XEN)    [<ffff82d08012cf82>] schedule.c#schedule+0x5ae/0x609
(XEN)    [<ffff82d080130180>] softirq.c#__do_softirq+0x7f/0x8a
(XEN)    [<ffff82d0801301d5>] do_softirq+0x13/0x15
(XEN)    [<ffff82d0801656b2>] domain.c#idle_loop+0x55/0x62
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 7:
(XEN) Assertion 'current == idle_vcpu[smp_processor_id()]' failed at domain.c:2168
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...
(XEN) Resetting with ACPI MEMORY or I/O RESET_REG.

Thanks,
Feng

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Jan Beulich]

>>> On 09.10.16 at 07:35, <feng.wu@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, September 28, 2016 5:59 PM
>> >>> On 28.09.16 at 08:51, <feng.wu@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Monday, September 26, 2016 8:58 PM
>> >> >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> >> > We don't set the affinity for posted format IRTE, since the
>> >> > destination of these interrupts is vCPU and the vCPU affinity
>> >> > is set during vCPU scheduling.
>> >>
>> >> I'm quite sure I did point out before that you talk about just affinity
>> >> changes here, yet ...
>> >>
>> >> > --- a/xen/drivers/passthrough/vtd/intremap.c
>> >> > +++ b/xen/drivers/passthrough/vtd/intremap.c
>> >> > @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
>> >> >      remap_rte->address_hi = 0;
>> >> >      remap_rte->data = index - i;
>> >> >
>> >> > -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> >> > -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>> >> > -    iommu_flush_iec_index(iommu, 0, index);
>> >> > +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
>> >> > +    {
>> >> > +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> >> > +        iommu_flush_cache_entry(iremap_entry, sizeof(struct
>> > iremap_entry));
>> >> > +        iommu_flush_iec_index(iommu, 0, index);
>> >> > +    }
>> >>
>> >> ... you suppress the update also in other cases. This _may_ be safe
>> >> at present, but you're digging a hole for someone else to fall into
>> >> down the road. Hence at the very least you should, in a to be added
>> >> "else" path, ASSERT() that nothing in the descriptor changed except
>> >> the bits representing affinity. Even better would be if in fact you
>> >> suppressed the update+flush only when nothing other than dst
>> >> changed.
>> >
>> > I am a little confused about the above comments, Posted IRTE and
>> > Remapped IRTE has different format, and when the IRTE is in posted
>> > format, typically, the updated information (delivery mode, dest mode,
>> > vector, dest, etc) has no meaning for posted IRTE. However, there are
>> > indeed some shared part between the two format as below:
>> > - p
>> > - fpd
>> > - im
>> > - sid
>> > - sq
>> > - svt
>> >
>> > Bits like sid, sq, and svt are not changed in this function,
>> 
>> How do you know? Judging just from current callers is - as said
>> before - calling for trouble down the road. And the function clearly
>> creates a brand new IRTE, which fully replaces the previous one.
> 
> This function copy the old IRTE to 'new_ire' and it doesn't touch
> fields like 'sid', 'sq', and 'svt', so how could these bits get changed?

The function calls set_msi_source_id(), doesn't it? In fact it looks
like the initial memcpy() is (almost) pointless, and might better be
replaced by explicit copying of existing fields (if any). And the
"if any" is quite relevant here, considering that the function gets
called also for installing brand new IRTEs, which then obviously
can't have any fields to retain.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/6] VMX: Statically assign two PI hooks

[Jan Beulich]

>>> On 09.10.16 at 10:30, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, September 28, 2016 5:39 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org 
>> Subject: RE: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
>> 
>> >>> On 28.09.16 at 08:48, <feng.wu@intel.com> wrote:
>> 
>> >
>> >> -----Original Message-----
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Monday, September 26, 2016 8:10 PM
>> >> To: Wu, Feng <feng.wu@intel.com>
>> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> >> devel@lists.xen.org 
>> >> Subject: Re: [Xen-devel] [PATCH v4 1/6] VMX: Statically assign two PI hooks
>> >>
>> >> >>> On 26.09.16 at 13:37, <JBeulich@suse.com> wrote:
>> >> >>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
>> >> >> PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are
>> >> >> needed even when any previously assigned device is detached
>> >> >> from the domain. Since 'SN' bit is also used to control the
>> >> >> CPU side PI and we change the state of SN bit in these two
>> >> >> functions, then evaluate this bit in vmx_deliver_posted_intr()
>> >> >> when trying to deliver the interrupt in posted way via software.
>> >> >> The problem is if we deassign the hooks while the vCPU is runnable
>> >> >> in the runqueue with 'SN' set, all the furture notificaton event
>> >> >> will be suppressed. This patch makes these two hooks statically
>> >> >> assigned.
>> >> >
>> >> > So if only SN left set is a problem, why do you need to also keep
>> >> > vmx_pi_switch_from in place? It's vmx_pi_switch_to() which clears
>> >> > the bit, and vmx_deliver_posted_intr() doesn't actively change it.
>> >>
>> >> And it doesn't appear completely unreasonable for
>> >> vmx_pi_switch_to() to remove itself (when it gets run with
>> >> the "from" hook still NULL and no new device being in the
>> >> process of getting assigned).
>> >
>> > I think this may introduce extra complex to the situation:
>> > 1. Especially for "no new device being in the process of getting assigned",
>> > since device assignment can be happened simultaneous when this function
>> > gets called, so does it mean we need to use a lock to protect it?
>> 
>> Since device addition/removal is already protected by a lock, this
>> would at least seem not impossible to do without causing lock
>> conflicts (and would certainly be required, yes).
> 
> I use pcidevs_lock()/pcidevs_unlock() in vmx_pi_switch_to() since this lock
> is held while device assignment. However, I got the following call trace during
> booting the guest. From the message, seems we cannot acquire that lock
> in this function.

Ah, yes - quite obviously, as we're running with interrupts disabled
there.

> Back to the original question, maybe it is worth remain the "to" hooks, and
> we might go too far if we really want to zap it.

Well, I would have preferred to clear such hooks if possible, and
it still does look to be possible (albeit not straightforward), but
since you don't appear to have much interest in this, and I don't
have the time to repeatedly try and suggest possibilities to try /
investigate (as I'd really expect you to do such investigations), I
guess I'll give up here. I'd like you to make very clear (in a code
comment) though that keeping the hook in place is not a functional
requirement, but just a means to ease the current implementation.
That way anyone with more interest in efficiency will know that it's
permissible to (carefully) remove that hook once no longer needed.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

[Wu, Feng]

> >> How do you know? Judging just from current callers is - as said
> >> before - calling for trouble down the road. And the function clearly
> >> creates a brand new IRTE, which fully replaces the previous one.
> >
> > This function copy the old IRTE to 'new_ire' and it doesn't touch
> > fields like 'sid', 'sq', and 'svt', so how could these bits get changed?
> 
> The function calls set_msi_source_id(), doesn't it? 

Oh, yes, it does! Sorry for the negligence.

Thanks,
Feng

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel