[PATCH] x86/pt: fix binding of MSIX entries already unmasked

[PATCH] x86/pt: fix binding of MSIX entries already unmasked

[Roger Pau Monne]

Hello,

The following two patches fix an issue where a MSIX interrupt bound to
a guest when the MSIX entry is already unmasked would be left masked,
and thus the guest would not receive any interrupts from the device.

The following two patches fix this by adding a new flag to the gflags
field used in XEN_DOMCTL_bind_pt_irq so that the caller can request
the interrupt to be unmasked once bound.

Thanks, Roger.

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

[PATCH XEN] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Roger Pau Monne]

The flag is part of the gflags, and should be used to request the
unmask of a MSI interrupt once it's bound.

This is required for the device model in order to be capable of
binding MSIX interrupts that have the entry mask bit already unset at
bind time. Without this fix the interrupts would be left masked.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported by: Andreas Kinzler <hfp@posteo.de>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/drivers/passthrough/io.c  | 23 ++++++++++++++++++++---
 xen/include/asm-x86/hvm/irq.h |  1 +
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 19a21bf85a..f68b31809f 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -342,13 +342,14 @@ 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;
 
         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
@@ -401,13 +402,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. */
@@ -438,6 +439,22 @@ int pt_irq_create_bind(
             pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
                            info, pirq_dpci->gmsi.gvec);
 
+        if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED )
+        {
+            struct irq_desc *desc = irq_to_desc(info->arch.irq);
+            unsigned long flags;
+
+            if ( !desc )
+            {
+                pt_irq_destroy_bind(d, pt_irq_bind);
+                return -EINVAL;
+            }
+
+            spin_lock_irqsave(&desc->lock, flags);
+            guest_mask_msi_irq(desc, false);
+            spin_unlock_irqrestore(&desc->lock, flags);
+        }
+
         break;
     }
 
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 106dc19613..9546c24879 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -136,6 +136,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
-- 
2.11.0 (Apple Git-81)


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

[PATCH QEMU] xen/pt: allow QEMU to request MSI unmasking at bind time

[Roger Pau Monne]

When a MSIX interrupt is bound to a guest using
xc_domain_update_msi_irq (XEN_DOMCTL_bind_pt_irq) the interrupt is
left masked by default.

This causes problems with guests that first configure interrupts and
clean the per-entry MSIX table mask bit and afterwards enable MSIX
globally. In such scenario the Xen internal msixtbl handlers would not
detect the unmasking of MSIX entries because vectors are not yet
registered since MSIX is not enabled, and vectors would be left
masked.

Introduce a new flag in the gflags field to signal Xen whether a MSIX
interrupt should be unmasked after being bound.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: Andreas Kinzler <hfp@posteo.de>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: qemu-devel@nongnu.org
---
 hw/xen/xen_pt_msi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index ff9a79f5d2..c00ac2fd7d 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 : (1u << 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;
-- 
2.11.0 (Apple Git-81)


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

Re: [PATCH QEMU] xen/pt: allow QEMU to request MSI unmasking at bind time

[Jan Beulich]

>>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
> @@ -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);
>  }

I don't think this is correct when the device has mask bits.

Jan


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

Re: [PATCH QEMU] xen/pt: allow QEMU to request MSI unmasking at bind time

[Roger Pau Monne]

On Thu, Aug 24, 2017 at 03:54:21AM -0600, Jan Beulich wrote:
> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
> > @@ -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);
> >  }
> 
> I don't think this is correct when the device has mask bits.

Right, I thought I modified this. I've already changed
pt_irq_create_bind so that the original behavior is keep if the unmask
bit is not set. In this case this should be 'true' in order to keep
the previous behavior, which was correct for MSI.

It also makes me wonder whether it would be better to change the name
of the parameter to "unmask", so that false -> no change to mask, true
-> unconditionally unmask.

Thanks, Roger.

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

Re: [PATCH XEN] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Jan Beulich]

