[coreboot] Resource Allocator for v3

[Myles Watson]

> -----Original Message-----
> From: coreboot-bounces at coreboot.org [mailto:coreboot-bounces at coreboot.org]
> On Behalf Of Peter Stuge
> Sent: Friday, November 21, 2008 10:14 PM
> To: 'Coreboot'
> Subject: Re: [coreboot] Resource Allocator for v3
> 
> Hi Myles,
> 
> Myles Watson wrote:
> > I've decided to take on the resource allocator.  I think it can be
> > written to work in three passes that will be clearly defined.
> 
> You beat me to it. :)
> 
> Me, Ron and Stefan had a close look at it.
> 
> 
> > Here's a summary of how it works right now (as best I can tell)
> >
> > There's a function called compute_allocate_resource in
> > device/device.c which is the main part of the function.
> 
> It's, well, let's say special.
> 
> 
> > 1. It calls read bus resources on the bus
> > 2. In a loop, it goes through all the resources (largest to
> > smallest) and assigns them values.  It marks them assigned, checks
> > that they don't conflict with PCI, etc.
> > 3. As it assigns values, it increments the base address it uses.
> > 4. After it assigns the values, it sets the resource's size
> >
> > Part of what makes this function confusing is how often it gets
> > called.
> 
> Yes.
> 
> 
> > It gets called in the device code during phase4, but it also gets
> > called every time a PCI bus resource is "recorded."  So it gets
> > called lots of times, and thus there's a need for the
> > have_resources flag which means "have read resources."
> 
> Yes. The code is very clever, it can resolve any kind of conflict in
> resources, but our conclusion was that this should not really be
> neccesary if allocation is made a separate step. We couldn't think of
> a case where this behavior was really required, we believe the
> function has been written to be completely general.
> 
> 
> > My proposal is to split it into distinct pieces.  I've implemented
> > most of it but it's not quite there.
> 
> Awesome. This was also our idea.
> 
> 
> > 1. read_resources - Just read resources.  No setting PCI Config
> > values, etc.
> > 2. compute_resource_needs - Go through using the same algorithm as
> > compute_allocate_resources, but don't assign the values.  Just sum
> > them up.
> > This is also the time to propagate minimum resource alignments,
> > etc.  It is a depth-first dts traversal.
> 
> I started thinking about how to rewrite the allocation and my idea
> was to recurse down to the bottom and sum on the way up. It was quite
> straightforward in my mind, I hope I was right. :)

Yes, I think it would work this way.  I think it could all get done at once,
but I was hoping that it would be simpler and easier to understand in
multiple passes.  Later I wondered about the duplicated code...

> Suggest name sum_resources instead.

That's fine with me.
 
> > 3. assign_resource_values - Go through and assign actual values to
> > the resources, and make sure everything is reachable.  This is also
> > mostly the same as compute_allocate_resources, but it goes through
> > breadth first instead of depth first.
> 
> I disagree with rolling constraint checking into this step. Either
> put that into 2 above (rather not) or insert 2.5 validate_resources.

If part two does its job correctly, there should be no need for checking.  I
guess that's a big if at first :)


> > 4. set_resources
> >
> > I'm hoping to move the resource-specific code out of
> > device/pci_device.c, and much of the PCI-specific code out of
> > device/device.c  It doesn't seem like the allocation code should
> > know or care how to disable any specific type of resource when it
> > has size 0.  Same thing for the resource code.  It shouldn't fiddle
> > with start and end values.  The have_resources flag is completely
> > gone since resources only get read once at the beginning.
> 
> Lovely.
> 
> 
> > Right now I'm trying to understand the legacy PCI decoding problem.
> > I understand that legacy PCI only decoded 10 bits, so there are
> > aliasing problems.
> 
> What's "legacy PCI" ?

Here's the part I was referring to:

		if (resource->flags & IORESOURCE_IO) {
			/* Don't allow potential aliases over the legacy PCI
			 * expansion card addresses. The legacy PCI decodes
			 * only 10 bits, uses 0x100 - 0x3ff. Therefore, only
			 * 0x00 - 0xff can be used out of each 0x400 block
of
			 * I/O space.
			 */
			if ((base & 0x300) != 0) {
				base = (base & ~0x3ff) + 0x400;
			}
			/* Don't allow allocations in the VGA I/O range.
			 * PCI has special cases for that.
			 */
			else if ((base >= 0x3b0) && (base <= 0x3df)) {
				base = 0x3e0;
			}
		}

It seems like I'm misreading this and/or the check isn't broad enough.  If
there are really devices claiming all IO where the last 10 bits decode to
0x300, then many more ranges should be affected.  For example, a size 0x200
allocation from 0x200-0x3ff doesn't trigger this check.  Neither does a size
0x200 allocation from 0x1200 - 0x13ff, but a size 0x20 allocation from
0x1320-0x133f does.

My theory is that this only matters for devices that are subtractively
decoding the address space.  At least in the k8 case, the 0x1200-0x13ff
range transactions will never conflict with the "legacy PCI" unless the
device is on the southbridge.  If it's anywhere else, the HT routing means
that the southbridge will never see the transaction.
 
> 
> > The other thing I'm not sure about is prefetchable memory.  I see
> > that it has its own resource in k8, but it doesn't seem clear how
> > it was meant to be used.  For right now it's unimportant, but I
> > really need it to work correctly later.
> 
> I think Ron has some input on this.
> 
> 
> > I'll post some logs when it's closer to being ready.  I just needed
> > to write this down before the weekend.
> 
> Great work so far!

Thanks.

Regards,
Myles