[SRU][Xenial][PATCH v2 0/3] Fix corruption on blocked_vcpu_on_cpu list

[Ioanna Alifieraki]

BugLink: https://bugs.launchpad.net/bugs/1908428

The following 3 patches fix the bug reported in [1].
They are backported to apply on 4.4 kernels.
Backport is required becasue upstream commit
bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
which renames  vmx_pre/post_block functions to pi_pre/post_block
is missing from 4.4.
Original patches come from [2] and have been accepted upstream.

[1] https://marc.info/?l=kvm&m=149559827906211&w=2
[2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../ 

Paolo Bonzini (3):
  KVM: VMX: extract __pi_post_block
  KVM: VMX: avoid double list add with VT-d posted interrupts
  KVM: VMX: simplify and fix vmx_vcpu_pi_load

 arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 90 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

From: Paolo Bonzini <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1908428

Simple code movement patch, preparing for the next one.

Cc: Huangweidong <[hidden email]>
Cc: Gonglei <[hidden email]>
Cc: wangxin <[hidden email]>
Cc: Radim Krčmář <[hidden email]>
Tested-by: Longpeng (Mike) <[hidden email]>
Cc: [hidden email]
Signed-off-by: Paolo Bonzini <[hidden email]>
(backported from commit cd39e1176d320157831ce030b4c869bd2d5eb142)
[hunk 2 : Original commit changes pi_post_block function.
This function is not present in 4.4 kernel, see upstream commit
bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block).
Make relevant changes in vmx_post_block function.]
Signed-off-by: Ioanna Alifieraki <[hidden email]>
---
 arch/x86/kvm/vmx.c | 71 +++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 46d1293baa04..21d8d716ab5e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11145,6 +11145,43 @@ static void vmx_enable_log_dirty_pt_masked(struct kvm *kvm,
  kvm_mmu_clear_dirty_pt_masked(kvm, memslot, offset, mask);
 }
 
