[Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Marcel Apfelbaum]

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

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);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Marcel Apfelbaum]

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

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Cao jin]

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?

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Marcel Apfelbaum]

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

>

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Cao jin]

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?

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Marcel Apfelbaum]

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

>

Re: [Qemu-devel] [PATCH V4] hw/pxb: add chassis_nr property

[Cao jin]

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:)
-- 
Yours Sincerely,

Cao jin