Regression PCI passthrough from 4.5.5 to 4.6.0-rc1

Commit Message

Roger Pau Monne Aug. 22, 2017, 8:06 a.m. UTC

On Mon, Aug 21, 2017 at 04:18:56PM +0100, Roger Pau Monné wrote:
> On Mon, Aug 21, 2017 at 09:14:45AM -0600, Jan Beulich wrote:
> > >>> On 21.08.17 at 16:49, <[e-mail]> wrote:
> > > Another option is to (ab)use the msi.gflags field to add another flag
> > > in order to signal Xen whether the interrupt should be unmasked. This
> > > is in line with what you suggest below.
> > 
> > From a brief look it looks like this would be doable, but the way these
> > flags are being communicated is rather ugly (the values used here
> > aren't part of the public interface, and hence it wasn't immediately
> > clear whether using one of the unused bits would be an option, but
> > it looks like it is).
> 
> Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
> used to signal to Xen whether the interrupt should be unmasked after
> binding. I have a half-drafted patch, will finish it now.

Hello,

Andreas, could you please give a try to the attached two patches? One
is for Xen and the other one is for QEMU.

Thanks, Roger.

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index ff9a79f5d2..971441dbab 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -24,6 +24,7 @@
 #define XEN_PT_GFLAGS_SHIFT_DM             9
 #define XEN_PT_GFLAGSSHIFT_DELIV_MODE     12
 #define XEN_PT_GFLAGSSHIFT_TRG_MODE       15
+#define XEN_PT_GFLAGSSHIFT_UNMASKED       16
 
 #define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
 
@@ -155,7 +156,8 @@ static int msi_msix_update(XenPCIPassthroughState *s,
                            int pirq,
                            bool is_msix,
                            int msix_entry,
-                           int *old_pirq)
+                           int *old_pirq,
+                           bool masked)
 {
     PCIDevice *d = &s->dev;
     uint8_t gvec = msi_vector(data);
@@ -171,6 +173,8 @@ static int msi_msix_update(XenPCIPassthroughState *s,
         table_addr = s->msix->mmio_base_addr;
     }
 
+    gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
+
     rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
                                   pirq, gflags, table_addr);
 
@@ -274,7 +278,7 @@ int xen_pt_msi_update(XenPCIPassthroughState *s)
 {
     XenPTMSI *msi = s->msi;
     return msi_msix_update(s, msi_addr64(msi), msi->data, msi->pirq,
-                           false, 0, &msi->pirq);
+                           false, 0, &msi->pirq, false);
 }
 
 void xen_pt_msi_disable(XenPCIPassthroughState *s)
@@ -355,7 +359,8 @@ static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr,
     }
 
     rc = msi_msix_update(s, entry->addr, entry->data, pirq, true,
-                         entry_nr, &entry->pirq);
+                         entry_nr, &entry->pirq,
+                         vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
     if (!rc) {
         entry->updated = false;

Comments

Andreas Kinzler Aug. 23, 2017, 5:13 p.m. UTC | #1

>> > From a brief look it looks like this would be doable, but the way  
>> these flags are being communicated is rather ugly (the values used here
>> > aren't part of the public interface, and hence it wasn't immediately
>> > clear whether using one of the unused bits would be an option, but
>> > it looks like it is).
>> Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
>> used to signal to Xen whether the interrupt should be unmasked after
>> binding. I have a half-drafted patch, will finish it now.
> Andreas, could you please give a try to the attached two patches? One
> is for Xen and the other one is for QEMU.

Seems to work after I fixed a bug ;-)

-gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
+gflags |= masked ? 0 : (1 << XEN_PT_GFLAGSSHIFT_UNMASKED);

Please let Jan and/or others review the patches.

Regards Andreas

Roger Pau Monne Aug. 24, 2017, 8:23 a.m. UTC | #2

