| Message ID | 4A4966EF.6010809@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Yinghai Lu wrote: >>> >> That doesn't quite look right; for one thing it doesn't handle the >> (admittedly somewhat unlikely) case of end pointing to the end of the >> address space. >> >> Something like: >> >> if (start > (resource_size_t)end-1) >> continue; >> > + if (start > (resource_size_t)end) > continue; Erk... thinko on my part. Should have been (resource_size_t)-1. -hpa
H. Peter Anvin wrote: > Yinghai Lu wrote: >>>> >>> That doesn't quite look right; for one thing it doesn't handle the >>> (admittedly somewhat unlikely) case of end pointing to the end of the >>> address space. >>> >>> Something like: >>> >>> if (start > (resource_size_t)end-1) >>> continue; >>> > >> + if (start > (resource_size_t)end) >> continue; > > Erk... thinko on my part. Should have been (resource_size_t)-1. > how about start is already 32M ? you will insert blank one... 64bit, we could expand range that is above 4g to be aligned. YH -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 29 Jun 2009, Yinghai Lu wrote: > + end = round_up(start, ram_alignment(start)) - 1; > + if (start > (resource_size_t)end) > continue; > - reserve_region_with_split(&iomem_resource, start, > - end - 1, "RAM buffer"); > + reserve_region_with_split(&iomem_resource, (resource_size_t)start, > + (resource_size_t)end, "RAM buffer"); Hmm. You shouldn't need the casts with reserve_region_with_split(), and they just make things uglier. Also, I wonder if we should do something like this instead #define MAX_RESOURCE_SIZE ((resource_size_t)-1) ... end = round_up(start, ram_alignment(start)) - 1; if (end > MAX_RESOURCE_SIZE) end = MAX_RESOURCE_SIZE; if (start > end) continue; Because otherwise we'll just be ignoring resources that cross the resource size boundary, which sounds wrong. We _could_ have a RAM resource that crosses the 4GB boundary, after all. Yeah, it doesn't happen much in practice, because usually the 3G-4G range is left for PCI mappings etc, so we might never hit this in practice, but still, this sounds like a more correct thing to do. It also avoids the cast. We simply cap the end to the max that 'resource_size_t' can hold. That said, I have to admit that I'm getting tired of these bugs that only happen when we have a 32-bit resource_size_t. So I can understand the attraction to just forcing it to 64-bit and forgetting about these irritating issues. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds wrote: > ... > end = round_up(start, ram_alignment(start)) - 1; > if (end > MAX_RESOURCE_SIZE) > end = MAX_RESOURCE_SIZE; > if (start > end) > continue; > > Because otherwise we'll just be ignoring resources that cross the resource > size boundary, which sounds wrong. > > We _could_ have a RAM resource that crosses the 4GB boundary, after all. > We could, but the *alignment pad* shouldn't be able to cross a power-of-two boundary ("end" is always an aligned-up version of "start"). > That said, I have to admit that I'm getting tired of these bugs that only > happen when we have a 32-bit resource_size_t. So I can understand the > attraction to just forcing it to 64-bit and forgetting about these > irritating issues. Probably would be worth figuring out just how much it would be. -hpa
Yinghai Lu wrote: > > how about start is already 32M ? > you will insert blank one... > I was rather assuming that zero range length was handled elsewhere... -hpa
Index: linux-2.6/arch/x86/kernel/e820.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/e820.c +++ linux-2.6/arch/x86/kernel/e820.c @@ -1367,9 +1367,9 @@ void __init e820_reserve_resources(void) } /* How much should we pad RAM ending depending on where it is? */ -static unsigned long ram_alignment(resource_size_t pos) +static u64 ram_alignment(u64 pos) { - unsigned long mb = pos >> 20; + u64 mb = pos >> 20; /* To 64kB in the first megabyte */ if (!mb) @@ -1400,17 +1400,17 @@ void __init e820_reserve_resources_late( * avoid stolen RAM: */ for (i = 0; i < e820.nr_map; i++) { - struct e820entry *entry = &e820_saved.map[i]; - resource_size_t start, end; + struct e820entry *entry = &e820.map[i]; + u64 start, end; if (entry->type != E820_RAM) continue; start = entry->addr + entry->size; - end = round_up(start, ram_alignment(start)); - if (start == end) + end = round_up(start, ram_alignment(start)) - 1; + if (start > (resource_size_t)end) continue; - reserve_region_with_split(&iomem_resource, start, - end - 1, "RAM buffer"); + reserve_region_with_split(&iomem_resource, (resource_size_t)start, + (resource_size_t)end, "RAM buffer"); } }