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

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]

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/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 
Signed-off-by: Marcel Apfelbaum 
---
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/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 
Signed-off-by: Marcel Apfelbaum 
---
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/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 
Signed-off-by: Marcel Apfelbaum 
---
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);
  }