[+cc Yongji Xie]
On Thu, Jul 11, 2024 at 02:58:24PM -0400, Stewart Hildebrand wrote:
> On 7/11/24 14:40, Bjorn Helgaas wrote:
> > 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.
> ...
> >>> 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?
>
> Yep
>
> > It looks like that would force at
> > least 4K alignment independent of IORESOURCE_STARTALIGN.
>
> Changing pcibios_default_alignment() (without pci=resource_alignment=
> specified) results in IORESOURCE_STARTALIGN.
Mmmm. I guess it's this path:
pci_device_add
pci_reassigndev_resource_alignment
align = pci_specified_resource_alignment(&resize)
pcibios_default_alignment
for (i = 0; i <= PCI_ROM_RESOURCE; i++)
pci_request_resource_alignment(i, align, resize)
if (!resize)
r->flags |= IORESOURCE_STARTALIGN
where "resize" is false because the device wasn't mentioned in a
"pci=resource_alignment=..." parameter, so
pci_request_resource_alignment() sets IORESOURCE_STARTALIGN.
When 0a701aa63784 ("PCI: Add pcibios_default_alignment() for
arch-specific alignment control") added pcibios_default_alignment(),
we got a way to do arch-specific alignment, but if the alignment is
non-zero, the implementation *also* applies that alignment to ALL
devices in the system.
Prior to 0a701aa63784, I think pci_specified_resource_alignment()
only caused increased alignment for devices mentioned with a
"pci=resource_alignment=..." parameter.
I suppose the change to do it for all devices was intentional because
382746376993 ("powerpc/powernv: Override pcibios_default_alignment()
to force PCI devices to be page aligned") says it's for all PCI
devices on PowerNV.
Since 0a701aa63784 and 382746376993 were for VFIO, which is generic, I
kind of wish that we'd done it in a more generic way instead of making
a pcibios interface that is only implemented for PowerNV.
This series does make it generic by doing it in the weak
pcibios_default_alignment() that's used by default, so that's good.
It's ancient history now, but I'm also a little unsure about the way
pci_reassigndev_resource_alignment() is kind of tacked on at the end
in pci_device_add() and not integrated with the usual BAR sizing and
allocation machinery.
> >>> 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.
Thanks for working on this.
Bjorn