+static void __pi_post_block(struct kvm_vcpu *vcpu)
+{
+ struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
+ struct pi_desc old, new;
+ unsigned int dest;
+ unsigned long flags;
+
+ do {
+ old.control = new.control = pi_desc->control;
+
+ dest = cpu_physical_id(vcpu->cpu);
+
+ if (x2apic_enabled())
+ new.ndst = dest;
+ else
+ new.ndst = (dest << 8) & 0xFF00;
+
+ /* Allow posting non-urgent interrupts */
+ new.sn = 0;
+
+ /* set 'NV' to 'notification vector' */
+ new.nv = POSTED_INTR_VECTOR;
+ } while (cmpxchg64(&pi_desc->control, old.control,
+ new.control) != old.control);
+
+ if(vcpu->pre_pcpu != -1) {
+ spin_lock_irqsave(
+ &per_cpu(blocked_vcpu_on_cpu_lock,
+ vcpu->pre_pcpu), flags);
+ list_del(&vcpu->blocked_vcpu_list);
+ spin_unlock_irqrestore(
+ &per_cpu(blocked_vcpu_on_cpu_lock,
+ vcpu->pre_pcpu), flags);
+ vcpu->pre_pcpu = -1;
+ }
+}
+
 /*
  * This routine does the following things for vCPU which is going
  * to be blocked if VT-d PI is enabled.
@@ -11226,43 +11263,11 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
 
 static void vmx_post_block(struct kvm_vcpu *vcpu)
 {
- struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
- struct pi_desc old, new;
- unsigned int dest;
- unsigned long flags;
-
  if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
  !irq_remapping_cap(IRQ_POSTING_CAP))
  return;
 
- do {
- old.control = new.control = pi_desc->control;
-
- dest = cpu_physical_id(vcpu->cpu);
-
- if (x2apic_enabled())
- new.ndst = dest;
- else
- new.ndst = (dest << 8) & 0xFF00;
-
- /* Allow posting non-urgent interrupts */
- new.sn = 0;
-
- /* set 'NV' to 'notification vector' */
- new.nv = POSTED_INTR_VECTOR;
- } while (cmpxchg64(&pi_desc->control, old.control,
-   new.control) != old.control);
-
- if(vcpu->pre_pcpu != -1) {
- spin_lock_irqsave(
- &per_cpu(blocked_vcpu_on_cpu_lock,
- vcpu->pre_pcpu), flags);
- list_del(&vcpu->blocked_vcpu_list);
- spin_unlock_irqrestore(
- &per_cpu(blocked_vcpu_on_cpu_lock,
- vcpu->pre_pcpu), flags);
- vcpu->pre_pcpu = -1;
- }
+ __pi_post_block(vcpu);
 }
 
 /*
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

From: Paolo Bonzini <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1908428

In some cases, for example involving hot-unplug of assigned
devices, pi_post_block can forget to remove the vCPU from the
blocked_vcpu_list.  When this happens, the next call to
pi_pre_block corrupts the list.

Fix this in two ways.  First, check vcpu->pre_pcpu in pi_pre_block
and WARN instead of adding the element twice in the list.  Second,
always do the list removal in pi_post_block if vcpu->pre_pcpu is
set (not -1).

The new code keeps interrupts disabled for the whole duration of
pi_pre_block/pi_post_block.  This is not strictly necessary, but
easier to follow.  For the same reason, PI.ON is checked only
after the cmpxchg, and to handle it we just call the post-block
code.  This removes duplication of the list removal code.

Cc: Huangweidong <[hidden email]>
Cc: Gonglei <[hidden email]>
Cc: wangxin <[hidden email]>
Cc: Radim Krčmář <[hidden email]>
Tested-by: Longpeng (Mike) <[hidden email]>
Cc: [hidden email]
Signed-off-by: Paolo Bonzini <[hidden email]>
(backported from commit 8b306e2f3c41939ea528e6174c88cfbfff893ce1)
[hunk 4 and 5 : Original commit changes pi_pre_block function.
This function is not present in 4.4 kernel, see upstream commit
bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block).
Make relevant changes in vmx_pre_block function.]
Signed-off-by: Ioanna Alifieraki <[hidden email]>
---
 arch/x86/kvm/vmx.c | 61 +++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 21d8d716ab5e..e40ca7f5f346 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11150,10 +11150,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
  struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
  struct pi_desc old, new;
  unsigned int dest;
- unsigned long flags;
 
  do {
  old.control = new.control = pi_desc->control;
+ WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR,
+     "Wakeup handler not enabled while the VCPU is blocked\n");
 
  dest = cpu_physical_id(vcpu->cpu);
 
@@ -11170,14 +11171,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
  } while (cmpxchg64(&pi_desc->control, old.control,
  new.control) != old.control);
 
- if(vcpu->pre_pcpu != -1) {
- spin_lock_irqsave(
- &per_cpu(blocked_vcpu_on_cpu_lock,
- vcpu->pre_pcpu), flags);
+ if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) {
+ spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
  list_del(&vcpu->blocked_vcpu_list);
- spin_unlock_irqrestore(
- &per_cpu(blocked_vcpu_on_cpu_lock,
- vcpu->pre_pcpu), flags);
+ spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
  vcpu->pre_pcpu = -1;
  }
 }
@@ -11197,7 +11194,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
  */
 static int vmx_pre_block(struct kvm_vcpu *vcpu)
 {
- unsigned long flags;
  unsigned int dest;
  struct pi_desc old, new;
  struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
@@ -11206,34 +11202,20 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
  !irq_remapping_cap(IRQ_POSTING_CAP))
  return 0;
 
