* [PATCH 0/2] msix: couple fixes
@ 2010-10-22 20:31 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:31 UTC (permalink / raw)
To: mst; +Cc: kvm, qemu-devel, alex.williamson
I've been porting the qemu vfio driver to use the common msix
infrastructure and hit a couple issues. The first patch addresses
the problem that a device may already have MSI-X capability setup
and non-adjustable BARs, so msix_init should be able to use the
existing capability instead of adding it's own.
I'm surprised we need the second patch, as I'm not sure how things
work today without it. We allocate msix_irq_entries within a
KVM_CAP_IRQCHIP #ifdef. This should be pulled in from kvm.h, if we
have CONFIG_KVM, but that's pulled in from config-target.h, which
we don't include. We can add config.h to msix.c, but that only
works if the msix object is in Makefile.target instead of
Makefile.objs. Feel free to fix this differently, but I think
something is wrong there. Thanks,
Alex
---
Alex Williamson (2):
msix: Pull in config.h for CONFIG_KVM
msix: Allow msix_init on a device with existing MSI-X capability
Makefile.objs | 2 --
Makefile.target | 1 +
hw/msix.c | 68 ++++++++++++++++++++++++++++++++-----------------------
3 files changed, 40 insertions(+), 31 deletions(-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 0/2] msix: couple fixes
@ 2010-10-22 20:31 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:31 UTC (permalink / raw)
To: mst; +Cc: alex.williamson, qemu-devel, kvm
I've been porting the qemu vfio driver to use the common msix
infrastructure and hit a couple issues. The first patch addresses
the problem that a device may already have MSI-X capability setup
and non-adjustable BARs, so msix_init should be able to use the
existing capability instead of adding it's own.
I'm surprised we need the second patch, as I'm not sure how things
work today without it. We allocate msix_irq_entries within a
KVM_CAP_IRQCHIP #ifdef. This should be pulled in from kvm.h, if we
have CONFIG_KVM, but that's pulled in from config-target.h, which
we don't include. We can add config.h to msix.c, but that only
works if the msix object is in Makefile.target instead of
Makefile.objs. Feel free to fix this differently, but I think
something is wrong there. Thanks,
Alex
---
Alex Williamson (2):
msix: Pull in config.h for CONFIG_KVM
msix: Allow msix_init on a device with existing MSI-X capability
Makefile.objs | 2 --
Makefile.target | 1 +
hw/msix.c | 68 ++++++++++++++++++++++++++++++++-----------------------
3 files changed, 40 insertions(+), 31 deletions(-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
2010-10-22 20:31 ` [Qemu-devel] " Alex Williamson
@ 2010-10-22 20:40 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:40 UTC (permalink / raw)
To: mst; +Cc: kvm, qemu-devel, alex.williamson
To enable common msix support to be used with pass through devices,
don't attempt to change the BAR if the device already has an
MSI-X capability. This also means we want to pay closer attention
to the size when we map the msix table page, as it isn't necessarily
covering the entire end of the BAR.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
1 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index 43efbd2..4122395 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
{
int config_offset;
uint8_t *config;
- uint32_t new_size;
- if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
- return -EINVAL;
- if (bar_size > 0x80000000)
- return -ENOSPC;
-
- /* Add space for MSI-X structures */
- if (!bar_size) {
- new_size = MSIX_PAGE_SIZE;
- } else if (bar_size < MSIX_PAGE_SIZE) {
- bar_size = MSIX_PAGE_SIZE;
- new_size = MSIX_PAGE_SIZE * 2;
- } else {
- new_size = bar_size * 2;
- }
-
- pdev->msix_bar_size = new_size;
- config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
- if (config_offset < 0)
- return config_offset;
- config = pdev->config + config_offset;
-
- pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
- /* Table on top of BAR */
- pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
- /* Pending bits on top of that */
- pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
- bar_nr);
+ pdev->msix_bar_size = bar_size;
+
+ config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
+
+ if (!config_offset) {
+ uint32_t new_size;
+
+ if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
+ return -EINVAL;
+ if (bar_size > 0x80000000)
+ return -ENOSPC;
+
+ /* Add space for MSI-X structures */
+ if (!bar_size) {
+ new_size = MSIX_PAGE_SIZE;
+ } else if (bar_size < MSIX_PAGE_SIZE) {
+ bar_size = MSIX_PAGE_SIZE;
+ new_size = MSIX_PAGE_SIZE * 2;
+ } else {
+ new_size = bar_size * 2;
+ }
+
+ pdev->msix_bar_size = new_size;
+ config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
+ MSIX_CAP_LENGTH);
+ if (config_offset < 0)
+ return config_offset;
+ config = pdev->config + config_offset;
+
+ pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
+ /* Table on top of BAR */
+ pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
+ /* Pending bits on top of that */
+ pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
+ bar_nr);
+ }
pdev->msix_cap = config_offset;
/* Make flags bit writeable. */
pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
@@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
return;
if (size <= offset)
return;
- cpu_register_physical_memory(addr + offset, size - offset,
+ cpu_register_physical_memory(addr + offset,
+ MIN(size - offset, MSIX_PAGE_SIZE),
d->msix_mmio_index);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
@ 2010-10-22 20:40 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:40 UTC (permalink / raw)
To: mst; +Cc: alex.williamson, qemu-devel, kvm
To enable common msix support to be used with pass through devices,
don't attempt to change the BAR if the device already has an
MSI-X capability. This also means we want to pay closer attention
to the size when we map the msix table page, as it isn't necessarily
covering the entire end of the BAR.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
1 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index 43efbd2..4122395 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
{
int config_offset;
uint8_t *config;
- uint32_t new_size;
- if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
- return -EINVAL;
- if (bar_size > 0x80000000)
- return -ENOSPC;
-
- /* Add space for MSI-X structures */
- if (!bar_size) {
- new_size = MSIX_PAGE_SIZE;
- } else if (bar_size < MSIX_PAGE_SIZE) {
- bar_size = MSIX_PAGE_SIZE;
- new_size = MSIX_PAGE_SIZE * 2;
- } else {
- new_size = bar_size * 2;
- }
-
- pdev->msix_bar_size = new_size;
- config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
- if (config_offset < 0)
- return config_offset;
- config = pdev->config + config_offset;
-
- pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
- /* Table on top of BAR */
- pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
- /* Pending bits on top of that */
- pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
- bar_nr);
+ pdev->msix_bar_size = bar_size;
+
+ config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
+
+ if (!config_offset) {
+ uint32_t new_size;
+
+ if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
+ return -EINVAL;
+ if (bar_size > 0x80000000)
+ return -ENOSPC;
+
+ /* Add space for MSI-X structures */
+ if (!bar_size) {
+ new_size = MSIX_PAGE_SIZE;
+ } else if (bar_size < MSIX_PAGE_SIZE) {
+ bar_size = MSIX_PAGE_SIZE;
+ new_size = MSIX_PAGE_SIZE * 2;
+ } else {
+ new_size = bar_size * 2;
+ }
+
+ pdev->msix_bar_size = new_size;
+ config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
+ MSIX_CAP_LENGTH);
+ if (config_offset < 0)
+ return config_offset;
+ config = pdev->config + config_offset;
+
+ pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
+ /* Table on top of BAR */
+ pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
+ /* Pending bits on top of that */
+ pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
+ bar_nr);
+ }
pdev->msix_cap = config_offset;
/* Make flags bit writeable. */
pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
@@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
return;
if (size <= offset)
return;
- cpu_register_physical_memory(addr + offset, size - offset,
+ cpu_register_physical_memory(addr + offset,
+ MIN(size - offset, MSIX_PAGE_SIZE),
d->msix_mmio_index);
}
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-22 20:31 ` [Qemu-devel] " Alex Williamson
@ 2010-10-22 20:40 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:40 UTC (permalink / raw)
To: mst; +Cc: kvm, qemu-devel, alex.williamson
We need to pull in config.h or else kvm.h doesn't pull in
linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
defined. This requires moving the object over to Makefile.target
or else we can't find config-target.h
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
Makefile.objs | 2 --
Makefile.target | 1 +
hw/msix.c | 1 +
3 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index ca2d2d0..c097d25 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
# PCI watchdog devices
hw-obj-y += wdt_i6300esb.o
-hw-obj-y += msix.o
-
# PCI network cards
hw-obj-y += ne2000.o
hw-obj-y += eepro100.o
diff --git a/Makefile.target b/Makefile.target
index 347ad6b..63da13b 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -185,6 +185,7 @@ obj-y += rwhandler.o
obj-$(CONFIG_KVM) += kvm.o kvm-all.o
obj-$(CONFIG_NO_KVM) += kvm-stub.o
obj-y += memory.o
+obj-y += msix.o
LIBS+=-lz
diff --git a/hw/msix.c b/hw/msix.c
index 4122395..23256c9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -11,6 +11,7 @@
* the COPYING file in the top-level directory.
*/
+#include "config.h"
#include "hw.h"
#include "msix.h"
#include "pci.h"
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-22 20:40 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-22 20:40 UTC (permalink / raw)
To: mst; +Cc: alex.williamson, qemu-devel, kvm
We need to pull in config.h or else kvm.h doesn't pull in
linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
defined. This requires moving the object over to Makefile.target
or else we can't find config-target.h
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
Makefile.objs | 2 --
Makefile.target | 1 +
hw/msix.c | 1 +
3 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index ca2d2d0..c097d25 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
# PCI watchdog devices
hw-obj-y += wdt_i6300esb.o
-hw-obj-y += msix.o
-
# PCI network cards
hw-obj-y += ne2000.o
hw-obj-y += eepro100.o
diff --git a/Makefile.target b/Makefile.target
index 347ad6b..63da13b 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -185,6 +185,7 @@ obj-y += rwhandler.o
obj-$(CONFIG_KVM) += kvm.o kvm-all.o
obj-$(CONFIG_NO_KVM) += kvm-stub.o
obj-y += memory.o
+obj-y += msix.o
LIBS+=-lz
diff --git a/hw/msix.c b/hw/msix.c
index 4122395..23256c9 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -11,6 +11,7 @@
* the COPYING file in the top-level directory.
*/
+#include "config.h"
#include "hw.h"
#include "msix.h"
#include "pci.h"
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-22 20:40 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 1:50 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 1:50 UTC (permalink / raw)
To: mst; +Cc: kvm, qemu-devel
On Fri, 2010-10-22 at 14:40 -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined. This requires moving the object over to Makefile.target
> or else we can't find config-target.h
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> Makefile.objs | 2 --
> Makefile.target | 1 +
> hw/msix.c | 1 +
> 3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> # PCI watchdog devices
> hw-obj-y += wdt_i6300esb.o
>
> -hw-obj-y += msix.o
> -
> # PCI network cards
> hw-obj-y += ne2000.o
> hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
> obj-y += memory.o
> +obj-y += msix.o
Oops, memory.c isn't upstream, I'll push it down in my patch queue and
send a new one.
Alex
> LIBS+=-lz
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
> * the COPYING file in the top-level directory.
> */
>
> +#include "config.h"
> #include "hw.h"
> #include "msix.h"
> #include "pci.h"
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 1:50 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 1:50 UTC (permalink / raw)
To: mst; +Cc: qemu-devel, kvm
On Fri, 2010-10-22 at 14:40 -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined. This requires moving the object over to Makefile.target
> or else we can't find config-target.h
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> Makefile.objs | 2 --
> Makefile.target | 1 +
> hw/msix.c | 1 +
> 3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> # PCI watchdog devices
> hw-obj-y += wdt_i6300esb.o
>
> -hw-obj-y += msix.o
> -
> # PCI network cards
> hw-obj-y += ne2000.o
> hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
> obj-y += memory.o
> +obj-y += msix.o
Oops, memory.c isn't upstream, I'll push it down in my patch queue and
send a new one.
Alex
> LIBS+=-lz
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
> * the COPYING file in the top-level directory.
> */
>
> +#include "config.h"
> #include "hw.h"
> #include "msix.h"
> #include "pci.h"
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 1:50 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 7:41 ` Paolo Bonzini
-1 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2010-10-23 7:41 UTC (permalink / raw)
To: Alex Williamson; +Cc: mst, kvm, qemu-devel
On 10/23/2010 03:50 AM, Alex Williamson wrote:
> Oops, memory.c isn't upstream, I'll push it down in my patch queue and
> send a new one.
Neither is kvm_set_irq actually. :) This patch is only needed for qemu-kvm.
BTW, maybe the better solution would be to move the kvm_*_irq* functions
from qemu-kvm.c to kvm-all.c, add stubs to kvm-stub.c, and get rid of
the #ifdef completely in msix.c
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 7:41 ` Paolo Bonzini
0 siblings, 0 replies; 30+ messages in thread
From: Paolo Bonzini @ 2010-10-23 7:41 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm, mst
On 10/23/2010 03:50 AM, Alex Williamson wrote:
> Oops, memory.c isn't upstream, I'll push it down in my patch queue and
> send a new one.
Neither is kvm_set_irq actually. :) This patch is only needed for qemu-kvm.
BTW, maybe the better solution would be to move the kvm_*_irq* functions
from qemu-kvm.c to kvm-all.c, add stubs to kvm-stub.c, and get rid of
the #ifdef completely in msix.c
Paolo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-22 20:40 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 16:16 ` Michael S. Tsirkin
-1 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 16:16 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, qemu-devel
On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined. This requires moving the object over to Makefile.target
> or else we can't find config-target.h
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Why? We just moved it from .target to .objs, see
889e30cc18e21f2091b77267dca8096d7dd34f8b.
> ---
>
> Makefile.objs | 2 --
> Makefile.target | 1 +
> hw/msix.c | 1 +
> 3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> # PCI watchdog devices
> hw-obj-y += wdt_i6300esb.o
>
> -hw-obj-y += msix.o
> -
> # PCI network cards
> hw-obj-y += ne2000.o
> hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
> obj-y += memory.o
> +obj-y += msix.o
>
> LIBS+=-lz
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
> * the COPYING file in the top-level directory.
> */
>
> +#include "config.h"
> #include "hw.h"
> #include "msix.h"
> #include "pci.h"
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 16:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 16:16 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm
On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> We need to pull in config.h or else kvm.h doesn't pull in
> linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> defined. This requires moving the object over to Makefile.target
> or else we can't find config-target.h
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Why? We just moved it from .target to .objs, see
889e30cc18e21f2091b77267dca8096d7dd34f8b.
> ---
>
> Makefile.objs | 2 --
> Makefile.target | 1 +
> hw/msix.c | 1 +
> 3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca2d2d0..c097d25 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> # PCI watchdog devices
> hw-obj-y += wdt_i6300esb.o
>
> -hw-obj-y += msix.o
> -
> # PCI network cards
> hw-obj-y += ne2000.o
> hw-obj-y += eepro100.o
> diff --git a/Makefile.target b/Makefile.target
> index 347ad6b..63da13b 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> obj-$(CONFIG_NO_KVM) += kvm-stub.o
> obj-y += memory.o
> +obj-y += msix.o
>
> LIBS+=-lz
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 4122395..23256c9 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -11,6 +11,7 @@
> * the COPYING file in the top-level directory.
> */
>
> +#include "config.h"
> #include "hw.h"
> #include "msix.h"
> #include "pci.h"
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
2010-10-22 20:40 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 16:18 ` Michael S. Tsirkin
-1 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 16:18 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, qemu-devel
On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> To enable common msix support to be used with pass through devices,
> don't attempt to change the BAR if the device already has an
> MSI-X capability. This also means we want to pay closer attention
> to the size when we map the msix table page, as it isn't necessarily
> covering the entire end of the BAR.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> 1 files changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 43efbd2..4122395 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> {
> int config_offset;
> uint8_t *config;
> - uint32_t new_size;
>
> - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> - return -EINVAL;
> - if (bar_size > 0x80000000)
> - return -ENOSPC;
> -
> - /* Add space for MSI-X structures */
> - if (!bar_size) {
> - new_size = MSIX_PAGE_SIZE;
> - } else if (bar_size < MSIX_PAGE_SIZE) {
> - bar_size = MSIX_PAGE_SIZE;
> - new_size = MSIX_PAGE_SIZE * 2;
> - } else {
> - new_size = bar_size * 2;
> - }
> -
> - pdev->msix_bar_size = new_size;
> - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> - if (config_offset < 0)
> - return config_offset;
> - config = pdev->config + config_offset;
> -
> - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> - /* Table on top of BAR */
> - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> - /* Pending bits on top of that */
> - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> - bar_nr);
> + pdev->msix_bar_size = bar_size;
> +
> + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> +
> + if (!config_offset) {
> + uint32_t new_size;
> +
> + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> + return -EINVAL;
> + if (bar_size > 0x80000000)
> + return -ENOSPC;
> +
> + /* Add space for MSI-X structures */
> + if (!bar_size) {
> + new_size = MSIX_PAGE_SIZE;
> + } else if (bar_size < MSIX_PAGE_SIZE) {
> + bar_size = MSIX_PAGE_SIZE;
> + new_size = MSIX_PAGE_SIZE * 2;
> + } else {
> + new_size = bar_size * 2;
> + }
> +
> + pdev->msix_bar_size = new_size;
> + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> + MSIX_CAP_LENGTH);
> + if (config_offset < 0)
> + return config_offset;
> + config = pdev->config + config_offset;
> +
> + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> + /* Table on top of BAR */
> + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> + /* Pending bits on top of that */
> + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> + bar_nr);
> + }
> pdev->msix_cap = config_offset;
> /* Make flags bit writeable. */
> pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> return;
> if (size <= offset)
> return;
> - cpu_register_physical_memory(addr + offset, size - offset,
> + cpu_register_physical_memory(addr + offset,
> + MIN(size - offset, MSIX_PAGE_SIZE),
This is wrong I think, the table might not fit in a single page.
You would need to read table size out of from device config.
> d->msix_mmio_index);
> }
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
@ 2010-10-23 16:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 16:18 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm
On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> To enable common msix support to be used with pass through devices,
> don't attempt to change the BAR if the device already has an
> MSI-X capability. This also means we want to pay closer attention
> to the size when we map the msix table page, as it isn't necessarily
> covering the entire end of the BAR.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> 1 files changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 43efbd2..4122395 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> {
> int config_offset;
> uint8_t *config;
> - uint32_t new_size;
>
> - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> - return -EINVAL;
> - if (bar_size > 0x80000000)
> - return -ENOSPC;
> -
> - /* Add space for MSI-X structures */
> - if (!bar_size) {
> - new_size = MSIX_PAGE_SIZE;
> - } else if (bar_size < MSIX_PAGE_SIZE) {
> - bar_size = MSIX_PAGE_SIZE;
> - new_size = MSIX_PAGE_SIZE * 2;
> - } else {
> - new_size = bar_size * 2;
> - }
> -
> - pdev->msix_bar_size = new_size;
> - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> - if (config_offset < 0)
> - return config_offset;
> - config = pdev->config + config_offset;
> -
> - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> - /* Table on top of BAR */
> - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> - /* Pending bits on top of that */
> - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> - bar_nr);
> + pdev->msix_bar_size = bar_size;
> +
> + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> +
> + if (!config_offset) {
> + uint32_t new_size;
> +
> + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> + return -EINVAL;
> + if (bar_size > 0x80000000)
> + return -ENOSPC;
> +
> + /* Add space for MSI-X structures */
> + if (!bar_size) {
> + new_size = MSIX_PAGE_SIZE;
> + } else if (bar_size < MSIX_PAGE_SIZE) {
> + bar_size = MSIX_PAGE_SIZE;
> + new_size = MSIX_PAGE_SIZE * 2;
> + } else {
> + new_size = bar_size * 2;
> + }
> +
> + pdev->msix_bar_size = new_size;
> + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> + MSIX_CAP_LENGTH);
> + if (config_offset < 0)
> + return config_offset;
> + config = pdev->config + config_offset;
> +
> + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> + /* Table on top of BAR */
> + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> + /* Pending bits on top of that */
> + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> + bar_nr);
> + }
> pdev->msix_cap = config_offset;
> /* Make flags bit writeable. */
> pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> return;
> if (size <= offset)
> return;
> - cpu_register_physical_memory(addr + offset, size - offset,
> + cpu_register_physical_memory(addr + offset,
> + MIN(size - offset, MSIX_PAGE_SIZE),
This is wrong I think, the table might not fit in a single page.
You would need to read table size out of from device config.
> d->msix_mmio_index);
> }
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 16:16 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-10-23 16:52 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 16:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, qemu-devel
On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > We need to pull in config.h or else kvm.h doesn't pull in
> > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > defined. This requires moving the object over to Makefile.target
> > or else we can't find config-target.h
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>
> Why? We just moved it from .target to .objs, see
> 889e30cc18e21f2091b77267dca8096d7dd34f8b.
Maybe that's why it used to work. When building in the qemu-kvm.git
tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
allocated. Then when I call msix_vector_use, I get a seg fault.
Something is broken there. Thanks,
Alex
> > ---
> >
> > Makefile.objs | 2 --
> > Makefile.target | 1 +
> > hw/msix.c | 1 +
> > 3 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index ca2d2d0..c097d25 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> > # PCI watchdog devices
> > hw-obj-y += wdt_i6300esb.o
> >
> > -hw-obj-y += msix.o
> > -
> > # PCI network cards
> > hw-obj-y += ne2000.o
> > hw-obj-y += eepro100.o
> > diff --git a/Makefile.target b/Makefile.target
> > index 347ad6b..63da13b 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> > obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> > obj-$(CONFIG_NO_KVM) += kvm-stub.o
> > obj-y += memory.o
> > +obj-y += msix.o
> >
> > LIBS+=-lz
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 4122395..23256c9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -11,6 +11,7 @@
> > * the COPYING file in the top-level directory.
> > */
> >
> > +#include "config.h"
> > #include "hw.h"
> > #include "msix.h"
> > #include "pci.h"
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 16:52 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 16:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kvm
On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > We need to pull in config.h or else kvm.h doesn't pull in
> > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > defined. This requires moving the object over to Makefile.target
> > or else we can't find config-target.h
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>
> Why? We just moved it from .target to .objs, see
> 889e30cc18e21f2091b77267dca8096d7dd34f8b.
Maybe that's why it used to work. When building in the qemu-kvm.git
tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
allocated. Then when I call msix_vector_use, I get a seg fault.
Something is broken there. Thanks,
Alex
> > ---
> >
> > Makefile.objs | 2 --
> > Makefile.target | 1 +
> > hw/msix.c | 1 +
> > 3 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index ca2d2d0..c097d25 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -201,8 +201,6 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
> > # PCI watchdog devices
> > hw-obj-y += wdt_i6300esb.o
> >
> > -hw-obj-y += msix.o
> > -
> > # PCI network cards
> > hw-obj-y += ne2000.o
> > hw-obj-y += eepro100.o
> > diff --git a/Makefile.target b/Makefile.target
> > index 347ad6b..63da13b 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -185,6 +185,7 @@ obj-y += rwhandler.o
> > obj-$(CONFIG_KVM) += kvm.o kvm-all.o
> > obj-$(CONFIG_NO_KVM) += kvm-stub.o
> > obj-y += memory.o
> > +obj-y += msix.o
> >
> > LIBS+=-lz
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 4122395..23256c9 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -11,6 +11,7 @@
> > * the COPYING file in the top-level directory.
> > */
> >
> > +#include "config.h"
> > #include "hw.h"
> > #include "msix.h"
> > #include "pci.h"
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
2010-10-23 16:18 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-10-23 16:55 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 16:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, qemu-devel
On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > To enable common msix support to be used with pass through devices,
> > don't attempt to change the BAR if the device already has an
> > MSI-X capability. This also means we want to pay closer attention
> > to the size when we map the msix table page, as it isn't necessarily
> > covering the entire end of the BAR.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > 1 files changed, 38 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 43efbd2..4122395 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > {
> > int config_offset;
> > uint8_t *config;
> > - uint32_t new_size;
> >
> > - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > - return -EINVAL;
> > - if (bar_size > 0x80000000)
> > - return -ENOSPC;
> > -
> > - /* Add space for MSI-X structures */
> > - if (!bar_size) {
> > - new_size = MSIX_PAGE_SIZE;
> > - } else if (bar_size < MSIX_PAGE_SIZE) {
> > - bar_size = MSIX_PAGE_SIZE;
> > - new_size = MSIX_PAGE_SIZE * 2;
> > - } else {
> > - new_size = bar_size * 2;
> > - }
> > -
> > - pdev->msix_bar_size = new_size;
> > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > - if (config_offset < 0)
> > - return config_offset;
> > - config = pdev->config + config_offset;
> > -
> > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > - /* Table on top of BAR */
> > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > - /* Pending bits on top of that */
> > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > - bar_nr);
> > + pdev->msix_bar_size = bar_size;
> > +
> > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > +
> > + if (!config_offset) {
> > + uint32_t new_size;
> > +
> > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > + return -EINVAL;
> > + if (bar_size > 0x80000000)
> > + return -ENOSPC;
> > +
> > + /* Add space for MSI-X structures */
> > + if (!bar_size) {
> > + new_size = MSIX_PAGE_SIZE;
> > + } else if (bar_size < MSIX_PAGE_SIZE) {
> > + bar_size = MSIX_PAGE_SIZE;
> > + new_size = MSIX_PAGE_SIZE * 2;
> > + } else {
> > + new_size = bar_size * 2;
> > + }
> > +
> > + pdev->msix_bar_size = new_size;
> > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > + MSIX_CAP_LENGTH);
> > + if (config_offset < 0)
> > + return config_offset;
> > + config = pdev->config + config_offset;
> > +
> > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > + /* Table on top of BAR */
> > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > + /* Pending bits on top of that */
> > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > + bar_nr);
> > + }
> > pdev->msix_cap = config_offset;
> > /* Make flags bit writeable. */
> > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > return;
> > if (size <= offset)
> > return;
> > - cpu_register_physical_memory(addr + offset, size - offset,
> > + cpu_register_physical_memory(addr + offset,
> > + MIN(size - offset, MSIX_PAGE_SIZE),
>
> This is wrong I think, the table might not fit in a single page.
> You would need to read table size out of from device config.
That's true, but I was hoping to save that for later since we don't seem
to be running into that problem yet. Current device assignment code
assumes a single page, and I haven't heard of anyone with a vector table
that exceeds that yet. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
@ 2010-10-23 16:55 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 16:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kvm
On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > To enable common msix support to be used with pass through devices,
> > don't attempt to change the BAR if the device already has an
> > MSI-X capability. This also means we want to pay closer attention
> > to the size when we map the msix table page, as it isn't necessarily
> > covering the entire end of the BAR.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > 1 files changed, 38 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 43efbd2..4122395 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > {
> > int config_offset;
> > uint8_t *config;
> > - uint32_t new_size;
> >
> > - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > - return -EINVAL;
> > - if (bar_size > 0x80000000)
> > - return -ENOSPC;
> > -
> > - /* Add space for MSI-X structures */
> > - if (!bar_size) {
> > - new_size = MSIX_PAGE_SIZE;
> > - } else if (bar_size < MSIX_PAGE_SIZE) {
> > - bar_size = MSIX_PAGE_SIZE;
> > - new_size = MSIX_PAGE_SIZE * 2;
> > - } else {
> > - new_size = bar_size * 2;
> > - }
> > -
> > - pdev->msix_bar_size = new_size;
> > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > - if (config_offset < 0)
> > - return config_offset;
> > - config = pdev->config + config_offset;
> > -
> > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > - /* Table on top of BAR */
> > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > - /* Pending bits on top of that */
> > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > - bar_nr);
> > + pdev->msix_bar_size = bar_size;
> > +
> > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > +
> > + if (!config_offset) {
> > + uint32_t new_size;
> > +
> > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > + return -EINVAL;
> > + if (bar_size > 0x80000000)
> > + return -ENOSPC;
> > +
> > + /* Add space for MSI-X structures */
> > + if (!bar_size) {
> > + new_size = MSIX_PAGE_SIZE;
> > + } else if (bar_size < MSIX_PAGE_SIZE) {
> > + bar_size = MSIX_PAGE_SIZE;
> > + new_size = MSIX_PAGE_SIZE * 2;
> > + } else {
> > + new_size = bar_size * 2;
> > + }
> > +
> > + pdev->msix_bar_size = new_size;
> > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > + MSIX_CAP_LENGTH);
> > + if (config_offset < 0)
> > + return config_offset;
> > + config = pdev->config + config_offset;
> > +
> > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > + /* Table on top of BAR */
> > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > + /* Pending bits on top of that */
> > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > + bar_nr);
> > + }
> > pdev->msix_cap = config_offset;
> > /* Make flags bit writeable. */
> > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > return;
> > if (size <= offset)
> > return;
> > - cpu_register_physical_memory(addr + offset, size - offset,
> > + cpu_register_physical_memory(addr + offset,
> > + MIN(size - offset, MSIX_PAGE_SIZE),
>
> This is wrong I think, the table might not fit in a single page.
> You would need to read table size out of from device config.
That's true, but I was hoping to save that for later since we don't seem
to be running into that problem yet. Current device assignment code
assumes a single page, and I haven't heard of anyone with a vector table
that exceeds that yet. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 16:52 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 17:29 ` Michael S. Tsirkin
-1 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 17:29 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, qemu-devel
On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > We need to pull in config.h or else kvm.h doesn't pull in
> > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > defined. This requires moving the object over to Makefile.target
> > > or else we can't find config-target.h
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >
> > Why? We just moved it from .target to .objs, see
> > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
>
> Maybe that's why it used to work. When building in the qemu-kvm.git
> tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> allocated. Then when I call msix_vector_use, I get a seg fault.
> Something is broken there. Thanks,
>
> Alex
This is hopefully fixed in the latest bits.
bd8b215bce453706c3951460cc7e6627ccb90314
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 17:29 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 17:29 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm
On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > We need to pull in config.h or else kvm.h doesn't pull in
> > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > defined. This requires moving the object over to Makefile.target
> > > or else we can't find config-target.h
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >
> > Why? We just moved it from .target to .objs, see
> > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
>
> Maybe that's why it used to work. When building in the qemu-kvm.git
> tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> allocated. Then when I call msix_vector_use, I get a seg fault.
> Something is broken there. Thanks,
>
> Alex
This is hopefully fixed in the latest bits.
bd8b215bce453706c3951460cc7e6627ccb90314
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 17:29 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-10-23 18:42 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 18:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, qemu-devel
On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > defined. This requires moving the object over to Makefile.target
> > > > or else we can't find config-target.h
> > > >
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > >
> > > Why? We just moved it from .target to .objs, see
> > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> >
> > Maybe that's why it used to work. When building in the qemu-kvm.git
> > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > allocated. Then when I call msix_vector_use, I get a seg fault.
> > Something is broken there. Thanks,
> >
> > Alex
>
> This is hopefully fixed in the latest bits.
> bd8b215bce453706c3951460cc7e6627ccb90314
Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
which dereferences msix_irq_entries, which is only allocated in
msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
meant to remove the ifdef from msix_init? I also note there's another
in msix_notify. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 18:42 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 18:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kvm
On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > defined. This requires moving the object over to Makefile.target
> > > > or else we can't find config-target.h
> > > >
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > >
> > > Why? We just moved it from .target to .objs, see
> > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> >
> > Maybe that's why it used to work. When building in the qemu-kvm.git
> > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > allocated. Then when I call msix_vector_use, I get a seg fault.
> > Something is broken there. Thanks,
> >
> > Alex
>
> This is hopefully fixed in the latest bits.
> bd8b215bce453706c3951460cc7e6627ccb90314
Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
which dereferences msix_irq_entries, which is only allocated in
msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
meant to remove the ifdef from msix_init? I also note there's another
in msix_notify. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 18:42 ` [Qemu-devel] " Alex Williamson
@ 2010-10-23 20:38 ` Michael S. Tsirkin
-1 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 20:38 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, qemu-devel
On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > > defined. This requires moving the object over to Makefile.target
> > > > > or else we can't find config-target.h
> > > > >
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > >
> > > > Why? We just moved it from .target to .objs, see
> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> > >
> > > Maybe that's why it used to work. When building in the qemu-kvm.git
> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > > allocated. Then when I call msix_vector_use, I get a seg fault.
> > > Something is broken there. Thanks,
> > >
> > > Alex
> >
> > This is hopefully fixed in the latest bits.
> > bd8b215bce453706c3951460cc7e6627ccb90314
>
> Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
> which dereferences msix_irq_entries, which is only allocated in
> msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
> meant to remove the ifdef from msix_init? I also note there's another
> in msix_notify. Thanks,
>
> Alex
Not sure what's wrong:
$git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
$
http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD
also does not show any ifdefs.
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 20:38 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2010-10-23 20:38 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm
On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
> > > > > We need to pull in config.h or else kvm.h doesn't pull in
> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
> > > > > defined. This requires moving the object over to Makefile.target
> > > > > or else we can't find config-target.h
> > > > >
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > >
> > > > Why? We just moved it from .target to .objs, see
> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
> > >
> > > Maybe that's why it used to work. When building in the qemu-kvm.git
> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
> > > allocated. Then when I call msix_vector_use, I get a seg fault.
> > > Something is broken there. Thanks,
> > >
> > > Alex
> >
> > This is hopefully fixed in the latest bits.
> > bd8b215bce453706c3951460cc7e6627ccb90314
>
> Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
> which dereferences msix_irq_entries, which is only allocated in
> msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
> meant to remove the ifdef from msix_init? I also note there's another
> in msix_notify. Thanks,
>
> Alex
Not sure what's wrong:
$git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
$
http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD
also does not show any ifdefs.
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
2010-10-23 20:38 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-10-23 21:01 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 21:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, qemu-devel
On Sat, Oct 23, 2010 at 2:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
>> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
>> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
>> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
>> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
>> > > > > We need to pull in config.h or else kvm.h doesn't pull in
>> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
>> > > > > defined. This requires moving the object over to Makefile.target
>> > > > > or else we can't find config-target.h
>> > > > >
>> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > > >
>> > > > Why? We just moved it from .target to .objs, see
>> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
>> > >
>> > > Maybe that's why it used to work. When building in the qemu-kvm.git
>> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
>> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
>> > > allocated. Then when I call msix_vector_use, I get a seg fault.
>> > > Something is broken there. Thanks,
>> > >
>> > > Alex
>> >
>> > This is hopefully fixed in the latest bits.
>> > bd8b215bce453706c3951460cc7e6627ccb90314
>>
>> Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
>> which dereferences msix_irq_entries, which is only allocated in
>> msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
>> meant to remove the ifdef from msix_init? I also note there's another
>> in msix_notify. Thanks,
>>
>> Alex
>
> Not sure what's wrong:
> $git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
> $
>
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD
>
> also does not show any ifdefs.
Hmm, somehow my tree missed 763a04a920f1098e57ad6b46c91c3e531adc961d
Ignore this patch and I'll refresh again. Sorry for the noise.
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM
@ 2010-10-23 21:01 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-10-23 21:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kvm
On Sat, Oct 23, 2010 at 2:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Oct 23, 2010 at 12:42:44PM -0600, Alex Williamson wrote:
>> On Sat, 2010-10-23 at 19:29 +0200, Michael S. Tsirkin wrote:
>> > On Sat, Oct 23, 2010 at 10:52:43AM -0600, Alex Williamson wrote:
>> > > On Sat, 2010-10-23 at 18:16 +0200, Michael S. Tsirkin wrote:
>> > > > On Fri, Oct 22, 2010 at 02:40:39PM -0600, Alex Williamson wrote:
>> > > > > We need to pull in config.h or else kvm.h doesn't pull in
>> > > > > linux/config.h, which we need if we ever want KVM_CAP_IRQCHIP
>> > > > > defined. This requires moving the object over to Makefile.target
>> > > > > or else we can't find config-target.h
>> > > > >
>> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > > >
>> > > > Why? We just moved it from .target to .objs, see
>> > > > 889e30cc18e21f2091b77267dca8096d7dd34f8b.
>> > >
>> > > Maybe that's why it used to work. When building in the qemu-kvm.git
>> > > tree, I'm not getting CONFIG_KVM defined, which means I'm not getting
>> > > KVM_CAP_IRQCHIP defined, which results in msix_irq_entries not being
>> > > allocated. Then when I call msix_vector_use, I get a seg fault.
>> > > Something is broken there. Thanks,
>> > >
>> > > Alex
>> >
>> > This is hopefully fixed in the latest bits.
>> > bd8b215bce453706c3951460cc7e6627ccb90314
>>
>> Nope, my tree includes that. It's not kvm_set_irq, it's kvm_msix_add,
>> which dereferences msix_irq_entries, which is only allocated in
>> msix_init if KVM_CAP_IRQCHIP is defined, which it's not. Maybe you also
>> meant to remove the ifdef from msix_init? I also note there's another
>> in msix_notify. Thanks,
>>
>> Alex
>
> Not sure what's wrong:
> $git grep KVM_CAP_IRQCHIP origin/master -- hw/msix.c
> $
>
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob;f=hw/msix.c;hb=HEAD
>
> also does not show any ifdefs.
Hmm, somehow my tree missed 763a04a920f1098e57ad6b46c91c3e531adc961d
Ignore this patch and I'll refresh again. Sorry for the noise.
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
2010-10-23 16:55 ` [Qemu-devel] " Alex Williamson
@ 2010-10-28 15:00 ` Avi Kivity
-1 siblings, 0 replies; 30+ messages in thread
From: Avi Kivity @ 2010-10-28 15:00 UTC (permalink / raw)
To: Alex Williamson; +Cc: Michael S. Tsirkin, kvm, qemu-devel
On 10/23/2010 06:55 PM, Alex Williamson wrote:
> On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > > To enable common msix support to be used with pass through devices,
> > > don't attempt to change the BAR if the device already has an
> > > MSI-X capability. This also means we want to pay closer attention
> > > to the size when we map the msix table page, as it isn't necessarily
> > > covering the entire end of the BAR.
> > >
> > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> > > ---
> > >
> > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > > 1 files changed, 38 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/hw/msix.c b/hw/msix.c
> > > index 43efbd2..4122395 100644
> > > --- a/hw/msix.c
> > > +++ b/hw/msix.c
> > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > > {
> > > int config_offset;
> > > uint8_t *config;
> > > - uint32_t new_size;
> > >
> > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > - return -EINVAL;
> > > - if (bar_size> 0x80000000)
> > > - return -ENOSPC;
> > > -
> > > - /* Add space for MSI-X structures */
> > > - if (!bar_size) {
> > > - new_size = MSIX_PAGE_SIZE;
> > > - } else if (bar_size< MSIX_PAGE_SIZE) {
> > > - bar_size = MSIX_PAGE_SIZE;
> > > - new_size = MSIX_PAGE_SIZE * 2;
> > > - } else {
> > > - new_size = bar_size * 2;
> > > - }
> > > -
> > > - pdev->msix_bar_size = new_size;
> > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > > - if (config_offset< 0)
> > > - return config_offset;
> > > - config = pdev->config + config_offset;
> > > -
> > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > - /* Table on top of BAR */
> > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > - /* Pending bits on top of that */
> > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > - bar_nr);
> > > + pdev->msix_bar_size = bar_size;
> > > +
> > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > > +
> > > + if (!config_offset) {
> > > + uint32_t new_size;
> > > +
> > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > + return -EINVAL;
> > > + if (bar_size> 0x80000000)
> > > + return -ENOSPC;
> > > +
> > > + /* Add space for MSI-X structures */
> > > + if (!bar_size) {
> > > + new_size = MSIX_PAGE_SIZE;
> > > + } else if (bar_size< MSIX_PAGE_SIZE) {
> > > + bar_size = MSIX_PAGE_SIZE;
> > > + new_size = MSIX_PAGE_SIZE * 2;
> > > + } else {
> > > + new_size = bar_size * 2;
> > > + }
> > > +
> > > + pdev->msix_bar_size = new_size;
> > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > > + MSIX_CAP_LENGTH);
> > > + if (config_offset< 0)
> > > + return config_offset;
> > > + config = pdev->config + config_offset;
> > > +
> > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > + /* Table on top of BAR */
> > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > + /* Pending bits on top of that */
> > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > + bar_nr);
> > > + }
> > > pdev->msix_cap = config_offset;
> > > /* Make flags bit writeable. */
> > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > > return;
> > > if (size<= offset)
> > > return;
> > > - cpu_register_physical_memory(addr + offset, size - offset,
> > > + cpu_register_physical_memory(addr + offset,
> > > + MIN(size - offset, MSIX_PAGE_SIZE),
> >
> > This is wrong I think, the table might not fit in a single page.
> > You would need to read table size out of from device config.
>
> That's true, but I was hoping to save that for later since we don't seem
> to be running into that problem yet. Current device assignment code
> assumes a single page, and I haven't heard of anyone with a vector table
> that exceeds that yet. Thanks,
>
Ok; applied. Please add some warning if the condition happens so the
breakage is at least not silent.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
@ 2010-10-28 15:00 ` Avi Kivity
0 siblings, 0 replies; 30+ messages in thread
From: Avi Kivity @ 2010-10-28 15:00 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm, Michael S. Tsirkin
On 10/23/2010 06:55 PM, Alex Williamson wrote:
> On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > > To enable common msix support to be used with pass through devices,
> > > don't attempt to change the BAR if the device already has an
> > > MSI-X capability. This also means we want to pay closer attention
> > > to the size when we map the msix table page, as it isn't necessarily
> > > covering the entire end of the BAR.
> > >
> > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> > > ---
> > >
> > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > > 1 files changed, 38 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/hw/msix.c b/hw/msix.c
> > > index 43efbd2..4122395 100644
> > > --- a/hw/msix.c
> > > +++ b/hw/msix.c
> > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > > {
> > > int config_offset;
> > > uint8_t *config;
> > > - uint32_t new_size;
> > >
> > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > - return -EINVAL;
> > > - if (bar_size> 0x80000000)
> > > - return -ENOSPC;
> > > -
> > > - /* Add space for MSI-X structures */
> > > - if (!bar_size) {
> > > - new_size = MSIX_PAGE_SIZE;
> > > - } else if (bar_size< MSIX_PAGE_SIZE) {
> > > - bar_size = MSIX_PAGE_SIZE;
> > > - new_size = MSIX_PAGE_SIZE * 2;
> > > - } else {
> > > - new_size = bar_size * 2;
> > > - }
> > > -
> > > - pdev->msix_bar_size = new_size;
> > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > > - if (config_offset< 0)
> > > - return config_offset;
> > > - config = pdev->config + config_offset;
> > > -
> > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > - /* Table on top of BAR */
> > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > - /* Pending bits on top of that */
> > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > - bar_nr);
> > > + pdev->msix_bar_size = bar_size;
> > > +
> > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > > +
> > > + if (!config_offset) {
> > > + uint32_t new_size;
> > > +
> > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > + return -EINVAL;
> > > + if (bar_size> 0x80000000)
> > > + return -ENOSPC;
> > > +
> > > + /* Add space for MSI-X structures */
> > > + if (!bar_size) {
> > > + new_size = MSIX_PAGE_SIZE;
> > > + } else if (bar_size< MSIX_PAGE_SIZE) {
> > > + bar_size = MSIX_PAGE_SIZE;
> > > + new_size = MSIX_PAGE_SIZE * 2;
> > > + } else {
> > > + new_size = bar_size * 2;
> > > + }
> > > +
> > > + pdev->msix_bar_size = new_size;
> > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > > + MSIX_CAP_LENGTH);
> > > + if (config_offset< 0)
> > > + return config_offset;
> > > + config = pdev->config + config_offset;
> > > +
> > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > + /* Table on top of BAR */
> > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > + /* Pending bits on top of that */
> > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > + bar_nr);
> > > + }
> > > pdev->msix_cap = config_offset;
> > > /* Make flags bit writeable. */
> > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > > return;
> > > if (size<= offset)
> > > return;
> > > - cpu_register_physical_memory(addr + offset, size - offset,
> > > + cpu_register_physical_memory(addr + offset,
> > > + MIN(size - offset, MSIX_PAGE_SIZE),
> >
> > This is wrong I think, the table might not fit in a single page.
> > You would need to read table size out of from device config.
>
> That's true, but I was hoping to save that for later since we don't seem
> to be running into that problem yet. Current device assignment code
> assumes a single page, and I haven't heard of anyone with a vector table
> that exceeds that yet. Thanks,
>
Ok; applied. Please add some warning if the condition happens so the
breakage is at least not silent.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
2010-10-28 15:00 ` [Qemu-devel] " Avi Kivity
@ 2010-11-01 15:56 ` Alex Williamson
-1 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-11-01 15:56 UTC (permalink / raw)
To: Avi Kivity; +Cc: Michael S. Tsirkin, kvm, qemu-devel
On Thu, 2010-10-28 at 17:00 +0200, Avi Kivity wrote:
> On 10/23/2010 06:55 PM, Alex Williamson wrote:
> > On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > > > To enable common msix support to be used with pass through devices,
> > > > don't attempt to change the BAR if the device already has an
> > > > MSI-X capability. This also means we want to pay closer attention
> > > > to the size when we map the msix table page, as it isn't necessarily
> > > > covering the entire end of the BAR.
> > > >
> > > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> > > > ---
> > > >
> > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > > > 1 files changed, 38 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/hw/msix.c b/hw/msix.c
> > > > index 43efbd2..4122395 100644
> > > > --- a/hw/msix.c
> > > > +++ b/hw/msix.c
> > > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > > > {
> > > > int config_offset;
> > > > uint8_t *config;
> > > > - uint32_t new_size;
> > > >
> > > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > > - return -EINVAL;
> > > > - if (bar_size> 0x80000000)
> > > > - return -ENOSPC;
> > > > -
> > > > - /* Add space for MSI-X structures */
> > > > - if (!bar_size) {
> > > > - new_size = MSIX_PAGE_SIZE;
> > > > - } else if (bar_size< MSIX_PAGE_SIZE) {
> > > > - bar_size = MSIX_PAGE_SIZE;
> > > > - new_size = MSIX_PAGE_SIZE * 2;
> > > > - } else {
> > > > - new_size = bar_size * 2;
> > > > - }
> > > > -
> > > > - pdev->msix_bar_size = new_size;
> > > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > > > - if (config_offset< 0)
> > > > - return config_offset;
> > > > - config = pdev->config + config_offset;
> > > > -
> > > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > > - /* Table on top of BAR */
> > > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > > - /* Pending bits on top of that */
> > > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > > - bar_nr);
> > > > + pdev->msix_bar_size = bar_size;
> > > > +
> > > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > > > +
> > > > + if (!config_offset) {
> > > > + uint32_t new_size;
> > > > +
> > > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > > + return -EINVAL;
> > > > + if (bar_size> 0x80000000)
> > > > + return -ENOSPC;
> > > > +
> > > > + /* Add space for MSI-X structures */
> > > > + if (!bar_size) {
> > > > + new_size = MSIX_PAGE_SIZE;
> > > > + } else if (bar_size< MSIX_PAGE_SIZE) {
> > > > + bar_size = MSIX_PAGE_SIZE;
> > > > + new_size = MSIX_PAGE_SIZE * 2;
> > > > + } else {
> > > > + new_size = bar_size * 2;
> > > > + }
> > > > +
> > > > + pdev->msix_bar_size = new_size;
> > > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > > > + MSIX_CAP_LENGTH);
> > > > + if (config_offset< 0)
> > > > + return config_offset;
> > > > + config = pdev->config + config_offset;
> > > > +
> > > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > > + /* Table on top of BAR */
> > > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > > + /* Pending bits on top of that */
> > > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > > + bar_nr);
> > > > + }
> > > > pdev->msix_cap = config_offset;
> > > > /* Make flags bit writeable. */
> > > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > > > return;
> > > > if (size<= offset)
> > > > return;
> > > > - cpu_register_physical_memory(addr + offset, size - offset,
> > > > + cpu_register_physical_memory(addr + offset,
> > > > + MIN(size - offset, MSIX_PAGE_SIZE),
> > >
> > > This is wrong I think, the table might not fit in a single page.
> > > You would need to read table size out of from device config.
> >
> > That's true, but I was hoping to save that for later since we don't seem
> > to be running into that problem yet. Current device assignment code
> > assumes a single page, and I haven't heard of anyone with a vector table
> > that exceeds that yet. Thanks,
> >
>
> Ok; applied. Please add some warning if the condition happens so the
> breakage is at least not silent.
Looks like this is already handled before we even get here. msix_init()
checks nentries against the max supported in a single page and returns
-EINVAL if over. So I think we're safely covered until we add support
for more pages. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] Re: [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability
@ 2010-11-01 15:56 ` Alex Williamson
0 siblings, 0 replies; 30+ messages in thread
From: Alex Williamson @ 2010-11-01 15:56 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, kvm, Michael S. Tsirkin
On Thu, 2010-10-28 at 17:00 +0200, Avi Kivity wrote:
> On 10/23/2010 06:55 PM, Alex Williamson wrote:
> > On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote:
> > > > To enable common msix support to be used with pass through devices,
> > > > don't attempt to change the BAR if the device already has an
> > > > MSI-X capability. This also means we want to pay closer attention
> > > > to the size when we map the msix table page, as it isn't necessarily
> > > > covering the entire end of the BAR.
> > > >
> > > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> > > > ---
> > > >
> > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++--------------------------
> > > > 1 files changed, 38 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/hw/msix.c b/hw/msix.c
> > > > index 43efbd2..4122395 100644
> > > > --- a/hw/msix.c
> > > > +++ b/hw/msix.c
> > > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > > > {
> > > > int config_offset;
> > > > uint8_t *config;
> > > > - uint32_t new_size;
> > > >
> > > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > > - return -EINVAL;
> > > > - if (bar_size> 0x80000000)
> > > > - return -ENOSPC;
> > > > -
> > > > - /* Add space for MSI-X structures */
> > > > - if (!bar_size) {
> > > > - new_size = MSIX_PAGE_SIZE;
> > > > - } else if (bar_size< MSIX_PAGE_SIZE) {
> > > > - bar_size = MSIX_PAGE_SIZE;
> > > > - new_size = MSIX_PAGE_SIZE * 2;
> > > > - } else {
> > > > - new_size = bar_size * 2;
> > > > - }
> > > > -
> > > > - pdev->msix_bar_size = new_size;
> > > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > > > - if (config_offset< 0)
> > > > - return config_offset;
> > > > - config = pdev->config + config_offset;
> > > > -
> > > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > > - /* Table on top of BAR */
> > > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > > - /* Pending bits on top of that */
> > > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > > - bar_nr);
> > > > + pdev->msix_bar_size = bar_size;
> > > > +
> > > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX);
> > > > +
> > > > + if (!config_offset) {
> > > > + uint32_t new_size;
> > > > +
> > > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1)
> > > > + return -EINVAL;
> > > > + if (bar_size> 0x80000000)
> > > > + return -ENOSPC;
> > > > +
> > > > + /* Add space for MSI-X structures */
> > > > + if (!bar_size) {
> > > > + new_size = MSIX_PAGE_SIZE;
> > > > + } else if (bar_size< MSIX_PAGE_SIZE) {
> > > > + bar_size = MSIX_PAGE_SIZE;
> > > > + new_size = MSIX_PAGE_SIZE * 2;
> > > > + } else {
> > > > + new_size = bar_size * 2;
> > > > + }
> > > > +
> > > > + pdev->msix_bar_size = new_size;
> > > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > > > + MSIX_CAP_LENGTH);
> > > > + if (config_offset< 0)
> > > > + return config_offset;
> > > > + config = pdev->config + config_offset;
> > > > +
> > > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > > > + /* Table on top of BAR */
> > > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> > > > + /* Pending bits on top of that */
> > > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> > > > + bar_nr);
> > > > + }
> > > > pdev->msix_cap = config_offset;
> > > > /* Make flags bit writeable. */
> > > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num,
> > > > return;
> > > > if (size<= offset)
> > > > return;
> > > > - cpu_register_physical_memory(addr + offset, size - offset,
> > > > + cpu_register_physical_memory(addr + offset,
> > > > + MIN(size - offset, MSIX_PAGE_SIZE),
> > >
> > > This is wrong I think, the table might not fit in a single page.
> > > You would need to read table size out of from device config.
> >
> > That's true, but I was hoping to save that for later since we don't seem
> > to be running into that problem yet. Current device assignment code
> > assumes a single page, and I haven't heard of anyone with a vector table
> > that exceeds that yet. Thanks,
> >
>
> Ok; applied. Please add some warning if the condition happens so the
> breakage is at least not silent.
Looks like this is already handled before we even get here. msix_init()
checks nentries against the max supported in a single page and returns
-EINVAL if over. So I think we're safely covered until we add support
for more pages. Thanks,
Alex
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2010-11-01 15:56 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-22 20:31 [PATCH 0/2] msix: couple fixes Alex Williamson
2010-10-22 20:31 ` [Qemu-devel] " Alex Williamson
2010-10-22 20:40 ` [PATCH 1/2] msix: Allow msix_init on a device with existing MSI-X capability Alex Williamson
2010-10-22 20:40 ` [Qemu-devel] " Alex Williamson
2010-10-23 16:18 ` Michael S. Tsirkin
2010-10-23 16:18 ` [Qemu-devel] " Michael S. Tsirkin
2010-10-23 16:55 ` Alex Williamson
2010-10-23 16:55 ` [Qemu-devel] " Alex Williamson
2010-10-28 15:00 ` Avi Kivity
2010-10-28 15:00 ` [Qemu-devel] " Avi Kivity
2010-11-01 15:56 ` Alex Williamson
2010-11-01 15:56 ` [Qemu-devel] " Alex Williamson
2010-10-22 20:40 ` [PATCH 2/2] msix: Pull in config.h for CONFIG_KVM Alex Williamson
2010-10-22 20:40 ` [Qemu-devel] " Alex Williamson
2010-10-23 1:50 ` Alex Williamson
2010-10-23 1:50 ` [Qemu-devel] " Alex Williamson
2010-10-23 7:41 ` Paolo Bonzini
2010-10-23 7:41 ` [Qemu-devel] " Paolo Bonzini
2010-10-23 16:16 ` Michael S. Tsirkin
2010-10-23 16:16 ` [Qemu-devel] " Michael S. Tsirkin
2010-10-23 16:52 ` Alex Williamson
2010-10-23 16:52 ` [Qemu-devel] " Alex Williamson
2010-10-23 17:29 ` Michael S. Tsirkin
2010-10-23 17:29 ` [Qemu-devel] " Michael S. Tsirkin
2010-10-23 18:42 ` Alex Williamson
2010-10-23 18:42 ` [Qemu-devel] " Alex Williamson
2010-10-23 20:38 ` Michael S. Tsirkin
2010-10-23 20:38 ` [Qemu-devel] " Michael S. Tsirkin
2010-10-23 21:01 ` Alex Williamson
2010-10-23 21:01 ` [Qemu-devel] " Alex Williamson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.