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-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
