Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Mon, Dec 5, 2011 at 2:08 AM, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 04:47:17PM -0700, Cam Macdonell wrote:
>> On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin wrote:
>> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> >> the ivshmem device.
>> >
>> > I think the following should fix it. Compiled-only -
>> > could you pls check? If yes let's apply to the stable branch.
>>
>> Thanks for the patch Michael.
>>
>> It addresses the need for msix_write_config() to be called, but the
>> addition of the msix_reset() is causing a reset of the vectors after
>> they've been initialized in pci_ivshmem_init(). So, interrupts still
>> aren't delivered with this patch applied as it is.
>>
>> In particular, a reset occurs after pci_ivshmem_init runs, so the
>> msix_entry_used array is reset to 0s, which causes the interrupt
>> delivery to fail.
>>
>> If I comment out the msix_reset(), then interrupts are delivered.
>> Would the reset be caused by a bug in the guest driver? or do I need
>> to reconfigure the msix after reset? I'm unclear as to the proper
>> behaviour after a reset.
>>
>> Thanks,
>> Cam
>>
>
> I didn't anticipate this, virtio only configures vectors when msi-x is
> enabled.
> For 1.0 it's safest to do the same and just re-enable after reset.
>
> So we'll end up with something like the following - does it help?
Yes, this fixes interrupt delivery.
>
> Side note: exit(1) is not the best way to handle
> errors in the init function. I think you should add error_report, then
> goto err on failures to init, cleanup what you have set up
> meanwhile and return an error code.
Thanks, I'll fix that.
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..c58f4d3 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t *
> buf, int flags)
> return;
> }
>
> +/* Select the MSI-X vectors used by device.
> + * ivshmem maps events to vectors statically, so
> + * we just enable all vectors on init and after reset. */
> +static void ivshmem_use_msix(IVShmemState * s)
> +{
> + int i;
> +
> + if (!msix_present(&s->dev)) {
> + return;
> + }
> +
> + for (i = 0; i < s->vectors; i++) {
> + msix_vector_use(&s->dev, i);
> + }
> +}
> +
> static void ivshmem_reset(DeviceState *d)
> {
> IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
> s->intrstatus = 0;
> + msix_reset(&s->dev);
> + ivshmem_use_msix(s);
> return;
> }
>
> @@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
> return value;
> }
>
> -static void ivshmem_setup_msi(IVShmemState * s) {
> -
> - int i;
> -
> - /* allocate the MSI-X vectors */
> -
> +static void ivshmem_setup_msi(IVShmemState * s)
> +{
> memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
> if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
> pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> @@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
> exit(1);
> }
>
> - /* 'activate' the vectors */
> - for (i = 0; i < s->vectors; i++) {
> - msix_vector_use(&s->dev, i);
> - }
> -
> /* allocate Qemu char devices for receiving interrupts */
> s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
> +
> + ivshmem_use_msix(s);
> }
>
> static void ivshmem_save(QEMUFile* f, void *opaque)
> @@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
> version_id)
> return 0;
> }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> + uint32_t val, int len)
> +{
> + pci_default_write_config(pci_dev, address, val, len);
> + msix_write_config(pci_dev, address, val, len);
> +}
> +
> static int pci_ivshmem_init(PCIDevice *dev)
> {
> IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
> }
>
> + s->dev.config_write = ivshmem_write_config;
> +
> return 0;
> }
>
>
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Sun, Dec 04, 2011 at 04:47:17PM -0700, Cam Macdonell wrote:
> On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin wrote:
> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> >> the ivshmem device.
> >
> > I think the following should fix it. Compiled-only -
> > could you pls check? If yes let's apply to the stable branch.
>
> Thanks for the patch Michael.
>
> It addresses the need for msix_write_config() to be called, but the
> addition of the msix_reset() is causing a reset of the vectors after
> they've been initialized in pci_ivshmem_init(). So, interrupts still
> aren't delivered with this patch applied as it is.
>
> In particular, a reset occurs after pci_ivshmem_init runs, so the
> msix_entry_used array is reset to 0s, which causes the interrupt
> delivery to fail.
>
> If I comment out the msix_reset(), then interrupts are delivered.
> Would the reset be caused by a bug in the guest driver? or do I need
> to reconfigure the msix after reset? I'm unclear as to the proper
> behaviour after a reset.
>
> Thanks,
> Cam
>
I didn't anticipate this, virtio only configures vectors when msi-x is enabled.
For 1.0 it's safest to do the same and just re-enable after reset.
So we'll end up with something like the following - does it help?
Side note: exit(1) is not the best way to handle
errors in the init function. I think you should add error_report, then
goto err on failures to init, cleanup what you have set up
meanwhile and return an error code.
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..c58f4d3 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -500,11 +500,29 @@ static void ivshmem_read(void *opaque, const uint8_t *
buf, int flags)
return;
}
+/* Select the MSI-X vectors used by device.
+ * ivshmem maps events to vectors statically, so
+ * we just enable all vectors on init and after reset. */
+static void ivshmem_use_msix(IVShmemState * s)
+{
+int i;
+
+if (!msix_present(&s->dev)) {
+return;
+}
+
+for (i = 0; i < s->vectors; i++) {
+msix_vector_use(&s->dev, i);
+}
+}
+
static void ivshmem_reset(DeviceState *d)
{
IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
s->intrstatus = 0;
+msix_reset(&s->dev);
+ivshmem_use_msix(s);
return;
}
@@ -535,12 +553,8 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
return value;
}
-static void ivshmem_setup_msi(IVShmemState * s) {
-
-int i;
-
-/* allocate the MSI-X vectors */
-
+static void ivshmem_setup_msi(IVShmemState * s)
+{
memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
@@ -551,13 +565,10 @@ static void ivshmem_setup_msi(IVShmemState * s) {
exit(1);
}
-/* 'activate' the vectors */
-for (i = 0; i < s->vectors; i++) {
-msix_vector_use(&s->dev, i);
-}
-
/* allocate Qemu char devices for receiving interrupts */
s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
+
+ivshmem_use_msix(s);
}
static void ivshmem_save(QEMUFile* f, void *opaque)
@@ -610,6 +621,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
version_id)
return 0;
}
+static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
+uint32_t val, int len)
+{
+pci_default_write_config(pci_dev, address, val, len);
+msix_write_config(pci_dev, address, val, len);
+}
+
static int pci_ivshmem_init(PCIDevice *dev)
{
IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -734,6 +752,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
}
+s->dev.config_write = ivshmem_write_config;
+
return 0;
}
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Sun, Dec 4, 2011 at 3:20 AM, Michael S. Tsirkin wrote:
> On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> the ivshmem device.
>
> I think the following should fix it. Compiled-only -
> could you pls check? If yes let's apply to the stable branch.
Thanks for the patch Michael.
It addresses the need for msix_write_config() to be called, but the
addition of the msix_reset() is causing a reset of the vectors after
they've been initialized in pci_ivshmem_init(). So, interrupts still
aren't delivered with this patch applied as it is.
In particular, a reset occurs after pci_ivshmem_init runs, so the
msix_entry_used array is reset to 0s, which causes the interrupt
delivery to fail.
If I comment out the msix_reset(), then interrupts are delivered.
Would the reset be caused by a bug in the guest driver? or do I need
to reconfigure the msix after reset? I'm unclear as to the proper
behaviour after a reset.
Thanks,
Cam
>
> -->
>
> ivshmem: add missing msix calls
>
> ivshmem used msix but didn't call it on either reset or
> config write paths. This used to partically work since
> guests don't use all of msi-x configuration fields,
> and reset is rarely used, but the patch 'msix: track function masked
> in pci device state' broke that. Fix by adding appropriate calls.
>
> Signed-off-by: Michael S. Tsirkin
>
> --
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..3680c0f 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
> IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
> s->intrstatus = 0;
> + msix_reset(&s->dev);
> return;
> }
>
> @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
> version_id)
> return 0;
> }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> + uint32_t val, int len)
> +{
> + pci_default_write_config(pci_dev, address, val, len);
> + msix_write_config(pci_dev, address, val, len);
> +}
> +
> static int pci_ivshmem_init(PCIDevice *dev)
> {
> IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
> }
>
> + s->dev.config_write = ivshmem_write_config;
> +
> return 0;
> }
>
>
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Sun, Dec 04, 2011 at 01:35:03PM +0100, Jan Kiszka wrote:
> On 2011-12-04 11:20, Michael S. Tsirkin wrote:
> > On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> >> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> >> the ivshmem device.
> >
> > I think the following should fix it. Compiled-only -
> > could you pls check? If yes let's apply to the stable branch.
> >
> > -->
> >
> > ivshmem: add missing msix calls
> >
> > ivshmem used msix but didn't call it on either reset or
> > config write paths. This used to partically work since
> > guests don't use all of msi-x configuration fields,
> > and reset is rarely used, but the patch 'msix: track function masked
> > in pci device state' broke that. Fix by adding appropriate calls.
> >
> > Signed-off-by: Michael S. Tsirkin
> >
> > --
> >
> > diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> > index 242fbea..3680c0f 100644
> > --- a/hw/ivshmem.c
> > +++ b/hw/ivshmem.c
> > @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
> > IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
> >
> > s->intrstatus = 0;
> > +msix_reset(&s->dev);
> > return;
> > }
> >
> > @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
> > version_id)
> > return 0;
> > }
> >
> > +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> > +uint32_t val, int len)
> > +{
> > +pci_default_write_config(pci_dev, address, val, len);
> > +msix_write_config(pci_dev, address, val, len);
> > +}
> > +
> > static int pci_ivshmem_init(PCIDevice *dev)
> > {
> > IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> > @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
> >
> > }
> >
> > +s->dev.config_write = ivshmem_write_config;
> > +
> > return 0;
> > }
> >
> >
> >
>
> But please fix this for real and merge [1]&[2] (with depending patches)
> into master. The above is just boilerplate code from device POV.
>
> Jan
>
> [1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80240
> [2] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80244
>
Yes, I agree we should make it easier for devices.
What annoyed me was the need to put msix in save/load.
And that is because of the need to do this in a specific
order. I hope to switch to an unordered format and
then this will become straight-forward.
--
MST
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On 2011-12-04 11:20, Michael S. Tsirkin wrote:
> On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
>> Based on a git bisect, this patch breaks msi-x interrupt delivery in
>> the ivshmem device.
>
> I think the following should fix it. Compiled-only -
> could you pls check? If yes let's apply to the stable branch.
>
> -->
>
> ivshmem: add missing msix calls
>
> ivshmem used msix but didn't call it on either reset or
> config write paths. This used to partically work since
> guests don't use all of msi-x configuration fields,
> and reset is rarely used, but the patch 'msix: track function masked
> in pci device state' broke that. Fix by adding appropriate calls.
>
> Signed-off-by: Michael S. Tsirkin
>
> --
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..3680c0f 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
> IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
>
> s->intrstatus = 0;
> +msix_reset(&s->dev);
> return;
> }
>
> @@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
> version_id)
> return 0;
> }
>
> +static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> + uint32_t val, int len)
> +{
> +pci_default_write_config(pci_dev, address, val, len);
> +msix_write_config(pci_dev, address, val, len);
> +}
> +
> static int pci_ivshmem_init(PCIDevice *dev)
> {
> IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
> }
>
> +s->dev.config_write = ivshmem_write_config;
> +
> return 0;
> }
>
>
>
But please fix this for real and merge [1]&[2] (with depending patches)
into master. The above is just boilerplate code from device POV.
Jan
[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80240
[2] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/80244
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> the ivshmem device.
I think the following should fix it. Compiled-only -
could you pls check? If yes let's apply to the stable branch.
-->
ivshmem: add missing msix calls
ivshmem used msix but didn't call it on either reset or
config write paths. This used to partically work since
guests don't use all of msi-x configuration fields,
and reset is rarely used, but the patch 'msix: track function masked
in pci device state' broke that. Fix by adding appropriate calls.
Signed-off-by: Michael S. Tsirkin
--
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..3680c0f 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -505,6 +505,7 @@ static void ivshmem_reset(DeviceState *d)
IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
s->intrstatus = 0;
+msix_reset(&s->dev);
return;
}
@@ -610,6 +611,13 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int
version_id)
return 0;
}
+static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
+uint32_t val, int len)
+{
+pci_default_write_config(pci_dev, address, val, len);
+msix_write_config(pci_dev, address, val, len);
+}
+
static int pci_ivshmem_init(PCIDevice *dev)
{
IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -734,6 +742,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
}
+s->dev.config_write = ivshmem_write_config;
+
return 0;
}
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote:
> Based on a git bisect, this patch breaks msi-x interrupt delivery in
> the ivshmem device.
>
> On Mon, Nov 21, 2011 at 9:57 AM, Michael S. Tsirkin wrote:
> > Only go over the table when function is masked.
> > This is not really important for qemu.git but helps
> > fix a bug in qemu-kvm.git.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > hw/msix.c | 21 ++---
> > hw/pci.h | 2 ++
> > 2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b15bafc..63b41b9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev,
> > unsigned short nentries,
> > /* Make flags bit writable. */
> > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > MSIX_MASKALL_MASK;
> > + pdev->msix_function_masked = true;
> > return 0;
>
> iiuc, this masks the msix by default.
Yes, because msi-x is disabled by default, that's
in the pci spec.
> > }
> >
> > @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int
> > vector)
> > *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> > }
> >
> > -static int msix_function_masked(PCIDevice *dev)
> > -{
> > - return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > MSIX_MASKALL_MASK;
> > -}
> > -
> > static int msix_is_masked(PCIDevice *dev, int vector)
> > {
> > unsigned offset =
> > vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > - return msix_function_masked(dev) ||
> > + return dev->msix_function_masked ||
> > dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > }
> >
> > @@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev,
> > int vector)
> > }
> > }
> >
> > +static void msix_update_function_masked(PCIDevice *dev)
> > +{
> > + dev->msix_function_masked = !msix_enabled(dev) ||
> > + (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > MSIX_MASKALL_MASK);
> > +}
> > +
> > /* Handle MSI-X capability config write. */
> > void msix_write_config(PCIDevice *dev, uint32_t addr,
> > uint32_t val, int len)
> > {
> > unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> > int vector;
> > + bool was_masked;
> >
> > if (!range_covers_byte(addr, len, enable_pos)) {
> > return;
> > }
> >
> > + was_masked = dev->msix_function_masked;
> > + msix_update_function_masked(dev);
> > +
> > if (!msix_enabled(dev)) {
> > return;
> > }
> >
> > pci_device_deassert_intx(dev);
> >
> > - if (msix_function_masked(dev)) {
> > + if (dev->msix_function_masked == was_masked) {
> > return;
> > }
>
> So I believe my bug is due to the fact the new logic included in this
> patch requires msix_write_config() to be called to unmask the vectors.
Not exactly, to enable msi-x really.
> Virtio-pci calls msix_write_config(), but ivshmem does not (nor does
> PCIe so I'm not sure if it's also affected).
At this point PCIe is a stub.
> I haven't been able to fix the bug yet, but I wanted to make sure I
> was looking in the correct place. Any help of further explanation of
> this patch would be greatly appreciated.
>
> Sincerely,
> Cam
So I think you just need to call msix_write_config,
otherwise msix is not getting enabled.
BTW looking at the ivshmem code, this bit looks wrong:
pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
I think the spec says IO/MEMORY must be disabled at init time since BARs
are not yet set to anything reasonable.
> >
> > @@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> > msix_free_irq_entries(dev);
> > qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> > qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) /
> > 8);
> > + msix_update_function_masked(dev);
> > }
> >
> > /* Does device support MSI-X? */
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 4b2e785..625e717 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -178,6 +178,8 @@ struct PCIDevice {
> > unsigned *msix_entry_used;
> > /* Region including the MSI-X table */
> > uint32_t msix_bar_size;
> > + /* MSIX function mask set or MSIX disabled */
> > + bool msix_function_masked;
> > /* Version id needed for VMState */
> > int32_t version_id;
> >
> > --
> > 1.7.5.53.gc233e
> >
> >
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
On 2011-12-03 00:34, Cam Macdonell wrote: > So I believe my bug is due to the fact the new logic included in this > patch requires msix_write_config() to be called to unmask the vectors. > Virtio-pci calls msix_write_config(), but ivshmem does not (nor does > PCIe so I'm not sure if it's also affected). msi[x]_write_config should not have to be called from device models but it must be right now. I proposed a patch a while ago to improve this. Same for msix_reset. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
Based on a git bisect, this patch breaks msi-x interrupt delivery in
the ivshmem device.
On Mon, Nov 21, 2011 at 9:57 AM, Michael S. Tsirkin wrote:
> Only go over the table when function is masked.
> This is not really important for qemu.git but helps
> fix a bug in qemu-kvm.git.
>
> Signed-off-by: Michael S. Tsirkin
> ---
> hw/msix.c | 21 ++---
> hw/pci.h | 2 ++
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index b15bafc..63b41b9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned
> short nentries,
> /* Make flags bit writable. */
> pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> MSIX_MASKALL_MASK;
> + pdev->msix_function_masked = true;
> return 0;
iiuc, this masks the msix by default.
> }
>
> @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
> }
>
> -static int msix_function_masked(PCIDevice *dev)
> -{
> - return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> MSIX_MASKALL_MASK;
> -}
> -
> static int msix_is_masked(PCIDevice *dev, int vector)
> {
> unsigned offset =
> vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> - return msix_function_masked(dev) ||
> + return dev->msix_function_masked ||
> dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> }
>
> @@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int
> vector)
> }
> }
>
> +static void msix_update_function_masked(PCIDevice *dev)
> +{
> + dev->msix_function_masked = !msix_enabled(dev) ||
> + (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> MSIX_MASKALL_MASK);
> +}
> +
> /* Handle MSI-X capability config write. */
> void msix_write_config(PCIDevice *dev, uint32_t addr,
> uint32_t val, int len)
> {
> unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> int vector;
> + bool was_masked;
>
> if (!range_covers_byte(addr, len, enable_pos)) {
> return;
> }
>
> + was_masked = dev->msix_function_masked;
> + msix_update_function_masked(dev);
> +
> if (!msix_enabled(dev)) {
> return;
> }
>
> pci_device_deassert_intx(dev);
>
> - if (msix_function_masked(dev)) {
> + if (dev->msix_function_masked == was_masked) {
> return;
> }
So I believe my bug is due to the fact the new logic included in this
patch requires msix_write_config() to be called to unmask the vectors.
Virtio-pci calls msix_write_config(), but ivshmem does not (nor does
PCIe so I'm not sure if it's also affected).
I haven't been able to fix the bug yet, but I wanted to make sure I
was looking in the correct place. Any help of further explanation of
this patch would be greatly appreciated.
Sincerely,
Cam
>
> @@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> msix_free_irq_entries(dev);
> qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> + msix_update_function_masked(dev);
> }
>
> /* Does device support MSI-X? */
> diff --git a/hw/pci.h b/hw/pci.h
> index 4b2e785..625e717 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -178,6 +178,8 @@ struct PCIDevice {
> unsigned *msix_entry_used;
> /* Region including the MSI-X table */
> uint32_t msix_bar_size;
> + /* MSIX function mask set or MSIX disabled */
> + bool msix_function_masked;
> /* Version id needed for VMState */
> int32_t version_id;
>
> --
> 1.7.5.53.gc233e
>
>
[Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state
Only go over the table when function is masked.
This is not really important for qemu.git but helps
fix a bug in qemu-kvm.git.
Signed-off-by: Michael S. Tsirkin
---
hw/msix.c | 21 ++---
hw/pci.h |2 ++
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index b15bafc..63b41b9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned
short nentries,
/* Make flags bit writable. */
pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
MSIX_MASKALL_MASK;
+pdev->msix_function_masked = true;
return 0;
}
@@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
*msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
}
-static int msix_function_masked(PCIDevice *dev)
-{
-return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
MSIX_MASKALL_MASK;
-}
-
static int msix_is_masked(PCIDevice *dev, int vector)
{
unsigned offset =
vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-return msix_function_masked(dev) ||
+return dev->msix_function_masked ||
dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
}
@@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *dev, int
vector)
}
}
+static void msix_update_function_masked(PCIDevice *dev)
+{
+dev->msix_function_masked = !msix_enabled(dev) ||
+(dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK);
+}
+
/* Handle MSI-X capability config write. */
void msix_write_config(PCIDevice *dev, uint32_t addr,
uint32_t val, int len)
{
unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
int vector;
+bool was_masked;
if (!range_covers_byte(addr, len, enable_pos)) {
return;
}
+was_masked = dev->msix_function_masked;
+msix_update_function_masked(dev);
+
if (!msix_enabled(dev)) {
return;
}
pci_device_deassert_intx(dev);
-if (msix_function_masked(dev)) {
+if (dev->msix_function_masked == was_masked) {
return;
}
@@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
msix_free_irq_entries(dev);
qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+msix_update_function_masked(dev);
}
/* Does device support MSI-X? */
diff --git a/hw/pci.h b/hw/pci.h
index 4b2e785..625e717 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -178,6 +178,8 @@ struct PCIDevice {
unsigned *msix_entry_used;
/* Region including the MSI-X table */
uint32_t msix_bar_size;
+/* MSIX function mask set or MSIX disabled */
+bool msix_function_masked;
/* Version id needed for VMState */
int32_t version_id;
--
1.7.5.53.gc233e
