Re: [PATCH v1 00/12] PCI: Rework shadow ROM handling

[Andy Lutomirski]

On Fri, Mar 11, 2016 at 3:29 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Fri, Mar 11, 2016 at 01:16:09PM -0800, Andy Lutomirski wrote:
>> On Tue, Mar 8, 2016 at 9:45 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>> > On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
>> >> The purpose of this series is to:
>> >>
>> >>   - Fix the "BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment"
>> >>     messages reported by Linus [1], Andy [2], and others.
>> >>
>> >>   - Move arch-specific shadow ROM location knowledge, e.g.,
>> >>     0xC0000-0xDFFFF, from PCI core to arch code.
>> >>
>> >>   - Fix the ia64 and MIPS Loongson 3 oddity of keeping virtual
>> >>     addresses in shadow ROM struct resource (resources should always
>> >>     contain *physical* addresses).
>> >>
>> >>   - Remove now-unused IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY
>> >>     flags.
>> >>
>> >> This series is based on v4.5-rc1, and it's available on my
>> >> pci/resource git branch (along with a couple tiny unrelated patches)
>> >> at [3].
>> >>
>> >> Bjorn
>> >>
>> >>
>> >> [1] http://lkml.kernel.org/r/CA+55aFyVMfTBB0oz_yx8+eQOEJnzGtCsYSj9QuhEpdZ9BHdq5A@xxxxxxxxxxxxxx
>> >> [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw@xxxxxxxxxxxxxx
>> >> [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/resource
>> >>
>> >>
>> >> ---
>> >>
>> >> Bjorn Helgaas (12):
>> >>       PCI: Mark shadow copy of VGA ROM as IORESOURCE_PCI_FIXED
>> >>       PCI: Don't assign or reassign immutable resources
>> >>       PCI: Don't enable/disable ROM BAR if we're using a RAM shadow copy
>> >>       PCI: Set ROM shadow location in arch code, not in PCI core
>> >>       PCI: Clean up pci_map_rom() whitespace
>> >>       ia64/PCI: Use temporary struct resource * to avoid repetition
>> >>       ia64/PCI: Use ioremap() instead of open-coded equivalent
>> >>       ia64/PCI: Keep CPU physical (not virtual) addresses in shadow ROM resource
>> >>       MIPS: Loongson 3: Use temporary struct resource * to avoid repetition
>> >>       MIPS: Loongson 3: Keep CPU physical (not virtual) addresses in shadow ROM resource
>> >>       PCI: Remove unused IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY
>> >>       PCI: Simplify sysfs ROM cleanup
>> >>
>> >>
>> >>  arch/ia64/pci/fixup.c              |   21 +++++++--
>> >>  arch/ia64/sn/kernel/io_acpi_init.c |   22 ++++++----
>> >>  arch/ia64/sn/kernel/io_init.c      |   51 ++++++++--------------
>> >>  arch/mips/pci/fixup-loongson3.c    |   19 +++++---
>> >>  arch/x86/pci/fixup.c               |   21 +++++++--
>> >>  drivers/pci/pci-sysfs.c            |   13 +-----
>> >>  drivers/pci/remove.c               |    1
>> >>  drivers/pci/rom.c                  |   83 +++++++++++-------------------------
>> >>  drivers/pci/setup-res.c            |    6 +++
>> >>  include/linux/ioport.h             |    4 --
>> >>  10 files changed, 111 insertions(+), 130 deletions(-)
>> >
>> > I applied this series to pci/resource for v4.6.
>>
>> This gets rid of all the warnings for me until I try to read my i915
>> device's rom using sysfs.  Then I get:
>>
>> i915 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55,
>> got 0xffff
>>
>> So I suspect that something is still subtly wrong -- I'd imagine that
>> this should either work or the intialization code should detect that
>> there is no usable ROM and not expose it.
>>
>> (To be clear, there's no regression here.)
>
> Hmmm.  Thanks for testing this.  As you say, I think this is the way
> it's always been, but it does seem non-intuitive.
>
> That "Invalid PCI ROM header signature" warning comes from
> pci_get_rom_size().  We don't call that at enumeration-time; we only
> call it later when somebody tries to read the ROM via sysfs:
>
>   pci_bus_add_device
>     pci_fixup_device(pci_fixup_final)
>       pci_fixup_video                 # final fixup
>         res->flags = MEM | SHADOW | PCI_FIXED
>     pci_create_sysfs_dev_files
>       if (SHADOW)
>         rom_size = 0x20000            # oops, I should have fixed this too
>       if (rom_size)
>         attr->read = pci_read_rom
>         sysfs_create_bin_file         # sysfs "rom" file
>
>   pci_read_rom                        # sysfs "rom" read function
>     pci_map_rom
>       ioremap
>       pci_get_rom_size
>         dev_err("Invalid PCI ROM header signature")
>     memcpy_fromio
>     pci_unmap_rom
>
> I think this is the same for regular ROMs on the device as it is for
> the magic VGA shadow ROM.  In both cases, we create the sysfs "rom"
> file regardless of what the ROM contains.
>
> I guess we could try to read the ROM at enumeration time and look for
> a valid signature.  I've considered doing that anyway so we could
> cache the ROM contents and avoid permanently allocating MMIO space for
> it, since many BIOSes don't allocate space, and Linux isn't really smart
> enough to do a good job of it itself.
>
> I don't know why the PCI core cares about the ROM signature anyway,
> since it doesn't use the content.  Maybe we should get rid of
> pci_get_rom_size() and allow you to read whatever is there, like
> we do for other BARs.  I suppose there's some history behind limiting
> the size, but I don't know what it is.

FWIW, if I disable all the checks in pci_get_rom_size, I learn that my
video ROM consists entirely of 0xff bytes.  Maybe there just isn't a
ROM shadow on my laptop.

--Andy