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

[Bjorn Helgaas]

On Wed, Jul 10, 2024 at 12:16:24PM -0400, Stewart Hildebrand wrote:
> 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").

Ah, OK.  IORESOURCE_STARTALIGN is used for bridge windows, which don't
need to be aligned on their size as BARs do.  It would be terrible if
that usage was broken, which is why I was alarmed by the idea of it
not working on x86.

But this patch is only relevant for BARs.  I was a little confused
about IORESOURCE_STARTALIGN for a BAR, but I guess the point is to
force alignment on *more* than the BAR's size, e.g., to prevent
multiple BARs from being put in the same page.

Bottom line, this would need to be a little more specific so it
doesn't suggest that IORESOURCE_STARTALIGN for windows is broken.

IIUC, the main purpose of the series is to align all BARs to at least
4K.  I don't think the series relies on IORESOURCE_STARTALIGN to do
that.  But there's an issue with "pci=resource_alignment=..." that you
noticed sort of incidentally, and this patch fixes that?  If so, it's
important to mention that parameter.

> >> 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.

Sounds like this might be a separate patch unless it logically has to
be part of this one to avoid an issue.

> >> ---
> >>  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;
> >> +						}

I wondered why this only touched x86 and whether other arches need a
similar change.  This is used in two paths:

  1) pcibios_resource_survey_bus(), which is only implemented by x86

  2) pcibios_resource_survey(), which is implemented by x86 and
  powerpc.  The powerpc pcibios_allocate_resources() is similar to the
  x86 pcibios_allocate_dev_resources(), but powerpc doesn't have the
  r->end and r->start updates you're making conditional.

So it looks like x86 is indeed the only place that needs this change.
None of this stuff is arch-specific, so it's a shame that we don't
have generic code for it all.  Sigh, oh well.

> >>  					}
> >>  				}
> >>  			}
> >> -- 
> >> 2.45.2
> >>
>