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

[PATCH v4] 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.

This is fixed 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 v4] 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.

Note that this commit introduces a change to the domctl, which
requires a bump of the interface version. This is done done here
because the interface version has already been bumped in this release
cycle.

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>
---
Changes since v2:
 - Use _irqrestore.

Changes since v1:
 - Use pirq_spin_lock_irq_desc.
---
 xen/drivers/passthrough/io.c  | 22 +++++++++++++++++++---
 xen/include/asm-x86/hvm/irq.h |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 19a21bf85a..1d260bd7ba 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,21 @@ 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 )
+        {
+            unsigned long flags;
+            struct irq_desc *desc = pirq_spin_lock_irq_desc(info, &flags);
+
+            if ( !desc )
+            {
+                pt_irq_destroy_bind(d, pt_irq_bind);
+                return -EINVAL;
+            }
+
+            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

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

[Roger Pau Monne]

When a MSI 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 MSI
interrupt should be unmasked after being bound.

This also requires to track the mask register for MSI interrupts, so
QEMU can also notify to Xen whether the MSI interrupt should be bound
masked or unmasked

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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
---
Changes since v2:
 - Fix coding style.

Changes since v1:
 - Track MSI mask bits.
 - Notify Xen of whether MSI interrupts should be unmasked after
   binding, instead of hardcoding it.
---
 hw/xen/xen_pt.h             |  1 +
 hw/xen/xen_pt_config_init.c | 20 ++++++++++++++++++--
 hw/xen/xen_pt_msi.c         | 13 ++++++++++---
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 191d9caea1..aa39a9aa5f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -180,6 +180,7 @@ typedef struct XenPTMSI {
     uint32_t addr_hi;  /* guest message upper address */
     uint16_t data;     /* guest message data */
     uint32_t ctrl_offset; /* saved control offset */
+    uint32_t mask;     /* guest mask bits */
     int pirq;          /* guest pirq corresponding */
     bool initialized;  /* when guest MSI is initialized */
     bool mapped;       /* when pirq is mapped */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 1f04ec5eec..a3ce33e78b 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1315,6 +1315,22 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     return 0;
 }
 
+static int xen_pt_mask_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
+                                 uint32_t *val, uint32_t dev_value,
+                                 uint32_t valid_mask)
+{
+    int rc;
+
+    rc = xen_pt_long_reg_write(s, cfg_entry, val, dev_value, valid_mask);
+    if (rc) {
+        return rc;
+    }
+
+    s->msi->mask = *val;
+
+    return 0;
+}
+
 /* MSI Capability Structure reg static information table */
 static XenPTRegInfo xen_pt_emu_reg_msi[] = {
     /* Next Pointer reg */
@@ -1393,7 +1409,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .emu_mask   = 0xFFFFFFFF,
         .init       = xen_pt_mask_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
-        .u.dw.write = xen_pt_long_reg_write,
+        .u.dw.write = xen_pt_mask_reg_write,
     },
     /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
     {
@@ -1404,7 +1420,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .emu_mask   = 0xFFFFFFFF,
         .init       = xen_pt_mask_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
-        .u.dw.write = xen_pt_long_reg_write,
+        .u.dw.write = xen_pt_mask_reg_write,
     },
     /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
     {
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index ff9a79f5d2..6d1e3bdeb4 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);
 
@@ -273,8 +277,10 @@ int xen_pt_msi_setup(XenPCIPassthroughState *s)
 int xen_pt_msi_update(XenPCIPassthroughState *s)
 {
     XenPTMSI *msi = s->msi;
+
+    /* Current MSI emulation in QEMU only supports 1 vector */
     return msi_msix_update(s, msi_addr64(msi), msi->data, msi->pirq,
-                           false, 0, &msi->pirq);
+                           false, 0, &msi->pirq, msi->mask & 1);
 }
 
 void xen_pt_msi_disable(XenPCIPassthroughState *s)
@@ -355,7 +361,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)

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

[Roger Pau Monne]

