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é <address@hidden>
> Reviewed-by: Jan Beulich <address@hidden>
> Reported-by: Andreas Kinzler <address@hidden>

Acked-by: Stefano Stabellini <address@hidden>

I'll queue it up.


> ---
> Cc: Stefano Stabellini <address@hidden>
> Cc: Anthony Perard <address@hidden>
> Cc: Jan Beulich <address@hidden>
> Cc: address@hidden
> ---
> 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)
>