Re: [Qemu-devel] [PATCH v5 4/4] docs: update documentation considering PCIE-PCI bridge

2017-08-13 Thread Marcel Apfelbaum 

On 11/08/2017 2:31, Aleksandr Bezzubikov wrote:

Signed-off-by: Aleksandr Bezzubikov 
---
  docs/pcie.txt|  49 ++--
  docs/pcie_pci_bridge.txt | 115 +++
  2 files changed, 141 insertions(+), 23 deletions(-)
  create mode 100644 docs/pcie_pci_bridge.txt

diff --git a/docs/pcie.txt b/docs/pcie.txt
index 5bada24..76b85ec 100644
--- a/docs/pcie.txt
+++ b/docs/pcie.txt
@@ -46,7 +46,7 @@ Place only the following kinds of devices directly on the 
Root Complex:
  (2) PCI Express Root Ports (ioh3420), for starting exclusively PCI Express
  hierarchies.
  
-(3) DMI-PCI Bridges (i82801b11-bridge), for starting legacy PCI

+(3) PCI Express to PCI Bridge (pcie-pci-bridge), for starting legacy PCI
  hierarchies.
  
  (4) Extra Root Complexes (pxb-pcie), if multiple PCI Express Root Buses

@@ -55,18 +55,18 @@ Place only the following kinds of devices directly on the 
Root Complex:
 pcie.0 bus
 

  |||  |
-   ---   --   --   --
-   | PCI Dev |   | PCIe Root Port |   | DMI-PCI Bridge |   |  pxb-pcie  |
-   ---   --   --   --
+   ---   --   ---   --
+   | PCI Dev |   | PCIe Root Port |   | PCIe-PCI Bridge |   |  pxb-pcie  |
+   ---   --   ---   --
  
  2.1.1 To plug a device into pcie.0 as a Root Complex Integrated Endpoint use:

-device [,bus=pcie.0]
  2.1.2 To expose a new PCI Express Root Bus use:
-device pxb-pcie,id=pcie.1,bus_nr=x[,numa_node=y][,addr=z]
-  Only PCI Express Root Ports and DMI-PCI bridges can be connected
-  to the pcie.1 bus:
+  PCI Express Root Ports and PCI Express to PCI bridges can be
+  connected to the pcie.1 bus:
-device 
ioh3420,id=root_port1[,bus=pcie.1][,chassis=x][,slot=y][,addr=z]
 \
-  -device i82801b11-bridge,id=dmi_pci_bridge1,bus=pcie.1
+  -device pcie-pci-bridge,id=pcie_pci_bridge1,bus=pcie.1
  
  
  2.2 PCI Express only hierarchy

@@ -130,24 +130,24 @@ Notes:
  Legacy PCI devices can be plugged into pcie.0 as Integrated Endpoints,
  but, as mentioned in section 5, doing so means the legacy PCI
  device in question will be incapable of hot-unplugging.
-Besides that use DMI-PCI Bridges (i82801b11-bridge) in combination
-with PCI-PCI Bridges (pci-bridge) to start PCI hierarchies.
+Besides that use PCI Express to PCI Bridges (pcie-pci-bridge) in
+combination with PCI-PCI Bridges (pci-bridge) to start PCI hierarchies.
  
-Prefer flat hierarchies. For most scenarios a single DMI-PCI Bridge

+Prefer flat hierarchies. For most scenarios a single PCI Express to PCI Bridge
  (having 32 slots) and several PCI-PCI Bridges attached to it
  (each supporting also 32 slots) will support hundreds of legacy devices.
-The recommendation is to populate one PCI-PCI Bridge under the DMI-PCI Bridge
-until is full and then plug a new PCI-PCI Bridge...
+The recommendation is to populate one PCI-PCI Bridge under the
+PCI Express to PCI Bridge until is full and then plug a new PCI-PCI Bridge...
  
 pcie.0 bus

 --
  ||