When a MSI 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 MSI
interrupt should be unmasked after being bound.

This also requires to track the mask register for MSI interrupts, so
QEMU can also notify to Xen whether the MSI interrupt should be bound
masked or unmasked

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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
---
Changes since v2:
 - Fix coding style.

Changes since v1:
 - Track MSI mask bits.
 - Notify Xen of whether MSI interrupts should be unmasked after
   binding, instead of hardcoding it.
---
 hw/xen/xen_pt.h             |  1 +
 hw/xen/xen_pt_config_init.c | 20 ++++++++++++++++++--
 hw/xen/xen_pt_msi.c         | 13 ++++++++++---
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 191d9caea1..aa39a9aa5f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -180,6 +180,7 @@ typedef struct XenPTMSI {
     uint32_t addr_hi;  /* guest message upper address */
     uint16_t data;     /* guest message data */
     uint32_t ctrl_offset; /* saved control offset */
+    uint32_t mask;     /* guest mask bits */
     int pirq;          /* guest pirq corresponding */
     bool initialized;  /* when guest MSI is initialized */
     bool mapped;       /* when pirq is mapped */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 1f04ec5eec..a3ce33e78b 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1315,6 +1315,22 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     return 0;
 }
 
+static int xen_pt_mask_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
+                                 uint32_t *val, uint32_t dev_value,
+                                 uint32_t valid_mask)
+{
+    int rc;
+
+    rc = xen_pt_long_reg_write(s, cfg_entry, val, dev_value, valid_mask);
+    if (rc) {
+        return rc;
+    }
+
+    s->msi->mask = *val;
+
+    return 0;
+}
+
 /* MSI Capability Structure reg static information table */
 static XenPTRegInfo xen_pt_emu_reg_msi[] = {
     /* Next Pointer reg */
@@ -1393,7 +1409,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .emu_mask   = 0xFFFFFFFF,
         .init       = xen_pt_mask_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
-        .u.dw.write = xen_pt_long_reg_write,
+        .u.dw.write = xen_pt_mask_reg_write,
     },
     /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
     {
@@ -1404,7 +1420,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .emu_mask   = 0xFFFFFFFF,
         .init       = xen_pt_mask_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
-        .u.dw.write = xen_pt_long_reg_write,
+        .u.dw.write = xen_pt_mask_reg_write,
     },
     /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
     {
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index ff9a79f5d2..6d1e3bdeb4 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);
 
@@ -273,8 +277,10 @@ int xen_pt_msi_setup(XenPCIPassthroughState *s)
 int xen_pt_msi_update(XenPCIPassthroughState *s)
 {
     XenPTMSI *msi = s->msi;
+
+    /* Current MSI emulation in QEMU only supports 1 vector */
     return msi_msix_update(s, msi_addr64(msi), msi->data, msi->pirq,
-                           false, 0, &msi->pirq);
+                           false, 0, &msi->pirq, msi->mask & 1);
 }
 
 void xen_pt_msi_disable(XenPCIPassthroughState *s)
@@ -355,7 +361,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 XEN v4] x86/pt: add a MSI unmask flag to XEN_DOMCTL_bind_pt_irq

[Jan Beulich]

>>> On 24.08.17 at 17:07, <roger.pau@citrix.com> wrote:
> 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.
> 
> Note that this commit introduces a change to the domctl, which
> requires a bump of the interface version. This is done done here

