[Qemu-devel] [PATCH] pci: add standard bridge device

[Qemu-devel] [PATCH] pci: add standard bridge device

[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

TODO: device hotplug support.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Makefile.objs       |    2 +-
 hw/pci_bridge_dev.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 hw/pci_bridge_dev.c

diff --git a/Makefile.objs b/Makefile.objs
index cea15e4..9e82b12 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -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
diff --git a/hw/pci_bridge_dev.c b/hw/pci_bridge_dev.c
new file mode 100644
index 0000000..c7ab5ad
--- /dev/null
+++ b/hw/pci_bridge_dev.c
@@ -0,0 +1,70 @@
+/*
+ * Standard PCI Bridge Device
+ *
+ * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com>
+ *
+ * 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);
-- 
1.7.5.53.gc233e

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Isaku Yamahata]

On Mon, Jul 04, 2011 at 12:43:59PM +0300, Michael S. Tsirkin wrote:
> +/* 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.

-- 
yamahata

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Tue, Jul 05, 2011 at 10:29:36PM +0900, Isaku Yamahata wrote:
> On Mon, Jul 04, 2011 at 12:43:59PM +0300, Michael S. Tsirkin wrote:
> > +/* 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.

Good catch, thanks.

> -- 
> yamahata

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
> 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 openldap
-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

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  Makefile.objs       |    2 +-
>  hw/pci_bridge_dev.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 1 deletions(-)
>  create mode 100644 hw/pci_bridge_dev.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index cea15e4..9e82b12 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -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
> diff --git a/hw/pci_bridge_dev.c b/hw/pci_bridge_dev.c
> new file mode 100644
> index 0000000..c7ab5ad
> --- /dev/null
> +++ b/hw/pci_bridge_dev.c
> @@ -0,0 +1,70 @@
> +/*
> + * Standard PCI Bridge Device
> + *
> + * Copyright (c) 2011 Red Hat Inc. Author: Michael S. Tsirkin <mst@redhat.com>
> + *
> + * 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);

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/17/2011 04:37 PM, Wen Congyang Write:
> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
>> 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

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

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

>From 3ce0000e5a14f0ff7aeac148f9416eac6fa7c6ca Mon Sep 17 00:00:00 2001
From: Wen Congyang <wency@cn.fujitsu.com>
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 <wency@cn.fujitsu.com>

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

diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 464d897..df16faa 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -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);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/17/2011 08:22 PM, Wen Congyang wrote:
> At 08/17/2011 04:37 PM, Wen Congyang Write:
> >  At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
> >>  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
>
> ap
> >  -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
> >
>
> 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.

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/18/2011 11:15 PM, Avi Kivity Write:
> On 08/17/2011 08:22 PM, Wen Congyang wrote:
>> At 08/17/2011 04:37 PM, Wen Congyang Write:
>> >  At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
>> >>  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
>>
>> ap
>> >  -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
>> >
>>
>> 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.

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.

Thanks
Wen Congyang

> 
> Basically it gets simpler and correcter.
> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/18/2011 10:12 PM, Wen Congyang wrote:
> >>
> >>  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.
>
> 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.

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

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/19/2011 11:26 PM, Avi Kivity Write:
> On 08/18/2011 10:12 PM, Wen Congyang wrote:
>> >>
>> >>  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.
>>
>> 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?

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

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/22/2011 06:13 AM, Wen Congyang wrote:
> At 08/19/2011 11:26 PM, Avi Kivity Write:
> >  On 08/18/2011 10:12 PM, Wen Congyang wrote:
> >>  >>
> >>  >>   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.
> >>
> >>  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?

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

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

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Thu, Aug 18, 2011 at 08:15:43AM -0700, Avi Kivity wrote:
> 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.

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Thu, Aug 18, 2011 at 11:22:31AM +0800, Wen Congyang wrote:
> >From 3ce0000e5a14f0ff7aeac148f9416eac6fa7c6ca Mon Sep 17 00:00:00 2001
> From: Wen Congyang <wency@cn.fujitsu.com>
> 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 <wency@cn.fujitsu.com>

I've applied this for now so we can make progress.

> ---
>  hw/pci_bridge.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 464d897..df16faa 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -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);
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/26/2011 12:43 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 18, 2011 at 08:15:43AM -0700, Avi Kivity wrote:
> >  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.

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

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Aug 28, 2011 at 10:50:49AM +0300, Avi Kivity wrote:
> On 08/26/2011 12:43 PM, Michael S. Tsirkin wrote:
> >On Thu, Aug 18, 2011 at 08:15:43AM -0700, Avi Kivity wrote:
> >>  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.
> 
> 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?


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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/28/2011 02:41 PM, Michael S. Tsirkin wrote:
> >
> >  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.

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

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Aug 28, 2011 at 04:10:14PM +0300, Avi Kivity wrote:
> On 08/28/2011 02:41 PM, Michael S. Tsirkin wrote:
> >>
> >>  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.
> 
> Not sure I understand - on real hardware, does it see the BAR or not?

It does.

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

Sorry, I've been unclear. Or maybe I misunderstood?

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.


> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 08/28/2011 04:42 PM, Michael S. Tsirkin wrote:
> On Sun, Aug 28, 2011 at 04:10:14PM +0300, Avi Kivity wrote:
> >  On 08/28/2011 02:41 PM, Michael S. Tsirkin wrote:
> >  >>
> >  >>   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.
> >
> >  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?

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

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()

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

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/22/2011 02:23 PM, Avi Kivity Write:
> On 08/22/2011 06:13 AM, Wen Congyang wrote:
>> At 08/19/2011 11:26 PM, Avi Kivity Write:
>> >  On 08/18/2011 10:12 PM, Wen Congyang wrote:
>> >>  >>
>> >>  >>   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.
>> >>
>> >>  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?
> 
> Yes.  There was code to handle it, but I don't think it was correct.
> 
>>
>> >
>> >>  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.
>>
>> 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.
> 

I test it with rtl8139(Because I have a such real hardware).
I add some fprintf in the callback function: rtl8139_mmio_writeb(),
rtl8139_mmio_writel(), rtl8139_mmio_writew(), rtl8139_mmio_readb(),
rtl8139_mmio_readw(), rtl8139_mmio_readl().
My test way:
1. unbind the device from rtl8139 driver
2. modify the BAR
3. reprobe the device

If the BAR is visible, the OS will access it when we reprobe the device.
If the BAR is not visible, the OS will access it.
I can not reprobe the real device if the BAR is not visible. But I can
reprobe the virtual device if the BAR is not visible.

Thanks
Wen Congyang

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/22/2011 02:23 PM, Avi Kivity Write:
> On 08/22/2011 06:13 AM, Wen Congyang wrote:
>> At 08/19/2011 11:26 PM, Avi Kivity Write:
>> >  On 08/18/2011 10:12 PM, Wen Congyang wrote:
>> >>  >>
>> >>  >>   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.
>> >>
>> >>  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?
> 
> Yes.  There was code to handle it, but I don't think it was correct.
> 
>>
>> >
>> >>  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.
>>
>> 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.

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

Thanks
Wen Congyang

> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/02/2011 05:56 AM, Wen Congyang wrote:
> >
> >  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.
>
>

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Aug 28, 2011 at 04:53:33PM +0300, Avi Kivity wrote:
> On 08/28/2011 04:42 PM, Michael S. Tsirkin wrote:
> >On Sun, Aug 28, 2011 at 04:10:14PM +0300, Avi Kivity wrote:
> >>  On 08/28/2011 02:41 PM, Michael S. Tsirkin wrote:
> >>  >>
> >>  >>   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.
> >>
> >>  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?
> 
> >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?
> 
> 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?

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

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



> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:
> >
> >  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).

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 03:40:58PM +0300, Avi Kivity wrote:
> On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:
> >>
> >>  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).
> 
> >>  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 )

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


> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:01 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 03:40:58PM +0300, Avi Kivity wrote:
> >  On 09/04/2011 03:30 PM, Michael S. Tsirkin wrote:
> >  >>
> >  >>   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).
> >
> >  >>   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 )
>
> 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 <-+
   |
   +--- pci1_alias0 (a bridge)
   |            |
   +--- BAR0.0  |
                |
pci1 <-+
   |
   +--- pci2_alias0 (another bridge)
   |            |
   +--- BAR1.0  |
                |
pci2 <-+
   |
   +--- BAR2.0


BAR0.0 is only filtered by pci0_alias*, BAR1.0 is filtered by 
pci[01]_alias*.  Since pci_register_bar() adds the BAR as a subregion of 
the bridge's pci_address_space(), it works without any global knowledge, 
with this topology, or if pci1 and pci2 are siblings instead of parent 
and child.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:05 PM, Avi Kivity wrote:
>
> It follows naturally:
>
> system_memory
>   |
>   +--- pci0_alias0 (aliases part of pci0)
>                |
> pci0 <-+

The pointer from pci0_alias to pci0 looked a lot better when I drew it.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> 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?

diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..56dfa18 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
-    r->filtered_size = size;
     r->type = type;
     r->memory = NULL;
 
@@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num)
     return pci_dev->io_regions[region_num].addr;
 }
 
-static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
-                              uint8_t type)
-{
-    pcibus_t base = *addr;
-    pcibus_t limit = *addr + *size - 1;
-    PCIDevice *br;
-
-    for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
-        uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-
-        if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-            if (!(cmd & PCI_COMMAND_IO)) {
-                goto no_map;
-            }
-        } else {
-            if (!(cmd & PCI_COMMAND_MEMORY)) {
-                goto no_map;
-            }
-        }
-
-        base = MAX(base, pci_bridge_get_base(br, type));
-        limit = MIN(limit, pci_bridge_get_limit(br, type));
-    }
-
-    if (base > limit) {
-        goto no_map;
-    }
-    *addr = base;
-    *size = limit - base + 1;
-    return;
-no_map:
-    *addr = PCI_BAR_UNMAPPED;
-    *size = 0;
-}
-
 static pcibus_t pci_bar_address(PCIDevice *d,
 				int reg, uint8_t type, pcibus_t size)
 {
@@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
     int i;
-    pcibus_t new_addr, filtered_size;
+    pcibus_t new_addr;
 
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &d->io_regions[i];
@@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d)
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
 
-        /* bridge filtering */
-        filtered_size = r->size;
-        if (new_addr != PCI_BAR_UNMAPPED) {
-            pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
-        }
-
         /* This bar isn't changed */
-        if (new_addr == r->addr && filtered_size == r->filtered_size)
+        if (new_addr == r->addr)
             continue;
 
         /* now do the real mapping */
@@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d)
             memory_region_del_subregion(r->address_space, r->memory);
         }
         r->addr = new_addr;
