[2/4] PCI: add functionality for resizing resources v2

From: Christian König <christian.koenig@amd.com>

This allows device drivers to request resizing their BARs.

The function only tries to reprogram the windows of the bridge directly above
the requesting device and only the BAR of the same type (usually mem, 64bit,
prefetchable). This is done to make sure not to disturb other drivers by
changing the BARs of their devices.

If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
returned to the calling device driver.

v2: rebase on changes in rbar support

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/pci/setup-bus.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-res.c | 51 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 114 insertions(+)

On Mon, Mar 13, 2017 at 2:41 PM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This allows device drivers to request resizing their BARs.
>
> The function only tries to reprogram the windows of the bridge directly above
> the requesting device and only the BAR of the same type (usually mem, 64bit,
> prefetchable). This is done to make sure not to disturb other drivers by
> changing the BARs of their devices.
>
> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
> returned to the calling device driver.

Some style comments.

> v2: rebase on changes in rbar support

This kind of comments usually goes after --- delimiter below.

> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---

...here.

> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> +{
> +       const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> +               IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
> +
> +       struct resource saved;
> +       LIST_HEAD(add_list);
> +       LIST_HEAD(fail_head);

> +       struct pci_dev_resource *fail_res;

Perhaps move before definition of 'saved'?

> +       unsigned i;
> +       int ret = 0;
> +
> +       /* Release all children from the matching bridge resource */
> +       for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; ++i) {
> +               struct resource *res = &bridge->resource[i];
> +


> +               if ((res->flags & type_mask) != (type & type_mask))

IIUC it would be
if ((res->flags ^ type) & type_mask)

(I consider 'diff' as XOR operation is more understandable, but it's up to you)

> +                       continue;

> +       /* restore size and flags */
> +       list_for_each_entry(fail_res, &fail_head, list) {
> +               struct resource *res = fail_res->res;
> +
> +               res->start = fail_res->start;
> +               res->end = fail_res->end;
> +               res->flags = fail_res->flags;
> +       }
> +
> +       /* Revert to the old configuration */
> +       if (!list_empty(&fail_head)) {
> +               struct resource *res = &bridge->resource[i];
> +

> +               res->start = saved.start;
> +               res->end = saved.end;
> +               res->flags = saved.flags;

Would
 *res = saved;
work?

> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> +       struct resource *res = dev->resource + resno;
> +       u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
> +       int old = pci_rbar_get_current_size(dev, resno);
> +       u64 bytes = 1ULL << (size + 20);
> +       int ret = 0;
> +

I would put
 sizes = pci_rbar_get_possible_sizes(dev, resno);
here

> +       if (!sizes)
> +               return -ENOTSUPP;
> +
> +       if (!(sizes & (1 << size)))

BIT(size) ?

> +               return -EINVAL;
> +

and
 old = pci_rbar_get_current_size(dev, resno);
here

> +       if (old < 0)
> +               return old;

> +error_resize:
> +       pci_rbar_set_size(dev, resno, old);
> +       res->end = res->start + (1ULL << (old + 20)) - 1;

BIT_ULL(old + 20) ?

Hi Christian,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/PCI-add-resizeable-BAR-infrastructure-v3/20170314-163334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-x077-201711 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/pci/setup-res.c: In function 'pci_resize_resource':
>> drivers/pci/setup-res.c:422:2: warning: ignoring return value of 'pci_reenable_device', declared with attribute warn_unused_result [-Wunused-result]
     pci_reenable_device(dev->bus->self);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/setup-res.c:432:2: warning: ignoring return value of 'pci_reenable_device', declared with attribute warn_unused_result [-Wunused-result]
     pci_reenable_device(dev->bus->self);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/pci_reenable_device +422 drivers/pci/setup-res.c

   406			res->end = resource_size(res) - 1;
   407			res->start = 0;
   408			if (resno < PCI_BRIDGE_RESOURCES)
   409				pci_update_resource(dev, resno);
   410		}
   411	
   412		ret = pci_rbar_set_size(dev, resno, size);
   413		if (ret)
   414			goto error_reassign;
   415	
   416		res->end = res->start + bytes - 1;
   417	
   418		ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
   419		if (ret)
   420			goto error_resize;
   421	
 > 422		pci_reenable_device(dev->bus->self);
   423		return 0;
   424	
   425	error_resize:
   426		pci_rbar_set_size(dev, resno, old);
   427		res->end = res->start + (1ULL << (old + 20)) - 1;
   428	
   429	error_reassign:
   430		pci_assign_unassigned_bus_resources(dev->bus);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On Mon, Mar 13, 2017 at 01:41:34PM +0100, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> This allows device drivers to request resizing their BARs.
> 
> The function only tries to reprogram the windows of the bridge directly above
> the requesting device and only the BAR of the same type (usually mem, 64bit,
> prefetchable). This is done to make sure not to disturb other drivers by
> changing the BARs of their devices.
> 
> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
> returned to the calling device driver.
> 
> v2: rebase on changes in rbar support
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/pci/setup-bus.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/setup-res.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |  2 ++
>  3 files changed, 114 insertions(+)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index f30ca75..cfab2c7 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1923,6 +1923,67 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  }
>  EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
>  
> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> +{
> +	const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> +		IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
> +
> +	struct resource saved;
> +	LIST_HEAD(add_list);
> +	LIST_HEAD(fail_head);
> +	struct pci_dev_resource *fail_res;
> +	unsigned i;
> +	int ret = 0;
> +
> +	/* Release all children from the matching bridge resource */
> +	for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; ++i) {

Nit: use post-increment unless you need pre-increment.

> +		struct resource *res = &bridge->resource[i];
> +
> +		if ((res->flags & type_mask) != (type & type_mask))
> +			continue;
> +
> +		saved = *res;
> +		if (res->parent) {
> +			release_child_resources(res);

Doesn't this recursively release *all* child resources?  There could
be BARs from several devices, or even windows of downstream bridges,
inside this window.  The drivers of those other devices aren't
expecting things to change here.

> +			release_resource(res);
> +		}
> +		res->start = 0;
> +		res->end = 0;
> +		break;
> +	}
> +
> +	if (i == PCI_BRIDGE_RESOURCE_END)
> +		return -ENOENT;
> +
> +	__pci_bus_size_bridges(bridge->subordinate, &add_list);
> +	__pci_bridge_assign_resources(bridge, &add_list, &fail_head);
> +	BUG_ON(!list_empty(&add_list));
> +
> +	/* restore size and flags */
> +	list_for_each_entry(fail_res, &fail_head, list) {
> +		struct resource *res = fail_res->res;
> +
> +		res->start = fail_res->start;
> +		res->end = fail_res->end;
> +		res->flags = fail_res->flags;
> +	}
> +
> +	/* Revert to the old configuration */
> +	if (!list_empty(&fail_head)) {
> +		struct resource *res = &bridge->resource[i];
> +
> +		res->start = saved.start;
> +		res->end = saved.end;
> +		res->flags = saved.flags;
> +
> +		pci_claim_resource(bridge, i);
> +		ret = -ENOSPC;
> +	}
> +
> +	free_list(&fail_head);
> +	return ret;
> +}
> +
>  void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 9526e34..3bb1e29 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -363,6 +363,57 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsiz
>  	return 0;
>  }
>  
> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> +	struct resource *res = dev->resource + resno;
> +	u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
> +	int old = pci_rbar_get_current_size(dev, resno);
> +	u64 bytes = 1ULL << (size + 20);
> +	int ret = 0;

