Re: git pull] drm for v4.1-rc1

Re: git pull] drm for v4.1-rc1

[Linus Torvalds]

Hmm. The odd Intel PCI resource mess is back.

Or maybe it never went away.

I get these when suspending. Things *work*, but it's really spamming
my logs a fair bit:

  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  pci_bus 0000:01: Allocating resources
  pci_bus 0000:02: Allocating resources
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
  i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment

That resource is complete garbage. "flags 0x2" is not even a valid
flag value. I'm *guessing* it might be IORESOURCE_ROM_SHADOW, but if
that is valid, then it should also have have had the IORESOURCE_MEM
bit, and it doesn't.

(The low 8 bits of the resource flags depend on the high bits, which
is why I say that I'm "guessing" at that ROM_SHADOW bit. It could be
something else, like a IORESOURCE_MEM_CACHEABLE PnP bit - but that
should not show up for PCI, and "BAR 6" is normally the ROM resource,
so the ROM_SHADOW bit makes some sense.

The only place that sets IORESOURCE_ROM_SHADOW that I find is the x86
pci_fixup_video() function. That one checks for PCI_COMMAND_IO |
PCI_COMMAND_MEMORY in the PCI command word, though. Why are the other
bits not set?

Both i915/dri people and PCI people on the Cc. This warning does *not*
happen at bootup, but only at suspend time. So my suspicion is that
somebody messes with the PCI ROM resource, and disables it or
something, but the IORESOURCE_ROM_SHADOW never gets cleared. And then
because res->flags is non-zero, the PCI scanning code doesn't ignore
the resource.

Just before the whole bogus alignment check, the PCI code does

                if (!(r->flags) || r->parent)
                        continue;

(don't ask me about the odd parenthesis) which *should* have
triggered, but that IORESOURCE_ROM_SHADOW bit screws things up.

Anybody?

            Linus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: git pull] drm for v4.1-rc1

[Bjorn Helgaas]

On Tue, Apr 21, 2015 at 11:07 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Hmm. The odd Intel PCI resource mess is back.
>
> Or maybe it never went away.
>
> I get these when suspending. Things *work*, but it's really spamming
> my logs a fair bit:
>
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   pci_bus 0000:01: Allocating resources
>   pci_bus 0000:02: Allocating resources
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>
> That resource is complete garbage. "flags 0x2" is not even a valid
> flag value. I'm *guessing* it might be IORESOURCE_ROM_SHADOW, but if
> that is valid, then it should also have have had the IORESOURCE_MEM
> bit, and it doesn't.
>
> (The low 8 bits of the resource flags depend on the high bits, which
> is why I say that I'm "guessing" at that ROM_SHADOW bit. It could be
> something else, like a IORESOURCE_MEM_CACHEABLE PnP bit - but that
> should not show up for PCI, and "BAR 6" is normally the ROM resource,
> so the ROM_SHADOW bit makes some sense.
>
> The only place that sets IORESOURCE_ROM_SHADOW that I find is the x86
> pci_fixup_video() function. That one checks for PCI_COMMAND_IO |
> PCI_COMMAND_MEMORY in the PCI command word, though. Why are the other
> bits not set?
>
> Both i915/dri people and PCI people on the Cc. This warning does *not*
> happen at bootup, but only at suspend time. So my suspicion is that
> somebody messes with the PCI ROM resource, and disables it or
> something, but the IORESOURCE_ROM_SHADOW never gets cleared. And then
> because res->flags is non-zero, the PCI scanning code doesn't ignore
> the resource.
>
> Just before the whole bogus alignment check, the PCI code does
>
>                 if (!(r->flags) || r->parent)
>                         continue;
>
> (don't ask me about the odd parenthesis) which *should* have
> triggered, but that IORESOURCE_ROM_SHADOW bit screws things up.
>
> Anybody?

I'll look into this.  It's been around a long time, but hasn't
percolated to the top of my list until now.

https://bugzilla.kernel.org/show_bug.cgi?id=16063 says "echo 1
>/sys/bus/pci/rescan" is also a reproducer.

Bjorn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: git pull] drm for v4.1-rc1

[Bjorn Helgaas]

[+cc Matthew]

On Tue, Apr 21, 2015 at 09:07:45AM -0700, Linus Torvalds wrote:
> Hmm. The odd Intel PCI resource mess is back.
> 
> Or maybe it never went away.
> 
> I get these when suspending. Things *work*, but it's really spamming
> my logs a fair bit:
> 
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   pci_bus 0000:01: Allocating resources
>   pci_bus 0000:02: Allocating resources
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
> 
> That resource is complete garbage. "flags 0x2" is not even a valid
> flag value. I'm *guessing* it might be IORESOURCE_ROM_SHADOW, but if
> that is valid, then it should also have have had the IORESOURCE_MEM
> bit, and it doesn't.

Your i915 does not have a ROM BAR in hardware.  If the default video
device has no ROM BAR, pci_fixup_video() sets IORESOURCE_ROM_SHADOW
even though the resource flags are zero because the BAR itself doesn't
exist.

If IORESOURCE_ROM_SHADOW is set, pci_map_rom() assumes there's a
shadow ROM image at 0xC0000.  Is there a shadow image even if the
device itself doesn't have a ROM BAR?

We could fabricate a resource even if the BAR doesn't exist, e.g.,
"flags = IORESOURCE_MEM | ... | IORESOURCE_ROM_SHADOW", but that
would be ugly and error-prone in other places that use the ROM.

Matthew added dev->rom for ROM images supplied by the platform
(84c1b80e3263 ("PCI: Add support for non-BAR ROMs")).  A shadow
image seems like a similar thing.  I think it would be cleaner to get
rid of IORESOURCE_ROM_SHADOW altogether and instead set "dev->rom =
0xC0000" if there's a shadow image, e.g.:

  int pcibios_add_device(struct pci_dev *dev)
  {
    if (dev-is-default-vga-device) {
      dev->rom = 0xC0000;
      dev->romlen = 0x20000;
    }

    pa_data = boot_params.hdr.setup_data;
    while (pa_data) {
      ...
      if (data->type == SETUP_PCI) {
        rom = (struct pci_setup_rom *)data;

        if (dev-is-rom-dev) {
          dev->rom = ...
          dev->romlen = rom->pcilen;
        }
      }
    }

But the rules for figuring out which image to use seem ...
complicated.

Bjorn
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: git pull] drm for v4.1-rc1

[Matthew Garrett]

On Thu, Apr 23, 2015 at 2:30 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> Your i915 does not have a ROM BAR in hardware.  If the default video
> device has no ROM BAR, pci_fixup_video() sets IORESOURCE_ROM_SHADOW
> even though the resource flags are zero because the BAR itself doesn't
> exist.
>
> If IORESOURCE_ROM_SHADOW is set, pci_map_rom() assumes there's a
> shadow ROM image at 0xC0000.  Is there a shadow image even if the
> device itself doesn't have a ROM BAR?

On UEFI systems, there's no special reason to believe that there's
anything at 0xc0000 - it depends on whether a CSM got loaded or not.
Everything is awful. So...

>   int pcibios_add_device(struct pci_dev *dev)
>   {
>     if (dev-is-default-vga-device) {
>       dev->rom = 0xC0000;
>       dev->romlen = 0x20000;
>     }

I don't know what we want to do here. This is, at some level,
fundamentally wrong - however, it also wouldn't surprise me if this is
also the only copy of the video ROM we have on some UEFI systems,
especially since I believe that Windows 7 still required that there be
a legacy ROM it could use for bootloader modesetting on UEFI
platforms. So simply making this conditional on BIOS may break
existing machines. But if there *is* a ROM there then we should be
able to id it from the usual video ROM signature?

Re: git pull] drm for v4.1-rc1

[Linus Torvalds]

On Thu, Apr 23, 2015 at 2:56 PM, Matthew Garrett
<matthew.garrett@coreos.com> wrote:
> On Thu, Apr 23, 2015 at 2:30 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>>     if (dev-is-default-vga-device) {
>>       dev->rom = 0xC0000;
>>       dev->romlen = 0x20000;
>>     }
>
> I don't know what we want to do here. This is, at some level,
> fundamentally wrong - however, it also wouldn't surprise me if this is
> also the only copy of the video ROM we have on some UEFI systems,
> especially since I believe that Windows 7 still required that there be
> a legacy ROM it could use for bootloader modesetting on UEFI
> platforms. So simply making this conditional on BIOS may break
> existing machines. But if there *is* a ROM there then we should be
> able to id it from the usual video ROM signature?

I'm not sure why we want that IORESOURCE_ROM_SHADOW thing at all, but
yes, if what this is all about is the magic video ROM at 0xc0000, then

 (a) it should have nothing what-so-ever to do with the actual PCI
BAR, since it's been *ages* since people actually had an expansion rom
like that, and it's much more common that the video ROM comes as part
of the system BIOS on laptops etc.

 (b) yes, the sane thing to do would be to just look for the ROM
signature, 0x55 0xaa at 2kB incrementing headers (and checking the
proper checksum too).

There is no way to see that from the PCI device state, because as
mentioned, quite often the "ROM" is entirely fake, and is not just
some shadowed copy of a real underlying hardware ROM, but is
fundamentally just a RAM image decompressed from some other source and
then marked read-only.

                   Linus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: git pull] drm for v4.1-rc1

[Matthew Garrett]

On Thu, Apr 23, 2015 at 3:47 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> I'm not sure why we want that IORESOURCE_ROM_SHADOW thing at all, but
> yes, if what this is all about is the magic video ROM at 0xc0000, then

It's used in the PCI layer to say "Read from 0xc0000 rather than the
ROM BAR" and then ends up as a shorthand for "Was this the boot video
device" in various places because we're bad at software.

> There is no way to see that from the PCI device state, because as
> mentioned, quite often the "ROM" is entirely fake, and is not just
> some shadowed copy of a real underlying hardware ROM, but is
> fundamentally just a RAM image decompressed from some other source and
> then marked read-only.

If only - nvidias used to rewrite their image at runtime.

-- 
Matthew Garrett | matthew.garrett@coreos.com

Re: git pull] drm for v4.1-rc1

