| Message ID | 20200129152937.311162-10-s.miroshnichenko@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Allow BAR movement during boot and hotplug | expand |
On Wed, Jan 29, 2020 at 06:29:20PM +0300, Sergei Miroshnichenko wrote: > When movable BARs are enabled, and if a bridge contains a device with fixed > (IORESOURCE_PCI_FIXED) or immovable BARs, the corresponding windows can't What is the difference between fixed (IORESOURCE_PCI_FIXED) and immovable BARs? I'm hesitant to add a new concept ("immovable") with a different name but very similar meaning. I understand that in the case of bridge windows, you may need to track only part of the window, as opposed to a BAR where the entire BAR has to be either fixed or movable, but I think adding a new term will be confusing. > be moved too far away from their original positions - they must still > contain all the fixed/immovable BARs, like that: > > 1) Window position before a bus rescan: > > | <-- root bridge window --> | > | | > | | <-- bridge window --> | | > | | movable BARs | **fixed BAR** | | > ^^^^^^^^^^^^ > > 2) Possible valid outcome after rescan and move: > > | <-- root bridge window --> | > | | > | | <-- bridge window --> | | > | | **fixed BAR** | Movable BARs | | > ^^^^^^^^^^^^ > > An immovable area of a bridge window is a range that covers all the fixed > and immovable BARs of direct children, and all the fixed area of children > bridges: > > | <-- root bridge window --> | > | | > | | <-- bridge window level 1 --> | | > | | ******************** immovable area ******************* | | > | | | | > | | **fixed BAR** | <-- bridge window level 2 --> | BARs | | > | | | *********** immovable area *********** | | | > | | | | | | > | | | **fixed BAR** | BARs | **fixed BAR** | | | > ^^^^ > > To store these areas, the .immovable_range field has been added to struct > pci_bus for every bridge window type: IO, MEM and PREFETCH. It is filled > recursively from leaves to the root before a rescan. > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > --- > drivers/pci/pci.h | 14 ++++++++ > drivers/pci/probe.c | 88 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 6 ++++ > 3 files changed, 108 insertions(+) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 3b4c982772d3..5f2051c8531c 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -404,6 +404,20 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) > return dev->error_state == pci_channel_io_perm_failure; > } > > +static inline int pci_get_bridge_resource_idx(struct resource *r) > +{ > + int idx = 1; > + > + if (r->flags & IORESOURCE_IO) > + idx = 0; > + else if (!(r->flags & IORESOURCE_PREFETCH)) > + idx = 1; > + else if (r->flags & IORESOURCE_MEM_64) > + idx = 2; Random nit: No variables or elses required: if (r->flags & IORESOURCE_IO) return 0; if (!(r->flags & IORESOURCE_PREFETCH)) return 1; ... > + return idx; > +}
On Thu, 2020-01-30 at 15:26 -0600, Bjorn Helgaas wrote: > On Wed, Jan 29, 2020 at 06:29:20PM +0300, Sergei Miroshnichenko > wrote: > > When movable BARs are enabled, and if a bridge contains a device > > with fixed > > (IORESOURCE_PCI_FIXED) or immovable BARs, the corresponding windows > > can't > > What is the difference between fixed (IORESOURCE_PCI_FIXED) and > immovable BARs? I'm hesitant to add a new concept ("immovable") with > a different name but very similar meaning. > > I understand that in the case of bridge windows, you may need to > track > only part of the window, as opposed to a BAR where the entire BAR has > to be either fixed or movable, but I think adding a new term will be > confusing. > I thought the term "fixed BAR" has some legacy scent, and those marked with flag IORESOURCE_PCI_FIXED are fixed forever. But a BAR may become movable after rmmod-ing its driver that didn't support movable BARs. Still, the IORESOURCE_PCI_FIXED is used just a few times in the kernel, so the "fixed BAR" term is probably not so well-established, so I don't mind referring all non-movables as "fixed": both marked with the flag and not. Will change all of them in v8. > > be moved too far away from their original positions - they must > > still > > contain all the fixed/immovable BARs, like that: > > > > 1) Window position before a bus rescan: > > > > | <-- root bridge > > window --> | > > | > > | > > | | <-- bridge window --> > > | | > > | | movable BARs | **fixed BAR** > > | | > > ^^^^^^^^^^^^ > > > > 2) Possible valid outcome after rescan and move: > > > > | <-- root bridge > > window --> | > > | > > | > > | | <-- bridge window --> > > | | > > | | **fixed BAR** | Movable BARs > > | | > > ^^^^^^^^^^^^ > > > > An immovable area of a bridge window is a range that covers all the > > fixed > > and immovable BARs of direct children, and all the fixed area of > > children > > bridges: > > > > | <-- root bridge > > window --> | > > | > > | > > | | <-- bridge window level 1 - > > -> | | > > | | ******************** immovable area > > ******************* | | > > | | > > | | > > | | **fixed BAR** | <-- bridge window level 2 --> | > > BARs | | > > | | | *********** immovable area *********** > > | | | > > | | | | > > | | > > | | | **fixed BAR** | BARs | **fixed BAR** > > | | | > > ^^^^ > > > > To store these areas, the .immovable_range field has been added to > > struct > > pci_bus for every bridge window type: IO, MEM and PREFETCH. It is > > filled > > recursively from leaves to the root before a rescan. > > > > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> > > --- > > drivers/pci/pci.h | 14 ++++++++ > > drivers/pci/probe.c | 88 > > +++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 6 ++++ > > 3 files changed, 108 insertions(+) > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index 3b4c982772d3..5f2051c8531c 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -404,6 +404,20 @@ static inline bool > > pci_dev_is_disconnected(const struct pci_dev *dev) > > return dev->error_state == pci_channel_io_perm_failure; > > } > > > > +static inline int pci_get_bridge_resource_idx(struct resource *r) > > +{ > > + int idx = 1; > > + > > + if (r->flags & IORESOURCE_IO) > > + idx = 0; > > + else if (!(r->flags & IORESOURCE_PREFETCH)) > > + idx = 1; > > + else if (r->flags & IORESOURCE_MEM_64) > > + idx = 2; > > Random nit: No variables or elses required: > > if (r->flags & IORESOURCE_IO) > return 0; > if (!(r->flags & IORESOURCE_PREFETCH)) > return 1; > ... > Thank you! Best regards, Serge > > + return idx; > > +}
On Fri, Jan 31, 2020 at 04:59:44PM +0000, Sergei Miroshnichenko wrote: > On Thu, 2020-01-30 at 15:26 -0600, Bjorn Helgaas wrote: > > On Wed, Jan 29, 2020 at 06:29:20PM +0300, Sergei Miroshnichenko > > wrote: > > > When movable BARs are enabled, and if a bridge contains a device > > > with fixed > > > (IORESOURCE_PCI_FIXED) or immovable BARs, the corresponding windows > > > can't > > > > What is the difference between fixed (IORESOURCE_PCI_FIXED) and > > immovable BARs? I'm hesitant to add a new concept ("immovable") with > > a different name but very similar meaning. > > > > I understand that in the case of bridge windows, you may need to > > track > > only part of the window, as opposed to a BAR where the entire BAR has > > to be either fixed or movable, but I think adding a new term will be > > confusing. > > I thought the term "fixed BAR" has some legacy scent, and those marked > with flag IORESOURCE_PCI_FIXED are fixed forever. But a BAR may become > movable after rmmod-ing its driver that didn't support movable BARs. > > Still, the IORESOURCE_PCI_FIXED is used just a few times in the kernel, > so the "fixed BAR" term is probably not so well-established, so I don't > mind referring all non-movables as "fixed": both marked with the flag > and not. Will change all of them in v8. Yeah, "fixed" is a relatively new idea and not part of the actual spec. And you're right that "immovable" is slightly different because sometimes it's a consequence of lack of driver support. This probably isn't a big deal either way. I think there are two cases: (a) IORESOURCE_PCI_FIXED resources (i.e., legacy things that can't be changed because they don't have BARs) and (b) BARs that can't be changed because there's a driver bound that doesn't support moving them. Either way, we have to treat them the same for resource allocation, so I'm not sure it's worth differentiating at this level. Bjorn
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 3b4c982772d3..5f2051c8531c 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -404,6 +404,20 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) return dev->error_state == pci_channel_io_perm_failure; } +static inline int pci_get_bridge_resource_idx(struct resource *r) +{ + int idx = 1; + + if (r->flags & IORESOURCE_IO) + idx = 0; + else if (!(r->flags & IORESOURCE_PREFETCH)) + idx = 1; + else if (r->flags & IORESOURCE_MEM_64) + idx = 2; + + return idx; +} + /* pci_dev priv_flags */ #define PCI_DEV_ADDED 0 #define PCI_DEV_DISABLED_BARS 1 diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1e8bbf5c99f6..bb584038d5b4 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -546,6 +546,7 @@ void pci_read_bridge_bases(struct pci_bus *child) static struct pci_bus *pci_alloc_bus(struct pci_bus *parent) { struct pci_bus *b; + int idx; b = kzalloc(sizeof(*b), GFP_KERNEL); if (!b) @@ -562,6 +563,11 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent) if (parent) b->domain_nr = parent->domain_nr; #endif + for (idx = 0; idx < PCI_BRIDGE_RESOURCE_NUM; ++idx) { + b->immovable_range[idx].start = 0; + b->immovable_range[idx].end = 0; + } + return b; } @@ -3228,6 +3234,87 @@ static void pci_setup_bridges(struct pci_bus *bus) pci_setup_bridge(bus); } +static void pci_bus_update_immovable_range(struct pci_bus *bus) +{ + struct pci_dev *dev; + int idx; + resource_size_t start, end; + + for (idx = 0; idx < PCI_BRIDGE_RESOURCE_NUM; ++idx) { + bus->immovable_range[idx].start = 0; + bus->immovable_range[idx].end = 0; + } + + list_for_each_entry(dev, &bus->devices, bus_list) + if (dev->subordinate) + pci_bus_update_immovable_range(dev->subordinate); + + list_for_each_entry(dev, &bus->devices, bus_list) { + int i; + struct pci_bus *child = dev->subordinate; + + for (i = 0; i < PCI_BRIDGE_RESOURCES; ++i) { + struct resource *r = &dev->resource[i]; + + if (!r->flags || (r->flags & IORESOURCE_UNSET) || !r->parent) + continue; + + if (!pci_dev_bar_movable(dev, r)) { + idx = pci_get_bridge_resource_idx(r); + start = bus->immovable_range[idx].start; + end = bus->immovable_range[idx].end; + + if (!start || start > r->start) + start = r->start; + if (end < r->end) + end = r->end; + + if (bus->immovable_range[idx].start != start || + bus->immovable_range[idx].end != end) { + dev_dbg(&bus->dev, "Found fixed BAR%d 0x%llx-0x%llx in %s, expand the fixed bridge window %d to 0x%llx-0x%llx\n", + i, + (unsigned long long)r->start, + (unsigned long long)r->end, + dev_name(&dev->dev), idx, + (unsigned long long)start, + (unsigned long long)end); + bus->immovable_range[idx].start = start; + bus->immovable_range[idx].end = end; + } + } + } + + if (child) { + for (idx = 0; idx < PCI_BRIDGE_RESOURCE_NUM; ++idx) { + struct resource *child_immovable_range = + &child->immovable_range[idx]; + + if (child_immovable_range->start >= + child_immovable_range->end) + continue; + + start = bus->immovable_range[idx].start; + end = bus->immovable_range[idx].end; + + if (!start || start > child_immovable_range->start) + start = child_immovable_range->start; + if (end < child_immovable_range->end) + end = child_immovable_range->end; + + if (start < bus->immovable_range[idx].start || + end > bus->immovable_range[idx].end) { + dev_dbg(&bus->dev, "Expand the fixed bridge window %d from %s to 0x%llx-0x%llx\n", + idx, dev_name(&child->dev), + (unsigned long long)start, + (unsigned long long)end); + bus->immovable_range[idx].start = start; + bus->immovable_range[idx].end = end; + } + } + } + } +} + static struct pci_dev *pci_find_next_new_device(struct pci_bus *bus) { struct pci_dev *dev; @@ -3324,6 +3411,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus) if (pci_can_move_bars) { pci_bus_rescan_prepare(root); + pci_bus_update_immovable_range(root); pci_bus_release_root_bridge_resources(root); max = pci_scan_child_bus(root); diff --git a/include/linux/pci.h b/include/linux/pci.h index a8f2c26757fe..d6eb498f7997 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -582,6 +582,12 @@ struct pci_bus { struct list_head resources; /* Address space routed to this bus */ struct resource busn_res; /* Bus numbers routed to this bus */ + /* + * If there are fixed or immovable resources in the bridge window, this range + * contains the lowest start address and highest end address of them. + */ + struct resource immovable_range[PCI_BRIDGE_RESOURCE_NUM]; + struct pci_ops *ops; /* Configuration access functions */ struct msi_controller *msi; /* MSI controller */ void *sysdata; /* Hook for sys-specific extension */
When movable BARs are enabled, and if a bridge contains a device with fixed (IORESOURCE_PCI_FIXED) or immovable BARs, the corresponding windows can't be moved too far away from their original positions - they must still contain all the fixed/immovable BARs, like that: 1) Window position before a bus rescan: | <-- root bridge window --> | | | | | <-- bridge window --> | | | | movable BARs | **fixed BAR** | | ^^^^^^^^^^^^ 2) Possible valid outcome after rescan and move: | <-- root bridge window --> | | | | | <-- bridge window --> | | | | **fixed BAR** | Movable BARs | | ^^^^^^^^^^^^ An immovable area of a bridge window is a range that covers all the fixed and immovable BARs of direct children, and all the fixed area of children bridges: | <-- root bridge window --> | | | | | <-- bridge window level 1 --> | | | | ******************** immovable area ******************* | | | | | | | | **fixed BAR** | <-- bridge window level 2 --> | BARs | | | | | *********** immovable area *********** | | | | | | | | | | | | **fixed BAR** | BARs | **fixed BAR** | | | ^^^^ To store these areas, the .immovable_range field has been added to struct pci_bus for every bridge window type: IO, MEM and PREFETCH. It is filled recursively from leaves to the root before a rescan. Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com> --- drivers/pci/pci.h | 14 ++++++++ drivers/pci/probe.c | 88 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 6 ++++ 3 files changed, 108 insertions(+)