-        r->filtered_size = filtered_size;
         if (r->addr != PCI_BAR_UNMAPPED) {
-            /*
-             * TODO: currently almost all the map funcions assumes
-             * filtered_size == size and addr & ~(size - 1) == addr.
-             * However with bridge filtering, they aren't always true.
-             * Teach them such cases, such that filtered_size < size and
-             * addr & (size - 1) != 0.
-             */
             if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
                 memory_region_add_subregion_overlap(r->address_space,
                                                     r->addr,
@@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
     return res;
 }
 
-static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
-{
-    pci_update_mappings(d);
-}
-
-void pci_bridge_update_mappings(PCIBus *b)
-{
-    PCIBus *child;
-
-    pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
-
-    QLIST_FOREACH(child, &b->child, sibling) {
-        pci_bridge_update_mappings(child);
-    }
-}
-
 /* Whether a given bus number is in range of the secondary
  * bus of the given bridge device. */
 static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 391217e..65e1568 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -90,7 +90,6 @@ typedef struct PCIIORegion {
     pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
     pcibus_t size;
-    pcibus_t filtered_size;
     uint8_t type;
     MemoryRegion *memory;
     MemoryRegion *address_space;
@@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-void pci_bridge_update_mappings(PCIBus *b);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index e0b339e..469dfe8 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
+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;
+}
+
+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,
+	     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);
+}
+
+static void pci_bridge_region_cleanup(PCIBridge *br)
+{
+    PCIBus *sec_bus = &br->sec_bus;
+    PCIBus *parent = br->dev.bus;
+    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);
+}
+
+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);
+
+#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
+}
+
 /* default write_config function for PCI-to-PCI bridge */
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
@@ -151,7 +220,7 @@ void pci_bridge_write_config(PCIDevice *d,
         /* memory base/limit, prefetchable base/limit and
            io base/limit upper 16 */
         ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        pci_bridge_update_mappings(&s->sec_bus);
+        pci_bridge_update_mappings(s);
     }
 
     newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
