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