* [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes
@ 2010-06-21 6:03 Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate Isaku Yamahata
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:03 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
Changes v3 -> v4:
- introduced multifunction property and add validation check to catch
user operation errors.
- some more clean up.
patch description:
When pci devices are populated as multi-function,
OS can fail to probe function > 0. It's because multi function
bit of header type register in configuration space isn't set,
so OS probes only function 0 skipping function > 0 as optimization.
This patch set make qemu set multi function bit when function > 0
is populated.
Isaku Yamahata (6):
pci: use PCI_DEVFN() where appropriate.
pci: remove PCIDeviceInfo::header_type
pci: set PCI multi-function bit appropriately.
pci: don't overwrite multi functio bit in pci header type.
pci: use pci_create_simple_mf()
pci_bridge: make pci bridge aware of pci multi function bit.
hw/ac97.c | 1 -
hw/acpi_piix4.c | 1 -
hw/apb_pci.c | 11 ++-----
hw/dec_pci.c | 4 +-
hw/grackle_pci.c | 1 -
hw/gt64xxx.c | 2 +-
hw/ide/cmd646.c | 1 -
hw/ide/piix.c | 1 -
hw/macio.c | 1 -
hw/ne2000.c | 1 -
hw/openpic.c | 1 -
hw/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++--------
hw/pci.h | 16 ++++++++--
hw/pcnet.c | 1 -
hw/piix4.c | 4 +--
hw/piix_pci.c | 5 +--
hw/ppce500_pci.c | 3 +-
hw/prep_pci.c | 1 -
hw/rtl8139.c | 1 -
hw/sun4u.c | 1 -
hw/unin_pci.c | 16 ++++------
hw/usb-uhci.c | 1 -
hw/versatile_pci.c | 2 +-
hw/vga-pci.c | 1 -
hw/virtio-pci.c | 1 -
hw/vmware_vga.c | 1 -
hw/wdt_i6300esb.c | 1 -
27 files changed, 101 insertions(+), 63 deletions(-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate.
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
@ 2010-06-21 6:03 ` Isaku Yamahata
2010-06-21 11:57 ` [Qemu-devel] " Michael S. Tsirkin
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 2/6] pci: remove PCIDeviceInfo::header_type Isaku Yamahata
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:03 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
Use PCI_DEVFN() and PCI_FUNC_MAX where appropriate.
This patch make it clear that func = 0 and
added assert() to ensure it.
This patch guarantees that auto assigned function is always
single function device at function = 0.
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Yu Liu <yu.liu@freescale.com>
Cc: Paul Brook <paul@codesourcery.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/gt64xxx.c | 2 +-
hw/pci.c | 4 +++-
hw/pci.h | 1 +
hw/ppce500_pci.c | 3 ++-
hw/unin_pci.c | 12 ++++++------
hw/versatile_pci.c | 2 +-
6 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 7691e1d..8e2cf14 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1115,7 +1115,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
s->pci->bus = pci_register_bus(NULL, "pci",
pci_gt64120_set_irq, pci_gt64120_map_irq,
- pic, 144, 4);
+ pic, PCI_DEVFN(18, 0), 4);
s->ISD_handle = cpu_register_io_memory(gt64120_read, gt64120_write, s);
d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
0, NULL, NULL);
diff --git a/hw/pci.c b/hw/pci.c
index ef17eb4..28ba9cf 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -224,6 +224,7 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
const char *name, int devfn_min)
{
qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
+ assert(PCI_FUNC(devfn_min) == 0);
bus->devfn_min = devfn_min;
QLIST_INIT(&bus->child);
vmstate_register(-1, &vmstate_pcibus, bus);
@@ -600,8 +601,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
uint8_t header_type)
{
if (devfn < 0) {
+ assert(PCI_FUNC(bus->devfn_min) == 0);
for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
- devfn += 8) {
+ devfn += PCI_FUNC_MAX) {
if (!bus->devices[devfn])
goto found;
}
diff --git a/hw/pci.h b/hw/pci.h
index 6a2bc6a..077387d 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -14,6 +14,7 @@
#define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
#define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
#define PCI_FUNC(devfn) ((devfn) & 0x07)
+#define PCI_FUNC_MAX 8
/* Class, Vendor and Device IDs from Linux's pci_ids.h */
#include "pci_ids.h"
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 336d284..f949fe3 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -279,7 +279,8 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
controller->pci_state.bus = pci_register_bus(NULL, "pci",
mpc85xx_pci_set_irq,
mpc85xx_pci_map_irq,
- pci_irqs, 0x88, 4);
+ pci_irqs, PCI_DEVFN(0x11, 0),
+ 4);
d = pci_register_device(controller->pci_state.bus,
"host bridge", sizeof(PCIDevice),
0, NULL, NULL);
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index f0a773d..0ecf40f 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -228,10 +228,10 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
d = FROM_SYSBUS(UNINState, s);
d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
pci_unin_set_irq, pci_unin_map_irq,
- pic, 11 << 3, 4);
+ pic, PCI_DEVFN(11, 0), 4);
#if 0
- pci_create_simple(d->host_state.bus, 11 << 3, "uni-north");
+ pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north");
#endif
sysbus_mmio_map(s, 0, 0xf2800000);
@@ -240,11 +240,11 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
/* DEC 21154 bridge */
#if 0
/* XXX: not activated as PPC BIOS doesn't handle multiple buses properly */
- pci_create_simple(d->host_state.bus, 12 << 3, "dec-21154");
+ pci_create_simple(d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154");
#endif
/* Uninorth AGP bus */
- pci_create_simple(d->host_state.bus, 11 << 3, "uni-north-agp");
+ pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp");
dev = qdev_create(NULL, "uni-north-agp");
qdev_init_nofail(dev);
s = sysbus_from_qdev(dev);
@@ -254,7 +254,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
/* Uninorth internal bus */
#if 0
/* XXX: not needed for now */
- pci_create_simple(d->host_state.bus, 14 << 3, "uni-north-pci");
+ pci_create_simple(d->host_state.bus, PCI_DEVFN(14, 0), "uni-north-pci");
dev = qdev_create(NULL, "uni-north-pci");
qdev_init_nofail(dev);
s = sysbus_from_qdev(dev);
@@ -280,7 +280,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic)
d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
pci_unin_set_irq, pci_unin_map_irq,
- pic, 11 << 3, 4);
+ pic, PCI_DEVFN(11, 0), 4);
sysbus_mmio_map(s, 0, 0xf0800000);
sysbus_mmio_map(s, 1, 0xf0c00000);
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index 199bc19..a76bdfa 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -127,7 +127,7 @@ static int pci_vpb_init(SysBusDevice *dev)
}
bus = pci_register_bus(&dev->qdev, "pci",
pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
- 11 << 3, 4);
+ PCI_DEVFN(11, 0), 4);
/* ??? Register memory space. */
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 2/6] pci: remove PCIDeviceInfo::header_type
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate Isaku Yamahata
@ 2010-06-21 6:03 ` Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately Isaku Yamahata
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:03 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
replace PCIDeviceInfo::header_type with is_bridge
as suggested by Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/apb_pci.c | 2 +-
hw/dec_pci.c | 2 +-
hw/pci.c | 15 ++++++++-------
hw/pci.h | 8 ++++++--
4 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 31c8d70..6b9cd59 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -436,7 +436,7 @@ static PCIDeviceInfo pbm_pci_host_info = {
.qdev.name = "pbm",
.qdev.size = sizeof(PCIDevice),
.init = pbm_pci_host_init,
- .header_type = PCI_HEADER_TYPE_BRIDGE,
+ .is_bridge = 1,
};
static SysBusDeviceInfo pbm_host_info = {
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 024c67c..83ce4cd 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -90,7 +90,7 @@ static PCIDeviceInfo dec_21154_pci_host_info = {
.qdev.name = "dec-21154",
.qdev.size = sizeof(PCIDevice),
.init = dec_21154_pci_host_init,
- .header_type = PCI_HEADER_TYPE_BRIDGE,
+ .is_bridge = 1,
};
static void dec_register_devices(void)
diff --git a/hw/pci.c b/hw/pci.c
index 28ba9cf..b6c0a10 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -598,7 +598,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
const char *name, int devfn,
PCIConfigReadFunc *config_read,
PCIConfigWriteFunc *config_write,
- uint8_t header_type)
+ bool is_bridge)
{
if (devfn < 0) {
assert(PCI_FUNC(bus->devfn_min) == 0);
@@ -621,13 +621,12 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
pci_dev->irq_state = 0;
pci_config_alloc(pci_dev);
- header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
- if (header_type == PCI_HEADER_TYPE_NORMAL) {
+ if (!is_bridge) {
pci_set_default_subsystem_id(pci_dev);
}
pci_init_cmask(pci_dev);
pci_init_wmask(pci_dev);
- if (header_type == PCI_HEADER_TYPE_BRIDGE) {
+ if (is_bridge) {
pci_init_wmask_bridge(pci_dev);
}
@@ -1556,7 +1555,9 @@ static int pci_bridge_initfn(PCIDevice *dev)
pci_set_word(dev->config + PCI_STATUS,
PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
- dev->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_BRIDGE;
+ dev->config[PCI_HEADER_TYPE] =
+ (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
+ PCI_HEADER_TYPE_BRIDGE;
pci_set_word(dev->config + PCI_SEC_STATUS,
PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
return 0;
@@ -1607,7 +1608,7 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
devfn = pci_dev->devfn;
pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
info->config_read, info->config_write,
- info->header_type);
+ info->is_bridge);
if (pci_dev == NULL)
return -1;
rc = info->init(pci_dev);
@@ -1857,7 +1858,7 @@ static PCIDeviceInfo bridge_info = {
.init = pci_bridge_initfn,
.exit = pci_bridge_exitfn,
.config_write = pci_bridge_write_config,
- .header_type = PCI_HEADER_TYPE_BRIDGE,
+ .is_bridge = 1,
.qdev.props = (Property[]) {
DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
diff --git a/hw/pci.h b/hw/pci.h
index 077387d..76adc66 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -326,8 +326,12 @@ typedef struct {
PCIConfigReadFunc *config_read;
PCIConfigWriteFunc *config_write;
- /* pci config header type */
- uint8_t header_type;
+ /*
+ * pci-to-pci bridge or normal device.
+ * This doesn't mean pci host switch.
+ * When card bus bridge is supported, this would be enhanced.
+ */
+ int is_bridge;
/* pcie stuff */
int is_express; /* is this device pci express? */
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 2/6] pci: remove PCIDeviceInfo::header_type Isaku Yamahata
@ 2010-06-21 6:03 ` Isaku Yamahata
2010-06-21 9:45 ` [Qemu-devel] " Juan Quintela
2010-06-21 12:36 ` Michael S. Tsirkin
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type Isaku Yamahata
` (2 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:03 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
Set PCI multi-function bit according to multifunction property.
PCI address, devfn ,is exported to users as addr property,
so users can populate pci function(PCIDevice in qemu)
at arbitrary devfn.
It means each function(PCIDevice) don't know whether pci device
(PCIDevice[8]) is multi function or not.
So this patch allows user to set multifunction bit via property
and checks whether multifunction bit is set correctly.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
changes v3 -> v4:
- introduce multifunction property.
changes v2 -> v3:
- introduce PCI_FUNC_MAX
- more commit log
changes v1 -> v2:
---
hw/pci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
hw/pci.h | 4 ++++
2 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index b6c0a10..abc3c1d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
+ DEFINE_PROP_UINT8("multifunction", PCIDevice, mf, 0),
DEFINE_PROP_END_OF_LIST()
}
};
@@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
}
+static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
+{
+ uint8_t slot = PCI_SLOT(dev->devfn);
+ uint8_t func = PCI_FUNC(dev->devfn);
+
+ /* we are here before bus->devices[dev->devfn] = dev */
+ assert(!bus->devices[dev->devfn]);
+
+ if (dev->mf) {
+ dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
+ }
+
+ if (func) {
+ PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
+ if (d && !d->mf) {
+ /* function 0 should set multifunction bit */
+ error_report("PCI: single function device can't be populated "
+ "in function %x.%x", slot, func);
+ return -1;
+ }
+ return 0;
+ }
+
+ if (dev->mf) {
+ return 0;
+ }
+ /* function 0 indicates single function, so function > 0 must be NULL */
+ for (func = 1; func < PCI_FUNC_MAX; ++func) {
+ if (bus->devices[PCI_DEVFN(slot, func)]) {
+ error_report("PCI: %x.0 indicates single function, "
+ "but %x.%x is already populated.",
+ slot, slot, func);
+ return -1;
+ }
+ }
+ return 0;
+}
+
static void pci_config_alloc(PCIDevice *pci_dev)
{
int config_size = pci_config_size(pci_dev);
@@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
if (is_bridge) {
pci_init_wmask_bridge(pci_dev);
}
+ if (pci_init_multifunction(bus, pci_dev)) {
+ return NULL;
+ }
if (!config_read)
config_read = pci_default_read_config;
@@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
}
}
-PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
+PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name)
{
DeviceState *dev;
dev = qdev_create(&bus->qbus, name);
qdev_prop_set_uint32(dev, "addr", devfn);
+ qdev_prop_set_uint8(dev, "multifunction", mf);
return DO_UPCAST(PCIDevice, qdev, dev);
}
-PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
+PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
+ const char *name)
{
- PCIDevice *dev = pci_create(bus, devfn, name);
+ PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
qdev_init_nofail(&dev->qdev);
return dev;
}
+PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
+{
+ return pci_create_mf(bus, devfn, 0, name);
+}
+
+PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
+{
+ return pci_create_simple_mf(bus, devfn, 0, name);
+}
+
static int pci_find_space(PCIDevice *pdev, uint8_t size)
{
int config_size = pci_config_size(pdev);
diff --git a/hw/pci.h b/hw/pci.h
index 76adc66..685fd44 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -131,6 +131,7 @@ struct PCIDevice {
/* the following fields are read only */
PCIBus *bus;
uint32_t devfn;
+ uint8_t mf; /* multi function capabile device */
char name[64];
PCIIORegion io_regions[PCI_NUM_REGIONS];
@@ -343,6 +344,9 @@ typedef struct {
void pci_qdev_register(PCIDeviceInfo *info);
void pci_qdev_register_many(PCIDeviceInfo *info);
+PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name);
+PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
+ const char *name);
PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type.
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
` (2 preceding siblings ...)
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately Isaku Yamahata
@ 2010-06-21 6:03 ` Isaku Yamahata
2010-06-21 13:15 ` [Qemu-devel] " Michael S. Tsirkin
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 5/6] pci: use pci_create_simple_mf() Isaku Yamahata
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit Isaku Yamahata
5 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:03 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
Don't overwrite pci header type.
Otherwise, multi function bit which pci_init_header_type() sets
appropriately is lost.
Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero
which is already zero cleared.
how to test:
run qemu and issue info pci to see whether a device in question is
normal device, not pci-to-pci bridge.
This is handy because guest os isn't required.
tested changes:
The following files are covered by using following commands.
sparc64-softmmu
apb_pci.c, vga-pci.c, cmd646.c, ne2k_pci.c, sun4u.c
ppc-softmmu
grackle_pci.c, cmd646.c, ne2k_pci.c, vga-pci.c, macio.c
ppc-softmmu -M mac99
unin_pci.c(uni-north, uni-north-agp)
ppc64-softmmu
pci-ohci, ne2k_pci, vga-pci, unin_pci.c(u3-agp)
x86_64-softmmu
acpi_piix4.c, ide/piix.c, piix4.c, piix_pci.c
-vga vmware vmware_vga.c
-watchdog i6300esb wdt_i6300esb.c
-usb usb-uhci.c
-sound ac97 ac97.c
-nic model=rtl8139 rtl8139.c
-nic model=pcnet pcnet.c
-balloon virtio virtio-pci.c:
untested changes:
The following changes aren't tested.
prep_pci.c: ppc-softmmu -M prep should cover, but core dumped.
unin_pci.c(uni-north-pci): the caller is commented out.
openpic.c: the caller is commented out in ppc_prep.c
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
chnages v2 -> v3:
- added record what was tested in the commit message
changes v1 -> v2:
- set header type of bridge type in pci_bridge_initfn().
- dropped ugly hunk in apb_pci.c.
---
hw/ac97.c | 1 -
hw/acpi_piix4.c | 1 -
hw/apb_pci.c | 2 --
hw/grackle_pci.c | 1 -
hw/ide/cmd646.c | 1 -
hw/ide/piix.c | 1 -
hw/macio.c | 1 -
hw/ne2000.c | 1 -
hw/openpic.c | 1 -
hw/pcnet.c | 1 -
hw/piix4.c | 3 +--
hw/piix_pci.c | 4 +---
hw/prep_pci.c | 1 -
hw/rtl8139.c | 1 -
hw/sun4u.c | 1 -
hw/unin_pci.c | 4 ----
hw/usb-uhci.c | 1 -
hw/vga-pci.c | 1 -
hw/virtio-pci.c | 1 -
hw/vmware_vga.c | 1 -
hw/wdt_i6300esb.c | 1 -
21 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/hw/ac97.c b/hw/ac97.c
index 4319bc8..d71072d 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1295,7 +1295,6 @@ static int ac97_initfn (PCIDevice *dev)
c[PCI_REVISION_ID] = 0x01; /* rid revision ro */
c[PCI_CLASS_PROG] = 0x00; /* pi programming interface ro */
pci_config_set_class (c, PCI_CLASS_MULTIMEDIA_AUDIO); /* ro */
- c[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* headtyp header type ro */
/* TODO set when bar is registered. no need to override. */
/* nabmar native audio mixer base address rw */
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 8d1a628..bfa1d9a 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -369,7 +369,6 @@ static int piix4_pm_initfn(PCIDevice *dev)
pci_conf[0x08] = 0x03; // revision number
pci_conf[0x09] = 0x00;
pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_OTHER);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
pci_conf[0x3d] = 0x01; // interrupt pin 1
pci_conf[0x40] = 0x01; /* PM io base read only bit */
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 6b9cd59..69a774d 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -427,8 +427,6 @@ static int pbm_pci_host_init(PCIDevice *d)
PCI_STATUS_FAST_BACK | PCI_STATUS_66MHZ |
PCI_STATUS_DEVSEL_MEDIUM);
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
- pci_set_byte(d->config + PCI_HEADER_TYPE,
- PCI_HEADER_TYPE_NORMAL);
return 0;
}
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index aa0c51b..b3a5f54 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -126,7 +126,6 @@ static int grackle_pci_host_init(PCIDevice *d)
d->config[0x08] = 0x00; // revision
d->config[0x09] = 0x01;
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
return 0;
}
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 559147f..756ee81 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -240,7 +240,6 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
pci_conf[PCI_CLASS_PROG] = 0x8f;
pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
pci_conf[0x51] = 0x04; // enable IDE0
if (d->secondary) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index dad6e86..8817915 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -122,7 +122,6 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
qemu_register_reset(piix3_reset, d);
diff --git a/hw/macio.c b/hw/macio.c
index e92e82a..789ca55 100644
--- a/hw/macio.c
+++ b/hw/macio.c
@@ -110,7 +110,6 @@ void macio_init (PCIBus *bus, int device_id, int is_oldworld, int pic_mem_index,
pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_APPLE);
pci_config_set_device_id(d->config, device_id);
pci_config_set_class(d->config, PCI_CLASS_OTHERS << 8);
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
d->config[0x3d] = 0x01; // interrupt on pin 1
diff --git a/hw/ne2000.c b/hw/ne2000.c
index 78fe14f..126e7cf 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -723,7 +723,6 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8029);
pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
/* TODO: RST# value should be 0. PCI spec 6.2.4 */
pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
diff --git a/hw/openpic.c b/hw/openpic.c
index ac21993..2bbf787 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -1194,7 +1194,6 @@ qemu_irq *openpic_init (PCIBus *bus, int *pmem_index, int nb_cpus,
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_IBM);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_IBM_OPENPIC2);
pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER); // FIXME?
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
pci_conf[0x3d] = 0x00; // no interrupt pin
/* Register I/O spaces */
diff --git a/hw/pcnet.c b/hw/pcnet.c
index 5e63eb5..5e75930 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1990,7 +1990,6 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
/* TODO: 0 is the default anyway, no need to set it. */
pci_conf[PCI_CLASS_PROG] = 0x00;
pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
/* TODO: not necessary, is set when BAR is registered. */
pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_SPACE_IO);
diff --git a/hw/piix4.c b/hw/piix4.c
index f75951b..03926a7 100644
--- a/hw/piix4.c
+++ b/hw/piix4.c
@@ -93,8 +93,7 @@ static int piix4_initfn(PCIDevice *d)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB_0); // 82371AB/EB/MB PIIX4 PCI-to-ISA bridge
pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
- pci_conf[PCI_HEADER_TYPE] =
- PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic
+ pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
piix4_dev = d;
qemu_register_reset(piix4_reset, d);
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 16645cd..0b0c92b 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -208,7 +208,6 @@ static int i440fx_initfn(PCIDevice *dev)
pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82441);
d->dev.config[0x08] = 0x02; // revision
pci_config_set_class(d->dev.config, PCI_CLASS_BRIDGE_HOST);
- d->dev.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
d->dev.config[I440FX_SMRAM] = 0x02;
@@ -337,8 +336,7 @@ static int piix3_initfn(PCIDevice *dev)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_0); // 82371SB PIIX3 PCI-to-ISA bridge (Step A1)
pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
- pci_conf[PCI_HEADER_TYPE] =
- PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic
+ pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
qemu_register_reset(piix3_reset, d);
return 0;
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 144fde0..0c2afe9 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -137,7 +137,6 @@ PCIBus *pci_prep_init(qemu_irq *pic)
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
d->config[0x0C] = 0x08; // cache_line_size
d->config[0x0D] = 0x10; // latency_timer
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
d->config[0x34] = 0x00; // capabilities_pointer
return s->bus;
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 72e2242..441f0a9 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3361,7 +3361,6 @@ static int pci_rtl8139_init(PCIDevice *dev)
pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MASTER;
pci_conf[PCI_REVISION_ID] = RTL8139_PCI_REVID; /* >=0x20 is for 8139C+ */
pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
/* TODO: value should be 0 at RST# */
pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */
/* TODO: start of capability list, but no capability
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 40b5f1f..cf5a8c4 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -562,7 +562,6 @@ pci_ebus_init1(PCIDevice *s)
s->config[0x09] = 0x00; // programming i/f
pci_config_set_class(s->config, PCI_CLASS_BRIDGE_OTHER);
s->config[0x0D] = 0x0a; // latency_timer
- s->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
pci_register_bar(s, 0, 0x1000000, PCI_BASE_ADDRESS_SPACE_MEMORY,
ebus_mmio_mapfunc);
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 0ecf40f..717ffd1 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -298,7 +298,6 @@ static int unin_main_pci_host_init(PCIDevice *d)
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
d->config[0x0C] = 0x08; // cache_line_size
d->config[0x0D] = 0x10; // latency_timer
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
d->config[0x34] = 0x00; // capabilities_pointer
return 0;
}
@@ -311,7 +310,6 @@ static int unin_agp_pci_host_init(PCIDevice *d)
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
d->config[0x0C] = 0x08; // cache_line_size
d->config[0x0D] = 0x10; // latency_timer
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
// d->config[0x34] = 0x80; // capabilities_pointer
return 0;
}
@@ -327,7 +325,6 @@ static int u3_agp_pci_host_init(PCIDevice *d)
d->config[0x0C] = 0x08;
/* latency timer */
d->config[0x0D] = 0x10;
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
return 0;
}
@@ -339,7 +336,6 @@ static int unin_internal_pci_host_init(PCIDevice *d)
pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST);
d->config[0x0C] = 0x08; // cache_line_size
d->config[0x0D] = 0x10; // latency_timer
- d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
d->config[0x34] = 0x00; // capabilities_pointer
return 0;
}
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 624d55b..058bf59 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -1108,7 +1108,6 @@ static int usb_uhci_common_initfn(UHCIState *s)
pci_conf[PCI_REVISION_ID] = 0x01; // revision number
pci_conf[PCI_CLASS_PROG] = 0x00;
pci_config_set_class(pci_conf, PCI_CLASS_SERIAL_USB);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
/* TODO: reset value should be 0. */
pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3
pci_conf[0x60] = 0x10; // release number
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index eef78ed..2315f70 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -90,7 +90,6 @@ static int pci_vga_initfn(PCIDevice *dev)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_QEMU);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_QEMU_VGA);
pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
- pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
/* XXX: VGA_RAM_SIZE must be a power of two */
pci_register_bar(&d->dev, 0, VGA_RAM_SIZE,
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index e101fa0..0e25f25 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -506,7 +506,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
config[0x09] = pif;
pci_config_set_class(config, class_code);
- config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
config[0x2c] = vendor & 0xFF;
config[0x2d] = (vendor >> 8) & 0xFF;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index bf2a699..38fe976 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1246,7 +1246,6 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
pci_config_set_class(s->card.config, PCI_CLASS_DISPLAY_VGA);
s->card.config[PCI_CACHE_LINE_SIZE] = 0x08; /* Cache line size */
s->card.config[PCI_LATENCY_TIMER] = 0x40; /* Latency timer */
- s->card.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
s->card.config[PCI_SUBSYSTEM_VENDOR_ID] = PCI_VENDOR_ID_VMWARE & 0xff;
s->card.config[PCI_SUBSYSTEM_VENDOR_ID + 1] = PCI_VENDOR_ID_VMWARE >> 8;
s->card.config[PCI_SUBSYSTEM_ID] = SVGA_PCI_DEVICE_ID & 0xff;
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index be0e89e..46e1df8 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -411,7 +411,6 @@ static int i6300esb_init(PCIDevice *dev)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_ESB_9);
pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER);
- pci_conf[PCI_HEADER_TYPE] = 0x00;
pci_register_bar(&d->dev, 0, 0x10,
PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 5/6] pci: use pci_create_simple_mf()
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
` (3 preceding siblings ...)
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type Isaku Yamahata
@ 2010-06-21 6:04 ` Isaku Yamahata
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit Isaku Yamahata
5 siblings, 0 replies; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:04 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
use pci_create_simple_mf() for normal device which sets multifunction bit.
At the moment, only pc_piix.c and mips_malta.c uses multifunction
devices with piix3/4 pci-isa bridge.
And other boards don't populate those devices.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/piix4.c | 3 +--
hw/piix_pci.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/piix4.c b/hw/piix4.c
index 03926a7..298c70f 100644
--- a/hw/piix4.c
+++ b/hw/piix4.c
@@ -93,7 +93,6 @@ static int piix4_initfn(PCIDevice *d)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB_0); // 82371AB/EB/MB PIIX4 PCI-to-ISA bridge
pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
- pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
piix4_dev = d;
qemu_register_reset(piix4_reset, d);
@@ -104,7 +103,7 @@ int piix4_init(PCIBus *bus, int devfn)
{
PCIDevice *d;
- d = pci_create_simple(bus, devfn, "PIIX4");
+ d = pci_create_simple_mf(bus, devfn, 1, "PIIX4");
return d->devfn;
}
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 0b0c92b..7cd33e6 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -234,7 +234,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
*pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
piix3 = DO_UPCAST(PIIX3State, dev,
- pci_create_simple(b, -1, "PIIX3"));
+ pci_create_simple_mf(b, -1, 1, "PIIX3"));
piix3->pic = pic;
pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, 4);
(*pi440fx_state)->piix3 = piix3;
@@ -336,7 +336,6 @@ static int piix3_initfn(PCIDevice *dev)
pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_0); // 82371SB PIIX3 PCI-to-ISA bridge (Step A1)
pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
- pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
qemu_register_reset(piix3_reset, d);
return 0;
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit.
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
` (4 preceding siblings ...)
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 5/6] pci: use pci_create_simple_mf() Isaku Yamahata
@ 2010-06-21 6:04 ` Isaku Yamahata
2010-06-21 13:22 ` [Qemu-devel] " Michael S. Tsirkin
5 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 6:04 UTC (permalink / raw)
To: qemu-devel; +Cc: yu.liu, mst, blauwirbel, yamahata, paul, aurelien
make pci bridge aware of pci multi function property and let pci generic
code to set the bit.
Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
hw/apb_pci.c | 7 ++-----
hw/dec_pci.c | 2 +-
hw/pci.c | 5 +++--
hw/pci.h | 3 ++-
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 69a774d..4b78704 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -312,9 +312,6 @@ static void apb_pci_bridge_init(PCIBus *b)
PCI_STATUS_FAST_BACK | PCI_STATUS_66MHZ |
PCI_STATUS_DEVSEL_MEDIUM);
pci_set_byte(dev->config + PCI_REVISION_ID, 0x11);
- pci_set_byte(dev->config + PCI_HEADER_TYPE,
- pci_get_byte(dev->config + PCI_HEADER_TYPE) |
- PCI_HEADER_TYPE_MULTI_FUNCTION);
}
PCIBus *pci_apb_init(target_phys_addr_t special_base,
@@ -350,13 +347,13 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
pci_create_simple(d->bus, 0, "pbm");
/* APB secondary busses */
- *bus2 = pci_bridge_init(d->bus, PCI_DEVFN(1, 0),
+ *bus2 = pci_bridge_init(d->bus, PCI_DEVFN(1, 0), 1,
PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
pci_apb_map_irq,
"Advanced PCI Bus secondary bridge 1");
apb_pci_bridge_init(*bus2);
- *bus3 = pci_bridge_init(d->bus, PCI_DEVFN(1, 1),
+ *bus3 = pci_bridge_init(d->bus, PCI_DEVFN(1, 1), 1,
PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
pci_apb_map_irq,
"Advanced PCI Bus secondary bridge 2");
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 83ce4cd..f042f80 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -55,7 +55,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
dev = qdev_create(NULL, "dec-21154");
qdev_init_nofail(dev);
- ret = pci_bridge_init(parent_bus, devfn,
+ ret = pci_bridge_init(parent_bus, devfn, 0,
PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_21154,
dec_map_irq, "DEC 21154 PCI-PCI bridge");
diff --git a/hw/pci.c b/hw/pci.c
index abc3c1d..7e4fbae 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1613,13 +1613,14 @@ static int pci_bridge_exitfn(PCIDevice *pci_dev)
return 0;
}
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint8_t mf,
+ uint16_t vid, uint16_t did,
pci_map_irq_fn map_irq, const char *name)
{
PCIDevice *dev;
PCIBridge *s;
- dev = pci_create(bus, devfn, "pci-bridge");
+ dev = pci_create_mf(bus, devfn, mf, "pci-bridge");
qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
qdev_init_nofail(&dev->qdev);
diff --git a/hw/pci.h b/hw/pci.h
index 685fd44..889bbea 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -231,7 +231,8 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
void do_pci_info_print(Monitor *mon, const QObject *data);
void do_pci_info(Monitor *mon, QObject **ret_data);
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint8_t mf,
+ uint16_t vid, uint16_t did,
pci_map_irq_fn map_irq, const char *name);
PCIDevice *pci_bridge_get_device(PCIBus *bus);
--
1.6.6.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately Isaku Yamahata
@ 2010-06-21 9:45 ` Juan Quintela
2010-06-21 9:52 ` Isaku Yamahata
2010-06-21 12:36 ` Michael S. Tsirkin
1 sibling, 1 reply; 19+ messages in thread
From: Juan Quintela @ 2010-06-21 9:45 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: yu.liu, mst, qemu-devel, blauwirbel, paul, aurelien
Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> Set PCI multi-function bit according to multifunction property.
> PCI address, devfn ,is exported to users as addr property,
> so users can populate pci function(PCIDevice in qemu)
> at arbitrary devfn.
> It means each function(PCIDevice) don't know whether pci device
> (PCIDevice[8]) is multi function or not.
> So this patch allows user to set multifunction bit via property
> and checks whether multifunction bit is set correctly.
[...]
> @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> if (is_bridge) {
> pci_init_wmask_bridge(pci_dev);
> }
> + if (pci_init_multifunction(bus, pci_dev)) {
> + return NULL;
> + }
Don't we have here a mem leak?
what frees what pci_config_alloc() assignates in the error case?
Later, Juan.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-21 9:45 ` [Qemu-devel] " Juan Quintela
@ 2010-06-21 9:52 ` Isaku Yamahata
0 siblings, 0 replies; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-21 9:52 UTC (permalink / raw)
To: Juan Quintela; +Cc: yu.liu, mst, qemu-devel, blauwirbel, paul, aurelien
On Mon, Jun 21, 2010 at 11:45:15AM +0200, Juan Quintela wrote:
> Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> > Set PCI multi-function bit according to multifunction property.
> > PCI address, devfn ,is exported to users as addr property,
> > so users can populate pci function(PCIDevice in qemu)
> > at arbitrary devfn.
> > It means each function(PCIDevice) don't know whether pci device
> > (PCIDevice[8]) is multi function or not.
> > So this patch allows user to set multifunction bit via property
> > and checks whether multifunction bit is set correctly.
>
> [...]
>
> > @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > if (is_bridge) {
> > pci_init_wmask_bridge(pci_dev);
> > }
> > + if (pci_init_multifunction(bus, pci_dev)) {
> > + return NULL;
> > + }
>
> Don't we have here a mem leak?
> what frees what pci_config_alloc() assignates in the error case?
Oh, you're right. Will fix it.
--
yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] Re: [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate.
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate Isaku Yamahata
@ 2010-06-21 11:57 ` Michael S. Tsirkin
0 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-21 11:57 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Mon, Jun 21, 2010 at 03:03:56PM +0900, Isaku Yamahata wrote:
> Use PCI_DEVFN() and PCI_FUNC_MAX where appropriate.
> This patch make it clear that func = 0 and
> added assert() to ensure it.
> This patch guarantees that auto assigned function is always
> single function device at function = 0.
>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Yu Liu <yu.liu@freescale.com>
> Cc: Paul Brook <paul@codesourcery.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
How was this tested? I suggest splitting the asserts
to a separate patch, then you can compare
stripped object files before and after the change,
should be identical.
> ---
> hw/gt64xxx.c | 2 +-
> hw/pci.c | 4 +++-
> hw/pci.h | 1 +
> hw/ppce500_pci.c | 3 ++-
> hw/unin_pci.c | 12 ++++++------
> hw/versatile_pci.c | 2 +-
> 6 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index 7691e1d..8e2cf14 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1115,7 +1115,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
>
> s->pci->bus = pci_register_bus(NULL, "pci",
> pci_gt64120_set_irq, pci_gt64120_map_irq,
> - pic, 144, 4);
> + pic, PCI_DEVFN(18, 0), 4);
> s->ISD_handle = cpu_register_io_memory(gt64120_read, gt64120_write, s);
> d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
> 0, NULL, NULL);
> diff --git a/hw/pci.c b/hw/pci.c
> index ef17eb4..28ba9cf 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -224,6 +224,7 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
> const char *name, int devfn_min)
> {
> qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
> + assert(PCI_FUNC(devfn_min) == 0);
> bus->devfn_min = devfn_min;
> QLIST_INIT(&bus->child);
> vmstate_register(-1, &vmstate_pcibus, bus);
> @@ -600,8 +601,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> uint8_t header_type)
> {
> if (devfn < 0) {
> + assert(PCI_FUNC(bus->devfn_min) == 0);
I think that the assert when bus is created is enough.
> for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> - devfn += 8) {
> + devfn += PCI_FUNC_MAX) {
> if (!bus->devices[devfn])
> goto found;
> }
> diff --git a/hw/pci.h b/hw/pci.h
> index 6a2bc6a..077387d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -14,6 +14,7 @@
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f)
> #define PCI_FUNC(devfn) ((devfn) & 0x07)
> +#define PCI_FUNC_MAX 8
>
> /* Class, Vendor and Device IDs from Linux's pci_ids.h */
> #include "pci_ids.h"
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 336d284..f949fe3 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -279,7 +279,8 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
> controller->pci_state.bus = pci_register_bus(NULL, "pci",
> mpc85xx_pci_set_irq,
> mpc85xx_pci_map_irq,
> - pci_irqs, 0x88, 4);
> + pci_irqs, PCI_DEVFN(0x11, 0),
> + 4);
> d = pci_register_device(controller->pci_state.bus,
> "host bridge", sizeof(PCIDevice),
> 0, NULL, NULL);
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index f0a773d..0ecf40f 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -228,10 +228,10 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
> d = FROM_SYSBUS(UNINState, s);
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic, 11 << 3, 4);
> + pic, PCI_DEVFN(11, 0), 4);
>
> #if 0
> - pci_create_simple(d->host_state.bus, 11 << 3, "uni-north");
> + pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north");
> #endif
>
> sysbus_mmio_map(s, 0, 0xf2800000);
> @@ -240,11 +240,11 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
> /* DEC 21154 bridge */
> #if 0
> /* XXX: not activated as PPC BIOS doesn't handle multiple buses properly */
> - pci_create_simple(d->host_state.bus, 12 << 3, "dec-21154");
> + pci_create_simple(d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154");
> #endif
>
> /* Uninorth AGP bus */
> - pci_create_simple(d->host_state.bus, 11 << 3, "uni-north-agp");
> + pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp");
> dev = qdev_create(NULL, "uni-north-agp");
> qdev_init_nofail(dev);
> s = sysbus_from_qdev(dev);
> @@ -254,7 +254,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
> /* Uninorth internal bus */
> #if 0
> /* XXX: not needed for now */
> - pci_create_simple(d->host_state.bus, 14 << 3, "uni-north-pci");
> + pci_create_simple(d->host_state.bus, PCI_DEVFN(14, 0), "uni-north-pci");
> dev = qdev_create(NULL, "uni-north-pci");
> qdev_init_nofail(dev);
> s = sysbus_from_qdev(dev);
> @@ -280,7 +280,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic)
>
> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
> pci_unin_set_irq, pci_unin_map_irq,
> - pic, 11 << 3, 4);
> + pic, PCI_DEVFN(11, 0), 4);
>
> sysbus_mmio_map(s, 0, 0xf0800000);
> sysbus_mmio_map(s, 1, 0xf0c00000);
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 199bc19..a76bdfa 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -127,7 +127,7 @@ static int pci_vpb_init(SysBusDevice *dev)
> }
> bus = pci_register_bus(&dev->qdev, "pci",
> pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
> - 11 << 3, 4);
> + PCI_DEVFN(11, 0), 4);
>
> /* ??? Register memory space. */
>
> --
> 1.6.6.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately Isaku Yamahata
2010-06-21 9:45 ` [Qemu-devel] " Juan Quintela
@ 2010-06-21 12:36 ` Michael S. Tsirkin
2010-06-23 7:25 ` Isaku Yamahata
1 sibling, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-21 12:36 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Mon, Jun 21, 2010 at 03:03:58PM +0900, Isaku Yamahata wrote:
> Set PCI multi-function bit according to multifunction property.
> PCI address, devfn ,is exported to users as addr property,
> so users can populate pci function(PCIDevice in qemu)
> at arbitrary devfn.
> It means each function(PCIDevice) don't know whether pci device
> (PCIDevice[8]) is multi function or not.
> So this patch allows user to set multifunction bit via property
> and checks whether multifunction bit is set correctly.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Applying it this way will break bisect.
We also need to handle migration compatibility.
I propose we split it this way:
- patch to add multifunction property (ignored)
- set property in builtin devices where appropriate
- patch to look at property and set bit in header
> ---
> changes v3 -> v4:
> - introduce multifunction property.
>
> changes v2 -> v3:
> - introduce PCI_FUNC_MAX
> - more commit log
>
> changes v1 -> v2:
> ---
> hw/pci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> hw/pci.h | 4 ++++
> 2 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index b6c0a10..abc3c1d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> + DEFINE_PROP_UINT8("multifunction", PCIDevice, mf, 0),
Please make this a bit property, not UINT8. It can be stored in
cap_present.
> DEFINE_PROP_END_OF_LIST()
> }
> };
> @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> }
>
> +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> +{
IMO we should just add in pci_register_device:
if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
} else if (PCI_FUNC(dev->devfn)) {
error_report("PCI: single function device can't be populated %x.%x",
PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
return -1;
}
And be done with it.
> + uint8_t slot = PCI_SLOT(dev->devfn);
> + uint8_t func = PCI_FUNC(dev->devfn);
> +
> + /* we are here before bus->devices[dev->devfn] = dev */
> + assert(!bus->devices[dev->devfn]);
Can users trigger this?
If yes, this needs and error, not an assert.
> +
> + if (dev->mf) {
> + dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> + }
> +
> + if (func) {
Please open-code func above.
> + PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
> + if (d && !d->mf) {
> + /* function 0 should set multifunction bit */
> + error_report("PCI: single function device can't be populated "
> + "in function %x.%x", slot, func);
> + return -1;
> + }
> + return 0;
> + }
> +
> + if (dev->mf) {
> + return 0;
> + }
> + /* function 0 indicates single function, so function > 0 must be NULL */
We don't need the below test: each function will be checked
when it is added.
> + for (func = 1; func < PCI_FUNC_MAX; ++func) {
> + if (bus->devices[PCI_DEVFN(slot, func)]) {
> + error_report("PCI: %x.0 indicates single function, "
> + "but %x.%x is already populated.",
> + slot, slot, func);
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> static void pci_config_alloc(PCIDevice *pci_dev)
> {
> int config_size = pci_config_size(pci_dev);
> @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> if (is_bridge) {
> pci_init_wmask_bridge(pci_dev);
> }
> + if (pci_init_multifunction(bus, pci_dev)) {
> + return NULL;
> + }
>
> if (!config_read)
> config_read = pci_default_read_config;
> @@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
> }
> }
>
> -PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name)
> {
> DeviceState *dev;
>
> dev = qdev_create(&bus->qbus, name);
> qdev_prop_set_uint32(dev, "addr", devfn);
> + qdev_prop_set_uint8(dev, "multifunction", mf);
> return DO_UPCAST(PCIDevice, qdev, dev);
> }
>
> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> + const char *name)
> {
> - PCIDevice *dev = pci_create(bus, devfn, name);
> + PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
> qdev_init_nofail(&dev->qdev);
> return dev;
> }
>
> +PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +{
> + return pci_create_mf(bus, devfn, 0, name);
> +}
> +
> +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +{
> + return pci_create_simple_mf(bus, devfn, 0, name);
> +}
> +
> static int pci_find_space(PCIDevice *pdev, uint8_t size)
> {
> int config_size = pci_config_size(pdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 76adc66..685fd44 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -131,6 +131,7 @@ struct PCIDevice {
> /* the following fields are read only */
> PCIBus *bus;
> uint32_t devfn;
> + uint8_t mf; /* multi function capabile device */
Add a bit in cap_present please.
> char name[64];
> PCIIORegion io_regions[PCI_NUM_REGIONS];
>
> @@ -343,6 +344,9 @@ typedef struct {
> void pci_qdev_register(PCIDeviceInfo *info);
> void pci_qdev_register_many(PCIDeviceInfo *info);
>
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name);
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> + const char *name);
mf->multifunction
But do we need the extra functions? I thought qdev can handle
the flag?
> PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
> PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>
> --
> 1.6.6.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] Re: [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type.
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type Isaku Yamahata
@ 2010-06-21 13:15 ` Michael S. Tsirkin
0 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-21 13:15 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Mon, Jun 21, 2010 at 03:03:59PM +0900, Isaku Yamahata wrote:
> diff --git a/hw/piix4.c b/hw/piix4.c
> index f75951b..03926a7 100644
> --- a/hw/piix4.c
> +++ b/hw/piix4.c
> @@ -93,8 +93,7 @@ static int piix4_initfn(PCIDevice *d)
> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB_0); // 82371AB/EB/MB PIIX4 PCI-to-ISA bridge
> pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
> - pci_conf[PCI_HEADER_TYPE] =
> - PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic
> + pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
>
IMO it's cleaner not to change the same line twice.
Just move this bit to the next patch.
> piix4_dev = d;
> qemu_register_reset(piix4_reset, d);
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 16645cd..0b0c92b 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -208,7 +208,6 @@ static int i440fx_initfn(PCIDevice *dev)
> pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82441);
> d->dev.config[0x08] = 0x02; // revision
> pci_config_set_class(d->dev.config, PCI_CLASS_BRIDGE_HOST);
> - d->dev.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
>
> d->dev.config[I440FX_SMRAM] = 0x02;
>
> @@ -337,8 +336,7 @@ static int piix3_initfn(PCIDevice *dev)
> pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_0); // 82371SB PIIX3 PCI-to-ISA bridge (Step A1)
> pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA);
> - pci_conf[PCI_HEADER_TYPE] =
> - PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic
> + pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
>
same here
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] Re: [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit.
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit Isaku Yamahata
@ 2010-06-21 13:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-21 13:22 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Mon, Jun 21, 2010 at 03:04:01PM +0900, Isaku Yamahata wrote:
> make pci bridge aware of pci multi function property and let pci generic
> code to set the bit.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> hw/apb_pci.c | 7 ++-----
> hw/dec_pci.c | 2 +-
> hw/pci.c | 5 +++--
> hw/pci.h | 3 ++-
> 4 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 69a774d..4b78704 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -312,9 +312,6 @@ static void apb_pci_bridge_init(PCIBus *b)
> PCI_STATUS_FAST_BACK | PCI_STATUS_66MHZ |
> PCI_STATUS_DEVSEL_MEDIUM);
> pci_set_byte(dev->config + PCI_REVISION_ID, 0x11);
> - pci_set_byte(dev->config + PCI_HEADER_TYPE,
> - pci_get_byte(dev->config + PCI_HEADER_TYPE) |
> - PCI_HEADER_TYPE_MULTI_FUNCTION);
> }
>
> PCIBus *pci_apb_init(target_phys_addr_t special_base,
Was not the above added by another patch in these series?
If yes please just move the chunk here, do not change the same
line twice.
> @@ -350,13 +347,13 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
> pci_create_simple(d->bus, 0, "pbm");
>
> /* APB secondary busses */
> - *bus2 = pci_bridge_init(d->bus, PCI_DEVFN(1, 0),
> + *bus2 = pci_bridge_init(d->bus, PCI_DEVFN(1, 0), 1,
> PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
> pci_apb_map_irq,
> "Advanced PCI Bus secondary bridge 1");
> apb_pci_bridge_init(*bus2);
>
> - *bus3 = pci_bridge_init(d->bus, PCI_DEVFN(1, 1),
> + *bus3 = pci_bridge_init(d->bus, PCI_DEVFN(1, 1), 1,
> PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_SIMBA,
> pci_apb_map_irq,
> "Advanced PCI Bus secondary bridge 2");
> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> index 83ce4cd..f042f80 100644
> --- a/hw/dec_pci.c
> +++ b/hw/dec_pci.c
> @@ -55,7 +55,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
>
> dev = qdev_create(NULL, "dec-21154");
> qdev_init_nofail(dev);
> - ret = pci_bridge_init(parent_bus, devfn,
> + ret = pci_bridge_init(parent_bus, devfn, 0,
> PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_21154,
> dec_map_irq, "DEC 21154 PCI-PCI bridge");
>
> diff --git a/hw/pci.c b/hw/pci.c
> index abc3c1d..7e4fbae 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1613,13 +1613,14 @@ static int pci_bridge_exitfn(PCIDevice *pci_dev)
> return 0;
> }
>
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
> +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint8_t mf,
> + uint16_t vid, uint16_t did,
> pci_map_irq_fn map_irq, const char *name)
> {
> PCIDevice *dev;
> PCIBridge *s;
>
> - dev = pci_create(bus, devfn, "pci-bridge");
> + dev = pci_create_mf(bus, devfn, mf, "pci-bridge");
> qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
> qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
> qdev_init_nofail(&dev->qdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 685fd44..889bbea 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -231,7 +231,8 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>
> void do_pci_info_print(Monitor *mon, const QObject *data);
> void do_pci_info(Monitor *mon, QObject **ret_data);
> -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
> +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint8_t mf,
make it bool multifunction
even better - can this be initialized with qdev somehow?
> + uint16_t vid, uint16_t did,
> pci_map_irq_fn map_irq, const char *name);
> PCIDevice *pci_bridge_get_device(PCIBus *bus);
>
> --
> 1.6.6.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-21 12:36 ` Michael S. Tsirkin
@ 2010-06-23 7:25 ` Isaku Yamahata
2010-06-23 9:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-23 7:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Mon, Jun 21, 2010 at 03:36:00PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 21, 2010 at 03:03:58PM +0900, Isaku Yamahata wrote:
> > Set PCI multi-function bit according to multifunction property.
> > PCI address, devfn ,is exported to users as addr property,
> > so users can populate pci function(PCIDevice in qemu)
> > at arbitrary devfn.
> > It means each function(PCIDevice) don't know whether pci device
> > (PCIDevice[8]) is multi function or not.
> > So this patch allows user to set multifunction bit via property
> > and checks whether multifunction bit is set correctly.
> >
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> Applying it this way will break bisect.
> We also need to handle migration compatibility.
> I propose we split it this way:
> - patch to add multifunction property (ignored)
> - set property in builtin devices where appropriate
> - patch to look at property and set bit in header
Done.
>
> > ---
> > changes v3 -> v4:
> > - introduce multifunction property.
> >
> > changes v2 -> v3:
> > - introduce PCI_FUNC_MAX
> > - more commit log
> >
> > changes v1 -> v2:
> > ---
> > hw/pci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > hw/pci.h | 4 ++++
> > 2 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/pci.c b/hw/pci.c
> > index b6c0a10..abc3c1d 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
> > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> > + DEFINE_PROP_UINT8("multifunction", PCIDevice, mf, 0),
>
> Please make this a bit property, not UINT8. It can be stored in
> cap_present.
>
> > DEFINE_PROP_END_OF_LIST()
> > }
> > };
> > @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > }
> >
> > +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > +{
>
> IMO we should just add in pci_register_device:
>
> if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
> dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> } else if (PCI_FUNC(dev->devfn)) {
> error_report("PCI: single function device can't be populated %x.%x",
> PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> return -1;
> }
>
> And be done with it.
Unfortunately there are two ways to set the bit.
- set the bit of all the function.
Example: Intel X58(north bridge.)
- set the bit of only function = 0.
Example: PIIX3, PIIX4, ... ICH10.
lspci -x would help to see what your pc has.
>
> > + uint8_t slot = PCI_SLOT(dev->devfn);
> > + uint8_t func = PCI_FUNC(dev->devfn);
> > +
> > + /* we are here before bus->devices[dev->devfn] = dev */
> > + assert(!bus->devices[dev->devfn]);
>
> Can users trigger this?
> If yes, this needs and error, not an assert.
>
> > +
> > + if (dev->mf) {
> > + dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > + }
> > +
> > + if (func) {
>
> Please open-code func above.
>
> > + PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
> > + if (d && !d->mf) {
> > + /* function 0 should set multifunction bit */
> > + error_report("PCI: single function device can't be populated "
> > + "in function %x.%x", slot, func);
> > + return -1;
> > + }
> > + return 0;
> > + }
> > +
> > + if (dev->mf) {
> > + return 0;
> > + }
> > + /* function 0 indicates single function, so function > 0 must be NULL */
>
>
> We don't need the below test: each function will be checked
> when it is added.
>
> > + for (func = 1; func < PCI_FUNC_MAX; ++func) {
> > + if (bus->devices[PCI_DEVFN(slot, func)]) {
> > + error_report("PCI: %x.0 indicates single function, "
> > + "but %x.%x is already populated.",
> > + slot, slot, func);
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static void pci_config_alloc(PCIDevice *pci_dev)
> > {
> > int config_size = pci_config_size(pci_dev);
> > @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > if (is_bridge) {
> > pci_init_wmask_bridge(pci_dev);
> > }
> > + if (pci_init_multifunction(bus, pci_dev)) {
> > + return NULL;
> > + }
> >
> > if (!config_read)
> > config_read = pci_default_read_config;
> > @@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
> > }
> > }
> >
> > -PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> > +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name)
> > {
> > DeviceState *dev;
> >
> > dev = qdev_create(&bus->qbus, name);
> > qdev_prop_set_uint32(dev, "addr", devfn);
> > + qdev_prop_set_uint8(dev, "multifunction", mf);
> > return DO_UPCAST(PCIDevice, qdev, dev);
> > }
> >
> > -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> > +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> > + const char *name)
> > {
> > - PCIDevice *dev = pci_create(bus, devfn, name);
> > + PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
> > qdev_init_nofail(&dev->qdev);
> > return dev;
> > }
> >
> > +PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> > +{
> > + return pci_create_mf(bus, devfn, 0, name);
> > +}
> > +
> > +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> > +{
> > + return pci_create_simple_mf(bus, devfn, 0, name);
> > +}
> > +
> > static int pci_find_space(PCIDevice *pdev, uint8_t size)
> > {
> > int config_size = pci_config_size(pdev);
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 76adc66..685fd44 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -131,6 +131,7 @@ struct PCIDevice {
> > /* the following fields are read only */
> > PCIBus *bus;
> > uint32_t devfn;
> > + uint8_t mf; /* multi function capabile device */
>
> Add a bit in cap_present please.
>
> > char name[64];
> > PCIIORegion io_regions[PCI_NUM_REGIONS];
> >
> > @@ -343,6 +344,9 @@ typedef struct {
> > void pci_qdev_register(PCIDeviceInfo *info);
> > void pci_qdev_register_many(PCIDeviceInfo *info);
> >
> > +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name);
> > +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> > + const char *name);
>
> mf->multifunction
>
> But do we need the extra functions? I thought qdev can handle
> the flag?
If the device tree was fully created via qdev from configuration file,
that would be correct.
However the conversion isn't completed, so convenient functions
are exported to the code which creates the device tree.
Please notice that devfn here is also property.
While I could add one more parameter to pci_create_simple(),
I would have to touch many callers.
>
> > PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
> > PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
> >
> > --
> > 1.6.6.1
>
--
yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-23 7:25 ` Isaku Yamahata
@ 2010-06-23 9:52 ` Michael S. Tsirkin
2010-06-23 10:13 ` Isaku Yamahata
0 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-23 9:52 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Wed, Jun 23, 2010 at 04:25:20PM +0900, Isaku Yamahata wrote:
> On Mon, Jun 21, 2010 at 03:36:00PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 21, 2010 at 03:03:58PM +0900, Isaku Yamahata wrote:
> > > Set PCI multi-function bit according to multifunction property.
> > > PCI address, devfn ,is exported to users as addr property,
> > > so users can populate pci function(PCIDevice in qemu)
> > > at arbitrary devfn.
> > > It means each function(PCIDevice) don't know whether pci device
> > > (PCIDevice[8]) is multi function or not.
> > > So this patch allows user to set multifunction bit via property
> > > and checks whether multifunction bit is set correctly.
> > >
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >
> > Applying it this way will break bisect.
> > We also need to handle migration compatibility.
> > I propose we split it this way:
> > - patch to add multifunction property (ignored)
> > - set property in builtin devices where appropriate
> > - patch to look at property and set bit in header
>
> Done.
>
> >
> > > ---
> > > changes v3 -> v4:
> > > - introduce multifunction property.
> > >
> > > changes v2 -> v3:
> > > - introduce PCI_FUNC_MAX
> > > - more commit log
> > >
> > > changes v1 -> v2:
> > > ---
> > > hw/pci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > hw/pci.h | 4 ++++
> > > 2 files changed, 61 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index b6c0a10..abc3c1d 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
> > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > > DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> > > + DEFINE_PROP_UINT8("multifunction", PCIDevice, mf, 0),
> >
> > Please make this a bit property, not UINT8. It can be stored in
> > cap_present.
> >
> > > DEFINE_PROP_END_OF_LIST()
> > > }
> > > };
> > > @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > > }
> > >
> > > +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > +{
> >
> > IMO we should just add in pci_register_device:
> >
> > if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
> > dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > } else if (PCI_FUNC(dev->devfn)) {
> > error_report("PCI: single function device can't be populated %x.%x",
> > PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > return -1;
> > }
> >
> > And be done with it.
>
> Unfortunately there are two ways to set the bit.
> - set the bit of all the function.
> Example: Intel X58(north bridge.)
> - set the bit of only function = 0.
> Example: PIIX3, PIIX4, ... ICH10.
>
> lspci -x would help to see what your pc has.
This is correct:
The order in which configuration software probes devices residing on a
bus segment is not specified. Typically, configuration software either
starts with Device Number 0 and works up or starts at Device Number 31
and works down. If a single function device is detected (i.e., bit 7 in
the Header Type register of function 0 is 0), no more functions for that
Device Number will be checked. If a multi-function device is detected
(i.e., bit 7 in the Header Type register of function 0 is 1), then all
remaining Function Numbers will be checked.
So what my proposal would do is set the bit for all functions.
I don't think it matters - do you?
If you want to try and match the behaviour you observe
in actual hardware exactly, we can add
/* Some devices only set multifunction status bit in function 0. */
static void pci_clear_multifunction(...) {
if (PCI_FUNC(dev->devfn))
dev->config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
}
and devices can call this in their init routine.
> >
> > > + uint8_t slot = PCI_SLOT(dev->devfn);
> > > + uint8_t func = PCI_FUNC(dev->devfn);
> > > +
> > > + /* we are here before bus->devices[dev->devfn] = dev */
> > > + assert(!bus->devices[dev->devfn]);
> >
> > Can users trigger this?
> > If yes, this needs and error, not an assert.
> >
> > > +
> > > + if (dev->mf) {
> > > + dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > + }
> > > +
> > > + if (func) {
> >
> > Please open-code func above.
> >
> > > + PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
> > > + if (d && !d->mf) {
> > > + /* function 0 should set multifunction bit */
> > > + error_report("PCI: single function device can't be populated "
> > > + "in function %x.%x", slot, func);
> > > + return -1;
> > > + }
> > > + return 0;
> > > + }
> > > +
> > > + if (dev->mf) {
> > > + return 0;
> > > + }
> > > + /* function 0 indicates single function, so function > 0 must be NULL */
> >
> >
> > We don't need the below test: each function will be checked
> > when it is added.
> >
> > > + for (func = 1; func < PCI_FUNC_MAX; ++func) {
> > > + if (bus->devices[PCI_DEVFN(slot, func)]) {
> > > + error_report("PCI: %x.0 indicates single function, "
> > > + "but %x.%x is already populated.",
> > > + slot, slot, func);
> > > + return -1;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static void pci_config_alloc(PCIDevice *pci_dev)
> > > {
> > > int config_size = pci_config_size(pci_dev);
> > > @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > > if (is_bridge) {
> > > pci_init_wmask_bridge(pci_dev);
> > > }
> > > + if (pci_init_multifunction(bus, pci_dev)) {
> > > + return NULL;
> > > + }
> > >
> > > if (!config_read)
> > > config_read = pci_default_read_config;
> > > @@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
> > > }
> > > }
> > >
> > > -PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> > > +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name)
> > > {
> > > DeviceState *dev;
> > >
> > > dev = qdev_create(&bus->qbus, name);
> > > qdev_prop_set_uint32(dev, "addr", devfn);
> > > + qdev_prop_set_uint8(dev, "multifunction", mf);
> > > return DO_UPCAST(PCIDevice, qdev, dev);
> > > }
> > >
> > > -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> > > +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> > > + const char *name)
> > > {
> > > - PCIDevice *dev = pci_create(bus, devfn, name);
> > > + PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
> > > qdev_init_nofail(&dev->qdev);
> > > return dev;
> > > }
> > >
> > > +PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> > > +{
> > > + return pci_create_mf(bus, devfn, 0, name);
> > > +}
> > > +
> > > +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> > > +{
> > > + return pci_create_simple_mf(bus, devfn, 0, name);
> > > +}
> > > +
> > > static int pci_find_space(PCIDevice *pdev, uint8_t size)
> > > {
> > > int config_size = pci_config_size(pdev);
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 76adc66..685fd44 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -131,6 +131,7 @@ struct PCIDevice {
> > > /* the following fields are read only */
> > > PCIBus *bus;
> > > uint32_t devfn;
> > > + uint8_t mf; /* multi function capabile device */
> >
> > Add a bit in cap_present please.
> >
> > > char name[64];
> > > PCIIORegion io_regions[PCI_NUM_REGIONS];
> > >
> > > @@ -343,6 +344,9 @@ typedef struct {
> > > void pci_qdev_register(PCIDeviceInfo *info);
> > > void pci_qdev_register_many(PCIDeviceInfo *info);
> > >
> > > +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char *name);
> > > +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> > > + const char *name);
> >
> > mf->multifunction
> >
> > But do we need the extra functions? I thought qdev can handle
> > the flag?
>
> If the device tree was fully created via qdev from configuration file,
> that would be correct.
> However the conversion isn't completed, so convenient functions
> are exported to the code which creates the device tree.
> Please notice that devfn here is also property.
>
> While I could add one more parameter to pci_create_simple(),
> I would have to touch many callers.
>
> >
> > > PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
> > > PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
> > >
> > > --
> > > 1.6.6.1
> >
>
> --
> yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-23 9:52 ` Michael S. Tsirkin
@ 2010-06-23 10:13 ` Isaku Yamahata
2010-06-23 10:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-23 10:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Wed, Jun 23, 2010 at 12:52:10PM +0300, Michael S. Tsirkin wrote:
> > > > @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > > > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > > > }
> > > >
> > > > +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > > +{
> > >
> > > IMO we should just add in pci_register_device:
> > >
> > > if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > } else if (PCI_FUNC(dev->devfn)) {
> > > error_report("PCI: single function device can't be populated %x.%x",
> > > PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > > return -1;
> > > }
> > >
> > > And be done with it.
> >
> > Unfortunately there are two ways to set the bit.
> > - set the bit of all the function.
> > Example: Intel X58(north bridge.)
> > - set the bit of only function = 0.
> > Example: PIIX3, PIIX4, ... ICH10.
> >
> > lspci -x would help to see what your pc has.
>
> This is correct:
> The order in which configuration software probes devices residing on a
> bus segment is not specified. Typically, configuration software either
> starts with Device Number 0 and works up or starts at Device Number 31
> and works down. If a single function device is detected (i.e., bit 7 in
> the Header Type register of function 0 is 0), no more functions for that
> Device Number will be checked. If a multi-function device is detected
> (i.e., bit 7 in the Header Type register of function 0 is 1), then all
> remaining Function Numbers will be checked.
>
> So what my proposal would do is set the bit for all functions.
> I don't think it matters - do you?
> If you want to try and match the behaviour you observe
> in actual hardware exactly, we can add
> /* Some devices only set multifunction status bit in function 0. */
> static void pci_clear_multifunction(...) {
> if (PCI_FUNC(dev->devfn))
> dev->config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> }
>
> and devices can call this in their init routine.
Personally I'm okay with either way as long as you accept the patch series.
In fact the existing qemu PIIX3/4 sets the bit of only function 0
and doesn't set the bit of function > 0.
- It would be better not to change the existing behavior.
- If all functions in a device are required to set multifunction bit,
pci ide and ochi usb initialization code must be touched
for pc and mips malta.
Said that, which way do you want to go?
- The current patches.(v5 9/9)
My preference.
- require all functions in a device to set multi function bit.
patch pci ide, ochi usb
It will result in qemu behavior change.
- require all functions in a device to set multi function bit.
patch pci ide, ochi usb.
But try not to chage the existing qemu behavior by using
pci_clear_multifunction()
--
yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-23 10:13 ` Isaku Yamahata
@ 2010-06-23 10:41 ` Michael S. Tsirkin
2010-06-23 23:48 ` Isaku Yamahata
0 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-23 10:41 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Wed, Jun 23, 2010 at 07:13:38PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 23, 2010 at 12:52:10PM +0300, Michael S. Tsirkin wrote:
> > > > > @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > > > > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > > > > }
> > > > >
> > > > > +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > > > +{
> > > >
> > > > IMO we should just add in pci_register_device:
> > > >
> > > > if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
> > > > dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > > } else if (PCI_FUNC(dev->devfn)) {
> > > > error_report("PCI: single function device can't be populated %x.%x",
> > > > PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> > > > return -1;
> > > > }
> > > >
> > > > And be done with it.
> > >
> > > Unfortunately there are two ways to set the bit.
> > > - set the bit of all the function.
> > > Example: Intel X58(north bridge.)
> > > - set the bit of only function = 0.
> > > Example: PIIX3, PIIX4, ... ICH10.
> > >
> > > lspci -x would help to see what your pc has.
> >
> > This is correct:
> > The order in which configuration software probes devices residing on a
> > bus segment is not specified. Typically, configuration software either
> > starts with Device Number 0 and works up or starts at Device Number 31
> > and works down. If a single function device is detected (i.e., bit 7 in
> > the Header Type register of function 0 is 0), no more functions for that
> > Device Number will be checked. If a multi-function device is detected
> > (i.e., bit 7 in the Header Type register of function 0 is 1), then all
> > remaining Function Numbers will be checked.
> >
> > So what my proposal would do is set the bit for all functions.
> > I don't think it matters - do you?
> > If you want to try and match the behaviour you observe
> > in actual hardware exactly, we can add
> > /* Some devices only set multifunction status bit in function 0. */
> > static void pci_clear_multifunction(...) {
> > if (PCI_FUNC(dev->devfn))
> > dev->config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > }
> >
> > and devices can call this in their init routine.
>
> Personally I'm okay with either way as long as you accept the patch series.
>
> In fact the existing qemu PIIX3/4 sets the bit of only function 0
> and doesn't set the bit of function > 0.
> - It would be better not to change the existing behavior.
> - If all functions in a device are required to set multifunction bit,
> pci ide and ochi usb initialization code must be touched
> for pc and mips malta.
> Said that, which way do you want to go?
> - The current patches.(v5 9/9)
> My preference.
I think that your patchset is correct, I'll take it after a bit of review.
I will try to find a bit of time to rearrange the code in pci.c a bit,
but this can come afterwards.
I think it's unfortunate that we need to scan the bus to check
other devices in the same function, but I don't have better ideas.
> - require all functions in a device to set multi function bit.
> patch pci ide, ochi usb
> It will result in qemu behavior change.
>
> - require all functions in a device to set multi function bit.
> patch pci ide, ochi usb.
> But try not to chage the existing qemu behavior by using
> pci_clear_multifunction()
>
> --
> yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-23 10:41 ` Michael S. Tsirkin
@ 2010-06-23 23:48 ` Isaku Yamahata
2010-06-24 8:19 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Isaku Yamahata @ 2010-06-23 23:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Wed, Jun 23, 2010 at 01:41:57PM +0300, Michael S. Tsirkin wrote:
> I will try to find a bit of time to rearrange the code in pci.c a bit,
> but this can come afterwards.
I'd like to move pci_bridge_xxx into pci_bridge.c because
pci.c has grown.
Are you okay with it? Or should I wait for your rearrange?
I'd like to avoid stepping on each other.
--
yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.
2010-06-23 23:48 ` Isaku Yamahata
@ 2010-06-24 8:19 ` Michael S. Tsirkin
0 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2010-06-24 8:19 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: blauwirbel, yu.liu, qemu-devel, aurelien, paul
On Thu, Jun 24, 2010 at 08:48:42AM +0900, Isaku Yamahata wrote:
> On Wed, Jun 23, 2010 at 01:41:57PM +0300, Michael S. Tsirkin wrote:
> > I will try to find a bit of time to rearrange the code in pci.c a bit,
> > but this can come afterwards.
>
> I'd like to move pci_bridge_xxx into pci_bridge.c because
> pci.c has grown.
> Are you okay with it? Or should I wait for your rearrange?
> I'd like to avoid stepping on each other.
I'm looking at rearranging it myself, but doing some API cleanup first.
It will be easier if you keep adding to pci.c meanwhile.
> --
> yamahata
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2010-06-24 8:25 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-21 6:03 [Qemu-devel] [PATCH v4 0/6] pci: multi-function bit fixes Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 1/6] pci: use PCI_DEVFN() where appropriate Isaku Yamahata
2010-06-21 11:57 ` [Qemu-devel] " Michael S. Tsirkin
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 2/6] pci: remove PCIDeviceInfo::header_type Isaku Yamahata
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 3/6] pci: set PCI multi-function bit appropriately Isaku Yamahata
2010-06-21 9:45 ` [Qemu-devel] " Juan Quintela
2010-06-21 9:52 ` Isaku Yamahata
2010-06-21 12:36 ` Michael S. Tsirkin
2010-06-23 7:25 ` Isaku Yamahata
2010-06-23 9:52 ` Michael S. Tsirkin
2010-06-23 10:13 ` Isaku Yamahata
2010-06-23 10:41 ` Michael S. Tsirkin
2010-06-23 23:48 ` Isaku Yamahata
2010-06-24 8:19 ` Michael S. Tsirkin
2010-06-21 6:03 ` [Qemu-devel] [PATCH v4 4/6] pci: don't overwrite multi functio bit in pci header type Isaku Yamahata
2010-06-21 13:15 ` [Qemu-devel] " Michael S. Tsirkin
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 5/6] pci: use pci_create_simple_mf() Isaku Yamahata
2010-06-21 6:04 ` [Qemu-devel] [PATCH v4 6/6] pci_bridge: make pci bridge aware of pci multi function bit Isaku Yamahata
2010-06-21 13:22 ` [Qemu-devel] " Michael S. Tsirkin
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.