Re: [PATCH v3 5/8] x86/PCI: Preserve IORESOURCE_STARTALIGN alignment

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Aug 07, 2024 at 11:17:14AM -0400, Stewart Hildebrand wrote:
> There is a corner case in pcibios_allocate_dev_resources() where the
> IORESOURCE_STARTALIGN alignment of memory BAR resources gets
> overwritten. This does not affect bridge resources. The corner case is
> not yet possible to trigger on x86, but it will be possible once the
> default resource alignment changes, and memory BAR resources will start
> to use IORESOURCE_STARTALIGN. 

I see from [8/8] that "Changing pcibios_default_alignment() results in
the second method of alignment with IORESOURCE_STARTALIGN", but that
connection is not at all obvious, and there's no patch in this series
that sets IORESOURCE_STARTALIGN, so it's kind of hard to connect all
the dots here.

The only caller of pcibios_default_alignment() is
pci_specified_resource_alignment(), and that doesn't mention
IORESOURCE_STARTALIGN either.

Neither does pcibios_allocate_dev_resources().

I feel like we've had this conversation before; apologies if so.  But
we need to figure out to make this more explicit and less magic.

> Account for IORESOURCE_STARTALIGN in
> preparation for changing the default resource alignment.
> 
> Skip the pcibios_save_fw_addr() call since the resource doesn't contain
> a valid address when alignment has been requested. The impact of this is
> that we won't be able to restore the firmware allocated BAR, which does
> not meet alignment requirements anyway.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v2->v3:
> * no change
> 
> v1->v2:
> * capitalize subject text
> * clarify commit message
> * skip pcibios_save_fw_addr() call
> ---
>  arch/x86/pci/i386.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> index 3abd55902dbc..13d7f7ac3bde 100644
> --- a/arch/x86/pci/i386.c
> +++ b/arch/x86/pci/i386.c
> @@ -256,7 +256,7 @@ static void alloc_resource(struct pci_dev *dev, int idx, int pass)
>  		if (r->flags & IORESOURCE_PCI_FIXED) {
>  			dev_info(&dev->dev, "BAR %d %pR is immovable\n",
>  				 idx, r);
> -		} else {
> +		} else if (!(r->flags & IORESOURCE_STARTALIGN)) {
>  			/* We'll assign a new address later */
>  			pcibios_save_fw_addr(dev, idx, r->start);
>  			r->end -= r->start;
> -- 
> 2.46.0
>