@@ -247,9 +316,14 @@ int pci_bridge_initfn(PCIDevice *dev)
     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);
+    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);
+    pci_bridge_region_init(br);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
     return 0;
@@ -259,8 +333,17 @@ int pci_bridge_initfn(PCIDevice *dev)
 int pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
+    PCIBus *sec_bus = &s->sec_bus;
     assert(QLIST_EMPTY(&s->sec_bus.child));
     QLIST_REMOVE(&s->sec_bus, sibling);
+    pci_bridge_region_cleanup(s);
+    g_free(sec_bus->alias_pref_mem);
+    g_free(sec_bus->alias_mem);
+    g_free(sec_bus->alias_io);
+    memory_region_destroy(sec_bus->address_space_mem);
+    g_free(sec_bus->address_space_mem);
+    memory_region_destroy(sec_bus->address_space_io);
+    g_free(sec_bus->address_space_io);
     /* qbus_free() is called automatically by qdev_free() */
     return 0;
 }
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index c7fd23d..578c8d2 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -27,6 +27,9 @@ struct PCIBus {
     target_phys_addr_t mem_base;
     MemoryRegion *address_space_mem;
     MemoryRegion *address_space_io;
+    MemoryRegion *alias_pref_mem;
+    MemoryRegion *alias_mem;
+    MemoryRegion *alias_io;
 
     QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
     QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> >  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).

> Is the region name for debugging only?

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

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

> @@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
>       return limit;
>   }
>
> +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?

> +
> +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,
> +	     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?

> +
> +static void pci_bridge_region_cleanup(PCIBridge *br)
> +{
> +    PCIBus *sec_bus =&br->sec_bus;
> +    PCIBus *parent = br->dev.bus;
> +    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.

> +
> +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)
> +
> +#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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 04:55:33PM +0300, Avi Kivity wrote:
> On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote:
> >>  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).
> 
> >Is the region name for debugging only?
> 
> For everything except RAM regions (there, the name is also used for
> save/restore).
> 
> >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.
> 
> >@@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
> >      return limit;
> >  }
> >
> >+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

To make sure: 0 size is ok?

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


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

> >+
> >+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,
> >+	     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?

Yes.
These are controlled by VGA Enable and VGA palette snooping enable
bits in the bridge, which we don't yet implement. It's on the TODO,
at the moment vga devices behind a bridge don't work.
the relevant bits from the spec:

------------------

The VGA Enable bit in the Bridge Control register (see Section 3.2.5.18)
is used to control response by the bridge to both the VGA frame buffer
addresses and to the VGA register addresses. When a VGA compatible
device is located downstream of a PCI-to-PCI bridge, the VGA Enable bit
must be set. When set, the bridge will positively decode and forward
memory accesses to VGA frame buffer addresses and I/O accesses to VGA
registers from the primary to secondary interface and block forwarding
of these same accesses from the secondary to primary interface (see
Section 4.5.1).

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

We also may need to foward palette snooping accesses:

behaviors of each device type are described below.
The VGA palette addresses are as follows (inclusive of ISA aliases -
AD[15::10] are not decoded):
AD[9::0] = 3C6h, 3C8h, and 3C9h

------------------


> >+
> >+static void pci_bridge_region_cleanup(PCIBridge *br)
> >+{
> >+    PCIBus *sec_bus =&br->sec_bus;
> >+    PCIBus *parent = br->dev.bus;
> >+    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.
> 
> >+
> >+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)