I think we should fail the request if the device is enabled, i.e., if
the PCI_COMMAND_MEMORY bit is set.  We can't safely change the BAR
while memory decoding is enabled.

I know there's code in pci_std_update_resource() that turns off
PCI_COMMAND_MEMORY, but I think that's a mistake: I think it should
fail when asked to update an enabled BAR the same way
pci_iov_update_resource() does.

I'm not sure why you call pci_reenable_device() below, but I think I
would rather have the driver do something like this:

  pci_disable_device(dev);
  pci_resize_resource(dev, 0, size);
  pci_enable_device(dev);

That way it's very clear to the driver that it must re-read all BARs
after resizing this one.

> +	if (!sizes)
> +		return -ENOTSUPP;
> +
> +	if (!(sizes & (1 << size)))
> +		return -EINVAL;
> +
> +	if (old < 0)
> +		return old;
> +
> +	/* Make sure the resource isn't assigned before making it larger. */
> +	if (resource_size(res) < bytes && res->parent) {
> +		release_resource(res);
> +		res->end = resource_size(res) - 1;
> +		res->start = 0;
> +		if (resno < PCI_BRIDGE_RESOURCES)
> +			pci_update_resource(dev, resno);

Why do we need to write this zero to the BAR?  If PCI_COMMAND_MEMORY
is set, I think this is dangerous, and if it's not set, I think it's
unnecessary.

I think we should set the IORESOURCE_UNSET bit to indicate that the
resource does not have an address assigned to it.  It should get
cleared later anyway, but having IORESOURCE_UNSET will make any debug
messages emitted while reassigning resources make more sense.

> +	}
> +
> +	ret = pci_rbar_set_size(dev, resno, size);
> +	if (ret)
> +		goto error_reassign;
> +
> +	res->end = res->start + bytes - 1;
> +
> +	ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
> +	if (ret)
> +		goto error_resize;
> +
> +	pci_reenable_device(dev->bus->self);

