| Message ID | 20150818142435.GC11571@e104818-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18 August 2015 at 16:24, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Aug 18, 2015 at 11:00:27AM +0100, Will Deacon wrote: >> On Tue, Aug 18, 2015 at 10:34:42AM +0100, Ard Biesheuvel wrote: >> > The linear region size of a 39-bit VA kernel is only 256 GB, which >> > may be insufficient to cover all of system RAM, even on platforms >> > that have much less than 256 GB of memory but which is laid out >> > very sparsely. >> > >> > So make sure we clip the memory we will not be able to map before >> > installing it into the memblock memory table, by setting >> > MAX_MEMBLOCK_ADDR accordingly. >> > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > --- >> > arch/arm64/include/asm/memory.h | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h >> > index f800d45ea226..44a59c20e773 100644 >> > --- a/arch/arm64/include/asm/memory.h >> > +++ b/arch/arm64/include/asm/memory.h >> > @@ -114,6 +114,14 @@ extern phys_addr_t memstart_addr; >> > #define PHYS_OFFSET ({ memstart_addr; }) >> > >> > /* >> > + * The maximum physical address that the linear direct mapping >> > + * of system RAM can cover. (PAGE_OFFSET can be interpreted as >> > + * a 2's complement signed quantity and negated to derive the >> > + * maximum size of the linear mapping.) >> > + */ >> > +#define MAX_MEMBLOCK_ADDR ({ memstart_addr - PAGE_OFFSET - 1; }) > > With the current memory layout, this could also be __pa(~0UL). I guess > we could solve this with a single patch, though I'm not sure whether we > break other architectures: > Every 32-bit DT arch with highmem, quite likely. > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 07496560e5b9..ff8a885d5ff0 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -967,7 +967,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, > } > > #ifdef CONFIG_HAVE_MEMBLOCK > -#define MAX_PHYS_ADDR ((phys_addr_t)~0) > +#define MAX_PHYS_ADDR (__pa(~0UL)) > > void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) > { > >> If we initialised memory_limit to this value and changed early_mem to >> use min (i.e. only restrict the limit further), would that avoid the >> need to change the of code? > > Only that we can't initialise memory_limit statically since > memstart_addr is not constant. We would need to do this somewhere before > early_mem() is run (in setup_machine_fdt or immediately after it). > > My point to Ard was that since we already do a sanity check on the > memblocks in early_init_dt_add_memory_arch() and ignore those below > PHYS_OFFSET, it makes sense to reuse the same function since it already > has all the logic in place and corresponding warnings. With a later > memblock limiting we would have to add extra printing to inform the > user. > > -- > Catalin
On 18 August 2015 at 16:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 18 August 2015 at 16:24, Catalin Marinas <catalin.marinas@arm.com> wrote: >> On Tue, Aug 18, 2015 at 11:00:27AM +0100, Will Deacon wrote: >>> On Tue, Aug 18, 2015 at 10:34:42AM +0100, Ard Biesheuvel wrote: >>> > The linear region size of a 39-bit VA kernel is only 256 GB, which >>> > may be insufficient to cover all of system RAM, even on platforms >>> > that have much less than 256 GB of memory but which is laid out >>> > very sparsely. >>> > >>> > So make sure we clip the memory we will not be able to map before >>> > installing it into the memblock memory table, by setting >>> > MAX_MEMBLOCK_ADDR accordingly. >>> > >>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> > --- >>> > arch/arm64/include/asm/memory.h | 8 ++++++++ >>> > 1 file changed, 8 insertions(+) >>> > >>> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h >>> > index f800d45ea226..44a59c20e773 100644 >>> > --- a/arch/arm64/include/asm/memory.h >>> > +++ b/arch/arm64/include/asm/memory.h >>> > @@ -114,6 +114,14 @@ extern phys_addr_t memstart_addr; >>> > #define PHYS_OFFSET ({ memstart_addr; }) >>> > >>> > /* >>> > + * The maximum physical address that the linear direct mapping >>> > + * of system RAM can cover. (PAGE_OFFSET can be interpreted as >>> > + * a 2's complement signed quantity and negated to derive the >>> > + * maximum size of the linear mapping.) >>> > + */ >>> > +#define MAX_MEMBLOCK_ADDR ({ memstart_addr - PAGE_OFFSET - 1; }) >> >> With the current memory layout, this could also be __pa(~0UL). I guess >> we could solve this with a single patch, though I'm not sure whether we >> break other architectures: >> > > Every 32-bit DT arch with highmem, quite likely. > I mean (L)PAE, of course > >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index 07496560e5b9..ff8a885d5ff0 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -967,7 +967,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, >> } >> >> #ifdef CONFIG_HAVE_MEMBLOCK >> -#define MAX_PHYS_ADDR ((phys_addr_t)~0) >> +#define MAX_PHYS_ADDR (__pa(~0UL)) >> >> void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) >> { >> >>> If we initialised memory_limit to this value and changed early_mem to >>> use min (i.e. only restrict the limit further), would that avoid the >>> need to change the of code? >> >> Only that we can't initialise memory_limit statically since >> memstart_addr is not constant. We would need to do this somewhere before >> early_mem() is run (in setup_machine_fdt or immediately after it). >> >> My point to Ard was that since we already do a sanity check on the >> memblocks in early_init_dt_add_memory_arch() and ignore those below >> PHYS_OFFSET, it makes sense to reuse the same function since it already >> has all the logic in place and corresponding warnings. With a later >> memblock limiting we would have to add extra printing to inform the >> user. >> >> -- >> Catalin
On Tue, Aug 18, 2015 at 03:24:36PM +0100, Catalin Marinas wrote: > With the current memory layout, this could also be __pa(~0UL). I guess > we could solve this with a single patch, though I'm not sure whether we > break other architectures: No. __pa() is only valid within the lowmem mapping, not outside of it. What you'd get for a virtual address of ~0UL is not well defined, so you should not use this in generic code.
On Tue, Aug 18, 2015 at 03:51:49PM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 18, 2015 at 03:24:36PM +0100, Catalin Marinas wrote: > > With the current memory layout, this could also be __pa(~0UL). I guess > > we could solve this with a single patch, though I'm not sure whether we > > break other architectures: > > No. __pa() is only valid within the lowmem mapping, not outside of it. > What you'd get for a virtual address of ~0UL is not well defined, so > you should not use this in generic code. You are right. We'll keep this max memblock address in the arch code.
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 07496560e5b9..ff8a885d5ff0 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -967,7 +967,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, } #ifdef CONFIG_HAVE_MEMBLOCK -#define MAX_PHYS_ADDR ((phys_addr_t)~0) +#define MAX_PHYS_ADDR (__pa(~0UL)) void __init __weak early_init_dt_add_memory_arch(u64 base, u64 size) {