* [Qemu-devel] [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-13 21:09 ` [Qemu-devel] " Michael S. Tsirkin
2010-11-16 21:33 ` Marcelo Tosatti
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 2/9] pci: Remove pci_enable_capability_support() Alex Williamson
` (7 subsequent siblings)
8 siblings, 2 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Make use of wmask, just like the rest of config space.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/pci.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index 92aaa85..4bc5882 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1175,13 +1175,15 @@ uint32_t pci_default_read_config(PCIDevice *d,
return pci_read_config(d, address, len);
}
-static void pci_write_config(PCIDevice *pci_dev,
- uint32_t address, uint32_t val, int len)
+static void pci_write_config_with_mask(PCIDevice *d, uint32_t addr,
+ uint32_t val, int l)
{
int i;
- for (i = 0; i < len; i++) {
- pci_dev->config[address + i] = val & 0xff;
- val >>= 8;
+ uint32_t config_size = pci_config_size(d);
+
+ for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
+ uint8_t wmask = d->wmask[addr + i];
+ d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
}
}
@@ -1202,23 +1204,19 @@ uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
void pci_default_cap_write_config(PCIDevice *pci_dev,
uint32_t address, uint32_t val, int len)
{
- pci_write_config(pci_dev, address, val, len);
+ pci_write_config_with_mask(pci_dev, address, val, len);
}
void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
{
- int i, was_irq_disabled = pci_irq_disabled(d);
- uint32_t config_size = pci_config_size(d);
+ int was_irq_disabled = pci_irq_disabled(d);
if (pci_access_cap_config(d, addr, l)) {
d->cap.config_write(d, addr, val, l);
return;
}
- for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
- uint8_t wmask = d->wmask[addr + i];
- d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
- }
+ pci_write_config_with_mask(d, addr, val, l);
#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
if (kvm_enabled() && kvm_irqchip_in_kernel() &&
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask Alex Williamson
@ 2010-11-13 21:09 ` Michael S. Tsirkin
2010-11-16 21:33 ` Marcelo Tosatti
1 sibling, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2010-11-13 21:09 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm
On Fri, Nov 12, 2010 at 10:46:10AM -0700, Alex Williamson wrote:
> Make use of wmask, just like the rest of config space.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Hmm, this will create conflicts on merging with qemu.kvm
which already has w1mask in same function.
I'll apply a patch splitting mask use to a separate function
to make life easier for you.
> ---
>
> hw/pci.c | 22 ++++++++++------------
> 1 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 92aaa85..4bc5882 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1175,13 +1175,15 @@ uint32_t pci_default_read_config(PCIDevice *d,
> return pci_read_config(d, address, len);
> }
>
> -static void pci_write_config(PCIDevice *pci_dev,
> - uint32_t address, uint32_t val, int len)
> +static void pci_write_config_with_mask(PCIDevice *d, uint32_t addr,
> + uint32_t val, int l)
> {
> int i;
> - for (i = 0; i < len; i++) {
> - pci_dev->config[address + i] = val & 0xff;
> - val >>= 8;
> + uint32_t config_size = pci_config_size(d);
> +
> + for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> + uint8_t wmask = d->wmask[addr + i];
> + d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> }
> }
>
> @@ -1202,23 +1204,19 @@ uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
> void pci_default_cap_write_config(PCIDevice *pci_dev,
> uint32_t address, uint32_t val, int len)
> {
> - pci_write_config(pci_dev, address, val, len);
> + pci_write_config_with_mask(pci_dev, address, val, len);
> }
>
> void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> {
> - int i, was_irq_disabled = pci_irq_disabled(d);
> - uint32_t config_size = pci_config_size(d);
> + int was_irq_disabled = pci_irq_disabled(d);
>
> if (pci_access_cap_config(d, addr, l)) {
> d->cap.config_write(d, addr, val, l);
> return;
> }
>
> - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> - uint8_t wmask = d->wmask[addr + i];
> - d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> - }
> + pci_write_config_with_mask(d, addr, val, l);
>
> #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
> if (kvm_enabled() && kvm_irqchip_in_kernel() &&
^ permalink raw reply [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask Alex Williamson
2010-11-13 21:09 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-11-16 21:33 ` Marcelo Tosatti
1 sibling, 0 replies; 14+ messages in thread
From: Marcelo Tosatti @ 2010-11-16 21:33 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm, mst
On Fri, Nov 12, 2010 at 10:46:10AM -0700, Alex Williamson wrote:
> Make use of wmask, just like the rest of config space.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> hw/pci.c | 22 ++++++++++------------
> 1 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 92aaa85..4bc5882 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1175,13 +1175,15 @@ uint32_t pci_default_read_config(PCIDevice *d,
> return pci_read_config(d, address, len);
> }
>
> -static void pci_write_config(PCIDevice *pci_dev,
> - uint32_t address, uint32_t val, int len)
> +static void pci_write_config_with_mask(PCIDevice *d, uint32_t addr,
> + uint32_t val, int l)
> {
> int i;
> - for (i = 0; i < len; i++) {
> - pci_dev->config[address + i] = val & 0xff;
> - val >>= 8;
> + uint32_t config_size = pci_config_size(d);
> +
> + for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> + uint8_t wmask = d->wmask[addr + i];
> + d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> }
> }
>
> @@ -1202,23 +1204,19 @@ uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
> void pci_default_cap_write_config(PCIDevice *pci_dev,
> uint32_t address, uint32_t val, int len)
> {
> - pci_write_config(pci_dev, address, val, len);
> + pci_write_config_with_mask(pci_dev, address, val, len);
> }
>
> void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> {
> - int i, was_irq_disabled = pci_irq_disabled(d);
> - uint32_t config_size = pci_config_size(d);
> + int was_irq_disabled = pci_irq_disabled(d);
>
> if (pci_access_cap_config(d, addr, l)) {
> d->cap.config_write(d, addr, val, l);
> return;
> }
>
> - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> - uint8_t wmask = d->wmask[addr + i];
> - d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> - }
> + pci_write_config_with_mask(d, addr, val, l);
Please do not introduce more differences between qemu-kvm and qemu
(use pci_write_config_with_mask only on qemu-kvm code).
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 2/9] pci: Remove pci_enable_capability_support()
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 3/9] device-assignment: Use PCI capabilities support Alex Williamson
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
This interface doesn't make much sense, adding a capability can
take care of everything, just provide a means to register
capability read/write handlers.
Device assignment does it's own thing, so requires a couple
ugly hacks that will be cleaned by subsequent patches.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 12 ++++++++---
hw/pci.c | 52 +++++++++++++++++++++---------------------------
hw/pci.h | 9 +++-----
3 files changed, 35 insertions(+), 38 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index bde231d..311c197 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1292,7 +1292,12 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
PCIRegion *pci_region = dev->real_device.regions;
int next_cap_pt = 0;
+ pci_dev->cap.supported = 1;
+ pci_dev->cap.start = PCI_CAPABILITY_CONFIG_DEFAULT_START_ADDR;
pci_dev->cap.length = 0;
+ pci_dev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+ pci_dev->config[PCI_CAPABILITY_LIST] = pci_dev->cap.start;
+
#ifdef KVM_CAP_IRQ_ROUTING
#ifdef KVM_CAP_DEVICE_MSI
/* Expose MSI capability
@@ -1468,9 +1473,10 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
dev->h_busnr = dev->host.bus;
dev->h_devfn = PCI_DEVFN(dev->host.dev, dev->host.func);
- if (pci_enable_capability_support(pci_dev, 0, NULL,
- assigned_device_pci_cap_write_config,
- assigned_device_pci_cap_init) < 0)
+ pci_register_capability_handlers(pci_dev, NULL,
+ assigned_device_pci_cap_write_config);
+
+ if (assigned_device_pci_cap_init(pci_dev) < 0)
goto out;
/* assign device to guest */
diff --git a/hw/pci.c b/hw/pci.c
index 4bc5882..607eb27 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -787,6 +787,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
config_write = pci_default_write_config;
pci_dev->config_read = config_read;
pci_dev->config_write = config_write;
+ pci_dev->cap.config_read = pci_default_cap_read_config;
+ pci_dev->cap.config_write = pci_default_cap_write_config;
bus->devices[devfn] = pci_dev;
pci_dev->irq = qemu_allocate_irqs(pci_set_irq, pci_dev, PCI_NUM_PINS);
pci_dev->version_id = 2; /* Current pci device vmstate version */
@@ -1894,35 +1896,21 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
return dev;
}
-int pci_enable_capability_support(PCIDevice *pci_dev,
- uint32_t config_start,
- PCICapConfigReadFunc *config_read,
- PCICapConfigWriteFunc *config_write,
- PCICapConfigInitFunc *config_init)
+void pci_register_capability_handlers(PCIDevice *pdev,
+ PCICapConfigReadFunc *config_read,
+ PCICapConfigWriteFunc *config_write)
{
- if (!pci_dev)
- return -ENODEV;
-
- pci_dev->config[0x06] |= 0x10; // status = capabilities
-
- if (config_start == 0)
- pci_dev->cap.start = PCI_CAPABILITY_CONFIG_DEFAULT_START_ADDR;
- else if (config_start >= 0x40 && config_start < 0xff)
- pci_dev->cap.start = config_start;
- else
- return -EINVAL;
+ if (config_read) {
+ pdev->cap.config_read = config_read;
+ } else {
+ pdev->cap.config_read = pci_default_cap_read_config;
+ }
- if (config_read)
- pci_dev->cap.config_read = config_read;
- else
- pci_dev->cap.config_read = pci_default_cap_read_config;
- if (config_write)
- pci_dev->cap.config_write = config_write;
- else
- pci_dev->cap.config_write = pci_default_cap_write_config;
- pci_dev->cap.supported = 1;
- pci_dev->config[PCI_CAPABILITY_LIST] = pci_dev->cap.start;
- return config_init(pci_dev);
+ if (config_write) {
+ pdev->cap.config_write = config_write;
+ } else {
+ pdev->cap.config_write = pci_default_cap_write_config;
+ }
}
PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
@@ -2046,12 +2034,16 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
config[PCI_CAP_LIST_ID] = cap_id;
config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
pdev->config[PCI_CAPABILITY_LIST] = offset;
- pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
memset(pdev->used + offset, 0xFF, size);
/* Make capability read-only by default */
memset(pdev->wmask + offset, 0, size);
/* Check capability by default */
memset(pdev->cmask + offset, 0xFF, size);
+
+ pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+ pdev->cap.supported = 1;
+ pdev->cap.start = pdev->cap.start ? MIN(pdev->cap.start, offset) : offset;
+
return offset;
}
@@ -2079,8 +2071,10 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
memset(pdev->cmask + offset, 0, size);
memset(pdev->used + offset, 0, size);
- if (!pdev->config[PCI_CAPABILITY_LIST])
+ if (!pdev->config[PCI_CAPABILITY_LIST]) {
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
+ pdev->cap.start = pdev->cap.length = 0;
+ }
}
/* Reserve space for capability at a known offset (to call after load). */
diff --git a/hw/pci.h b/hw/pci.h
index 334e928..0712e55 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -87,7 +87,6 @@ typedef void PCICapConfigWriteFunc(PCIDevice *pci_dev,
uint32_t address, uint32_t val, int len);
typedef uint32_t PCICapConfigReadFunc(PCIDevice *pci_dev,
uint32_t address, int len);
-typedef int PCICapConfigInitFunc(PCIDevice *pci_dev);
typedef struct PCIIORegion {
pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
@@ -228,11 +227,9 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr,
pcibus_t size, int type);
-int pci_enable_capability_support(PCIDevice *pci_dev,
- uint32_t config_start,
- PCICapConfigReadFunc *config_read,
- PCICapConfigWriteFunc *config_write,
- PCICapConfigInitFunc *config_init);
+void pci_register_capability_handlers(PCIDevice *pci_dev,
+ PCICapConfigReadFunc *config_read,
+ PCICapConfigWriteFunc *config_write);
int pci_map_irq(PCIDevice *pci_dev, int pin);
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 3/9] device-assignment: Use PCI capabilities support
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 1/9] pci: pci_default_cap_write_config ignores wmask Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 2/9] pci: Remove pci_enable_capability_support() Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 4/9] pci: Replace used bitmap with capability byte map Alex Williamson
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Convert to use common pci_add_capabilities() rather than creating
our own mess.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 112 +++++++++++++++++++++++++++---------------------
1 files changed, 63 insertions(+), 49 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 311c197..74cdd26 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -38,6 +38,7 @@
#include "device-assignment.h"
#include "loader.h"
#include "monitor.h"
+#include "range.h"
#include <pci/header.h>
/* From linux/ioport.h */
@@ -1075,17 +1076,17 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev, unsigned int ctrl_pos)
}
if (ctrl_byte & PCI_MSI_FLAGS_ENABLE) {
+ int pos = ctrl_pos - PCI_MSI_FLAGS;
assigned_dev->entry = calloc(1, sizeof(struct kvm_irq_routing_entry));
if (!assigned_dev->entry) {
perror("assigned_dev_update_msi: ");
return;
}
assigned_dev->entry->u.msi.address_lo =
- *(uint32_t *)(pci_dev->config + pci_dev->cap.start +
- PCI_MSI_ADDRESS_LO);
+ pci_get_long(pci_dev->config + pos + PCI_MSI_ADDRESS_LO);
assigned_dev->entry->u.msi.address_hi = 0;
- assigned_dev->entry->u.msi.data = *(uint16_t *)(pci_dev->config +
- pci_dev->cap.start + PCI_MSI_DATA_32);
+ assigned_dev->entry->u.msi.data =
+ pci_get_word(pci_dev->config + pos + PCI_MSI_DATA_32);
assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
r = kvm_get_irq_route_gsi();
if (r < 0) {
@@ -1123,10 +1124,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
struct kvm_assigned_msix_entry msix_entry;
void *va = adev->msix_table_page;
- if (adev->cap.available & ASSIGNED_DEVICE_CAP_MSI)
- pos = pci_dev->cap.start + PCI_CAPABILITY_CONFIG_MSI_LENGTH;
- else
- pos = pci_dev->cap.start;
+ pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSIX);
entries_max_nr = *(uint16_t *)(pci_dev->config + pos + 2);
entries_max_nr &= PCI_MSIX_TABSIZE;
@@ -1260,26 +1258,23 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev, uint32_t ad
uint32_t val, int len)
{
AssignedDevice *assigned_dev = container_of(pci_dev, AssignedDevice, dev);
- unsigned int pos = pci_dev->cap.start, ctrl_pos;
pci_default_cap_write_config(pci_dev, address, val, len);
#ifdef KVM_CAP_IRQ_ROUTING
#ifdef KVM_CAP_DEVICE_MSI
if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSI) {
- ctrl_pos = pos + PCI_MSI_FLAGS;
- if (address <= ctrl_pos && address + len > ctrl_pos)
- assigned_dev_update_msi(pci_dev, ctrl_pos);
- pos += PCI_CAPABILITY_CONFIG_MSI_LENGTH;
+ int pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSI);
+ if (ranges_overlap(address, len, pos + PCI_MSI_FLAGS, 1)) {
+ assigned_dev_update_msi(pci_dev, pos + PCI_MSI_FLAGS);
+ }
}
#endif
#ifdef KVM_CAP_DEVICE_MSIX
if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
- ctrl_pos = pos + 3;
- if (address <= ctrl_pos && address + len > ctrl_pos) {
- ctrl_pos--; /* control is word long */
- assigned_dev_update_msix(pci_dev, ctrl_pos);
+ int pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSIX);
+ if (ranges_overlap(address, len, pos + PCI_MSIX_FLAGS + 1, 1)) {
+ assigned_dev_update_msix(pci_dev, pos + PCI_MSIX_FLAGS);
}
- pos += PCI_CAPABILITY_CONFIG_MSIX_LENGTH;
}
#endif
#endif
@@ -1290,58 +1285,77 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
{
AssignedDevice *dev = container_of(pci_dev, AssignedDevice, dev);
PCIRegion *pci_region = dev->real_device.regions;
- int next_cap_pt = 0;
- pci_dev->cap.supported = 1;
- pci_dev->cap.start = PCI_CAPABILITY_CONFIG_DEFAULT_START_ADDR;
+ /* Clear initial capabilities pointer and status copied from hw */
+ pci_set_byte(pci_dev->config + PCI_CAPABILITY_LIST, 0);
+ pci_set_word(pci_dev->config + PCI_STATUS,
+ pci_get_word(pci_dev->config + PCI_STATUS) &
+ ~PCI_STATUS_CAP_LIST);
+
pci_dev->cap.length = 0;
- pci_dev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
- pci_dev->config[PCI_CAPABILITY_LIST] = pci_dev->cap.start;
#ifdef KVM_CAP_IRQ_ROUTING
#ifdef KVM_CAP_DEVICE_MSI
/* Expose MSI capability
* MSI capability is the 1st capability in capability config */
if (pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI)) {
+ int vpos, ppos;
+ uint16_t flags;
+
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
- memset(&pci_dev->config[pci_dev->cap.start + pci_dev->cap.length],
- 0, PCI_CAPABILITY_CONFIG_MSI_LENGTH);
- pci_dev->config[pci_dev->cap.start + pci_dev->cap.length] =
- PCI_CAP_ID_MSI;
+ vpos = pci_add_capability(pci_dev, PCI_CAP_ID_MSI,
+ PCI_CAPABILITY_CONFIG_MSI_LENGTH);
+
+ memset(pci_dev->config + vpos + PCI_CAP_FLAGS, 0,
+ PCI_CAPABILITY_CONFIG_MSI_LENGTH - PCI_CAP_FLAGS);
+
+ /* Only 32-bit/no-mask currently supported */
+ ppos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI);
+ flags = assigned_dev_pci_read_word(pci_dev, ppos + PCI_MSI_FLAGS);
+ flags &= PCI_MSI_FLAGS_QMASK;
+ pci_set_word(pci_dev->config + vpos + PCI_MSI_FLAGS, flags);
+
+ /* Set writable fields */
+ pci_set_word(pci_dev->wmask + vpos + PCI_MSI_FLAGS,
+ PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
+ pci_set_long(pci_dev->wmask + vpos + PCI_MSI_ADDRESS_LO, 0xfffffffc);
+ pci_set_long(pci_dev->wmask + vpos + PCI_MSI_DATA_32, 0xffff);
pci_dev->cap.length += PCI_CAPABILITY_CONFIG_MSI_LENGTH;
- next_cap_pt = 1;
}
#endif
#ifdef KVM_CAP_DEVICE_MSIX
/* Expose MSI-X capability */
if (pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX)) {
- int pos, entry_nr, bar_nr;
+ int vpos, ppos, entry_nr, bar_nr;
uint32_t msix_table_entry;
+
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
- memset(&pci_dev->config[pci_dev->cap.start + pci_dev->cap.length],
- 0, PCI_CAPABILITY_CONFIG_MSIX_LENGTH);
- pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX);
- entry_nr = assigned_dev_pci_read_word(pci_dev, pos + 2) &
- PCI_MSIX_TABSIZE;
- pci_dev->config[pci_dev->cap.start + pci_dev->cap.length] = 0x11;
- *(uint16_t *)(pci_dev->config + pci_dev->cap.start +
- pci_dev->cap.length + 2) = entry_nr;
+ vpos = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX,
+ PCI_CAPABILITY_CONFIG_MSIX_LENGTH);
+
+ memset(pci_dev->config + vpos + PCI_CAP_FLAGS, 0,
+ PCI_CAPABILITY_CONFIG_MSIX_LENGTH - PCI_CAP_FLAGS);
+
+ /* Only enable and function mask bits are writable */
+ pci_set_word(pci_dev->wmask + vpos + PCI_MSIX_FLAGS,
+ PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
+
+ ppos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX);
+
+ entry_nr = assigned_dev_pci_read_word(pci_dev, ppos + PCI_MSIX_FLAGS);
+ entry_nr &= PCI_MSIX_TABSIZE;
+ pci_set_word(pci_dev->config + vpos + PCI_MSIX_FLAGS, entry_nr);
+
msix_table_entry = assigned_dev_pci_read_long(pci_dev,
- pos + PCI_MSIX_TABLE);
- *(uint32_t *)(pci_dev->config + pci_dev->cap.start +
- pci_dev->cap.length + PCI_MSIX_TABLE) = msix_table_entry;
- *(uint32_t *)(pci_dev->config + pci_dev->cap.start +
- pci_dev->cap.length + PCI_MSIX_PBA) =
- assigned_dev_pci_read_long(pci_dev, pos + PCI_MSIX_PBA);
+ ppos + PCI_MSIX_TABLE);
+ pci_set_long(pci_dev->config + vpos + PCI_MSIX_TABLE, msix_table_entry);
+
+ pci_set_long(pci_dev->config + vpos + PCI_MSIX_PBA,
+ assigned_dev_pci_read_long(pci_dev, ppos + PCI_MSIX_PBA));
+
bar_nr = msix_table_entry & PCI_MSIX_BIR;
msix_table_entry &= ~PCI_MSIX_BIR;
dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry;
- if (next_cap_pt != 0) {
- pci_dev->config[pci_dev->cap.start + next_cap_pt] =
- pci_dev->cap.start + pci_dev->cap.length;
- next_cap_pt += PCI_CAPABILITY_CONFIG_MSI_LENGTH;
- } else
- next_cap_pt = 1;
pci_dev->cap.length += PCI_CAPABILITY_CONFIG_MSIX_LENGTH;
}
#endif
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 4/9] pci: Replace used bitmap with capability byte map
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (2 preceding siblings ...)
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 3/9] device-assignment: Use PCI capabilities support Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 5/9] pci: Remove cap.length, cap.start, cap.supported Alex Williamson
` (4 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Capabilities are allocated in bytes, so we can track both whether
a byte is used and by what capability in the same structure.
Remove pci_reserve_capability() as there are no users.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/pci.c | 16 +++++-----------
hw/pci.h | 6 ++----
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index 607eb27..80610b3 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -730,7 +730,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
pci_dev->config = qemu_mallocz(config_size);
pci_dev->cmask = qemu_mallocz(config_size);
pci_dev->wmask = qemu_mallocz(config_size);
- pci_dev->used = qemu_mallocz(config_size);
+ pci_dev->cap_map = qemu_mallocz(config_size);
}
static void pci_config_free(PCIDevice *pci_dev)
@@ -738,7 +738,7 @@ static void pci_config_free(PCIDevice *pci_dev)
qemu_free(pci_dev->config);
qemu_free(pci_dev->cmask);
qemu_free(pci_dev->wmask);
- qemu_free(pci_dev->used);
+ qemu_free(pci_dev->cap_map);
}
/* -1 for devfn means auto assign */
@@ -1929,7 +1929,7 @@ static int pci_find_space(PCIDevice *pdev, uint8_t size)
int offset = PCI_CONFIG_HEADER_SIZE;
int i;
for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
- if (pdev->used[i])
+ if (pdev->cap_map[i])
offset = i + 1;
else if (i - offset + 1 == size)
return offset;
@@ -2034,7 +2034,7 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
config[PCI_CAP_LIST_ID] = cap_id;
config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
pdev->config[PCI_CAPABILITY_LIST] = offset;
- memset(pdev->used + offset, 0xFF, size);
+ memset(pdev->cap_map + offset, cap_id, size);
/* Make capability read-only by default */
memset(pdev->wmask + offset, 0, size);
/* Check capability by default */
@@ -2069,7 +2069,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
memset(pdev->wmask + offset, 0xff, size);
/* Clear cmask as device-specific registers can't be checked */
memset(pdev->cmask + offset, 0, size);
- memset(pdev->used + offset, 0, size);
+ memset(pdev->cap_map + offset, 0, size);
if (!pdev->config[PCI_CAPABILITY_LIST]) {
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
@@ -2077,12 +2077,6 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
}
}
-/* Reserve space for capability at a known offset (to call after load). */
-void pci_reserve_capability(PCIDevice *pdev, uint8_t offset, uint8_t size)
-{
- memset(pdev->used + offset, 0xff, size);
-}
-
uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
{
return pci_find_capability_list(pdev, cap_id, NULL);
diff --git a/hw/pci.h b/hw/pci.h
index 0712e55..2265c70 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -151,8 +151,8 @@ struct PCIDevice {
/* Used to implement R/W bytes */
uint8_t *wmask;
- /* Used to allocate config space for capabilities. */
- uint8_t *used;
+ /* Used to allocate config space and track capabilities. */
+ uint8_t *cap_map;
/* the following fields are read only */
PCIBus *bus;
@@ -239,8 +239,6 @@ int pci_add_capability_at_offset(PCIDevice *pci_dev, uint8_t cap_id,
void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
-void pci_reserve_capability(PCIDevice *pci_dev, uint8_t offset, uint8_t size);
-
uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
uint32_t pci_default_read_config(PCIDevice *d,
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 5/9] pci: Remove cap.length, cap.start, cap.supported
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (3 preceding siblings ...)
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 4/9] pci: Replace used bitmap with capability byte map Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 6/9] device-assignment: Move PCI capabilities to match physical hardware Alex Williamson
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Capabilities aren't required to be contiguous, so cap.length never
really made much sense. Likewise, cap.start is mostly meaningless
too. Both of these are better served by the capability map. We
can also get rid of cap.supported, since it's really now unused
and redundant with flag in the status word anyway.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 4 ----
hw/pci.c | 8 +-------
hw/pci.h | 2 --
3 files changed, 1 insertions(+), 13 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 74cdd26..322fa9f 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1292,8 +1292,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
pci_get_word(pci_dev->config + PCI_STATUS) &
~PCI_STATUS_CAP_LIST);
- pci_dev->cap.length = 0;
-
#ifdef KVM_CAP_IRQ_ROUTING
#ifdef KVM_CAP_DEVICE_MSI
/* Expose MSI capability
@@ -1320,7 +1318,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
pci_set_long(pci_dev->wmask + vpos + PCI_MSI_ADDRESS_LO, 0xfffffffc);
pci_set_long(pci_dev->wmask + vpos + PCI_MSI_DATA_32, 0xffff);
- pci_dev->cap.length += PCI_CAPABILITY_CONFIG_MSI_LENGTH;
}
#endif
#ifdef KVM_CAP_DEVICE_MSIX
@@ -1356,7 +1353,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
bar_nr = msix_table_entry & PCI_MSIX_BIR;
msix_table_entry &= ~PCI_MSIX_BIR;
dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry;
- pci_dev->cap.length += PCI_CAPABILITY_CONFIG_MSIX_LENGTH;
}
#endif
#endif
diff --git a/hw/pci.c b/hw/pci.c
index 80610b3..a0a6126 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1191,10 +1191,7 @@ static void pci_write_config_with_mask(PCIDevice *d, uint32_t addr,
int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len)
{
- if (pci_dev->cap.supported && address >= pci_dev->cap.start &&
- (address + len) < pci_dev->cap.start + pci_dev->cap.length)
- return 1;
- return 0;
+ return pci_dev->cap_map[address];
}
uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
@@ -2041,8 +2038,6 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
memset(pdev->cmask + offset, 0xFF, size);
pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
- pdev->cap.supported = 1;
- pdev->cap.start = pdev->cap.start ? MIN(pdev->cap.start, offset) : offset;
return offset;
}
@@ -2073,7 +2068,6 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
if (!pdev->config[PCI_CAPABILITY_LIST]) {
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
- pdev->cap.start = pdev->cap.length = 0;
}
}
diff --git a/hw/pci.h b/hw/pci.h
index 2265c70..177008a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -208,8 +208,6 @@ struct PCIDevice {
/* Device capability configuration space */
struct {
- int supported;
- unsigned int start, length;
PCICapConfigReadFunc *config_read;
PCICapConfigWriteFunc *config_write;
} cap;
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 6/9] device-assignment: Move PCI capabilities to match physical hardware
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (4 preceding siblings ...)
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 5/9] pci: Remove cap.length, cap.start, cap.supported Alex Williamson
@ 2010-11-12 17:46 ` Alex Williamson
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 7/9] pci: Pass ID for capability read/write handlers Alex Williamson
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:46 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Now that common PCI code doesn't have a hangup on capabilities
being contiguous, move assigned device capabilities to match
their offset on physical hardware. This helps for drivers that
assume a capability configuration and don't bother searching.
We can also remove several calls to assigned_dev_pci_read_* because
we're overlaying the capability at the same location as the initial
copy we made of config space. We can therefore just use pci_get_*.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 67 +++++++++++++++---------------------------------
1 files changed, 21 insertions(+), 46 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 322fa9f..39f19be 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -366,16 +366,6 @@ static uint8_t assigned_dev_pci_read_byte(PCIDevice *d, int pos)
return (uint8_t)assigned_dev_pci_read(d, pos, 1);
}
-static uint16_t assigned_dev_pci_read_word(PCIDevice *d, int pos)
-{
- return (uint16_t)assigned_dev_pci_read(d, pos, 2);
-}
-
-static uint32_t assigned_dev_pci_read_long(PCIDevice *d, int pos)
-{
- return assigned_dev_pci_read(d, pos, 4);
-}
-
static uint8_t pci_find_cap_offset(PCIDevice *d, uint8_t cap)
{
int id;
@@ -1285,6 +1275,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
{
AssignedDevice *dev = container_of(pci_dev, AssignedDevice, dev);
PCIRegion *pci_region = dev->real_device.regions;
+ int pos;
/* Clear initial capabilities pointer and status copied from hw */
pci_set_byte(pci_dev->config + PCI_CAPABILITY_LIST, 0);
@@ -1296,60 +1287,44 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
#ifdef KVM_CAP_DEVICE_MSI
/* Expose MSI capability
* MSI capability is the 1st capability in capability config */
- if (pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI)) {
- int vpos, ppos;
- uint16_t flags;
-
+ if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI))) {
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
- vpos = pci_add_capability(pci_dev, PCI_CAP_ID_MSI,
- PCI_CAPABILITY_CONFIG_MSI_LENGTH);
-
- memset(pci_dev->config + vpos + PCI_CAP_FLAGS, 0,
- PCI_CAPABILITY_CONFIG_MSI_LENGTH - PCI_CAP_FLAGS);
+ pci_add_capability_at_offset(pci_dev, PCI_CAP_ID_MSI, pos,
+ PCI_CAPABILITY_CONFIG_MSI_LENGTH);
/* Only 32-bit/no-mask currently supported */
- ppos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI);
- flags = assigned_dev_pci_read_word(pci_dev, ppos + PCI_MSI_FLAGS);
- flags &= PCI_MSI_FLAGS_QMASK;
- pci_set_word(pci_dev->config + vpos + PCI_MSI_FLAGS, flags);
+ pci_set_word(pci_dev->config + pos + PCI_MSI_FLAGS,
+ pci_get_word(pci_dev->config + pos + PCI_MSI_FLAGS) &
+ PCI_MSI_FLAGS_QMASK);
+ pci_set_long(pci_dev->config + pos + PCI_MSI_ADDRESS_LO, 0);
+ pci_set_word(pci_dev->config + pos + PCI_MSI_DATA_32, 0);
/* Set writable fields */
- pci_set_word(pci_dev->wmask + vpos + PCI_MSI_FLAGS,
+ pci_set_word(pci_dev->wmask + pos + PCI_MSI_FLAGS,
PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
- pci_set_long(pci_dev->wmask + vpos + PCI_MSI_ADDRESS_LO, 0xfffffffc);
- pci_set_long(pci_dev->wmask + vpos + PCI_MSI_DATA_32, 0xffff);
+ pci_set_long(pci_dev->wmask + pos + PCI_MSI_ADDRESS_LO, 0xfffffffc);
+ pci_set_word(pci_dev->wmask + pos + PCI_MSI_DATA_32, 0xffff);
}
#endif
#ifdef KVM_CAP_DEVICE_MSIX
/* Expose MSI-X capability */
- if (pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX)) {
- int vpos, ppos, entry_nr, bar_nr;
+ if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX))) {
+ int bar_nr;
uint32_t msix_table_entry;
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
- vpos = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX,
- PCI_CAPABILITY_CONFIG_MSIX_LENGTH);
+ pci_add_capability_at_offset(pci_dev, PCI_CAP_ID_MSIX, pos,
+ PCI_CAPABILITY_CONFIG_MSIX_LENGTH);
- memset(pci_dev->config + vpos + PCI_CAP_FLAGS, 0,
- PCI_CAPABILITY_CONFIG_MSIX_LENGTH - PCI_CAP_FLAGS);
+ pci_set_word(pci_dev->config + pos + PCI_MSIX_FLAGS,
+ pci_get_word(pci_dev->config + pos + PCI_MSIX_FLAGS) &
+ PCI_MSIX_TABSIZE);
/* Only enable and function mask bits are writable */
- pci_set_word(pci_dev->wmask + vpos + PCI_MSIX_FLAGS,
+ pci_set_word(pci_dev->wmask + pos + PCI_MSIX_FLAGS,
PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
- ppos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX);
-
- entry_nr = assigned_dev_pci_read_word(pci_dev, ppos + PCI_MSIX_FLAGS);
- entry_nr &= PCI_MSIX_TABSIZE;
- pci_set_word(pci_dev->config + vpos + PCI_MSIX_FLAGS, entry_nr);
-
- msix_table_entry = assigned_dev_pci_read_long(pci_dev,
- ppos + PCI_MSIX_TABLE);
- pci_set_long(pci_dev->config + vpos + PCI_MSIX_TABLE, msix_table_entry);
-
- pci_set_long(pci_dev->config + vpos + PCI_MSIX_PBA,
- assigned_dev_pci_read_long(pci_dev, ppos + PCI_MSIX_PBA));
-
+ msix_table_entry = pci_get_long(pci_dev->config + pos + PCI_MSIX_TABLE);
bar_nr = msix_table_entry & PCI_MSIX_BIR;
msix_table_entry &= ~PCI_MSIX_BIR;
dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry;
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 7/9] pci: Pass ID for capability read/write handlers
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (5 preceding siblings ...)
2010-11-12 17:46 ` [Qemu-devel] [PATCH v2 6/9] device-assignment: Move PCI capabilities to match physical hardware Alex Williamson
@ 2010-11-12 17:47 ` Alex Williamson
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 8/9] pci: Remove capability read/write config handlers Alex Williamson
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 9/9] pci: Store capability offsets in PCIDevice Alex Williamson
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:47 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
Any handlers that actually want to interact with specific capabilities
are going to want to know the capability ID being accessed. With the
capability map, this is readily available, so we can save handlers the
trouble of figuring it out.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 36 ++++++++++++++++++++++--------------
hw/pci.c | 14 ++++++++------
hw/pci.h | 8 ++++----
3 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 39f19be..179c7dc 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1244,30 +1244,38 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev, unsigned int ctrl_pos)
#endif
#endif
-static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev, uint32_t address,
+static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
+ uint8_t cap_id,
+ uint32_t address,
uint32_t val, int len)
{
- AssignedDevice *assigned_dev = container_of(pci_dev, AssignedDevice, dev);
+ pci_default_cap_write_config(pci_dev, cap_id, address, val, len);
- pci_default_cap_write_config(pci_dev, address, val, len);
+ switch (cap_id) {
#ifdef KVM_CAP_IRQ_ROUTING
+ case PCI_CAP_ID_MSI:
#ifdef KVM_CAP_DEVICE_MSI
- if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSI) {
- int pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSI);
- if (ranges_overlap(address, len, pos + PCI_MSI_FLAGS, 1)) {
- assigned_dev_update_msi(pci_dev, pos + PCI_MSI_FLAGS);
+ {
+ uint8_t cap = pci_find_capability(pci_dev, cap_id);
+ if (ranges_overlap(address - cap, len, PCI_MSI_FLAGS, 1)) {
+ assigned_dev_update_msi(pci_dev, cap + PCI_MSI_FLAGS);
+ }
}
- }
#endif
+ break;
+
+ case PCI_CAP_ID_MSIX:
#ifdef KVM_CAP_DEVICE_MSIX
- if (assigned_dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
- int pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSIX);
- if (ranges_overlap(address, len, pos + PCI_MSIX_FLAGS + 1, 1)) {
- assigned_dev_update_msix(pci_dev, pos + PCI_MSIX_FLAGS);
- }
- }
+ {
+ uint8_t cap = pci_find_capability(pci_dev, cap_id);
+ if (ranges_overlap(address - cap, len, PCI_MSIX_FLAGS + 1, 1)) {
+ assigned_dev_update_msix(pci_dev, cap + PCI_MSIX_FLAGS);
+ }
+ }
#endif
+ break;
#endif
+ }
return;
}
diff --git a/hw/pci.c b/hw/pci.c
index a0a6126..337afc4 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1168,10 +1168,11 @@ static uint32_t pci_read_config(PCIDevice *d,
uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)
{
+ uint8_t cap_id;
assert(len == 1 || len == 2 || len == 4);
- if (pci_access_cap_config(d, address, len)) {
- return d->cap.config_read(d, address, len);
+ if ((cap_id = pci_access_cap_config(d, address, len))) {
+ return d->cap.config_read(d, cap_id, address, len);
}
return pci_read_config(d, address, len);
@@ -1194,13 +1195,13 @@ int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len)
return pci_dev->cap_map[address];
}
-uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
+uint32_t pci_default_cap_read_config(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, int len)
{
return pci_read_config(pci_dev, address, len);
}
-void pci_default_cap_write_config(PCIDevice *pci_dev,
+void pci_default_cap_write_config(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, uint32_t val, int len)
{
pci_write_config_with_mask(pci_dev, address, val, len);
@@ -1209,9 +1210,10 @@ void pci_default_cap_write_config(PCIDevice *pci_dev,
void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
{
int was_irq_disabled = pci_irq_disabled(d);
+ uint8_t cap_id;
- if (pci_access_cap_config(d, addr, l)) {
- d->cap.config_write(d, addr, val, l);
+ if ((cap_id = pci_access_cap_config(d, addr, l))) {
+ d->cap.config_write(d, cap_id, addr, val, l);
return;
}
diff --git a/hw/pci.h b/hw/pci.h
index 177008a..3f0b4e0 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -83,9 +83,9 @@ typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
pcibus_t addr, pcibus_t size, int type);
typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
-typedef void PCICapConfigWriteFunc(PCIDevice *pci_dev,
+typedef void PCICapConfigWriteFunc(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, uint32_t val, int len);
-typedef uint32_t PCICapConfigReadFunc(PCIDevice *pci_dev,
+typedef uint32_t PCICapConfigReadFunc(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, int len);
typedef struct PCIIORegion {
@@ -245,9 +245,9 @@ void pci_default_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len);
void pci_device_save(PCIDevice *s, QEMUFile *f);
int pci_device_load(PCIDevice *s, QEMUFile *f);
-uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
+uint32_t pci_default_cap_read_config(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, int len);
-void pci_default_cap_write_config(PCIDevice *pci_dev,
+void pci_default_cap_write_config(PCIDevice *pci_dev, uint8_t cap_id,
uint32_t address, uint32_t val, int len);
int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len);
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 8/9] pci: Remove capability read/write config handlers
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (6 preceding siblings ...)
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 7/9] pci: Pass ID for capability read/write handlers Alex Williamson
@ 2010-11-12 17:47 ` Alex Williamson
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 9/9] pci: Store capability offsets in PCIDevice Alex Williamson
8 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:47 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
These are just as easy to handle out of the main config read/write
handlers. Also expand cap_map to config_map so we can use it to
track all of config space.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/device-assignment.c | 22 +++++++++++-----
hw/pci.c | 66 ++++++++++++------------------------------------
hw/pci.h | 25 +++---------------
3 files changed, 35 insertions(+), 78 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 179c7dc..85fa50d 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -63,6 +63,11 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev);
static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
+static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
+ uint8_t cap_id,
+ uint32_t address,
+ uint32_t val, int len);
+
static uint32_t assigned_dev_ioport_rw(AssignedDevRegion *dev_region,
uint32_t addr, int len, uint32_t *val)
{
@@ -400,20 +405,25 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
{
int fd;
ssize_t ret;
+ uint8_t cap_id = pci_get_cap_id(d, address);
AssignedDevice *pci_dev = container_of(d, AssignedDevice, dev);
DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
((d->devfn >> 3) & 0x1F), (d->devfn & 0x7),
(uint16_t) address, val, len);
+ if (cap_id && cap_id != PCI_CAP_ID_BASIC) {
+ return assigned_device_pci_cap_write_config(d, cap_id, address,
+ val, len);
+ }
+
if (address == 0x4) {
pci_default_write_config(d, address, val, len);
/* Continue to program the card */
}
if ((address >= 0x10 && address <= 0x24) || address == 0x30 ||
- address == 0x34 || address == 0x3c || address == 0x3d ||
- pci_access_cap_config(d, address, len)) {
+ address == 0x34 || address == 0x3c || address == 0x3d) {
/* used for update-mappings (BAR emulation) */
pci_default_write_config(d, address, val, len);
return;
@@ -443,13 +453,14 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
{
uint32_t val = 0;
int fd;
+ uint8_t cap_id = pci_get_cap_id(d, address);
ssize_t ret;
AssignedDevice *pci_dev = container_of(d, AssignedDevice, dev);
if (address < 0x4 || (pci_dev->need_emulate_cmd && address == 0x4) ||
(address >= 0x10 && address <= 0x24) || address == 0x30 ||
address == 0x34 || address == 0x3c || address == 0x3d ||
- pci_access_cap_config(d, address, len)) {
+ (cap_id && cap_id != PCI_CAP_ID_BASIC)) {
val = pci_default_read_config(d, address, len);
DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
(d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
@@ -1249,7 +1260,7 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
uint32_t address,
uint32_t val, int len)
{
- pci_default_cap_write_config(pci_dev, cap_id, address, val, len);
+ pci_default_write_config(pci_dev, address, val, len);
switch (cap_id) {
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1466,9 +1477,6 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
dev->h_busnr = dev->host.bus;
dev->h_devfn = PCI_DEVFN(dev->host.dev, dev->host.func);
- pci_register_capability_handlers(pci_dev, NULL,
- assigned_device_pci_cap_write_config);
-
if (assigned_device_pci_cap_init(pci_dev) < 0)
goto out;
diff --git a/hw/pci.c b/hw/pci.c
index 337afc4..bc25be7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -730,7 +730,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
pci_dev->config = qemu_mallocz(config_size);
pci_dev->cmask = qemu_mallocz(config_size);
pci_dev->wmask = qemu_mallocz(config_size);
- pci_dev->cap_map = qemu_mallocz(config_size);
+ pci_dev->config_map = qemu_mallocz(config_size);
}
static void pci_config_free(PCIDevice *pci_dev)
@@ -738,7 +738,7 @@ static void pci_config_free(PCIDevice *pci_dev)
qemu_free(pci_dev->config);
qemu_free(pci_dev->cmask);
qemu_free(pci_dev->wmask);
- qemu_free(pci_dev->cap_map);
+ qemu_free(pci_dev->config_map);
}
/* -1 for devfn means auto assign */
@@ -767,6 +767,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
pci_dev->irq_state = 0;
pci_config_alloc(pci_dev);
+ memset(pci_dev->config_map, PCI_CAP_ID_BASIC, PCI_CONFIG_HEADER_SIZE);
if (!is_bridge) {
pci_set_default_subsystem_id(pci_dev);
@@ -787,8 +788,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
config_write = pci_default_write_config;
pci_dev->config_read = config_read;
pci_dev->config_write = config_write;
- pci_dev->cap.config_read = pci_default_cap_read_config;
- pci_dev->cap.config_write = pci_default_cap_write_config;
bus->devices[devfn] = pci_dev;
pci_dev->irq = qemu_allocate_irqs(pci_set_irq, pci_dev, PCI_NUM_PINS);
pci_dev->version_id = 2; /* Current pci device vmstate version */
@@ -1168,13 +1167,8 @@ static uint32_t pci_read_config(PCIDevice *d,
uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)
{
- uint8_t cap_id;
assert(len == 1 || len == 2 || len == 4);
- if ((cap_id = pci_access_cap_config(d, address, len))) {
- return d->cap.config_read(d, cap_id, address, len);
- }
-
return pci_read_config(d, address, len);
}
@@ -1190,32 +1184,14 @@ static void pci_write_config_with_mask(PCIDevice *d, uint32_t addr,
}
}
-int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len)
-{
- return pci_dev->cap_map[address];
-}
-
-uint32_t pci_default_cap_read_config(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, int len)
-{
- return pci_read_config(pci_dev, address, len);
-}
-
-void pci_default_cap_write_config(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, uint32_t val, int len)
+uint8_t pci_get_cap_id(PCIDevice *pci_dev, uint32_t addr)
{
- pci_write_config_with_mask(pci_dev, address, val, len);
+ return pci_dev->config_map[addr];
}
void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
{
int was_irq_disabled = pci_irq_disabled(d);
- uint8_t cap_id;
-
- if ((cap_id = pci_access_cap_config(d, addr, l))) {
- d->cap.config_write(d, cap_id, addr, val, l);
- return;
- }
pci_write_config_with_mask(d, addr, val, l);
@@ -1895,23 +1871,6 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
return dev;
}
-void pci_register_capability_handlers(PCIDevice *pdev,
- PCICapConfigReadFunc *config_read,
- PCICapConfigWriteFunc *config_write)
-{
- if (config_read) {
- pdev->cap.config_read = config_read;
- } else {
- pdev->cap.config_read = pci_default_cap_read_config;
- }
-
- if (config_write) {
- pdev->cap.config_write = config_write;
- } else {
- pdev->cap.config_write = pci_default_cap_write_config;
- }
-}
-
PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
{
return pci_create_multifunction(bus, devfn, false, name);
@@ -1928,7 +1887,7 @@ static int pci_find_space(PCIDevice *pdev, uint8_t size)
int offset = PCI_CONFIG_HEADER_SIZE;
int i;
for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
- if (pdev->cap_map[i])
+ if (pdev->config_map[i])
offset = i + 1;
else if (i - offset + 1 == size)
return offset;
@@ -2029,11 +1988,18 @@ static void pci_del_option_rom(PCIDevice *pdev)
int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size)
{
- uint8_t *config = pdev->config + offset;
+ uint8_t i, *config = pdev->config + offset;
+
+ for (i = 0; i < size; i++) {
+ if (pdev->config_map[offset + i]) {
+ return -EFAULT;
+ }
+ }
+
config[PCI_CAP_LIST_ID] = cap_id;
config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
pdev->config[PCI_CAPABILITY_LIST] = offset;
- memset(pdev->cap_map + offset, cap_id, size);
+ memset(pdev->config_map + offset, cap_id, size);
/* Make capability read-only by default */
memset(pdev->wmask + offset, 0, size);
/* Check capability by default */
@@ -2066,7 +2032,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
memset(pdev->wmask + offset, 0xff, size);
/* Clear cmask as device-specific registers can't be checked */
memset(pdev->cmask + offset, 0, size);
- memset(pdev->cap_map + offset, 0, size);
+ memset(pdev->config_map + offset, 0, size);
if (!pdev->config[PCI_CAPABILITY_LIST]) {
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
diff --git a/hw/pci.h b/hw/pci.h
index 3f0b4e0..cea1c3a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -83,11 +83,6 @@ typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
pcibus_t addr, pcibus_t size, int type);
typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
-typedef void PCICapConfigWriteFunc(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, uint32_t val, int len);
-typedef uint32_t PCICapConfigReadFunc(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, int len);
-
typedef struct PCIIORegion {
pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
#define PCI_BAR_UNMAPPED (~(pcibus_t)0)
@@ -114,6 +109,8 @@ typedef struct PCIIORegion {
#define PCI_NUM_PINS 4 /* A-D */
+#define PCI_CAP_ID_BASIC 0xff
+
/* Bits in cap_present field. */
enum {
QEMU_PCI_CAP_MSIX = 0x1,
@@ -152,7 +149,7 @@ struct PCIDevice {
uint8_t *wmask;
/* Used to allocate config space and track capabilities. */
- uint8_t *cap_map;
+ uint8_t *config_map;
/* the following fields are read only */
PCIBus *bus;
@@ -205,12 +202,6 @@ struct PCIDevice {
struct kvm_msix_message *msix_irq_entries;
msix_mask_notifier_func msix_mask_notifier;
-
- /* Device capability configuration space */
- struct {
- PCICapConfigReadFunc *config_read;
- PCICapConfigWriteFunc *config_write;
- } cap;
};
PCIDevice *pci_register_device(PCIBus *bus, const char *name,
@@ -225,10 +216,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr,
pcibus_t size, int type);
-void pci_register_capability_handlers(PCIDevice *pci_dev,
- PCICapConfigReadFunc *config_read,
- PCICapConfigWriteFunc *config_write);
-
int pci_map_irq(PCIDevice *pci_dev, int pin);
int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
@@ -245,11 +232,7 @@ void pci_default_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len);
void pci_device_save(PCIDevice *s, QEMUFile *f);
int pci_device_load(PCIDevice *s, QEMUFile *f);
-uint32_t pci_default_cap_read_config(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, int len);
-void pci_default_cap_write_config(PCIDevice *pci_dev, uint8_t cap_id,
- uint32_t address, uint32_t val, int len);
-int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len);
+uint8_t pci_get_cap_id(PCIDevice *pci_dev, uint32_t addr);
typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 9/9] pci: Store capability offsets in PCIDevice
2010-11-12 17:45 [Qemu-devel] [PATCH v2 0/9] PCI capability and device assignment improvements Alex Williamson
` (7 preceding siblings ...)
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 8/9] pci: Remove capability read/write config handlers Alex Williamson
@ 2010-11-12 17:47 ` Alex Williamson
2010-11-13 21:05 ` [Qemu-devel] " Michael S. Tsirkin
8 siblings, 1 reply; 14+ messages in thread
From: Alex Williamson @ 2010-11-12 17:47 UTC (permalink / raw)
To: kvm, mst; +Cc: chrisw, alex.williamson, qemu-devel
This not only makes pci_find_capability a directly lookup, but also
allows us to better track added capabilities and avoids the proliferation
of random additional capability offset markers.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/msix.c | 15 +++++++--------
hw/pci.c | 20 ++++++++++++++++++--
hw/pci.h | 5 +++--
3 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index b98b34a..060f27b 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -204,7 +204,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
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 |
MSIX_MASKALL_MASK;
@@ -253,7 +252,8 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
static int msix_function_masked(PCIDevice *dev)
{
- return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
+ return dev->config[dev->caps[PCI_CAP_ID_MSIX] +
+ MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
}
static int msix_is_masked(PCIDevice *dev, int vector)
@@ -275,7 +275,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
void msix_write_config(PCIDevice *dev, uint32_t addr,
uint32_t val, int len)
{
- unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
+ unsigned enable_pos = dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET;
int vector;
if (!range_covers_byte(addr, len, enable_pos)) {
@@ -334,7 +334,7 @@ static CPUReadMemoryFunc * const msix_mmio_read[] = {
void msix_mmio_map(PCIDevice *d, int region_num,
pcibus_t addr, pcibus_t size, int type)
{
- uint8_t *config = d->config + d->msix_cap;
+ uint8_t *config = d->config + d->caps[PCI_CAP_ID_MSIX];
uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
/* TODO: for assigned devices, we'll want to make it possible to map
@@ -437,7 +437,6 @@ int msix_uninit(PCIDevice *dev)
if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
return 0;
pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
- dev->msix_cap = 0;
msix_free_irq_entries(dev);
dev->msix_entries_nr = 0;
cpu_unregister_io_memory(dev->msix_mmio_index);
@@ -493,7 +492,7 @@ int msix_present(PCIDevice *dev)
int msix_enabled(PCIDevice *dev)
{
return (dev->cap_present & QEMU_PCI_CAP_MSIX) &&
- (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
+ (dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &
MSIX_ENABLE_MASK);
}
@@ -534,8 +533,8 @@ void msix_reset(PCIDevice *dev)
if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
return;
msix_free_irq_entries(dev);
- dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
- ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
+ dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &=
+ ~dev->wmask[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET];
memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
msix_mask_all(dev, dev->msix_entries_nr);
}
diff --git a/hw/pci.c b/hw/pci.c
index bc25be7..773afa5 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1990,15 +1990,24 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
{
uint8_t i, *config = pdev->config + offset;
+ /* Check overlap with existing capabilities, valid cap, already added */
for (i = 0; i < size; i++) {
if (pdev->config_map[offset + i]) {
return -EFAULT;
}
}
+ if (!cap_id || cap_id > PCI_CAP_ID_MAX) {
+ return -EINVAL;
+ }
+
+ if (pdev->caps[cap_id]) {
+ return -EFAULT;
+ }
+
config[PCI_CAP_LIST_ID] = cap_id;
config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
- pdev->config[PCI_CAPABILITY_LIST] = offset;
+ pdev->caps[cap_id] = pdev->config[PCI_CAPABILITY_LIST] = offset;
memset(pdev->config_map + offset, cap_id, size);
/* Make capability read-only by default */
memset(pdev->wmask + offset, 0, size);
@@ -2033,6 +2042,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
/* Clear cmask as device-specific registers can't be checked */
memset(pdev->cmask + offset, 0, size);
memset(pdev->config_map + offset, 0, size);
+ pdev->caps[cap_id] = 0;
if (!pdev->config[PCI_CAPABILITY_LIST]) {
pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
@@ -2041,7 +2051,13 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
{
- return pci_find_capability_list(pdev, cap_id, NULL);
+ if (cap_id == PCI_CAP_ID_BASIC) {
+ return 0;
+ }
+ if (cap_id && cap_id <= PCI_CAP_ID_MAX) {
+ return pdev->caps[cap_id];
+ }
+ return 0xff;
}
static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
diff --git a/hw/pci.h b/hw/pci.h
index cea1c3a..b4c19ba 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -110,6 +110,7 @@ typedef struct PCIIORegion {
#define PCI_NUM_PINS 4 /* A-D */
#define PCI_CAP_ID_BASIC 0xff
+#define PCI_CAP_ID_MAX PCI_CAP_ID_AF
/* Bits in cap_present field. */
enum {
@@ -170,8 +171,8 @@ struct PCIDevice {
/* Capability bits */
uint32_t cap_present;
- /* Offset of MSI-X capability in config space */
- uint8_t msix_cap;
+ /* Offset capabilities in config space */
+ uint8_t caps[PCI_CAP_ID_MAX + 1];
/* MSI-X entries */
int msix_entries_nr;
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [PATCH v2 9/9] pci: Store capability offsets in PCIDevice
2010-11-12 17:47 ` [Qemu-devel] [PATCH v2 9/9] pci: Store capability offsets in PCIDevice Alex Williamson
@ 2010-11-13 21:05 ` Michael S. Tsirkin
2010-11-15 3:49 ` Alex Williamson
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2010-11-13 21:05 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm
On Fri, Nov 12, 2010 at 10:47:21AM -0700, Alex Williamson wrote:
> This not only makes pci_find_capability a directly lookup, but also
> allows us to better track added capabilities and avoids the proliferation
> of random additional capability offset markers.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
There shouldn't be any need to store offsets
separately as find_capability gives you the value,
and duplicating same data in two places is bad
as we need to keep them in sync now.
We track offset to msi and msix capabilities as an optimization:
because they are used on data path. I can't see why would
we need to optimize any other capability like this.
> ---
>
> hw/msix.c | 15 +++++++--------
> hw/pci.c | 20 ++++++++++++++++++--
> hw/pci.h | 5 +++--
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index b98b34a..060f27b 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -204,7 +204,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> 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 |
> MSIX_MASKALL_MASK;
> @@ -253,7 +252,8 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>
> static int msix_function_masked(PCIDevice *dev)
> {
> - return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> + return dev->config[dev->caps[PCI_CAP_ID_MSIX] +
> + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> }
>
> static int msix_is_masked(PCIDevice *dev, int vector)
> @@ -275,7 +275,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
> void msix_write_config(PCIDevice *dev, uint32_t addr,
> uint32_t val, int len)
> {
> - unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> + unsigned enable_pos = dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET;
> int vector;
>
> if (!range_covers_byte(addr, len, enable_pos)) {
> @@ -334,7 +334,7 @@ static CPUReadMemoryFunc * const msix_mmio_read[] = {
> void msix_mmio_map(PCIDevice *d, int region_num,
> pcibus_t addr, pcibus_t size, int type)
> {
> - uint8_t *config = d->config + d->msix_cap;
> + uint8_t *config = d->config + d->caps[PCI_CAP_ID_MSIX];
> uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> /* TODO: for assigned devices, we'll want to make it possible to map
> @@ -437,7 +437,6 @@ int msix_uninit(PCIDevice *dev)
> if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
> return 0;
> pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> - dev->msix_cap = 0;
> msix_free_irq_entries(dev);
> dev->msix_entries_nr = 0;
> cpu_unregister_io_memory(dev->msix_mmio_index);
> @@ -493,7 +492,7 @@ int msix_present(PCIDevice *dev)
> int msix_enabled(PCIDevice *dev)
> {
> return (dev->cap_present & QEMU_PCI_CAP_MSIX) &&
> - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> + (dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &
> MSIX_ENABLE_MASK);
> }
>
> @@ -534,8 +533,8 @@ void msix_reset(PCIDevice *dev)
> if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
> return;
> msix_free_irq_entries(dev);
> - dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> - ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> + dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &=
> + ~dev->wmask[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET];
> memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
> msix_mask_all(dev, dev->msix_entries_nr);
> }
> diff --git a/hw/pci.c b/hw/pci.c
> index bc25be7..773afa5 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1990,15 +1990,24 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
> {
> uint8_t i, *config = pdev->config + offset;
>
> + /* Check overlap with existing capabilities, valid cap, already added */
> for (i = 0; i < size; i++) {
> if (pdev->config_map[offset + i]) {
> return -EFAULT;
> }
> }
>
> + if (!cap_id || cap_id > PCI_CAP_ID_MAX) {
> + return -EINVAL;
> + }
> +
> + if (pdev->caps[cap_id]) {
> + return -EFAULT;
> + }
> +
> config[PCI_CAP_LIST_ID] = cap_id;
> config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> - pdev->config[PCI_CAPABILITY_LIST] = offset;
> + pdev->caps[cap_id] = pdev->config[PCI_CAPABILITY_LIST] = offset;
> memset(pdev->config_map + offset, cap_id, size);
> /* Make capability read-only by default */
> memset(pdev->wmask + offset, 0, size);
> @@ -2033,6 +2042,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
> /* Clear cmask as device-specific registers can't be checked */
> memset(pdev->cmask + offset, 0, size);
> memset(pdev->config_map + offset, 0, size);
> + pdev->caps[cap_id] = 0;
>
> if (!pdev->config[PCI_CAPABILITY_LIST]) {
> pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> @@ -2041,7 +2051,13 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
>
> uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
> {
> - return pci_find_capability_list(pdev, cap_id, NULL);
> + if (cap_id == PCI_CAP_ID_BASIC) {
> + return 0;
> + }
> + if (cap_id && cap_id <= PCI_CAP_ID_MAX) {
> + return pdev->caps[cap_id];
> + }
> + return 0xff;
> }
>
> static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> diff --git a/hw/pci.h b/hw/pci.h
> index cea1c3a..b4c19ba 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -110,6 +110,7 @@ typedef struct PCIIORegion {
> #define PCI_NUM_PINS 4 /* A-D */
>
> #define PCI_CAP_ID_BASIC 0xff
> +#define PCI_CAP_ID_MAX PCI_CAP_ID_AF
>
> /* Bits in cap_present field. */
> enum {
> @@ -170,8 +171,8 @@ struct PCIDevice {
> /* Capability bits */
> uint32_t cap_present;
>
> - /* Offset of MSI-X capability in config space */
> - uint8_t msix_cap;
> + /* Offset capabilities in config space */
> + uint8_t caps[PCI_CAP_ID_MAX + 1];
>
> /* MSI-X entries */
> int msix_entries_nr;
^ permalink raw reply [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [PATCH v2 9/9] pci: Store capability offsets in PCIDevice
2010-11-13 21:05 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-11-15 3:49 ` Alex Williamson
0 siblings, 0 replies; 14+ messages in thread
From: Alex Williamson @ 2010-11-15 3:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: chrisw, qemu-devel, kvm
On Sat, 2010-11-13 at 23:05 +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 12, 2010 at 10:47:21AM -0700, Alex Williamson wrote:
> > This not only makes pci_find_capability a directly lookup, but also
> > allows us to better track added capabilities and avoids the proliferation
> > of random additional capability offset markers.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>
> There shouldn't be any need to store offsets
> separately as find_capability gives you the value,
> and duplicating same data in two places is bad
> as we need to keep them in sync now.
>
> We track offset to msi and msix capabilities as an optimization:
> because they are used on data path. I can't see why would
> we need to optimize any other capability like this.
That's what I figured. I'm not going to fight for this one, but I do
like the consistency of accessing all capabilities directly instead of
having a few that are more equal than the rest. Can the benefit of
having msix_cap or msi_cap even be measured? In my casual observations
with device assignment, those registers don't get touched enough to
warrant a special case either. We've got a lot bigger problems if we
can't keep a couple tables in sync between an add and delete call.
Thanks,
Alex
> > ---
> >
> > hw/msix.c | 15 +++++++--------
> > hw/pci.c | 20 ++++++++++++++++++--
> > hw/pci.h | 5 +++--
> > 3 files changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b98b34a..060f27b 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -204,7 +204,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > 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 |
> > MSIX_MASKALL_MASK;
> > @@ -253,7 +252,8 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >
> > static int msix_function_masked(PCIDevice *dev)
> > {
> > - return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> > + return dev->config[dev->caps[PCI_CAP_ID_MSIX] +
> > + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> > }
> >
> > static int msix_is_masked(PCIDevice *dev, int vector)
> > @@ -275,7 +275,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector)
> > void msix_write_config(PCIDevice *dev, uint32_t addr,
> > uint32_t val, int len)
> > {
> > - unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> > + unsigned enable_pos = dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET;
> > int vector;
> >
> > if (!range_covers_byte(addr, len, enable_pos)) {
> > @@ -334,7 +334,7 @@ static CPUReadMemoryFunc * const msix_mmio_read[] = {
> > void msix_mmio_map(PCIDevice *d, int region_num,
> > pcibus_t addr, pcibus_t size, int type)
> > {
> > - uint8_t *config = d->config + d->msix_cap;
> > + uint8_t *config = d->config + d->caps[PCI_CAP_ID_MSIX];
> > uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> > uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> > /* TODO: for assigned devices, we'll want to make it possible to map
> > @@ -437,7 +437,6 @@ int msix_uninit(PCIDevice *dev)
> > if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
> > return 0;
> > pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > - dev->msix_cap = 0;
> > msix_free_irq_entries(dev);
> > dev->msix_entries_nr = 0;
> > cpu_unregister_io_memory(dev->msix_mmio_index);
> > @@ -493,7 +492,7 @@ int msix_present(PCIDevice *dev)
> > int msix_enabled(PCIDevice *dev)
> > {
> > return (dev->cap_present & QEMU_PCI_CAP_MSIX) &&
> > - (dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > + (dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &
> > MSIX_ENABLE_MASK);
> > }
> >
> > @@ -534,8 +533,8 @@ void msix_reset(PCIDevice *dev)
> > if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
> > return;
> > msix_free_irq_entries(dev);
> > - dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> > - ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> > + dev->config[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET] &=
> > + ~dev->wmask[dev->caps[PCI_CAP_ID_MSIX] + MSIX_CONTROL_OFFSET];
> > memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
> > msix_mask_all(dev, dev->msix_entries_nr);
> > }
> > diff --git a/hw/pci.c b/hw/pci.c
> > index bc25be7..773afa5 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1990,15 +1990,24 @@ int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
> > {
> > uint8_t i, *config = pdev->config + offset;
> >
> > + /* Check overlap with existing capabilities, valid cap, already added */
> > for (i = 0; i < size; i++) {
> > if (pdev->config_map[offset + i]) {
> > return -EFAULT;
> > }
> > }
> >
> > + if (!cap_id || cap_id > PCI_CAP_ID_MAX) {
> > + return -EINVAL;
> > + }
> > +
> > + if (pdev->caps[cap_id]) {
> > + return -EFAULT;
> > + }
> > +
> > config[PCI_CAP_LIST_ID] = cap_id;
> > config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> > - pdev->config[PCI_CAPABILITY_LIST] = offset;
> > + pdev->caps[cap_id] = pdev->config[PCI_CAPABILITY_LIST] = offset;
> > memset(pdev->config_map + offset, cap_id, size);
> > /* Make capability read-only by default */
> > memset(pdev->wmask + offset, 0, size);
> > @@ -2033,6 +2042,7 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
> > /* Clear cmask as device-specific registers can't be checked */
> > memset(pdev->cmask + offset, 0, size);
> > memset(pdev->config_map + offset, 0, size);
> > + pdev->caps[cap_id] = 0;
> >
> > if (!pdev->config[PCI_CAPABILITY_LIST]) {
> > pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> > @@ -2041,7 +2051,13 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
> >
> > uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
> > {
> > - return pci_find_capability_list(pdev, cap_id, NULL);
> > + if (cap_id == PCI_CAP_ID_BASIC) {
> > + return 0;
> > + }
> > + if (cap_id && cap_id <= PCI_CAP_ID_MAX) {
> > + return pdev->caps[cap_id];
> > + }
> > + return 0xff;
> > }
> >
> > static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> > diff --git a/hw/pci.h b/hw/pci.h
> > index cea1c3a..b4c19ba 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -110,6 +110,7 @@ typedef struct PCIIORegion {
> > #define PCI_NUM_PINS 4 /* A-D */
> >
> > #define PCI_CAP_ID_BASIC 0xff
> > +#define PCI_CAP_ID_MAX PCI_CAP_ID_AF
> >
> > /* Bits in cap_present field. */
> > enum {
> > @@ -170,8 +171,8 @@ struct PCIDevice {
> > /* Capability bits */
> > uint32_t cap_present;
> >
> > - /* Offset of MSI-X capability in config space */
> > - uint8_t msix_cap;
> > + /* Offset capabilities in config space */
> > + uint8_t caps[PCI_CAP_ID_MAX + 1];
> >
> > /* MSI-X entries */
> > int msix_entries_nr;
^ permalink raw reply [flat|nested] 14+ messages in thread