[Qemu-devel] [PATCH 0/2] pci: multi-function bit fixes

[Qemu-devel] [PATCH 0/2] pci: multi-function bit fixes

[Isaku Yamahata]

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 (2):
  pci: set PCI multi-function bit appropriately.
  pci: don't overwrite pci header type.

 hw/ac97.c         |    1 -
 hw/acpi_piix4.c   |    1 -
 hw/apb_pci.c      |    3 ++-
 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/pci.c          |   28 ++++++++++++++++++++++++++++
 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 -
 22 files changed, 32 insertions(+), 27 deletions(-)

[Qemu-devel] [PATCH 1/2] pci: set PCI multi-function bit appropriately.

[Isaku Yamahata]

set PCI multi-function bit appropriately.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 3777c1c..a01e9ac 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -578,6 +578,33 @@ static void pci_init_wmask_bridge(PCIDevice *d)
     pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
 }
 
+static void pci_init_header_type(PCIBus *bus, PCIDevice *dev,
+                                 uint8_t devfn, uint8_t header_type)
+{
+    uint8_t slot = PCI_SLOT(devfn);
+    uint8_t func_max = 8;
+    uint8_t func;
+
+    dev->config[PCI_HEADER_TYPE] = header_type;
+
+    for (func = 0; func < func_max; ++func) {
+        if (bus->devices[PCI_DEVFN(slot, func)]) {
+            break;
+        }
+    }
+    if (func == func_max) {
+        return;
+    }
+
+    for (func = 0; func < func_max; ++func) {
+        if (bus->devices[PCI_DEVFN(slot, func)]) {
+            bus->devices[PCI_DEVFN(slot, func)]->config[PCI_HEADER_TYPE] |=
+                PCI_HEADER_TYPE_MULTI_FUNCTION;
+        }
+    }
+    dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
+}
+
 static void pci_config_alloc(PCIDevice *pci_dev)
 {
     int config_size = pci_config_size(pci_dev);
@@ -632,6 +659,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     if (header_type == PCI_HEADER_TYPE_BRIDGE) {
         pci_init_wmask_bridge(pci_dev);
     }
+    pci_init_header_type(bus, pci_dev, devfn, header_type);
 
     if (!config_read)
         config_read = pci_default_read_config;
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/2] pci: don't overwrite pci header type.

[Isaku Yamahata]

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.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/ac97.c         |    1 -
 hw/acpi_piix4.c   |    1 -
 hw/apb_pci.c      |    3 ++-
 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, 4 insertions(+), 27 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 31c8d70..cdf3bc2 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
                  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);
+                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
+                  PCI_HEADER_TYPE_MULTI_FUNCTION) | 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 d14d05e..51e8c46 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;
 
@@ -336,8 +335,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 f0a773d..7b1c94b 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

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> 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.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

...

> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 31c8d70..cdf3bc2 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
>                   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);
> +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);

what is this doing?

-- 
MST

Re: [Qemu-devel] [PATCH 2/2] pci: don't overwrite pci header type.

[malc]

On Tue, 15 Jun 2010, Isaku Yamahata wrote:

> 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.

ac97 changes are fine with me

[..snip..]

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Isaku Yamahata]

On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> > 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.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> ...
> 
> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > index 31c8d70..cdf3bc2 100644
> > --- a/hw/apb_pci.c
> > +++ b/hw/apb_pci.c
> > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> >                   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);
> > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> 
> what is this doing?

It changes the header type to normal device(bit 1-7) without overwriting
multi function bit(bit 8).

Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
on the other hand pbc_pci_host_init() sets the register
to PCI_HEADER_TYPE_NORMAL.
To be honest I don't know why it does so, but that is what Blue wants.
So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
unchanged.

If you don't like this hunk, I'll drop this hunk and leave it to Blue.
What do you think?


static PCIDeviceInfo pbm_pci_host_info = {
    .qdev.name = "pbm",
    .qdev.size = sizeof(PCIDevice),
    .init      = pbm_pci_host_init,
    .header_type  = PCI_HEADER_TYPE_BRIDGE,	<<<<< Here
};

