Message ID | 1457014732-19428-1-git-send-email-marcel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 03/03/2016 04:18 PM, Marcel Apfelbaum wrote: > Add a chassis_nr property instead of using PXB bus number > as internal bridge's chassis nr. ping Thanks, Marcel > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > v3->v4: > - re-coded to fit current codebase > v2->v3: > - use bus nr if chassis nr is 0 (Micahel S. Tsirkin) > v1->v2: > - Rebased on master > > docs/pci_expander_bridge.txt | 7 +++---- > hw/pci-bridge/pci_expander_bridge.c | 21 +++++++++++++++++++-- > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/docs/pci_expander_bridge.txt b/docs/pci_expander_bridge.txt > index e7c8fe9..fe058a6 100644 > --- a/docs/pci_expander_bridge.txt > +++ b/docs/pci_expander_bridge.txt > @@ -23,9 +23,9 @@ A detailed command line would be: > -m 2G > -object memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0 -numa node,nodeid=0,cpus=0,memdev=ram-node0 > -object memory-backend-ram,size=1024M,policy=bind,host-nodes=1,id=ram-node1 -numa node,nodeid=1,cpus=1,memdev=ram-node1 > --device pxb,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 -netdev user,id=nd -device e1000,bus=bridge1,addr=0x4,netdev=nd > --device pxb,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8, -device e1000,bus=bridge2,addr=0x3 > --device pxb,id=bridge3,bus=pci.0,bus_nr=40, -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 > +-device pxb,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4,chassis_nr=1 -netdev user,id=nd -device e1000,bus=bridge1,addr=0x4,netdev=nd > +-device pxb,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8,chassis_nr=2 -device e1000,bus=bridge2,addr=0x3 > +-device pxb,id=bridge3,bus=pci.0,bus_nr=40,chassis_nr=3 -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 > > Here you have: > - 2 NUMA nodes for the guest, 0 and 1. (both mapped to the same NUMA node in host, but you can and should put it in different host NUMA nodes) > @@ -55,4 +55,3 @@ The PXB is composed by: > - Using the bridge will enable hotplug support > - All the devices behind the bridge will use bridge's IO/MEM windows compacting > the PCI address space. > - > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > index d23b8da..6c4873a 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -46,6 +46,7 @@ typedef struct PXBDev { > PCIDevice parent_obj; > /*< public >*/ > > + uint8_t chassis_nr; > uint8_t bus_nr; > uint16_t numa_node; > } PXBDev; > @@ -237,7 +238,8 @@ static int pxb_dev_init_common(PCIDevice *dev, bool pcie) > bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); > bds = qdev_create(BUS(bus), "pci-bridge"); > bds->id = dev_name; > - qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr); > + qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, > + pxb->chassis_nr); > qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false); > } > > @@ -267,11 +269,17 @@ static int pxb_dev_init_common(PCIDevice *dev, bool pcie) > > static int pxb_dev_initfn(PCIDevice *dev) > { > + PXBDev *pxb = convert_to_pxb(dev); > + > if (pci_bus_is_express(dev->bus)) { > error_report("pxb devices cannot reside on a PCIe bus!"); > return -EINVAL; > } > > + if (!pxb->chassis_nr) { > + pxb->chassis_nr = pxb->bus_nr; > + } > + > return pxb_dev_init_common(dev, false); > } > > @@ -286,6 +294,8 @@ static Property pxb_dev_properties[] = { > /* Note: 0 is not a legal a PXB bus number. */ > DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + /* Note: 0 is not a legal chassis number. */ > + DEFINE_PROP_UINT8("chassis_nr", PXBDev, chassis_nr, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -322,6 +332,13 @@ static int pxb_pcie_dev_initfn(PCIDevice *dev) > return pxb_dev_init_common(dev, true); > } > > +static Property pxb_pcie_dev_properties[] = { > + /* Note: 0 is not a legal a PXB bus number. */ > + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -334,7 +351,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_BRIDGE_HOST; > > dc->desc = "PCI Express Expander Bridge"; > - dc->props = pxb_dev_properties; > + dc->props = pxb_pcie_dev_properties; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > } > >
Hi, On 03/03/2016 10:18 PM, Marcel Apfelbaum wrote: > Add a chassis_nr property instead of using PXB bus number > as internal bridge's chassis nr. > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > v3->v4: > - re-coded to fit current codebase > v2->v3: > - use bus nr if chassis nr is 0 (Micahel S. Tsirkin) > v1->v2: > - Rebased on master > > docs/pci_expander_bridge.txt | 7 +++---- > hw/pci-bridge/pci_expander_bridge.c | 21 +++++++++++++++++++-- > 2 files changed, 22 insertions(+), 6 deletions(-) > > > @@ -286,6 +294,8 @@ static Property pxb_dev_properties[] = { > /* Note: 0 is not a legal a PXB bus number. */ better remove the latter "a", or I guess it will conflict with my previous pxb cleanup patch. > DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + /* Note: 0 is not a legal chassis number. */ > + DEFINE_PROP_UINT8("chassis_nr", PXBDev, chassis_nr, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -322,6 +332,13 @@ static int pxb_pcie_dev_initfn(PCIDevice *dev) > return pxb_dev_init_common(dev, true); > } > > +static Property pxb_pcie_dev_properties[] = { > + /* Note: 0 is not a legal a PXB bus number. */ likewise > + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + DEFINE_PROP_END_OF_LIST(), > +}; And I have another personal question: In qemu design, it seems every pci bridge reside in a separate chassis, what`s benefit? why don`t put them all in the main chassis?
On 03/15/2016 10:00 AM, Cao jin wrote: > Hi, > > On 03/03/2016 10:18 PM, Marcel Apfelbaum wrote: >> Add a chassis_nr property instead of using PXB bus number >> as internal bridge's chassis nr. >> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> v3->v4: >> - re-coded to fit current codebase >> v2->v3: >> - use bus nr if chassis nr is 0 (Micahel S. Tsirkin) >> v1->v2: >> - Rebased on master >> >> docs/pci_expander_bridge.txt | 7 +++---- >> hw/pci-bridge/pci_expander_bridge.c | 21 +++++++++++++++++++-- >> 2 files changed, 22 insertions(+), 6 deletions(-) >> > >> >> @@ -286,6 +294,8 @@ static Property pxb_dev_properties[] = { >> /* Note: 0 is not a legal a PXB bus number. */ > > better remove the latter "a", or I guess it will conflict with my previous pxb cleanup patch. sure, thanks for bringing it up. > >> DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), >> DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), >> + /* Note: 0 is not a legal chassis number. */ >> + DEFINE_PROP_UINT8("chassis_nr", PXBDev, chassis_nr, 0), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -322,6 +332,13 @@ static int pxb_pcie_dev_initfn(PCIDevice *dev) >> return pxb_dev_init_common(dev, true); >> } >> >> +static Property pxb_pcie_dev_properties[] = { >> + /* Note: 0 is not a legal a PXB bus number. */ > > likewise ok > >> + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), >> + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), >> + DEFINE_PROP_END_OF_LIST(), >> +}; > > And I have another personal question: In qemu design, it seems every pci bridge reside in a separate chassis, what`s benefit? why don`t put them all in the main chassis? Please have a look on pci-to-pci bridge specification, chapter 13, slot numbering. Thanks for the review, Marcel >
hi On 03/15/2016 07:44 PM, Marcel Apfelbaum wrote: > On 03/15/2016 10:00 AM, Cao jin wrote: >> >> And I have another personal question: In qemu design, it seems every >> pci bridge reside in a separate chassis, what`s benefit? why don`t >> put them all in the main chassis? > > Please have a look on pci-to-pci bridge specification, chapter 13, slot > numbering. > thanks for the hint. I still have a question: in docs/pci_expander_bridge.txt, it says: create a TYPE_PCI_BRIDGE_DEV to enable hotplug support. But I didn`t see it can hotplug as following step: 1: ./qemu-system-x86_64 -device pxb,id=br,bus=pci.0,bus_nr=2 -hda linux.img -smp 2 --enable-kvm -m 1024 -monitor stdio 2: in monitor, type: device_add e1000,bus=br result: no message output to monitor, and don`t see e1000 nic in guest Is is a bug or I test it in a wrong way?
On 03/17/2016 12:02 PM, Cao jin wrote: > hi > > On 03/15/2016 07:44 PM, Marcel Apfelbaum wrote: >> On 03/15/2016 10:00 AM, Cao jin wrote: > >>> >>> And I have another personal question: In qemu design, it seems every >>> pci bridge reside in a separate chassis, what`s benefit? why don`t >>> put them all in the main chassis? >> >> Please have a look on pci-to-pci bridge specification, chapter 13, slot >> numbering. >> > > thanks for the hint. > I still have a question: in docs/pci_expander_bridge.txt, it says: create a TYPE_PCI_BRIDGE_DEV to enable hotplug support. > But I didn`t see it can hotplug as following step: > > 1: ./qemu-system-x86_64 -device pxb,id=br,bus=pci.0,bus_nr=2 -hda linux.img -smp 2 --enable-kvm -m 1024 -monitor stdio > > 2: in monitor, type: device_add e1000,bus=br > result: no message output to monitor, and don`t see e1000 nic in guest > > Is is a bug or I test it in a wrong way? Hi, You used hot-plug correctly, it doesn't work yet since I am still working on this feature. The bridge gives us the basics environment for hot-plug, what is missing is ACPI "bsel" mechanism. Thanks, Marcel >
On 03/24/2016 11:34 PM, Marcel Apfelbaum wrote: > > Hi, > > You used hot-plug correctly, it doesn't work yet since I am still > working on this feature. > The bridge gives us the basics environment for hot-plug, what is missing > is ACPI "bsel" mechanism. > > Thanks, > Marcel I see, Thanks for your information:)
diff --git a/docs/pci_expander_bridge.txt b/docs/pci_expander_bridge.txt index e7c8fe9..fe058a6 100644 --- a/docs/pci_expander_bridge.txt +++ b/docs/pci_expander_bridge.txt @@ -23,9 +23,9 @@ A detailed command line would be: -m 2G -object memory-backend-ram,size=1024M,policy=bind,host-nodes=0,id=ram-node0 -numa node,nodeid=0,cpus=0,memdev=ram-node0 -object memory-backend-ram,size=1024M,policy=bind,host-nodes=1,id=ram-node1 -numa node,nodeid=1,cpus=1,memdev=ram-node1 --device pxb,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4 -netdev user,id=nd -device e1000,bus=bridge1,addr=0x4,netdev=nd --device pxb,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8, -device e1000,bus=bridge2,addr=0x3 --device pxb,id=bridge3,bus=pci.0,bus_nr=40, -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 +-device pxb,id=bridge1,bus=pci.0,numa_node=1,bus_nr=4,chassis_nr=1 -netdev user,id=nd -device e1000,bus=bridge1,addr=0x4,netdev=nd +-device pxb,id=bridge2,bus=pci.0,numa_node=0,bus_nr=8,chassis_nr=2 -device e1000,bus=bridge2,addr=0x3 +-device pxb,id=bridge3,bus=pci.0,bus_nr=40,chassis_nr=3 -drive if=none,id=drive0,file=[img] -device virtio-blk-pci,drive=drive0,scsi=off,bus=bridge3,addr=1 Here you have: - 2 NUMA nodes for the guest, 0 and 1. (both mapped to the same NUMA node in host, but you can and should put it in different host NUMA nodes) @@ -55,4 +55,3 @@ The PXB is composed by: - Using the bridge will enable hotplug support - All the devices behind the bridge will use bridge's IO/MEM windows compacting the PCI address space. - diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index d23b8da..6c4873a 100644 --- a/hw/pci-bridge/pci_expander_bridge.c +++ b/hw/pci-bridge/pci_expander_bridge.c @@ -46,6 +46,7 @@ typedef struct PXBDev { PCIDevice parent_obj; /*< public >*/ + uint8_t chassis_nr; uint8_t bus_nr; uint16_t numa_node; } PXBDev; @@ -237,7 +238,8 @@ static int pxb_dev_init_common(PCIDevice *dev, bool pcie) bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); bds = qdev_create(BUS(bus), "pci-bridge"); bds->id = dev_name; - qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr); + qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, + pxb->chassis_nr); qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false); } @@ -267,11 +269,17 @@ static int pxb_dev_init_common(PCIDevice *dev, bool pcie) static int pxb_dev_initfn(PCIDevice *dev) { + PXBDev *pxb = convert_to_pxb(dev); + if (pci_bus_is_express(dev->bus)) { error_report("pxb devices cannot reside on a PCIe bus!"); return -EINVAL; } + if (!pxb->chassis_nr) { + pxb->chassis_nr = pxb->bus_nr; + } + return pxb_dev_init_common(dev, false); } @@ -286,6 +294,8 @@ static Property pxb_dev_properties[] = { /* Note: 0 is not a legal a PXB bus number. */ DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), + /* Note: 0 is not a legal chassis number. */ + DEFINE_PROP_UINT8("chassis_nr", PXBDev, chassis_nr, 0), DEFINE_PROP_END_OF_LIST(), }; @@ -322,6 +332,13 @@ static int pxb_pcie_dev_initfn(PCIDevice *dev) return pxb_dev_init_common(dev, true); } +static Property pxb_pcie_dev_properties[] = { + /* Note: 0 is not a legal a PXB bus number. */ + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), + DEFINE_PROP_END_OF_LIST(), +}; + static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -334,7 +351,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_BRIDGE_HOST; dc->desc = "PCI Express Expander Bridge"; - dc->props = pxb_dev_properties; + dc->props = pxb_pcie_dev_properties; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); }
Add a chassis_nr property instead of using PXB bus number as internal bridge's chassis nr. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- v3->v4: - re-coded to fit current codebase v2->v3: - use bus nr if chassis nr is 0 (Micahel S. Tsirkin) v1->v2: - Rebased on master docs/pci_expander_bridge.txt | 7 +++---- hw/pci-bridge/pci_expander_bridge.c | 21 +++++++++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-)