-   ---   --
-   | PCI Dev |   | DMI-PCI BRIDGE |
-   ----
+   ---   ---
+   | PCI Dev |   | PCIe-PCI Bridge |
+   ---   ---
 ||
----
-  | PCI-PCI Bridge || PCI-PCI Bridge |   ...
+  | PCI-PCI Bridge || PCI-PCI Bridge |
----
   |   |
--- ---
@@ -157,11 +157,11 @@ until is full and then plug a new PCI-PCI Bridge...
  2.3.1 To plug a PCI device into pcie.0 as an Integrated Endpoint use:
-device [,bus=pcie.0]
  2.3.2 Plugging a PCI device into a PCI-PCI Bridge:
-  -device i82801b11-bridge,id=dmi_pci_bridge1[,bus=pcie.0] 
   \
-  -device 
pci-bridge,id=pci_bridge1,bus=dmi_pci_bridge1[,chassis_nr=x][,addr=y]   \
+  -device pcie-pci-bridge,id=pcie_pci_bridge1[,bus=pcie.0] \
+  -device 
pci-bridge,id=pci_bridge1,bus=pcie_pci_bridge1[,chassis_nr=x][,addr=y] \
-device ,bus=pci_bridge1[,addr=x]
Note that 'addr' cannot be 0 unless shpc=off parameter is passed to
-  the PCI Bridge.
+  the PCI Bridge/PCI Express to PCI Bridge.
  
  3. IO space

Re: [Qemu-devel] [PATCH v5 4/4] docs: update documentation considering PCIE-PCI bridge

2017-08-11 Thread Laszlo Ersek 
On 08/11/17 01:31, Aleksandr Bezzubikov wrote:

> +PCIE-PCI bridge hot-plug
> +===
> +Guest OSes require extra efforts to enable PCIE-PCI bridge hot-plug.
> +Motivation - now on init any PCI Express root port which doesn't have
> +any device plugged in, has no free buses reserved to provide any of them
> +to a hot-plugged devices in future.
> +
> +To solve this problem we reserve additional buses on a firmware level.
> +Currently only SeaBIOS is supported.
> +The way of bus number to reserve delivery is special
> +Red Hat vendor-specific PCI capability, added to the root port
> +that is planned to have PCIE-PCI bridge hot-plugged in.
> +
> +Capability layout (defined in include/hw/pci/pci_bridge.h):
> +
> +uint8_t id; Standard PCI capability header field
> +uint8_t next;   Standard PCI capability header field
> +uint8_t len;Standard PCI vendor-specific capability header field
> +
> +uint8_t type;   Red Hat vendor-specific capability type
> +List of currently existing types:
> +RESOURCE_RESERVE = 1
> +
> +
> +uint32_t bus_res;   Minimum number of buses to reserve
> +
> +uint64_t io;   IO space to reserve
> +uint32_t mem   Non-prefetchable memory to reserve
> +
> +This two fields are mutually exclusive:

[*] mark this

> +uint32_t mem_pref_32;  Prefetchable memory to reserve (32-bit MMIO)
> +uint64_t mem_pref_64;  Prefetchable memory to reserve (64-bit MMIO)
> +
> +If any reservation field is -1 then this kind of reservation is not
> +needed and must be ignored by firmware.
> +
> +mem_pref_* fields mutual exclusiveness means they cannot be -1 both.

Please drop the last sentence; it is perfectly possible that a bridge
doesn't need either 32-bit or 64-bit prefetchable MMIO reservation.
"Mutually exclusive" usually means "at most one", not "exactly one".
(E.g., think of the "mutex" construct -- in the critical section being
protected by the mutex, there can be Thread 1, Thread 2, or none of them.)

So, beyond dropping the last sentence, I suggest to replace the one
marked with [*] with the following, for clarity:

At most one of the following two fields may be set to a value
different from -1:

With this update, for this patch:

Reviewed-by: Laszlo Ersek 

Thanks!
Laszlo