| Message ID | 1396868342-12005-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mo, 2014-04-07 at 13:59 +0300, Marcel Apfelbaum wrote: > If a pci-2-pci bridge supports hot-plug functionality but there are no devices > connected to it, reserve IO/mem in order to be able to attach devices > later. Do not waste space, use minimum allowed. Makes sense. > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); Should we also check for hotplug-capable pci express ports while being at it? cheers, Gerd
On Mon, Apr 07, 2014 at 02:01:41PM +0200, Gerd Hoffmann wrote: > On Mo, 2014-04-07 at 13:59 +0300, Marcel Apfelbaum wrote: > > If a pci-2-pci bridge supports hot-plug functionality but there are no devices > > connected to it, reserve IO/mem in order to be able to attach devices > > later. Do not waste space, use minimum allowed. > > Makes sense. > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > Should we also check for hotplug-capable pci express ports while being > at it? > > cheers, > Gerd > Can be a separate patch?
On Mon, Apr 07, 2014 at 01:59:02PM +0300, Marcel Apfelbaum wrote: > If a pci-2-pci bridge supports hot-plug functionality but there are no devices > connected to it, reserve IO/mem in order to be able to attach devices > later. Do not waste space, use minimum allowed. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > src/fw/pciinit.c | 3 +++ > src/hw/pci.c | 17 +++++++++++++++++ > src/hw/pci.h | 1 + > 3 files changed, 21 insertions(+) > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 64f1d41..9b5d7ad 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -677,12 +677,15 @@ 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); > for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { > u64 align = (type == PCI_REGION_TYPE_IO) ? > PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN; > 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) > + sum = align; /* reserve min size for hot-plug */ > u64 size = ALIGN(sum, align); > int is64 = pci_bios_bridge_region_is64(&s->r[type], > s->bus_dev, type); One thing I'd do is maybe check that the relevant memory type is enabled in the bridge (probably just by writing fff to base and reading it back). This will give hypervisors an option to avoid wasting resources: e.g. it's uncommon for express devices to claim IO. > diff --git a/src/hw/pci.c b/src/hw/pci.c > index caf9265..ee8a7f1 100644 > --- a/src/hw/pci.c > +++ b/src/hw/pci.c > @@ -225,6 +225,23 @@ pci_find_init_device(const struct pci_device_id *ids, void *arg) > return NULL; > } > > +u8 pci_find_capability(struct pci_device *pci, u8 cap_id) > +{ > + u8 cap; > + u16 status = pci_config_readw(pci->bdf, PCI_STATUS); > + > + if (!(status & PCI_STATUS_CAP_LIST)) > + return 0; > + > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > + return cap; I would also limit this to 256 iterations, to make sure we dont' get into an infinite loop with a broken device. > + > + return 0; > +} > + > + > void > pci_reboot(void) > { > diff --git a/src/hw/pci.h b/src/hw/pci.h > index 167a027..e828225 100644 > --- a/src/hw/pci.h > +++ b/src/hw/pci.h > @@ -116,6 +116,7 @@ int pci_init_device(const struct pci_device_id *ids > , struct pci_device *pci, void *arg); > struct pci_device *pci_find_init_device(const struct pci_device_id *ids > , void *arg); > +u8 pci_find_capability(struct pci_device *pci, u8 cap_id); > void pci_reboot(void); > > #endif > -- > 1.8.3.1
On Mon, 2014-04-07 at 15:11 +0300, Michael S. Tsirkin wrote: > On Mon, Apr 07, 2014 at 02:01:41PM +0200, Gerd Hoffmann wrote: > > On Mo, 2014-04-07 at 13:59 +0300, Marcel Apfelbaum wrote: > > > If a pci-2-pci bridge supports hot-plug functionality but there are no devices > > > connected to it, reserve IO/mem in order to be able to attach devices > > > later. Do not waste space, use minimum allowed. > > > > Makes sense. > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > Should we also check for hotplug-capable pci express ports while being > > at it? > > > > cheers, > > Gerd > > > > Can be a separate patch? > I think it can be. This one handles pci-2-pci bridges and avoids PCIe by choice. I do plan to handle PCIe hotplug corners later. Thanks, Marcel
Hi, > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > One thing I'd do is maybe check that the relevant memory type is > enabled in the bridge (probably just by writing fff to base and reading > it back). > This will give hypervisors an option to avoid wasting resources: > e.g. it's uncommon for express devices to claim IO. I don't think we'll need that for the SHPC bridge. For express it indeed makes sense to avoid claiming IO address space. I'd try to find something more automatic though, where you don't need some kind of "disable io for this express port" config option. For express ports which can only have a single device underneath we can check whenever we have a device and if one is present already don't bother claiming extra resources for hotplug. > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > + return cap; > > I would also limit this to 256 iterations, to make sure > we dont' get into an infinite loop with a broken device. Good point. cheers, Gerd
On Mon, 2014-04-07 at 14:44 +0200, Gerd Hoffmann wrote: > Hi, > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > One thing I'd do is maybe check that the relevant memory type is > > enabled in the bridge (probably just by writing fff to base and reading > > it back). > > > This will give hypervisors an option to avoid wasting resources: > > e.g. it's uncommon for express devices to claim IO. > > I don't think we'll need that for the SHPC bridge. I agree. This is why I chose to check shpc capability, because PCIe ports do not use it (as far as I know). > > For express it indeed makes sense to avoid claiming IO address space. > I'd try to find something more automatic though, where you don't need > some kind of "disable io for this express port" config option. > > For express ports which can only have a single device underneath we can > check whenever we have a device and if one is present already don't > bother claiming extra resources for hotplug. > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > + return cap; > > > > I would also limit this to 256 iterations, to make sure > > we dont' get into an infinite loop with a broken device. > > Good point. Yes, thanks! sending v2. Marcel > > cheers, > Gerd > >
On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > Hi, > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > One thing I'd do is maybe check that the relevant memory type is > > enabled in the bridge (probably just by writing fff to base and reading > > it back). > > > This will give hypervisors an option to avoid wasting resources: > > e.g. it's uncommon for express devices to claim IO. > > I don't think we'll need that for the SHPC bridge. Why not? I'm referring to this text in the bridge specification: The I/O Base and I/O Limit registers are optional and define an address range that is used by the bridge to determine when to forward I/O transactions from one interface to the other. If a bridge does not implement an I/O address range, then both the I/O Base and I/O Limit registers must be implemented as read-only registers that return zero when read. If a bridge supports an I/O address range, then these registers must be initialized by configuration software so default states are not specified. So we should probe bridge for I/O support before wasting I/O resources on it. The spec does not provide a way to detect this, but we can do it like this: - write value ffffffff to I/O base register - read back value value 0 means bridge does not support I/O. A similar trick should work for other optional resources. > For express it indeed makes sense to avoid claiming IO address space. > I'd try to find something more automatic though, where you don't need > some kind of "disable io for this express port" config option. Won't same trick as above work? > For express ports which can only have a single device underneath we can > check whenever we have a device and if one is present already don't > bother claiming extra resources for hotplug. > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > + return cap; > > > > I would also limit this to 256 iterations, to make sure > > we dont' get into an infinite loop with a broken device. > > Good point. > > cheers, > Gerd >
On Mon, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote: > On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > > One thing I'd do is maybe check that the relevant memory type is > > > enabled in the bridge (probably just by writing fff to base and reading > > > it back). > > > > > This will give hypervisors an option to avoid wasting resources: > > > e.g. it's uncommon for express devices to claim IO. > > > > I don't think we'll need that for the SHPC bridge. > > Why not? Because "has shpc" => not an PCIe port. (as far as I know) Anyway, why have shpc capability but no I/O or mem to support it? Thanks, Marcel > I'm referring to this text in the bridge specification: > > The I/O Base and I/O Limit registers are optional and define an address > range that is used > by the bridge to determine when to forward I/O transactions from one > interface to the > other. > If a bridge does not implement an I/O address range, then both the I/O > Base and I/O > Limit registers must be implemented as read-only registers that return > zero when read. If a > bridge supports an I/O address range, then these registers must be > initialized by > configuration software so default states are not specified. > > So we should probe bridge for I/O support before wasting I/O resources on it. > The spec does not provide a way to detect this, but we can do it like this: > > - write value ffffffff to I/O base register Why write? A simple read would be enough. It will never be 0(if I/O or mem is required) because of the "Base Address" part of the register which represents the address range, right? Thanks, Marcel > - read back value > > value 0 means bridge does not support I/O. > > > A similar trick should work for other optional resources. > > > > For express it indeed makes sense to avoid claiming IO address space. > > I'd try to find something more automatic though, where you don't need > > some kind of "disable io for this express port" config option. > > Won't same trick as above work? > > > For express ports which can only have a single device underneath we can > > check whenever we have a device and if one is present already don't > > bother claiming extra resources for hotplug. > > > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > > + return cap; > > > > > > I would also limit this to 256 iterations, to make sure > > > we dont' get into an infinite loop with a broken device. > > > > Good point. > > > > cheers, > > Gerd > >
On Mon, 2014-04-07 at 16:51 +0300, Marcel Apfelbaum wrote: > On Mon, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > > > Hi, > > > > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > > > > One thing I'd do is maybe check that the relevant memory type is > > > > enabled in the bridge (probably just by writing fff to base and reading > > > > it back). > > > > > > > This will give hypervisors an option to avoid wasting resources: > > > > e.g. it's uncommon for express devices to claim IO. > > > > > > I don't think we'll need that for the SHPC bridge. > > > > Why not? > Because "has shpc" => not an PCIe port. (as far as I know) > Anyway, why have shpc capability but no I/O or mem to support it? > I signed too soon :), I have another question below, > Thanks, > Marcel > [...] > > > > So we should probe bridge for I/O support before wasting I/O resources on it. > > The spec does not provide a way to detect this, but we can do it like this: > > > > - write value ffffffff to I/O base register > Why write? A simple read would be enough. > It will never be 0(if I/O or mem is required) because of the "Base Address" > part of the register which represents the address range, right? Here ^^^ > > Thanks, > Marcel [...]
On Mon, Apr 07, 2014 at 04:51:54PM +0300, Marcel Apfelbaum wrote: > On Mon, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > > > Hi, > > > > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > > > > One thing I'd do is maybe check that the relevant memory type is > > > > enabled in the bridge (probably just by writing fff to base and reading > > > > it back). > > > > > > > This will give hypervisors an option to avoid wasting resources: > > > > e.g. it's uncommon for express devices to claim IO. > > > > > > I don't think we'll need that for the SHPC bridge. > > > > Why not? > Because "has shpc" => not an PCIe port. (as far as I know) > Anyway, why have shpc capability but no I/O or mem to support it? > > Thanks, > Marcel > > > I'm referring to this text in the bridge specification: > > > > The I/O Base and I/O Limit registers are optional and define an address > > range that is used > > by the bridge to determine when to forward I/O transactions from one > > interface to the > > other. > > If a bridge does not implement an I/O address range, then both the I/O > > Base and I/O > > Limit registers must be implemented as read-only registers that return > > zero when read. If a > > bridge supports an I/O address range, then these registers must be > > initialized by > > configuration software so default states are not specified. > > > > So we should probe bridge for I/O support before wasting I/O resources on it. > > The spec does not provide a way to detect this, but we can do it like this: > > > > - write value ffffffff to I/O base register > Why write? A simple read would be enough. > It will never be 0(if I/O or mem is required) > because of the "Base Address" > part of the register which represents the address range, right? > > Thanks, > Marcel AFAIK the spec does not list reset value for this register. IIRC QEMU resets both to 0. > > - read back value > > > > value 0 means bridge does not support I/O. > > > > > > A similar trick should work for other optional resources. > > > > > > > For express it indeed makes sense to avoid claiming IO address space. > > > I'd try to find something more automatic though, where you don't need > > > some kind of "disable io for this express port" config option. > > > > Won't same trick as above work? > > > > > For express ports which can only have a single device underneath we can > > > check whenever we have a device and if one is present already don't > > > bother claiming extra resources for hotplug. > > > > > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > > > + return cap; > > > > > > > > I would also limit this to 256 iterations, to make sure > > > > we dont' get into an infinite loop with a broken device. > > > > > > Good point. > > > > > > cheers, > > > Gerd > > > > >
On Mon, 2014-04-07 at 17:09 +0300, Michael S. Tsirkin wrote: > On Mon, Apr 07, 2014 at 04:51:54PM +0300, Marcel Apfelbaum wrote: [...] > > > > I don't think we'll need that for the SHPC bridge. > > > > > > Why not? > > Because "has shpc" => not an PCIe port. (as far as I know) > > Anyway, why have shpc capability but no I/O or mem to support it? ^^^ [...] > > > AFAIK the spec does not list reset value for this register. > IIRC QEMU resets both to 0. Thanks, what do you think about the above ? ^^^ While I am not against it, it seems redundant. It has shpc => it needs I/O or mem space. Marcel > > > > > - read back value > > > > > > value 0 means bridge does not support I/O. > > > > > > > > > A similar trick should work for other optional resources. > > > > > > > > > > For express it indeed makes sense to avoid claiming IO address space. > > > > I'd try to find something more automatic though, where you don't need > > > > some kind of "disable io for this express port" config option. > > > > > > Won't same trick as above work? > > > > > > > For express ports which can only have a single device underneath we can > > > > check whenever we have a device and if one is present already don't > > > > bother claiming extra resources for hotplug. > > > > > > > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > > > > + return cap; > > > > > > > > > > I would also limit this to 256 iterations, to make sure > > > > > we dont' get into an infinite loop with a broken device. > > > > > > > > Good point. > > > > > > > > cheers, > > > > Gerd > > > > > > > >
On Mon, Apr 07, 2014 at 05:16:13PM +0300, Marcel Apfelbaum wrote: > On Mon, 2014-04-07 at 17:09 +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 07, 2014 at 04:51:54PM +0300, Marcel Apfelbaum wrote: > [...] > > > > > I don't think we'll need that for the SHPC bridge. > > > > > > > > Why not? > > > Because "has shpc" => not an PCIe port. (as far as I know) > > > Anyway, why have shpc capability but no I/O or mem to support it? > ^^^ > [...] > > > > > > AFAIK the spec does not list reset value for this register. > > IIRC QEMU resets both to 0. > Thanks, what do you think about the above ? ^^^ > While I am not against it, it seems redundant. > It has shpc => it needs I/O or mem space. > > Marcel Fair enough but it can have shpc and memory without io, or shpc and io without memory. > > > > > > > > - read back value > > > > > > > > value 0 means bridge does not support I/O. > > > > > > > > > > > > A similar trick should work for other optional resources. > > > > > > > > > > > > > For express it indeed makes sense to avoid claiming IO address space. > > > > > I'd try to find something more automatic though, where you don't need > > > > > some kind of "disable io for this express port" config option. > > > > > > > > Won't same trick as above work? > > > > > > > > > For express ports which can only have a single device underneath we can > > > > > check whenever we have a device and if one is present already don't > > > > > bother claiming extra resources for hotplug. > > > > > > > > > > > > + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; > > > > > > > + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) > > > > > > > + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) > > > > > > > + return cap; > > > > > > > > > > > > I would also limit this to 256 iterations, to make sure > > > > > > we dont' get into an infinite loop with a broken device. > > > > > > > > > > Good point. > > > > > > > > > > cheers, > > > > > Gerd > > > > > > > > > > > > >
On Mo, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote: > On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > > One thing I'd do is maybe check that the relevant memory type is > > > enabled in the bridge (probably just by writing fff to base and reading > > > it back). > > > > > This will give hypervisors an option to avoid wasting resources: > > > e.g. it's uncommon for express devices to claim IO. > > > > I don't think we'll need that for the SHPC bridge. > > Why not? With typical use cases for the shpc bridge you likely need the io window anyway. > I'm referring to this text in the bridge specification: > > The I/O Base and I/O Limit registers are optional and define an address > range that is used > by the bridge to determine when to forward I/O transactions from one > interface to the > other. > If a bridge does not implement an I/O address range, then both the I/O > Base and I/O > Limit registers must be implemented as read-only registers that return > zero when read. If a > bridge supports an I/O address range, then these registers must be > initialized by > configuration software so default states are not specified. > > So we should probe bridge for I/O support before wasting I/O resources on it. Yes, makes sense from a correctness point of view. I suspect you'll have a hard time to find such bridges in the x86 world though, so I'm not sure it is a good idea to emulate this in qemu. Guests might not handle it correctly. > > For express it indeed makes sense to avoid claiming IO address space. > > I'd try to find something more automatic though, where you don't need > > some kind of "disable io for this express port" config option. > > Won't same trick as above work? Yes, it will work. But as we probably want support io on express devices (because it is used in practice, even though being strongly discouraged in the pci express specs). So doing it that way would require a config switch on the qemu side to turn on/off io address space support for express switches/ports. cheers, Gerd
On Tue, Apr 08, 2014 at 08:05:23AM +0200, Gerd Hoffmann wrote: > On Mo, 2014-04-07 at 16:34 +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 07, 2014 at 02:44:06PM +0200, Gerd Hoffmann wrote: > > > Hi, > > > > > > > > + u8 shpc_cap = pci_find_capability(s->bus_dev, PCI_CAP_ID_SHPC); > > > > > > > One thing I'd do is maybe check that the relevant memory type is > > > > enabled in the bridge (probably just by writing fff to base and reading > > > > it back). > > > > > > > This will give hypervisors an option to avoid wasting resources: > > > > e.g. it's uncommon for express devices to claim IO. > > > > > > I don't think we'll need that for the SHPC bridge. > > > > Why not? > > With typical use cases for the shpc bridge you likely need the io window > anyway. That won't be true after virtio 1.0 is out. > > I'm referring to this text in the bridge specification: > > > > The I/O Base and I/O Limit registers are optional and define an address > > range that is used > > by the bridge to determine when to forward I/O transactions from one > > interface to the > > other. > > If a bridge does not implement an I/O address range, then both the I/O > > Base and I/O > > Limit registers must be implemented as read-only registers that return > > zero when read. If a > > bridge supports an I/O address range, then these registers must be > > initialized by > > configuration software so default states are not specified. > > > > So we should probe bridge for I/O support before wasting I/O resources on it. > > Yes, makes sense from a correctness point of view. So that's all I'm arguing for, for now: let's implement the spec correctly. Whether we should emulate such devices is a separate question. > I suspect you'll have a hard time to find such bridges in the x86 world > though, so I'm not sure it is a good idea to emulate this in qemu. > Guests might not handle it correctly. We'll have to test this. The handling is mostly done by BIOS so I don't expect a lot of problems. > > > For express it indeed makes sense to avoid claiming IO address space. > > > I'd try to find something more automatic though, where you don't need > > > some kind of "disable io for this express port" config option. > > > > Won't same trick as above work? > > Yes, it will work. > > But as we probably want support io on express devices (because it is > used in practice, even though being strongly discouraged in the pci > express specs). So doing it that way would require a config switch on > the qemu side to turn on/off io address space support for express > switches/ports. > > cheers, > Gerd True but I don't see a way around this anyway. At least this way we won't need PV.
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 64f1d41..9b5d7ad 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -677,12 +677,15 @@ 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); for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { u64 align = (type == PCI_REGION_TYPE_IO) ? PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN; 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) + sum = align; /* reserve min size for hot-plug */ u64 size = ALIGN(sum, align); int is64 = pci_bios_bridge_region_is64(&s->r[type], s->bus_dev, type); diff --git a/src/hw/pci.c b/src/hw/pci.c index caf9265..ee8a7f1 100644 --- a/src/hw/pci.c +++ b/src/hw/pci.c @@ -225,6 +225,23 @@ pci_find_init_device(const struct pci_device_id *ids, void *arg) return NULL; } +u8 pci_find_capability(struct pci_device *pci, u8 cap_id) +{ + u8 cap; + u16 status = pci_config_readw(pci->bdf, PCI_STATUS); + + if (!(status & PCI_STATUS_CAP_LIST)) + return 0; + + for (cap = pci_config_readb(pci->bdf, PCI_CAPABILITY_LIST); cap; + cap = pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_NEXT)) + if (pci_config_readb(pci->bdf, cap + PCI_CAP_LIST_ID) == cap_id) + return cap; + + return 0; +} + + void pci_reboot(void) { diff --git a/src/hw/pci.h b/src/hw/pci.h index 167a027..e828225 100644 --- a/src/hw/pci.h +++ b/src/hw/pci.h @@ -116,6 +116,7 @@ int pci_init_device(const struct pci_device_id *ids , struct pci_device *pci, void *arg); struct pci_device *pci_find_init_device(const struct pci_device_id *ids , void *arg); +u8 pci_find_capability(struct pci_device *pci, u8 cap_id); void pci_reboot(void); #endif
If a pci-2-pci bridge supports hot-plug functionality but there are no devices connected to it, reserve IO/mem in order to be able to attach devices later. Do not waste space, use minimum allowed. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- src/fw/pciinit.c | 3 +++ src/hw/pci.c | 17 +++++++++++++++++ src/hw/pci.h | 1 + 3 files changed, 21 insertions(+)