| Message ID | 20200129152937.311162-21-s.miroshnichenko@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Allow BAR movement during boot and hotplug | expand |
On Wednesday, January 29, 2020 4:29:31 PM CET Sergei Miroshnichenko wrote: > When the Movable BARs feature is supported, the PCI subsystem is able to > distribute existing BARs and allocate the new ones itself, without need to > reserve gaps by BIOS. > > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > --- > drivers/pnp/system.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c > index 6950503741eb..16cd260a609d 100644 > --- a/drivers/pnp/system.c > +++ b/drivers/pnp/system.c > @@ -12,6 +12,7 @@ > #include <linux/device.h> > #include <linux/init.h> > #include <linux/slab.h> > +#include <linux/pci.h> > #include <linux/kernel.h> > #include <linux/ioport.h> > > @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct pnp_dev *dev) > struct resource *res; > int i; > > +#ifdef CONFIG_PCI > + if (pci_can_move_bars) > + return; > +#endif Would it be a problem to define pci_can_move_bars() as a static inline returning 'false' when CONFIG_PCI is unset? The #ifdef wouldn't be needed here then. > + > for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) { > if (res->flags & IORESOURCE_DISABLED) > continue; >
On Wed, Jan 29, 2020 at 06:29:31PM +0300, Sergei Miroshnichenko wrote: > When the Movable BARs feature is supported, the PCI subsystem is able to > distribute existing BARs and allocate the new ones itself, without need to > reserve gaps by BIOS. > > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > --- > drivers/pnp/system.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c > index 6950503741eb..16cd260a609d 100644 > --- a/drivers/pnp/system.c > +++ b/drivers/pnp/system.c > @@ -12,6 +12,7 @@ > #include <linux/device.h> > #include <linux/init.h> > #include <linux/slab.h> > +#include <linux/pci.h> > #include <linux/kernel.h> > #include <linux/ioport.h> > > @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct pnp_dev *dev) > struct resource *res; > int i; > > +#ifdef CONFIG_PCI > + if (pci_can_move_bars) > + return; > +#endif I don't understand this. The reason this function exists is so we keep track of the resources consumed by PNP devices and we can keep from assigning those resources to other things like PCI devices. Admittedly we currently only do this for PNP0C01 and PNP0C02 devices, but we really should do it for all PNP devices. Why does Movable BARs mean that we no longer need this information? The whole point is that this information is needed *during* PCI resource allocation, so I don't understand the idea that "because the PCI subsystem is able to distribute existing BARs and allocate the new ones itself", we don't need to know about PNP resources to avoid. > for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) { > if (res->flags & IORESOURCE_DISABLED) > continue; > -- > 2.24.1 >
Hello Bjorn, On Thu, 2020-01-30 at 15:14 -0600, Bjorn Helgaas wrote: > On Wed, Jan 29, 2020 at 06:29:31PM +0300, Sergei Miroshnichenko > wrote: > > When the Movable BARs feature is supported, the PCI subsystem is > > able to > > distribute existing BARs and allocate the new ones itself, without > > need to > > reserve gaps by BIOS. > > > > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > > --- > > drivers/pnp/system.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c > > index 6950503741eb..16cd260a609d 100644 > > --- a/drivers/pnp/system.c > > +++ b/drivers/pnp/system.c > > @@ -12,6 +12,7 @@ > > #include <linux/device.h> > > #include <linux/init.h> > > #include <linux/slab.h> > > +#include <linux/pci.h> > > #include <linux/kernel.h> > > #include <linux/ioport.h> > > > > @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct > > pnp_dev *dev) > > struct resource *res; > > int i; > > > > +#ifdef CONFIG_PCI > > + if (pci_can_move_bars) > > + return; > > +#endif > > I don't understand this. The reason this function exists is so we > keep track of the resources consumed by PNP devices and we can keep > from assigning those resources to other things like PCI devices. > > Admittedly we currently only do this for PNP0C01 and PNP0C02 devices, > but we really should do it for all PNP devices. > > Why does Movable BARs mean that we no longer need this information? > The whole point is that this information is needed *during* PCI > resource allocation, so I don't understand the idea that "because the > PCI subsystem is able to distribute existing BARs and allocate the > new > ones itself", we don't need to know about PNP resources to avoid. > Oh. I've made this patch in assumption that non-PCI PNP devices should not reside in the PCI address space, and PCI PNP devices behave like usual PCI devices - with BARs handled by the common PCI subsystem. Do I understand correctly after digging a bit into drivers/pnp, that some of these resources are some kind of "invisible" BARs, which are used by drivers, but the PCI subsystem can't "see" them, so that's why the PNP reserves them? In this case I need just to discard this patch and to modify the pci_bus_release_root_bridge_resources() added in patch 06/26 - remove the pci_bus_for_each_resource(root_bus, r, i) block there, which releases such non-BAR resourses. I've just checked that it works, so the next version - v8 - of this patchset will be a bit lighter. Thank you for pointing that out! Best regards, Serge > > for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); > > i++) { > > if (res->flags & IORESOURCE_DISABLED) > > continue; > > -- > > 2.24.1 > >
On Fri, Jan 31, 2020 at 03:48:48PM +0000, Sergei Miroshnichenko wrote: > Hello Bjorn, > > On Thu, 2020-01-30 at 15:14 -0600, Bjorn Helgaas wrote: > > On Wed, Jan 29, 2020 at 06:29:31PM +0300, Sergei Miroshnichenko > > wrote: > > > When the Movable BARs feature is supported, the PCI subsystem is > > > able to > > > distribute existing BARs and allocate the new ones itself, without > > > need to > > > reserve gaps by BIOS. > > > > > > CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > > > --- > > > drivers/pnp/system.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c > > > index 6950503741eb..16cd260a609d 100644 > > > --- a/drivers/pnp/system.c > > > +++ b/drivers/pnp/system.c > > > @@ -12,6 +12,7 @@ > > > #include <linux/device.h> > > > #include <linux/init.h> > > > #include <linux/slab.h> > > > +#include <linux/pci.h> > > > #include <linux/kernel.h> > > > #include <linux/ioport.h> > > > > > > @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct > > > pnp_dev *dev) > > > struct resource *res; > > > int i; > > > > > > +#ifdef CONFIG_PCI > > > + if (pci_can_move_bars) > > > + return; > > > +#endif > > > > I don't understand this. The reason this function exists is so we > > keep track of the resources consumed by PNP devices and we can keep > > from assigning those resources to other things like PCI devices. > > > > Admittedly we currently only do this for PNP0C01 and PNP0C02 devices, > > but we really should do it for all PNP devices. > > > > Why does Movable BARs mean that we no longer need this > > information? The whole point is that this information is needed > > *during* PCI resource allocation, so I don't understand the idea > > that "because the PCI subsystem is able to distribute existing > > BARs and allocate the new ones itself", we don't need to know > > about PNP resources to avoid. > > Oh. I've made this patch in assumption that non-PCI PNP devices should > not reside in the PCI address space, and PCI PNP devices behave like > usual PCI devices - with BARs handled by the common PCI subsystem. I don't think we can rely on that assumption. I think all we should assume is that address space described by _CRS of any PNP device is unavailable for use by other devices (except for bridge windows, of course). > Do I understand correctly after digging a bit into drivers/pnp, that > some of these resources are some kind of "invisible" BARs, which are > used by drivers, but the PCI subsystem can't "see" them, so that's why > the PNP reserves them? ACPI/PNP _CRS is sort of a generalized BAR idea for devices that don't have a native configuration protocol. E.g., PCI has config space that supports both enumeration and resource configuration (BARs). There may be other buses that have similar ideas and don't need PNP devices. Generally, ACPI describes devices that can't be enumerated and configured via native means. > In this case I need just to discard this patch and to modify the > pci_bus_release_root_bridge_resources() added in patch 06/26 - remove > the pci_bus_for_each_resource(root_bus, r, i) block there, which > releases such non-BAR resourses. I've just checked that it works, so > the next version - v8 - of this patchset will be a bit lighter. Thank > you for pointing that out! > > Best regards, > Serge > > > > > for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); > > > i++) { > > > if (res->flags & IORESOURCE_DISABLED) > > > continue; > > > -- > > > 2.24.1 > > >
diff --git a/drivers/pnp/system.c b/drivers/pnp/system.c index 6950503741eb..16cd260a609d 100644 --- a/drivers/pnp/system.c +++ b/drivers/pnp/system.c @@ -12,6 +12,7 @@ #include <linux/device.h> #include <linux/init.h> #include <linux/slab.h> +#include <linux/pci.h> #include <linux/kernel.h> #include <linux/ioport.h> @@ -58,6 +59,11 @@ static void reserve_resources_of_dev(struct pnp_dev *dev) struct resource *res; int i; +#ifdef CONFIG_PCI + if (pci_can_move_bars) + return; +#endif + for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) { if (res->flags & IORESOURCE_DISABLED) continue;
When the Movable BARs feature is supported, the PCI subsystem is able to distribute existing BARs and allocate the new ones itself, without need to reserve gaps by BIOS. CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> --- drivers/pnp/system.c | 6 ++++++ 1 file changed, 6 insertions(+)