-- 
yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> > > 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.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > ...
> > 
> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > > index 31c8d70..cdf3bc2 100644
> > > --- a/hw/apb_pci.c
> > > +++ b/hw/apb_pci.c
> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> > >                   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);
> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> > 
> > what is this doing?
> 
> It changes the header type to normal device(bit 1-7) without overwriting
> multi function bit(bit 8).

Don't we know what the multi function bit value is?

> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> on the other hand pbc_pci_host_init() sets the register
> to PCI_HEADER_TYPE_NORMAL.
> To be honest I don't know why it does so, but that is what Blue wants.

BTW I think it would be prettier to have is_bridge instead of header_type
as a qdev property. Agree?

> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
> unchanged.
> 
> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
> What do you think?

Blue Swirl, could you comment on this please?

> static PCIDeviceInfo pbm_pci_host_info = {
>     .qdev.name = "pbm",
>     .qdev.size = sizeof(PCIDevice),
>     .init      = pbm_pci_host_init,
>     .header_type  = PCI_HEADER_TYPE_BRIDGE,	<<<<< Here
> };
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Isaku Yamahata]

On Wed, Jun 16, 2010 at 11:54:25AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
> > On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> > > > 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.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > 
> > > ...
> > > 
> > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > > > index 31c8d70..cdf3bc2 100644
> > > > --- a/hw/apb_pci.c
> > > > +++ b/hw/apb_pci.c
> > > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> > > >                   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);
> > > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> > > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> > > 
> > > what is this doing?
> > 
> > It changes the header type to normal device(bit 1-7) without overwriting
> > multi function bit(bit 8).
> 
> Don't we know what the multi function bit value is?

pci generic initialization, pci_qdev_init(), in pci.c sets (or clears) the bit
and then calls the device specific initialization function, pbm_pci_host_init()
in this case.
So we shouldn't clear the bit unconditionally.


> > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> > on the other hand pbc_pci_host_init() sets the register
> > to PCI_HEADER_TYPE_NORMAL.
> > To be honest I don't know why it does so, but that is what Blue wants.
> 
> BTW I think it would be prettier to have is_bridge instead of header_type
> as a qdev property. Agree?

The spec version 3.0 defines three header types.
0:normal device, 1:pci-to-pci bridge, 2:card bus bridge
So I'd like the name a bit more generic than is_bridge.
Any suggestion?


> > So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
> > unchanged.
> > 
> > If you don't like this hunk, I'll drop this hunk and leave it to Blue.
> > What do you think?
> 
> Blue Swirl, could you comment on this please?
> 
> > static PCIDeviceInfo pbm_pci_host_info = {
> >     .qdev.name = "pbm",
> >     .qdev.size = sizeof(PCIDevice),
> >     .init      = pbm_pci_host_init,
> >     .header_type  = PCI_HEADER_TYPE_BRIDGE,	<<<<< Here
> > };
> > 
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Wed, Jun 16, 2010 at 06:43:53PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 16, 2010 at 11:54:25AM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
> > > On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> > > > > 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.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > 
> > > > ...
> > > > 
> > > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > > > > index 31c8d70..cdf3bc2 100644
> > > > > --- a/hw/apb_pci.c
> > > > > +++ b/hw/apb_pci.c
> > > > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> > > > >                   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);
> > > > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> > > > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> > > > 
> > > > what is this doing?
> > > 
> > > It changes the header type to normal device(bit 1-7) without overwriting
> > > multi function bit(bit 8).
> > 
> > Don't we know what the multi function bit value is?
> 
> pci generic initialization, pci_qdev_init(), in pci.c sets (or clears) the bit
> and then calls the device specific initialization function, pbm_pci_host_init()
> in this case.
> So we shouldn't clear the bit unconditionally.
> 
> 
> > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> > > on the other hand pbc_pci_host_init() sets the register
> > > to PCI_HEADER_TYPE_NORMAL.
> > > To be honest I don't know why it does so, but that is what Blue wants.
> > 
> > BTW I think it would be prettier to have is_bridge instead of header_type
> > as a qdev property. Agree?
> 
> The spec version 3.0 defines three header types.
> 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge
> So I'd like the name a bit more generic than is_bridge.
> Any suggestion?

