Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas <bhelgaas@google.com>]

On Thu, Jul 03, 2014 at 12:59:21PM -0700, Yinghai Lu wrote:
> On Thu, Jul 3, 2014 at 6:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>
> >> >> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> >> >> +
> >> >> +     /* kernel does not support 64bit res */
> >> >> +     if (sizeof(resource_size_t) == 4)
> >> >> +             mem64_mask &= ~IORESOURCE_MEM_64;
> >
> > I think you're fixing two things at once, and they should be split
> > into two separate patches:
> >
> >   1) Change aligns[] size to increase support alignment from 8GB to 2^63
> >
> >      I'm not sure about going all the way to aligns[44].  That array
> >      by itself puts 352 bytes in the stack frame (240 of which are
> >      added by this patch), which seems excessive.  I suspect that
> >      supporting BARs up to 64GB or 128GB would be enough for the
> >      foreseeable future.
> 
> ok, let's use 128G this time.
> 
> >
> >   2) Adding mem64_mask
> >
> >      I think the idea is that even if we have a 64-bit window, we
> >      can't use anything above 4GB if we only have 32-bit resources.
> >      That's true, but I don't think we can enforce that in
> >      pbus_size_mem() because we're only figuring out how much space is
> >      needed; we have no idea where that space will be allocated.
> 
> with current version of __pci_bus_size_bridges, we separate pref_mem64 with
> pref_mem32 to calling pbus_size_mem.
> 
> so if we have pref 64bit window, we will only size pref 64 bit
> children under it.
> and will only assign pref 64bit mem64 to them late.
> 
> If the bridge does not support 64bit pref windows, and child support
> 64bit pref mmio, then bridge will try to use pref 32 window, in that
> case, .... could have
> size overflow as you state follow.

This problem has nothing to do with overflow.  We're concerned with
IORESOURCE_MEM_64, which tells us the width of the bridge window
registers: obviously we can't put a 64-bit bus address in a 32-bit
window register.

pbus_size_mem() figures out how much space we need.  If we need 4GB or
more and the kernel only supports 32-bit resources, we can fail
immediately because we can't describe a 4GB window in a 32-bit
resource.

But if we need less than 4GB, pbus_size_mem() should succeed.  We
might fail *later*, if we can't allocate bus addresses that fit in a
32-bit bridge window register.  But in pbus_size_mem(), we have no
idea where the space will come from.

So I don't think there's any point in looking at the sizes of
individual BARs in pbus_size_mem().  We should only look at the total
size required.

> > And I think there are more problems:
> >
> >   - I don't think we handle overflow of "size" correctly.  Assume that
> >     we have BARs of 2GB, 2GB, and 8GB.  If we have 32-bit resources,
> >     when we add those up, it will overflow and we'll mistakenly think
> >     we only need 8MB.
> 
> in this case we should check the overflow.
> 
> >
> >   - We shouldn't set "r->flags = 0".  The warning says we're disabling
> >     the BAR, but this *doesn't* disable the BAR, and in fact, there is
> >     no way to disable a single BAR.  What we can do is turn off
> >     PCI_COMMAND_MEMORY to disable all the memory BARs for the device.
> >     And to do that, we need to keep IORESOURCE_MEM in r->flags so
> >     pci_enable_resources() can tell that this is a memory BAR.
> 
> when we try to size it,  means that bar is not assigned. with r->flags
> = 0, means
> we will ignore it all the way.

With "r->flags = 0", we will not try to assign resources to the BAR,
but the hardware BAR still exists and contains some address.  If the
device has other memory BARs, pci_enable_resources() will turn on
PCI_COMMAND_MEMORY.  Now the "r->flags == 0" BAR is enabled but the
address it contains might conflict with other devices.  That's the
problem.

To fix this, I think we need to do "r->flags |= IORESOURCE_UNSET |
IORESOURCE_DISABLED".

Bjorn