"... is not done here ..." I suppose (I'll try to remember changing
that while committing, unless you tell me I've got it wrong).

> because the interface version has already been bumped in this release
> cycle.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported by: Andreas Kinzler <hfp@posteo.de>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

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

[Stefano Stabellini]

On Thu, 24 Aug 2017, Roger Pau Monne wrote:
> When a MSI 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 MSI
> interrupt should be unmasked after being bound.
> 
> This also requires to track the mask register for MSI interrupts, so
> QEMU can also notify to Xen whether the MSI interrupt should be bound
> masked or unmasked
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reported-by: Andreas Kinzler <hfp@posteo.de>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

I'll queue it up.


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: qemu-devel@nongnu.org
> ---
> Changes since v2:
>  - Fix coding style.
> 
> Changes since v1:
>  - Track MSI mask bits.
>  - Notify Xen of whether MSI interrupts should be unmasked after
>    binding, instead of hardcoding it.
> ---
>  hw/xen/xen_pt.h             |  1 +
>  hw/xen/xen_pt_config_init.c | 20 ++++++++++++++++++--
>  hw/xen/xen_pt_msi.c         | 13 ++++++++++---
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 191d9caea1..aa39a9aa5f 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -180,6 +180,7 @@ typedef struct XenPTMSI {
>      uint32_t addr_hi;  /* guest message upper address */
>      uint16_t data;     /* guest message data */
>      uint32_t ctrl_offset; /* saved control offset */
> +    uint32_t mask;     /* guest mask bits */
>      int pirq;          /* guest pirq corresponding */
>      bool initialized;  /* when guest MSI is initialized */
>      bool mapped;       /* when pirq is mapped */
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 1f04ec5eec..a3ce33e78b 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1315,6 +1315,22 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
>      return 0;
>  }
>  
> +static int xen_pt_mask_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> +                                 uint32_t *val, uint32_t dev_value,
> +                                 uint32_t valid_mask)
> +{
> +    int rc;
> +
> +    rc = xen_pt_long_reg_write(s, cfg_entry, val, dev_value, valid_mask);
> +    if (rc) {
> +        return rc;
> +    }
> +
> +    s->msi->mask = *val;
> +
> +    return 0;
> +}
> +
>  /* MSI Capability Structure reg static information table */
>  static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>      /* Next Pointer reg */
> @@ -1393,7 +1409,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>          .emu_mask   = 0xFFFFFFFF,
>          .init       = xen_pt_mask_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
> -        .u.dw.write = xen_pt_long_reg_write,
> +        .u.dw.write = xen_pt_mask_reg_write,
>      },
>      /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
>      {
> @@ -1404,7 +1420,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>          .emu_mask   = 0xFFFFFFFF,
>          .init       = xen_pt_mask_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
> -        .u.dw.write = xen_pt_long_reg_write,
> +        .u.dw.write = xen_pt_mask_reg_write,
>      },
>      /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
>      {
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index ff9a79f5d2..6d1e3bdeb4 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);
>  
> @@ -273,8 +277,10 @@ int xen_pt_msi_setup(XenPCIPassthroughState *s)
>  int xen_pt_msi_update(XenPCIPassthroughState *s)
>  {
>      XenPTMSI *msi = s->msi;
> +
> +    /* Current MSI emulation in QEMU only supports 1 vector */
>      return msi_msix_update(s, msi_addr64(msi), msi->data, msi->pirq,
> -                           false, 0, &msi->pirq);
> +                           false, 0, &msi->pirq, msi->mask & 1);
>  }
>  
>  void xen_pt_msi_disable(XenPCIPassthroughState *s)
> @@ -355,7 +361,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)
> 

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

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5981 bytes --]

On Thu, 24 Aug 2017, Roger Pau Monne wrote:
> When a MSI 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 MSI
> interrupt should be unmasked after being bound.
> 
> This also requires to track the mask register for MSI interrupts, so
> QEMU can also notify to Xen whether the MSI interrupt should be bound
> masked or unmasked
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reported-by: Andreas Kinzler <hfp@posteo.de>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

