[Qemu-devel] [PATCH] hw/pci: reserve IO and mem for pci express downstream ports with no devices attached

[Marcel Apfelbaum]

commit c6e298e1f12e0f4ca02b6da5e42919ae055f6830
    hw/pci: reserve IO and mem for pci-2-pci bridges with no devices attached

introduced support for hot-plugging devices behind pci-2-pci bridges.
Extend hotplug support also for pci express downstream ports.

Signed-off-by: Marcel Apfelbaum 
---
 src/fw/pciinit.c  | 23 +++++++++++++++++++++--
 src/hw/pci_regs.h |  2 ++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index 0ad548f..edb3fe9 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -636,6 +636,25 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
     return entry;
 }

+static int pci_bus_hotplug_support(struct pci_bus *bus)
+{
+    u8 shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC);
+    u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP);
+    int downstream_port = 0;
+    int slot_implemented = 0;
+
+    if (pcie_cap) {
+        u16 pcie_flags = pci_config_readw(bus->bus_dev->bdf,
+                                          pcie_cap + PCI_EXP_FLAGS);
+        u16 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
+                        PCI_EXP_FLAGS_TYPE_SHIFT) & 0xf;
+        downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
+                          (port_type == PCI_EXP_TYPE_ROOT_PORT);
+        slot_implemented = !!(pcie_flags & PCI_EXP_FLAGS_SLOT);
+    }
+    return shpc_cap || (downstream_port && slot_implemented);
+}
+
 static int pci_bios_check_devices(struct pci_bus *busses)
 {
     dprintf(1, "PCI: check devices\n");
@@ -678,7 +697,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
             continue;
         struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
         int type;
-        u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);
+        int hotplug_support = pci_bus_hotplug_support(s);
         for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
             u64 align = (type == PCI_REGION_TYPE_IO) ?
                 PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
@@ -687,7 +706,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
             if (pci_region_align(&s->r[type]) > align)
                  align = pci_region_align(&s->r[type]);
             u64 sum = pci_region_sum(&s->r[type]);
-            if (!sum && shpc_cap)
+            if (!sum && hotplug_support)
                 sum = align; /* reserve min size for hot-plug */
             u64 size = ALIGN(sum, align);
             int is64 = pci_bios_bridge_region_is64(&s->r[type],
diff --git a/src/hw/pci_regs.h b/src/hw/pci_regs.h
index e5effd4..6a71569 100644
--- a/src/hw/pci_regs.h
+++ b/src/hw/pci_regs.h
@@ -426,6 +426,8 @@
 #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
 #define  PCI_EXP_DEVCTL2_ARI	0x20	/* Alternative Routing-ID */

+#define PCI_EXP_FLAGS_TYPE_SHIFT 4
+
 /* Extended Capabilities (PCI-X 2.0 and Express) */
 #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
 #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)

[Qemu-devel] [PATCH] hw/pci: reserve IO and mem for pci express downstream ports with no devices attached

[Michael S. Tsirkin]

On Thu, Jun 19, 2014 at 04:52:17PM +0300, Marcel Apfelbaum wrote:
> commit c6e298e1f12e0f4ca02b6da5e42919ae055f6830
>     hw/pci: reserve IO and mem for pci-2-pci bridges with no devices attached
>
> introduced support for hot-plugging devices behind pci-2-pci bridges.
> Extend hotplug support also for pci express downstream ports.
> Signed-off-by: Marcel Apfelbaum
>
> src/fw/pciinit.c  | 23 +++++++++++++++++++++--
>  src/hw/pci_regs.h |  2 ++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 0ad548f..edb3fe9 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -636,6 +636,25 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
>      return entry;
>  }
>
> +static int pci_bus_hotplug_support(struct pci_bus *bus)
> +{
> u8 shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC);
> u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP);
> int downstream_port = 0;
> int slot_implemented = 0;
> if (pcie_cap) {
>    u16 pcie_flags = pci_config_readw(bus->bus_dev->bdf,
>                                      pcie_cap + PCI_EXP_FLAGS);
>    u16 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
>                    PCI_EXP_FLAGS_TYPE_SHIFT) & 0xf;