OK, I'll stick them here.

> >+
> >+#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.
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote:
> >  >
> >  >+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
>
> To make sure: 0 size is ok?

Yes.

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

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

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

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

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


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> On 09/04/2011 05:21 PM, Michael S. Tsirkin wrote:
> >>  >
> >>  >+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
> >
> >To make sure: 0 size is ok?
> 
> Yes.
> 
> >
> >>  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;
> 
> So long as we're nitpicking, limit + 1 - base.

If this makes you happier, OK :)

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

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?

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

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.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote:
> >  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?

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.

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:14:22PM +0300, Avi Kivity wrote:
> On 09/04/2011 05:54 PM, Michael S. Tsirkin wrote:
> >>  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?
> 
> Let's defer this for now.
> 
> Does PCI actually have 64-bit addresses?

Yes.

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

Jut using start/limit carefully should be enough ...


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

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

> (vga registers its legacy space as a subregion of pci_address_space(dev))
> 
> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> So long as we're nitpicking ...

OK, this should do it then.


diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..56dfa18 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
-    r->filtered_size = size;
     r->type = type;
     r->memory = NULL;
 
@@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num)
     return pci_dev->io_regions[region_num].addr;
 }
 
-static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
-                              uint8_t type)
-{
-    pcibus_t base = *addr;
-    pcibus_t limit = *addr + *size - 1;
-    PCIDevice *br;
-
-    for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
-        uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
-
-        if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-            if (!(cmd & PCI_COMMAND_IO)) {
-                goto no_map;
-            }
-        } else {
-            if (!(cmd & PCI_COMMAND_MEMORY)) {
-                goto no_map;
-            }
-        }
-
-        base = MAX(base, pci_bridge_get_base(br, type));
-        limit = MIN(limit, pci_bridge_get_limit(br, type));
-    }
-
-    if (base > limit) {
-        goto no_map;
-    }
-    *addr = base;
-    *size = limit - base + 1;
-    return;
-no_map:
-    *addr = PCI_BAR_UNMAPPED;
-    *size = 0;
-}
-
 static pcibus_t pci_bar_address(PCIDevice *d,
 				int reg, uint8_t type, pcibus_t size)
 {
@@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
     int i;
-    pcibus_t new_addr, filtered_size;
+    pcibus_t new_addr;
 
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &d->io_regions[i];
@@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d)
 
         new_addr = pci_bar_address(d, i, r->type, r->size);
 
-        /* bridge filtering */
-        filtered_size = r->size;
-        if (new_addr != PCI_BAR_UNMAPPED) {
-            pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
-        }
-
         /* This bar isn't changed */
-        if (new_addr == r->addr && filtered_size == r->filtered_size)
+        if (new_addr == r->addr)
             continue;
 
         /* now do the real mapping */
@@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d)
             memory_region_del_subregion(r->address_space, r->memory);
         }
         r->addr = new_addr;
-        r->filtered_size = filtered_size;
         if (r->addr != PCI_BAR_UNMAPPED) {
-            /*
-             * TODO: currently almost all the map funcions assumes
-             * filtered_size == size and addr & ~(size - 1) == addr.
-             * However with bridge filtering, they aren't always true.
-             * Teach them such cases, such that filtered_size < size and
-             * addr & (size - 1) != 0.
-             */
             if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
                 memory_region_add_subregion_overlap(r->address_space,
                                                     r->addr,
@@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
     return res;
 }
 
-static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
-{
-    pci_update_mappings(d);
-}
-
-void pci_bridge_update_mappings(PCIBus *b)
-{
-    PCIBus *child;
-
-    pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
-
-    QLIST_FOREACH(child, &b->child, sibling) {
-        pci_bridge_update_mappings(child);
-    }
-}
-
 /* Whether a given bus number is in range of the secondary
  * bus of the given bridge device. */
 static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 391217e..65e1568 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -90,7 +90,6 @@ typedef struct PCIIORegion {
     pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
 #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
     pcibus_t size;
-    pcibus_t filtered_size;
     uint8_t type;
     MemoryRegion *memory;
     MemoryRegion *address_space;
@@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-void pci_bridge_update_mappings(PCIBus *b);
 
 void pci_device_deassert_intx(PCIDevice *dev);
 
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index e0b339e..bedd615 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -135,6 +135,72 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
+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. */
+    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);
+}
+
+static void pci_bridge_cleanup_alias(MemoryRegion *alias,
+                                     MemoryRegion *parent_space)
+{
+    memory_region_del_subregion(parent_space, alias);
+    memory_region_destroy(alias);
+}
+
+static void pci_bridge_region_init(PCIBridge *br)
+{
+    PCIBus *sec_bus = &br->sec_bus;
+    PCIBus *parent = br->dev.bus;
+    pci_bridge_init_alias(br, sec_bus->alias_pref_mem,
+                          PCI_BASE_ADDRESS_MEM_PREFETCH,
+                          "pci_bridge_pref_mem",
+                          sec_bus->address_space_mem,
+                          parent->address_space_mem);
+    pci_bridge_init_alias(br, sec_bus->alias_mem,
+                          PCI_BASE_ADDRESS_SPACE_MEMORY,
+                          "pci_bridge_mem",
+                          sec_bus->address_space_mem,
+                          parent->address_space_mem);
+    pci_bridge_init_alias(br, sec_bus->alias_io,
+                          PCI_BASE_ADDRESS_SPACE_IO,
+                          "pci_bridge_io",
+                          sec_bus->address_space_io,
+                          parent->address_space_io);
+   /* TODO: VGA and VGA palatte snooping support. */
+}
+
+static void pci_bridge_region_cleanup(PCIBridge *br)
+{
+    PCIBus *sec_bus = &br->sec_bus;
+    PCIBus *parent = br->dev.bus;
+    pci_bridge_cleanup_alias(sec_bus->alias_io,
+                             parent->address_space_io);
+    pci_bridge_cleanup_alias(sec_bus->alias_mem,
+                             parent->address_space_mem);
+    pci_bridge_cleanup_alias(sec_bus->alias_pref_mem,
+                             parent->address_space_mem);
+}
+
+static void pci_bridge_update_mappings(PCIBridge *br)
+{
+    /* Make updates atomic to: handle the case of one VCPU updating the bridge
+     * while another accesses an unaffected region. */
+    memory_region_transaction_begin();
+    pci_bridge_region_cleanup(br);
+    pci_bridge_region_init(br);
+    memory_region_transaction_commit();
+}
+
 /* default write_config function for PCI-to-PCI bridge */
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
@@ -151,7 +217,7 @@ void pci_bridge_write_config(PCIDevice *d,
         /* memory base/limit, prefetchable base/limit and
            io base/limit upper 16 */
         ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        pci_bridge_update_mappings(&s->sec_bus);
+        pci_bridge_update_mappings(s);
     }
 
     newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
