Re: [Xen-devel] [PATCH] fix MSI injection on Xen

[Stefano Stabellini]

On Fri, 18 Dec 2015, Stefano Stabellini wrote:
> On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2015 at 05:16:18PM +, Stefano Stabellini wrote:
> > > On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> > > > On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> > > > > On Xen MSIs can be remapped into pirqs, which are a type of event
> > > > > channels. It's mostly for the benefit of PCI passthrough devices, to
> > > > > avoid the overhead of interacting with the emulated lapic.
> > > > > 
> > > > > However remapping interrupts and MSIs is also supported for emulated
> > > > > devices, such as the e1000 and virtio-net.
> > > > > 
> > > > > When an interrupt or an MSI is remapped into a pirq, masking and
> > > > > unmasking is done by masking and unmasking the event channel. The
> > > > > masking bit on the PCI config space or MSI-X table should be ignored,
> > > > > but it isn't at the moment.
> > > > > 
> > > > > As a consequence emulated devices which use MSI or MSI-X, such as
> > > > > virtio-net, don't work properly (the guest doesn't receive any
> > > > > notifications). The mechanism was working properly when xen_apic was
> > > > > introduced, but I haven't narrowed down which commit in particular is
> > > > > causing the regression.
> > > > > 
> > > > > Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> > > > > been remapped into pirqs.
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini 
> > > > > 
> > > > 
> > > > Is this a regression?
> > > > If not I'd rather defer this until 2.6 when I can review properly.
> > > 
> > > Unfortunately it is a regression, but given that probably it has been
> > > broken for multiple releases, I think you can wait until 2.6.
> > > 
> > 
> > Let's do that then. Pls ping me after 2.6.
> 
> Ping

Re-Ping


> > > > > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > > > > index f9c0484..3998725 100644
> > > > > --- a/hw/pci/msi.c
> > > > > +++ b/hw/pci/msi.c
> > > > > @@ -19,6 +19,7 @@
> > > > >   */
> > > > >  
> > > > >  #include "hw/pci/msi.h"
> > > > > +#include "hw/xen/xen.h"
> > > > >  #include "qemu/range.h"
> > > > >  
> > > > >  /* PCI_MSI_ADDRESS_LO */
> > > > > @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
> > > > >  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
> > > > >  {
> > > > >  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> > > > > -uint32_t mask;
> > > > > +uint32_t mask, data;
> > > > > +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> > > > >  assert(vector < PCI_MSI_VECTORS_MAX);
> > > > >  
> > > > >  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
> > > > >  return false;
> > > > >  }
> > > > >  
> > > > > +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> > > > > +if (xen_is_pirq_msi(data)) {
> > > > > +return false;
> > > > > +}
> > > > > +
> > > > >  mask = pci_get_long(dev->config +
> > > > >  msi_mask_off(dev, flags & 
> > > > > PCI_MSI_FLAGS_64BIT));
> > > > >  return mask & (1U << vector);
> > > > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > > > > index 7716bf3..96281c2 100644
> > > > > --- a/hw/pci/msix.c
> > > > > +++ b/hw/pci/msix.c
> > > > > @@ -18,6 +18,7 @@
> > > > >  #include "hw/pci/msi.h"
> > > > >  #include "hw/pci/msix.h"
> > > > >  #include "hw/pci/pci.h"
> > > > > +#include "hw/xen/xen.h"
> > > > >  #include "qemu/range.h"
> > > > >  
> > > > >  #define MSIX_CAP_LENGTH 12
> > > > > @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int 
> > > > > vector)
> > > > >  
> > > > >  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, 
> > > > > bool fmask)
> > > > >  {
> > > > > -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> > > > > PCI_MSIX_ENTRY_VECTOR_CTRL;
> > > > > -return fmask || dev->msix_table[offset] & 
> > > > > PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > > > +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> > > > > +uint32_t *data = (uint32_t *)>msix_table[offset + 
> > > > > PCI_MSIX_ENTRY_DATA];
> > > > > +/* MSIs on Xen can be remapped into pirqs. In those cases, 
> > > > > masking
> > > > > + * and unmasking go through the PV evtchn path. */
> > > > > +if (xen_is_pirq_msi(*data)) {
> > > > > +return false;
> > > > > +}
> > > > > +return fmask || dev->msix_table[offset + 
> > > > > PCI_MSIX_ENTRY_VECTOR_CTRL] &
> > > > > +PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > > >  }
> > > > >  
> > > > >  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> > > > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> > > > > index 82de2bc..375707e 100644
> > > > > --- a/hw/xen/xen_pt_msi.c
> > > > > +++ b/hw/xen/xen_pt_msi.c
> > > > > @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState 
> > > > > *s,
> > > > >  
> > > > >  assert((!is_msix && msix_entry == 0) || is_msix);
> > > > >  
> > > > > 