I'll queue it up.


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: qemu-devel@nongnu.org
> ---
> Changes since v2:
>  - Fix coding style.
> 
> Changes since v1:
>  - Track MSI mask bits.
>  - Notify Xen of whether MSI interrupts should be unmasked after
>    binding, instead of hardcoding it.
> ---
>  hw/xen/xen_pt.h             |  1 +
>  hw/xen/xen_pt_config_init.c | 20 ++++++++++++++++++--
>  hw/xen/xen_pt_msi.c         | 13 ++++++++++---
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 191d9caea1..aa39a9aa5f 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -180,6 +180,7 @@ typedef struct XenPTMSI {
>      uint32_t addr_hi;  /* guest message upper address */
>      uint16_t data;     /* guest message data */
>      uint32_t ctrl_offset; /* saved control offset */
> +    uint32_t mask;     /* guest mask bits */
>      int pirq;          /* guest pirq corresponding */
>      bool initialized;  /* when guest MSI is initialized */
>      bool mapped;       /* when pirq is mapped */
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 1f04ec5eec..a3ce33e78b 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1315,6 +1315,22 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
>      return 0;
>  }
>  
> +static int xen_pt_mask_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> +                                 uint32_t *val, uint32_t dev_value,
> +                                 uint32_t valid_mask)
> +{
> +    int rc;
> +
> +    rc = xen_pt_long_reg_write(s, cfg_entry, val, dev_value, valid_mask);
> +    if (rc) {
> +        return rc;
> +    }
> +
> +    s->msi->mask = *val;
> +
> +    return 0;
> +}
> +
>  /* MSI Capability Structure reg static information table */
>  static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>      /* Next Pointer reg */
> @@ -1393,7 +1409,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>          .emu_mask   = 0xFFFFFFFF,
>          .init       = xen_pt_mask_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
> -        .u.dw.write = xen_pt_long_reg_write,
> +        .u.dw.write = xen_pt_mask_reg_write,
>      },
>      /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
>      {
> @@ -1404,7 +1420,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
>          .emu_mask   = 0xFFFFFFFF,
>          .init       = xen_pt_mask_reg_init,
>          .u.dw.read  = xen_pt_long_reg_read,
> -        .u.dw.write = xen_pt_long_reg_write,
> +        .u.dw.write = xen_pt_mask_reg_write,
>      },
>      /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
>      {
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index ff9a79f5d2..6d1e3bdeb4 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);
>  
> @@ -273,8 +277,10 @@ int xen_pt_msi_setup(XenPCIPassthroughState *s)
>  int xen_pt_msi_update(XenPCIPassthroughState *s)
>  {
>      XenPTMSI *msi = s->msi;
> +
> +    /* Current MSI emulation in QEMU only supports 1 vector */
>      return msi_msix_update(s, msi_addr64(msi), msi->data, msi->pirq,
> -                           false, 0, &msi->pirq);
> +                           false, 0, &msi->pirq, msi->mask & 1);
>  }
>  
>  void xen_pt_msi_disable(XenPCIPassthroughState *s)
> @@ -355,7 +361,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)
> 

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 24.08.17 at 17:27, <JBeulich@suse.com> wrote:
>>>> On 24.08.17 at 17:07, <roger.pau@citrix.com> wrote:
>> 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.
>> 
>> Note that this commit introduces a change to the domctl, which
>> requires a bump of the interface version. This is done done here
> 
> "... is not done here ..." I suppose (I'll try to remember changing
> that while committing, unless you tell me I've got it wrong).

Since I haven't heard back yet: I'm intending to commit this only
once the above item was clarified.

Jan

>> because the interface version has already been bumped in this release
>> cycle.
>> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reported by: Andreas Kinzler <hfp@posteo.de>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> https://lists.xen.org/xen-devel 



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

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

[Roger Pau Monne]

On Tue, Aug 29, 2017 at 03:11:09AM -0600, Jan Beulich wrote:
> >>> On 24.08.17 at 17:27, <JBeulich@suse.com> wrote:
> >>>> On 24.08.17 at 17:07, <roger.pau@citrix.com> wrote:
> >> 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.
> >> 
> >> Note that this commit introduces a change to the domctl, which
> >> requires a bump of the interface version. This is done done here
> > 
> > "... is not done here ..." I suppose (I'll try to remember changing
> > that while committing, unless you tell me I've got it wrong).

Oh yes, sorry. Your fix is correct, please change it while committing.

Roger.

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