hw/xen/xen_pt_msi.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
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)
>>> 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.
© 2016 - 2024 Red Hat, Inc.