* [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2
@ 2009-06-02 6:42 Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 1/7] vmware_vga: clean up Isaku Yamahata
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
This patch series are for cleaning up pci bus/pci configuration
space implementation and pci bridge emulation.
I think patch 1-6 are ready to commit.
But the patch 7 for pci-to-pci bridge hasn't been finished.
Please comment on pci bridge emulation and multiple pci bus support.
[PATCH 0/7] RFC: pci bridge clean up and multiple pci bus support v2
[PATCH 1/7] vmware_vga: clean up
[PATCH 2/7] qemu: make default_write_config use mask table
[PATCH 3/7] pci: pci_default_config_write() clean up.
[PATCH 4/7] pci/config: convert pci configuration space handler to use callback.
[PATCH 5/7] pci: PCIBus clean up.
[PATCH 6/7] pci/brdige qdevfy.
[PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
Changes from v1
- many clean up patches
- qdevfied
Isaku Yamahata (6):
vmware_vga: clean up
pci: pci_default_config_write() clean up.
pci/config: convert pci configuration space handler to use callback.
pci: PCIBus clean up.
pci/brdige qdevfy.
pci bus: preliminary for multi pci bus support.
Michael S. Tsirkin (1):
qemu: make default_write_config use mask table
Makefile.target | 2 +-
hw/acpi.c | 11 +-
hw/apb_pci.c | 12 +-
hw/cirrus_vga.c | 17 ++-
hw/gt64xxx.c | 13 +-
hw/pc.c | 7 +
hw/pci-hotplug.c | 4 +-
hw/pci.c | 626 +++++++++++++++++++++++++++++++++++++----------------
hw/pci.h | 128 +++++++++++-
hw/pci_bridge.c | 232 ++++++++++++++++++++
hw/pci_bridge.h | 35 +++
hw/pci_ids.h | 3 +
hw/piix_pci.c | 18 +-
hw/vga.c | 10 +-
hw/vmware_vga.c | 2 +-
hw/wdt_i6300esb.c | 66 +++----
qemu-options.hx | 10 +
vl.c | 5 +
18 files changed, 932 insertions(+), 269 deletions(-)
create mode 100644 hw/pci_bridge.c
create mode 100644 hw/pci_bridge.h
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 1/7] vmware_vga: clean up
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-10 15:08 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 2/7] qemu: make default_write_config use mask table Isaku Yamahata
` (5 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
use NULL instead of 0 for pci_register_device() argument
for consistency. Any other caller uses NULL.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/vmware_vga.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 79da1ff..ee4f10a 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1217,7 +1217,7 @@ void pci_vmsvga_init(PCIBus *bus)
/* Setup PCI configuration */
s = (struct pci_vmsvga_state_s *)
pci_register_device(bus, "QEMUware SVGA",
- sizeof(struct pci_vmsvga_state_s), -1, 0, 0);
+ sizeof(struct pci_vmsvga_state_s), -1, NULL, NULL);
pci_config_set_vendor_id(s->card.config, PCI_VENDOR_ID_VMWARE);
pci_config_set_device_id(s->card.config, SVGA_PCI_DEVICE_ID);
s->card.config[PCI_COMMAND] = 0x07; /* I/O + Memory */
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 2/7] qemu: make default_write_config use mask table
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 1/7] vmware_vga: clean up Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 3/7] pci: pci_default_config_write() clean up Isaku Yamahata
` (4 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
From: Michael S. Tsirkin <mst@redhat.com>
Change much of hw/pci to use symbolic constants and a table-driven
design: add a mask table with writable bits set and readonly bits unset.
Detect change by comparing original and new registers.
As a result, writing a single byte in BAR registers now works as it
should. Writing to upper limit registers in the bridge also works as it
should. Writes to BAR registers trigger mapping update. Code is also
shorter.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Changelog since v1
- simplify the code some more
- use PCI_CONFIG_SPACE_SIZE instead of 0x100
- only trigger pci update when IO/MEM bits in command register have changed
---
hw/pci.c | 145 ++++++++++++-------------------------------------------------
hw/pci.h | 18 +++++++-
2 files changed, 46 insertions(+), 117 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index 0ab5b94..1e70e46 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -239,6 +239,17 @@ int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp)
return pci_parse_devaddr(devaddr, domp, busp, slotp);
}
+static void pci_init_mask(PCIDevice *dev)
+{
+ int i;
+ dev->mask[PCI_CACHE_LINE_SIZE] = 0xff;
+ dev->mask[PCI_INTERRUPT_LINE] = 0xff;
+ dev->mask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
+ | PCI_COMMAND_MASTER;
+ for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+ dev->mask[i] = 0xff;
+}
+
/* -1 for devfn means auto assign */
static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
const char *name, int devfn,
@@ -261,6 +272,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
pci_set_default_subsystem_id(pci_dev);
+ pci_init_mask(pci_dev);
if (!config_read)
config_read = pci_default_read_config;
@@ -334,6 +346,7 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
{
PCIIORegion *r;
uint32_t addr;
+ uint32_t mask;
if ((unsigned int)region_num >= PCI_NUM_REGIONS)
return;
@@ -349,12 +362,17 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
r->size = size;
r->type = type;
r->map_func = map_func;
+
+ mask = ~(size - 1);
if (region_num == PCI_ROM_SLOT) {
addr = 0x30;
+ /* ROM enable bit is writeable */
+ mask |= 1;
} else {
addr = 0x10 + region_num * 4;
}
*(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
+ *(uint32_t *)(pci_dev->mask + addr) = cpu_to_le32(mask);
}
static void pci_update_mappings(PCIDevice *d)
@@ -463,118 +481,21 @@ uint32_t pci_default_read_config(PCIDevice *d,
return val;
}
-void pci_default_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
+void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
{
- int can_write, i;
- uint32_t end, addr;
-
- if (len == 4 && ((address >= 0x10 && address < 0x10 + 4 * 6) ||
- (address >= 0x30 && address < 0x34))) {
- PCIIORegion *r;
- int reg;
+ uint8_t orig[PCI_CONFIG_SPACE_SIZE];
+ int i;
- if ( address >= 0x30 ) {
- reg = PCI_ROM_SLOT;
- }else{
- reg = (address - 0x10) >> 2;
- }
- r = &d->io_regions[reg];
- if (r->size == 0)
- goto default_config;
- /* compute the stored value */
- if (reg == PCI_ROM_SLOT) {
- /* keep ROM enable bit */
- val &= (~(r->size - 1)) | 1;
- } else {
- val &= ~(r->size - 1);
- val |= r->type;
- }
- *(uint32_t *)(d->config + address) = cpu_to_le32(val);
- pci_update_mappings(d);
- return;
- }
- default_config:
/* not efficient, but simple */
- addr = address;
- for(i = 0; i < len; i++) {
- /* default read/write accesses */
- switch(d->config[0x0e]) {
- case 0x00:
- case 0x80:
- switch(addr) {
- case 0x00:
- case 0x01:
- case 0x02:
- case 0x03:
- case 0x06:
- case 0x07:
- case 0x08:
- case 0x09:
- case 0x0a:
- case 0x0b:
- case 0x0e:
- case 0x10 ... 0x27: /* base */
- case 0x2c ... 0x2f: /* read-only subsystem ID & vendor ID */
- case 0x30 ... 0x33: /* rom */
- case 0x3d:
- can_write = 0;
- break;
- default:
- can_write = 1;
- break;
- }
- break;
- default:
- case 0x01:
- switch(addr) {
- case 0x00:
- case 0x01:
- case 0x02:
- case 0x03:
- case 0x06:
- case 0x07:
- case 0x08:
- case 0x09:
- case 0x0a:
- case 0x0b:
- case 0x0e:
- case 0x2c ... 0x2f: /* read-only subsystem ID & vendor ID */
- case 0x38 ... 0x3b: /* rom */
- case 0x3d:
- can_write = 0;
- break;
- default:
- can_write = 1;
- break;
- }
- break;
- }
- if (can_write) {
- /* Mask out writes to reserved bits in registers */
- switch (addr) {
- case 0x05:
- val &= ~PCI_COMMAND_RESERVED_MASK_HI;
- break;
- case 0x06:
- val &= ~PCI_STATUS_RESERVED_MASK_LO;
- break;
- case 0x07:
- val &= ~PCI_STATUS_RESERVED_MASK_HI;
- break;
- }
- d->config[addr] = val;
- }
- if (++addr > 0xff)
- break;
- val >>= 8;
+ memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
+ for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
+ uint8_t mask = d->mask[addr];
+ d->config[addr] = (d->config[addr] & ~mask) | (val & mask);
}
-
- end = address + len;
- if (end > PCI_COMMAND && address < (PCI_COMMAND + 2)) {
- /* if the command register is modified, we must modify the mappings */
+ if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
+ || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
+ & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
pci_update_mappings(d);
- }
}
void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
@@ -847,16 +768,8 @@ static void pci_bridge_write_config(PCIDevice *d,
{
PCIBridge *s = (PCIBridge *)d;
- if (address == 0x19 || (address == 0x18 && len > 1)) {
- if (address == 0x19)
- s->bus->bus_num = val & 0xff;
- else
- s->bus->bus_num = (val >> 8) & 0xff;
-#if defined(DEBUG_PCI)
- printf ("pci-bridge: %s: Assigned bus %d\n", d->name, s->bus->bus_num);
-#endif
- }
pci_default_write_config(d, address, val, len);
+ s->bus->bus_num = d->config[PCI_SECONDARY_BUS];
}
PCIBus *pci_find_bus(int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 0405837..c0c2380 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -100,16 +100,24 @@ typedef struct PCIIORegion {
#define PCI_COMMAND 0x04 /* 16 bits */
#define PCI_COMMAND_IO 0x1 /* Enable response in I/O space */
#define PCI_COMMAND_MEMORY 0x2 /* Enable response in Memory space */
+#define PCI_COMMAND_MASTER 0x4 /* Enable bus master */
#define PCI_STATUS 0x06 /* 16 bits */
#define PCI_REVISION_ID 0x08 /* 8 bits */
#define PCI_CLASS_DEVICE 0x0a /* Device class */
+#define PCI_CACHE_LINE_SIZE 0x0c /* 8 bits */
+#define PCI_LATENCY_TIMER 0x0d /* 8 bits */
#define PCI_HEADER_TYPE 0x0e /* 8 bits */
#define PCI_HEADER_TYPE_NORMAL 0
#define PCI_HEADER_TYPE_BRIDGE 1
#define PCI_HEADER_TYPE_CARDBUS 2
#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
+#define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */
+#define PCI_PRIMARY_BUS 0x18 /* Primary bus number */
+#define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */
+#define PCI_SEC_STATUS 0x1e /* Secondary status register, only bit 14 used */
#define PCI_SUBSYSTEM_VENDOR_ID 0x2c /* 16 bits */
#define PCI_SUBSYSTEM_ID 0x2e /* 16 bits */
+#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */
#define PCI_INTERRUPT_LINE 0x3c /* 8 bits */
#define PCI_INTERRUPT_PIN 0x3d /* 8 bits */
#define PCI_MIN_GNT 0x3e /* 8 bits */
@@ -139,10 +147,18 @@ typedef struct PCIIORegion {
#define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8)
+/* Size of the standard PCI config header */
+#define PCI_CONFIG_HEADER_SIZE 0x40
+/* Size of the standard PCI config space */
+#define PCI_CONFIG_SPACE_SIZE 0x100
+
struct PCIDevice {
DeviceState qdev;
/* PCI config space */
- uint8_t config[256];
+ uint8_t config[PCI_CONFIG_SPACE_SIZE];
+
+ /* Used to implement R/W bytes */
+ uint8_t mask[PCI_CONFIG_SPACE_SIZE];
/* the following fields are read only */
PCIBus *bus;
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 1/7] vmware_vga: clean up Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 2/7] qemu: make default_write_config use mask table Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-02 10:01 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 4/7] pci/config: convert pci configuration space handler to use callback Isaku Yamahata
` (3 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
clean up of pci_default_config_write() with callback.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v6
- rename shift to reg_offset.
- minor fixes
Changes v5
- rebased resolving conflict.
Changes v4
- export some helper functions
- dropped old_val from callback signature
Changes v3
- recreated on top of Michael's patch.
Changes v2
- converted static table into dynamic initialization.
- changed callback signature.
pci/config clean up. fix class interface.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
try to implement complete pci-to-pci bridge emulator.
work in progress.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/cirrus_vga.c | 2 +-
hw/pci.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++--------
hw/pci.h | 106 ++++++++++++++++++-
3 files changed, 376 insertions(+), 50 deletions(-)
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index f0bb8d9..f3fed9e 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -180,7 +180,7 @@
#define PCI_COMMAND_PALETTESNOOPING 0x0020
#define PCI_COMMAND_PARITYDETECTION 0x0040
#define PCI_COMMAND_ADDRESSDATASTEPPING 0x0080
-#define PCI_COMMAND_SERR 0x0100
+//#define PCI_COMMAND_SERR 0x0100 /* duplicated */
#define PCI_COMMAND_BACKTOBACKTRANS 0x0200
// PCI 0x08, 0xff000000 (0x09-0x0b:class,0x08:rev)
#define PCI_CLASS_BASE_DISPLAY 0x03
diff --git a/hw/pci.c b/hw/pci.c
index 1e70e46..2c8dece 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -21,6 +21,7 @@
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
+
#include "hw.h"
#include "pci.h"
#include "monitor.h"
@@ -48,7 +49,6 @@ struct PCIBus {
int irq_count[];
};
-static void pci_update_mappings(PCIDevice *d);
static void pci_set_irq(void *opaque, int irq_num, int level);
target_phys_addr_t pci_mem_base;
@@ -239,22 +239,128 @@ int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp)
return pci_parse_devaddr(devaddr, domp, busp, slotp);
}
-static void pci_init_mask(PCIDevice *dev)
+static void pci_conf_init(struct PCIConfigReg *config_regs,
+ uint32_t addr, pci_config_written_t callback,
+ uint32_t wmask, int len)
{
int i;
- dev->mask[PCI_CACHE_LINE_SIZE] = 0xff;
- dev->mask[PCI_INTERRUPT_LINE] = 0xff;
- dev->mask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
- | PCI_COMMAND_MASTER;
- for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
- dev->mask[i] = 0xff;
+
+ for (i = 0; i < len; i++) {
+ config_regs[addr].wmask = wmask & 0xff;
+ config_regs[addr].reg_offset = 8 * i;
+ config_regs[addr].callback = callback;
+
+ wmask >>= 8;
+ }
+}
+
+void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask)
+{
+ pci_conf_init(config_regs, addr, callback, wmask, 1);
+}
+
+void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask)
+{
+ pci_conf_init(config_regs, addr, callback, wmask, 2);
+}
+
+void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask)
+{
+ pci_conf_init(config_regs, addr, callback, wmask, 4);
+}
+
+static void pci_conf_update_mappings(struct PCIDevice *d,
+ uint32_t written, uint32_t mask)
+{
+ pci_update_mappings(d);
+}
+
+static void pci_conf_init_type_00_default(struct PCIConfigReg *config_regs)
+{
+ uint32_t addr;
+
+ /* Vendor ID, Device ID: read only */
+ pci_conf_initw(config_regs, PCI_VENDOR_ID, NULL, 0);
+ pci_conf_initw(config_regs, PCI_DEVICE_ID, NULL, 0);
+
+ pci_conf_initw(config_regs, PCI_COMMAND, pci_conf_update_mappings,
+ PCI_COMMAND_IO |
+ PCI_COMMAND_MEMORY |
+ PCI_COMMAND_MASTER |
+ PCI_COMMAND_SPECIAL |
+ PCI_COMMAND_INVALIDATE |
+ PCI_COMMAND_VGA_PALETTE |
+ PCI_COMMAND_PARITY |
+ PCI_COMMAND_WAIT |
+ PCI_COMMAND_SERR |
+ PCI_COMMAND_FAST_BACK |
+ PCI_COMMAND_INTX_DISABLE);
+
+ /* nothing is emulated at this moment */
+ pci_conf_initw(config_regs, PCI_STATUS, NULL, 0);
+
+ /* revision id, class code: read only */
+ pci_conf_initb(config_regs, PCI_REVISION_ID, NULL, 0);
+ pci_conf_initb(config_regs, PCI_CLASS_INTERFACE, NULL, 0);
+ pci_conf_initw(config_regs, PCI_CLASS_DEVICE, NULL, 0);
+
+ pci_conf_initb(config_regs, PCI_CACHE_LINE_SIZE, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_LATENCY_TIMER, NULL, ~0);
+
+ /* header type: read only */
+ pci_conf_initb(config_regs, PCI_HEADER_TYPE, NULL, 0);
+
+ /* BIST emulation isn't implemented */
+ pci_conf_initb(config_regs, PCI_BIST, NULL, 0);
+
+ /* bar:wmask will be updated by pci_register_io_region() */
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_0,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_1,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_2,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_3,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_4,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_5,
+ pci_conf_update_mappings, 0);
+
+ /* not card bus*/
+ pci_conf_initl(config_regs, PCI_CARDBUS_CIS, NULL, 0);
+
+ /* Subsystem ID, Subsystem Vendor ID: read only*/
+ pci_conf_initw(config_regs, PCI_SUBSYSTEM_VENDOR_ID, NULL, 0);
+ pci_conf_initw(config_regs, PCI_SUBSYSTEM_ID, NULL, 0);
+
+
+ /* mask will be updated by pci_register_io_region() */
+ pci_conf_initl(config_regs, PCI_ROM_ADDRESS, pci_conf_update_mappings, 0);
+
+ pci_conf_initb(config_regs, PCI_CAPABILITY_LIST, NULL, 0);
+
+ /* offset 0x35 ... 0x3d are reserved so is read only */
+
+ pci_conf_initb(config_regs, PCI_INTERRUPT_LINE, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_INTERRUPT_PIN, NULL, 0);
+ pci_conf_initb(config_regs, PCI_MIN_GNT, NULL, 0);
+ pci_conf_initb(config_regs, PCI_MAX_LAT, NULL, 0);
+
+ /* device dependent part */
+ for (addr = PCI_CONFIG_HEADER_SIZE; addr < PCI_CONFIG_SPACE_SIZE; addr++)
+ pci_conf_initb(config_regs, addr, NULL, ~0);
}
/* -1 for devfn means auto assign */
static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
const char *name, int devfn,
PCIConfigReadFunc *config_read,
- PCIConfigWriteFunc *config_write)
+ PCIConfigWriteFunc *config_write,
+ pci_conf_init_t conf_init)
{
if (pci_irq_index >= PCI_DEVICES_MAX)
return NULL;
@@ -272,7 +378,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
pci_set_default_subsystem_id(pci_dev);
- pci_init_mask(pci_dev);
+ conf_init(pci_dev->config_regs);
if (!config_read)
config_read = pci_default_read_config;
@@ -286,18 +392,31 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
return pci_dev;
}
-PCIDevice *pci_register_device(PCIBus *bus, const char *name,
- int instance_size, int devfn,
- PCIConfigReadFunc *config_read,
- PCIConfigWriteFunc *config_write)
+PCIDevice *pci_register_device_confreg(PCIBus *bus, const char *name,
+ int instance_size, int devfn,
+ PCIConfigReadFunc *config_read,
+ PCIConfigWriteFunc *config_write,
+ pci_conf_init_t conf_init)
{
PCIDevice *pci_dev;
pci_dev = qemu_mallocz(instance_size);
pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
- config_read, config_write);
+ config_read, config_write,
+ pci_conf_init_type_00_default);
return pci_dev;
}
+
+PCIDevice *pci_register_device(PCIBus *bus, const char *name,
+ int instance_size, int devfn,
+ PCIConfigReadFunc *config_read,
+ PCIConfigWriteFunc *config_write)
+{
+ return pci_register_device_confreg(bus, name, instance_size, devfn,
+ config_read, config_write,
+ pci_conf_init_type_00_default);
+}
+
static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
{
return addr + pci_mem_base;
@@ -346,7 +465,8 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
{
PCIIORegion *r;
uint32_t addr;
- uint32_t mask;
+ uint32_t wmask;
+ int i;
if ((unsigned int)region_num >= PCI_NUM_REGIONS)
return;
@@ -362,20 +482,21 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
r->size = size;
r->type = type;
r->map_func = map_func;
-
- mask = ~(size - 1);
+ wmask = ~(size - 1);
if (region_num == PCI_ROM_SLOT) {
addr = 0x30;
/* ROM enable bit is writeable */
- mask |= 1;
+ wmask |= PCI_ROM_ADDRESS_ENABLE;
} else {
addr = 0x10 + region_num * 4;
}
*(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
- *(uint32_t *)(pci_dev->mask + addr) = cpu_to_le32(mask);
+
+ for (i = 0; i < 4; i++)
+ pci_dev->config_regs[addr + i].wmask = (wmask >> (8 * i)) && 0xff;
}
-static void pci_update_mappings(PCIDevice *d)
+void pci_update_mappings(PCIDevice *d)
{
PCIIORegion *r;
int cmd, i;
@@ -481,21 +602,47 @@ uint32_t pci_default_read_config(PCIDevice *d,
return val;
}
-void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
+void pci_default_write_config(PCIDevice *d,
+ uint32_t addr, uint32_t val, int len)
{
- uint8_t orig[PCI_CONFIG_SPACE_SIZE];
int i;
- /* not efficient, but simple */
- memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
- for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
- uint8_t mask = d->mask[addr];
- d->config[addr] = (d->config[addr] & ~mask) | (val & mask);
+ uint32_t written[4] = {0, 0, 0, 0};
+ uint32_t mask[4] = {0, 0, 0, 0};
+ pci_config_written_t callback[4] = {NULL, NULL, NULL, NULL};
+ int nb_written = 0;
+ uint8_t reg_offset_prev = -1;
+
+ assert(len == 1 || len == 2 || len == 4);
+ for (i = 0; i < len; val >>= 8, ++i) {
+ uint8_t wmask = d->config_regs[addr].wmask;
+ uint8_t reg_offset = d->config_regs[addr].reg_offset;
+ pci_config_written_t c = d->config_regs[addr].callback;
+
+ d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
+
+ if (c != NULL) {
+ if (callback[nb_written] != c ||
+ reg_offset != reg_offset_prev + 8) {
+ if (callback[nb_written] != NULL)
+ nb_written++;
+ callback[nb_written] = c;
+ }
+
+ written[nb_written] |= ((uint32_t)val & 0xff) << reg_offset;
+ mask[nb_written] |= 0xff << reg_offset;
+ reg_offset_prev = reg_offset;
+ }
+
+ if (++addr >= PCI_CONFIG_SPACE_SIZE)
+ break;
}
- if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
- || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
- & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
- pci_update_mappings(d);
+
+ if (callback[nb_written] != NULL)
+ nb_written++;
+
+ for (i = 0; i < nb_written; i++)
+ (callback[i])(d, written[i], mask[i]);
}
void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
@@ -763,15 +910,6 @@ typedef struct {
PCIBus *bus;
} PCIBridge;
-static void pci_bridge_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
-{
- PCIBridge *s = (PCIBridge *)d;
-
- pci_default_write_config(d, address, val, len);
- s->bus->bus_num = d->config[PCI_SECONDARY_BUS];
-}
-
PCIBus *pci_find_bus(int bus_num)
{
PCIBus *bus = first_bus;
@@ -792,12 +930,104 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
return bus->devices[PCI_DEVFN(slot, function)];
}
+static void pci_conf_secondary_bus_changed(PCIDevice *d,
+ uint32_t written, uint32_t mask)
+{
+ PCIBridge *s = (PCIBridge*) d;
+ s->bus->bus_num = s->dev.config[PCI_SECONDARY_BUS];
+}
+
+static void pci_conf_init_type_01_default(struct PCIConfigReg *config_regs)
+{
+ uint32_t addr;
+
+ /* Vendor ID, Device ID: read only */
+ pci_conf_initw(config_regs, PCI_VENDOR_ID, NULL, 0);
+ pci_conf_initw(config_regs, PCI_DEVICE_ID, NULL, 0);
+
+ pci_conf_initw(config_regs, PCI_COMMAND, pci_conf_update_mappings,
+ PCI_COMMAND_IO |
+ PCI_COMMAND_MEMORY |
+ PCI_COMMAND_MASTER |
+ PCI_COMMAND_SPECIAL |
+ PCI_COMMAND_INVALIDATE |
+ PCI_COMMAND_VGA_PALETTE |
+ PCI_COMMAND_PARITY |
+ PCI_COMMAND_WAIT |
+ PCI_COMMAND_SERR |
+ PCI_COMMAND_FAST_BACK |
+ PCI_COMMAND_INTX_DISABLE);
+
+ /* nothing is emulated at this moment */
+ pci_conf_initw(config_regs, PCI_STATUS, NULL, 0);
+
+ /* revision id, class code: read only */
+ pci_conf_initb(config_regs, PCI_REVISION_ID, NULL, 0);
+ pci_conf_initb(config_regs, PCI_CLASS_INTERFACE, NULL, 0);
+ pci_conf_initw(config_regs, PCI_CLASS_DEVICE, NULL, 0);
+
+ pci_conf_initb(config_regs, PCI_CACHE_LINE_SIZE, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_LATENCY_TIMER, NULL, ~0);
+
+ /* header type: read only */
+ pci_conf_initb(config_regs, PCI_HEADER_TYPE, NULL, 0);
+
+ /* BIST emulation isn't implemented */
+ pci_conf_initb(config_regs, PCI_BIST, NULL, 0);
+
+ /* bar:wmask will be updated by pci_register_io_region() */
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_0,
+ pci_conf_update_mappings, 0);
+ pci_conf_initl(config_regs, PCI_BASE_ADDRESS_1,
+ pci_conf_update_mappings, 0);
+
+ pci_conf_initb(config_regs, PCI_PRIMARY_BUS, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_SECONDARY_BUS,
+ pci_conf_secondary_bus_changed, ~0);
+ pci_conf_initb(config_regs, PCI_SUBORDINATE_BUS, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_SEC_LATENCY_TIMER, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_IO_BASE, NULL, PCI_IO_RANGE_MASK & 0xff);
+ pci_conf_initb(config_regs, PCI_IO_LIMIT, NULL, PCI_IO_RANGE_MASK & 0xff);
+
+ /* sec status isn't emulated (yet) */
+ pci_conf_initw(config_regs, PCI_SEC_STATUS, NULL, 0);
+
+ pci_conf_initw(config_regs, PCI_MEMORY_BASE, NULL,
+ PCI_MEMORY_RANGE_MASK & 0xffff);
+ pci_conf_initw(config_regs, PCI_MEMORY_LIMIT, NULL,
+ PCI_MEMORY_RANGE_MASK & 0xffff);
+ pci_conf_initw(config_regs, PCI_PREF_MEMORY_BASE, NULL,
+ PCI_PREF_RANGE_MASK & 0xffff);
+ pci_conf_initw(config_regs, PCI_PREF_MEMORY_LIMIT, NULL,
+ PCI_PREF_RANGE_MASK & 0xffff);
+ pci_conf_initl(config_regs, PCI_PREF_BASE_UPPER32, NULL, ~0);
+ pci_conf_initl(config_regs, PCI_PREF_LIMIT_UPPER32, NULL, ~0);
+
+ /* only support 64K io port */
+ pci_conf_initw(config_regs, PCI_IO_BASE_UPPER16, NULL, 0);
+ pci_conf_initw(config_regs, PCI_IO_LIMIT_UPPER16, NULL, 0);
+
+ pci_conf_initb(config_regs, PCI_CAPABILITY_LIST, NULL, 0);
+
+ /* wmask will be updated by pci_register_io_region() */
+ pci_conf_initl(config_regs, PCI_ROM_ADDRESS1, pci_conf_update_mappings, 0);
+
+ pci_conf_initb(config_regs, PCI_INTERRUPT_LINE, NULL, ~0);
+ pci_conf_initb(config_regs, PCI_INTERRUPT_PIN, NULL, 0);
+ pci_conf_initw(config_regs, PCI_BRIDGE_CONTROL, NULL, ~0);
+
+ /* device dependent part */
+ for (addr = PCI_CONFIG_HEADER_SIZE; addr < PCI_CONFIG_SPACE_SIZE; addr++)
+ pci_conf_initb(config_regs, addr, NULL, ~0);
+}
+
PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
pci_map_irq_fn map_irq, const char *name)
{
PCIBridge *s;
- s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
- devfn, NULL, pci_bridge_write_config);
+ s = (PCIBridge *)pci_register_device_confreg(bus, name, sizeof(PCIBridge),
+ devfn, NULL, NULL,
+ pci_conf_init_type_01_default);
pci_config_set_vendor_id(s->dev.config, vid);
pci_config_set_device_id(s->dev.config, did);
@@ -833,7 +1063,7 @@ static void pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
devfn = qdev_get_prop_int(qdev, "devfn", -1);
pci_dev = do_pci_register_device(pci_dev, bus, "FIXME", devfn,
- NULL, NULL);//FIXME:config_read, config_write);
+ NULL, NULL, pci_conf_init_type_00_default);//FIXME:config_read, config_write);
assert(pci_dev);
info->init(pci_dev);
}
diff --git a/hw/pci.h b/hw/pci.h
index c0c2380..cdaceda 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -101,8 +101,17 @@ typedef struct PCIIORegion {
#define PCI_COMMAND_IO 0x1 /* Enable response in I/O space */
#define PCI_COMMAND_MEMORY 0x2 /* Enable response in Memory space */
#define PCI_COMMAND_MASTER 0x4 /* Enable bus master */
+#define PCI_COMMAND_SPECIAL 0x8 /* Enable response to special cycles */
+#define PCI_COMMAND_INVALIDATE 0x10 /* Use memory write and invalidate */
+#define PCI_COMMAND_VGA_PALETTE 0x20 /* Enable palette snooping */
+#define PCI_COMMAND_PARITY 0x40 /* Enable parity checking */
+#define PCI_COMMAND_WAIT 0x80 /* Enable address/data stepping */
+#define PCI_COMMAND_SERR 0x100 /* Enable SERR */
+#define PCI_COMMAND_FAST_BACK 0x200 /* Enable back-to-back writes */
+#define PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
#define PCI_STATUS 0x06 /* 16 bits */
#define PCI_REVISION_ID 0x08 /* 8 bits */
+#define PCI_CLASS_INTERFACE 0x09 /* Device class interface */
#define PCI_CLASS_DEVICE 0x0a /* Device class */
#define PCI_CACHE_LINE_SIZE 0x0c /* 8 bits */
#define PCI_LATENCY_TIMER 0x0d /* 8 bits */
@@ -111,10 +120,30 @@ typedef struct PCIIORegion {
#define PCI_HEADER_TYPE_BRIDGE 1
#define PCI_HEADER_TYPE_CARDBUS 2
#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
+#define PCI_BIST 0x0f /* 8 bits */
+#define PCI_BIST_CODE_MASK 0x0f /* Return result */
+#define PCI_BIST_START 0x40 /* 1 to start BIST, 2 secs or less */
+#define PCI_BIST_CAPABLE 0x80 /* 1 if BIST capable */
#define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */
+#define PCI_BASE_ADDRESS_1 0x14 /* 32 bits [htype 0,1 only] */
+#define PCI_BASE_ADDRESS_2 0x18 /* 32 bits [htype 0 only] */
+#define PCI_BASE_ADDRESS_3 0x1c /* 32 bits */
+#define PCI_BASE_ADDRESS_4 0x20 /* 32 bits */
+#define PCI_BASE_ADDRESS_5 0x24 /* 32 bits */
+#define PCI_BASE_ADDRESS_SPACE 0x01 /* 0 = memory, 1 = I/O */
+#define PCI_BASE_ADDRESS_SPACE_IO 0x01
+#define PCI_BASE_ADDRESS_SPACE_MEMORY 0x00
+#define PCI_BASE_ADDRESS_MEM_TYPE_MASK 0x06
+#define PCI_BASE_ADDRESS_MEM_TYPE_32 0x00 /* 32 bit address */
+#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02 /* Below 1M [obsolete] */
+#define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04 /* 64 bit address */
+#define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */
+#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL)
+#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL)
#define PCI_PRIMARY_BUS 0x18 /* Primary bus number */
#define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */
#define PCI_SEC_STATUS 0x1e /* Secondary status register, only bit 14 used */
+#define PCI_CARDBUS_CIS 0x28
#define PCI_SUBSYSTEM_VENDOR_ID 0x2c /* 16 bits */
#define PCI_SUBSYSTEM_ID 0x2e /* 16 bits */
#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */
@@ -127,6 +156,10 @@ typedef struct PCIIORegion {
#define PCI_SUBVENDOR_ID 0x2c /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
#define PCI_SUBDEVICE_ID 0x2e /* obsolete, use PCI_SUBSYSTEM_ID */
+#define PCI_ROM_ADDRESS 0x30 /* Bits 31..11 are address, 10..1 reserved */
+#define PCI_ROM_ADDRESS_ENABLE 0x01
+#define PCI_ROM_ADDRESS_MASK (~0x7ffUL)
+
/* Bits in the PCI Status Register (PCI 2.3 spec) */
#define PCI_STATUS_RESERVED1 0x007
#define PCI_STATUS_INT_STATUS 0x008
@@ -147,18 +180,66 @@ typedef struct PCIIORegion {
#define PCI_COMMAND_RESERVED_MASK_HI (PCI_COMMAND_RESERVED >> 8)
+/* Header type 1 (PCI-to-PCI bridges) */
+#define PCI_SUBORDINATE_BUS 0x1a /* Highest bus number behind the bridge */
+#define PCI_SEC_LATENCY_TIMER 0x1b /* Latency timer for secondary interface */
+#define PCI_IO_BASE 0x1c /* I/O range behind the bridge */
+#define PCI_IO_LIMIT 0x1d
+#define PCI_IO_RANGE_TYPE_MASK 0x0fUL /* I/O bridging type */
+#define PCI_IO_RANGE_TYPE_16 0x00
+#define PCI_IO_RANGE_TYPE_32 0x01
+#define PCI_IO_RANGE_MASK (~0x0fUL)
+#define PCI_MEMORY_BASE 0x20 /* Memory range behind */
+#define PCI_MEMORY_LIMIT 0x22
+#define PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
+#define PCI_MEMORY_RANGE_MASK (~0x0fUL)
+#define PCI_PREF_MEMORY_BASE 0x24 /* Prefetchable memory range behind */
+#define PCI_PREF_MEMORY_LIMIT 0x26
+#define PCI_PREF_RANGE_TYPE_MASK 0x0fUL
+#define PCI_PREF_RANGE_TYPE_32 0x00
+#define PCI_PREF_RANGE_TYPE_64 0x01
+#define PCI_PREF_RANGE_MASK (~0x0fUL)
+#define PCI_PREF_BASE_UPPER32 0x28 /* Upper half of prefetchable memory range */
+#define PCI_PREF_LIMIT_UPPER32 0x2c
+#define PCI_IO_BASE_UPPER16 0x30 /* Upper half of I/O addresses */
+#define PCI_IO_LIMIT_UPPER16 0x32
+/* 0x34 same as for htype 0 */
+/* 0x35-0x3b is reserved */
+#define PCI_ROM_ADDRESS1 0x38 /* Same as PCI_ROM_ADDRESS, but for htype 1 */
+/* 0x3c-0x3d are same as for htype 0 */
+#define PCI_BRIDGE_CONTROL 0x3e
+#define PCI_BRIDGE_CTL_PARITY 0x01 /* Enable parity detection on secondary interface */
+#define PCI_BRIDGE_CTL_SERR 0x02 /* The same for SERR forwarding */
+#define PCI_BRIDGE_CTL_ISA 0x04 /* Enable ISA mode */
+#define PCI_BRIDGE_CTL_VGA 0x08 /* Forward VGA addresses */
+#define PCI_BRIDGE_CTL_MASTER_ABORT 0x20 /* Report master aborts */
+#define PCI_BRIDGE_CTL_BUS_RESET 0x40 /* Secondary bus reset */
+#define PCI_BRIDGE_CTL_FAST_BACK 0x80 /* Fast Back2Back enabled on secondary interface */
+
+/* Bits in the PCI Command Register (PCI 2.3 spec) */
+#define PCI_COMMAND_RESERVED_BRIDGE 0xf880
+
+#define PCI_COMMAND_RESERVED_MASK_HI_BRIDGE (PCI_COMMAND_RESERVED >> 8)
+
/* Size of the standard PCI config header */
-#define PCI_CONFIG_HEADER_SIZE 0x40
+#define PCI_CONFIG_HEADER_SIZE 0x40
/* Size of the standard PCI config space */
-#define PCI_CONFIG_SPACE_SIZE 0x100
+#define PCI_CONFIG_SPACE_SIZE 0x100
+
+typedef void (*pci_config_written_t)(struct PCIDevice *d,
+ uint32_t written, uint32_t mask);
+struct PCIConfigReg {
+ uint8_t wmask;
+ /* offset of registers in bits for 2/4 bytes function register */
+ uint8_t reg_offset;
+ pci_config_written_t callback;
+};
struct PCIDevice {
DeviceState qdev;
/* PCI config space */
uint8_t config[PCI_CONFIG_SPACE_SIZE];
-
- /* Used to implement R/W bytes */
- uint8_t mask[PCI_CONFIG_SPACE_SIZE];
+ struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
/* the following fields are read only */
PCIBus *bus;
@@ -180,6 +261,21 @@ struct PCIDevice {
int irq_state[4];
};
+typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
+
+void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask);
+void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask);
+void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
+ pci_config_written_t callback, uint32_t wmask);
+void pci_update_mappings(PCIDevice *d);
+
+PCIDevice *pci_register_device_confreg(PCIBus *bus, const char *name,
+ int instance_size, int devfn,
+ PCIConfigReadFunc *config_read,
+ PCIConfigWriteFunc *config_write,
+ pci_conf_init_t conf_init);
PCIDevice *pci_register_device(PCIBus *bus, const char *name,
int instance_size, int devfn,
PCIConfigReadFunc *config_read,
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 4/7] pci/config: convert pci configuration space handler to use callback.
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
` (2 preceding siblings ...)
2009-06-02 6:42 ` [Qemu-devel] [PATCH 3/7] pci: pci_default_config_write() clean up Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-10 15:16 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 5/7] pci: PCIBus clean up Isaku Yamahata
` (2 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
convert pci configuration space handler into callbacks.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
changes v5
- typo
- don't change cirrus_vga.c behaviour.
---
hw/acpi.c | 11 ++++-----
hw/cirrus_vga.c | 15 ++++++++++-
hw/gt64xxx.c | 13 +---------
hw/piix_pci.c | 18 +++++++++-----
hw/vga.c | 10 +++++---
hw/wdt_i6300esb.c | 66 +++++++++++++++++++++-------------------------------
6 files changed, 63 insertions(+), 70 deletions(-)
diff --git a/hw/acpi.c b/hw/acpi.c
index fccae69..7fcbd00 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -433,12 +433,10 @@ static void pm_io_space_update(PIIX4PMState *s)
}
}
-static void pm_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
+static void pm_write_config_0x80(struct PCIDevice *d,
+ uint32_t written, uint32_t mask)
{
- pci_default_write_config(d, address, val, len);
- if (address == 0x80)
- pm_io_space_update((PIIX4PMState *)d);
+ pm_io_space_update((PIIX4PMState *)d);
}
static void pm_save(QEMUFile* f,void *opaque)
@@ -505,7 +503,8 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
s = (PIIX4PMState *)pci_register_device(bus,
"PM", sizeof(PIIX4PMState),
- devfn, NULL, pm_write_config);
+ devfn, NULL, NULL);
+ pci_conf_initb(s->dev.config_regs, 0x80, pm_write_config_0x80, 1);
pm_state = s;
pci_conf = s->dev.config;
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index f3fed9e..5b02f40 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3289,6 +3289,17 @@ static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
s->cirrus_mmio_io_addr);
}
+static void pci_cirrus_write_config_bar0(struct PCIDevice *d,
+ uint32_t written, uint32_t mask)
+{
+ PCICirrusVGAState *pvs = container_of(d, PCICirrusVGAState, dev);
+ CirrusVGAState *s = &pvs->cirrus_vga;
+
+ pci_update_mappings(d);
+ if (s->vga.map_addr && pvs->dev.io_regions[0].addr == -1)
+ s->vga.map_addr = 0;
+}
+
static void pci_cirrus_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len)
{
@@ -3296,8 +3307,6 @@ static void pci_cirrus_write_config(PCIDevice *d,
CirrusVGAState *s = &pvs->cirrus_vga;
pci_default_write_config(d, address, val, len);
- if (s->vga.map_addr && pvs->dev.io_regions[0].addr == -1)
- s->vga.map_addr = 0;
cirrus_update_memory_access(s);
}
@@ -3314,6 +3323,8 @@ void pci_cirrus_vga_init(PCIBus *bus)
d = (PCICirrusVGAState *)pci_register_device(bus, "Cirrus VGA",
sizeof(PCICirrusVGAState),
-1, NULL, pci_cirrus_write_config);
+ pci_conf_initl(d->dev.config_regs, PCI_BASE_ADDRESS_0,
+ pci_cirrus_write_config_bar0, 0);
pci_conf = d->dev.config;
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
pci_config_set_device_id(pci_conf, device_id);
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index ce3ffe2..0790ef1 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1083,17 +1083,6 @@ static void gt64120_reset(void *opaque)
gt64120_pci_mapping(s);
}
-static uint32_t gt64120_read_config(PCIDevice *d, uint32_t address, int len)
-{
- return pci_default_read_config(d, address, len);
-}
-
-static void gt64120_write_config(PCIDevice *d, uint32_t address, uint32_t val,
- int len)
-{
- pci_default_write_config(d, address, val, len);
-}
-
static void gt64120_save(QEMUFile* f, void *opaque)
{
PCIDevice *d = opaque;
@@ -1133,7 +1122,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
pic, 144, 4);
s->ISD_handle = cpu_register_io_memory(0, gt64120_read, gt64120_write, s);
d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
- 0, gt64120_read_config, gt64120_write_config);
+ 0, NULL, NULL);
/* FIXME: Malta specific hw assumptions ahead */
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 914a65a..7b6cf0d 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -128,13 +128,11 @@ void i440fx_init_memory_mappings(PCIDevice *d)
}
}
-static void i440fx_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
+/* XXX: implement SMRAM.D_LOCK */
+static void i440fx_write_config_0x59_0x5f_0x72(struct PCIDevice *d,
+ uint32_t written, uint32_t mask)
{
- /* XXX: implement SMRAM.D_LOCK */
- pci_default_write_config(d, address, val, len);
- if ((address >= 0x59 && address <= 0x5f) || address == 0x72)
- i440fx_update_memory_mappings(d);
+ i440fx_update_memory_mappings(d);
}
static void i440fx_save(QEMUFile* f, void *opaque)
@@ -174,6 +172,7 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic)
PCIBus *b;
PCIDevice *d;
I440FXState *s;
+ uint32_t addr;
s = qemu_mallocz(sizeof(I440FXState));
b = pci_register_bus(NULL, "pci",
@@ -191,7 +190,12 @@ PCIBus *i440fx_init(PCIDevice **pi440fx_state, qemu_irq *pic)
register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
d = pci_register_device(b, "i440FX", sizeof(PCIDevice), 0,
- NULL, i440fx_write_config);
+ NULL, NULL);
+ for (addr = 0x59; addr <= 0x5f; addr++)
+ pci_conf_initb(d->config_regs, addr,
+ i440fx_write_config_0x59_0x5f_0x72, 0xff);
+ pci_conf_initb(d->config_regs, 0x72,
+ i440fx_write_config_0x59_0x5f_0x72, 0xff);
pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(d->config, PCI_DEVICE_ID_INTEL_82441);
diff --git a/hw/vga.c b/hw/vga.c
index 013ff10..aeee91a 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2469,13 +2469,13 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
return 0;
}
-static void pci_vga_write_config(PCIDevice *d,
- uint32_t address, uint32_t val, int len)
+static void pci_vga_write_config_bar0(struct PCIDevice *d,
+ uint32_t written, uint32_t mask)
{
PCIVGAState *pvs = container_of(d, PCIVGAState, dev);
VGAState *s = &pvs->vga_state;
- pci_default_write_config(d, address, val, len);
+ pci_update_mappings(d);
if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
s->map_addr = 0;
}
@@ -2489,9 +2489,11 @@ int pci_vga_init(PCIBus *bus,
d = (PCIVGAState *)pci_register_device(bus, "VGA",
sizeof(PCIVGAState),
- -1, NULL, pci_vga_write_config);
+ -1, NULL, NULL);
if (!d)
return -1;
+ pci_conf_initl(d->dev.config_regs, PCI_BASE_ADDRESS_0,
+ pci_vga_write_config_bar0, 0);
s = &d->vga_state;
vga_common_init(s, VGA_RAM_SIZE);
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index f7ddea2..e1946b0 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -198,56 +198,39 @@ static void i6300esb_timer_expired(void *vp)
}
}
-static void i6300esb_config_write(PCIDevice *dev, uint32_t addr,
- uint32_t data, int len)
+static void i6300esb_config_write_esb_config_reg(struct PCIDevice *dev,
+ uint32_t data, uint32_t mask)
{
I6300State *d = (I6300State *) dev;
- int old;
i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len);
- if (addr == ESB_CONFIG_REG && len == 2) {
+ if (mask & ESB_WDT_REBOOT)
d->reboot_enabled = (data & ESB_WDT_REBOOT) == 0;
+ if (mask & ESB_WDT_FREQ)
d->clock_scale =
(data & ESB_WDT_FREQ) != 0 ? CLOCK_SCALE_1MHZ : CLOCK_SCALE_1KHZ;
+ if (mask & ESB_WDT_INTTYPE)
d->int_type = (data & ESB_WDT_INTTYPE);
- } else if (addr == ESB_LOCK_REG && len == 1) {
- if (!d->locked) {
- d->locked = (data & ESB_WDT_LOCK) != 0;
- d->free_run = (data & ESB_WDT_FUNC) != 0;
- old = d->enabled;
- d->enabled = (data & ESB_WDT_ENABLE) != 0;
- if (!old && d->enabled) /* Enabled transitioned from 0 -> 1 */
- i6300esb_restart_timer(d, 1);
- else if (!d->enabled)
- i6300esb_disable_timer(d);
- }
- } else {
- pci_default_write_config(dev, addr, data, len);
- }
}
-static uint32_t i6300esb_config_read(PCIDevice *dev, uint32_t addr, int len)
+static void i6300esb_config_write_esb_lock_reg(struct PCIDevice *dev,
+ uint32_t data, uint32_t mask)
{
I6300State *d = (I6300State *) dev;
- uint32_t data;
-
- i6300esb_debug ("addr = %x, len = %d\n", addr, len);
-
- if (addr == ESB_CONFIG_REG && len == 2) {
- data =
- (d->reboot_enabled ? 0 : ESB_WDT_REBOOT) |
- (d->clock_scale == CLOCK_SCALE_1MHZ ? ESB_WDT_FREQ : 0) |
- d->int_type;
- return data;
- } else if (addr == ESB_LOCK_REG && len == 1) {
- data =
- (d->free_run ? ESB_WDT_FUNC : 0) |
- (d->locked ? ESB_WDT_LOCK : 0) |
- (d->enabled ? ESB_WDT_ENABLE : 0);
- return data;
- } else {
- return pci_default_read_config(dev, addr, len);
+ int old;
+
+ i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len);
+
+ if (!d->locked) {
+ d->locked = (data & ESB_WDT_LOCK) != 0;
+ d->free_run = (data & ESB_WDT_FUNC) != 0;
+ old = d->enabled;
+ d->enabled = (data & ESB_WDT_ENABLE) != 0;
+ if (!old && d->enabled) /* Enabled transitioned from 0 -> 1 */
+ i6300esb_restart_timer(d, 1);
+ else if (!d->enabled)
+ i6300esb_disable_timer(d);
}
}
@@ -429,8 +412,13 @@ static void i6300esb_pc_init(PCIBus *pci_bus)
d = (I6300State *)
pci_register_device (pci_bus, "i6300esb_wdt", sizeof (I6300State),
- -1,
- i6300esb_config_read, i6300esb_config_write);
+ -1, NULL, NULL);
+ pci_conf_initw(d->dev.config_regs, ESB_CONFIG_REG,
+ i6300esb_config_write_esb_config_reg,
+ ESB_WDT_REBOOT | ESB_WDT_FREQ | ESB_WDT_INTTYPE);
+ pci_conf_initb(d->dev.config_regs, ESB_LOCK_REG,
+ i6300esb_config_write_esb_lock_reg,
+ ESB_WDT_FUNC | ESB_WDT_LOCK | ESB_WDT_ENABLE);
d->reboot_enabled = 1;
d->clock_scale = CLOCK_SCALE_1KHZ;
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 5/7] pci: PCIBus clean up.
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
` (3 preceding siblings ...)
2009-06-02 6:42 ` [Qemu-devel] [PATCH 4/7] pci/config: convert pci configuration space handler to use callback Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 6/7] pci/brdige qdevfy Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support Isaku Yamahata
6 siblings, 0 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
- use LIST_xxx macros for bus list.
- remove duplicated member in PCIDevice and PCIBus
- introduce helper function to get PCIDevice/PCIBus
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/pci-hotplug.c | 4 +-
hw/pci.c | 141 +++++++++++++++++++++++++++++++++---------------------
hw/pci.h | 3 +-
3 files changed, 91 insertions(+), 57 deletions(-)
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 4d18ea2..eec65f2 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -158,8 +158,8 @@ void pci_device_hot_add(Monitor *mon, const char *pci_addr, const char *type,
if (dev) {
qemu_system_device_hot_add(bus, PCI_SLOT(dev->devfn), 1);
monitor_printf(mon, "OK domain %d, bus %d, slot %d, function %d\n",
- 0, pci_bus_num(dev->bus), PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn));
+ 0, pci_bus_num(pci_get_parent_bus(dev)),
+ PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
} else
monitor_printf(mon, "failed to add %s\n", opts);
}
diff --git a/hw/pci.c b/hw/pci.c
index 2c8dece..602eeb0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -41,8 +41,8 @@ struct PCIBus {
SetIRQFunc *low_set_irq;
qemu_irq *irq_opaque;
PCIDevice *devices[256];
- PCIDevice *parent_dev;
- PCIBus *next;
+ LIST_ENTRY(PCIBus) next;
+
/* The bus IRQ state is the logical OR of the connected devices.
Keep a count of the number of devices with raised IRQs. */
int nirq;
@@ -55,7 +55,27 @@ target_phys_addr_t pci_mem_base;
static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU;
static int pci_irq_index;
-static PCIBus *first_bus;
+static LIST_HEAD(, PCIBus) first_bus;
+
+static PCIBus *qbus_to_pcibus(struct BusState *qbus)
+{
+ return container_of(qbus, PCIBus, qbus);
+}
+
+static PCIDevice *qdev_to_pcidev(struct DeviceState *qdev)
+{
+ return container_of(qdev, PCIDevice, qdev);
+}
+
+PCIBus *pci_get_parent_bus(PCIDevice *dev)
+{
+ return qbus_to_pcibus(dev->qdev.parent_bus);
+}
+
+PCIDevice *pci_bus_to_dev(PCIBus *bus)
+{
+ return qdev_to_pcidev(bus->qbus.parent);
+}
static void pcibus_save(QEMUFile *f, void *opaque)
{
@@ -103,8 +123,8 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
bus->irq_opaque = pic;
bus->devfn_min = devfn_min;
bus->nirq = nirq;
- bus->next = first_bus;
- first_bus = bus;
+
+ LIST_INSERT_HEAD(&first_bus, bus, next);
register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
return bus;
}
@@ -112,11 +132,19 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
static PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq)
{
PCIBus *bus;
- bus = qemu_mallocz(sizeof(PCIBus));
+ PCIBus *parent_bus;
+ int devfn_min = qdev_get_prop_int(&dev->qdev, "devfn_min", 0);
+ int nirq = qdev_get_prop_int(&dev->qdev, "nirq", 0);
+
+ bus = FROM_QBUS(PCIBus, qbus_create(BUS_TYPE_PCI,
+ sizeof(PCIBus) + (nirq * sizeof(int)),
+ &dev->qdev, "pci"));
bus->map_irq = map_irq;
- bus->parent_dev = dev;
- bus->next = dev->bus->next;
- dev->bus->next = bus;
+ bus->devfn_min = devfn_min;
+ bus->nirq = nirq;
+
+ parent_bus = pci_get_parent_bus(dev);
+ LIST_INSERT_AFTER(parent_bus, bus, next);
return bus;
}
@@ -373,7 +401,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
return NULL;
found: ;
}
- pci_dev->bus = bus;
pci_dev->devfn = devfn;
pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
@@ -454,7 +481,7 @@ int pci_unregister_device(PCIDevice *pci_dev)
qemu_free_irqs(pci_dev->irq);
pci_irq_index--;
- pci_dev->bus->devices[pci_dev->devfn] = NULL;
+ pci_get_parent_bus(pci_dev)->devices[pci_dev->devfn] = NULL;
qdev_free(&pci_dev->qdev);
return 0;
}
@@ -645,6 +672,16 @@ void pci_default_write_config(PCIDevice *d,
(callback[i])(d, written[i], mask[i]);
}
+static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
+{
+ PCIBus *s = from;
+
+ while (s && s->bus_num != bus_num)
+ s = LIST_NEXT(s, next);
+
+ return s;
+}
+
void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
{
PCIBus *s = opaque;
@@ -656,8 +693,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
addr, val, len);
#endif
bus_num = (addr >> 16) & 0xff;
- while (s && s->bus_num != bus_num)
- s = s->next;
+ s = pci_find_bus_from(s, bus_num);
if (!s)
return;
pci_dev = s->devices[(addr >> 8) & 0xff];
@@ -679,8 +715,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
uint32_t val;
bus_num = (addr >> 16) & 0xff;
- while (s && s->bus_num != bus_num)
- s= s->next;
+ s = pci_find_bus_from(s, bus_num);
if (!s)
goto fail;
pci_dev = s->devices[(addr >> 8) & 0xff];
@@ -730,11 +765,11 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
pci_dev->irq_state[irq_num] = level;
for (;;) {
- bus = pci_dev->bus;
+ bus = pci_get_parent_bus(pci_dev);
irq_num = bus->map_irq(pci_dev, irq_num);
if (bus->set_irq)
break;
- pci_dev = bus->parent_dev;
+ pci_dev = pci_bus_to_dev(bus);
}
bus->irq_count[irq_num] += change;
bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
@@ -796,7 +831,8 @@ static void pci_info_device(PCIDevice *d)
const pci_class_desc *desc;
monitor_printf(mon, " Bus %2d, device %3d, function %d:\n",
- d->bus->bus_num, d->devfn >> 3, d->devfn & 7);
+ pci_get_parent_bus(d)->bus_num,
+ d->devfn >> 3, d->devfn & 7);
class = le16_to_cpu(*((uint16_t *)(d->config + PCI_CLASS_DEVICE)));
monitor_printf(mon, " ");
desc = pci_class_descriptions;
@@ -838,12 +874,11 @@ static void pci_info_device(PCIDevice *d)
void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
{
- PCIBus *bus = first_bus;
+ PCIBus *bus;
PCIDevice *d;
int devfn;
- while (bus && bus->bus_num != bus_num)
- bus = bus->next;
+ bus = pci_find_bus_from(LIST_FIRST(&first_bus), bus_num);
if (bus) {
for(devfn = 0; devfn < 256; devfn++) {
d = bus->devices[devfn];
@@ -905,19 +940,9 @@ PCIDevice *pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn,
return NULL;
}
-typedef struct {
- PCIDevice dev;
- PCIBus *bus;
-} PCIBridge;
-
PCIBus *pci_find_bus(int bus_num)
{
- PCIBus *bus = first_bus;
-
- while (bus && bus->bus_num != bus_num)
- bus = bus->next;
-
- return bus;
+ return pci_find_bus_from(LIST_FIRST(&first_bus), bus_num);
}
PCIDevice *pci_find_device(int bus_num, int slot, int function)
@@ -930,11 +955,17 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function)
return bus->devices[PCI_DEVFN(slot, function)];
}
+static PCIBus *pci_bridge_get_secbus(PCIDevice *d)
+{
+ /* assuming only one bus is registered */
+ return qbus_to_pcibus(LIST_FIRST(&d->qdev.child_bus));
+}
+
static void pci_conf_secondary_bus_changed(PCIDevice *d,
uint32_t written, uint32_t mask)
{
- PCIBridge *s = (PCIBridge*) d;
- s->bus->bus_num = s->dev.config[PCI_SECONDARY_BUS];
+ PCIBus *bus = pci_bridge_get_secbus(d);
+ bus->bus_num = d->config[PCI_SECONDARY_BUS];
}
static void pci_conf_init_type_01_default(struct PCIConfigReg *config_regs)
@@ -1024,28 +1055,30 @@ static void pci_conf_init_type_01_default(struct PCIConfigReg *config_regs)
PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
pci_map_irq_fn map_irq, const char *name)
{
- PCIBridge *s;
- s = (PCIBridge *)pci_register_device_confreg(bus, name, sizeof(PCIBridge),
- devfn, NULL, NULL,
- pci_conf_init_type_01_default);
-
- pci_config_set_vendor_id(s->dev.config, vid);
- pci_config_set_device_id(s->dev.config, did);
-
- s->dev.config[0x04] = 0x06; // command = bus master, pci mem
- s->dev.config[0x05] = 0x00;
- s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
- s->dev.config[0x07] = 0x00; // status = fast devsel
- s->dev.config[0x08] = 0x00; // revision
- s->dev.config[0x09] = 0x00; // programming i/f
- pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PCI);
- s->dev.config[0x0D] = 0x10; // latency_timer
- s->dev.config[PCI_HEADER_TYPE] =
+ PCIDevice *d;
+ uint8_t *pci_conf;
+
+ d = pci_register_device_confreg(bus, name, sizeof(PCIDevice),
+ devfn, NULL, NULL,
+ pci_conf_init_type_01_default);
+
+ pci_conf = d->config;
+ pci_config_set_vendor_id(pci_conf, vid);
+ pci_config_set_device_id(pci_conf, did);
+
+ pci_conf[0x04] = 0x06; // command = bus master, pci mem
+ pci_conf[0x05] = 0x00;
+ pci_conf[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
+ pci_conf[0x07] = 0x00; // status = fast devsel
+ pci_conf[0x08] = 0x00; // revision
+ pci_conf[0x09] = 0x00; // programming i/f
+ pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_PCI);
+ pci_conf[0x0D] = 0x10; // latency_timer
+ pci_conf[PCI_HEADER_TYPE] =
PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type
- s->dev.config[0x1E] = 0xa0; // secondary status
+ pci_conf[0x1E] = 0xa0; // secondary status
- s->bus = pci_register_secondary_bus(&s->dev, map_irq);
- return s->bus;
+ return pci_register_secondary_bus(d, map_irq);
}
typedef struct {
diff --git a/hw/pci.h b/hw/pci.h
index cdaceda..c649442 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -242,7 +242,6 @@ struct PCIDevice {
struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
/* the following fields are read only */
- PCIBus *bus;
int devfn;
char name[64];
PCIIORegion io_regions[PCI_NUM_REGIONS];
@@ -314,6 +313,8 @@ int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp);
void pci_info(Monitor *mon);
PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
pci_map_irq_fn map_irq, const char *name);
+PCIBus *pci_get_parent_bus(PCIDevice *dev);
+PCIDevice *pci_bus_to_dev(PCIBus *bus);
static inline void
pci_config_set_vendor_id(uint8_t *pci_config, uint16_t val)
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 6/7] pci/brdige qdevfy.
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
` (4 preceding siblings ...)
2009-06-02 6:42 ` [Qemu-devel] [PATCH 5/7] pci: PCIBus clean up Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support Isaku Yamahata
6 siblings, 0 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
pci/brdige qdevfy.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/apb_pci.c | 12 +++---
hw/pci.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++---------
hw/pci.h | 15 ++++++--
hw/pci_ids.h | 2 +
4 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index dac5cd3..e85e28c 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -266,11 +266,11 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
/* APB secondary busses */
- *bus2 = pci_bridge_init(s->bus, 8, PCI_VENDOR_ID_SUN,
- PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
- "Advanced PCI Bus secondary bridge 1");
- *bus3 = pci_bridge_init(s->bus, 9, PCI_VENDOR_ID_SUN,
- PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
- "Advanced PCI Bus secondary bridge 2");
+ *bus2 = pci_bridge_create_simple(s->bus, 8, PCI_VENDOR_ID_SUN,
+ PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
+ "Advanced PCI Bus secondary bridge 1");
+ *bus3 = pci_bridge_create_simple(s->bus, 9, PCI_VENDOR_ID_SUN,
+ PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
+ "Advanced PCI Bus secondary bridge 2");
return s->bus;
}
diff --git a/hw/pci.c b/hw/pci.c
index 602eeb0..0ffdfef 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -129,25 +129,29 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
return bus;
}
-static PCIBus *pci_register_secondary_bus(PCIDevice *dev, pci_map_irq_fn map_irq)
+/* XXX qemu_irq, nirq */
+static PCIBus *pci_register_secondary_bus(PCIDevice *dev,
+ int bus_num, int devfn_min, int nirq)
{
PCIBus *bus;
PCIBus *parent_bus;
- int devfn_min = qdev_get_prop_int(&dev->qdev, "devfn_min", 0);
- int nirq = qdev_get_prop_int(&dev->qdev, "nirq", 0);
bus = FROM_QBUS(PCIBus, qbus_create(BUS_TYPE_PCI,
sizeof(PCIBus) + (nirq * sizeof(int)),
&dev->qdev, "pci"));
- bus->map_irq = map_irq;
+ bus->bus_num = bus_num;
bus->devfn_min = devfn_min;
- bus->nirq = nirq;
parent_bus = pci_get_parent_bus(dev);
LIST_INSERT_AFTER(parent_bus, bus, next);
return bus;
}
+void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq)
+{
+ bus->map_irq = map_irq;
+}
+
int pci_bus_num(PCIBus *s)
{
return s->bus_num;
@@ -1052,20 +1056,31 @@ static void pci_conf_init_type_01_default(struct PCIConfigReg *config_regs)
pci_conf_initb(config_regs, addr, NULL, ~0);
}
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
- pci_map_irq_fn map_irq, const char *name)
+typedef struct {
+ DeviceInfo qdev;
+ pci_qdev_initfn init;
+} PCIBridgeInfo;
+
+static void pci_bridge_qdev_init(DeviceState *qdev, DeviceInfo *base)
{
- PCIDevice *d;
- uint8_t *pci_conf;
+ PCIDevice *pci_dev = qdev_to_pcidev(qdev);
+ PCIBridgeInfo *info = container_of(base, PCIBridgeInfo, qdev);
- d = pci_register_device_confreg(bus, name, sizeof(PCIDevice),
- devfn, NULL, NULL,
- pci_conf_init_type_01_default);
+ int devfn = qdev_get_prop_int(qdev, "devfn", -1);
+ int sec_bus = qdev_get_prop_int(qdev, "sec_bus", 0);
+ int sub_bus = qdev_get_prop_int(qdev, "sub_bus", sec_bus);
+ int devfn_min = qdev_get_prop_int(qdev, "devfn_min", 0);
+ int nirq = qdev_get_prop_int(qdev, "nirq", 0);
+ char *pci_name = qdev_get_prop_ptr(qdev, "pci_name");
- pci_conf = d->config;
- pci_config_set_vendor_id(pci_conf, vid);
- pci_config_set_device_id(pci_conf, did);
+ uint8_t *pci_conf;
+
+ assert(sec_bus <= sub_bus);
+ pci_dev = do_pci_register_device(pci_dev, pci_get_parent_bus(pci_dev),
+ pci_name, devfn, NULL, NULL,
+ pci_conf_init_type_01_default);
+ pci_conf = pci_dev->config;
pci_conf[0x04] = 0x06; // command = bus master, pci mem
pci_conf[0x05] = 0x00;
pci_conf[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
@@ -1076,9 +1091,68 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
pci_conf[0x0D] = 0x10; // latency_timer
pci_conf[PCI_HEADER_TYPE] =
PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type
+ pci_conf[0x18] = pci_get_parent_bus(pci_dev)->bus_num;// primary bus number
+ pci_conf[0x19] = sec_bus; // secondary bus number
+ pci_conf[0x1A] = sub_bus; // subordinate bus number
pci_conf[0x1E] = 0xa0; // secondary status
- return pci_register_secondary_bus(d, map_irq);
+ pci_register_secondary_bus(pci_dev, sec_bus, devfn_min, nirq);
+
+ /* vid/did, map_irq will be set */
+ info->init(pci_dev);
+}
+
+void pci_bridge_qdev_register(const char* name, int size, pci_qdev_initfn init)
+{
+ PCIBridgeInfo *info;
+
+ info = qemu_mallocz(sizeof(*info));
+ info->init = init;
+ info->qdev.init = pci_bridge_qdev_init;
+ info->qdev.bus_type = BUS_TYPE_PCI;
+
+ qdev_register(name, size, &info->qdev);
+}
+
+/* for compat */
+#define PCI_BRIDGE_DEFAULT "default PCI to PCI bridge"
+static void pci_bridge_default_qdev_init(PCIDevice *dev)
+{
+ /* nothing */
+}
+
+static void pci_brdige_register_device(void)
+{
+ pci_bridge_qdev_register(PCI_BRIDGE_DEFAULT,
+ sizeof(PCIDevice), pci_bridge_default_qdev_init);
+}
+device_init(pci_brdige_register_device);
+
+PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, uint16_t vid,
+ uint16_t did, pci_map_irq_fn map_irq,
+ const char *pci_name)
+{
+ DeviceState *qdev;
+ uint8_t* pci_conf;
+ PCIDevice *d;
+ PCIBus *b;
+ char *pci_name_dup = qemu_strdup(pci_name); /* XXX:leak */
+
+ qdev = qdev_create(&bus->qbus, PCI_BRIDGE_DEFAULT);
+
+ qdev_set_prop_int(qdev, "devfn", devfn);
+ qdev_set_prop_ptr(qdev, "pci_name", pci_name_dup);
+
+ qdev_init(qdev);
+
+ d = qdev_to_pcidev(qdev);
+ pci_conf = d->config;
+ pci_config_set_vendor_id(pci_conf, vid);
+ pci_config_set_device_id(pci_conf, did);
+
+ b = pci_get_parent_bus(d);
+ pci_bridge_set_map_irq(b, map_irq);
+ return b;
}
typedef struct {
diff --git a/hw/pci.h b/hw/pci.h
index c649442..76f3f28 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -306,15 +306,13 @@ int pci_bus_num(PCIBus *s);
void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d));
PCIBus *pci_find_bus(int bus_num);
PCIDevice *pci_find_device(int bus_num, int slot, int function);
+PCIBus *pci_get_parent_bus(PCIDevice *dev);
+PCIDevice *pci_bus_to_dev(PCIBus *bus);
int pci_read_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp);
int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp);
void pci_info(Monitor *mon);
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
- pci_map_irq_fn map_irq, const char *name);
-PCIBus *pci_get_parent_bus(PCIDevice *dev);
-PCIDevice *pci_bus_to_dev(PCIBus *bus);
static inline void
pci_config_set_vendor_id(uint8_t *pci_config, uint16_t val)
@@ -339,6 +337,15 @@ void pci_qdev_register(const char *name, int size, pci_qdev_initfn init);
PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
+void pci_bridge_qdev_register(const char* name, int size,
+ pci_qdev_initfn init);
+
+void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq);
+
+PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, uint16_t vid,
+ uint16_t did, pci_map_irq_fn map_irq,
+ const char *pci_name);
+
/* lsi53c895a.c */
#define LSI_MAX_DEVS 7
void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id);
diff --git a/hw/pci_ids.h b/hw/pci_ids.h
index 427fcd5..7bc4853 100644
--- a/hw/pci_ids.h
+++ b/hw/pci_ids.h
@@ -93,3 +93,5 @@
#define PCI_DEVICE_ID_INTEL_82371AB 0x7111
#define PCI_DEVICE_ID_INTEL_82371AB_2 0x7112
#define PCI_DEVICE_ID_INTEL_82371AB_3 0x7113
+
+#define PCI_VENDOR_ID_INVALID 0xffff
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
` (5 preceding siblings ...)
2009-06-02 6:42 ` [Qemu-devel] [PATCH 6/7] pci/brdige qdevfy Isaku Yamahata
@ 2009-06-02 6:42 ` Isaku Yamahata
2009-06-02 7:13 ` [Qemu-devel] " Avi Kivity
2009-06-02 12:56 ` [Qemu-devel] " Markus Armbruster
6 siblings, 2 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 6:42 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, mtosatti, armbru, yamahata, paul, avi
This patch is preliminary for multi pci bus support to
add -pci option.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
changes
- qdevfied
---
Makefile.target | 2 +-
hw/apb_pci.c | 4 +-
hw/pc.c | 7 ++
hw/pci.c | 12 +++-
hw/pci.h | 2 +-
hw/pci_bridge.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/pci_bridge.h | 35 ++++++++
hw/pci_ids.h | 1 +
qemu-options.hx | 10 +++
vl.c | 5 +
10 files changed, 304 insertions(+), 6 deletions(-)
create mode 100644 hw/pci_bridge.c
create mode 100644 hw/pci_bridge.h
diff --git a/Makefile.target b/Makefile.target
index 445d55f..5eb1338 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -494,7 +494,7 @@ endif #CONFIG_BSD_USER
ifndef CONFIG_USER_ONLY
OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o \
- gdbstub.o gdbstub-xml.o
+ gdbstub.o gdbstub-xml.o pci_bridge.o
# virtio has to be here due to weird dependency between PCI and virtio-net.
# need to fix this properly
OBJS+=virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index e85e28c..21667a6 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -268,9 +268,9 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
/* APB secondary busses */
*bus2 = pci_bridge_create_simple(s->bus, 8, PCI_VENDOR_ID_SUN,
PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
- "Advanced PCI Bus secondary bridge 1");
+ "Advanced PCI Bus secondary bridge 1", 1);
*bus3 = pci_bridge_create_simple(s->bus, 9, PCI_VENDOR_ID_SUN,
PCI_DEVICE_ID_SUN_SIMBA, pci_apb_map_irq,
- "Advanced PCI Bus secondary bridge 2");
+ "Advanced PCI Bus secondary bridge 2", 2);
return s->bus;
}
diff --git a/hw/pc.c b/hw/pc.c
index 66cc780..894396f 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -25,6 +25,7 @@
#include "pc.h"
#include "fdc.h"
#include "pci.h"
+#include "pci_bridge.h"
#include "block.h"
#include "sysemu.h"
#include "audio/audio.h"
@@ -1041,6 +1042,12 @@ static void pc_init1(ram_addr_t ram_size,
}
}
+ if (pci_enabled) {
+ if (pci_device_init() < 0) {
+ exit(1);
+ }
+ }
+
watchdog_pc_init(pci_bus);
for(i = 0; i < nb_nics; i++) {
diff --git a/hw/pci.c b/hw/pci.c
index 0ffdfef..41e9930 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -129,7 +129,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
return bus;
}
-/* XXX qemu_irq, nirq */
+/* XXX qemu_irq */
static PCIBus *pci_register_secondary_bus(PCIDevice *dev,
int bus_num, int devfn_min, int nirq)
{
@@ -141,6 +141,13 @@ static PCIBus *pci_register_secondary_bus(PCIDevice *dev,
&dev->qdev, "pci"));
bus->bus_num = bus_num;
bus->devfn_min = devfn_min;
+ bus->nirq = nirq;
+
+#if 0
+ bus->set_irq = set_irq;
+ bus->irq_opaque = pic;
+ register_savevm("PCIBUS", nbus++, 1, pcibus_save, pcibus_load, bus);
+#endif
parent_bus = pci_get_parent_bus(dev);
LIST_INSERT_AFTER(parent_bus, bus, next);
@@ -1130,7 +1137,7 @@ device_init(pci_brdige_register_device);
PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, uint16_t vid,
uint16_t did, pci_map_irq_fn map_irq,
- const char *pci_name)
+ const char *pci_name, int sec_bus_num)
{
DeviceState *qdev;
uint8_t* pci_conf;
@@ -1142,6 +1149,7 @@ PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, uint16_t vid,
qdev_set_prop_int(qdev, "devfn", devfn);
qdev_set_prop_ptr(qdev, "pci_name", pci_name_dup);
+ qdev_set_prop_int(qdev, "sec_bus_num", sec_bus_num);
qdev_init(qdev);
diff --git a/hw/pci.h b/hw/pci.h
index 76f3f28..95ff3d2 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -344,7 +344,7 @@ void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq);
PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, uint16_t vid,
uint16_t did, pci_map_irq_fn map_irq,
- const char *pci_name);
+ const char *pci_name, int sec_bus_num);
/* lsi53c895a.c */
#define LSI_MAX_DEVS 7
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
new file mode 100644
index 0000000..3aedaa1
--- /dev/null
+++ b/hw/pci_bridge.c
@@ -0,0 +1,232 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ * VA Linux Systems Japan K.K.
+ *
+ */
+
+#include <stdlib.h>
+
+#include "sysemu.h"
+#include "hw.h"
+#include "pci.h"
+#include "pci_bridge.h"
+
+#define PCI_BRIDGE_INTEL_82801BA_11 "i82801ba11"
+static int i82801ba11_map_irq(PCIDevice *pci_dev, int irq_num)
+{
+ return (PCI_SLOT(pci_dev->devfn) + irq_num + 1) % 4;
+}
+
+static void i82801ba11_qdev_init(PCIDevice *d)
+{
+ PCIBus *b;
+ uint8_t *pci_conf;
+
+ pci_conf = d->config;
+
+ pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
+ pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82801BA_11);
+ b = pci_get_parent_bus(d);
+ pci_bridge_set_map_irq(b, i82801ba11_map_irq);
+}
+
+static void i82801ba11_register_device(void)
+{
+ pci_bridge_qdev_register(PCI_BRIDGE_INTEL_82801BA_11, sizeof(PCIDevice),
+ i82801ba11_qdev_init);
+}
+
+device_init(i82801ba11_register_device);
+
+struct PCIDeviceInfo {
+ int dom;
+ int bus;
+ unsigned int slot;
+ unsigned int func;
+ int secondary_bus;
+
+ const char *model;
+ const char *options;
+};
+
+#define MAX_PCI_DEVS 256
+static int nb_pci_devices;
+static struct PCIDeviceInfo pd_table[MAX_PCI_DEVS];
+
+/*
+ * Parse [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
+ */
+static int pci_parse_devfn(const char *addr,
+ int *domp, int *busp,
+ unsigned int *slotp, unsigned int *funcp)
+{
+ const char *p;
+ char *e;
+ unsigned long val;
+ unsigned long dom = 0;
+ unsigned long bus = 0;
+ unsigned int slot = 0;
+ unsigned int func = 0;
+
+ p = addr;
+ val = strtoul(p, &e, 16);
+ if (e == p)
+ return -1;
+ if (*e == ':') {
+ bus = val;
+ p = e + 1;
+ val = strtoul(p, &e, 16);
+ if (e == p)
+ return -1;
+ if (*e == ':') {
+ dom = bus;
+ bus = val;
+ p = e + 1;
+ val = strtoul(p, &e, 16);
+ if (e == p)
+ return -1;
+ }
+ }
+ slot = val;
+
+ if (*e != '.')
+ return -1;
+ p = e + 1;
+ val = strtoul(p, &e, 16);
+ if (e == p)
+ return -1;
+ func = val;
+
+ if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
+ return -1;
+
+ /* For now, only 0 domain is supported */
+ if (dom != 0)
+ return -1;
+
+ *domp = dom;
+ *busp = bus;
+ *slotp = slot;
+ *funcp = func;
+ return 0;
+}
+
+int pci_device_parse(const char* options)
+{
+ int ret = -1;
+ char buf[1024];
+ char *e;
+ struct PCIDeviceInfo *pd = &pd_table[nb_pci_devices];
+
+ if (nb_pci_devices >= MAX_PCI_DEVS) {
+ /* XXX:WARN */
+ goto out;
+ }
+
+
+ if (get_param_value(buf, sizeof(buf), "pci_addr", options) < 0) {
+ /* XXX error */
+ goto out;
+ }
+ if (pci_parse_devfn(buf, &pd->dom, &pd->bus, &pd->slot, &pd->func) < 0) {
+ goto out;
+ }
+
+ if (get_param_value(buf, sizeof(buf), "secondary", options) < 0) {
+ goto out;
+ }
+ pd->secondary_bus = strtoul(buf, &e, 16);
+ if (buf == e) {
+ goto out;
+ }
+ if (pd->secondary_bus > 0xff) {
+ goto out;
+ }
+
+ if (get_param_value(buf, sizeof(buf), "model", options) < 0) {
+ goto out;
+ }
+ pd->model = strdup(buf);
+ if (pd->model == NULL) {
+ goto out;
+ }
+
+ /* save for later use */
+ pd->options = strdup(options);
+ if (pd->options == NULL) {
+ goto out;
+ }
+
+ nb_pci_devices++;
+ ret = 0;
+
+out:
+ return ret;
+}
+
+static int pci_device_init_one(struct PCIDeviceInfo *pd)
+{
+ PCIBus *parent_bus;
+ PCIBus *bus;
+
+ if (pci_find_device(pd->bus, pd->slot, pd->func) != NULL) {
+ return -1;
+ }
+
+ parent_bus = pci_find_bus(pd->bus);
+ if (parent_bus == NULL) {
+ return -1;
+ }
+
+ bus = pci_bridge_create_simple(parent_bus, PCI_DEVFN(pd->slot, pd->func),
+ PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_82801BA_11,
+ &i82801ba11_map_irq, "PCI to PCI Bridge",
+ pd->secondary_bus);
+ return 0;
+}
+
+int pci_device_init(void)
+{
+ int i;
+ int ret = 0;
+
+ for (i = 0; i < nb_pci_devices; i++) {
+ struct PCIDeviceInfo *pd = &pd_table[i];
+ const char *model = pd->model;
+
+ if (!strcmp(model, "bridge")) {
+ ret = pci_device_init_one(pd);
+ } else {
+ /* unknown model */
+ ret = -1;
+ }
+
+ if (ret < 0)
+ break;
+ }
+ return ret;
+}
+
+/*
+ * Local variables:
+ * c-indent-level: 4
+ * c-basic-offset: 4
+ * tab-width: 8
+ * indent-tab-mode: nil
+ * End:
+ */
diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
new file mode 100644
index 0000000..90037f0
--- /dev/null
+++ b/hw/pci_bridge.h
@@ -0,0 +1,35 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ * VA Linux Systems Japan K.K.
+ *
+ */
+
+#ifndef QEMU_PCI_BRIDGE_H
+#define QEMU_PCI_BRIDGE_H
+
+int pci_device_parse(const char* options);
+int pci_device_init(void);
+
+#endif /* QEMU_PCI_BRIDGE_H */
+/*
+ * Local variables:
+ * c-indent-level: 4
+ * c-basic-offset: 4
+ * tab-width: 8
+ * indent-tab-mode: nil
+ * End:
+ */
diff --git a/hw/pci_ids.h b/hw/pci_ids.h
index 7bc4853..b226373 100644
--- a/hw/pci_ids.h
+++ b/hw/pci_ids.h
@@ -86,6 +86,7 @@
#define PCI_VENDOR_ID_INTEL 0x8086
#define PCI_DEVICE_ID_INTEL_82441 0x1237
#define PCI_DEVICE_ID_INTEL_82801AA_5 0x2415
+#define PCI_DEVICE_ID_INTEL_82801BA_11 0x244e
#define PCI_DEVICE_ID_INTEL_82371SB_0 0x7000
#define PCI_DEVICE_ID_INTEL_82371SB_1 0x7010
#define PCI_DEVICE_ID_INTEL_82371SB_2 0x7020
diff --git a/qemu-options.hx b/qemu-options.hx
index 87af798..c5be603 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -886,6 +886,16 @@ override the default configuration (@option{-net nic -net user}) which
is activated if no @option{-net} options are provided.
ETEXI
+DEF("pci", HAS_ARG, QEMU_OPTION_pci, \
+ "-pci pci_addr=pci_addr,model=type[,model specific options]\n"
+ "pci_addr = [[<domain>:]<bus>:]<slot>.<func>\n"
+ "model = bridge\n")
+STEXI
+@item -pci pci_addr=@var{pci_addr},model=@var{type}[,@var{model specific options}]
+connect pci device of model @var{type} at pci bus address of @var{pci_addr}
+with model specific options.
+ETEXI
+
#ifdef CONFIG_SLIRP
DEF("tftp", HAS_ARG, QEMU_OPTION_tftp, \
"-tftp dir allow tftp access to files in dir [-net user]\n")
diff --git a/vl.c b/vl.c
index f8c0d00..0082250 100644
--- a/vl.c
+++ b/vl.c
@@ -134,6 +134,7 @@ int main(int argc, char **argv)
#include "hw/usb.h"
#include "hw/pcmcia.h"
#include "hw/pc.h"
+#include "hw/pci_bridge.h"
#include "hw/audiodev.h"
#include "hw/isa.h"
#include "hw/baum.h"
@@ -5139,6 +5140,10 @@ int main(int argc, char **argv, char **envp)
net_clients[nb_net_clients] = optarg;
nb_net_clients++;
break;
+ case QEMU_OPTION_pci:
+ if (pci_device_parse(optarg) < 0)
+ exit(1);
+ break;
#ifdef CONFIG_SLIRP
case QEMU_OPTION_tftp:
tftp_prefix = optarg;
--
1.6.0.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 6:42 ` [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support Isaku Yamahata
@ 2009-06-02 7:13 ` Avi Kivity
2009-06-02 7:46 ` Isaku Yamahata
2009-06-02 12:56 ` [Qemu-devel] " Markus Armbruster
1 sibling, 1 reply; 23+ messages in thread
From: Avi Kivity @ 2009-06-02 7:13 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: mst, mtosatti, qemu-devel, armbru, paul
Isaku Yamahata wrote:
> This patch is preliminary for multi pci bus support to
> add -pci option.
>
What is the motivation for this work?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 7:13 ` [Qemu-devel] " Avi Kivity
@ 2009-06-02 7:46 ` Isaku Yamahata
2009-06-02 8:51 ` Avi Kivity
2009-06-02 13:03 ` Markus Armbruster
0 siblings, 2 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-02 7:46 UTC (permalink / raw)
To: Avi Kivity; +Cc: mst, mtosatti, qemu-devel, armbru, paul
On Tue, Jun 02, 2009 at 10:13:19AM +0300, Avi Kivity wrote:
> Isaku Yamahata wrote:
> >This patch is preliminary for multi pci bus support to
> >add -pci option.
> >
>
> What is the motivation for this work?
-pci might be too generic. -pci_bridge or something else might be
better. I think, eventually they would be replaced with config file work.
So this option isn't so important.
The short term motivation is 128+ pci slot support.
I know Markus has tried on kvm before and was rejected because
of scalability. As he also wants it, I'm willing to collaborate with him.
The long term motivation is to support MMCFG and PCIe port emulator,
then eventually PCIe native direct attach support including
PCIe native functionality like AER.
This requires more newer chipset emulation than piix and more.
--
yamahata
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 7:46 ` Isaku Yamahata
@ 2009-06-02 8:51 ` Avi Kivity
2009-06-02 13:03 ` Markus Armbruster
1 sibling, 0 replies; 23+ messages in thread
From: Avi Kivity @ 2009-06-02 8:51 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: armbru, paul, mtosatti, qemu-devel, mst
Isaku Yamahata wrote:
> On Tue, Jun 02, 2009 at 10:13:19AM +0300, Avi Kivity wrote:
>
>> Isaku Yamahata wrote:
>>
>>> This patch is preliminary for multi pci bus support to
>>> add -pci option.
>>>
>>>
>> What is the motivation for this work?
>>
>
> -pci might be too generic. -pci_bridge or something else might be
> better. I think, eventually they would be replaced with config file work.
> So this option isn't so important.
>
> The short term motivation is 128+ pci slot support.
> I know Markus has tried on kvm before and was rejected because
> of scalability. As he also wants it, I'm willing to collaborate with him.
>
Thanks for the explanation.
> The long term motivation is to support MMCFG and PCIe port emulator,
> then eventually PCIe native direct attach support including
> PCIe native functionality like AER.
> This requires more newer chipset emulation than piix and more.
>
Yes, this is definitely interesting.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-02 6:42 ` [Qemu-devel] [PATCH 3/7] pci: pci_default_config_write() clean up Isaku Yamahata
@ 2009-06-02 10:01 ` Michael S. Tsirkin
2009-06-03 2:31 ` Isaku Yamahata
0 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-02 10:01 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: mtosatti, qemu-devel, armbru, paul, avi
On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> +struct PCIConfigReg {
> + uint8_t wmask;
> + /* offset of registers in bits for 2/4 bytes function register */
> + uint8_t reg_offset;
Sorry about being dense, but the comment still doesn't help me much.
Can't we simply use the index in the array as offset?
> + pci_config_written_t callback;
> +};
>
> struct PCIDevice {
> DeviceState qdev;
> /* PCI config space */
> uint8_t config[PCI_CONFIG_SPACE_SIZE];
> -
> - /* Used to implement R/W bytes */
> - uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
I still think separate mask/config/callback arrays
are better - they are easier for devices to use. E.g. a single memset
can make a range of register writeable, and a single function call
does everything necessary to save a range the whole config space.
Add a callback array if you like, and be done with it.
>
> /* the following fields are read only */
> PCIBus *bus;
> @@ -180,6 +261,21 @@ struct PCIDevice {
> int irq_state[4];
> };
>
> +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> +
> +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> + pci_config_written_t callback, uint32_t wmask);
> +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> + pci_config_written_t callback, uint32_t wmask);
> +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> + pci_config_written_t callback, uint32_t wmask);
If we got rid of reg_offset, I think we won't need these.
We'd just do dev->callback[REGISTER] = my_callback.
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 6:42 ` [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support Isaku Yamahata
2009-06-02 7:13 ` [Qemu-devel] " Avi Kivity
@ 2009-06-02 12:56 ` Markus Armbruster
1 sibling, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2009-06-02 12:56 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: paul, mtosatti, avi, qemu-devel, mst
Isaku Yamahata <yamahata@valinux.co.jp> writes:
> This patch is preliminary for multi pci bus support to
> add -pci option.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> ---
[...]
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> new file mode 100644
> index 0000000..3aedaa1
> --- /dev/null
> +++ b/hw/pci_bridge.c
> @@ -0,0 +1,232 @@
[...]
> +/*
> + * Parse [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
> + */
> +static int pci_parse_devfn(const char *addr,
> + int *domp, int *busp,
> + unsigned int *slotp, unsigned int *funcp)
> +{
> + const char *p;
> + char *e;
> + unsigned long val;
> + unsigned long dom = 0;
> + unsigned long bus = 0;
> + unsigned int slot = 0;
> + unsigned int func = 0;
> +
> + p = addr;
> + val = strtoul(p, &e, 16);
> + if (e == p)
> + return -1;
> + if (*e == ':') {
> + bus = val;
> + p = e + 1;
> + val = strtoul(p, &e, 16);
> + if (e == p)
> + return -1;
> + if (*e == ':') {
> + dom = bus;
> + bus = val;
> + p = e + 1;
> + val = strtoul(p, &e, 16);
> + if (e == p)
> + return -1;
> + }
> + }
> + slot = val;
> +
> + if (*e != '.')
> + return -1;
> + p = e + 1;
> + val = strtoul(p, &e, 16);
> + if (e == p)
> + return -1;
> + func = val;
> +
> + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
> + return -1;
> +
> + /* For now, only 0 domain is supported */
> + if (dom != 0)
> + return -1;
> +
> + *domp = dom;
> + *busp = bus;
> + *slotp = slot;
> + *funcp = func;
> + return 0;
> +}
This duplicates much of pci_parse_devaddr() from hw/pci.c. I suggest to
extend that function instead: make it take a funcp parameter, and parse
the ".<func>" part iff funcp != NULL.
> +
> +int pci_device_parse(const char* options)
Style nitpick: char *options
> +{
> + int ret = -1;
> + char buf[1024];
> + char *e;
> + struct PCIDeviceInfo *pd = &pd_table[nb_pci_devices];
> +
> + if (nb_pci_devices >= MAX_PCI_DEVS) {
> + /* XXX:WARN */
> + goto out;
> + }
> +
> +
> + if (get_param_value(buf, sizeof(buf), "pci_addr", options) < 0) {
> + /* XXX error */
> + goto out;
> + }
> + if (pci_parse_devfn(buf, &pd->dom, &pd->bus, &pd->slot, &pd->func) < 0) {
> + goto out;
> + }
> +
> + if (get_param_value(buf, sizeof(buf), "secondary", options) < 0) {
> + goto out;
> + }
> + pd->secondary_bus = strtoul(buf, &e, 16);
> + if (buf == e) {
> + goto out;
> + }
> + if (pd->secondary_bus > 0xff) {
> + goto out;
> + }
> +
> + if (get_param_value(buf, sizeof(buf), "model", options) < 0) {
> + goto out;
> + }
> + pd->model = strdup(buf);
> + if (pd->model == NULL) {
> + goto out;
> + }
> +
> + /* save for later use */
> + pd->options = strdup(options);
> + if (pd->options == NULL) {
> + goto out;
> + }
> +
> + nb_pci_devices++;
> + ret = 0;
> +
> +out:
> + return ret;
> +}
> +
> +static int pci_device_init_one(struct PCIDeviceInfo *pd)
> +{
> + PCIBus *parent_bus;
> + PCIBus *bus;
> +
> + if (pci_find_device(pd->bus, pd->slot, pd->func) != NULL) {
> + return -1;
> + }
> +
> + parent_bus = pci_find_bus(pd->bus);
> + if (parent_bus == NULL) {
> + return -1;
> + }
> +
> + bus = pci_bridge_create_simple(parent_bus, PCI_DEVFN(pd->slot, pd->func),
> + PCI_VENDOR_ID_INTEL,
> + PCI_DEVICE_ID_INTEL_82801BA_11,
> + &i82801ba11_map_irq, "PCI to PCI Bridge",
> + pd->secondary_bus);
> + return 0;
> +}
> +
> +int pci_device_init(void)
> +{
> + int i;
> + int ret = 0;
> +
> + for (i = 0; i < nb_pci_devices; i++) {
> + struct PCIDeviceInfo *pd = &pd_table[i];
> + const char *model = pd->model;
> +
> + if (!strcmp(model, "bridge")) {
> + ret = pci_device_init_one(pd);
> + } else {
> + /* unknown model */
> + ret = -1;
I think we better print suitable diagnostics here.
> + }
> +
> + if (ret < 0)
> + break;
> + }
> + return ret;
> +}
> +
> +/*
> + * Local variables:
> + * c-indent-level: 4
> + * c-basic-offset: 4
> + * tab-width: 8
> + * indent-tab-mode: nil
> + * End:
> + */
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 87af798..c5be603 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -886,6 +886,16 @@ override the default configuration (@option{-net nic -net user}) which
> is activated if no @option{-net} options are provided.
> ETEXI
>
> +DEF("pci", HAS_ARG, QEMU_OPTION_pci, \
> + "-pci pci_addr=pci_addr,model=type[,model specific options]\n"
> + "pci_addr = [[<domain>:]<bus>:]<slot>.<func>\n"
> + "model = bridge\n")
> +STEXI
> +@item -pci pci_addr=@var{pci_addr},model=@var{type}[,@var{model specific options}]
> +connect pci device of model @var{type} at pci bus address of @var{pci_addr}
> +with model specific options.
> +ETEXI
> +
I find pci_addr a bit unfortunate (which of the various addresses
associated with a PCI device is it?), but it follows the precedent set
by monitor commands drive_add, pci_add, pci_del.
> #ifdef CONFIG_SLIRP
> DEF("tftp", HAS_ARG, QEMU_OPTION_tftp, \
> "-tftp dir allow tftp access to files in dir [-net user]\n")
> diff --git a/vl.c b/vl.c
> index f8c0d00..0082250 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -134,6 +134,7 @@ int main(int argc, char **argv)
> #include "hw/usb.h"
> #include "hw/pcmcia.h"
> #include "hw/pc.h"
> +#include "hw/pci_bridge.h"
> #include "hw/audiodev.h"
> #include "hw/isa.h"
> #include "hw/baum.h"
> @@ -5139,6 +5140,10 @@ int main(int argc, char **argv, char **envp)
> net_clients[nb_net_clients] = optarg;
> nb_net_clients++;
> break;
> + case QEMU_OPTION_pci:
> + if (pci_device_parse(optarg) < 0)
> + exit(1);
> + break;
> #ifdef CONFIG_SLIRP
> case QEMU_OPTION_tftp:
> tftp_prefix = optarg;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.
2009-06-02 7:46 ` Isaku Yamahata
2009-06-02 8:51 ` Avi Kivity
@ 2009-06-02 13:03 ` Markus Armbruster
1 sibling, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2009-06-02 13:03 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel, paul, mtosatti, Avi Kivity, mst
Isaku Yamahata <yamahata@valinux.co.jp> writes:
> On Tue, Jun 02, 2009 at 10:13:19AM +0300, Avi Kivity wrote:
>> Isaku Yamahata wrote:
>> >This patch is preliminary for multi pci bus support to
>> >add -pci option.
>> >
>>
>> What is the motivation for this work?
>
> -pci might be too generic. -pci_bridge or something else might be
> better. I think, eventually they would be replaced with config file work.
> So this option isn't so important.
To make use of this code, we need a way to create PCI bridges. If we
can't have an option, maybe we could create bridges as needed, just like
we create SCSI controllers. But I don't like that at all.
I wouldn't advise waiting for completion of the config file stuff.
> The short term motivation is 128+ pci slot support.
Sorely needed, in my opinion.
> I know Markus has tried on kvm before and was rejected because
> of scalability. As he also wants it, I'm willing to collaborate with him.
>
> The long term motivation is to support MMCFG and PCIe port emulator,
> then eventually PCIe native direct attach support including
> PCIe native functionality like AER.
> This requires more newer chipset emulation than piix and more.
Good stuff.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-02 10:01 ` [Qemu-devel] " Michael S. Tsirkin
@ 2009-06-03 2:31 ` Isaku Yamahata
2009-06-03 7:22 ` Michael S. Tsirkin
2009-06-10 15:48 ` Michael S. Tsirkin
0 siblings, 2 replies; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-03 2:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > +struct PCIConfigReg {
> > + uint8_t wmask;
> > + /* offset of registers in bits for 2/4 bytes function register */
> > + uint8_t reg_offset;
>
> Sorry about being dense, but the comment still doesn't help me much.
> Can't we simply use the index in the array as offset?
No. I believe this is helpfull.
the next patch for hw/wdt_i6300esb.c is a good example.
With this, we can replace fragile address and len comparison
with one callback per one register function.
For that, the member which represents the position in function
is necessary.
>
> > + pci_config_written_t callback;
> > +};
> >
> > struct PCIDevice {
> > DeviceState qdev;
> > /* PCI config space */
> > uint8_t config[PCI_CONFIG_SPACE_SIZE];
> > -
> > - /* Used to implement R/W bytes */
> > - uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> > + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
>
> I still think separate mask/config/callback arrays
> are better - they are easier for devices to use. E.g. a single memset
> can make a range of register writeable, and a single function call
> does everything necessary to save a range the whole config space.
>
> Add a callback array if you like, and be done with it.
Hmm, I don't think so. Maybe it's a matter of taste.
Looking at your MSI/MSI-X patches, our patch series conflict with
each other. That's bad. Let's resolve the conflicts.
- mask v.s. wmask
I think mask is ambiguous because which bits it represents,
writable bits or read only bits.
let's rename it to common name.
wmask, w_mask, wr_mask, writable_mask or something like that.
What name do you prefer?
- array v.s. struct
I suppose this conflicts with you.
So after renaming, I'll make wmask into uint8_t wmask[]
(or whatever name we choose)
Then, we can avoid stepping on each other.
What do you think?
>
> >
> > /* the following fields are read only */
> > PCIBus *bus;
> > @@ -180,6 +261,21 @@ struct PCIDevice {
> > int irq_state[4];
> > };
> >
> > +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> > +
> > +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> > + pci_config_written_t callback, uint32_t wmask);
> > +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> > + pci_config_written_t callback, uint32_t wmask);
> > +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> > + pci_config_written_t callback, uint32_t wmask);
>
> If we got rid of reg_offset, I think we won't need these.
> We'd just do dev->callback[REGISTER] = my_callback.
>
>
--
yamahata
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-03 2:31 ` Isaku Yamahata
@ 2009-06-03 7:22 ` Michael S. Tsirkin
2009-06-03 12:25 ` Isaku Yamahata
2009-06-10 15:48 ` Michael S. Tsirkin
1 sibling, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-03 7:22 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote:
> On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > > +struct PCIConfigReg {
> > > + uint8_t wmask;
> > > + /* offset of registers in bits for 2/4 bytes function register */
> > > + uint8_t reg_offset;
> >
> > Sorry about being dense, but the comment still doesn't help me much.
> > Can't we simply use the index in the array as offset?
>
> No. I believe this is helpfull.
> the next patch for hw/wdt_i6300esb.c is a good example.
> With this, we can replace fragile address and len comparison
> with one callback per one register function.
> For that, the member which represents the position in function
> is necessary.
>
> >
> > > + pci_config_written_t callback;
> > > +};
> > >
> > > struct PCIDevice {
> > > DeviceState qdev;
> > > /* PCI config space */
> > > uint8_t config[PCI_CONFIG_SPACE_SIZE];
> > > -
> > > - /* Used to implement R/W bytes */
> > > - uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> > > + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
> >
> > I still think separate mask/config/callback arrays
> > are better - they are easier for devices to use. E.g. a single memset
> > can make a range of register writeable, and a single function call
> > does everything necessary to save a range the whole config space.
> >
> > Add a callback array if you like, and be done with it.
>
> Hmm, I don't think so. Maybe it's a matter of taste.
> Looking at your MSI/MSI-X patches, our patch series conflict with
> each other. That's bad. Let's resolve the conflicts.
>
> - mask v.s. wmask
> I think mask is ambiguous because which bits it represents,
> writable bits or read only bits.
> let's rename it to common name.
> wmask, w_mask, wr_mask, writable_mask or something like that.
> What name do you prefer?
Not terribly important. I guess I like wmask if you let me pick.
> - array v.s. struct
> I suppose this conflicts with you.
> So after renaming, I'll make wmask into uint8_t wmask[]
> (or whatever name we choose)
>
> Then, we can avoid stepping on each other.
> What do you think?
Agreed. So what I need to do is rename mask into wmask, is that right?
>
> >
> > >
> > > /* the following fields are read only */
> > > PCIBus *bus;
> > > @@ -180,6 +261,21 @@ struct PCIDevice {
> > > int irq_state[4];
> > > };
> > >
> > > +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> > > +
> > > +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> > > +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> > > +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> >
> > If we got rid of reg_offset, I think we won't need these.
> > We'd just do dev->callback[REGISTER] = my_callback.
> >
> >
>
> --
> yamahata
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-03 7:22 ` Michael S. Tsirkin
@ 2009-06-03 12:25 ` Isaku Yamahata
2009-06-05 10:43 ` Michael S. Tsirkin
0 siblings, 1 reply; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-03 12:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Wed, Jun 03, 2009 at 10:22:39AM +0300, Michael S. Tsirkin wrote:
> Agreed. So what I need to do is rename mask into wmask, is that right?
Yes. That's right
--
yamahata
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-03 12:25 ` Isaku Yamahata
@ 2009-06-05 10:43 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-05 10:43 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel, armbru, mtosatti, paul, avi
On Wed, Jun 03, 2009 at 09:25:46PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 03, 2009 at 10:22:39AM +0300, Michael S. Tsirkin wrote:
> > Agreed. So what I need to do is rename mask into wmask, is that right?
>
> Yes. That's right
>
OK, I did that. HTH.
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 1/7] vmware_vga: clean up
2009-06-02 6:42 ` [Qemu-devel] [PATCH 1/7] vmware_vga: clean up Isaku Yamahata
@ 2009-06-10 15:08 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-10 15:08 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: mtosatti, qemu-devel, armbru, paul, avi
On Tue, Jun 02, 2009 at 03:42:44PM +0900, Isaku Yamahata wrote:
> use NULL instead of 0 for pci_register_device() argument
> for consistency. Any other caller uses NULL.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
ACK.
Isn't this independend of other patches in the series?
If yes, it's probably better to post them separately.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/vmware_vga.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 79da1ff..ee4f10a 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1217,7 +1217,7 @@ void pci_vmsvga_init(PCIBus *bus)
> /* Setup PCI configuration */
> s = (struct pci_vmsvga_state_s *)
> pci_register_device(bus, "QEMUware SVGA",
> - sizeof(struct pci_vmsvga_state_s), -1, 0, 0);
> + sizeof(struct pci_vmsvga_state_s), -1, NULL, NULL);
> pci_config_set_vendor_id(s->card.config, PCI_VENDOR_ID_VMWARE);
> pci_config_set_device_id(s->card.config, SVGA_PCI_DEVICE_ID);
> s->card.config[PCI_COMMAND] = 0x07; /* I/O + Memory */
> --
> 1.6.0.2
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 4/7] pci/config: convert pci configuration space handler to use callback.
2009-06-02 6:42 ` [Qemu-devel] [PATCH 4/7] pci/config: convert pci configuration space handler to use callback Isaku Yamahata
@ 2009-06-10 15:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-10 15:16 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: mtosatti, qemu-devel, armbru, paul, avi
On Tue, Jun 02, 2009 at 03:42:47PM +0900, Isaku Yamahata wrote:
> convert pci configuration space handler into callbacks.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> changes v5
> - typo
> - don't change cirrus_vga.c behaviour.
> ---
> hw/acpi.c | 11 ++++-----
> hw/cirrus_vga.c | 15 ++++++++++-
> hw/gt64xxx.c | 13 +---------
> hw/piix_pci.c | 18 +++++++++-----
> hw/vga.c | 10 +++++---
> hw/wdt_i6300esb.c | 66 +++++++++++++++++++++-------------------------------
> 6 files changed, 63 insertions(+), 70 deletions(-)
It might be a good idea to split this up, and handle each device
in a separate patch.
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index ce3ffe2..0790ef1 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1083,17 +1083,6 @@ static void gt64120_reset(void *opaque)
> gt64120_pci_mapping(s);
> }
>
> -static uint32_t gt64120_read_config(PCIDevice *d, uint32_t address, int len)
> -{
> - return pci_default_read_config(d, address, len);
> -}
> -
> -static void gt64120_write_config(PCIDevice *d, uint32_t address, uint32_t val,
> - int len)
> -{
> - pci_default_write_config(d, address, val, len);
> -}
> -
> static void gt64120_save(QEMUFile* f, void *opaque)
> {
> PCIDevice *d = opaque;
> @@ -1133,7 +1122,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
> pic, 144, 4);
> s->ISD_handle = cpu_register_io_memory(0, gt64120_read, gt64120_write, s);
> d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
> - 0, gt64120_read_config, gt64120_write_config);
> + 0, NULL, NULL);
>
> /* FIXME: Malta specific hw assumptions ahead */
>
This seems a trivially correct cleanup that can be done independently of
the rest of the series.
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-03 2:31 ` Isaku Yamahata
2009-06-03 7:22 ` Michael S. Tsirkin
@ 2009-06-10 15:48 ` Michael S. Tsirkin
2009-06-15 9:12 ` Isaku Yamahata
1 sibling, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-10 15:48 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote:
> On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > > +struct PCIConfigReg {
> > > + uint8_t wmask;
> > > + /* offset of registers in bits for 2/4 bytes function register */
> > > + uint8_t reg_offset;
> >
> > Sorry about being dense, but the comment still doesn't help me much.
> > Can't we simply use the index in the array as offset?
>
> No. I believe this is helpfull.
> the next patch for hw/wdt_i6300esb.c is a good example.
> With this, we can replace fragile address and len comparison
> with one callback per one register function.
>
> For that, the member which represents the position in function
> is necessary.
So maybe this is going too far into a table-driven direction then.
Tables are good for common case, exceptions are better handled
by regular functional design.
I agree addr/len comparisons are fragile, but can't we simply implement
functions to encapsulate them? Along the lines of:
static inline int offset_in_range(int offset, int address, int len)
{
return address <= offset && address + len > offset;
}
static inline int ranges_match(int addr1, int len1,
int addr2, int len2)
{
return offset_affected(addr1, addr2, len2) ||
offset_affected(addr2, addr1, len1);
}
Switching address and len comparison to use this would be a good cleanup
IMO.
>
> >
> > > + pci_config_written_t callback;
> > > +};
> > >
> > > struct PCIDevice {
> > > DeviceState qdev;
> > > /* PCI config space */
> > > uint8_t config[PCI_CONFIG_SPACE_SIZE];
> > > -
> > > - /* Used to implement R/W bytes */
> > > - uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> > > + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
> >
> > I still think separate mask/config/callback arrays
> > are better - they are easier for devices to use. E.g. a single memset
> > can make a range of register writeable, and a single function call
> > does everything necessary to save a range the whole config space.
> >
> > Add a callback array if you like, and be done with it.
>
> Hmm, I don't think so. Maybe it's a matter of taste.
> Looking at your MSI/MSI-X patches, our patch series conflict with
> each other. That's bad. Let's resolve the conflicts.
>
> - mask v.s. wmask
> I think mask is ambiguous because which bits it represents,
> writable bits or read only bits.
> let's rename it to common name.
> wmask, w_mask, wr_mask, writable_mask or something like that.
> What name do you prefer?
>
> - array v.s. struct
> I suppose this conflicts with you.
> So after renaming, I'll make wmask into uint8_t wmask[]
> (or whatever name we choose)
>
> Then, we can avoid stepping on each other.
> What do you think?
>
>
> >
> > >
> > > /* the following fields are read only */
> > > PCIBus *bus;
> > > @@ -180,6 +261,21 @@ struct PCIDevice {
> > > int irq_state[4];
> > > };
> > >
> > > +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> > > +
> > > +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> > > +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> > > +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> > > + pci_config_written_t callback, uint32_t wmask);
> >
> > If we got rid of reg_offset, I think we won't need these.
> > We'd just do dev->callback[REGISTER] = my_callback.
> >
> >
>
> --
> yamahata
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-10 15:48 ` Michael S. Tsirkin
@ 2009-06-15 9:12 ` Isaku Yamahata
2009-06-15 10:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 23+ messages in thread
From: Isaku Yamahata @ 2009-06-15 9:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Wed, Jun 10, 2009 at 06:48:06PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote:
> > On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > > > +struct PCIConfigReg {
> > > > + uint8_t wmask;
> > > > + /* offset of registers in bits for 2/4 bytes function register */
> > > > + uint8_t reg_offset;
> > >
> > > Sorry about being dense, but the comment still doesn't help me much.
> > > Can't we simply use the index in the array as offset?
> >
> > No. I believe this is helpfull.
> > the next patch for hw/wdt_i6300esb.c is a good example.
> > With this, we can replace fragile address and len comparison
> > with one callback per one register function.
> >
> > For that, the member which represents the position in function
> > is necessary.
>
> So maybe this is going too far into a table-driven direction then.
> Tables are good for common case, exceptions are better handled
> by regular functional design.
>
> I agree addr/len comparisons are fragile, but can't we simply implement
> functions to encapsulate them? Along the lines of:
>
> static inline int offset_in_range(int offset, int address, int len)
> {
> return address <= offset && address + len > offset;
> }
>
> static inline int ranges_match(int addr1, int len1,
> int addr2, int len2)
> {
> return offset_affected(addr1, addr2, len2) ||
> offset_affected(addr2, addr1, len1);
> }
>
> Switching address and len comparison to use this would be a good cleanup
> IMO.
Introducing helper function sounds a good idea.
I suppose that reg_offset and related functions can be removed
by helper functions.
So new callback function type would be
typedef void (*pci_config_written_t)(struct PCIDevice *d,
uint32_t addr, uint32_t val, int len);
Since this is same as PCIConfigWriteFunc, pci_config_written_t would
be removed with the next version.
thanks,
--
yamahata
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.
2009-06-15 9:12 ` Isaku Yamahata
@ 2009-06-15 10:42 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2009-06-15 10:42 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: paul, mtosatti, avi, qemu-devel, armbru
On Mon, Jun 15, 2009 at 06:12:04PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 10, 2009 at 06:48:06PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote:
> > > On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > > > > +struct PCIConfigReg {
> > > > > + uint8_t wmask;
> > > > > + /* offset of registers in bits for 2/4 bytes function register */
> > > > > + uint8_t reg_offset;
> > > >
> > > > Sorry about being dense, but the comment still doesn't help me much.
> > > > Can't we simply use the index in the array as offset?
> > >
> > > No. I believe this is helpfull.
> > > the next patch for hw/wdt_i6300esb.c is a good example.
> > > With this, we can replace fragile address and len comparison
> > > with one callback per one register function.
> > >
> > > For that, the member which represents the position in function
> > > is necessary.
> >
> > So maybe this is going too far into a table-driven direction then.
> > Tables are good for common case, exceptions are better handled
> > by regular functional design.
> >
> > I agree addr/len comparisons are fragile, but can't we simply implement
> > functions to encapsulate them? Along the lines of:
> >
> > static inline int offset_in_range(int offset, int address, int len)
> > {
> > return address <= offset && address + len > offset;
> > }
> >
> > static inline int ranges_match(int addr1, int len1,
> > int addr2, int len2)
> > {
> > return offset_affected(addr1, addr2, len2) ||
> > offset_affected(addr2, addr1, len1);
> > }
> >
> > Switching address and len comparison to use this would be a good cleanup
> > IMO.
>
> Introducing helper function sounds a good idea.
Should be a separate patch IMO.
> I suppose that reg_offset and related functions can be removed
> by helper functions.
> So new callback function type would be
>
> typedef void (*pci_config_written_t)(struct PCIDevice *d,
> uint32_t addr, uint32_t val, int len);
>
> Since this is same as PCIConfigWriteFunc, pci_config_written_t would
> be removed with the next version.
--
MST
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2009-06-15 10:44 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-02 6:42 [Qemu-devel] [PATCH 0/7] pci bridge clean up and multiple pci bus support v2 Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 1/7] vmware_vga: clean up Isaku Yamahata
2009-06-10 15:08 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 2/7] qemu: make default_write_config use mask table Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 3/7] pci: pci_default_config_write() clean up Isaku Yamahata
2009-06-02 10:01 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-03 2:31 ` Isaku Yamahata
2009-06-03 7:22 ` Michael S. Tsirkin
2009-06-03 12:25 ` Isaku Yamahata
2009-06-05 10:43 ` Michael S. Tsirkin
2009-06-10 15:48 ` Michael S. Tsirkin
2009-06-15 9:12 ` Isaku Yamahata
2009-06-15 10:42 ` Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 4/7] pci/config: convert pci configuration space handler to use callback Isaku Yamahata
2009-06-10 15:16 ` [Qemu-devel] " Michael S. Tsirkin
2009-06-02 6:42 ` [Qemu-devel] [PATCH 5/7] pci: PCIBus clean up Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 6/7] pci/brdige qdevfy Isaku Yamahata
2009-06-02 6:42 ` [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support Isaku Yamahata
2009-06-02 7:13 ` [Qemu-devel] " Avi Kivity
2009-06-02 7:46 ` Isaku Yamahata
2009-06-02 8:51 ` Avi Kivity
2009-06-02 13:03 ` Markus Armbruster
2009-06-02 12:56 ` [Qemu-devel] " Markus Armbruster
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.