@@ -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);
+    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);
+    pci_bridge_region_init(br);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
     return 0;
@@ -259,8 +329,17 @@ int pci_bridge_initfn(PCIDevice *dev)
 int pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
+    PCIBus *sec_bus = &s->sec_bus;
     assert(QLIST_EMPTY(&s->sec_bus.child));
     QLIST_REMOVE(&s->sec_bus, sibling);
+    pci_bridge_region_cleanup(s);
+    g_free(sec_bus->alias_pref_mem);
+    g_free(sec_bus->alias_mem);
+    g_free(sec_bus->alias_io);
+    memory_region_destroy(sec_bus->address_space_mem);
+    g_free(sec_bus->address_space_mem);
+    memory_region_destroy(sec_bus->address_space_io);
+    g_free(sec_bus->address_space_io);
     /* qbus_free() is called automatically by qdev_free() */
     return 0;
 }
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index c7fd23d..578c8d2 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -27,6 +27,9 @@ struct PCIBus {
     target_phys_addr_t mem_base;
     MemoryRegion *address_space_mem;
     MemoryRegion *address_space_io;
+    MemoryRegion *alias_pref_mem;
+    MemoryRegion *alias_mem;
+    MemoryRegion *alias_io;
 
     QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */
     QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >
> >  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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:26 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> >  So long as we're nitpicking ...
>
> +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?

> +    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);
> +    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.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >>
> >>  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.

So a full 16 bit decode is done for now?

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:42:42PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:26 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 05:36:56PM +0300, Avi Kivity wrote:
> >>  So long as we're nitpicking ...
> >
> >+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?

Will fix.

> >+    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);
> >+    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.

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

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
> >  On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >  >>
> >  >>   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.
>
> 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?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 06:46 PM, Michael S. Tsirkin wrote:
> >
> >  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)

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:46:46PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:45 PM, Michael S. Tsirkin wrote:
> >On Sun, Sep 04, 2011 at 06:37:08PM +0300, Avi Kivity wrote:
> >>  On 09/04/2011 06:24 PM, Michael S. Tsirkin wrote:
> >>  >>
> >>  >>   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.
> >
> >So a full 16 bit decode is done for now?
> >
> 
> Correct.

Then we probably can ignore the issue and do 16 bit decode all over
for now.

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

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 06:49:34PM +0300, Avi Kivity wrote:
> On 09/04/2011 06:46 PM, Michael S. Tsirkin wrote:
> >>
> >>  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)


I'll switch my code to that, you take care of piix.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote:
> >  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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Sun, Sep 04, 2011 at 07:22:54PM +0300, Avi Kivity wrote:
> On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote:
> >>  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.

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

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Mon, Jul 04, 2011 at 12:43:58PM +0300, Michael S. Tsirkin wrote:
> 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?

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/04/2011 08:03 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 04, 2011 at 07:22:54PM +0300, Avi Kivity wrote:
> >  On 09/04/2011 07:19 PM, Michael S. Tsirkin wrote:
> >  >>   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.
>
> The bridge has a 16 bit decode bit. I'm guessing we should
> make BIOS enable that.
>

I'm guessing you're right.

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Jul 04, 2011 at 12:43:58PM +0300, Michael S. Tsirkin wrote:
>> 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?

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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Mon, Sep 05, 2011 at 10:17:01AM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Jul 04, 2011 at 12:43:58PM +0300, Michael S. Tsirkin wrote:
> >> 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?
> 
> Looking...  qdev_device_help() shows only device properties, not bus
> properties.  I'd call that a bug.

Hmm, but is "bus" a bus property?
Care fixing all these?

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Gerd Hoffmann]

   Hi,

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

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Mon, Sep 05, 2011 at 11:53:11AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >>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.
> 
> cheers,
>   Gerd

At the moment the help text is wrong:

-device driver[,prop[=value][,...]]
                add device (based on driver)
                prop=value,... sets driver properties
                use -device ? to print all possible drivers
                use -device driver,? to print all possible properties