- vcpu->pre_pcpu = vcpu->cpu;
- spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
-  vcpu->pre_pcpu), flags);
- list_add_tail(&vcpu->blocked_vcpu_list,
-      &per_cpu(blocked_vcpu_on_cpu,
-      vcpu->pre_pcpu));
- spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock,
-       vcpu->pre_pcpu), flags);
+ WARN_ON(irqs_disabled());
+ local_irq_disable();
+ if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
+ vcpu->pre_pcpu = vcpu->cpu;
+ spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
+ list_add_tail(&vcpu->blocked_vcpu_list,
+      &per_cpu(blocked_vcpu_on_cpu,
+       vcpu->pre_pcpu));
+ spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
+ }
 
  do {
  old.control = new.control = pi_desc->control;
 
- /*
- * We should not block the vCPU if
- * an interrupt is posted for it.
- */
- if (pi_test_on(pi_desc) == 1) {
- spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
-  vcpu->pre_pcpu), flags);
- list_del(&vcpu->blocked_vcpu_list);
- spin_unlock_irqrestore(
- &per_cpu(blocked_vcpu_on_cpu_lock,
- vcpu->pre_pcpu), flags);
- vcpu->pre_pcpu = -1;
-
- return 1;
- }
-
  WARN((pi_desc->sn == 1),
      "Warning: SN field of posted-interrupts "
      "is set before blocking\n");
@@ -11258,16 +11240,23 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
  } while (cmpxchg64(&pi_desc->control, old.control,
    new.control) != old.control);
 
- return 0;
+ /* We should not block the vCPU if an interrupt is posted for it.  */
+ if (pi_test_on(pi_desc) == 1)
+ __pi_post_block(vcpu);
+
+ local_irq_enable();
+ return (vcpu->pre_pcpu == -1);
 }
 
 static void vmx_post_block(struct kvm_vcpu *vcpu)
 {
- if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
- !irq_remapping_cap(IRQ_POSTING_CAP))
+ if (vcpu->pre_pcpu == -1)
  return;
 
+ WARN_ON(irqs_disabled());
+ local_irq_disable();
  __pi_post_block(vcpu);
+ local_irq_enable();
 }
 
 /*
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

From: Paolo Bonzini <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1908428

The simplify part: do not touch pi_desc.nv, we can set it when the
VCPU is first created.  Likewise, pi_desc.sn is only handled by
vmx_vcpu_pi_load, do not touch it in __pi_post_block.

The fix part: do not check kvm_arch_has_assigned_device, instead
check the SN bit to figure out whether vmx_vcpu_pi_put ran before.
This matches what the previous patch did in pi_post_block.

Cc: Huangweidong <[hidden email]>
Cc: Gonglei <[hidden email]>
Cc: wangxin <[hidden email]>
Cc: Radim Krčmář <[hidden email]>
Tested-by: Longpeng (Mike) <[hidden email]>
Cc: [hidden email]
Signed-off-by: Paolo Bonzini <[hidden email]>
(backported from commit 31afb2ea2b10a7d17ce3db4cdb0a12b63b2fe08a)
[hunk 1 : resolve conflict on if-clause removed.]
Signed-off-by: Ioanna Alifieraki <[hidden email]>
---
 arch/x86/kvm/vmx.c | 67 ++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e40ca7f5f346..421703d5ed02 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2235,42 +2235,41 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
  struct pi_desc old, new;
  unsigned int dest;
 
- if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
- !irq_remapping_cap(IRQ_POSTING_CAP))
+ /*
+ * In case of hot-plug or hot-unplug, we may have to undo
+ * vmx_vcpu_pi_put even if there is no assigned device.  And we
+ * always keep PI.NDST up to date for simplicity: it makes the
+ * code easier, and CPU migration is not a fast path.
+ */
+ if (!pi_test_sn(pi_desc) && vcpu->cpu == cpu)
+ return;
+
+ /*
+ * First handle the simple case where no cmpxchg is necessary; just
+ * allow posting non-urgent interrupts.
+ *
+ * If the 'nv' field is POSTED_INTR_WAKEUP_VECTOR, do not change
+ * PI.NDST: pi_post_block will do it for us and the wakeup_handler
+ * expects the VCPU to be on the blocked_vcpu_list that matches
+ * PI.NDST.
+ */
+ if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR ||
+    vcpu->cpu == cpu) {
+ pi_clear_sn(pi_desc);
  return;
+ }
 
