Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4

[Linus Torvalds]

On Fri, 17 Apr 2009, Ingo Molnar wrote:
> 
> Could we perhaps round up to 1MB in this case too?

(The below 1MB one). 

I'd argue against it, at least in this incarnation. I can well imagine 
somebody wanting to do resource management in the 640k-1M window, so..

> Would it make sense to round up everything that is listed in the 
> E820 map? Just in case the BIOS is not entirely honest about the 
> true extent of that area.

Well, it would probably work, but on the other hand, when we see 
"E820_RAM", that means that we really _can_ trust that that E820 entry is 
right, since we're going to use it as RAM (and Windows would too), and if 
it wasn't RAM, really bad things would happen.

So E820_RAM is a _lot_ more trustworthy than the other cases. If we're 
rounding up by reasonably large amounts like 32MB or even more, I really 
think we should do so for the things we really know are there, and that we 
really fundamentally know come in big granularities.

The other entries in the e820 map can reasonably be 4kB or something, 
because they are an IO-APIC or whatever. I can't say that I'd feel happy 
putting a guard area around something like that. But RAM? Sure, it can 
come in 384kB chunks (think RAM remapping for the low 1MB area), but it 
doesn't tend to happen when we're talking gigs any more.

> > +		start = entry->addr + entry->size;
> > +		end = round_up(start, ram_alignment(start));
> > +		if (start == end)
> > +			continue;
> 
> Hm, indeed, the continue is needed - reserve_region_with_split() 
> lets zero-sized resources be inserted silently. I'd have missed this 
> case. Do zero-sized memory resources have a special role somewhere?

No. But it wouldn't be a zero-size region, it would be a one-byte sized 
region. It's just that my patch was missing the "-1" from the end that I 
meant to put there:

> > +		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");

That 'end' there should be 'end-1', and that also explains why "start == 
end" must have a continue.

The 'end' in a resource region is the last byte, not the 'byte after'. 

So there was a small buglet in the patch, but as I mentioned, using 
"reserve_region_with_split()" is really wrong anyway, because we do not 
want to recurse into existing regions, just split _around_ them. So the 
patch was meant as 

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/