linux-kernel - Re: [RFC PATCH 4/6] x86: PCI: preserve IORESOURCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eda06f77-14b3-4503-9a86-c55092ccf5b7@amd.com>
Date: Thu, 11 Jul 2024 14:58:24 -0400
From: Stewart Hildebrand <stewart.hildebrand@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>,
	<x86@...nel.org>, <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 4/6] x86: PCI: preserve IORESOURCE_STARTALIGN
 alignment

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

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.

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