[Alex Deucher]

On Thu, Apr 23, 2015 at 5:30 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Matthew]
>
> On Tue, Apr 21, 2015 at 09:07:45AM -0700, Linus Torvalds wrote:
>> Hmm. The odd Intel PCI resource mess is back.
>>
>> Or maybe it never went away.
>>
>> I get these when suspending. Things *work*, but it's really spamming
>> my logs a fair bit:
>>
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   pci_bus 0000:01: Allocating resources
>>   pci_bus 0000:02: Allocating resources
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>   i915 0000:00:02.0: BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
>>
>> That resource is complete garbage. "flags 0x2" is not even a valid
>> flag value. I'm *guessing* it might be IORESOURCE_ROM_SHADOW, but if
>> that is valid, then it should also have have had the IORESOURCE_MEM
>> bit, and it doesn't.
>
> Your i915 does not have a ROM BAR in hardware.  If the default video
> device has no ROM BAR, pci_fixup_video() sets IORESOURCE_ROM_SHADOW
> even though the resource flags are zero because the BAR itself doesn't
> exist.
>
> If IORESOURCE_ROM_SHADOW is set, pci_map_rom() assumes there's a
> shadow ROM image at 0xC0000.  Is there a shadow image even if the
> device itself doesn't have a ROM BAR?

