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@xxxxxxx>
>> ---
>> 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
>>