We also need documentation for properties and devices,
but that's a separate problem.


-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/04/2011 04:25 PM, Avi Kivity Write:
> On 09/02/2011 05:56 AM, Wen Congyang wrote:
>> >
>> >  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.
>>
>>
> 
> 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?

Thanks
Wen Congyang

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Avi Kivity]

On 09/06/2011 06:06 AM, Wen Congyang wrote:
> >  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?
>

No idea what this means.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

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

bus is indeed treated specially, but the user doesn't care.  The help
text should cover it just like any "normal" property.

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/06/2011 03:45 PM, Avi Kivity Write:
> On 09/06/2011 06:06 AM, Wen Congyang wrote:
>> >  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?
>>
> 
> 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

> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Wed, Sep 07, 2011 at 12:39:09PM +0800, Wen Congyang wrote:
> At 09/06/2011 03:45 PM, Avi Kivity Write:
> > On 09/06/2011 06:06 AM, Wen Congyang wrote:
> >> >  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?
> >>
> > 
> > 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
> 
> > 

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.

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/07/2011 07:52 PM, Michael S. Tsirkin Write:
> On Wed, Sep 07, 2011 at 12:39:09PM +0800, Wen Congyang wrote:
>> At 09/06/2011 03:45 PM, Avi Kivity Write:
>>> On 09/06/2011 06:06 AM, Wen Congyang wrote:
>>>>>  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?
>>>>
>>>
>>> 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
>>
>>>
> 
> 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.

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


> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/08/2011 02:15 PM, Wen Congyang Write:
> At 09/07/2011 07:52 PM, Michael S. Tsirkin Write:
>> On Wed, Sep 07, 2011 at 12:39:09PM +0800, Wen Congyang wrote:
>>> At 09/06/2011 03:45 PM, Avi Kivity Write:
>>>> On 09/06/2011 06:06 AM, Wen Congyang wrote:
>>>>>>  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?
>>>>>
>>>>
>>>> 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
>>>
>>>>
>>
>> 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.
> 
> 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.

Here is the seabios's code:
==========
static void pci_bios_init_device(struct pci_device *pci)
{
    u16 bdf = pci->bdf;
    int pin, pic_irq;

    dprintf(1, "PCI: bus=%d devfn=0x%02x: vendor_id=0x%04x device_id=0x%04x\n"
            , pci_bdf_to_bus(bdf), pci_bdf_to_devfn(bdf)
            , pci->vendor, pci->device);
    pci_init_device(pci_class_tbl, pci, NULL);

    /* enable memory mappings */
    pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);

    /* map the interrupt */
    pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
    if (pin != 0) {
        pin = pci_slot_get_pirq(bdf, pin - 1);
        pic_irq = pci_irqs[pin];
        pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pic_irq);
    }

    pci_init_device(pci_device_tbl, pci, NULL);
}

static void pci_bios_init_device_in_bus(int bus)
{
    struct pci_device *pci;
    foreachpci(pci) {
        u8 pci_bus = pci_bdf_to_bus(pci->bdf);
        if (pci_bus < bus)
            continue;
        if (pci_bus > bus)
            break;
        pci_bios_init_device(pci);
    }
}

...
void
pci_setup(void)
{
...
    pci_bios_map_device_in_bus(0 /* host bus */);

    pci_bios_init_device_in_bus(0 /* host bus */);
...
}
==========

The PCI_INTERRUPT_LINE register is set in the function
pci_setup() calls pci_bios_init_device_in_bus() calls pci_bios_init_device().
According to the code, it only inits the PCI device on bus 0.

I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
it to uio_pci_generic:
diff --git a/src/pciinit.c b/src/pciinit.c
index 597c8ea..c2e97d7 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start, u32 end)
 void
 pci_setup(void)
 {
+    int bus;
+
     if (CONFIG_COREBOOT || usingXen()) {
         // PCI setup already done by coreboot or Xen - just do probe.
         pci_probe();
@@ -603,9 +605,11 @@ pci_setup(void)
     dprintf(1, "=== PCI new allocation pass #2 ===\n");
     dprintf(1, "PCI: init bases bus 0 (primary)\n");
     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 */);
 
-    pci_bios_init_device_in_bus(0 /* host bus */);
+        pci_bios_init_device_in_bus(bus /* host bus */);
+    }
 
     struct pci_device *pci;
     foreachpci(pci) {


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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

   Hi,

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

> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -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 */);

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

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

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 1197 bytes --]

diff --git a/src/pciinit.c b/src/pciinit.c
index 597c8ea..676e35e 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -45,7 +45,7 @@ static struct pci_bus {
 } *busses;
 static int busses_count;
 
-static void pci_bios_init_device_in_bus(int bus);
+static void pci_bios_init_device_all(void);
 static void pci_bios_check_device_in_bus(int bus);
 static void pci_bios_init_bus_bases(struct pci_bus *bus);
 static void pci_bios_map_device_in_bus(int bus);
@@ -254,15 +254,10 @@ static void pci_bios_init_device(struct pci_device *pci)
     pci_init_device(pci_device_tbl, pci, NULL);
 }
 
-static void pci_bios_init_device_in_bus(int bus)
+static void pci_bios_init_device_all(void)
 {
     struct pci_device *pci;
     foreachpci(pci) {
-        u8 pci_bus = pci_bdf_to_bus(pci->bdf);
-        if (pci_bus < bus)
-            continue;
-        if (pci_bus > bus)
-            break;
         pci_bios_init_device(pci);
     }
 }
