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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, September 26, 2016 9:03 PM
> To: Wu, Feng 
> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com;
> george.dun...@eu.citrix.com; Tian, Kevin ; xen-
> de...@lists.xen.org
> Subject: Re: [PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline
> 
> >>> On 21.09.16 at 04:37,  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 = _cpu(vmx_pi_blocking, cpu).lock;
> > +struct list_head *blocked_vcpus = _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(_online_map);
> > +new_lock = _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: [Xen-devel] [PATCH v4 6/6] VMX: Fixup PI descritpor when cpu is offline

[Jan Beulich]

>>> On 21.09.16 at 04:37,  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 = _cpu(vmx_pi_blocking, cpu).lock;
> +struct list_head *blocked_vcpus = _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(_online_map);
> +new_lock = _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

[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 
---
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 = _cpu(vmx_pi_blocking, cpu).lock;
+struct list_head *blocked_vcpus = _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(_online_map);
+new_lock = _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(>pi_desc.ndst,
+ x2apic_enabled ? dest : MASK_INSR(dest, 
PI_xAPIC_NDST_MASK));
+
+list_move(>pi_blocking.list,
+  _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