Could we just have functions that set up header for
each type, such as
pci_init_normal_header()
pci_init_p2p_bridge_header()
pci_init_cardbus_header()

> > > So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
> > > unchanged.
> > > 
> > > If you don't like this hunk, I'll drop this hunk and leave it to Blue.
> > > What do you think?
> > 
> > Blue Swirl, could you comment on this please?
> > 
> > > static PCIDeviceInfo pbm_pci_host_info = {
> > >     .qdev.name = "pbm",
> > >     .qdev.size = sizeof(PCIDevice),
> > >     .init      = pbm_pci_host_init,
> > >     .header_type  = PCI_HEADER_TYPE_BRIDGE,	<<<<< Here
> > > };
> > > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Isaku Yamahata]

On Wed, Jun 16, 2010 at 02:19:44PM +0300, Michael S. Tsirkin wrote:
> > > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> > > > on the other hand pbc_pci_host_init() sets the register
> > > > to PCI_HEADER_TYPE_NORMAL.
> > > > To be honest I don't know why it does so, but that is what Blue wants.
> > > 
> > > BTW I think it would be prettier to have is_bridge instead of header_type
> > > as a qdev property. Agree?
> > 
> > The spec version 3.0 defines three header types.
> > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge
> > So I'd like the name a bit more generic than is_bridge.
> > Any suggestion?
> 
> Could we just have functions that set up header for
> each type, such as
> pci_init_normal_header()
> pci_init_p2p_bridge_header()
> pci_init_cardbus_header()

I see. You mean device specific initialization function should
call one of them. Then header_type property will be dropped.

I'll split pci p2p bridge related functions into a file
at first. Then introduce helper functions.
-- 
yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Wed, Jun 16, 2010 at 08:38:18PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 16, 2010 at 02:19:44PM +0300, Michael S. Tsirkin wrote:
> > > > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> > > > > on the other hand pbc_pci_host_init() sets the register
> > > > > to PCI_HEADER_TYPE_NORMAL.
> > > > > To be honest I don't know why it does so, but that is what Blue wants.
> > > > 
> > > > BTW I think it would be prettier to have is_bridge instead of header_type
> > > > as a qdev property. Agree?
> > > 
> > > The spec version 3.0 defines three header types.
> > > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge
> > > So I'd like the name a bit more generic than is_bridge.
> > > Any suggestion?
> > 
> > Could we just have functions that set up header for
> > each type, such as
> > pci_init_normal_header()
> > pci_init_p2p_bridge_header()
> > pci_init_cardbus_header()
> 
> I see. You mean device specific initialization function should
> call one of them. Then header_type property will be dropped.
> I'll split pci p2p bridge related functions into a file
> at first.
> Then introduce helper functions.

Just to clarify what I meant:
the common pci spec implementation should be in pci.c,
any platform that supports pci will need it.
What I think we want to move to pc_pci_bridge.c or such
is this:
static PCIDeviceInfo bridge_info = {
    .qdev.name    = "pci-bridge",
    .qdev.size    = sizeof(PCIBridge),
    .init         = pci_bridge_initfn,
    .exit         = pci_bridge_exitfn,
    .config_write = pci_bridge_write_config, 
    .header_type  = PCI_HEADER_TYPE_BRIDGE,
    .qdev.props   = (Property[]) {
        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
        DEFINE_PROP_END_OF_LIST(),
    }
};

Because if I understand correctly, this is not "the bridge",
it's just a pci bridge that PC has, but it is currently
instanciated even on platforms where it's unused.
This way we can avoid linking it on these platforms.

But I think the bridge header setup is common
so it should be implemented in a set of
common functions and stay in pci.c, then all bridges
can call these functions.

> -- 
> yamahata

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Blue Swirl]