@@ -605,7 +600,7 @@ pci_setup(void)
     pci_bios_init_bus_bases(&busses[0]);
     pci_bios_map_device_in_bus(0 /* host bus */);
 
-    pci_bios_init_device_in_bus(0 /* host bus */);
+    pci_bios_init_device_all();
 
     struct pci_device *pci;
     foreachpci(pci) {

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
>   Hi,
> 
>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
>> set, and I can bind
>> it to uio_pci_generic:
> 
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -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 */);
> 
> 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.

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

> 
> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
> >   Hi,
> > 
> >> I modify the code like this, and the PCI_INTERRUPT_LINE register is
> >> set, and I can bind
> >> it to uio_pci_generic:
> > 
> >> --- a/src/pciinit.c
> >> +++ b/src/pciinit.c
> >> @@ -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 */);
> > 
> > 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.


> > 
> >> -    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
> 
> > 
> > cheers,
> >   Gerd

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/08/2011 06:42 PM, Michael S. Tsirkin Write:
> On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
>> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
>>>   Hi,
>>>
>>>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
>>>> set, and I can bind
>>>> it to uio_pci_generic:
>>>
>>>> --- a/src/pciinit.c
>>>> +++ b/src/pciinit.c
>>>> @@ -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 */);
>>>
>>> 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.

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

Thanks
Wen Congyang

> 
> 
>>>
>>>> -    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
>>
>>>
>>> cheers,
>>>   Gerd
> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Thu, Sep 08, 2011 at 07:03:10PM +0800, Wen Congyang wrote:
> At 09/08/2011 06:42 PM, Michael S. Tsirkin Write:
> > On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
> >> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
> >>>   Hi,
> >>>
> >>>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
> >>>> set, and I can bind
> >>>> it to uio_pci_generic:
> >>>
> >>>> --- a/src/pciinit.c
> >>>> +++ b/src/pciinit.c
> >>>> @@ -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 */);
> >>>
> >>> 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.
> 
> I use gdb to debug, and find that this function is never called.
> 
> Thanks
> Wen Congyang

No, I mean that bios must implement this logic.
You don't see it called probably because ivshmem
does not cause interrupts for you.

> > 
> > 
> >>>
> >>>> -    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
> >>
> >>>
> >>> cheers,
> >>>   Gerd
> > 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 08/19/2011 11:26 PM, Avi Kivity Write:
> On 08/18/2011 10:12 PM, Wen Congyang wrote:
>> >>
>> >>  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.
>>
>> 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.
> 
>> 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.
> 

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:
# lspci -vv
...
00:1c.1 PCI bridge: Intel Corporation N10/ICH 7 Family PCI Express Port 2 (rev 01) (prog-if 00 [Normal decode])
...
	Bus: primary=00, secondary=02, subordinate=02, sec-latency=0
	I/O behind bridge: 0000d000-0000dfff
	Memory behind bridge: fea00000-feafffff
	Prefetchable memory behind bridge: 0000000080400000-00000000805fffff
...
00:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1) (prog-if 01 [Subtractive decode])
...
	Bus: primary=00, secondary=03, subordinate=03, sec-latency=32
	I/O behind bridge: 0000e000-0000efff
	Memory behind bridge: feb00000-febfffff
	Prefetchable memory behind bridge: 0000000080600000-00000000806fffff
...
03:01.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8139/8139C/8139C+ (rev 10)
...
	Region 0: I/O ports at a800 [size=256]
	Region 1: Memory at febfbc00 (32-bit, non-prefetchable) [size=256]
	Expansion ROM at 80600000 [disabled] [size=128K]
...

# od -t x1 /sys/bus/pci/devices/0000\:03\:01.0/config
0000000 ec 10 39 81 03 00 90 82 10 00 00 02 00 00 00 00
0000020 01 a8 00 00 00 bc af fe 00 00 00 00 00 00 00 00
0000040 00 00 00 00 00 00 00 00 00 00 00 00 ec 10 39 81
0000060 00 00 bc fe 50 00 00 00 00 00 00 00 06 01 20 40
0000100 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0000120 01 00 02 76 00 00 00 00 00 00 00 00 00 00 00 00
0000140 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
*
0000400
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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Fri, Sep 09, 2011 at 02:43:24PM +0800, Wen Congyang wrote:
> > 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

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

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/09/2011 03:12 PM, Michael S. Tsirkin Write:
> On Fri, Sep 09, 2011 at 02:43:24PM +0800, Wen Congyang wrote:
>>> 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
> 
> 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

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Fri, Sep 09, 2011 at 03:24:54PM +0800, Wen Congyang wrote:
> At 09/09/2011 03:12 PM, Michael S. Tsirkin Write:
> > On Fri, Sep 09, 2011 at 02:43:24PM +0800, Wen Congyang wrote:
> >>> 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
> > 
> > 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.

> and how to know
> whether it is visible?
> 
> Thanks
> Wen Congyang

Read the BAR memory as you did.

-- 
MST

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 09/09/2011 03:34 PM, Michael S. Tsirkin Write:
> On Fri, Sep 09, 2011 at 03:24:54PM +0800, Wen Congyang wrote:
>> At 09/09/2011 03:12 PM, Michael S. Tsirkin Write:
>>> On Fri, Sep 09, 2011 at 02:43:24PM +0800, Wen Congyang wrote:
>>>>> 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
>>>
>>> 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.
> 
>> and how to know
>> whether it is visible?
>>
>> Thanks
>> Wen Congyang
> 
> Read the BAR memory as you did.