>>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
> @@ -438,6 +439,22 @@ int pt_irq_create_bind(
>              pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
>                             info, pirq_dpci->gmsi.gvec);
>  
> +        if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED )
> +        {
> +            struct irq_desc *desc = irq_to_desc(info->arch.irq);
> +            unsigned long flags;
> +
> +            if ( !desc )
> +            {
> +                pt_irq_destroy_bind(d, pt_irq_bind);
> +                return -EINVAL;
> +            }
> +
> +            spin_lock_irqsave(&desc->lock, flags);
> +            guest_mask_msi_irq(desc, false);
> +            spin_unlock_irqrestore(&desc->lock, flags);
> +        }
> +
>          break;
>      }

I think you would better use pirq_spin_lock_irq_desc() here. And
wouldn't the addition better be moved up a little (perhaps right
after the dropping of the domain's event lock)?

> --- a/xen/include/asm-x86/hvm/irq.h
> +++ b/xen/include/asm-x86/hvm/irq.h
> @@ -136,6 +136,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

As pointed out earlier this effectively is a change to domctl.h, which
(if it hadn't happened already in this release cycle) would require
bumping of the interface version. Please add a note to the commit
message clarifying this, as this complicates possible intentions of
backporting this change.

Jan


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

Re: [PATCH XEN] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Roger Pau Monne]

On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote:
> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
> > @@ -438,6 +439,22 @@ int pt_irq_create_bind(
> >              pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
> >                             info, pirq_dpci->gmsi.gvec);
> >  
> > +        if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED )
> > +        {
> > +            struct irq_desc *desc = irq_to_desc(info->arch.irq);
> > +            unsigned long flags;
> > +
> > +            if ( !desc )
> > +            {
> > +                pt_irq_destroy_bind(d, pt_irq_bind);
> > +                return -EINVAL;
> > +            }
> > +
> > +            spin_lock_irqsave(&desc->lock, flags);
> > +            guest_mask_msi_irq(desc, false);
> > +            spin_unlock_irqrestore(&desc->lock, flags);
> > +        }
> > +
> >          break;
> >      }
> 
> I think you would better use pirq_spin_lock_irq_desc() here. And
> wouldn't the addition better be moved up a little (perhaps right
> after the dropping of the domain's event lock)?

Shouldn't the unmask happen after the posted interrupt is setup? Or it
doesn't really matter?

I though it was safer to unmask once the bind process was finished.

> > --- a/xen/include/asm-x86/hvm/irq.h
> > +++ b/xen/include/asm-x86/hvm/irq.h
> > @@ -136,6 +136,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
> 
> As pointed out earlier this effectively is a change to domctl.h, which
> (if it hadn't happened already in this release cycle) would require
> bumping of the interface version. Please add a note to the commit
> message clarifying this, as this complicates possible intentions of
> backporting this change.

Ack, thanks.

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

Re: [PATCH QEMU] xen/pt: allow QEMU to request MSI unmasking at bind time

[Jan Beulich]

>>> On 24.08.17 at 12:06, <roger.pau@citrix.com> wrote:
> On Thu, Aug 24, 2017 at 03:54:21AM -0600, Jan Beulich wrote:
>> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
>> > @@ -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);
>> >  }
>> 
>> I don't think this is correct when the device has mask bits.
> 
> Right, I thought I modified this. I've already changed
> pt_irq_create_bind so that the original behavior is keep if the unmask
> bit is not set. In this case this should be 'true' in order to keep
> the previous behavior, which was correct for MSI.

Wouldn't you want to pass the state of the mask bit here,
rather than uniformly hard coding true or false?

> It also makes me wonder whether it would be better to change the name
> of the parameter to "unmask", so that false -> no change to mask, true
> -> unconditionally unmask.

I don't care much about this aspect, but perhaps the qemu
maintainers do.

Jan


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

Re: [PATCH XEN] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Jan Beulich]

