Re: [PATCH v10 4/4] PCI: Don't extend device's size when using default alignment for all devices

[Bjorn Helgaas]

On Mon, Apr 17, 2017 at 1:27 AM, Yongji Xie  wrote:
> 2017-04-15 6:54 GMT+08:00 Bjorn Helgaas :
>>
>> On Mon, Apr 10, 2017 at 07:58:14PM +0800, Yongji Xie wrote:
>> > Currently we reassign the alignment by extending resources' size in
>> > pci_reassigndev_resource_alignment(). This could potentially break
>> > some drivers when the driver uses the size to locate register
>> > whose length is related to the size. Some examples as below:
>> >
>> > - misc/Hpilo.c:
>>
>> If you're going to quote filenames, they need to match the actual
>> files in the tree.  In this case, "hpilo.c", not "Hpilo.c".  Also
>> "Nes_hw.c" below is incorrect.
>>
>
> Will fix it.
>
>>
>> > off = pci_resource_len(pdev, bar) - 0x2000;
>> >
>> > - net/ethernet/chelsio/cxgb4/cxgb4_uld.h:
>> > (pci_resource_len((pdev), 2) - roundup_pow_of_two((vres)->ocq.size))
>> >
>> > - infiniband/hw/nes/Nes_hw.c:
>> > num_pds = pci_resource_len(nesdev->pcidev, BAR_1) >> PAGE_SHIFT;
>>
>> This BAR is apparently at least a page in size already, so this only
>> breaks if we request alignment of more than a page size.
>>
>
> Oh, yes. I'll remove this one.
>
>> > This risk could be easily prevented before because we only had one way
>> > (kernel parameter resource_alignment) to touch those codes. And even
>> > some users may be happy to see the extended size.
>>
>> Currently, pci_reassigndev_resource_alignment() extends the size of
>> some resources when we use "pci=resource_alignment=..."  That
>> certainly breaks places like the ones above.
>>
>> What do you mean by "some users may be happy to see the extended
>> size"?  We're increasing a resource size to something larger than what
>> the BAR actually responds to.  I don't see how that can ever be an
>> *improvement*.  I can see how most drivers won't care, and a few (like
>> the ones you cite above) will break.  But I don't see how any driver
>> could be *helped* by us making the resource larger.
>>
>
> From commit 32a9a68 (PCI: allow assignment of memory resources with a
> specified alignment), it seems that "pci=resource_alignment" was introduced
> because we want to use PCI pass-through for some page-unaligned devices.
>
> So there might be some cases that users use "pci=resource_alignment" to
> extend the BAR's size then passthrough them. That's why I say "some users
> may be happy to see the extended size". We are not able to passthrough
> the device unless we extend its BARs to PAGE_SIZE.
>
>>
>> > But now we introduce pcibios_default_alignment() to set default
>> > alignment
>> > for all PCI devices which would also touch those codes. It would be hard
>> > to prevent the risk in this case. So this patch tries to use
>> > START_ALIGNMENT to identify the resource's alignment without extending
>> > the size when the alignment reassigning is caused by the default
>> > alignment.
>> >
>> > Signed-off-by: Yongji Xie 
>> > ---
>> >  drivers/pci/pci.c |   34 --
>> >  1 file changed, 24 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> > index 02f1255..358366e 100644
>> > --- a/drivers/pci/pci.c
>> > +++ b/drivers/pci/pci.c
>> > @@ -4959,11 +4959,13 @@ resource_size_t __weak
>> > pcibios_default_alignment(struct pci_dev *dev)
>> >  /**
>> >   * pci_specified_resource_alignment - get resource alignment specified
>> > by user.
>> >   * @dev: the PCI device to get
>> > + * @resize: whether or not to change resources' size when reassigning
>> > alignment
>> >   *
>> >   * RETURNS: Resource alignment if it is specified.
>> >   *  Zero if it is not specified.
>> >   */
>> > -static resource_size_t pci_specified_resource_alignment(struct pci_dev
>> > *dev)
>> > +static resource_size_t pci_specified_resource_alignment(struct pci_dev
>> > *dev,
>> > + bool *resize)
>> >  {
>> >   int seg, bus, slot, func, align_order, count;
>> >   unsigned short vendor, device, subsystem_vendor, subsystem_device;
>> > @@ -5005,6 +5007,7 @@ static resource_size_t
>> > pci_specified_resource_alignment(struct pci_dev *dev)
>> >   (!device || (device == dev->device)) &&
>> >   (!subsystem_vendor || (subsystem_vendor ==
>> > dev->subsystem_vendor)) &&
>> >   (!subsystem_device || (subsystem_device ==
>> > dev->subsystem_device))) {
>> > + *resize = true;
>> >   if (align_order == -1)
>> >   align = PAGE_SIZE;
>> >   else
>> > @@ -5030,6 +5033,7 @@ static resource_size_t
>> > pci_specified_resource_alignment(struct pci_dev *dev)
>> >   bus == dev->bus->number &&
>> >   slot == PCI_SLOT(dev->devfn) &&
>> >   func == PCI_FUNC(dev->devfn)) {
>> > +  

Re: [PATCH v10 4/4] PCI: Don't extend device's size when using default alignment for all devices

[Yongji Xie]

2017-04-15 6:54 GMT+08:00 Bjorn Helgaas :

> On Mon, Apr 10, 2017 at 07:58:14PM +0800, Yongji Xie wrote:
> > Currently we reassign the alignment by extending resources' size in
> > pci_reassigndev_resource_alignment(). This could potentially break
> > some drivers when the driver uses the size to locate register
> > whose length is related to the size. Some examples as below:
> >
> > - misc/Hpilo.c:
>
> If you're going to quote filenames, they need to match the actual
> files in the tree.  In this case, "hpilo.c", not "Hpilo.c".  Also
> "Nes_hw.c" below is incorrect.
>
>
Will fix it.


> > off = pci_resource_len(pdev, bar) - 0x2000;
> >
> > - net/ethernet/chelsio/cxgb4/cxgb4_uld.h:
> > (pci_resource_len((pdev), 2) - roundup_pow_of_two((vres)->ocq.size))
> >
> > - infiniband/hw/nes/Nes_hw.c:
> > num_pds = pci_resource_len(nesdev->pcidev, BAR_1) >> PAGE_SHIFT;
>
> This BAR is apparently at least a page in size already, so this only
> breaks if we request alignment of more than a page size.
>
>
Oh, yes. I'll remove this one.

> This risk could be easily prevented before because we only had one way
> > (kernel parameter resource_alignment) to touch those codes. And even
> > some users may be happy to see the extended size.
>
> Currently, pci_reassigndev_resource_alignment() extends the size of
> some resources when we use "pci=resource_alignment=..."  That
> certainly breaks places like the ones above.
>
> What do you mean by "some users may be happy to see the extended
> size"?  We're increasing a resource size to something larger than what
> the BAR actually responds to.  I don't see how that can ever be an
> *improvement*.  I can see how most drivers won't care, and a few (like
> the ones you cite above) will break.  But I don't see how any driver
> could be *helped* by us making the resource larger.
>
>
>From commit 32a9a68 (PCI: allow assignment of memory resources with a
specified alignment), it seems that "pci=resource_alignment" was introduced
because we want to use PCI pass-through for some page-unaligned devices.

