| Message ID | 20240807151723.613742-6-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI: Align small BARs | expand |
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 >
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;
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. 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(-)