> +	return 0;
> +
> +error_resize:
> +	pci_rbar_set_size(dev, resno, old);
> +	res->end = res->start + (1ULL << (old + 20)) - 1;
> +
> +error_reassign:
> +	pci_assign_unassigned_bus_resources(dev->bus);
> +	pci_setup_bridge(dev->bus);

Could this bridge-related recovery code go inside
pci_reassign_bridge_resources()?

> +	pci_reenable_device(dev->bus->self);
> +	return ret;
> +}
> +EXPORT_SYMBOL(pci_resize_resource);
> +
>  int pci_enable_resources(struct pci_dev *dev, int mask)
>  {
>  	u16 cmd, old_cmd;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6954e50..8eaebb4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1055,6 +1055,7 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
>  void pci_update_resource(struct pci_dev *dev, int resno);
>  int __must_check pci_assign_resource(struct pci_dev *dev, int i);
>  int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
> +int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
>  int pci_select_bars(struct pci_dev *dev, unsigned long flags);
>  bool pci_device_is_present(struct pci_dev *pdev);
>  void pci_ignore_hotplug(struct pci_dev *dev);
> @@ -1135,6 +1136,7 @@ void pci_assign_unassigned_resources(void);
>  void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge);
>  void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
>  void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus);
> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type);
>  void pdev_enable_device(struct pci_dev *);
>  int pci_enable_resources(struct pci_dev *, int mask);
>  void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
> -- 
> 2.7.4
>

Sorry for the delayed response, have been busy with other stuff recently.

Am 13.03.2017 um 17:43 schrieb Andy Shevchenko:
>> v2: rebase on changes in rbar support
> This kind of comments usually goes after --- delimiter below.

That would remove it from the commit message which I don't want.