Re: [Xen-devel] [PATCH] fix MSI injection on Xen

[Stefano Stabellini]

On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2015 at 05:16:18PM +, Stefano Stabellini wrote:
> > On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> > > On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> > > > On Xen MSIs can be remapped into pirqs, which are a type of event
> > > > channels. It's mostly for the benefit of PCI passthrough devices, to
> > > > avoid the overhead of interacting with the emulated lapic.
> > > > 
> > > > However remapping interrupts and MSIs is also supported for emulated
> > > > devices, such as the e1000 and virtio-net.
> > > > 
> > > > When an interrupt or an MSI is remapped into a pirq, masking and
> > > > unmasking is done by masking and unmasking the event channel. The
> > > > masking bit on the PCI config space or MSI-X table should be ignored,
> > > > but it isn't at the moment.
> > > > 
> > > > As a consequence emulated devices which use MSI or MSI-X, such as
> > > > virtio-net, don't work properly (the guest doesn't receive any
> > > > notifications). The mechanism was working properly when xen_apic was
> > > > introduced, but I haven't narrowed down which commit in particular is
> > > > causing the regression.
> > > > 
> > > > Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> > > > been remapped into pirqs.
> > > > 
> > > > Signed-off-by: Stefano Stabellini 
> > > > 
> > > 
> > > Is this a regression?
> > > If not I'd rather defer this until 2.6 when I can review properly.
> > 
> > Unfortunately it is a regression, but given that probably it has been
> > broken for multiple releases, I think you can wait until 2.6.
> > 
> 
> Let's do that then. Pls ping me after 2.6.

Ping


> > > > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > > > index f9c0484..3998725 100644
> > > > --- a/hw/pci/msi.c
> > > > +++ b/hw/pci/msi.c
> > > > @@ -19,6 +19,7 @@
> > > >   */
> > > >  
> > > >  #include "hw/pci/msi.h"
> > > > +#include "hw/xen/xen.h"
> > > >  #include "qemu/range.h"
> > > >  
> > > >  /* PCI_MSI_ADDRESS_LO */
> > > > @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
> > > >  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
> > > >  {
> > > >  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> > > > -uint32_t mask;
> > > > +uint32_t mask, data;
> > > > +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> > > >  assert(vector < PCI_MSI_VECTORS_MAX);
> > > >  
> > > >  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
> > > >  return false;
> > > >  }
> > > >  
> > > > +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> > > > +if (xen_is_pirq_msi(data)) {
> > > > +return false;
> > > > +}
> > > > +
> > > >  mask = pci_get_long(dev->config +
> > > >  msi_mask_off(dev, flags & 
> > > > PCI_MSI_FLAGS_64BIT));
> > > >  return mask & (1U << vector);
> > > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > > > index 7716bf3..96281c2 100644
> > > > --- a/hw/pci/msix.c
> > > > +++ b/hw/pci/msix.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include "hw/pci/msi.h"
> > > >  #include "hw/pci/msix.h"
> > > >  #include "hw/pci/pci.h"
> > > > +#include "hw/xen/xen.h"
> > > >  #include "qemu/range.h"
> > > >  
> > > >  #define MSIX_CAP_LENGTH 12
> > > > @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int 
> > > > vector)
> > > >  
> > > >  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, 
> > > > bool fmask)
> > > >  {
> > > > -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> > > > PCI_MSIX_ENTRY_VECTOR_CTRL;
> > > > -return fmask || dev->msix_table[offset] & 
> > > > PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > > +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> > > > +uint32_t *data = (uint32_t *)>msix_table[offset + 
> > > > PCI_MSIX_ENTRY_DATA];
> > > > +/* MSIs on Xen can be remapped into pirqs. In those cases, masking
> > > > + * and unmasking go through the PV evtchn path. */
> > > > +if (xen_is_pirq_msi(*data)) {
> > > > +return false;
> > > > +}
> > > > +return fmask || dev->msix_table[offset + 
> > > > PCI_MSIX_ENTRY_VECTOR_CTRL] &
> > > > +PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > >  }
> > > >  
> > > >  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> > > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> > > > index 82de2bc..375707e 100644
> > > > --- a/hw/xen/xen_pt_msi.c
> > > > +++ b/hw/xen/xen_pt_msi.c
> > > > @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
> > > >  
> > > >  assert((!is_msix && msix_entry == 0) || is_msix);
> > > >  
> > > > -if (gvec == 0) {
> > > > -/* if gvec is 0, the guest is asking for a particular pirq that
> > > > - * is passed as dest_id */
> > > > +if (xen_is_pirq_msi(data)) {
> > > >  *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
> > > >  if (!*ppirq) {
> > > >  

Re: [Xen-devel] [PATCH] fix MSI injection on Xen

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> On Xen MSIs can be remapped into pirqs, which are a type of event
> channels. It's mostly for the benefit of PCI passthrough devices, to
> avoid the overhead of interacting with the emulated lapic.
> 
> However remapping interrupts and MSIs is also supported for emulated
> devices, such as the e1000 and virtio-net.
> 
> When an interrupt or an MSI is remapped into a pirq, masking and
> unmasking is done by masking and unmasking the event channel. The
> masking bit on the PCI config space or MSI-X table should be ignored,
> but it isn't at the moment.
> 
> As a consequence emulated devices which use MSI or MSI-X, such as
> virtio-net, don't work properly (the guest doesn't receive any
> notifications). The mechanism was working properly when xen_apic was
> introduced, but I haven't narrowed down which commit in particular is
> causing the regression.
> 
> Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> been remapped into pirqs.
> 
> Signed-off-by: Stefano Stabellini 
> 

Is this a regression?
If not I'd rather defer this until 2.6 when I can review properly.

> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index f9c0484..3998725 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "hw/pci/msi.h"
> +#include "hw/xen/xen.h"
>  #include "qemu/range.h"
>  
>  /* PCI_MSI_ADDRESS_LO */
> @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
>  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
>  {
>  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> -uint32_t mask;
> +uint32_t mask, data;
> +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>  assert(vector < PCI_MSI_VECTORS_MAX);
>  
>  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
>  return false;
>  }
>  
> +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> +if (xen_is_pirq_msi(data)) {
> +return false;
> +}
> +
>  mask = pci_get_long(dev->config +
>  msi_mask_off(dev, flags & PCI_MSI_FLAGS_64BIT));
>  return mask & (1U << vector);
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 7716bf3..96281c2 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -18,6 +18,7 @@
>  #include "hw/pci/msi.h"
>  #include "hw/pci/msix.h"
>  #include "hw/pci/pci.h"
> +#include "hw/xen/xen.h"
>  #include "qemu/range.h"
>  
>  #define MSIX_CAP_LENGTH 12
> @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>  
>  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool 
> fmask)
>  {
> -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> PCI_MSIX_ENTRY_VECTOR_CTRL;
> -return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> +uint32_t *data = (uint32_t *)>msix_table[offset + 
> PCI_MSIX_ENTRY_DATA];
> +/* MSIs on Xen can be remapped into pirqs. In those cases, masking
> + * and unmasking go through the PV evtchn path. */
> +if (xen_is_pirq_msi(*data)) {
> +return false;
> +}
> +return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
> +PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>  
>  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 82de2bc..375707e 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
>  
>  assert((!is_msix && msix_entry == 0) || is_msix);
>  
> -if (gvec == 0) {
> -/* if gvec is 0, the guest is asking for a particular pirq that
> - * is passed as dest_id */
> +if (xen_is_pirq_msi(data)) {
>  *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
>  if (!*ppirq) {
>  /* this probably identifies an misconfiguration of the guest,
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 4356af4..b15b2f5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -32,6 +32,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
>  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
>  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int 
> len);
>  void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
> +int xen_is_pirq_msi(uint32_t msi_data);
>  
>  qemu_irq *xen_interrupt_controller_init(void);
>  
> diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
> index 46867d8..ce22a82 100644
> --- a/xen-hvm-stub.c
> +++ b/xen-hvm-stub.c
> @@ -30,6 +30,11 @@ void xen_hvm_inject_msi(uint64_t addr, uint32_t data)
>  {
>  }
>  
> +int xen_is_pirq_msi(uint32_t msi_data)
> +{
> +return 0;
> +}
> +
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>  {
>  }
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 3371c4e..21dd301 100644
> --- 

Re: [Xen-devel] [PATCH] fix MSI injection on Xen

[Stefano Stabellini]

On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> > On Xen MSIs can be remapped into pirqs, which are a type of event
> > channels. It's mostly for the benefit of PCI passthrough devices, to
> > avoid the overhead of interacting with the emulated lapic.
> > 
> > However remapping interrupts and MSIs is also supported for emulated
> > devices, such as the e1000 and virtio-net.
> > 
> > When an interrupt or an MSI is remapped into a pirq, masking and
> > unmasking is done by masking and unmasking the event channel. The
> > masking bit on the PCI config space or MSI-X table should be ignored,
> > but it isn't at the moment.
> > 
> > As a consequence emulated devices which use MSI or MSI-X, such as
> > virtio-net, don't work properly (the guest doesn't receive any
> > notifications). The mechanism was working properly when xen_apic was
> > introduced, but I haven't narrowed down which commit in particular is
> > causing the regression.
> > 
> > Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> > been remapped into pirqs.
> > 
> > Signed-off-by: Stefano Stabellini 
> > 
> 
> Is this a regression?
> If not I'd rather defer this until 2.6 when I can review properly.

Unfortunately it is a regression, but given that probably it has been
broken for multiple releases, I think you can wait until 2.6.


> > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > index f9c0484..3998725 100644
> > --- a/hw/pci/msi.c
> > +++ b/hw/pci/msi.c
> > @@ -19,6 +19,7 @@
> >   */
> >  
> >  #include "hw/pci/msi.h"
> > +#include "hw/xen/xen.h"
> >  #include "qemu/range.h"
> >  
> >  /* PCI_MSI_ADDRESS_LO */
> > @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
> >  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
> >  {
> >  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> > -uint32_t mask;
> > +uint32_t mask, data;
> > +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> >  assert(vector < PCI_MSI_VECTORS_MAX);
> >  
> >  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
> >  return false;
> >  }
> >  
> > +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> > +if (xen_is_pirq_msi(data)) {
> > +return false;
> > +}
> > +
> >  mask = pci_get_long(dev->config +
> >  msi_mask_off(dev, flags & PCI_MSI_FLAGS_64BIT));
> >  return mask & (1U << vector);
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index 7716bf3..96281c2 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -18,6 +18,7 @@
> >  #include "hw/pci/msi.h"
> >  #include "hw/pci/msix.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/xen/xen.h"
> >  #include "qemu/range.h"
> >  
> >  #define MSIX_CAP_LENGTH 12
> > @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >  
> >  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool 
> > fmask)
> >  {
> > -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> > PCI_MSIX_ENTRY_VECTOR_CTRL;
> > -return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> > +uint32_t *data = (uint32_t *)>msix_table[offset + 
> > PCI_MSIX_ENTRY_DATA];
> > +/* MSIs on Xen can be remapped into pirqs. In those cases, masking
> > + * and unmasking go through the PV evtchn path. */
> > +if (xen_is_pirq_msi(*data)) {
> > +return false;
> > +}
> > +return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] &
> > +PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >  }
> >  
> >  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> > index 82de2bc..375707e 100644
> > --- a/hw/xen/xen_pt_msi.c
> > +++ b/hw/xen/xen_pt_msi.c
> > @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
> >  
> >  assert((!is_msix && msix_entry == 0) || is_msix);
> >  
> > -if (gvec == 0) {
> > -/* if gvec is 0, the guest is asking for a particular pirq that
> > - * is passed as dest_id */
> > +if (xen_is_pirq_msi(data)) {
> >  *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
> >  if (!*ppirq) {
> >  /* this probably identifies an misconfiguration of the guest,
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index 4356af4..b15b2f5 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -32,6 +32,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int 
> > irq_num);
> >  void xen_piix3_set_irq(void *opaque, int irq_num, int level);
> >  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int 
> > len);
> >  void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
> > +int xen_is_pirq_msi(uint32_t msi_data);
> >  
> >  qemu_irq *xen_interrupt_controller_init(void);
> >  
> > diff --git 

Re: [Xen-devel] [PATCH] fix MSI injection on Xen

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 05:16:18PM +, Stefano Stabellini wrote:
> On Wed, 2 Dec 2015, Michael S. Tsirkin wrote:
> > On Wed, Dec 02, 2015 at 04:56:21PM +, Stefano Stabellini wrote:
> > > On Xen MSIs can be remapped into pirqs, which are a type of event
> > > channels. It's mostly for the benefit of PCI passthrough devices, to
> > > avoid the overhead of interacting with the emulated lapic.
> > > 
> > > However remapping interrupts and MSIs is also supported for emulated
> > > devices, such as the e1000 and virtio-net.
> > > 
> > > When an interrupt or an MSI is remapped into a pirq, masking and
> > > unmasking is done by masking and unmasking the event channel. The
> > > masking bit on the PCI config space or MSI-X table should be ignored,
> > > but it isn't at the moment.
> > > 
> > > As a consequence emulated devices which use MSI or MSI-X, such as
> > > virtio-net, don't work properly (the guest doesn't receive any
> > > notifications). The mechanism was working properly when xen_apic was
> > > introduced, but I haven't narrowed down which commit in particular is
> > > causing the regression.
> > > 
> > > Fix the issue by ignoring the masking bit for MSI and MSI-X which have
> > > been remapped into pirqs.
> > > 
> > > Signed-off-by: Stefano Stabellini 
> > > 
> > 
> > Is this a regression?
> > If not I'd rather defer this until 2.6 when I can review properly.
> 
> Unfortunately it is a regression, but given that probably it has been
> broken for multiple releases, I think you can wait until 2.6.
> 

Let's do that then. Pls ping me after 2.6.

> > > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> > > index f9c0484..3998725 100644
> > > --- a/hw/pci/msi.c
> > > +++ b/hw/pci/msi.c
> > > @@ -19,6 +19,7 @@
> > >   */
> > >  
> > >  #include "hw/pci/msi.h"
> > > +#include "hw/xen/xen.h"
> > >  #include "qemu/range.h"
> > >  
> > >  /* PCI_MSI_ADDRESS_LO */
> > > @@ -253,13 +254,19 @@ void msi_reset(PCIDevice *dev)
> > >  static bool msi_is_masked(const PCIDevice *dev, unsigned int vector)
> > >  {
> > >  uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> > > -uint32_t mask;
> > > +uint32_t mask, data;
> > > +bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> > >  assert(vector < PCI_MSI_VECTORS_MAX);
> > >  
> > >  if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
> > >  return false;
> > >  }
> > >  
> > > +data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> > > +if (xen_is_pirq_msi(data)) {
> > > +return false;
> > > +}
> > > +
> > >  mask = pci_get_long(dev->config +
> > >  msi_mask_off(dev, flags & PCI_MSI_FLAGS_64BIT));
> > >  return mask & (1U << vector);
> > > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > > index 7716bf3..96281c2 100644
> > > --- a/hw/pci/msix.c
> > > +++ b/hw/pci/msix.c
> > > @@ -18,6 +18,7 @@
> > >  #include "hw/pci/msi.h"
> > >  #include "hw/pci/msix.h"
> > >  #include "hw/pci/pci.h"
> > > +#include "hw/xen/xen.h"
> > >  #include "qemu/range.h"
> > >  
> > >  #define MSIX_CAP_LENGTH 12
> > > @@ -77,8 +78,15 @@ static void msix_clr_pending(PCIDevice *dev, int 
> > > vector)
> > >  
> > >  static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool 
> > > fmask)
> > >  {
> > > -unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + 
> > > PCI_MSIX_ENTRY_VECTOR_CTRL;
> > > -return fmask || dev->msix_table[offset] & 
> > > PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > +unsigned offset = vector * PCI_MSIX_ENTRY_SIZE;
> > > +uint32_t *data = (uint32_t *)>msix_table[offset + 
> > > PCI_MSIX_ENTRY_DATA];
> > > +/* MSIs on Xen can be remapped into pirqs. In those cases, masking
> > > + * and unmasking go through the PV evtchn path. */
> > > +if (xen_is_pirq_msi(*data)) {
> > > +return false;
> > > +}
> > > +return fmask || dev->msix_table[offset + PCI_MSIX_ENTRY_VECTOR_CTRL] 
> > > &
> > > +PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > >  }
> > >  
> > >  bool msix_is_masked(PCIDevice *dev, unsigned int vector)
> > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> > > index 82de2bc..375707e 100644
> > > --- a/hw/xen/xen_pt_msi.c
> > > +++ b/hw/xen/xen_pt_msi.c
> > > @@ -113,9 +113,7 @@ static int msi_msix_setup(XenPCIPassthroughState *s,
> > >  
> > >  assert((!is_msix && msix_entry == 0) || is_msix);
> > >  
> > > -if (gvec == 0) {
> > > -/* if gvec is 0, the guest is asking for a particular pirq that
> > > - * is passed as dest_id */
> > > +if (xen_is_pirq_msi(data)) {
> > >  *ppirq = msi_ext_dest_id(addr >> 32) | msi_dest_id(addr);
> > >  if (!*ppirq) {
> > >  /* this probably identifies an misconfiguration of the guest,
> > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > > index 4356af4..b15b2f5 100644
> > > --- a/include/hw/xen/xen.h
> > > +++ b/include/hw/xen/xen.h
> > > @@ -32,6 +32,7 @@ int xen_pci_slot_get_pirq(PCIDevice