Very likely yes.  With integrated APUs and mobile dGPUs, the vbios
image is often stored as part of the system rom rather than as a
dedicated rom for the GPU.  The vbios image is then copied to the
shadow location during sbios init to provide OS access to the rom.

Alex

>
> We could fabricate a resource even if the BAR doesn't exist, e.g.,
> "flags = IORESOURCE_MEM | ... | IORESOURCE_ROM_SHADOW", but that
> would be ugly and error-prone in other places that use the ROM.
>
> Matthew added dev->rom for ROM images supplied by the platform
> (84c1b80e3263 ("PCI: Add support for non-BAR ROMs")).  A shadow
> image seems like a similar thing.  I think it would be cleaner to get
> rid of IORESOURCE_ROM_SHADOW altogether and instead set "dev->rom =
> 0xC0000" if there's a shadow image, e.g.:
>
>   int pcibios_add_device(struct pci_dev *dev)
>   {
>     if (dev-is-default-vga-device) {
>       dev->rom = 0xC0000;
>       dev->romlen = 0x20000;
>     }
>
>     pa_data = boot_params.hdr.setup_data;
>     while (pa_data) {
>       ...
>       if (data->type == SETUP_PCI) {
>         rom = (struct pci_setup_rom *)data;
>
>         if (dev-is-rom-dev) {
>           dev->rom = ...
>           dev->romlen = rom->pcilen;
>         }
>       }
>     }
>
> But the rules for figuring out which image to use seem ...
> complicated.
>
> Bjorn
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel