| Message ID | 20170824094753.77795-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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.
>>> 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
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.
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;
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(-)