Re: [RFC PATCH 4/6] x86: PCI: preserve IORESOURCE_STARTALIGN alignment

[Stewart Hildebrand <stewart.hildebrand@amd.com>]

On 7/9/24 12:19, Bjorn Helgaas wrote:
> On Tue, Jul 09, 2024 at 09:36:01AM -0400, Stewart Hildebrand wrote:
>> Currently, it's not possible to use the IORESOURCE_STARTALIGN flag on
>> x86 due to the alignment being overwritten in
>> pcibios_allocate_dev_resources(). Make one small change in arch/x86 to
>> make it work on x86.
> 
> Is this a regression?  I didn't look up when IORESOURCE_STARTALIGN was
> added, but likely it was for some situation on x86, so presumably it
> worked at one time.  If something broke it in the meantime, it would
> be nice to identify the commit that broke it.

No, I don't have reason to believe it's a regression.

IORESOURCE_STARTALIGN was introduced in commit 884525655d07 ("PCI: clean
up resource alignment management").

There are some interesting commits mentioning 884525655d07:
5f17cfce5776 ("PCI: fix pbus_size_mem() resource alignment for CardBus
              controllers")
934b7024f0ed ("Fix cardbus resource allocation")

It would seem that 884525655d07 missed updating the bits in
arch/x86/pci/i386.c. I don't think a Fixes: tag is strictly necessary
because I think the issue would only start to trigger once the default
alignment is updated in the last patch of this series.

As far as I can tell, it only breaks in a certain corner case that's not
really possible to trigger yet. The corner case seems to be the
following:
* Only on x86
* The BAR has been set in firmware
* Alignment has been requested via IORESOURCE_STARTALIGN
* The IORESOURCE_UNSET flag is set
* Only PCI_STD_RESOURCES and PCI_IOV_RESOURCES resources (not bridge
  windows)

I think the reason this hasn't been seen until now is that it's not
possible to request IORESOURCE_STARTALIGN alignment via the
pci=resource_alignment= option. That option instead sets
IORESOURCE_SIZEALIGN, and in that case it works fine.

After the last patch in this series that changes the default alignment,
we will be starting to use IORESOURCE_STARTALIGN on all
not-sufficiently-aligned resources, and the corner case would be more
likely (rather, possible at all) to trigger.

My commit message is overstating the severity of the issue. So, how
about I make the commit message less scary:

There is a corner case in pcibios_allocate_dev_resources() that doesn't
account for IORESOURCE_STARTALIGN, in which case the alignment would be
overwritten. As far as I can tell, the corner case is not yet possible
to trigger, but it will be possible once the resource alignment changes.
Account for IORESOURCE_STARTALIGN in preparation for changing the
default resource alignment.

> Nit: follow the subject line conventions for this and the other
> patches.  Learn them with "git log --oneline".  For this patch,
> "x86/PCI: <Capitalized text>" is appropriate.

Thanks for pointing this out, I'll fix

>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> RFC: We don't have enough info in this function to re-calculate the
>>      alignment value in case of IORESOURCE_STARTALIGN. Luckily our
>>      alignment value seems to be intact, so just don't touch it...
>>      Alternatively, we could call pci_reassigndev_resource_alignment()
>>      after the loop. Would that be preferable?

Any comments on this? After some more thought, I think calling
pci_reassigndev_resource_alignment() after the loop is probably more
correct, so if there aren't any comments, I'll plan on changing it.

>> ---
>>  arch/x86/pci/i386.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
>> index f2f4a5d50b27..ff6e61389ec7 100644
>> --- a/arch/x86/pci/i386.c
>> +++ b/arch/x86/pci/i386.c
>> @@ -283,8 +283,11 @@ static void pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
>>  						/* We'll assign a new address later */
>>  						pcibios_save_fw_addr(dev,
>>  								idx, r->start);
>> -						r->end -= r->start;
>> -						r->start = 0;
>> +						if (!(r->flags &
>> +						      IORESOURCE_STARTALIGN)) {
>> +							r->end -= r->start;
>> +							r->start = 0;
>> +						}
>>  					}
>>  				}
>>  			}
>> -- 
>> 2.45.2
>>