On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
>> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
>> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
>> > > 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.
>> > >
>> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> >
>> > ...
>> >
>> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> > > index 31c8d70..cdf3bc2 100644
>> > > --- a/hw/apb_pci.c
>> > > +++ b/hw/apb_pci.c
>> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
>> > >                   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);
>> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
>> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
>> >
>> > what is this doing?
>>
>> It changes the header type to normal device(bit 1-7) without overwriting
>> multi function bit(bit 8).
>
> Don't we know what the multi function bit value is?
>
>> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
>> on the other hand pbc_pci_host_init() sets the register
>> to PCI_HEADER_TYPE_NORMAL.
>> To be honest I don't know why it does so, but that is what Blue wants.
>
> BTW I think it would be prettier to have is_bridge instead of header_type
> as a qdev property. Agree?

Good idea.

>> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
>> unchanged.
>>
>> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
>> What do you think?
>
> Blue Swirl, could you comment on this please?

I'd go for is_bridge and drop the override for header type in apb_pci.c then.

>> static PCIDeviceInfo pbm_pci_host_info = {
>>     .qdev.name = "pbm",
>>     .qdev.size = sizeof(PCIDevice),
>>     .init      = pbm_pci_host_init,
>>     .header_type  = PCI_HEADER_TYPE_BRIDGE,   <<<<< Here
>> };
>>
>> --
>> yamahata
>

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote:
> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> >> > > 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.
> >> > >
> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >> >
> >> > ...
> >> >
> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> >> > > index 31c8d70..cdf3bc2 100644
> >> > > --- a/hw/apb_pci.c
> >> > > +++ b/hw/apb_pci.c
> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> >> > >                   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);
> >> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> >> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> >> >
> >> > what is this doing?
> >>
> >> It changes the header type to normal device(bit 1-7) without overwriting
> >> multi function bit(bit 8).
> >
> > Don't we know what the multi function bit value is?
> >
> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> >> on the other hand pbc_pci_host_init() sets the register
> >> to PCI_HEADER_TYPE_NORMAL.
> >> To be honest I don't know why it does so, but that is what Blue wants.
> >
> > BTW I think it would be prettier to have is_bridge instead of header_type
> > as a qdev property. Agree?
> 
> Good idea.
> 
> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
> >> unchanged.
> >>
> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
> >> What do you think?
> >
> > Blue Swirl, could you comment on this please?
> 
> I'd go for is_bridge and drop the override for header type in apb_pci.c then.

Yes, but what header type does it need?

> >> static PCIDeviceInfo pbm_pci_host_info = {
> >>     .qdev.name = "pbm",
> >>     .qdev.size = sizeof(PCIDevice),
> >>     .init      = pbm_pci_host_init,
> >>     .header_type  = PCI_HEADER_TYPE_BRIDGE,   <<<<< Here
> >> };
> >>
> >> --
> >> yamahata
> >

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Blue Swirl]

On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote:
>> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
>> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
>> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
>> >> > > 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.
>> >> > >
>> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> >> >
>> >> > ...
>> >> >
>> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> >> > > index 31c8d70..cdf3bc2 100644
>> >> > > --- a/hw/apb_pci.c
>> >> > > +++ b/hw/apb_pci.c
>> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
>> >> > >                   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);
>> >> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
>> >> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
>> >> >
>> >> > what is this doing?
>> >>
>> >> It changes the header type to normal device(bit 1-7) without overwriting
>> >> multi function bit(bit 8).
>> >
>> > Don't we know what the multi function bit value is?
>> >
>> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
>> >> on the other hand pbc_pci_host_init() sets the register
>> >> to PCI_HEADER_TYPE_NORMAL.
>> >> To be honest I don't know why it does so, but that is what Blue wants.
>> >
>> > BTW I think it would be prettier to have is_bridge instead of header_type
>> > as a qdev property. Agree?
>>
>> Good idea.
>>
>> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
>> >> unchanged.
>> >>
>> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
>> >> What do you think?
>> >
>> > Blue Swirl, could you comment on this please?
>>
>> I'd go for is_bridge and drop the override for header type in apb_pci.c then.
>
> Yes, but what header type does it need?

