pci: add standard bridge device

hw/pci_bridge_dev.c line 35

Quoted 6 lines · Michael S. Tsirkin

hw/pci_bridge_dev.c new file +6 -0

+/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
+ * revision 1.2 */
+/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
+static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
+{
+    return (irq_num + PCI_SLOT(dev->devfn) + irq_num) % PCI_NUM_PINS;

               ^^^^^^^                          ^^^^^^^

Typo. There are 2 irq_num.

Signature · 1 line

yamahata

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

TODO: device hotplug support.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Makefile.objs       |    2 +-
 hw/pci_bridge_dev.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 hw/pci_bridge_dev.c

Makefile.objs +1 -1

index cea15e4..9e82b12 100644
@@ -174,7 +174,7 @@ hw-obj-y += vl.o loader.o
 hw-obj-$(CONFIG_VIRTIO) += virtio.o virtio-console.o
 hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 hw-obj-y += fw_cfg.o
-hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
+hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
 hw-obj-$(CONFIG_PCI) += msix.o msi.o
 hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
 hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o

hw/pci_bridge_dev.c new file +70 -0

new file mode 100644
index 0000000..c7ab5ad
@@ -0,0 +1,70 @@
+/*
+ * Standard PCI Bridge Device
+ *
+ * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <[email protected]>
+ *
+ * http://www.pcisig.com/specifications/conventional/pci_to_pci_bridge_architecture/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "pci_bridge.h"
+#include "pci_ids.h"
+#include "pci_internals.h"
+
+#define REDHAT_PCI_VENDOR_ID 0x1b36
+#define PCI_BRIDGE_DEV_VENDOR_ID REDHAT_PCI_VENDOR_ID
+#define PCI_BRIDGE_DEV_DEVICE_ID 0x1
+
+/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
+ * revision 1.2 */
+/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
+static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
+{
+    return (irq_num + PCI_SLOT(dev->devfn) + irq_num) % PCI_NUM_PINS;
+}
+
+static int pci_bridge_dev_initfn(PCIDevice *dev)
+{
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
+    br->map_irq = pci_bridge_dev_map_irq_fn;
+    /* If we don't specify the name, the bus will be addressed as <id>.0, where
+     * id is the parent id.  But it seems more natural to address the bus using
+     * the parent device name. */
+    if (dev->qdev.id && *dev->qdev.id) {
+        br->bus_name = dev->qdev.id;
+    }
+    return pci_bridge_initfn(dev);
+}
+
+static PCIDeviceInfo pci_bridge_dev_info = {
+    .qdev.name = "pci-bridge",
+    .qdev.desc = "Standard PCI Bridge",
+    .qdev.size = sizeof(PCIBridge),
+    .qdev.reset = pci_bridge_reset,
+    .is_bridge = 1,
+    .config_write = pci_bridge_write_config,
+    .init = pci_bridge_dev_initfn,
+    .exit = pci_bridge_exitfn,
+    .vendor_id = PCI_BRIDGE_DEV_VENDOR_ID,
+    .device_id = PCI_BRIDGE_DEV_DEVICE_ID,
+    .class_id = PCI_CLASS_BRIDGE_PCI,
+};
+
+static void pci_bridge_dev_register(void)
+{
+    pci_qdev_register(&pci_bridge_dev_info);
+}
+
+device_init(pci_bridge_dev_register);

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 9 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

TODO: device hotplug support.

I try this patch, and found that when I use pci bridge, qemu will core dump.

Here is my command line:
/usr/local2/bin/qemu-system-x86_64 -M pc-0.14 -enable-kvm -m 512 -name vm1 -drive file=/var/lib/libvirt/images/vm1.img,if=none,id=drive-ide0-0-0,format=qcow2,cache=writethrough -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -vnc 0.0.0.0:1 -device pci-bridge,id=bridge1,bus=pci.0,addr=0x08.0x0 -netdev user,id=u -device ne2k_pci,id=net2,bus=bridge1,netdev=u

Here is the backtrace:
Core was generated by `/usr/local2/bin/qemu-system-x86_64 -M pc-0.14 -enable-kvm -m 512 -name vm1 -dri'.
Program terminated with signal 11, Segmentation fault.
#0 0x0000000000438e34 in memory_region_add_subregion_common (mr=0x0, offset=49152, subregion=0x1de5d58) at /home/wency/source/qemu/memory.c:1152
1152 QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-2.el6.x86_64 celt051-0.5.1.3-0.el6.x86_64 cyrus-sasl-gssapi-2.1.23-8.el6.x86_64 cyrus-sasl-lib-2.1.23-8.el6.x86_64 cyrus-sasl-md5-2.1.23-8.el6.x86_64 cyrus-sasl-plain-2.1.23-8.el6.x86_64 db4-4.7.25-16.el6.x86_64 glib2-2.22.5-6.el6.x86_64 glibc-2.12-1.25.el6.x86_64 keyutils-libs-1.4-1.el6.x86_64 krb5-libs-1.9-9.el6.x86_64 libX11-1.3-2.el6.x86_64 libXau-1.0.5-1.el6.x86_64 libaio-0.3.107-10.el6.x86_64 libattr-2.4.44-4.el6.x86_64 libcom_err-1.41.12-7.el6.x86_64 libcurl-7.19.7-26.el6.x86_64 libgcrypt-1.4.5-5.el6.x86_64 libgpg-error-1.7-3.el6.x86_64 libidn-1.18-2.el6.x86_64 libjpeg-6b-46.el6.x86_64 libpng-1.2.44-1.el6.x86_64 libselinux-2.0.94-5.el6.x86_64 libssh2-1.2.2-7.el6.x86_64 libtasn1-2.3-3.el6.x86_64 libuuid-2.17.2-12.el6.x86_64 libxcb-1.5-1.el6.x86_64 ncurses-libs-5.7-3.20090208.el6.x86_64 nspr-4.8.7-1.el6.x86_64 nss-3.12.9-9.el6.x86_64 nss-softokn-freebl-3.12.9-3.el6.x86_64 nss-util-3.12.9-1.el6.x86_64 openld

-2.4.23-15.el6.x86_64 openssl-1.0.0-10.el6.x86_64 pixman-0.18.4-1.el6_0.1.x86_64 spice-server-0.8.0-1.el6.x86_64 zlib-1.2.3-25.el6.x86_64 (gdb) bt

#0  0x0000000000438e34 in memory_region_add_subregion_common (mr=0x0, offset=49152, subregion=0x1de5d58) at /home/wency/source/qemu/memory.c:1152
#1  0x0000000000439090 in memory_region_add_subregion_overlap (mr=0x0, offset=49152, subregion=0x1de5d58, priority=1) at /home/wency/source/qemu/memory.c:1194
#2  0x00000000005c55fe in pci_update_mappings (d=0x1de5900) at /home/wency/source/qemu/hw/pci.c:1063
#3  0x00000000005c5982 in pci_default_write_config (d=0x1de5900, addr=4, val=0, l=2) at /home/wency/source/qemu/hw/pci.c:1121
#4  0x00000000005cbfbf in pci_host_config_write_common (pci_dev=0x1de5900, addr=4, limit=256, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:54
#5  0x00000000005cc0d1 in pci_data_write (s=0x1da2b90, addr=2147549188, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:75
#6  0x00000000005cc2b1 in pci_host_data_write (handler=0x1da2b60, addr=3324, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:125
#7  0x000000000042c884 in ioport_simple_writew (opaque=0x1da2b60, addr=3324, value=1) at /home/wency/source/qemu/rwhandler.c:50
#8  0x0000000000499e85 in ioport_write (index=1, address=3324, data=1) at ioport.c:81
#9  0x000000000049a8e1 in cpu_outw (addr=3324, val=1) at ioport.c:280

#10 0x0000000000433c5d in kvm_handle_io (port=3324, data=0x7f0b30f86000, direction=1, size=2, count=1) at /home/wency/source/qemu/kvm-all.c:837 #11 0x00000000004341c8 in kvm_cpu_exec (env=0x1b7fc70) at /home/wency/source/qemu/kvm-all.c:976
#12 0x000000000040da99 in cpu_exec_all () at /home/wency/source/qemu/cpus.c:1102
#13 0x00000000005b60c4 in main_loop () at /home/wency/source/qemu/vl.c:1392
#14 0x00000000005baa49 in main (argc=20, argv=0x7ffffa6b5a38, envp=0x7ffffa6b5ae0) at /home/wency/source/qemu/vl.c:3356

If I do not attach any device on bus bridge1, qemu can work nice.

Thanks
Wen Congyang

From 3ce0000e5a14f0ff7aeac148f9416eac6fa7c6ca Mon Sep 17 00:00:00 2001

At 08/17/2011 04:37 PM, Wen Congyang Write:

Quoted 16 lines

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Review

Quoted 9 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

(quoted diff) +0 -0

 	-device pci-bridge,id=bridge1 \
 	-netdev user,id=u \
 	-device ne2k_pci,id=net2,bus=bridge1,netdev=u
 
 TODO: device hotplug support.

I try this patch, and found that when I use pci bridge, qemu will core dump.

Here is my command line:
/usr/local2/bin/qemu-system-x86_64 -M pc-0.14 -enable-kvm -m 512 -name vm1 -drive file=/var/lib/libvirt/images/vm1.img,if=none,id=drive-ide0-0-0,format=qcow2,cache=writethrough -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -vnc 0.0.0.0:1 -device pci-bridge,id=bridge1,bus=pci.0,addr=0x08.0x0 -netdev user,id=u -device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 6 lines

Here is the backtrace:
Core was generated by `/usr/local2/bin/qemu-system-x86_64 -M pc-0.14 -enable-kvm -m 512 -name vm1 -dri'.
Program terminated with signal 11, Segmentation fault.
#0 0x0000000000438e34 in memory_region_add_subregion_common (mr=0x0, offset=49152, subregion=0x1de5d58) at /home/wency/source/qemu/memory.c:1152
1152 QTAILQ_FOREACH(other,&mr->subregions, subregions_link) {
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-2.el6.x86_64 celt051-0.5.1.3-0.el6.x86_64 cyrus-sasl-gssapi-2.1.23-8.el6.x86_64 cyrus-sasl-lib-2.1.23-8.el6.x86_64 cyrus-sasl-md5-2.1.23-8.el6.x86_64 cyrus-sasl-plain-2.1.23-8.el6.x86_64 db4-4.7.25-16.el6.x86_64 glib2-2.22.5-6.el6.x86_64 glibc-2.12-1.25.el6.x86_64 keyutils-libs-1.4-1.el6.x86_64 krb5-libs-1.9-9.el6.x86_64 libX11-1.3-2.el6.x86_64 libXau-1.0.5-1.el6.x86_64 libaio-0.3.107-10.el6.x86_64 libattr-2.4.44-4.el6.x86_64 libcom_err-1.41.12-7.el6.x86_64 libcurl-7.19.7-26.el6.x86_64 libgcrypt-1.4.5-5.el6.x86_64 libgpg-error-1.7-3.el6.x86_64 libidn-1.18-2.el6.x86_64 libjpeg-6b-46.el6.x86_64 libpng-1.2.44-1.el6.x86_64 libselinux-2.0.94-5.el6.x86_64 libssh2-1.2.2-7.el6.x86_64 libtasn1-2.3-3.el6.x86_64 libuuid-2.17.2-12.el6.x86_64 libxcb-1.5-1.el6.x86_64 ncurses-libs-5.7-3.20090208.el6.x86_64 nspr-4.8.7-1.el6.x86_64 nss-3.12.9-9.el6.x86_64 nss-softokn-freebl-3.12.9-3.el6.x86_64 nss-util-3.12.9-1.el6.x86_64 openld

ap

Quoted 18 lines

-2.4.23-15.el6.x86_64 openssl-1.0.0-10.el6.x86_64 pixman-0.18.4-1.el6_0.1.x86_64 spice-server-0.8.0-1.el6.x86_64 zlib-1.2.3-25.el6.x86_64 (gdb) bt

 #0  0x0000000000438e34 in memory_region_add_subregion_common (mr=0x0, offset=49152, subregion=0x1de5d58) at /home/wency/source/qemu/memory.c:1152
 #1  0x0000000000439090 in memory_region_add_subregion_overlap (mr=0x0, offset=49152, subregion=0x1de5d58, priority=1) at /home/wency/source/qemu/memory.c:1194
 #2  0x00000000005c55fe in pci_update_mappings (d=0x1de5900) at /home/wency/source/qemu/hw/pci.c:1063
 #3  0x00000000005c5982 in pci_default_write_config (d=0x1de5900, addr=4, val=0, l=2) at /home/wency/source/qemu/hw/pci.c:1121
 #4  0x00000000005cbfbf in pci_host_config_write_common (pci_dev=0x1de5900, addr=4, limit=256, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:54
 #5  0x00000000005cc0d1 in pci_data_write (s=0x1da2b90, addr=2147549188, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:75
 #6  0x00000000005cc2b1 in pci_host_data_write (handler=0x1da2b60, addr=3324, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:125
 #7  0x000000000042c884 in ioport_simple_writew (opaque=0x1da2b60, addr=3324, value=1) at /home/wency/source/qemu/rwhandler.c:50
 #8  0x0000000000499e85 in ioport_write (index=1, address=3324, data=1) at ioport.c:81
 #9  0x000000000049a8e1 in cpu_outw (addr=3324, val=1) at ioport.c:280

#10 0x0000000000433c5d in kvm_handle_io (port=3324, data=0x7f0b30f86000, direction=1, size=2, count=1) at /home/wency/source/qemu/kvm-all.c:837 #11 0x00000000004341c8 in kvm_cpu_exec (env=0x1b7fc70) at /home/wency/source/qemu/kvm-all.c:976
#12 0x000000000040da99 in cpu_exec_all () at /home/wency/source/qemu/cpus.c:1102
#13 0x00000000005b60c4 in main_loop () at /home/wency/source/qemu/vl.c:1392
#14 0x00000000005baa49 in main (argc=20, argv=0x7ffffa6b5a38, envp=0x7ffffa6b5ae0) at /home/wency/source/qemu/vl.c:3356

Quoted 5 lines

If I do not attach any device on bus bridge1, qemu can work nice.

Thanks
Wen Congyang

The following patch can fix this problem, but I'm not sure whether it is right.

Quoted 97 lines

At 08/17/2011 04:37 PM, Wen Congyang Write:

Quoted 9 lines

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

     -device pci-bridge,id=bridge1 \
     -netdev user,id=u \
     -device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

I try this patch, and found that when I use pci bridge, qemu will

core dump.

Quoted 3 lines

Here is my command line:
/usr/local2/bin/qemu-system-x86_64 -M pc-0.14 -enable-kvm -m 512

-name vm1 -drive file=/var/lib/libvirt/images/vm1.img,if=none,id=drive-ide0-0-0,format=qcow2,cache=writethrough -device
ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -vnc 0.0.0.0:1 -device pci-bridge,id=bridge1,bus=pci.0,addr=0x08.0x0
-netdev user,id=u -device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 3 lines

Here is the backtrace:
Core was generated by `/usr/local2/bin/qemu-system-x86_64 -M

pc-0.14 -enable-kvm -m 512 -name vm1 -dri'.

Quoted 2 lines

Program terminated with signal 11, Segmentation fault. #0 0x0000000000438e34 in memory_region_add_subregion_common

(mr=0x0, offset=49152, subregion=0x1de5d58) at
/home/wency/source/qemu/memory.c:1152

Quoted 2 lines

1152 QTAILQ_FOREACH(other,&mr->subregions, subregions_link) {
Missing separate debuginfos, use: debuginfo-install

SDL-1.2.14-2.el6.x86_64 celt051-0.5.1.3-0.el6.x86_64 cyrus-sasl-gssapi-2.1.23-8.el6.x86_64 cyrus-sasl-lib-2.1.23-8.el6.x86_64 cyrus-sasl-md5-2.1.23-8.el6.x86_64 cyrus-sasl-plain-2.1.23-8.el6.x86_64 db4-4.7.25-16.el6.x86_64 glib2-2.22.5-6.el6.x86_64 glibc-2.12-1.25.el6.x86_64 keyutils-libs-1.4-1.el6.x86_64 krb5-libs-1.9-9.el6.x86_64 libX11-1.3-2.el6.x86_64 libXau-1.0.5-1.el6.x86_64 libaio-0.3.107-10.el6.x86_64 libattr-2.4.44-4.el6.x86_64 libcom_err-1.41.12-7.el6.x86_64 libcurl-7.19.7-26.el6.x86_64 libgcrypt-1.4.5-5.el6.x86_64 libgpg-error-1.7-3.el6.x86_64 libidn-1.18-2.el6.x86_64 libjpeg-6b-46.el6.x86_64 libpng-1.2.44-1.el6.x86_64 libselinux-2.0.94-5.el6.x86_64 libssh2-1.2.2-7.el6.x86_64 libtasn1-2.3-3.el6.x86_64 libuuid-2.17.2-12.el6.x86_64 libxcb-1.5-1.el6.x86_64 ncurses-libs-5.7-3.20090208.el6.x86_64 nspr-4.8.7-1.el6.x86_64 nss-3.12.9-9.el6.x86_64 nss-softokn-freebl-3.12.9-3.el6.x86_64 nss-util-3.12.9-1.el6.x86_64 openld

ap

Quoted 1 line

-2.4.23-15.el6.x86_64 openssl-1.0.0-10.el6.x86_64

pixman-0.18.4-1.el6_0.1.x86_64 spice-server-0.8.0-1.el6.x86_64 zlib-1.2.3-25.el6.x86_64

Quoted 2 lines

(gdb) bt
#0 0x0000000000438e34 in memory_region_add_subregion_common

(mr=0x0, offset=49152, subregion=0x1de5d58) at
/home/wency/source/qemu/memory.c:1152

Quoted 1 line

#1 0x0000000000439090 in memory_region_add_subregion_overlap

(mr=0x0, offset=49152, subregion=0x1de5d58, priority=1) at /home/wency/source/qemu/memory.c:1194

Quoted 1 line

#2 0x00000000005c55fe in pci_update_mappings (d=0x1de5900) at

/home/wency/source/qemu/hw/pci.c:1063

Quoted 1 line

#3 0x00000000005c5982 in pci_default_write_config (d=0x1de5900,

addr=4, val=0, l=2) at /home/wency/source/qemu/hw/pci.c:1121

Quoted 1 line

#4 0x00000000005cbfbf in pci_host_config_write_common

(pci_dev=0x1de5900, addr=4, limit=256, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:54

Quoted 1 line

#5 0x00000000005cc0d1 in pci_data_write (s=0x1da2b90,

addr=2147549188, val=1, len=2) at
/home/wency/source/qemu/hw/pci_host.c:75

Quoted 1 line

#6 0x00000000005cc2b1 in pci_host_data_write (handler=0x1da2b60,

addr=3324, val=1, len=2) at /home/wency/source/qemu/hw/pci_host.c:125

Quoted 1 line

#7 0x000000000042c884 in ioport_simple_writew (opaque=0x1da2b60,

addr=3324, value=1) at /home/wency/source/qemu/rwhandler.c:50

Quoted 1 line

#8 0x0000000000499e85 in ioport_write (index=1, address=3324,

data=1) at ioport.c:81

Quoted 2 lines

#9 0x000000000049a8e1 in cpu_outw (addr=3324, val=1) at ioport.c:280 #10 0x0000000000433c5d in kvm_handle_io (port=3324,

data=0x7f0b30f86000, direction=1, size=2, count=1) at /home/wency/source/qemu/kvm-all.c:837

Quoted 1 line

#11 0x00000000004341c8 in kvm_cpu_exec (env=0x1b7fc70) at

/home/wency/source/qemu/kvm-all.c:976

Quoted 1 line

#12 0x000000000040da99 in cpu_exec_all () at

/home/wency/source/qemu/cpus.c:1102

Quoted 1 line

#13 0x00000000005b60c4 in main_loop () at

/home/wency/source/qemu/vl.c:1392

Quoted 1 line

#14 0x00000000005baa49 in main (argc=20, argv=0x7ffffa6b5a38,

envp=0x7ffffa6b5ae0) at /home/wency/source/qemu/vl.c:3356

Quoted 6 lines

If I do not attach any device on bus bridge1, qemu can work nice.

Thanks
Wen Congyang

Quoted 2 lines

The following patch can fix this problem, but I'm not sure whether it is right.

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

Basically it gets simpler and correcter.

Quoted 4 lines

Quoted 3 lines

The following patch can fix this problem, but I'm not sure whether it is right.

Quoted 2 lines

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read PCI bridge's config space(command, base and limit). If base> limit, it will set addr to PCI_BAR_UNMAPPED.

I do not find anything that needs to updated to use the memory API.

I add a scsi controller on pci bus1, and a scsi disk on this controller. I can read and write this disk, and I do not meet any problem.

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

The following patch can fix this problem, but I'm not sure whether it is right.

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read PCI bridge's config space(command, base and limit). If base> limit, it will set addr to PCI_BAR_UNMAPPED.

Quoted 1 line

I do not find anything that needs to updated to use the memory API.

Currently it doesn't do any filtering at all. Bridges need to create a new address space, then attach aliases of this region (corresponding to the filtered area and to the legacy vga space) to the parent bus' address space.

Quoted 4 lines

I add a scsi controller on pci bus1, and a scsi disk on this controller. I can read and write this disk, and I do not meet any problem.

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

At 08/19/2011 11:26 PM, Avi Kivity Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

The following patch can fix this problem, but I'm not sure whether it is right.

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read
PCI bridge's config space(command, base and limit). If base> limit,
it will set addr to PCI_BAR_UNMAPPED.

Quoted 6 lines

Quoted 1 line

I do not find anything that needs to updated to use the memory API.

Currently it doesn't do any filtering at all. Bridges need to create a new address space, then attach aliases of this region (corresponding to the filtered area and to the legacy vga space) to the parent bus' address space.

Hmm, does this problem exist before memory API is introduced?

Quoted 6 lines

Quoted 5 lines

I add a scsi controller on pci bus1, and a scsi disk on this controller. I can read and write this disk, and I do not meet any problem.

Quoted 2 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

How to put a BAR outside the filtered area and confirm whether it would be virible to the guest?

Quoted 17 lines

At 08/19/2011 11:26 PM, Avi Kivity Write:

Quoted 3 lines · Wen Congyang

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The following patch can fix this problem, but I'm not sure

whether it

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is right.

It's correct but insufficient, the filtering code

(pci_bridge_filter)

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read
PCI bridge's config space(command, base and limit). If base> limit,
it will set addr to PCI_BAR_UNMAPPED.

I do not find anything that needs to updated to use the memory API.

Quoted 8 lines

Quoted 1 line

Currently it doesn't do any filtering at all. Bridges need to

create a

Quoted 1 line

new address space, then attach aliases of this region

(corresponding to

Quoted 2 lines

the filtered area and to the legacy vga space) to the parent bus' address space.

Hmm, does this problem exist before memory API is introduced?

Yes. There was code to handle it, but I don't think it was correct.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

I add a scsi controller on pci bus1, and a scsi disk on this

controller.

Quoted 3 lines

Quoted 2 lines

I can read and write this disk, and I do not meet any problem.

Quoted 8 lines

Quoted 2 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

How to put a BAR outside the filtered area and confirm whether it would be
virible to the guest?

You could use something like kvm-unit-tests.git to write a simple test that sets up a BAR (say from hw/ivshmem.c), writes and reads to see that it is visible, programs the bridge to filter part of the BAR out, then writes and reads again to verify that the correct part is filtered out.

Quoted 17 lines

At 08/19/2011 11:26 PM, Avi Kivity Write:

Quoted 3 lines · Wen Congyang

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The following patch can fix this problem, but I'm not sure

whether it

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is right.

It's correct but insufficient, the filtering code

(pci_bridge_filter)

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read
PCI bridge's config space(command, base and limit). If base> limit,
it will set addr to PCI_BAR_UNMAPPED.

I do not find anything that needs to updated to use the memory API.

Quoted 8 lines

Quoted 1 line

Currently it doesn't do any filtering at all. Bridges need to

create a

Quoted 1 line

new address space, then attach aliases of this region

(corresponding to

Quoted 2 lines

the filtered area and to the legacy vga space) to the parent bus' address space.

Hmm, does this problem exist before memory API is introduced?

Yes. There was code to handle it, but I don't think it was correct.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

I add a scsi controller on pci bus1, and a scsi disk on this

controller.

Quoted 3 lines

Quoted 2 lines

I can read and write this disk, and I do not meet any problem.

Quoted 8 lines

Quoted 2 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

How to put a BAR outside the filtered area and confirm whether it would be
virible to the guest?

You could use something like kvm-unit-tests.git to write a simple test that sets up a BAR (say from hw/ivshmem.c), writes and reads to see that it is visible, programs the bridge to filter part of the BAR out, then writes and reads again to verify that the correct part is filtered out.

Quoted 5 lines

You could use something like kvm-unit-tests.git to write a simple test that sets up a BAR (say from hw/ivshmem.c), writes and reads to see that it is visible, programs the bridge to filter part of the BAR out, then writes and reads again to verify that the correct part is filtered out.

I am testing ivshmem now. But I do not know how to access the memory specified in the BAR.

Quoted 8 lines

Quoted 3 lines

You could use something like kvm-unit-tests.git to write a simple test that sets up a BAR (say from hw/ivshmem.c), writes and reads to see

that

Quoted 2 lines

it is visible, programs the bridge to filter part of the BAR out, then writes and reads again to verify that the correct part is filtered

out.

Quoted 4 lines

I am testing ivshmem now. But I do not know how to access the memory specified in the BAR.

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You just mmap() the BAR from userspace and play with it.

Quoted 3 lines

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You just mmap() the BAR from userspace and play with it.

When I try to bind ivshmem to uio_pci_generic, I get the following messages:
uio_pci_generic 0000:01:01.0: No IRQ assigned to device: no support for interrupts?

Quoted 5 lines

Quoted 2 lines

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You

just

Quoted 1 line

mmap() the BAR from userspace and play with it.

Quoted 5 lines

When I try to bind ivshmem to uio_pci_generic, I get the following messages:
uio_pci_generic 0000:01:01.0: No IRQ assigned to device: no support for interrupts?

No idea what this means.

At 09/06/2011 03:45 PM, Avi Kivity Write:

Quoted 6 lines · Wen Congyang

Quoted 5 lines

Quoted 2 lines

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You

just

Quoted 1 line

mmap() the BAR from userspace and play with it.

Quoted 7 lines

Quoted 5 lines

When I try to bind ivshmem to uio_pci_generic, I get the following messages:
uio_pci_generic 0000:01:01.0: No IRQ assigned to device: no support for interrupts?

No idea what this means.

PCI 3.0 6.2.4
For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) of the standard dual 8259 configuration. The value 255 is defined as meaning "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 are reserved.

The register is interrupt line.

I read the config of this device, the interrupt line is 0. It means that it uses the IRQ0.

The following is the uio_pci_generic's code:
static int __devinit probe(struct pci_dev *pdev,
const struct pci_device_id *id)

{
	struct uio_pci_generic_dev *gdev;
	int err;

	err = pci_enable_device(pdev);
	if (err) {
		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
			__func__, err);
		return err;
	}

	if (!pdev->irq) {
		dev_warn(&pdev->dev, "No IRQ assigned to device: "
			 "no support for interrupts?\n");
		pci_disable_device(pdev);
		return -ENODEV;
	}

...
}

This function will be called when we write 'domain:bus:slot.function' to /sys/bus/pci/drivers/uio_pci_generic/bind. pdev->irq is 0, it means the device uses IRQ0. But we refuse it. I do not why.

To Michael S. Tsirkin
This code is writen by you. Do you know why you check whether pdev->irq is 0?

Thanks
Wen Congyang

Quoted 1 line

Quoted 47 lines

At 09/06/2011 03:45 PM, Avi Kivity Write:

Quoted 6 lines · Wen Congyang

Quoted 5 lines

Quoted 2 lines

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You

just

Quoted 1 line

mmap() the BAR from userspace and play with it.

Quoted 7 lines

Quoted 5 lines

When I try to bind ivshmem to uio_pci_generic, I get the following messages:
uio_pci_generic 0000:01:01.0: No IRQ assigned to device: no support for interrupts?

No idea what this means.

PCI 3.0 6.2.4
For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) of the standard dual 8259 configuration. The value 255 is defined as meaning "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 are reserved.

The register is interrupt line.

I read the config of this device, the interrupt line is 0. It means that it uses the IRQ0.

The following is the uio_pci_generic's code:
static int __devinit probe(struct pci_dev *pdev,
const struct pci_device_id *id)

{
	struct uio_pci_generic_dev *gdev;
	int err;

	err = pci_enable_device(pdev);
	if (err) {
		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
			__func__, err);
		return err;
	}

	if (!pdev->irq) {
		dev_warn(&pdev->dev, "No IRQ assigned to device: "
			 "no support for interrupts?\n");
		pci_disable_device(pdev);
		return -ENODEV;
	}

...
}

Quoted 10 lines

This function will be called when we write 'domain:bus:slot.function' to /sys/bus/pci/drivers/uio_pci_generic/bind. pdev->irq is 0, it means the device uses IRQ0. But we refuse it. I do not why.

To Michael S. Tsirkin
This code is writen by you. Do you know why you check whether pdev->irq is 0?

Thanks
Wen Congyang

Quoted 1 line

Well I see this in linux:

/*
* Read interrupt line and base address registers.
* The architecture-dependent code can tweak these, of course. */
static void pci_read_irq(struct pci_dev *dev)

{
        unsigned char irq;

        pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
        dev->pin = irq;
        if (irq)
                pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
        dev->irq = irq;
}

Thus a device without an interrupt pin will get irq set to 0, and this seems the right way to detect such devices. I don't think PCI devices really use IRQ0 in practice, its probably used for PC things. More likely the system is misconfigured. Try lspci -vv to see what went wrong.

At 09/07/2011 07:52 PM, Michael S. Tsirkin Write:

Quoted 76 lines · Wen Congyang

Quoted 47 lines

At 09/06/2011 03:45 PM, Avi Kivity Write:

Quoted 6 lines · Wen Congyang

Quoted 5 lines

Quoted 2 lines

Use the uio driver -
http://docs.blackfin.uclinux.org/kernel/generated/uio-howto/. You

just

Quoted 1 line

mmap() the BAR from userspace and play with it.

Quoted 7 lines

Quoted 5 lines

When I try to bind ivshmem to uio_pci_generic, I get the following messages:
uio_pci_generic 0000:01:01.0: No IRQ assigned to device: no support for interrupts?

No idea what this means.

PCI 3.0 6.2.4
For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) of the standard dual 8259 configuration. The value 255 is defined as meaning "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 are reserved.

The register is interrupt line.

I read the config of this device, the interrupt line is 0. It means that it uses the IRQ0.

The following is the uio_pci_generic's code:
static int __devinit probe(struct pci_dev *pdev,
const struct pci_device_id *id)

{
	struct uio_pci_generic_dev *gdev;
	int err;

	err = pci_enable_device(pdev);
	if (err) {
		dev_err(&pdev->dev, "%s: pci_enable_device failed: %d\n",
			__func__, err);
		return err;
	}

	if (!pdev->irq) {
		dev_warn(&pdev->dev, "No IRQ assigned to device: "
			 "no support for interrupts?\n");
		pci_disable_device(pdev);
		return -ENODEV;
	}

...
}

Quoted 10 lines

This function will be called when we write 'domain:bus:slot.function' to /sys/bus/pci/drivers/uio_pci_generic/bind. pdev->irq is 0, it means the device uses IRQ0. But we refuse it. I do not why.

To Michael S. Tsirkin
This code is writen by you. Do you know why you check whether pdev->irq is 0?

Thanks
Wen Congyang

Quoted 1 line

Well I see this in linux:

/*
* Read interrupt line and base address registers.
* The architecture-dependent code can tweak these, of course. */
static void pci_read_irq(struct pci_dev *dev)

{
        unsigned char irq;

        pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
        dev->pin = irq;
        if (irq)
                pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
        dev->irq = irq;
}

Quoted 5 lines

Thus a device without an interrupt pin will get irq set to 0, and this seems the right way to detect such devices. I don't think PCI devices really use IRQ0 in practice, its probably used for PC things. More likely the system is misconfigured. Try lspci -vv to see what went wrong.

Yes, the PCI device shoulde not use IRQ0. I debug qemu's code, and find the PCI_INTERRUPT_LINE register is not set by qemu:
=============
Hardware watchpoint 6: ((uint8_t *) 0x164e410)[0x3c]

Old value = 0 '\000'
New value = 10 '\n'
pci_default_write_config (d=0x1653ed0, addr=60, val=10, l=1) at /home/wency/source/qemu/hw/pci.c:1115
1115 d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
Missing separate debuginfos, use: debuginfo-install cyrus-sasl-gssapi-2.1.23-8.el6.x86_64 cyrus-sasl-md5-2.1.23-8.el6.x86_64 cyrus-sasl-plain-2.1.23-8.el6.x86_64 db4-4.7.25-16.el6.x86_64 (gdb) bt

#0  pci_default_write_config (d=0x1653ed0, addr=60, val=10, l=1) at /home/wency/source/qemu/hw/pci.c:1115
#1  0x00000000004d5827 in pci_host_config_write_common (pci_dev=0x1653ed0, addr=60, limit=256, val=10, len=1) at /home/wency/source/qemu/hw/pci_host.c:54
#2  0x00000000004d5939 in pci_data_write (s=0x15f95a0, addr=2147502140, val=10, len=1) at /home/wency/source/qemu/hw/pci_host.c:75
#3  0x00000000004d5b19 in pci_host_data_write (handler=0x15f9570, addr=3324, val=10, len=1) at /home/wency/source/qemu/hw/pci_host.c:125
#4  0x000000000063ee06 in ioport_simple_writeb (opaque=0x15f9570, addr=3324, value=10) at /home/wency/source/qemu/rwhandler.c:48
#5  0x0000000000470db9 in ioport_write (index=0, address=3324, data=10) at ioport.c:81
#6  0x00000000004717bc in cpu_outb (addr=3324, val=10 '\n') at ioport.c:273
#7  0x00000000005ef25d in kvm_handle_io (port=3324, data=0x7ffff7ff8000, direction=1, size=1, count=1) at /home/wency/source/qemu/kvm-all.c:834
#8  0x00000000005ef7e6 in kvm_cpu_exec (env=0x13da0d0) at /home/wency/source/qemu/kvm-all.c:976
#9  0x00000000005c1a7b in qemu_kvm_cpu_thread_fn (arg=0x13da0d0) at /home/wency/source/qemu/cpus.c:661

#10 0x00000032864077e1 in start_thread () from /lib64/libpthread.so.0 #11 0x00000032858e68ed in clone () from /lib64/libc.so.6
=============

If I put ivshmem on bus 0, the PCI_INTERRUPT_LINE register can be set. So I guess this register is set by bios. I use the newest seabios, and PCI_INTERRUPT_LINE register is not set if the deivce is not on bus0.

# lspci -vv
00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02)
	Subsystem: Red Hat, Inc Qemu virtual machine
	Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
Subsystem: Red Hat, Inc Qemu virtual machine
Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] (prog-if 80 [Master])

	Subsystem: Red Hat, Inc Qemu virtual machine
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Region 0: [virtual] Memory at 000001f0 (32-bit, non-prefetchable) [size=8]
	Region 1: [virtual] Memory at 000003f0 (type 3, non-prefetchable) [size=1]
	Region 2: [virtual] Memory at 00000170 (32-bit, non-prefetchable) [size=8]
	Region 3: [virtual] Memory at 00000370 (type 3, non-prefetchable) [size=1]
	Region 4: I/O ports at d100 [size=16]
	Kernel driver in use: ata_piix
	Kernel modules: ata_piix

00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
Subsystem: Red Hat, Inc Qemu virtual machine
Physical Slot: 1
Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 9
Kernel driver in use: piix4_smbus
Kernel modules: i2c-piix4

00:02.0 VGA compatible controller: Cirrus Logic GD 5446 (prog-if 00 [VGA controller])
Subsystem: Red Hat, Inc Device 1100
Physical Slot: 2
Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Region 0: Memory at fc000000 (32-bit, prefetchable) [size=32M]
Region 1: Memory at f8020000 (32-bit, non-prefetchable) [size=4K]
Expansion ROM at f8000000 [disabled] [size=64K]
Kernel modules: cirrusfb

00:03.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8139/8139C/8139C+ (rev 20)
Subsystem: Red Hat, Inc Device 1100
Physical Slot: 3
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 11
Region 0: I/O ports at d000 [size=256]
Region 1: Memory at f8021000 (32-bit, non-prefetchable) [size=256]
Expansion ROM at f8010000 [disabled] [size=64K]
Kernel driver in use: 8139cp
Kernel modules: 8139too, 8139cp

00:08.0 PCI bridge: Red Hat, Inc. Device 0001 (prog-if 00 [Normal decode])
Physical Slot: 8
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
I/O behind bridge: 0000c000-0000cfff
Memory behind bridge: f0000000-f7ffffff
Prefetchable memory behind bridge: fe000000-fe0fffff
Secondary status: 66MHz+ FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-

01:01.0 RAM memory: Red Hat, Inc Device 1110
Subsystem: Red Hat, Inc Device 1100
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 0
Region 0: Memory at f4000000 (32-bit, non-prefetchable) [disabled] [size=256] Region 2: Memory at f0000000 (32-bit, non-prefetchable) [disabled] [size=32M] Kernel modules: virtio_pci

01:01.1 RAM memory: Red Hat, Inc Device 1110
Subsystem: Red Hat, Inc Device 1100
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 0
Region 0: Memory at f4001000 (32-bit, non-prefetchable) [disabled] [size=256] Region 2: Memory at f2000000 (32-bit, non-prefetchable) [disabled] [size=32M] Kernel modules: virtio_pci

Quoted 1 line

Hi,

Quoted 8 lines

I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
it to uio_pci_generic:

src/pciinit.c +0 -0

@@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
 u32 end)

src/pciinit.c static int pci_bios_init_root_regions(u32 start, · line 578

Quoted 4 lines

src/pciinit.c +2 -1

      pci_bios_init_bus_bases(&busses[0]);
-    pci_bios_map_device_in_bus(0 /* host bus */);
+    for (bus = 0; bus<= MaxPCIBus; bus++) {
+        pci_bios_map_device_in_bus(bus /* host bus */);

No. pci_bios_map_device_in_bus goes down recursively when it finds a bridge, so it should cover all devices already.

src/pciinit.c static int pci_bios_init_root_regions(u32 start, · line 580

Quoted 4 lines

src/pciinit.c +2 -1

-    pci_bios_init_device_in_bus(0 /* host bus */);
+        pci_bios_init_device_in_bus(bus /* host bus */);
+    }

That is correct. Can be done easier though by just not limiting device initialization to a specific bus like in the attached patch. Does that one work for you?

cheers,
Gerd

At 09/08/2011 05:43 PM, Gerd Hoffmann Write:

Quoted 16 lines

Hi,

Quoted 13 lines

I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
it to uio_pci_generic:

src/pciinit.c +2 -1

@@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
 u32 end)
 
      pci_bios_init_bus_bases(&busses[0]);
-    pci_bios_map_device_in_bus(0 /* host bus */);
+    for (bus = 0; bus<= MaxPCIBus; bus++) {
+        pci_bios_map_device_in_bus(bus /* host bus */);

Quoted 2 lines

No. pci_bios_map_device_in_bus goes down recursively when it finds a bridge, so it should cover all devices already.

Yes, pci_bios_map_device() goes down recursively.

Quoted 8 lines

src/pciinit.c static int pci_bios_init_root_regions(u32 start, · line 581

Quoted 4 lines

src/pciinit.c +2 -1

-    pci_bios_init_device_in_bus(0 /* host bus */);
+        pci_bios_init_device_in_bus(bus /* host bus */);
+    }

That is correct. Can be done easier though by just not limiting device initialization to a specific bus like in the attached patch. Does that one work for you?

I test it, and it works for me.

Thanks
Wen Congyang

Quoted 3 lines

cheers,
Gerd

Quoted 17 lines

At 09/08/2011 05:43 PM, Gerd Hoffmann Write:

Quoted 16 lines

Hi,

Quoted 13 lines

I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
it to uio_pci_generic:

src/pciinit.c +2 -1

@@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
 u32 end)
 
      pci_bios_init_bus_bases(&busses[0]);
-    pci_bios_map_device_in_bus(0 /* host bus */);
+    for (bus = 0; bus<= MaxPCIBus; bus++) {
+        pci_bios_map_device_in_bus(bus /* host bus */);

Quoted 4 lines

Quoted 2 lines

No. pci_bios_map_device_in_bus goes down recursively when it finds a bridge, so it should cover all devices already.

Yes, pci_bios_map_device() goes down recursively.

The value seems to be wrong though, I think.
It seems to simply use the interrupt pin as array index. Instead, each bridge should interrupts as follows:

/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
* revision 1.2 */
/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */ static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)

{
    return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
}

until we get to the host bridge.

Quoted 19 lines

Quoted 10 lines

src/pciinit.c static int pci_bios_init_root_regions(u32 start, · line 581

Quoted 6 lines

src/pciinit.c +2 -1

-    pci_bios_init_device_in_bus(0 /* host bus */);
+        pci_bios_init_device_in_bus(bus /* host bus */);
+    }

That is correct. Can be done easier though by just not limiting device initialization to a specific bus like in the attached patch. Does that one work for you?

I test it, and it works for me.

Thanks
Wen Congyang

Quoted 3 lines

cheers,
Gerd

At 09/08/2011 06:42 PM, Michael S. Tsirkin Write:

Quoted 35 lines · Wen Congyang

Quoted 17 lines

At 09/08/2011 05:43 PM, Gerd Hoffmann Write:

Quoted 16 lines

Hi,

Quoted 13 lines

I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
it to uio_pci_generic:

src/pciinit.c +2 -1

@@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
 u32 end)
 
      pci_bios_init_bus_bases(&busses[0]);
-    pci_bios_map_device_in_bus(0 /* host bus */);
+    for (bus = 0; bus<= MaxPCIBus; bus++) {
+        pci_bios_map_device_in_bus(bus /* host bus */);

Quoted 4 lines

Quoted 2 lines

No. pci_bios_map_device_in_bus goes down recursively when it finds a bridge, so it should cover all devices already.

Yes, pci_bios_map_device() goes down recursively.

The value seems to be wrong though, I think.
It seems to simply use the interrupt pin as array index. Instead, each bridge should interrupts as follows:

/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
* revision 1.2 */
/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */ static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)

{
    return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
}

Quoted 1 line

until we get to the host bridge.

I use gdb to debug, and find that this function is never called.

Thanks
Wen Congyang

Quoted 20 lines

Quoted 19 lines

Quoted 10 lines

src/pciinit.c static int pci_bios_init_root_regions(u32 start, · line 581

Quoted 6 lines

src/pciinit.c +2 -1

-    pci_bios_init_device_in_bus(0 /* host bus */);
+        pci_bios_init_device_in_bus(bus /* host bus */);
+    }

That is correct. Can be done easier though by just not limiting device initialization to a specific bus like in the attached patch. Does that one work for you?

I test it, and it works for me.

Thanks
Wen Congyang

Quoted 3 lines

cheers,
Gerd

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

The following patch can fix this problem, but I'm not sure whether it is right.

Quoted 8 lines

Quoted 2 lines

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

I read the function pci_bridge_filter(), and the function only read PCI bridge's config space(command, base and limit). If base> limit, it will set addr to PCI_BAR_UNMAPPED.

I do not find anything that needs to updated to use the memory API.

Currently it doesn't do any filtering at all. Bridges need to create a new address space, then attach aliases of this region (corresponding to the filtered area and to the legacy vga space) to the parent bus' address space.

Quoted 3 lines

I add a scsi controller on pci bus1, and a scsi disk on this controller. I can read and write this disk, and I do not meet any problem.

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

Quoted 3 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

I test it on real hardware. If I put a BAR outside the filterer area, and then run 'lspci -vv', the BAR does not change:

The BAR1 is feafbc00, and it is in the bus2's range.
I map the BAR(mmap /sys/bus/pci/devices/0000\:03\:01.0/resource1), and find I can read and write the memory.

Thanks
Wen Congyang

Quoted 6 lines

Quoted 3 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

I test it on real hardware. If I put a BAR outside the filterer area, and then run 'lspci -vv', the BAR does not change:

...

Quoted 4 lines

The BAR1 is feafbc00, and it is in the bus2's range.
I map the BAR(mmap /sys/bus/pci/devices/0000\:03\:01.0/resource1), and find I can read and write the memory.

Quoted 2 lines

Thanks
Wen Congyang

So, it's as expected. Nothing seems wrong with this picture. But this is not the test that Avi suggested.

At 09/09/2011 03:12 PM, Michael S. Tsirkin Write:

Quoted 15 lines · Wen Congyang

Quoted 6 lines

Quoted 3 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

I test it on real hardware. If I put a BAR outside the filterer area, and then run 'lspci -vv', the BAR does not change:

...

Quoted 4 lines

The BAR1 is feafbc00, and it is in the bus2's range.
I map the BAR(mmap /sys/bus/pci/devices/0000\:03\:01.0/resource1), and find I can read and write the memory.

Quoted 5 lines

Quoted 2 lines

Thanks
Wen Congyang

So, it's as expected. Nothing seems wrong with this picture. But this is not the test that Avi suggested.

Sorry for my misunderstand.
My question is: How to put a BAR outside the filterer area,

and how to know
whether it is visible?

Thanks
Wen Congyang

Quoted 16 lines

At 09/09/2011 03:12 PM, Michael S. Tsirkin Write:

Quoted 15 lines · Wen Congyang

Quoted 6 lines

Quoted 3 lines

However, filtering doesn't work. You could put a BAR outside the filtered area and it would be visible to the guest.

I test it on real hardware. If I put a BAR outside the filterer area, and then run 'lspci -vv', the BAR does not change:

...

Quoted 4 lines

The BAR1 is feafbc00, and it is in the bus2's range.
I map the BAR(mmap /sys/bus/pci/devices/0000\:03\:01.0/resource1), and find I can read and write the memory.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Thanks
Wen Congyang

So, it's as expected. Nothing seems wrong with this picture. But this is not the test that Avi suggested.

Sorry for my misunderstand.
My question is: How to put a BAR outside the filterer area,

Write into address/limit registers on the bridge to make them not cover the BAR behind.

Quoted 5 lines

and how to know
whether it is visible?

Thanks
Wen Congyang

Read the BAR memory as you did.

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

Basically it gets simpler and correcter.

Quoted 4 lines · Avi Kivity

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

Basically it gets simpler and correcter.

I've been struggling with the following problem: bridges have two memory
ranges: prefetcheable and non-prefetcheable.

Memory in the device can be behind the prefetcheable and non-prefetcheable memory range, but things only work correctly if non-prefetcheable memory on the device is put behind a non-prefetcheable range. Prefetcheable memory can go anywhere I think.

This didn't work correctly before the memory API change, but it was easy to fix ... Now I'm not sure how.

Quoted 14 lines · Michael S. Tsirkin

Quoted 4 lines · Avi Kivity

It's correct but insufficient, the filtering code (pci_bridge_filter) needs to be updated to use the memory API.

Basically it gets simpler and correcter.

I've been struggling with the following problem: bridges have two memory
ranges: prefetcheable and non-prefetcheable.

Memory in the device can be behind the prefetcheable and non-prefetcheable memory range, but things only work correctly if non-prefetcheable memory on the device is put behind a non-prefetcheable range. Prefetcheable memory can go anywhere I think.

Quoted 2 lines

This didn't work correctly before the memory API change, but it was easy to fix ... Now I'm not sure how.

If it really matters, you can add a prefetchability attribute to MemoryRegions. Does it though?

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Quoted 3 lines

If it really matters, you can add a prefetchability attribute to MemoryRegions. Does it though?

Well, its another one of these things that
guests *probably* won't in practice use.
But I don't see a way to be sure.

If the guest puts a prefetcheable memory BAR behind a non-prefetcheable range in the bridge, it won't be able to access that BAR, and it should.

Prefetcheable BARs on devices are less common than non-prefetcheable, but they do exist:
I have a system with 2 devices: a VGA controller from Matrox and an ethernet card from Mellanox have prefetcheable BARs.

I'm not sure how prefetcheability attribute will help. Could you explain pls?

Quoted 8 lines · Michael S. Tsirkin

Quoted 3 lines

If it really matters, you can add a prefetchability attribute to MemoryRegions. Does it though?

Well, its another one of these things that
guests *probably* won't in practice use.
But I don't see a way to be sure.

Quoted 3 lines

If the guest puts a prefetcheable memory BAR behind a non-prefetcheable range in the bridge, it won't be able to access that BAR, and it should.

Not sure I understand - on real hardware, does it see the BAR or not?

Quoted 10 lines

Prefetcheable BARs on devices are less common than non-prefetcheable, but they do exist:
I have a system with 2 devices: a VGA controller from Matrox and an ethernet card from Mellanox have prefetcheable BARs.

I'm not sure how prefetcheability attribute will help. Could you explain pls?

Have an attribute for a region "does not allow prefetchable memory" in subregions
Have an attribute for a region "prefetchable memory"

When rendering the memory map, ignore any "prefetchable memory" regions that are subregions of a "does not allow prefetchable memory" region.

(if I understood correctly - not sure)

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 9 lines · Avi Kivity

Quoted 8 lines · Michael S. Tsirkin

Quoted 3 lines

If it really matters, you can add a prefetchability attribute to MemoryRegions. Does it though?

Well, its another one of these things that guests *probably* won't in practice use. But I don't see a way to be sure.

Quoted 5 lines

Quoted 3 lines

If the guest puts a prefetcheable memory BAR behind a non-prefetcheable range in the bridge, it won't be able to access that BAR, and it should.

Not sure I understand - on real hardware, does it see the BAR or not?

It does.

ATM we have each BAR as a memory region, which is
in turn within io or memory address space region.
With bridges, each bridge has a single range
covering legal io addresses below it, and two ranges for memory.

Example from a real system:

         Memory behind bridge: 98200000-982fffff
         Prefetchable memory behind bridge: 0000000097000000-00000000977fffff

And a device can have:

         Region 0: Memory at 98200000 (64-bit, non-prefetchable) [size=1M]
         Region 2: Memory at 97000000 (64-bit, prefetchable) [size=8M]

guest can in theory reprogram this:

         Memory behind bridge: 98200000-98afffff
         Prefetchable memory behind bridge: 0000000097000000-00000000977fffff

and

         Region 0: Memory at 98200000 (64-bit, non-prefetchable) [size=1M]
         Region 2: Memory at 98300000 (64-bit, prefetchable) [size=8M]

and the device will work (presumably, guests will try to avoid this as they assume prefetchable ranges are faster).

Thus, which range the device BAR is behind depends on the programmed values. How do we model that?

A side note on bus filtering:
In cases of bus range partially hinding the BAR from the guest, one can even have multiple non-contigious bits of the BAR visible and the rest hidden.

I'm not saying it's very important to model this, I'm guessing the only important cases are all of the BAR visible and all of the BAR hidden.

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Avi Kivity

Quoted 8 lines · Michael S. Tsirkin

Quoted 3 lines

If it really matters, you can add a prefetchability attribute to MemoryRegions. Does it though?

Well, its another one of these things that guests *probably* won't in practice use. But I don't see a way to be sure.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

If the guest puts a prefetcheable memory BAR behind a non-prefetcheable range in the bridge, it won't be able to access that BAR, and it should.

Not sure I understand - on real hardware, does it see the BAR or not?

It does.

Ok, was different from what I thought. So anything that matches the two windows is exposed (after clipping). If the guest enables the legacy vga range, then there are three regions, with equal treatment, yes?

Quoted 24 lines

ATM we have each BAR as a memory region, which is
in turn within io or memory address space region.
With bridges, each bridge has a single range
covering legal io addresses below it, and two ranges for memory.

Example from a real system:

        Memory behind bridge: 98200000-982fffff
        Prefetchable memory behind bridge: 0000000097000000-00000000977fffff

And a device can have:

        Region 0: Memory at 98200000 (64-bit, non-prefetchable) [size=1M]
        Region 2: Memory at 97000000 (64-bit, prefetchable) [size=8M]

guest can in theory reprogram this:

        Memory behind bridge: 98200000-98afffff
        Prefetchable memory behind bridge: 0000000097000000-00000000977fffff

and

        Region 0: Memory at 98200000 (64-bit, non-prefetchable) [size=1M]
        Region 2: Memory at 98300000 (64-bit, prefetchable) [size=8M]

Quoted 5 lines

and the device will work (presumably, guests will try to avoid this as they assume prefetchable ranges are faster).

Thus, which range the device BAR is behind depends on the programmed values. How do we model that?

Create a memory region for the bridge's address space. This region is not directly added to system_memory or its descendants.

Devices
under the bridge see this region as its pci_address_space(). The region is as large as the entire address space - it does not take into account any windows.

For each of the three windows (pref, non-pref, vga), create an alias with the appropriate start and size. Map the alias into the bridge's parent's pci_address_space(), as subregions.

fx440 does exactly this, with the following cosmetic changes:

- the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
- instead of mapping them to the parent bridge's pci_address_space(), we map them to get_system_memory()

Quoted 4 lines

A side note on bus filtering:
In cases of bus range partially hinding the BAR from the guest, one can even have multiple non-contigious bits of the BAR visible and the rest hidden.

The memory API will deal with this perfectly.

Quoted 3 lines

I'm not saying it's very important to model this, I'm guessing the only important cases are all of the BAR visible and all of the BAR hidden.

It should all work. Including a sub-bridge's windows being fragmented by the parent bridge. Too bad it doesn't matter in practice, because it's a really neat solution to this non-problem.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 3 lines

Create a memory region for the bridge's address space. This region is not directly added to system_memory or its descendants.

I do this for each bridge in the hierarchy, right?

Quoted 5 lines

fx440 does exactly this, with the following cosmetic changes:

- the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
- instead of mapping them to the parent bridge's pci_address_space(), we map them to get_system_memory()

Hmm, what ties the windows of a child bridge
to be within the windows of a parent?

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

Create a memory region for the bridge's address space. This region is not directly added to system_memory or its descendants.

I do this for each bridge in the hierarchy, right?

Each bridge does this independently (so yes).

Quoted 9 lines

Quoted 5 lines

fx440 does exactly this, with the following cosmetic changes:

- the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
- instead of mapping them to the parent bridge's pci_address_space(), we map them to get_system_memory()

Hmm, what ties the windows of a child bridge
to be within the windows of a parent?

system_memory
|
+--- pci0_alias0 (aliases part of pci0)

pci0
|
+--- pci1_alias0 (a bridge)

pci1
|
+--- pci2_alias0 (another bridge)

pci2
|
+--- BAR

When rendering the memory hierarchy, the only parts of BAR which are visible are those that fit the clipping regions pci0_alias0 ^ pci1_alias0 ^ pci2_alias0. If there are multiple aliases (like the low and high PCI holes, and PAM, it becomes (pci0_alias0 v pci0_alias1) ^ (pci1_alias0v pci1_alias1) ^ (pci2_alias0 v pci2_alias1). ( "^" == intersection, "v" == union )

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 35 lines · Avi Kivity

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

Create a memory region for the bridge's address space. This region is not directly added to system_memory or its descendants.

I do this for each bridge in the hierarchy, right?

Each bridge does this independently (so yes).

Quoted 9 lines

Quoted 5 lines

fx440 does exactly this, with the following cosmetic changes:

- the windows are different (vga, pci hole, 64-bit pci area, PAMx, SMRAM)
- instead of mapping them to the parent bridge's pci_address_space(), we map them to get_system_memory()

Hmm, what ties the windows of a child bridge
to be within the windows of a parent?

system_memory
|
+--- pci0_alias0 (aliases part of pci0)

pci0
|
+--- pci1_alias0 (a bridge)

pci1
|
+--- pci2_alias0 (another bridge)

pci2
|
+--- BAR

Quoted 6 lines

When rendering the memory hierarchy, the only parts of BAR which are visible are those that fit the clipping regions pci0_alias0 ^ pci1_alias0 ^ pci2_alias0. If there are multiple aliases (like the low and high PCI holes, and PAM, it becomes (pci0_alias0 v pci0_alias1) ^ (pci1_alias0v pci1_alias1) ^ (pci2_alias0 v pci2_alias1). ( "^" == intersection, "v" == union )

What about BAR directly behind pci0?
That should be unaffected by bridges pci1 and pci2 since the device is not behind that.

It follows naturally:

system_memory
|

  +--- pci0_alias0 (aliases part of pci0)
               |

pci0 <-+

It follows naturally:

Quoted 1 line · Avi Kivity

It follows naturally:

OK, so it seems the following is more or less what you suggest? I'm not sure I create/destroy subregions properly. Both the alias and the subregion get the same start value?

Is the region name for debugging only?

When does priority matter? In case of overlap?

hw/pci_bridge.c +17 -0

+
+static void pci_bridge_region_init(PCIBridge *br)
+{
+    PCIBus *sec_bus =&br->sec_bus;
+    PCIBus *parent = br->dev.bus;
+    memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem",
+	     sec_bus->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH));
+    memory_region_add_subregion_overlap(parent->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+             sec_bus->alias_pref_mem, 1);
+    memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory",
+	     sec_bus->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
+	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY));
+    memory_region_add_subregion_overlap(parent->address_space_mem,

hw/pci_bridge.c +5 -0

+
+static void pci_bridge_region_cleanup(PCIBridge *br)
+{
+    PCIBus *sec_bus =&br->sec_bus;
+    PCIBus *parent = br->dev.bus;

Quoted 6 lines · Michael S. Tsirkin

Quoted 1 line · Avi Kivity

It follows naturally:

OK, so it seems the following is more or less what you suggest? I'm not sure I create/destroy subregions properly. Both the alias and the subregion get the same start value?

Yes (so addresses are not shifted).

Quoted 1 line

Is the region name for debugging only?

For everything except RAM regions (there, the name is also used for save/restore).

Quoted 1 line

When does priority matter? In case of overlap?

Yes. In this case, since overlap resolution is not defined by the spec, the actual priority does not matter.

Quoted 4 lines

hw/pci_bridge.c +0 -0

@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
     return limit;
 }
 

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 144

Quoted 7 lines

hw/pci_bridge.c +7 -0

+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
+{
+    return pci_bridge_get_limit(bridge, type)>=
+        pci_bridge_get_base(bridge, type) ?
+        pci_bridge_get_limit(bridge, type) -
+        pci_bridge_get_base(bridge, type)  + 1 : 0;
+}

Correct

but unreadable.

Doesn't work for limit == 2^64-1, is this a
possible value?

Quoted 17 lines

hw/pci_bridge.c +17 -0

+
+static void pci_bridge_region_init(PCIBridge *br)
+{
+    PCIBus *sec_bus =&br->sec_bus;
+    PCIBus *parent = br->dev.bus;
+    memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem",
+	     sec_bus->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH));
+    memory_region_add_subregion_overlap(parent->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH),
+             sec_bus->alias_pref_mem, 1);
+    memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory",
+	     sec_bus->address_space_mem,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
+	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY));
+    memory_region_add_subregion_overlap(parent->address_space_mem,

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 171

Quoted 10 lines

hw/pci_bridge.c +10 -0

+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY),
+             sec_bus->alias_mem, 1);
+    memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io",
+	     sec_bus->address_space_io,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
+	     pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO));
+    memory_region_add_subregion_overlap(parent->address_space_io,
+	     pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO),
+             sec_bus->alias_io, 1);
+}

This looks right. Might want to use pci_address_space() instead of ->address_space_mem.

Don't you have to do something similar for the vga window?

Quoted 5 lines

hw/pci_bridge.c +5 -0

+
+static void pci_bridge_region_cleanup(PCIBridge *br)
+{
+    PCIBus *sec_bus =&br->sec_bus;
+    PCIBus *parent = br->dev.bus;

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 186

Quoted 10 lines

hw/pci_bridge.c +10 -0

+    memory_region_del_subregion(parent->address_space_mem,
+                                sec_bus->alias_pref_mem);
+    memory_region_destroy(sec_bus->alias_pref_mem);
+    memory_region_del_subregion(parent->address_space_mem,
+                                sec_bus->alias_mem);
+    memory_region_destroy(sec_bus->alias_mem);
+    memory_region_del_subregion(parent->address_space_io,
+                                sec_bus->alias_io);
+    memory_region_destroy(sec_bus->alias_io);
+}

This is fine too.

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 194

Quoted 8 lines

hw/pci_bridge.c +8 -0

+
+static void pci_bridge_update_mappings(PCIBridge *br)
+{
+    /* TODO: this doesn't handle the case of one VCPU
+     * updating the bridge while another accesses an unaffected
+     * region. To fix we'll need new memory region APIs. */
+    pci_bridge_region_cleanup(br);
+    pci_bridge_region_init(br);

memory_region_transaction_{begin,commit}()

(isn't 100% implemented, but at least the API is in place)

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 205

Quoted 11 lines

hw/pci_bridge.c +11 -0

+
+#if 0
+    TODO: do we need to propagate updates to child buses?
+
+    pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
+
+    QLIST_FOREACH(child,&b->child, sibling) {
+        pci_bridge_update_mappings(child);
+    }
+#endif
+}

Don't need this.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 9 lines

Quoted 8 lines

hw/pci_bridge.c +7 -0

+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
+{
+    return pci_bridge_get_limit(bridge, type)>=
+        pci_bridge_get_base(bridge, type) ?
+        pci_bridge_get_limit(bridge, type) -
+        pci_bridge_get_base(bridge, type)  + 1 : 0;
+}

Quoted 1 line

Correct

To make sure: 0 size is ok?

Quoted 2 lines

but unreadable.

variables will make it clearer.

base = pci_bridge_get_base(bridge, type);
limit = pci_bridge_get_limit(bridge, type);
return limit>= bridge ? limit - base + 1 : 0;

Quoted 4 lines

Doesn't work for limit == 2^64-1, is this a
possible value?

You mean when base == 0? Yes, but what can I do?
This seems to be an API limitation of the memory API:
memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX); and the same for aliases: max size seems to be INT64_MAX,
the last byte isn't covered.

The only way to fix this would be to pass first/last instead of offset/size. Too late for such an API change?

VGA I/O addresses (including ISA aliases address - AD[15::10] are not decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

Quoted 9 lines · Michael S. Tsirkin

Quoted 9 lines

Quoted 8 lines

hw/pci_bridge.c +7 -0

+static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type)
+{
+    return pci_bridge_get_limit(bridge, type)>=
+        pci_bridge_get_base(bridge, type) ?
+        pci_bridge_get_limit(bridge, type) -
+        pci_bridge_get_base(bridge, type)  + 1 : 0;
+}

Quoted 3 lines

Quoted 1 line

Correct

To make sure: 0 size is ok?

Yes.

Quoted 3 lines

Quoted 2 lines

but unreadable.

Quoted 5 lines

variables will make it clearer.

base = pci_bridge_get_base(bridge, type);
limit = pci_bridge_get_limit(bridge, type);
return limit>= bridge ? limit - base + 1 : 0;

So long as we're nitpicking, limit + 1 - base.

Quoted 10 lines

Quoted 4 lines

Doesn't work for limit == 2^64-1, is this a
possible value?

You mean when base == 0? Yes, but what can I do?
This seems to be an API limitation of the memory API:
memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX); and the same for aliases: max size seems to be INT64_MAX,
the last byte isn't covered.

Right.

Quoted 2 lines

The only way to fix this would be to pass first/last instead of offset/size. Too late for such an API change?

Way too late. And also won't work, since often the offset is determined by one party and the size by another.

Quoted 3 lines

VGA I/O addresses (including ISA aliases address - AD[15::10] are not decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

These "not decoded" bits mean you need to instantiate tons of aliases to implement correctly.
I plan to add core support for that eventually.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 2 lines

Way too late. And also won't work, since often the offset is determined by one party and the size by another.

For things like BARs, yes - but these don't need to be that big normally. We could add an additinal API that gets first/last parameters. last< first means 0 size. Feasible?

Quoted 5 lines

Quoted 4 lines

VGA I/O addresses (including ISA aliases address - AD[15::10] are not decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

Quoted 3 lines

These "not decoded" bits mean you need to instantiate tons of aliases to implement correctly.
I plan to add core support for that eventually.

There's a flag to enable 16-bit decode actually:
bit 4 in bridge control register.
How does VGA work at the moment without a bridge? Ignores the ISA aliases? then we can do that too I think.

Quoted 3 lines · Michael S. Tsirkin

Quoted 2 lines

Way too late. And also won't work, since often the offset is determined by one party and the size by another.

Quoted 4 lines

For things like BARs, yes - but these don't need to be that big normally. We could add an additinal API that gets first/last parameters. last< first means 0 size. Feasible?

Let's defer this for now.

Does PCI actually have 64-bit addresses?

Note that most/all cpus don't.
We may need 65-bit arithmetic for that.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

VGA I/O addresses (including ISA aliases address - AD[15::10] are not decoded):
AD[9::0] = 3B0h through 3BBh and 3C0h through 3DFh

These "not decoded" bits mean you need to instantiate tons of aliases to implement correctly.
I plan to add core support for that eventually.

Quoted 5 lines

There's a flag to enable 16-bit decode actually:
bit 4 in bridge control register.
How does VGA work at the moment without a bridge? Ignores the ISA aliases? then we can do that too I think.

Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away.

(vga registers its legacy space as a subregion of pci_address_space(dev))

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 3 lines

Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away.

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines

Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away.

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?

No.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away.

Quoted 4 lines

Quoted 2 lines

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?

No.

So a full 16 bit decode is done for now?

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines

Of course it doesn't ignore it. See the 440fx implementation, if you disable VGA access (via the SMRAM register), vga goes away.

Yes but that's for VGA RAM, right? I'm talking about the IO addresses:
are tons of aliases created as you suggest?

Quoted 4 lines

Quoted 1 line

No.

So a full 16 bit decode is done for now?

Correct.

But isn't it needed? Otherwise why don't vga accesses alias with a virtio device at 0xc3c0?

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 2 lines

But isn't it needed? Otherwise why don't vga accesses alias with a virtio device at 0xc3c0?

It really depends on the device I think.

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

But isn't it needed? Otherwise why don't vga accesses alias with a virtio device at 0xc3c0?

It really depends on the device I think.

It's probably the bus. ISA may not decode A10-15, but if the pci/isa bridge does, then it doesn't matter.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Michael S. Tsirkin

Quoted 2 lines

But isn't it needed? Otherwise why don't vga accesses alias with a virtio device at 0xc3c0?

Quoted 5 lines

Quoted 2 lines

It really depends on the device I think.

It's probably the bus. ISA may not decode A10-15, but if the pci/isa bridge does, then it doesn't matter.

The bridge has a 16 bit decode bit. I'm guessing we should make BIOS enable that.

So long as we're nitpicking ...

hw/pci_bridge.c +6 -1

+    pcibus_t size = limit>= base ? limit + 1 - base : 0;
+
+    memory_region_init_alias(alias, name, space, base, size);
+    memory_region_add_subregion_overlap(parent_space, base, alias, 1);
+}
+

@@ -246,10 +312,14 @@ int pci_bridge_initfn(PCIDevice *dev)
                          br->bus_name);
      sec_bus->parent_dev = dev;
      sec_bus->map_irq = br->map_irq;
-    /* TODO: use memory API to perform memory filtering. */

Quoted 3 lines · Michael S. Tsirkin

Quoted 1 line · Avi Kivity

So long as we're nitpicking ...

hw/pci_bridge.c pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) · line 146

Quoted 9 lines

hw/pci_bridge.c +9 -0

+static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
+                                  uint8_t type, const char *name,
+                                  MemoryRegion *space,
+                                  MemoryRegion *parent_space)
+{
+    pcibus_t base = pci_bridge_get_base(&bridge->dev, type);
+    pcibus_t limit = pci_bridge_get_limit(&bridge->dev, type);
+    /* TODO: this doesn't handle base = 0 limit = 2^64 - 1 correctly.
+     * Apparent_spacely no way to do this with existing memory APIs. */

Spelling out spaces is a new fashion?

Quoted 16 lines

hw/pci_bridge.c +7 -4

+    pcibus_t size = limit>= base ? limit + 1 - base : 0;
+
+    memory_region_init_alias(alias, name, space, base, size);
+    memory_region_add_subregion_overlap(parent_space, base, alias, 1);
+}
+

@@ -246,10 +312,14 @@ int pci_bridge_initfn(PCIDevice *dev)
                         br->bus_name);
     sec_bus->parent_dev = dev;
     sec_bus->map_irq = br->map_irq;
-    /* TODO: use memory API to perform memory filtering. */
-    sec_bus->address_space_mem = parent->address_space_mem;
-    sec_bus->address_space_io = parent->address_space_io;
-
+    sec_bus->address_space_mem = g_new(MemoryRegion, 1);

hw/pci_bridge.c int pci_bridge_initfn(PCIDevice *dev) · line 321

Quoted 6 lines

hw/pci_bridge.c +6 -0

+    memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX);
+    sec_bus->address_space_io = g_new(MemoryRegion, 1);
+    memory_region_init(sec_bus->address_space_io, "pci_bridge_io", 65536);
+    sec_bus->alias_pref_mem = g_new(MemoryRegion, 1);
+    sec_bus->alias_mem = g_new(MemoryRegion, 1);
+    sec_bus->alias_io = g_new(MemoryRegion, 1);

Why pointers? Regular fields require less upkeep.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 2 lines

Why pointers? Regular fields require less upkeep.

Good point. Why does PIIX use pointers? I just copied that ...

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

Why pointers? Regular fields require less upkeep.

Good point. Why does PIIX use pointers? I just copied that ...

It doesn't, at least not completely:

struct PCII440FXState {
    PCIDevice dev;
    MemoryRegion *system_memory;
    MemoryRegion *pci_address_space;
    MemoryRegion *ram_memory;
    MemoryRegion pci_hole;
    MemoryRegion pci_hole_64bit;
    PAMMemoryRegion pam_regions[13];
    MemoryRegion smram_region;
    uint8_t smm_enabled;
    bool smram_enabled;
    PIIX3State *piix3;
};

Arguably, ram_memory and pci_address_space should not be pointers, since this address space is supplied by 440fx.

(pci_hole, pci_hole64, pam_regions[], and smram_regions are all aliases analogous to the bridge windows)

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 1 line

From 3ce0000e5a14f0ff7aeac148f9416eac6fa7c6ca Mon Sep 17 00:00:00 2001

From: Wen Congyang <[email protected]>
Date: Thu, 18 Aug 2011 09:33:19 +0800
Subject: [PATCH] PCI_Bridge: use parent bus's address space

The pci device may call pci_register_bar() to use PCI bus's address space. But we forget to init PCI bus's address space if it is not bus 0. It will cause qemu crashed.

Signed-off-by Wen Congyang <[email protected]>

---
 hw/pci_bridge.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

hw/pci_bridge.c +2 -0

index 464d897..df16faa 100644
@@ -246,6 +246,8 @@ int pci_bridge_initfn(PCIDevice *dev)
                         br->bus_name);
     sec_bus->parent_dev = dev;
     sec_bus->map_irq = br->map_irq;
+    sec_bus->address_space_mem = parent->address_space_mem;
+    sec_bus->address_space_io = parent->address_space_io;
 
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);

Signature · 1 line

1.7.1

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 7 lines · Michael S. Tsirkin

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

BTW, I just noticed that -device ne2k_pci,? does
not list the bus option. Any idea why?

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Michael S. Tsirkin

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 2 lines

BTW, I just noticed that -device ne2k_pci,? does
not list the bus option. Any idea why?

Looking... qdev_device_help() shows only device properties, not bus properties. I'd call that a bug.

Quoted 2 lines

Looking... qdev_device_help() shows only device properties, not bus properties. I'd call that a bug.

Hmm, but is "bus" a bus property?

Hi,

Quoted 3 lines

Quoted 2 lines

Looking... qdev_device_help() shows only device properties, not bus properties. I'd call that a bug.

Quoted 1 line

Hmm, but is "bus" a bus property?

It isn't. bus= is handled by qdev core (id= too). addr= actually is a (pci) bus property.

cheers,
Gerd

Hi,

Quoted 4 lines

Quoted 2 lines

Looking... qdev_device_help() shows only device properties, not bus properties. I'd call that a bug.

Hmm, but is "bus" a bus property?

It isn't. bus= is handled by qdev core (id= too). addr= actually is a (pci) bus property.

Quoted 60 lines

Hi, Michael S. Tsirkin

At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Hi, Michael S. Tsirkin

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

Do you have any plan to implement this?

I think this will be needed before merging the bridge code.

What will you plan to support?

1. all PCI-to-PCI bridge is not hotpluggable.
host bridge
|
---------------------
| |
bridge bridge <= *not* hotpluggable
| |
----------- ------------
| | | |
slot slot slot slot <= hotplug here

2. PCI-to-PCI bridge is hotpluggable.

             bridge
               |
      -------------------
         |           |
bridge on slot   bridge on slot         <= hot-plug here
         |           |
      -------     -------
       |   |       |   |
     slot slot   slot slot        <= hot-plug here 

Quoted 11 lines

Quoted 7 lines

I read the qemu's code, and find that qemu uses PIIX4_PM to support pci device hot plugging on PCI bus 0. How to support it on the other bus? Add PIIX4_PM to each PCI bus or implement a new power management?

Thanks
Wen Congyang

There are many valid options. One is shpc interface. I started looking into this but got preempted by other tasks. Hope to get back to this at some point.

Some old OS does not support shpc. So I think it's better to use ACPI to do it.

Quoted 6 lines

Currently, we get which device is removed or inserted by reading the I/O port 0xae00(length: 8 bytes), and _EJ0 method uses I/O port 0xae08(length: 4 bytes). How do we determine this I/O address? Is there any spec to describe it?

Thanks
Wen Congyang

Can we discuss these questions on the mailing list?

At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:

Quoted 46 lines · Wen Congyang

Quoted 45 lines

Hi, Michael S. Tsirkin

At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Hi, Michael S. Tsirkin

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

Do you have any plan to implement this?

I think this will be needed before merging the bridge code.

What will you plan to support?

1. all PCI-to-PCI bridge is not hotpluggable.
host bridge
|
---------------------
| |
bridge bridge <= *not* hotpluggable
| |
----------- ------------
| | | |
slot slot slot slot <= hotplug here

2. PCI-to-PCI bridge is hotpluggable.

             bridge
               |
      -------------------
         |           |
bridge on slot   bridge on slot         <= hot-plug here
         |           |
      -------     -------
       |   |       |   |
     slot slot   slot slot        <= hot-plug here 

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I read the qemu's code, and find that qemu uses PIIX4_PM to support pci device hot plugging on PCI bus 0. How to support it on the other bus? Add PIIX4_PM to each PCI bus or implement a new power management?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Thanks
Wen Congyang

There are many valid options. One is shpc interface. I started looking into this but got preempted by other tasks. Hope to get back to this at some point.

Some old OS does not support shpc. So I think it's better to use ACPI to do it.

Quoted 3 lines

Quoted 3 lines

Currently, we get which device is removed or inserted by reading the I/O port 0xae00(length: 8 bytes), and _EJ0 method uses I/O port 0xae08(length: 4 bytes). How do we determine this I/O address? Is there any spec to describe it?

Quoted 4 lines

Quoted 2 lines

Thanks
Wen Congyang

Can we discuss these questions on the mailing list?

No problem.
I have cced qemu mailing list.

Thanks
Wen Congyang

Quoted 47 lines

At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:

Quoted 46 lines · Wen Congyang

Quoted 45 lines

Hi, Michael S. Tsirkin

At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Hi, Michael S. Tsirkin

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

Do you have any plan to implement this?

I think this will be needed before merging the bridge code.

What will you plan to support?

1. all PCI-to-PCI bridge is not hotpluggable.
host bridge
|
---------------------
| |
bridge bridge <= *not* hotpluggable
| |
----------- ------------
| | | |
slot slot slot slot <= hotplug here

2. PCI-to-PCI bridge is hotpluggable.

             bridge
               |
      -------------------
         |           |
bridge on slot   bridge on slot         <= hot-plug here
         |           |
      -------     -------
       |   |       |   |
     slot slot   slot slot        <= hot-plug here 

It seems easier to start with a non hotpluggable bridge. I'm still trying to understand how is bridge hotplug supposed to work under ACPI, which wants all devices described in a static page.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

I read the qemu's code, and find that qemu uses PIIX4_PM to support pci device hot plugging on PCI bus 0. How to support it on the other bus? Add PIIX4_PM to each PCI bus or implement a new power management?

Thanks
Wen Congyang

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

There are many valid options. One is shpc interface. I started looking into this but got preempted by other tasks. Hope to get back to this at some point.

Some old OS does not support shpc. So I think it's better to use ACPI to do it.

Yes, but ACPI can drive SHPC.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Currently, we get which device is removed or inserted by reading the I/O port 0xae00(length: 8 bytes), and _EJ0 method uses I/O port 0xae08(length: 4 bytes). How do we determine this I/O address? Is there any spec to describe it?

I don't think so.

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Thanks
Wen Congyang

Can we discuss these questions on the mailing list?

No problem.
I have cced qemu mailing list.

Thanks
Wen Congyang

At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:

Quoted 47 lines · Wen Congyang

Quoted 46 lines

Hi, Michael S. Tsirkin

At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Hi, Michael S. Tsirkin

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

Do you have any plan to implement this?

I think this will be needed before merging the bridge code.

What will you plan to support?

1. all PCI-to-PCI bridge is not hotpluggable.
host bridge
|
---------------------
| |
bridge bridge <= *not* hotpluggable
| |
----------- ------------
| | | |
slot slot slot slot <= hotplug here

2. PCI-to-PCI bridge is hotpluggable.

             bridge
               |
      -------------------
         |           |
bridge on slot   bridge on slot         <= hot-plug here
         |           |
      -------     -------
       |   |       |   |
     slot slot   slot slot        <= hot-plug here 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

I read the qemu's code, and find that qemu uses PIIX4_PM to support pci device hot plugging on PCI bus 0. How to support it on the other bus? Add PIIX4_PM to each PCI bus or implement a new power management?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Thanks
Wen Congyang

There are many valid options. One is shpc interface. I started looking into this but got preempted by other tasks. Hope to get back to this at some point.

Some old OS does not support shpc. So I think it's better to use ACPI to do it.

Quoted 3 lines

Quoted 3 lines

Currently, we get which device is removed or inserted by reading the I/O port 0xae00(length: 8 bytes), and _EJ0 method uses I/O port 0xae08(length: 4 bytes). How do we determine this I/O address? Is there any spec to describe it?

Quoted 4 lines

Quoted 2 lines

Thanks
Wen Congyang

Can we discuss these questions on the mailing list?

No problem.
I have cced qemu mailing list.

Thanks
Wen Congyang

Quoted 48 lines

At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:

Quoted 47 lines · Wen Congyang

Quoted 46 lines

Hi, Michael S. Tsirkin

At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:

Quoted 12 lines · Wen Congyang

Quoted 11 lines

Hi, Michael S. Tsirkin

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:

Quoted 8 lines

This adds support for a standard pci to pci bridge, enabling support for more than 32 PCI devices in the system. To use, specify the device id as a 'bus' option. Example:

	-device pci-bridge,id=bridge1 \
	-netdev user,id=u \
	-device ne2k_pci,id=net2,bus=bridge1,netdev=u

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

TODO: device hotplug support.

Do you have any plan to implement this?

I think this will be needed before merging the bridge code.

What will you plan to support?

1. all PCI-to-PCI bridge is not hotpluggable.
host bridge
|
---------------------
| |
bridge bridge <= *not* hotpluggable
| |
----------- ------------
| | | |
slot slot slot slot <= hotplug here

2. PCI-to-PCI bridge is hotpluggable.

             bridge
               |
      -------------------
         |           |
bridge on slot   bridge on slot         <= hot-plug here
         |           |
      -------     -------
       |   |       |   |
     slot slot   slot slot        <= hot-plug here 

If P2P bridge hotplug is supported, pci bus numbering needs refinement for numbering bus sparsely.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

I read the qemu's code, and find that qemu uses PIIX4_PM to support pci device hot plugging on PCI bus 0. How to support it on the other bus? Add PIIX4_PM to each PCI bus or implement a new power management?

Thanks
Wen Congyang

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

There are many valid options. One is shpc interface. I started looking into this but got preempted by other tasks. Hope to get back to this at some point.

Some old OS does not support shpc. So I think it's better to use ACPI to do it.

Out of curiosity, what OS do you have in mind?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Currently, we get which device is removed or inserted by reading the I/O port 0xae00(length: 8 bytes), and _EJ0 method uses I/O port 0xae08(length: 4 bytes). How do we determine this I/O address? Is there any spec to describe it?

As Michael already stated, we can support shpc with acpi. What's the point of enhancing adhoc qemu specific hotplug controller instead of shpc?
We've suffered from its adhoc interface, I think.

thanks,

Quoted 11 lines

Quoted 4 lines

Quoted 2 lines

Thanks
Wen Congyang

Can we discuss these questions on the mailing list?

No problem.
I have cced qemu mailing list.

Thanks
Wen Congyang