0xf is not needed here: PCI_EXP_FLAGS_TYPE is enough.
Also should be u8.
>    downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
>                      (port_type == PCI_EXP_TYPE_ROOT_PORT);

This is correct, the spec says:
    Configuration Space, and other capabilities.
    8 Slot Implemented – When Set, this bit indicates that the Link
    HwInit associated with this Port is connected to a slot (as compared to
    being connected to a system-integrated device or being
    disabled).
    This bit is valid for Downstream Ports. This bit is undefined for
    Upstream Ports.

Maybe add a comment in case people will wonder.
>    slot_implemented = !!(pcie_flags & PCI_EXP_FLAGS_SLOT);

I would do
        return downstream_port && slot_implemented;
here, avoid testing shpc for express devices.

Also allows moving downstream_port && slot_implemented here.
> }
> return shpc_cap || (downstream_port && slot_implemented);
>
> +}
>
> static int pci_bios_check_devices(struct pci_bus *busses)
>  {
>      dprintf(1, "PCI: check devices\n");
> @@ -678,7 +697,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>              continue;
>          struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
>          int type;
>    u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);
>    int hotplug_support = pci_bus_hotplug_support(s);
>    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
>        u64 align = (type == PCI_REGION_TYPE_IO) ?
>            PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
>
> @@ -687,7 +706,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
>              if (pci_region_align(&s->r[type]) > align)
>                   align = pci_region_align(&s->r[type]);
>              u64 sum = pci_region_sum(&s->r[type]);
>        if (!sum && shpc_cap)
>        if (!sum && hotplug_support)
>            sum = align; /* reserve min size for hot-plug */
>        u64 size = ALIGN(sum, align);
>        int is64 = pci_bios_bridge_region_is64(&s->r[type],
>
> diff --git a/src/hw/pci_regs.h b/src/hw/pci_regs.h
> index e5effd4..6a71569 100644
> --- a/src/hw/pci_regs.h
> +++ b/src/hw/pci_regs.h
> @@ -426,6 +426,8 @@
>  #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
>  #define  PCI_EXP_DEVCTL2_ARI	0x20	/* Alternative Routing-ID */
>
> +#define PCI_EXP_FLAGS_TYPE_SHIFT 4

It's generally not a good idea to randomly add macros
in random places.
In this case this is not needed. Just call __ffs on
PCI_EXP_FLAGS_TYPE.
> /* Extended Capabilities (PCI-X 2.0 and Express) */
>  #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
>
> 1.8.3.1

[Qemu-devel] [PATCH] hw/pci: reserve IO and mem for pci express downstream ports with no devices attached

[Marcel Apfelbaum]

On Thu, 2014-06-19 at 17:21 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 19, 2014 at 04:52:17PM +0300, Marcel Apfelbaum wrote:
> > commit c6e298e1f12e0f4ca02b6da5e42919ae055f6830
> >     hw/pci: reserve IO and mem for pci-2-pci bridges with no devices attached
> >
> > introduced support for hot-plugging devices behind pci-2-pci bridges.
> > Extend hotplug support also for pci express downstream ports.
> > Signed-off-by: Marcel Apfelbaum
> >
> > src/fw/pciinit.c  | 23 +++++++++++++++++++++--
> >  src/hw/pci_regs.h |  2 ++
> >  2 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > index 0ad548f..edb3fe9 100644
> > --- a/src/fw/pciinit.c
> > +++ b/src/fw/pciinit.c
> > @@ -636,6 +636,25 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> >      return entry;
> >  }
> >
> > +static int pci_bus_hotplug_support(struct pci_bus *bus)
> > +{
> > u8 shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC);
> > u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP);
> > int downstream_port = 0;
> > int slot_implemented = 0;
> > if (pcie_cap) {
> >    u16 pcie_flags = pci_config_readw(bus->bus_dev->bdf,
> >                                      pcie_cap + PCI_EXP_FLAGS);
> >    u16 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
> >                    PCI_EXP_FLAGS_TYPE_SHIFT) & 0xf;
>
> 0xf is not needed here: PCI_EXP_FLAGS_TYPE is enough.
> Also should be u8.