The type should be bridge (to allow writes to bridge registers), but
PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM
specification says so).

>> >> static PCIDeviceInfo pbm_pci_host_info = {
>> >>     .qdev.name = "pbm",
>> >>     .qdev.size = sizeof(PCIDevice),
>> >>     .init      = pbm_pci_host_init,
>> >>     .header_type  = PCI_HEADER_TYPE_BRIDGE,   <<<<< Here
>> >> };
>> >>
>> >> --
>> >> yamahata
>> >
>

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Michael S. Tsirkin]

On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote:
> On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote:
> >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
> >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
> >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
> >> >> > > 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.
> >> >> > >
> >> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> >> >> >
> >> >> > ...
> >> >> >
> >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> >> >> > > index 31c8d70..cdf3bc2 100644
> >> >> > > --- a/hw/apb_pci.c
> >> >> > > +++ b/hw/apb_pci.c
> >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
> >> >> > >                   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);
> >> >> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
> >> >> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
> >> >> >
> >> >> > what is this doing?
> >> >>
> >> >> It changes the header type to normal device(bit 1-7) without overwriting
> >> >> multi function bit(bit 8).
> >> >
> >> > Don't we know what the multi function bit value is?
> >> >
> >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
> >> >> on the other hand pbc_pci_host_init() sets the register
> >> >> to PCI_HEADER_TYPE_NORMAL.
> >> >> To be honest I don't know why it does so, but that is what Blue wants.
> >> >
> >> > BTW I think it would be prettier to have is_bridge instead of header_type
> >> > as a qdev property. Agree?
> >>
> >> Good idea.
> >>
> >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
> >> >> unchanged.
> >> >>
> >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
> >> >> What do you think?
> >> >
> >> > Blue Swirl, could you comment on this please?
> >>
> >> I'd go for is_bridge and drop the override for header type in apb_pci.c then.
> >
> > Yes, but what header type does it need?
> 
> The type should be bridge (to allow writes to bridge registers), but
> PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM
> specification says so).

I can no longer get the PBM specs now: are there
alternative links? Need to fix links in code.


> >> >> static PCIDeviceInfo pbm_pci_host_info = {
> >> >>     .qdev.name = "pbm",
> >> >>     .qdev.size = sizeof(PCIDevice),
> >> >>     .init      = pbm_pci_host_init,
> >> >>     .header_type  = PCI_HEADER_TYPE_BRIDGE,   <<<<< Here
> >> >> };
> >> >>
> >> >> --
> >> >> yamahata
> >> >
> >

[Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Blue Swirl]

On Wed, Jun 16, 2010 at 7:22 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote:
>> On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote:
>> >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
>> >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
>> >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
>> >> >> > > 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.
>> >> >> > >
>> >> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> >> >> >
>> >> >> > ...
>> >> >> >
>> >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> >> >> > > index 31c8d70..cdf3bc2 100644
>> >> >> > > --- a/hw/apb_pci.c
>> >> >> > > +++ b/hw/apb_pci.c
>> >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
>> >> >> > >                   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);
>> >> >> > > +                 (pci_get_byte(d->config + PCI_HEADER_TYPE) &
>> >> >> > > +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
>> >> >> >
>> >> >> > what is this doing?
>> >> >>
>> >> >> It changes the header type to normal device(bit 1-7) without overwriting
>> >> >> multi function bit(bit 8).
>> >> >
>> >> > Don't we know what the multi function bit value is?
>> >> >
>> >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
>> >> >> on the other hand pbc_pci_host_init() sets the register
>> >> >> to PCI_HEADER_TYPE_NORMAL.
>> >> >> To be honest I don't know why it does so, but that is what Blue wants.
>> >> >
>> >> > BTW I think it would be prettier to have is_bridge instead of header_type
>> >> > as a qdev property. Agree?
>> >>
>> >> Good idea.
>> >>
>> >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
>> >> >> unchanged.
>> >> >>
>> >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
>> >> >> What do you think?
>> >> >
>> >> > Blue Swirl, could you comment on this please?
>> >>
>> >> I'd go for is_bridge and drop the override for header type in apb_pci.c then.
>> >
>> > Yes, but what header type does it need?
>>
>> The type should be bridge (to allow writes to bridge registers), but
>> PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM
>> specification says so).
>
> I can no longer get the PBM specs now: are there
> alternative links? Need to fix links in code.