So there might be some cases that users use "pci=resource_alignment" to
extend the BAR's size then passthrough them. That's why I say "some users
may be happy to see the extended size". We are not able to passthrough
the device unless we extend its BARs to PAGE_SIZE.


> > But now we introduce pcibios_default_alignment() to set default alignment
> > for all PCI devices which would also touch those codes. It would be hard
> > to prevent the risk in this case. So this patch tries to use
> > START_ALIGNMENT to identify the resource's alignment without extending
> > the size when the alignment reassigning is caused by the default
> alignment.
> >
> > Signed-off-by: Yongji Xie 
> > ---
> >  drivers/pci/pci.c |   34 --
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 02f1255..358366e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4959,11 +4959,13 @@ resource_size_t __weak
> pcibios_default_alignment(struct pci_dev *dev)
> >  /**
> >   * pci_specified_resource_alignment - get resource alignment specified
> by user.
> >   * @dev: the PCI device to get
> > + * @resize: whether or not to change resources' size when reassigning
> alignment
> >   *
> >   * RETURNS: Resource alignment if it is specified.
> >   *  Zero if it is not specified.
> >   */
> > -static resource_size_t pci_specified_resource_alignment(struct pci_dev
> *dev)
> > +static resource_size_t pci_specified_resource_alignment(struct pci_dev
> *dev,
> > + bool *resize)
> >  {
> >   int seg, bus, slot, func, align_order, count;
> >   unsigned short vendor, device, subsystem_vendor, subsystem_device;
> > @@ -5005,6 +5007,7 @@ static resource_size_t
> pci_specified_resource_alignment(struct pci_dev *dev)
> >   (!device || (device == dev->device)) &&
> >   (!subsystem_vendor || (subsystem_vendor ==
> dev->subsystem_vendor)) &&
> >   (!subsystem_device || (subsystem_device ==
> dev->subsystem_device))) {
> > + *resize = true;
> >   if (align_order == -1)
> >   align = PAGE_SIZE;
> >   else
> > @@ -5030,6 +5033,7 @@ static resource_size_t
> pci_specified_resource_alignment(struct pci_dev *dev)
> >   bus == dev->bus->number &&
> >   slot == PCI_SLOT(dev->devfn) &&
> >   func == PCI_FUNC(dev->devfn)) {
> > + *resize = true;
> >   if (align_order == -1)
> >   align = PAGE_SIZE;
> >   else
> > @@ -5062,6 +5066,7 

Re: [PATCH v10 4/4] PCI: Don't extend device's size when using default alignment for all devices

[Bjorn Helgaas]

On Mon, Apr 10, 2017 at 07:58:14PM +0800, Yongji Xie wrote:
> Currently we reassign the alignment by extending resources' size in
> pci_reassigndev_resource_alignment(). This could potentially break
> some drivers when the driver uses the size to locate register
> whose length is related to the size. Some examples as below:
> 
> - misc/Hpilo.c:

If you're going to quote filenames, they need to match the actual
files in the tree.  In this case, "hpilo.c", not "Hpilo.c".  Also
"Nes_hw.c" below is incorrect.

> off = pci_resource_len(pdev, bar) - 0x2000;
> 
> - net/ethernet/chelsio/cxgb4/cxgb4_uld.h:
> (pci_resource_len((pdev), 2) - roundup_pow_of_two((vres)->ocq.size))
> 
> - infiniband/hw/nes/Nes_hw.c:
> num_pds = pci_resource_len(nesdev->pcidev, BAR_1) >> PAGE_SHIFT;

This BAR is apparently at least a page in size already, so this only
breaks if we request alignment of more than a page size.

> This risk could be easily prevented before because we only had one way
> (kernel parameter resource_alignment) to touch those codes. And even
> some users may be happy to see the extended size.

Currently, pci_reassigndev_resource_alignment() extends the size of
some resources when we use "pci=resource_alignment=..."  That
certainly breaks places like the ones above.

What do you mean by "some users may be happy to see the extended
size"?  We're increasing a resource size to something larger than what
the BAR actually responds to.  I don't see how that can ever be an
*improvement*.  I can see how most drivers won't care, and a few (like
the ones you cite above) will break.  But I don't see how any driver
could be *helped* by us making the resource larger.

> But now we introduce pcibios_default_alignment() to set default alignment
> for all PCI devices which would also touch those codes. It would be hard
> to prevent the risk in this case. So this patch tries to use
> START_ALIGNMENT to identify the resource's alignment without extending
> the size when the alignment reassigning is caused by the default alignment.
> 
> Signed-off-by: Yongji Xie 
> ---
>  drivers/pci/pci.c |   34 --
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 02f1255..358366e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4959,11 +4959,13 @@ resource_size_t __weak 
> pcibios_default_alignment(struct pci_dev *dev)
>  /**
>   * pci_specified_resource_alignment - get resource alignment specified by 
> user.
>   * @dev: the PCI device to get
> + * @resize: whether or not to change resources' size when reassigning 
> alignment
>   *
>   * RETURNS: Resource alignment if it is specified.
>   *  Zero if it is not specified.
>   */
> -static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
> +static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> + bool *resize)
>  {
>   int seg, bus, slot, func, align_order, count;
>   unsigned short vendor, device, subsystem_vendor, subsystem_device;
> @@ -5005,6 +5007,7 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev)
>   (!device || (device == dev->device)) &&
>   (!subsystem_vendor || (subsystem_vendor == 
> dev->subsystem_vendor)) &&
>   (!subsystem_device || (subsystem_device == 
> dev->subsystem_device))) {
> + *resize = true;
>   if (align_order == -1)
>   align = PAGE_SIZE;
>   else
> @@ -5030,6 +5033,7 @@ static resource_size_t 
> pci_specified_resource_alignment(struct pci_dev *dev)
>   bus == dev->bus->number &&
>   slot == PCI_SLOT(dev->devfn) &&
>   func == PCI_FUNC(dev->devfn)) {
> + *resize = true;
>   if (align_order == -1)
>   align = PAGE_SIZE;
>   else
> @@ -5062,6 +5066,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev 
> *dev)
>   struct resource *r;
>   resource_size_t align, size;
>   u16 command;
> + bool resize = false;
>  
>   /*
>* VF BARs are read-only zero according to SR-IOV spec r1.1, sec
> @@ -5073,7 +5078,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev 
> *dev)
>   return;
>  
>   /* check if specified PCI is target device to reassign */
> - align = pci_specified_resource_alignment(dev);
> + align = pci_specified_resource_alignment(dev, );
>   if (!align)
>   return;
>  
> @@ -5101,15 +5106,24 @@ void pci_reassigndev_resource_alignment(struct 
> pci_dev *dev)
>   }
>  
>   size = 