+ /* The full case.  */
  do {
  old.control = new.control = pi_desc->control;
 
- /*
- * If 'nv' field is POSTED_INTR_WAKEUP_VECTOR, there
- * are two possible cases:
- * 1. After running 'pre_block', context switch
- *    happened. For this case, 'sn' was set in
- *    vmx_vcpu_put(), so we need to clear it here.
- * 2. After running 'pre_block', we were blocked,
- *    and woken up by some other guy. For this case,
- *    we don't need to do anything, 'pi_post_block'
- *    will do everything for us. However, we cannot
- *    check whether it is case #1 or case #2 here
- *    (maybe, not needed), so we also clear sn here,
- *    I think it is not a big deal.
- */
- if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR) {
- if (vcpu->cpu != cpu) {
- dest = cpu_physical_id(cpu);
-
- if (x2apic_enabled())
- new.ndst = dest;
- else
- new.ndst = (dest << 8) & 0xFF00;
- }
+ dest = cpu_physical_id(cpu);
 
- /* set 'NV' to 'notification vector' */
- new.nv = POSTED_INTR_VECTOR;
- }
+ if (x2apic_enabled())
+ new.ndst = dest;
+ else
+ new.ndst = (dest << 8) & 0xFF00;
 
- /* Allow posting non-urgent interrupts */
  new.sn = 0;
  } while (cmpxchg64(&pi_desc->control, old.control,
    new.control) != old.control);
@@ -9283,6 +9282,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
  vmx->nested.current_vmptr = -1ull;
  vmx->nested.current_vmcs12 = NULL;
 
+ /*
+ * Enforce invariant: pi_desc.nv is always either POSTED_INTR_VECTOR
+ * or POSTED_INTR_WAKEUP_VECTOR.
+ */
+ vmx->pi_desc.nv = POSTED_INTR_VECTOR;
+ vmx->pi_desc.sn = 1;
+
  return &vmx->vcpu;
 
 free_vmcs:
@@ -11163,9 +11169,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
  else
  new.ndst = (dest << 8) & 0xFF00;
 