That sucks. I hope this is only temporary.

>
>
>> >> >> static PCIDeviceInfo pbm_pci_host_info = {
>> >> >>     .qdev.name = "pbm",
>> >> >>     .qdev.size = sizeof(PCIDevice),
>> >> >>     .init      = pbm_pci_host_init,
>> >> >>     .header_type  = PCI_HEADER_TYPE_BRIDGE,   <<<<< Here
>> >> >> };
>> >> >>
>> >> >> --
>> >> >> yamahata
>> >> >
>> >
>

Re: [Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type.

[Anthony Liguori]

On 06/16/2010 02:22 PM, Michael S. Tsirkin wrote:
> On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote:
>    
>> On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin<mst@redhat.com>  wrote:
>>      
>>> On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote:
>>>        
>>>> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin<mst@redhat.com>  wrote:
>>>>          
>>>>> On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote:
>>>>>            
>>>>>> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote:
>>>>>>              
>>>>>>> On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote:
>>>>>>>                
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
>>>>>>>>                  
>>>>>>> ...
>>>>>>>
>>>>>>>                
>>>>>>>> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>>>>>>>> index 31c8d70..cdf3bc2 100644
>>>>>>>> --- a/hw/apb_pci.c
>>>>>>>> +++ b/hw/apb_pci.c
>>>>>>>> @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d)
>>>>>>>>                    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);
>>>>>>>> +                 (pci_get_byte(d->config + PCI_HEADER_TYPE)&
>>>>>>>> +                  PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL);
>>>>>>>>                  
>>>>>>> what is this doing?
>>>>>>>                
>>>>>> It changes the header type to normal device(bit 1-7) without overwriting
>>>>>> multi function bit(bit 8).
>>>>>>              
>>>>> Don't we know what the multi function bit value is?
>>>>>
>>>>>            
>>>>>> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo,
>>>>>> on the other hand pbc_pci_host_init() sets the register
>>>>>> to PCI_HEADER_TYPE_NORMAL.
>>>>>> To be honest I don't know why it does so, but that is what Blue wants.
>>>>>>              
>>>>> BTW I think it would be prettier to have is_bridge instead of header_type
>>>>> as a qdev property. Agree?
>>>>>            
>>>> Good idea.
>>>>
>>>>          
>>>>>> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7)
>>>>>> unchanged.
>>>>>>
>>>>>> If you don't like this hunk, I'll drop this hunk and leave it to Blue.
>>>>>> What do you think?
>>>>>>              
>>>>> Blue Swirl, could you comment on this please?
>>>>>            
>>>> I'd go for is_bridge and drop the override for header type in apb_pci.c then.
>>>>          
>>> Yes, but what header type does it need?
>>>        
>> The type should be bridge (to allow writes to bridge registers), but
>> PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM
>> specification says so).
>>      
> I can no longer get the PBM specs now: are there
> alternative links? Need to fix links in code.
>    

BTW, I set up http://wiki.qemu.org/Documentation/HardwareManuals so we 
could start archiving these specification when allowed.

Regards,

Anthony Liguori

>    
>>>>>> static PCIDeviceInfo pbm_pci_host_info = {
>>>>>>      .qdev.name = "pbm",
>>>>>>      .qdev.size = sizeof(PCIDevice),
>>>>>>      .init      = pbm_pci_host_init,
>>>>>>      .header_type  = PCI_HEADER_TYPE_BRIDGE,<<<<<  Here
>>>>>> };
>>>>>>
>>>>>> --
>>>>>> yamahata
>>>>>>              
>>>>>            
>>>        
>