[PATCH v10 4/4] PCI: Don't extend device's size when using default alignment for all devices

[Yongji Xie]

Currently we reassign the alignment by extending resources' size in
pci_reassigndev_resource_alignment(). This could potentially break
some drivers when the driver uses the size to locate register
whose length is related to the size. Some examples as below:

- misc/Hpilo.c:
off = pci_resource_len(pdev, bar) - 0x2000;

- net/ethernet/chelsio/cxgb4/cxgb4_uld.h:
(pci_resource_len((pdev), 2) - roundup_pow_of_two((vres)->ocq.size))

- infiniband/hw/nes/Nes_hw.c:
num_pds = pci_resource_len(nesdev->pcidev, BAR_1) >> PAGE_SHIFT;

This risk could be easily prevented before because we only had one way
(kernel parameter resource_alignment) to touch those codes. And even
some users may be happy to see the extended size.

But now we introduce pcibios_default_alignment() to set default alignment
for all PCI devices which would also touch those codes. It would be hard
to prevent the risk in this case. So this patch tries to use
START_ALIGNMENT to identify the resource's alignment without extending
the size when the alignment reassigning is caused by the default alignment.

Signed-off-by: Yongji Xie 
---
 drivers/pci/pci.c |   34 --
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 02f1255..358366e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4959,11 +4959,13 @@ resource_size_t __weak pcibios_default_alignment(struct 
pci_dev *dev)
 /**
  * pci_specified_resource_alignment - get resource alignment specified by user.
  * @dev: the PCI device to get
+ * @resize: whether or not to change resources' size when reassigning alignment
  *
  * RETURNS: Resource alignment if it is specified.
  *  Zero if it is not specified.
  */
-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
+static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+   bool *resize)
 {
int seg, bus, slot, func, align_order, count;
unsigned short vendor, device, subsystem_vendor, subsystem_device;
@@ -5005,6 +5007,7 @@ static resource_size_t 
pci_specified_resource_alignment(struct pci_dev *dev)
(!device || (device == dev->device)) &&
(!subsystem_vendor || (subsystem_vendor == 
dev->subsystem_vendor)) &&
(!subsystem_device || (subsystem_device == 
dev->subsystem_device))) {
+   *resize = true;
if (align_order == -1)
align = PAGE_SIZE;
else
@@ -5030,6 +5033,7 @@ static resource_size_t 
pci_specified_resource_alignment(struct pci_dev *dev)
bus == dev->bus->number &&
slot == PCI_SLOT(dev->devfn) &&
func == PCI_FUNC(dev->devfn)) {
+   *resize = true;
if (align_order == -1)
align = PAGE_SIZE;
else
@@ -5062,6 +5066,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev 
*dev)
struct resource *r;
resource_size_t align, size;
u16 command;
+   bool resize = false;
 
/*
 * VF BARs are read-only zero according to SR-IOV spec r1.1, sec
@@ -5073,7 +5078,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev 
*dev)
return;
 
/* check if specified PCI is target device to reassign */
-   align = pci_specified_resource_alignment(dev);
+   align = pci_specified_resource_alignment(dev, );
if (!align)
return;
 
@@ -5101,15 +5106,24 @@ void pci_reassigndev_resource_alignment(struct pci_dev 
*dev)
}
 
size = resource_size(r);
-   if (size < align) {
-   size = align;
-   dev_info(>dev,
-   "Rounding up size of resource #%d to %#llx.\n",
-   i, (unsigned long long)size);
+   if (resize) {
+   if (size < align) {
+   size = align;
+   dev_info(>dev,
+   "Rounding up size of resource #%d to 
%#llx.\n",
+   i, (unsigned long long)size);
+   }
+   r->flags |= IORESOURCE_UNSET;
+   r->start = 0;
+   } else {
+   if (size < align) {
+   r->flags &= ~IORESOURCE_SIZEALIGN;
+   r->flags |= IORESOURCE_STARTALIGN |
+   IORESOURCE_UNSET;
+   r->start = align;
+