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

[Bjorn Helgaas]

On Wed, Jul 10, 2024 at 06:49:42PM -0400, Stewart Hildebrand wrote:
> On 7/10/24 17:24, Bjorn Helgaas wrote:
> > 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.
> 
> I'll make the commit message clearer.
> 
> > 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.
> 
> Yes, it does rely on IORESOURCE_STARTALIGN for BARs.

Oh, I missed that, sorry.  The only places I see that set
IORESOURCE_STARTALIGN are pci_request_resource_alignment(), which is
where I got the "pci=resource_alignment=..." connection, and
pbus_size_io(), pbus_size_mem(), and pci_bus_size_cardbus(), which are
for bridge windows, AFAICS.

Doesn't the >= 4K alignment in this series hinge on the
pcibios_default_alignment() change?  It looks like that would force at
least 4K alignment independent of IORESOURCE_STARTALIGN.

> > But there's an issue with "pci=resource_alignment=..." that you
> > noticed sort of incidentally, and this patch fixes that?
> 
> No, pci=resource_alignment= results in IORESOURCE_SIZEALIGN, which
> breaks pcitest. And we'd like pcitest to work properly for PCI
> passthrough validation with Xen, hence the need for
> IORESOURCE_STARTALIGN.