On Wed, Aug 23, 2017 at 07:13:00PM +0200, Andreas Kinzler wrote:
> > > > From a brief look it looks like this would be doable, but the way
> > > these flags are being communicated is rather ugly (the values used
> > > here
> > > > aren't part of the public interface, and hence it wasn't immediately
> > > > clear whether using one of the unused bits would be an option, but
> > > > it looks like it is).
> > > Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
> > > used to signal to Xen whether the interrupt should be unmasked after
> > > binding. I have a half-drafted patch, will finish it now.
> > Andreas, could you please give a try to the attached two patches? One
> > is for Xen and the other one is for QEMU.
> 
> Seems to work after I fixed a bug ;-)
> 
> -gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
> +gflags |= masked ? 0 : (1 << XEN_PT_GFLAGSSHIFT_UNMASKED);
> 
> Please let Jan and/or others review the patches.

Thanks. I would like to add your tested-by if possible, since I'm not
able to trigger this behavior myself.

Roger.

Pasi Kärkkäinen Nov. 2, 2017, 5:02 p.m. UTC | #3

On Thu, Aug 24, 2017 at 09:23:16AM +0100, Roger Pau Monné wrote:
> On Wed, Aug 23, 2017 at 07:13:00PM +0200, Andreas Kinzler wrote:
> > > > > From a brief look it looks like this would be doable, but the way
> > > > these flags are being communicated is rather ugly (the values used
> > > > here
> > > > > aren't part of the public interface, and hence it wasn't immediately
> > > > > clear whether using one of the unused bits would be an option, but
> > > > > it looks like it is).
> > > > Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
> > > > used to signal to Xen whether the interrupt should be unmasked after
> > > > binding. I have a half-drafted patch, will finish it now.
> > > Andreas, could you please give a try to the attached two patches? One
> > > is for Xen and the other one is for QEMU.
> > 
> > Seems to work after I fixed a bug ;-)
> > 
> > -gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
> > +gflags |= masked ? 0 : (1 << XEN_PT_GFLAGSSHIFT_UNMASKED);
> > 
> > Please let Jan and/or others review the patches.
> 
> Thanks. I would like to add your tested-by if possible, since I'm not
> able to trigger this behavior myself.
> 

Hmm.. did these patches get merged / acked? 


Thanks,

-- Pasi

> Roger.
>

Roger Pau Monne Nov. 2, 2017, 5:11 p.m. UTC | #4

On Thu, Nov 02, 2017 at 07:02:49PM +0200, Pasi Kärkkäinen wrote:
> On Thu, Aug 24, 2017 at 09:23:16AM +0100, Roger Pau Monné wrote:
> > On Wed, Aug 23, 2017 at 07:13:00PM +0200, Andreas Kinzler wrote:
> > > > > > From a brief look it looks like this would be doable, but the way
> > > > > these flags are being communicated is rather ugly (the values used
> > > > > here
> > > > > > aren't part of the public interface, and hence it wasn't immediately
> > > > > > clear whether using one of the unused bits would be an option, but
> > > > > > it looks like it is).
> > > > > Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
> > > > > used to signal to Xen whether the interrupt should be unmasked after
> > > > > binding. I have a half-drafted patch, will finish it now.
> > > > Andreas, could you please give a try to the attached two patches? One
> > > > is for Xen and the other one is for QEMU.
> > > 
> > > Seems to work after I fixed a bug ;-)
> > > 
> > > -gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
> > > +gflags |= masked ? 0 : (1 << XEN_PT_GFLAGSSHIFT_UNMASKED);
> > > 
> > > Please let Jan and/or others review the patches.
> > 
> > Thanks. I would like to add your tested-by if possible, since I'm not
> > able to trigger this behavior myself.
> > 
> 
> Hmm.. did these patches get merged / acked? 

Yes, both the QEMU and the Xen sides have been merged.

Roger.

Pasi Kärkkäinen Nov. 2, 2017, 5:13 p.m. UTC | #5