OK, thanks
> >    downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
> >                      (port_type == PCI_EXP_TYPE_ROOT_PORT);
>
> This is correct, the spec says:
>     Configuration Space, and other capabilities.
>     8 Slot Implemented – When Set, this bit indicates that the Link
>     HwInit associated with this Port is connected to a slot (as compared to
>     being connected to a system-integrated device or being
>     disabled).
>     This bit is valid for Downstream Ports. This bit is undefined for
>     Upstream Ports.
>
> Maybe add a comment in case people will wonder.

Sure
> >    slot_implemented = !!(pcie_flags & PCI_EXP_FLAGS_SLOT);
>
> I would do
>         return downstream_port && slot_implemented;
> here, avoid testing shpc for express devices.
>
> Also allows moving downstream_port && slot_implemented here.

Makes sense
> > }
> > return shpc_cap || (downstream_port && slot_implemented);
> >
> > +}
> >
> > static int pci_bios_check_devices(struct pci_bus *busses)
> >  {
> >      dprintf(1, "PCI: check devices\n");
> > @@ -678,7 +697,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> >              continue;
> >          struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
> >          int type;
> >    u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);
> >    int hotplug_support = pci_bus_hotplug_support(s);
> >    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> >        u64 align = (type == PCI_REGION_TYPE_IO) ?
> >            PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> >
> > @@ -687,7 +706,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> >              if (pci_region_align(&s->r[type]) > align)
> >                   align = pci_region_align(&s->r[type]);
> >              u64 sum = pci_region_sum(&s->r[type]);
> >        if (!sum && shpc_cap)
> >        if (!sum && hotplug_support)
> >            sum = align; /* reserve min size for hot-plug */
> >        u64 size = ALIGN(sum, align);
> >        int is64 = pci_bios_bridge_region_is64(&s->r[type],
> >
> > diff --git a/src/hw/pci_regs.h b/src/hw/pci_regs.h
> > index e5effd4..6a71569 100644
> > --- a/src/hw/pci_regs.h
> > +++ b/src/hw/pci_regs.h
> > @@ -426,6 +426,8 @@
> >  #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
> >  #define  PCI_EXP_DEVCTL2_ARI	0x20	/* Alternative Routing-ID */
> >
> > +#define PCI_EXP_FLAGS_TYPE_SHIFT 4
>
> It's generally not a good idea to randomly add macros
> in random places.
> In this case this is not needed. Just call __ffs on
> PCI_EXP_FLAGS_TYPE.

I didn't see any usage of __ffs method on seabios code, so
I wondered if adding an include to string.h would be ok.
Would it?

Thanks,
Marcel
> > /* Extended Capabilities (PCI-X 2.0 and Express) */
> >  #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> > #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
> >
> > 1.8.3.1

[Qemu-devel] [PATCH] hw/pci: reserve IO and mem for pci express downstream ports with no devices attached

[Michael S. Tsirkin]

On Thu, Jun 19, 2014 at 05:31:26PM +0300, Marcel Apfelbaum wrote:
> On Thu, 2014-06-19 at 17:21 +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 19, 2014 at 04:52:17PM +0300, Marcel Apfelbaum wrote:
> > > commit c6e298e1f12e0f4ca02b6da5e42919ae055f6830
> > >     hw/pci: reserve IO and mem for pci-2-pci bridges with no devices attached
> > >
> > > introduced support for hot-plugging devices behind pci-2-pci bridges.
> > > Extend hotplug support also for pci express downstream ports.
> > > Signed-off-by: Marcel Apfelbaum
> > >
> > > src/fw/pciinit.c  | 23 +++++++++++++++++++++--
> > >  src/hw/pci_regs.h |  2 ++
> > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > > index 0ad548f..edb3fe9 100644
> > > --- a/src/fw/pciinit.c
> > > +++ b/src/fw/pciinit.c
> > > @@ -636,6 +636,25 @@ pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> > >      return entry;
> > >  }
> > >
> > > +static int pci_bus_hotplug_support(struct pci_bus *bus)
> > > +{
> > > u8 shpc_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_SHPC);
> > > u8 pcie_cap = pci_find_capability(bus->bus_dev, PCI_CAP_ID_EXP);
> > > int downstream_port = 0;
> > > int slot_implemented = 0;
> > > if (pcie_cap) {
> > >    u16 pcie_flags = pci_config_readw(bus->bus_dev->bdf,
> > >                                      pcie_cap + PCI_EXP_FLAGS);
> > >    u16 port_type = ((pcie_flags & PCI_EXP_FLAGS_TYPE) >>
> > >                    PCI_EXP_FLAGS_TYPE_SHIFT) & 0xf;
> >
> > 0xf is not needed here: PCI_EXP_FLAGS_TYPE is enough.
> > Also should be u8.
>
> OK, thanks
> > >    downstream_port = (port_type == PCI_EXP_TYPE_DOWNSTREAM) ||
> > >                      (port_type == PCI_EXP_TYPE_ROOT_PORT);
> >
> > This is correct, the spec says:
> >     Configuration Space, and other capabilities.
> >     8 Slot Implemented – When Set, this bit indicates that the Link
> >     HwInit associated with this Port is connected to a slot (as compared to
> >     being connected to a system-integrated device or being
> >     disabled).
> >     This bit is valid for Downstream Ports. This bit is undefined for
> >     Upstream Ports.
> >
> > Maybe add a comment in case people will wonder.
>
> Sure
> > >    slot_implemented = !!(pcie_flags & PCI_EXP_FLAGS_SLOT);
> >
> > I would do
> >         return downstream_port && slot_implemented;
> > here, avoid testing shpc for express devices.
> >
> > Also allows moving downstream_port && slot_implemented here.
>
> Makes sense
> > > }
> > > return shpc_cap || (downstream_port && slot_implemented);
> > >
> > > +}
> > >
> > > static int pci_bios_check_devices(struct pci_bus *busses)
> > >  {
> > >      dprintf(1, "PCI: check devices\n");
> > > @@ -678,7 +697,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> > >              continue;
> > >          struct pci_bus *parent = &busses[pci_bdf_to_bus(s->bus_dev->bdf)];
> > >          int type;
> > >    u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC);
> > >    int hotplug_support = pci_bus_hotplug_support(s);
> > >    for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> > >        u64 align = (type == PCI_REGION_TYPE_IO) ?
> > >            PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> > >
> > > @@ -687,7 +706,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> > >              if (pci_region_align(&s->r[type]) > align)
> > >                   align = pci_region_align(&s->r[type]);
> > >              u64 sum = pci_region_sum(&s->r[type]);
> > >        if (!sum && shpc_cap)
> > >        if (!sum && hotplug_support)
> > >            sum = align; /* reserve min size for hot-plug */
> > >        u64 size = ALIGN(sum, align);
> > >        int is64 = pci_bios_bridge_region_is64(&s->r[type],
> > >
> > > diff --git a/src/hw/pci_regs.h b/src/hw/pci_regs.h
> > > index e5effd4..6a71569 100644
> > > --- a/src/hw/pci_regs.h
> > > +++ b/src/hw/pci_regs.h
> > > @@ -426,6 +426,8 @@
> > >  #define PCI_EXP_DEVCTL2		40	/* Device Control 2 */
> > >  #define  PCI_EXP_DEVCTL2_ARI	0x20	/* Alternative Routing-ID */
> > >
> > > +#define PCI_EXP_FLAGS_TYPE_SHIFT 4
> >
> > It's generally not a good idea to randomly add macros
> > in random places.
> > In this case this is not needed. Just call __ffs on
> > PCI_EXP_FLAGS_TYPE.
>
> I didn't see any usage of __ffs method on seabios code, so
> I wondered if adding an include to string.h would be ok.
> Would it?
>
> Thanks,
> Marcel

No - seabios has its own string.h, but src/x86.h has __ffs.
Or you can use __builtin_ffs: it's optimized out
for a constant parameter, anyway.
> > > /* Extended Capabilities (PCI-X 2.0 and Express) */
> > >  #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
> > > #define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
> > >
> > > 1.8.3.1