>> +       unsigned i;
>> +       int ret = 0;
>> +
>> +       /* Release all children from the matching bridge resource */
>> +       for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; ++i) {
>> +               struct resource *res = &bridge->resource[i];
>> +
>
>> +               if ((res->flags & type_mask) != (type & type_mask))
> IIUC it would be
> if ((res->flags ^ type) & type_mask)
>
> (I consider 'diff' as XOR operation is more understandable, but it's up to you)

I think like it is is easier to read.


>> +               res->start = saved.start;
>> +               res->end = saved.end;
>> +               res->flags = saved.flags;
> Would
>   *res = saved;
> work?

No, res also contains a bunch of pointers into the tree which we should 
not override.

>> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>> +{
>> +       struct resource *res = dev->resource + resno;
>> +       u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
>> +       int old = pci_rbar_get_current_size(dev, resno);
>> +       u64 bytes = 1ULL << (size + 20);
>> +       int ret = 0;
>> +
> I would put
>   sizes = pci_rbar_get_possible_sizes(dev, resno);
> here

Good idea, done.


>> +       if (!sizes)
>> +               return -ENOTSUPP;
>> +
>> +       if (!(sizes & (1 << size)))
> BIT(size) ?

Done.

> and
>   old = pci_rbar_get_current_size(dev, resno);
> here

Good idea as well.

>> +error_resize:
>> +       pci_rbar_set_size(dev, resno, old);
>> +       res->end = res->start + (1ULL << (old + 20)) - 1;
> BIT_ULL(old + 20) ?

Nope, that is actually a size in bytes. Not a bitfield. So while BIT_ULL 
yields the right result it would be harder to understand.

Christian.

Hi Bjorn,

first of all sorry for the delay, had been busy with other stuff in the 
last few weeks.

Am 24.03.2017 um 22:34 schrieb Bjorn Helgaas:
>> +			release_child_resources(res);
> Doesn't this recursively release *all* child resources?  There could
> be BARs from several devices, or even windows of downstream bridges,
> inside this window.  The drivers of those other devices aren't
> expecting things to change here.

Yes, the original idea was that the driver calling this knows that the 
BARs will be changed for the bridge it belongs to.

But you're right. I've changed it in the way that our device driver will 
release all resource first and then call the function to resize its BAR.

The function will return an error in the next version of the patch if 
the bridge BAR which needs to be moved for this is still used by child 
resources.

>> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>> +{
>> +	struct resource *res = dev->resource + resno;
>> +	u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
>> +	int old = pci_rbar_get_current_size(dev, resno);
>> +	u64 bytes = 1ULL << (size + 20);
>> +	int ret = 0;
> I think we should fail the request if the device is enabled, i.e., if
> the PCI_COMMAND_MEMORY bit is set.  We can't safely change the BAR
> while memory decoding is enabled.
>
> I know there's code in pci_std_update_resource() that turns off
> PCI_COMMAND_MEMORY, but I think that's a mistake: I think it should
> fail when asked to update an enabled BAR the same way
> pci_iov_update_resource() does.
>
> I'm not sure why you call pci_reenable_device() below, but I think I
> would rather have the driver do something like this:
>
>    pci_disable_device(dev);
>    pci_resize_resource(dev, 0, size);
>    pci_enable_device(dev);
>
> That way it's very clear to the driver that it must re-read all BARs
> after resizing this one.

I've tried it, but this actually doesn't seem to work.

At least on the board I've tried pci_disable_device() doesn't clear the 
PCI_COMMAND_MEMORY bit, it just clears the master bit.

Additional to that the power management reference counting 
pci_disable_device() and pci_enable_device() doesn't look like what I 
want for this functionality.

How about the driver needs to disabled memory decoding itself before 
trying to resize anything?

>> +	if (!sizes)
>> +		return -ENOTSUPP;
>> +
>> +	if (!(sizes & (1 << size)))
>> +		return -EINVAL;
>> +
>> +	if (old < 0)
>> +		return old;
>> +
>> +	/* Make sure the resource isn't assigned before making it larger. */
>> +	if (resource_size(res) < bytes && res->parent) {
>> +		release_resource(res);
>> +		res->end = resource_size(res) - 1;
>> +		res->start = 0;
>> +		if (resno < PCI_BRIDGE_RESOURCES)
>> +			pci_update_resource(dev, resno);
> Why do we need to write this zero to the BAR?  If PCI_COMMAND_MEMORY
> is set, I think this is dangerous, and if it's not set, I think it's
> unnecessary.
>
> I think we should set the IORESOURCE_UNSET bit to indicate that the
> resource does not have an address assigned to it.  It should get
> cleared later anyway, but having IORESOURCE_UNSET will make any debug
> messages emitted while reassigning resources make more sense.

Makes sense, changed.

>> +	return 0;
>> +
>> +error_resize:
>> +	pci_rbar_set_size(dev, resno, old);
>> +	res->end = res->start + (1ULL << (old + 20)) - 1;
>> +
>> +error_reassign:
>> +	pci_assign_unassigned_bus_resources(dev->bus);
>> +	pci_setup_bridge(dev->bus);
> Could this bridge-related recovery code go inside
> pci_reassign_bridge_resources()?

No, we need to restore the original size of the resource before trying 
the recovery code when something goes wrong.

I will address all other comments in the next version of the patch as well.

Regards,
Christian.

On Tue, Apr 11, 2017 at 05:37:04PM +0200, Christian König wrote:

> >>+int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >>+{
> >>+	struct resource *res = dev->resource + resno;
> >>+	u32 sizes = pci_rbar_get_possible_sizes(dev, resno);
> >>+	int old = pci_rbar_get_current_size(dev, resno);
> >>+	u64 bytes = 1ULL << (size + 20);
> >>+	int ret = 0;
> >I think we should fail the request if the device is enabled, i.e., if
> >the PCI_COMMAND_MEMORY bit is set.  We can't safely change the BAR
> >while memory decoding is enabled.
> >
> >I know there's code in pci_std_update_resource() that turns off
> >PCI_COMMAND_MEMORY, but I think that's a mistake: I think it should
> >fail when asked to update an enabled BAR the same way
> >pci_iov_update_resource() does.
> >
> >I'm not sure why you call pci_reenable_device() below, but I think I
> >would rather have the driver do something like this:
> >
> >   pci_disable_device(dev);
> >   pci_resize_resource(dev, 0, size);
> >   pci_enable_device(dev);
> >
> >That way it's very clear to the driver that it must re-read all BARs
> >after resizing this one.
> 
> I've tried it, but this actually doesn't seem to work.
> 
> At least on the board I've tried pci_disable_device() doesn't clear
> the PCI_COMMAND_MEMORY bit, it just clears the master bit.

Yeah, you're right.  We generally turn *on* PCI_COMMAND_MEMORY in the
pci_enable_device() path, but the pci_disable_device() path doesn't
turn it off.

> Additional to that the power management reference counting
> pci_disable_device() and pci_enable_device() doesn't look like what
> I want for this functionality.
> 
> How about the driver needs to disabled memory decoding itself before
> trying to resize anything?

There are a few places that do that, so maybe that's the best option.

Bjorn