| Message ID | 20161102210054.16621-6-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 02, 2016 at 03:00:53PM -0600, Laura Abbott wrote: > > __pa_symbol is technically the marco that should be used for kernel > symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. Nit: s/marco/macro/ I see there are some other uses of __pa() that look like they could/should be __pa_symbol(), e.g. in mark_rodata_ro(). I guess strictly speaking those need to be updated to? Or is there a reason that we should not? Thanks, Mark. > Signed-off-by: Laura Abbott <labbott@redhat.com> > --- > arch/arm64/mm/init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 212c4d1..3236eb0 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -209,8 +209,8 @@ void __init arm64_memblock_init(void) > * linear mapping. Take care not to clip the kernel which may be > * high in memory. > */ > - memblock_remove(max_t(u64, memstart_addr + linear_region_size, __pa(_end)), > - ULLONG_MAX); > + memblock_remove(max_t(u64, memstart_addr + linear_region_size, > + __pa_symbol(_end)), ULLONG_MAX); > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { > /* ensure that memstart_addr remains sufficiently aligned */ > memstart_addr = round_up(memblock_end_of_DRAM() - linear_region_size, > -- > 2.10.1 >
On 11/02/2016 04:52 PM, Mark Rutland wrote: > On Wed, Nov 02, 2016 at 03:00:53PM -0600, Laura Abbott wrote: >> >> __pa_symbol is technically the marco that should be used for kernel >> symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. > > Nit: s/marco/macro/ > > I see there are some other uses of __pa() that look like they could/should be > __pa_symbol(), e.g. in mark_rodata_ro(). > > I guess strictly speaking those need to be updated to? Or is there a reason > that we should not? > If the concept of __pa_symbol is okay then yes I think all uses of __pa should eventually be converted for consistency and debugging. > Thanks, > Mark. > >> Signed-off-by: Laura Abbott <labbott@redhat.com> >> --- >> arch/arm64/mm/init.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 212c4d1..3236eb0 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -209,8 +209,8 @@ void __init arm64_memblock_init(void) >> * linear mapping. Take care not to clip the kernel which may be >> * high in memory. >> */ >> - memblock_remove(max_t(u64, memstart_addr + linear_region_size, __pa(_end)), >> - ULLONG_MAX); >> + memblock_remove(max_t(u64, memstart_addr + linear_region_size, >> + __pa_symbol(_end)), ULLONG_MAX); >> if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { >> /* ensure that memstart_addr remains sufficiently aligned */ >> memstart_addr = round_up(memblock_end_of_DRAM() - linear_region_size, >> -- >> 2.10.1 >>
On Wed, Nov 02, 2016 at 05:56:42PM -0600, Laura Abbott wrote: > On 11/02/2016 04:52 PM, Mark Rutland wrote: > >On Wed, Nov 02, 2016 at 03:00:53PM -0600, Laura Abbott wrote: > >> > >>__pa_symbol is technically the marco that should be used for kernel > >>symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. > > > >Nit: s/marco/macro/ > > > >I see there are some other uses of __pa() that look like they could/should be > >__pa_symbol(), e.g. in mark_rodata_ro(). > > > >I guess strictly speaking those need to be updated to? Or is there a reason > >that we should not? > > If the concept of __pa_symbol is okay then yes I think all uses of __pa > should eventually be converted for consistency and debugging. I have no strong feelings either way about __pa_symbol(); I'm not clear on what the purpose of __pa_symbol() is specifically, but I'm happy even if it's just for consistency with other architectures. However, if we use it I think that we should (attempt to) use it consistently from the outset. Thanks, Mark.
On Thu, Nov 03, 2016 at 03:51:07PM +0000, Mark Rutland wrote: > On Wed, Nov 02, 2016 at 05:56:42PM -0600, Laura Abbott wrote: > > On 11/02/2016 04:52 PM, Mark Rutland wrote: > > >On Wed, Nov 02, 2016 at 03:00:53PM -0600, Laura Abbott wrote: > > >> > > >>__pa_symbol is technically the marco that should be used for kernel > > >>symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. > > > > > >Nit: s/marco/macro/ > > > > > >I see there are some other uses of __pa() that look like they could/should be > > >__pa_symbol(), e.g. in mark_rodata_ro(). > > > > > >I guess strictly speaking those need to be updated to? Or is there a reason > > >that we should not? > > > > If the concept of __pa_symbol is okay then yes I think all uses of __pa > > should eventually be converted for consistency and debugging. > > I have no strong feelings either way about __pa_symbol(); I'm not clear on what > the purpose of __pa_symbol() is specifically, but I'm happy even if it's just > for consistency with other architectures. At a quick grep, it seems to only be used by mips and x86 and a single place in mm/memblock.c. Since we haven't seen any issues on arm/arm64 without this macro, can we not just continue to use __pa()? Thanks.
On 11/14/2016 10:19 AM, Catalin Marinas wrote: > On Thu, Nov 03, 2016 at 03:51:07PM +0000, Mark Rutland wrote: >> On Wed, Nov 02, 2016 at 05:56:42PM -0600, Laura Abbott wrote: >>> On 11/02/2016 04:52 PM, Mark Rutland wrote: >>>> On Wed, Nov 02, 2016 at 03:00:53PM -0600, Laura Abbott wrote: >>>>> >>>>> __pa_symbol is technically the marco that should be used for kernel >>>>> symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. >>>> >>>> Nit: s/marco/macro/ >>>> >>>> I see there are some other uses of __pa() that look like they could/should be >>>> __pa_symbol(), e.g. in mark_rodata_ro(). >>>> >>>> I guess strictly speaking those need to be updated to? Or is there a reason >>>> that we should not? >>> >>> If the concept of __pa_symbol is okay then yes I think all uses of __pa >>> should eventually be converted for consistency and debugging. >> >> I have no strong feelings either way about __pa_symbol(); I'm not clear on what >> the purpose of __pa_symbol() is specifically, but I'm happy even if it's just >> for consistency with other architectures. > > At a quick grep, it seems to only be used by mips and x86 and a single > place in mm/memblock.c. > > Since we haven't seen any issues on arm/arm64 without this macro, can we > not just continue to use __pa()? Technically yes but if it's introduced it may be confusing why it's being used some places but not others. Maybe the bounds in the debug virtual check should just be adjusted so we don't need __pa_symbol along with a nice fat comment explaining why. > > Thanks. > Thanks, Laura
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 212c4d1..3236eb0 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -209,8 +209,8 @@ void __init arm64_memblock_init(void) * linear mapping. Take care not to clip the kernel which may be * high in memory. */ - memblock_remove(max_t(u64, memstart_addr + linear_region_size, __pa(_end)), - ULLONG_MAX); + memblock_remove(max_t(u64, memstart_addr + linear_region_size, + __pa_symbol(_end)), ULLONG_MAX); if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { /* ensure that memstart_addr remains sufficiently aligned */ memstart_addr = round_up(memblock_end_of_DRAM() - linear_region_size,
__pa_symbol is technically the marco that should be used for kernel symbols. Switch to this as a pre-requisite for DEBUG_VIRTUAL. Signed-off-by: Laura Abbott <labbott@redhat.com> --- arch/arm64/mm/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)