- /* Allow posting non-urgent interrupts */
- new.sn = 0;
-
  /* set 'NV' to 'notification vector' */
  new.nv = POSTED_INTR_VECTOR;
  } while (cmpxchg64(&pi_desc->control, old.control,
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[William Breathitt Gray]

On Tue, Jan 05, 2021 at 11:54:57PM +0000, Ioanna Alifieraki wrote:

> BugLink: https://bugs.launchpad.net/bugs/1908428
>
> The following 3 patches fix the bug reported in [1].
> They are backported to apply on 4.4 kernels.
> Backport is required becasue upstream commit
> bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> which renames  vmx_pre/post_block functions to pi_pre/post_block
> is missing from 4.4.
> Original patches come from [2] and have been accepted upstream.
>
> [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../ 
>
> Paolo Bonzini (3):
>   KVM: VMX: extract __pi_post_block
>   KVM: VMX: avoid double list add with VT-d posted interrupts
>   KVM: VMX: simplify and fix vmx_vcpu_pi_load
>
>  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
>  1 file changed, 87 insertions(+), 90 deletions(-)
>
> --
> 2.17.1

Upstream commit
bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
looks pretty simple. If you pick that up first, then PATCH 1 and PATCH 2
can be cherry-picks right?

William Breathitt Gray

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

Hi William,

Thanks for the feedback. Even with commit bc22512bb24c(kvm: vmx:
rename vmx_pre/post_block to pi_pre/post_block)
the patches still won't cherry-pick because upstream commit
a0052191624e(kvm: vmx: check apicv is active before using VT-d posted
interrupt) is missing, however the backport
is cleaner and much simpler.
I'm happy to resubmit the patchset with commit bc22512bb24c(kvm: vmx:
rename vmx_pre/post_block to pi_pre/post_block)
included if it is a better option.

On Thu, Jan 7, 2021 at 8:36 AM William Breathitt Gray
<[hidden email]> wrote:

>
> On Tue, Jan 05, 2021 at 11:54:57PM +0000, Ioanna Alifieraki wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1908428
> >
> > The following 3 patches fix the bug reported in [1].
> > They are backported to apply on 4.4 kernels.
> > Backport is required becasue upstream commit
> > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > which renames  vmx_pre/post_block functions to pi_pre/post_block
> > is missing from 4.4.
> > Original patches come from [2] and have been accepted upstream.
> >
> > [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> > [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
> >
> > Paolo Bonzini (3):
> >   KVM: VMX: extract __pi_post_block
> >   KVM: VMX: avoid double list add with VT-d posted interrupts
> >   KVM: VMX: simplify and fix vmx_vcpu_pi_load
> >
> >  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
> >  1 file changed, 87 insertions(+), 90 deletions(-)
> >
> > --
> > 2.17.1
>
> Upstream commit
> bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> looks pretty simple. If you pick that up first, then PATCH 1 and PATCH 2
> can be cherry-picks right?
>
> William Breathitt Gray

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[William Breathitt Gray]

On Thu, Jan 14, 2021 at 03:21:41PM +0000, Ioanna Alifieraki wrote:
> Hi William,
>
> Thanks for the feedback. Even with commit bc22512bb24c(kvm: vmx:
> rename vmx_pre/post_block to pi_pre/post_block)
> the patches still won't cherry-pick because upstream commit
> a0052191624e(kvm: vmx: check apicv is active before using VT-d posted
> interrupt) is missing, however the backport
> is cleaner and much simpler.

Okay, I see what you mean, this would have more dependencies than just
commit bc22512bb24c. Well if the backport is a better solution here,
then let's go with it.

Thanks,

Acked-by: William Breathitt Gray <[hidden email]>

> I'm happy to resubmit the patchset with commit bc22512bb24c(kvm: vmx:
> rename vmx_pre/post_block to pi_pre/post_block)
> included if it is a better option.
>
> On Thu, Jan 7, 2021 at 8:36 AM William Breathitt Gray
> <[hidden email]> wrote:
> >
> > On Tue, Jan 05, 2021 at 11:54:57PM +0000, Ioanna Alifieraki wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/1908428
> > >
> > > The following 3 patches fix the bug reported in [1].
> > > They are backported to apply on 4.4 kernels.
> > > Backport is required becasue upstream commit
> > > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > > which renames  vmx_pre/post_block functions to pi_pre/post_block
> > > is missing from 4.4.
> > > Original patches come from [2] and have been accepted upstream.
> > >
> > > [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> > > [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
> > >
> > > Paolo Bonzini (3):
> > >   KVM: VMX: extract __pi_post_block
> > >   KVM: VMX: avoid double list add with VT-d posted interrupts
> > >   KVM: VMX: simplify and fix vmx_vcpu_pi_load
> > >
> > >  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
> > >  1 file changed, 87 insertions(+), 90 deletions(-)
> > >
> > > --
> > > 2.17.1
> >
> > Upstream commit
> > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > looks pretty simple. If you pick that up first, then PATCH 1 and PATCH 2
> > can be cherry-picks right?
> >
> > William Breathitt Gray

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

Ok, I didn't express myself clearly earlier.
What I mean is that if we pull in commit bc22512bb24c, the other
patches still need backporting
because of commit  a0052191624e, but in this case with  commit
bc22512bb24c in place the backport of the other
commits will be cleaner and simpler.
Commit bc22512bb24c does not depend on commit  a0052191624e.
My apologies for the confusion.

On Thu, Jan 14, 2021 at 11:11 PM William Breathitt Gray
<[hidden email]> wrote:

>
> On Thu, Jan 14, 2021 at 03:21:41PM +0000, Ioanna Alifieraki wrote:
> > Hi William,
> >
> > Thanks for the feedback. Even with commit bc22512bb24c(kvm: vmx:
> > rename vmx_pre/post_block to pi_pre/post_block)
> > the patches still won't cherry-pick because upstream commit
> > a0052191624e(kvm: vmx: check apicv is active before using VT-d posted
> > interrupt) is missing, however the backport
> > is cleaner and much simpler.
>
> Okay, I see what you mean, this would have more dependencies than just
> commit bc22512bb24c. Well if the backport is a better solution here,
> then let's go with it.
>
> Thanks,
>
> Acked-by: William Breathitt Gray <[hidden email]>
>
> > I'm happy to resubmit the patchset with commit bc22512bb24c(kvm: vmx:
> > rename vmx_pre/post_block to pi_pre/post_block)
> > included if it is a better option.
> >
> > On Thu, Jan 7, 2021 at 8:36 AM William Breathitt Gray
> > <[hidden email]> wrote:
> > >
> > > On Tue, Jan 05, 2021 at 11:54:57PM +0000, Ioanna Alifieraki wrote:
> > > > BugLink: https://bugs.launchpad.net/bugs/1908428
> > > >
> > > > The following 3 patches fix the bug reported in [1].
> > > > They are backported to apply on 4.4 kernels.
> > > > Backport is required becasue upstream commit
> > > > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > > > which renames  vmx_pre/post_block functions to pi_pre/post_block
> > > > is missing from 4.4.
> > > > Original patches come from [2] and have been accepted upstream.
> > > >
> > > > [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> > > > [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
> > > >
> > > > Paolo Bonzini (3):
> > > >   KVM: VMX: extract __pi_post_block
> > > >   KVM: VMX: avoid double list add with VT-d posted interrupts
> > > >   KVM: VMX: simplify and fix vmx_vcpu_pi_load
> > > >
> > > >  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
> > > >  1 file changed, 87 insertions(+), 90 deletions(-)
> > > >
> > > > --
> > > > 2.17.1
> > >
> > > Upstream commit
> > > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > > looks pretty simple. If you pick that up first, then PATCH 1 and PATCH 2
> > > can be cherry-picks right?
> > >
> > > William Breathitt Gray

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[William Breathitt Gray]

On Thu, Jan 14, 2021 at 11:31:34PM +0000, Ioanna Alifieraki wrote:
> Ok, I didn't express myself clearly earlier.
> What I mean is that if we pull in commit bc22512bb24c, the other
> patches still need backporting
> because of commit  a0052191624e, but in this case with  commit
> bc22512bb24c in place the backport of the other
> commits will be cleaner and simpler.
> Commit bc22512bb24c does not depend on commit  a0052191624e.
> My apologies for the confusion.

Ah, I understand now. In that case, resubmit a version 3 of this with
commit bc22512bb24c. I think that will keep the changes in this patchset
cleaner for the future. If you think commit a0052191624e will help as
well, then you can pick that up too (it looks like a simple and useful
bug fix).

(A side note: when you reply on the mailing list, please reply
underneath the message; replying underneath makes the discussion easier
to follow for other readers.)

Thanks,

William Breathitt Gray

>
> On Thu, Jan 14, 2021 at 11:11 PM William Breathitt Gray
> <[hidden email]> wrote:
> >
> > On Thu, Jan 14, 2021 at 03:21:41PM +0000, Ioanna Alifieraki wrote:
> > > Hi William,
> > >
> > > Thanks for the feedback. Even with commit bc22512bb24c(kvm: vmx:
> > > rename vmx_pre/post_block to pi_pre/post_block)
> > > the patches still won't cherry-pick because upstream commit
> > > a0052191624e(kvm: vmx: check apicv is active before using VT-d posted
> > > interrupt) is missing, however the backport
> > > is cleaner and much simpler.
> >
> > Okay, I see what you mean, this would have more dependencies than just
> > commit bc22512bb24c. Well if the backport is a better solution here,
> > then let's go with it.
> >
> > Thanks,
> >
> > Acked-by: William Breathitt Gray <[hidden email]>
> >
> > > I'm happy to resubmit the patchset with commit bc22512bb24c(kvm: vmx:
> > > rename vmx_pre/post_block to pi_pre/post_block)
> > > included if it is a better option.
> > >
> > > On Thu, Jan 7, 2021 at 8:36 AM William Breathitt Gray
> > > <[hidden email]> wrote:
> > > >
> > > > On Tue, Jan 05, 2021 at 11:54:57PM +0000, Ioanna Alifieraki wrote:
> > > > > BugLink: https://bugs.launchpad.net/bugs/1908428
> > > > >
> > > > > The following 3 patches fix the bug reported in [1].
> > > > > They are backported to apply on 4.4 kernels.
> > > > > Backport is required becasue upstream commit
> > > > > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > > > > which renames  vmx_pre/post_block functions to pi_pre/post_block
> > > > > is missing from 4.4.
> > > > > Original patches come from [2] and have been accepted upstream.
> > > > >
> > > > > [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> > > > > [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
> > > > >
> > > > > Paolo Bonzini (3):
> > > > >   KVM: VMX: extract __pi_post_block
> > > > >   KVM: VMX: avoid double list add with VT-d posted interrupts
> > > > >   KVM: VMX: simplify and fix vmx_vcpu_pi_load
> > > > >
> > > > >  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
> > > > >  1 file changed, 87 insertions(+), 90 deletions(-)
> > > > >
> > > > > --
> > > > > 2.17.1
> > > >
> > > > Upstream commit
> > > > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > > > looks pretty simple. If you pick that up first, then PATCH 1 and PATCH 2
> > > > can be cherry-picks right?
> > > >
> > > > William Breathitt Gray

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Stefan Bader-2]

On 06.01.21 00:54, Ioanna Alifieraki wrote:

> BugLink: https://bugs.launchpad.net/bugs/1908428
>
> The following 3 patches fix the bug reported in [1].
> They are backported to apply on 4.4 kernels.
> Backport is required becasue upstream commit
> bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> which renames  vmx_pre/post_block functions to pi_pre/post_block
> is missing from 4.4.
> Original patches come from [2] and have been accepted upstream.
>
> [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../ 
>
> Paolo Bonzini (3):
>   KVM: VMX: extract __pi_post_block
>   KVM: VMX: avoid double list add with VT-d posted interrupts
>   KVM: VMX: simplify and fix vmx_vcpu_pi_load
>
>  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
>  1 file changed, 87 insertions(+), 90 deletions(-)
>

The first patch appears like it can be safely ignored from a change point of
view as it effectively just moves a code section into its own function.
The other 2 are rather obscure in what is getting changed. What I mean is that
its hard to tell what this code does anyway. So I would rely more on the
successful testing. On the regression potential: if you have more insights on
how a regression might show up, that should be rather mentioned. Very vaguely
this changes around vCPU hotplug(?), so in that case regression potential would
be issues in other cases of vCPU addition/removal...

Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Ioanna Alifieraki]

On Wed, Jan 20, 2021 at 8:51 AM Stefan Bader <[hidden email]> wrote:

>
> On 06.01.21 00:54, Ioanna Alifieraki wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1908428
> >
> > The following 3 patches fix the bug reported in [1].
> > They are backported to apply on 4.4 kernels.
> > Backport is required becasue upstream commit
> > bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
> > which renames  vmx_pre/post_block functions to pi_pre/post_block
> > is missing from 4.4.
> > Original patches come from [2] and have been accepted upstream.
> >
> > [1] https://marc.info/?l=kvm&m=149559827906211&w=2
> > [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
> >
> > Paolo Bonzini (3):
> >   KVM: VMX: extract __pi_post_block
> >   KVM: VMX: avoid double list add with VT-d posted interrupts
> >   KVM: VMX: simplify and fix vmx_vcpu_pi_load
> >
> >  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
> >  1 file changed, 87 insertions(+), 90 deletions(-)
> >
>
> The first patch appears like it can be safely ignored from a change point of
> view as it effectively just moves a code section into its own function.
> The other 2 are rather obscure in what is getting changed. What I mean is that
> its hard to tell what this code does anyway. So I would rely more on the
> successful testing. On the regression potential: if you have more insights on
> how a regression might show up, that should be rather mentioned. Very vaguely
> this changes around vCPU hotplug(?), so in that case regression potential would
> be issues in other cases of vCPU addition/removal...
>
> Acked-by: Stefan Bader <[hidden email]>
>

Thanks for the feedback. I cannot reproduce the bug myself. The user
that reported the bug
has tested and so far they don't have mentioned any regression.
FYI, after exchanging a couple of emails with William I sent a version
3 of the patches on Jan 18
with subject :
[SRU][Xenial][PATCH v3 0/4] Fix corruption on blocked_vcpu_on_cpu list

I don't know how you handle these cases in the kernel team. Both v2
and v3 are functionally
the same, in v3 the backport is more straightforward because I pull in
an extra commit.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

[Stefan Bader-2]

On 20.01.21 11:52, Ioanna Alifieraki wrote:

> On Wed, Jan 20, 2021 at 8:51 AM Stefan Bader <[hidden email]> wrote:
>>
>> On 06.01.21 00:54, Ioanna Alifieraki wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1908428
>>>
>>> The following 3 patches fix the bug reported in [1].
>>> They are backported to apply on 4.4 kernels.
>>> Backport is required becasue upstream commit
>>> bc22512bb24c(kvm: vmx: rename vmx_pre/post_block to pi_pre/post_block)
>>> which renames  vmx_pre/post_block functions to pi_pre/post_block
>>> is missing from 4.4.
>>> Original patches come from [2] and have been accepted upstream.
>>>
>>> [1] https://marc.info/?l=kvm&m=149559827906211&w=2
>>> [2] https://lore.kernel.org/lkml/20170606105707.23207-1-pbonzini@.../
>>>
>>> Paolo Bonzini (3):
>>>   KVM: VMX: extract __pi_post_block
>>>   KVM: VMX: avoid double list add with VT-d posted interrupts
>>>   KVM: VMX: simplify and fix vmx_vcpu_pi_load
>>>
>>>  arch/x86/kvm/vmx.c | 177 ++++++++++++++++++++++-----------------------
>>>  1 file changed, 87 insertions(+), 90 deletions(-)
>>>
>>
>> The first patch appears like it can be safely ignored from a change point of
>> view as it effectively just moves a code section into its own function.
>> The other 2 are rather obscure in what is getting changed. What I mean is that
>> its hard to tell what this code does anyway. So I would rely more on the
>> successful testing. On the regression potential: if you have more insights on
>> how a regression might show up, that should be rather mentioned. Very vaguely
>> this changes around vCPU hotplug(?), so in that case regression potential would
>> be issues in other cases of vCPU addition/removal...
>>
>> Acked-by: Stefan Bader <[hidden email]>
>>
>
> Thanks for the feedback. I cannot reproduce the bug myself. The user
> that reported the bug
> has tested and so far they don't have mentioned any regression.
> FYI, after exchanging a couple of emails with William I sent a version
> 3 of the patches on Jan 18
> with subject :
> [SRU][Xenial][PATCH v3 0/4] Fix corruption on blocked_vcpu_on_cpu list
>
> I don't know how you handle these cases in the kernel team. Both v2
> and v3 are functionally
> the same, in v3 the backport is more straightforward because I pull in
> an extra commit.
>

It is not only me but sometimes also me. But generally to help us, please if

there is a v{x}+1 make sure that all previous threads have a final NACK reply.
Otherwise you get people, like me, looking into the wrong version, realizing the
next morning that they wasted time, getting grumpy and confused. And that won't
help either side. ;)

-Stefan



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team