>>> On 24.08.17 at 12:12, <roger.pau@citrix.com> wrote:
> On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote:
>> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
>> > @@ -438,6 +439,22 @@ int pt_irq_create_bind(
>> >              pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
>> >                             info, pirq_dpci->gmsi.gvec);
>> >  
>> > +        if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED )
>> > +        {
>> > +            struct irq_desc *desc = irq_to_desc(info->arch.irq);
>> > +            unsigned long flags;
>> > +
>> > +            if ( !desc )
>> > +            {
>> > +                pt_irq_destroy_bind(d, pt_irq_bind);
>> > +                return -EINVAL;
>> > +            }
>> > +
>> > +            spin_lock_irqsave(&desc->lock, flags);
>> > +            guest_mask_msi_irq(desc, false);
>> > +            spin_unlock_irqrestore(&desc->lock, flags);
>> > +        }
>> > +
>> >          break;
>> >      }
>> 
>> I think you would better use pirq_spin_lock_irq_desc() here. And
>> wouldn't the addition better be moved up a little (perhaps right
>> after the dropping of the domain's event lock)?
> 
> Shouldn't the unmask happen after the posted interrupt is setup? Or it
> doesn't really matter?
> 
> I though it was safer to unmask once the bind process was finished.

Yeah, I'm not entirely certain either, hence I've put it as a question.
Kevin, Chao?

Jan


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

Re: [PATCH XEN] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Chao Gao]

On Thu, Aug 24, 2017 at 04:17:32AM -0600, Jan Beulich wrote:
>>>> On 24.08.17 at 12:12, <roger.pau@citrix.com> wrote:
>> On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote:
>>> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
>>> > @@ -438,6 +439,22 @@ int pt_irq_create_bind(
>>> >              pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL,
>>> >                             info, pirq_dpci->gmsi.gvec);
>>> >  
>>> > +        if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED )
>>> > +        {
>>> > +            struct irq_desc *desc = irq_to_desc(info->arch.irq);
>>> > +            unsigned long flags;
>>> > +
>>> > +            if ( !desc )
>>> > +            {
>>> > +                pt_irq_destroy_bind(d, pt_irq_bind);
>>> > +                return -EINVAL;
>>> > +            }
>>> > +
>>> > +            spin_lock_irqsave(&desc->lock, flags);
>>> > +            guest_mask_msi_irq(desc, false);
>>> > +            spin_unlock_irqrestore(&desc->lock, flags);
>>> > +        }
>>> > +
>>> >          break;
>>> >      }
>>> 
>>> I think you would better use pirq_spin_lock_irq_desc() here. And
>>> wouldn't the addition better be moved up a little (perhaps right
>>> after the dropping of the domain's event lock)?
>> 
>> Shouldn't the unmask happen after the posted interrupt is setup? Or it
>> doesn't really matter?
>> 
>> I though it was safer to unmask once the bind process was finished.
>
>Yeah, I'm not entirely certain either, hence I've put it as a question.
>Kevin, Chao?
>

Hi, Jan and Roger.

pi_update_irte() right above the piece of code is to set IRTE properly
according to the request. Unmasking the msi without updating IRTE, I
think may leads to inject an interrupt whose vector or destination is
out of date.

Thanks
Chao

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

Re: [PATCH QEMU] xen/pt: allow QEMU to request MSI unmasking at bind time

[Roger Pau Monne]

On Thu, Aug 24, 2017 at 04:13:58AM -0600, Jan Beulich wrote:
> >>> On 24.08.17 at 12:06, <roger.pau@citrix.com> wrote:
> > On Thu, Aug 24, 2017 at 03:54:21AM -0600, Jan Beulich wrote:
> >> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote:
> >> > @@ -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);
> >> >  }
> >> 
> >> I don't think this is correct when the device has mask bits.
> > 
> > Right, I thought I modified this. I've already changed
> > pt_irq_create_bind so that the original behavior is keep if the unmask
> > bit is not set. In this case this should be 'true' in order to keep
> > the previous behavior, which was correct for MSI.
> 
> Wouldn't you want to pass the state of the mask bit here,
> rather than uniformly hard coding true or false?

Yes, I think so. I've overlooked the MSI code because I thought we
allowed QEMU to directly write to the mask register, but that's not
true, it's trapped by Xen.

Thanks, Roger.

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