On Thu, Nov 02, 2017 at 05:11:17PM +0000, Roger Pau Monné wrote:
> On Thu, Nov 02, 2017 at 07:02:49PM +0200, Pasi Kärkkäinen wrote:
> > On Thu, Aug 24, 2017 at 09:23:16AM +0100, Roger Pau Monné wrote:
> > > On Wed, Aug 23, 2017 at 07:13:00PM +0200, Andreas Kinzler wrote:
> > > > > > > From a brief look it looks like this would be doable, but the way
> > > > > > these flags are being communicated is rather ugly (the values used
> > > > > > here
> > > > > > > aren't part of the public interface, and hence it wasn't immediately
> > > > > > > clear whether using one of the unused bits would be an option, but
> > > > > > > it looks like it is).
> > > > > > Yes, it's not pretty... Last used bit is 15, hence bit 16 could be
> > > > > > used to signal to Xen whether the interrupt should be unmasked after
> > > > > > binding. I have a half-drafted patch, will finish it now.
> > > > > Andreas, could you please give a try to the attached two patches? One
> > > > > is for Xen and the other one is for QEMU.
> > > > 
> > > > Seems to work after I fixed a bug ;-)
> > > > 
> > > > -gflags |= masked ? 0 : XEN_PT_GFLAGSSHIFT_UNMASKED;
> > > > +gflags |= masked ? 0 : (1 << XEN_PT_GFLAGSSHIFT_UNMASKED);
> > > > 
> > > > Please let Jan and/or others review the patches.
> > > 
> > > Thanks. I would like to add your tested-by if possible, since I'm not
> > > able to trigger this behavior myself.
> > > 
> > 
> > Hmm.. did these patches get merged / acked? 
> 
> Yes, both the QEMU and the Xen sides have been merged.
> 

Great, thanks a lot!


> Roger.


-- Pasi

Patch

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index ffeaf70be6..7e63007fbf 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -343,13 +343,16 @@  int pt_irq_create_bind(
         uint8_t dest, dest_mode, delivery_mode;
         int dest_vcpu_id;
         const struct vcpu *vcpu;
+        uint32_t gflags = pt_irq_bind->u.msi.gflags & ~VMSI_UNMASKED;
+        struct irq_desc *desc;
+        unsigned long flags;
 
         if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
         {
             pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED | HVM_IRQ_DPCI_MACH_MSI |
                                HVM_IRQ_DPCI_GUEST_MSI;
             pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
-            pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
+            pirq_dpci->gmsi.gflags = gflags;
             /*
              * 'pt_irq_create_bind' can be called after 'pt_irq_destroy_bind'.
              * The 'pirq_cleanup_check' which would free the structure is only
@@ -402,13 +405,13 @@  int pt_irq_create_bind(
 
             /* If pirq is already mapped as vmsi, update guest data/addr. */
             if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec ||
-                 pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags )
+                 pirq_dpci->gmsi.gflags != gflags )
             {
                 /* Directly clear pending EOIs before enabling new MSI info. */
                 pirq_guest_eoi(info);
 
                 pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
-                pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags;
+                pirq_dpci->gmsi.gflags = gflags;
             }
         }
         /* Calculate dest_vcpu_id for MSI-type pirq migration. */
@@ -439,6 +442,13 @@  int pt_irq_create_bind(
             pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
                            info, pirq_dpci->gmsi.gvec);
 
+        desc = irq_to_desc(info->arch.irq);
+        ASSERT(desc);
+
+        spin_lock_irqsave(&desc->lock, flags);
+        guest_mask_msi_irq(desc, !(pt_irq_bind->u.msi.gflags & VMSI_UNMASKED));
+        spin_unlock_irqrestore(&desc->lock, flags);
+
         break;
     }
 
diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h
index 0d2c72c109..bdc843fb9a 100644
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -56,6 +56,7 @@  struct dev_intx_gsi_link {
 #define VMSI_DM_MASK      0x200
 #define VMSI_DELIV_MASK   0x7000
 #define VMSI_TRIG_MODE    0x8000
+#define VMSI_UNMASKED     0x10000
 
 #define GFLAGS_SHIFT_RH             8
 #define GFLAGS_SHIFT_DELIV_MODE     12