Thanks for your explain.
Wen Congyang

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:
> On Tue, Nov 01, 2011 at 09:27:25AM +0800, Wen Congyang wrote:
>> Hi, Michael S. Tsirkin
>>
>> At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:
>>> On Mon, Sep 26, 2011 at 02:18:15PM +0800, Wen Congyang wrote:
>>>> Hi, Michael S. Tsirkin 
>>>>
>>>> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
>>>>> 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.
>>>>
>>>> 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 
>>
>>>
>>>> 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.
>>
>> 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?

No problem.
I have cced qemu mailing list.

Thanks
Wen Congyang

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Michael S. Tsirkin]

On Tue, Nov 01, 2011 at 04:49:08PM +0800, Wen Congyang wrote:
> At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:
> > On Tue, Nov 01, 2011 at 09:27:25AM +0800, Wen Congyang wrote:
> >> Hi, Michael S. Tsirkin
> >>
> >> At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:
> >>> On Mon, Sep 26, 2011 at 02:18:15PM +0800, Wen Congyang wrote:
> >>>> Hi, Michael S. Tsirkin 
> >>>>
> >>>> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
> >>>>> 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.
> >>>>
> >>>> 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.

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

Yes, but ACPI can drive SHPC.

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

> >> Thanks
> >> Wen Congyang
> > 
> > Can we discuss these questions on the mailing list?
> 
> No problem.
> I have cced qemu mailing list.
> 
> Thanks
> Wen Congyang

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 11/01/2011 07:48 PM, Michael S. Tsirkin Write:
> On Tue, Nov 01, 2011 at 04:49:08PM +0800, Wen Congyang wrote:
>> At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:
>>> On Tue, Nov 01, 2011 at 09:27:25AM +0800, Wen Congyang wrote:
>>>> Hi, Michael S. Tsirkin
>>>>
>>>> At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:
>>>>> On Mon, Sep 26, 2011 at 02:18:15PM +0800, Wen Congyang wrote:
>>>>>> Hi, Michael S. Tsirkin 
>>>>>>
>>>>>> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
>>>>>>> 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.
>>>>>>
>>>>>> 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.

We can load ACPI SSDT dynamically. But I do not know whether guest OS
supports it.

> 
>>>>>
>>>>>> 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.
> 
> Yes, but ACPI can drive SHPC.

Yes. But if we implement SHPC, we should also to implement ACPI.

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

If we support hotplug behind PCI-to-PCI bridge, there are more than 32 slots that can
be hotpluggable. How do we know which device is remove or inserted? The I/O region 0xae00
(length: 8bytes) only supports 32 slots.

Thanks
Wen Congyang

> 
>>>> Thanks
>>>> Wen Congyang
>>>
>>> Can we discuss these questions on the mailing list?
>>
>> No problem.
>> I have cced qemu mailing list.
>>
>> Thanks
>> Wen Congyang
> 

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Isaku Yamahata]

On Tue, Nov 01, 2011 at 04:49:08PM +0800, Wen Congyang wrote:
> At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:
> > On Tue, Nov 01, 2011 at 09:27:25AM +0800, Wen Congyang wrote:
> >> Hi, Michael S. Tsirkin
> >>
> >> At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:
> >>> On Mon, Sep 26, 2011 at 02:18:15PM +0800, Wen Congyang wrote:
> >>>> Hi, Michael S. Tsirkin 
> >>>>
> >>>> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
> >>>>> 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.
> >>>>
> >>>> 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.


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

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


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

> >> Thanks
> >> Wen Congyang
> > 
> > Can we discuss these questions on the mailing list?
> 
> No problem.
> I have cced qemu mailing list.
> 
> Thanks
> Wen Congyang
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH] pci: add standard bridge device

[Wen Congyang]

At 11/02/2011 10:15 AM, Isaku Yamahata Write:
> On Tue, Nov 01, 2011 at 04:49:08PM +0800, Wen Congyang wrote:
>> At 11/01/2011 04:44 PM, Michael S. Tsirkin Write:
>>> On Tue, Nov 01, 2011 at 09:27:25AM +0800, Wen Congyang wrote:
>>>> Hi, Michael S. Tsirkin
>>>>
>>>> At 09/26/2011 03:08 PM, Michael S. Tsirkin Write:
>>>>> On Mon, Sep 26, 2011 at 02:18:15PM +0800, Wen Congyang wrote:
>>>>>> Hi, Michael S. Tsirkin 
>>>>>>
>>>>>> At 07/04/2011 05:43 PM, Michael S. Tsirkin Write:
>>>>>>> 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.
>>>>>>
>>>>>> 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.
> 
> 
>>>>>> 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.
> 
> Out of curiosity, what OS do you have in mind?

Windows. IIRC, Windows supports SHPC after vista.

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

Do you mean I/O region 0xae00, 0xae08 is not standard. It can work only with
qemu.

Thanks
Wen Congyang

> 
> thanks,
> 
>>>> Thanks
>>>> Wen Congyang
>>>
>>> Can we discuss these questions on the mailing list?
>>
>> No problem.
>> I have cced qemu mailing list.
>>
>> Thanks
>> Wen Congyang
>>
>