* [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-23 11:45 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-23 11:45 UTC (permalink / raw) To: linux-arm-kernel A little change to patch_map() function, use set_fixmap_offset() to make code more clear. Signed-off-by: yalin wang <yalin.wang2010@gmail.com> --- arch/arm64/kernel/insn.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index dd9671c..7dafd5a 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) return addr; BUG_ON(!page); - set_fixmap(fixmap, page_to_phys(page)); - - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + + (addr & ~PAGE_MASK)); } static void __kprobes patch_unmap(int fixmap) -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-23 11:45 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-23 11:45 UTC (permalink / raw) To: catalin.marinas, will.deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, linux-kernel A little change to patch_map() function, use set_fixmap_offset() to make code more clear. Signed-off-by: yalin wang <yalin.wang2010@gmail.com> --- arch/arm64/kernel/insn.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index dd9671c..7dafd5a 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) return addr; BUG_ON(!page); - set_fixmap(fixmap, page_to_phys(page)); - - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + + (addr & ~PAGE_MASK)); } static void __kprobes patch_unmap(int fixmap) -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-23 11:45 ` yalin wang @ 2015-07-23 13:03 ` Catalin Marinas -1 siblings, 0 replies; 19+ messages in thread From: Catalin Marinas @ 2015-07-23 13:03 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..7dafd5a 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (addr & ~PAGE_MASK)); It looks fine. Do you get any compiler warning for the automatic pointer to long conversion? You may want to add some explicit casts, otherwise: Acked-by: Catalin Marinas <catalin.marinas@arm.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-23 13:03 ` Catalin Marinas 0 siblings, 0 replies; 19+ messages in thread From: Catalin Marinas @ 2015-07-23 13:03 UTC (permalink / raw) To: yalin wang Cc: will.deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, linux-kernel On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..7dafd5a 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (addr & ~PAGE_MASK)); It looks fine. Do you get any compiler warning for the automatic pointer to long conversion? You may want to add some explicit casts, otherwise: Acked-by: Catalin Marinas <catalin.marinas@arm.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-23 13:03 ` Catalin Marinas @ 2015-07-24 3:56 ` yalin wang -1 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-24 3:56 UTC (permalink / raw) To: linux-arm-kernel > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: >> A little change to patch_map() function, >> use set_fixmap_offset() to make code more clear. >> >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> >> --- >> arch/arm64/kernel/insn.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >> index dd9671c..7dafd5a 100644 >> --- a/arch/arm64/kernel/insn.c >> +++ b/arch/arm64/kernel/insn.c >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) >> return addr; >> >> BUG_ON(!page); >> - set_fixmap(fixmap, page_to_phys(page)); >> - >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + >> + (addr & ~PAGE_MASK)); > > It looks fine. Do you get any compiler warning for the automatic pointer > to long conversion? You may want to add some explicit casts, otherwise: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> i have build it, there is no warning about this change. :) Thanks ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-24 3:56 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-24 3:56 UTC (permalink / raw) To: Catalin Marinas Cc: Will Deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, linux-kernel > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: >> A little change to patch_map() function, >> use set_fixmap_offset() to make code more clear. >> >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> >> --- >> arch/arm64/kernel/insn.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >> index dd9671c..7dafd5a 100644 >> --- a/arch/arm64/kernel/insn.c >> +++ b/arch/arm64/kernel/insn.c >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) >> return addr; >> >> BUG_ON(!page); >> - set_fixmap(fixmap, page_to_phys(page)); >> - >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + >> + (addr & ~PAGE_MASK)); > > It looks fine. Do you get any compiler warning for the automatic pointer > to long conversion? You may want to add some explicit casts, otherwise: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> i have build it, there is no warning about this change. :) Thanks ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-24 3:56 ` yalin wang @ 2015-07-24 9:56 ` Mark Rutland -1 siblings, 0 replies; 19+ messages in thread From: Mark Rutland @ 2015-07-24 9:56 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: > > > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > >> A little change to patch_map() function, > >> use set_fixmap_offset() to make code more clear. > >> > >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > >> --- > >> arch/arm64/kernel/insn.c | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > >> index dd9671c..7dafd5a 100644 > >> --- a/arch/arm64/kernel/insn.c > >> +++ b/arch/arm64/kernel/insn.c > >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > >> return addr; > >> > >> BUG_ON(!page); > >> - set_fixmap(fixmap, page_to_phys(page)); > >> - > >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > >> + (addr & ~PAGE_MASK)); > > > > It looks fine. Do you get any compiler warning for the automatic pointer > > to long conversion? You may want to add some explicit casts, otherwise: > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > i have build it, there is no warning about this change. :) I see no warnings with defconfig, but there's an (unrelated) set of warnings if CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: ---- In file included from ./arch/arm64/include/asm/fixmap.h:85:0, from arch/arm64/kernel/insn.c:32: arch/arm64/kernel/insn.c: In function ?__aarch64_insn_write?: include/asm-generic/fixmap.h:73:2: warning: ?addr? may be used uninitialized in this function [-Wmaybe-uninitialized] __set_fixmap(idx, phys, flags); \ ^ include/asm-generic/fixmap.h:72:16: note: ?addr? was declared here unsigned long addr; \ ^ include/asm-generic/fixmap.h:79:2: note: in expansion of macro ?__set_fixmap_offset? __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL) ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ?set_fixmap_offset? return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ ---- That seems to be due to the definition of __set_fixmap_offset in asm-generic/fixmap.h: /* Return a pointer with offset calculated */ #define __set_fixmap_offset(idx, phys, flags) \ ({ \ unsigned long addr; \ __set_fixmap(idx, phys, flags); \ addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ addr; \ }) Where that new addr variable shadows patch_map's addr argument when the call to __set_fixmap is expanded. Which means that this patch currently breaks CONFIG_DEBUG_SET_MODULE_RONX and CONFIG_DEBUG_RODATA. Thanks, Mark. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-24 9:56 ` Mark Rutland 0 siblings, 0 replies; 19+ messages in thread From: Mark Rutland @ 2015-07-24 9:56 UTC (permalink / raw) To: yalin wang Cc: Catalin Marinas, lauraa@codeaurora.org, Marc Zyngier, zlim.lnx@gmail.com, Punit Agrawal, Will Deacon, linux-kernel@vger.kernel.org, wcohen@redhat.com, linux-arm-kernel@lists.infradead.org On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: > > > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > >> A little change to patch_map() function, > >> use set_fixmap_offset() to make code more clear. > >> > >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > >> --- > >> arch/arm64/kernel/insn.c | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > >> index dd9671c..7dafd5a 100644 > >> --- a/arch/arm64/kernel/insn.c > >> +++ b/arch/arm64/kernel/insn.c > >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > >> return addr; > >> > >> BUG_ON(!page); > >> - set_fixmap(fixmap, page_to_phys(page)); > >> - > >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > >> + (addr & ~PAGE_MASK)); > > > > It looks fine. Do you get any compiler warning for the automatic pointer > > to long conversion? You may want to add some explicit casts, otherwise: > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > i have build it, there is no warning about this change. :) I see no warnings with defconfig, but there's an (unrelated) set of warnings if CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: ---- In file included from ./arch/arm64/include/asm/fixmap.h:85:0, from arch/arm64/kernel/insn.c:32: arch/arm64/kernel/insn.c: In function ‘__aarch64_insn_write’: include/asm-generic/fixmap.h:73:2: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized] __set_fixmap(idx, phys, flags); \ ^ include/asm-generic/fixmap.h:72:16: note: ‘addr’ was declared here unsigned long addr; \ ^ include/asm-generic/fixmap.h:79:2: note: in expansion of macro ‘__set_fixmap_offset’ __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL) ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ‘set_fixmap_offset’ return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ ---- That seems to be due to the definition of __set_fixmap_offset in asm-generic/fixmap.h: /* Return a pointer with offset calculated */ #define __set_fixmap_offset(idx, phys, flags) \ ({ \ unsigned long addr; \ __set_fixmap(idx, phys, flags); \ addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ addr; \ }) Where that new addr variable shadows patch_map's addr argument when the call to __set_fixmap is expanded. Which means that this patch currently breaks CONFIG_DEBUG_SET_MODULE_RONX and CONFIG_DEBUG_RODATA. Thanks, Mark. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-24 9:56 ` Mark Rutland @ 2015-07-24 10:07 ` Mark Rutland -1 siblings, 0 replies; 19+ messages in thread From: Mark Rutland @ 2015-07-24 10:07 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 24, 2015 at 10:56:18AM +0100, Mark Rutland wrote: > On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: > > > > > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > > >> A little change to patch_map() function, > > >> use set_fixmap_offset() to make code more clear. > > >> > > >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > > >> --- > > >> arch/arm64/kernel/insn.c | 5 ++--- > > >> 1 file changed, 2 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > >> index dd9671c..7dafd5a 100644 > > >> --- a/arch/arm64/kernel/insn.c > > >> +++ b/arch/arm64/kernel/insn.c > > >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > > >> return addr; > > >> > > >> BUG_ON(!page); > > >> - set_fixmap(fixmap, page_to_phys(page)); > > >> - > > >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > > >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > > >> + (addr & ~PAGE_MASK)); > > > > > > It looks fine. Do you get any compiler warning for the automatic pointer > > > to long conversion? You may want to add some explicit casts, otherwise: > > > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > > i have build it, there is no warning about this change. :) > > I see no warnings with defconfig, but there's an (unrelated) set of warnings if > CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: Actually they aren't unrelated after all. the unsigned long addr in __set_fixmap_offset shadows the void* addr from patch_map making the types match. With a few underscores addrd to __set_fixmap_offset's addr, I get the following regarding patch_map: ---- In file included from ./arch/arm64/include/asm/fixmap.h:85:0, from arch/arm64/kernel/insn.c:32: arch/arm64/kernel/insn.c: In function ?patch_map?: arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ?void *? and ?long unsigned int?) (addr & ~PAGE_MASK)); ^ include/asm-generic/fixmap.h:73:20: note: in definition of macro ?__set_fixmap_offset? __set_fixmap(idx, phys, flags); \ ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ?set_fixmap_offset? return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ?void *? and ?long unsigned int?) (addr & ~PAGE_MASK)); ^ include/asm-generic/fixmap.h:74:32: note: in definition of macro ?__set_fixmap_offset? __addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ?set_fixmap_offset? return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ make[1]: *** [arch/arm64/kernel/insn.o] Error 1 make: *** [arch/arm64/kernel/insn.o] Error 2 ---- we need both the cast in patch_map, and an update to __set_fixmap_offset to prevent the name clash. Thanks, Mark. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-24 10:07 ` Mark Rutland 0 siblings, 0 replies; 19+ messages in thread From: Mark Rutland @ 2015-07-24 10:07 UTC (permalink / raw) To: yalin wang Cc: lauraa@codeaurora.org, Marc Zyngier, Catalin Marinas, Punit Agrawal, Will Deacon, linux-kernel@vger.kernel.org, zlim.lnx@gmail.com, wcohen@redhat.com, linux-arm-kernel@lists.infradead.org On Fri, Jul 24, 2015 at 10:56:18AM +0100, Mark Rutland wrote: > On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: > > > > > On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > > On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: > > >> A little change to patch_map() function, > > >> use set_fixmap_offset() to make code more clear. > > >> > > >> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > > >> --- > > >> arch/arm64/kernel/insn.c | 5 ++--- > > >> 1 file changed, 2 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > >> index dd9671c..7dafd5a 100644 > > >> --- a/arch/arm64/kernel/insn.c > > >> +++ b/arch/arm64/kernel/insn.c > > >> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > > >> return addr; > > >> > > >> BUG_ON(!page); > > >> - set_fixmap(fixmap, page_to_phys(page)); > > >> - > > >> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > > >> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > > >> + (addr & ~PAGE_MASK)); > > > > > > It looks fine. Do you get any compiler warning for the automatic pointer > > > to long conversion? You may want to add some explicit casts, otherwise: > > > > > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > > i have build it, there is no warning about this change. :) > > I see no warnings with defconfig, but there's an (unrelated) set of warnings if > CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: Actually they aren't unrelated after all. the unsigned long addr in __set_fixmap_offset shadows the void* addr from patch_map making the types match. With a few underscores addrd to __set_fixmap_offset's addr, I get the following regarding patch_map: ---- In file included from ./arch/arm64/include/asm/fixmap.h:85:0, from arch/arm64/kernel/insn.c:32: arch/arm64/kernel/insn.c: In function ‘patch_map’: arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ‘void *’ and ‘long unsigned int’) (addr & ~PAGE_MASK)); ^ include/asm-generic/fixmap.h:73:20: note: in definition of macro ‘__set_fixmap_offset’ __set_fixmap(idx, phys, flags); \ ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ‘set_fixmap_offset’ return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ‘void *’ and ‘long unsigned int’) (addr & ~PAGE_MASK)); ^ include/asm-generic/fixmap.h:74:32: note: in definition of macro ‘__set_fixmap_offset’ __addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ ^ arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ‘set_fixmap_offset’ return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + ^ make[1]: *** [arch/arm64/kernel/insn.o] Error 1 make: *** [arch/arm64/kernel/insn.o] Error 2 ---- we need both the cast in patch_map, and an update to __set_fixmap_offset to prevent the name clash. Thanks, Mark. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-24 10:07 ` Mark Rutland @ 2015-07-24 11:53 ` yalin wang -1 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-24 11:53 UTC (permalink / raw) To: linux-arm-kernel > ? 2015?7?24??18:07?Mark Rutland <mark.rutland@arm.com> ??? > > On Fri, Jul 24, 2015 at 10:56:18AM +0100, Mark Rutland wrote: >> On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: >>> >>>> On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: >>>> >>>> On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: >>>>> A little change to patch_map() function, >>>>> use set_fixmap_offset() to make code more clear. >>>>> >>>>> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> >>>>> --- >>>>> arch/arm64/kernel/insn.c | 5 ++--- >>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >>>>> index dd9671c..7dafd5a 100644 >>>>> --- a/arch/arm64/kernel/insn.c >>>>> +++ b/arch/arm64/kernel/insn.c >>>>> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) >>>>> return addr; >>>>> >>>>> BUG_ON(!page); >>>>> - set_fixmap(fixmap, page_to_phys(page)); >>>>> - >>>>> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); >>>>> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + >>>>> + (addr & ~PAGE_MASK)); >>>> >>>> It looks fine. Do you get any compiler warning for the automatic pointer >>>> to long conversion? You may want to add some explicit casts, otherwise: >>>> >>>> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >>> i have build it, there is no warning about this change. :) >> >> I see no warnings with defconfig, but there's an (unrelated) set of warnings if >> CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: > > Actually they aren't unrelated after all. the unsigned long addr in > __set_fixmap_offset shadows the void* addr from patch_map making the > types match. > > With a few underscores addrd to __set_fixmap_offset's addr, I get the > following regarding patch_map: > > ---- > In file included from ./arch/arm64/include/asm/fixmap.h:85:0, > from arch/arm64/kernel/insn.c:32: > arch/arm64/kernel/insn.c: In function ?patch_map?: > arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ?void *? and ?long unsigned int?) > (addr & ~PAGE_MASK)); > ^ > include/asm-generic/fixmap.h:73:20: note: in definition of macro ?__set_fixmap_offset? > __set_fixmap(idx, phys, flags); \ > ^ > arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ?set_fixmap_offset? > return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > ^ > arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ?void *? and ?long unsigned int?) > (addr & ~PAGE_MASK)); > ^ > include/asm-generic/fixmap.h:74:32: note: in definition of macro ?__set_fixmap_offset? > __addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ > ^ > arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ?set_fixmap_offset? > return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > ^ > make[1]: *** [arch/arm64/kernel/insn.o] Error 1 > make: *** [arch/arm64/kernel/insn.o] Error 2 > ---- > > we need both the cast in patch_map, and an update to __set_fixmap_offset to > prevent the name clash. > > Thanks, > Mark. i see, i will send V2 to solve this. Thanks ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-24 11:53 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-24 11:53 UTC (permalink / raw) To: Mark Rutland Cc: yalin wang, lauraa@codeaurora.org, Marc Zyngier, Catalin Marinas, Punit Agrawal, Will Deacon, linux-kernel@vger.kernel.org, zlim.lnx@gmail.com, wcohen@redhat.com, linux-arm-kernel@lists.infradead.org > 在 2015年7月24日，18:07，Mark Rutland <mark.rutland@arm.com> 写道： > > On Fri, Jul 24, 2015 at 10:56:18AM +0100, Mark Rutland wrote: >> On Fri, Jul 24, 2015 at 04:56:59AM +0100, yalin wang wrote: >>> >>>> On Jul 23, 2015, at 21:03, Catalin Marinas <catalin.marinas@arm.com> wrote: >>>> >>>> On Thu, Jul 23, 2015 at 07:45:53PM +0800, yalin wang wrote: >>>>> A little change to patch_map() function, >>>>> use set_fixmap_offset() to make code more clear. >>>>> >>>>> Signed-off-by: yalin wang <yalin.wang2010@gmail.com> >>>>> --- >>>>> arch/arm64/kernel/insn.c | 5 ++--- >>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >>>>> index dd9671c..7dafd5a 100644 >>>>> --- a/arch/arm64/kernel/insn.c >>>>> +++ b/arch/arm64/kernel/insn.c >>>>> @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) >>>>> return addr; >>>>> >>>>> BUG_ON(!page); >>>>> - set_fixmap(fixmap, page_to_phys(page)); >>>>> - >>>>> - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); >>>>> + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + >>>>> + (addr & ~PAGE_MASK)); >>>> >>>> It looks fine. Do you get any compiler warning for the automatic pointer >>>> to long conversion? You may want to add some explicit casts, otherwise: >>>> >>>> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >>> i have build it, there is no warning about this change. :) >> >> I see no warnings with defconfig, but there's an (unrelated) set of warnings if >> CONFIG_DEBUG_SET_MODULE_RONX or CONFIG_DEBUG_RODATA are enabled: > > Actually they aren't unrelated after all. the unsigned long addr in > __set_fixmap_offset shadows the void* addr from patch_map making the > types match. > > With a few underscores addrd to __set_fixmap_offset's addr, I get the > following regarding patch_map: > > ---- > In file included from ./arch/arm64/include/asm/fixmap.h:85:0, > from arch/arm64/kernel/insn.c:32: > arch/arm64/kernel/insn.c: In function ‘patch_map’: > arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ‘void *’ and ‘long unsigned int’) > (addr & ~PAGE_MASK)); > ^ > include/asm-generic/fixmap.h:73:20: note: in definition of macro ‘__set_fixmap_offset’ > __set_fixmap(idx, phys, flags); \ > ^ > arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ‘set_fixmap_offset’ > return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > ^ > arch/arm64/kernel/insn.c:105:10: error: invalid operands to binary & (have ‘void *’ and ‘long unsigned int’) > (addr & ~PAGE_MASK)); > ^ > include/asm-generic/fixmap.h:74:32: note: in definition of macro ‘__set_fixmap_offset’ > __addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \ > ^ > arch/arm64/kernel/insn.c:104:17: note: in expansion of macro ‘set_fixmap_offset’ > return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > ^ > make[1]: *** [arch/arm64/kernel/insn.o] Error 1 > make: *** [arch/arm64/kernel/insn.o] Error 2 > ---- > > we need both the cast in patch_map, and an update to __set_fixmap_offset to > prevent the name clash. > > Thanks, > Mark. i see, i will send V2 to solve this. Thanks ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC] arm64:use set_fixmap_offset to make it more clear 2015-07-23 11:45 ` yalin wang @ 2015-07-23 15:29 ` Laura Abbott -1 siblings, 0 replies; 19+ messages in thread From: Laura Abbott @ 2015-07-23 15:29 UTC (permalink / raw) To: linux-arm-kernel On 07/23/2015 04:45 AM, yalin wang wrote: > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..7dafd5a 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (addr & ~PAGE_MASK)); > } > > static void __kprobes patch_unmap(int fixmap) > Reviewed-by: Laura Abbott <labbott@redhat.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] arm64:use set_fixmap_offset to make it more clear @ 2015-07-23 15:29 ` Laura Abbott 0 siblings, 0 replies; 19+ messages in thread From: Laura Abbott @ 2015-07-23 15:29 UTC (permalink / raw) To: yalin wang, catalin.marinas, will.deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, linux-kernel On 07/23/2015 04:45 AM, yalin wang wrote: > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..7dafd5a 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (addr & ~PAGE_MASK)); > } > > static void __kprobes patch_unmap(int fixmap) > Reviewed-by: Laura Abbott <labbott@redhat.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC V2] arm64:use set_fixmap_offset to make it more clear 2015-07-23 11:45 ` yalin wang ` (2 preceding siblings ...) (?) @ 2015-07-24 11:52 ` yalin wang 2015-07-28 6:31 ` yalin wang -1 siblings, 1 reply; 19+ messages in thread From: yalin wang @ 2015-07-24 11:52 UTC (permalink / raw) To: catalin.marinas, will.deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, linux-kernel A little change to patch_map() function, use set_fixmap_offset() to make code more clear. Signed-off-by: yalin wang <yalin.wang2010@gmail.com> --- arch/arm64/kernel/insn.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index dd9671c..f341866 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) return addr; BUG_ON(!page); - set_fixmap(fixmap, page_to_phys(page)); - - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + + (uintaddr & ~PAGE_MASK)); } static void __kprobes patch_unmap(int fixmap) -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC V2] arm64:use set_fixmap_offset to make it more clear 2015-07-24 11:52 ` [RFC V2] " yalin wang @ 2015-07-28 6:31 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-28 6:31 UTC (permalink / raw) To: linux-arm-kernel > On Jul 24, 2015, at 19:52, yalin wang <yalin.wang2010@gmail.com> wrote: > > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..f341866 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (uintaddr & ~PAGE_MASK)); > } > > static void __kprobes patch_unmap(int fixmap) > -- > 1.9.1 > Marinas, this V2 patch can build without warning even CONFIG_DEBUG_SET_MODULE_RONX enabled, could you review it. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC V2] arm64:use set_fixmap_offset to make it more clear @ 2015-07-28 6:31 ` yalin wang 0 siblings, 0 replies; 19+ messages in thread From: yalin wang @ 2015-07-28 6:31 UTC (permalink / raw) To: Catalin Marinas Cc: Will Deacon, zlim.lnx, punit.agrawal, marc.zyngier, lauraa, wcohen, linux-arm-kernel, open list > On Jul 24, 2015, at 19:52, yalin wang <yalin.wang2010@gmail.com> wrote: > > A little change to patch_map() function, > use set_fixmap_offset() to make code more clear. > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > --- > arch/arm64/kernel/insn.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index dd9671c..f341866 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > return addr; > > BUG_ON(!page); > - set_fixmap(fixmap, page_to_phys(page)); > - > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > + (uintaddr & ~PAGE_MASK)); > } > > static void __kprobes patch_unmap(int fixmap) > -- > 1.9.1 > Marinas, this V2 patch can build without warning even CONFIG_DEBUG_SET_MODULE_RONX enabled, could you review it. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC V2] arm64:use set_fixmap_offset to make it more clear 2015-07-28 6:31 ` yalin wang @ 2015-07-28 8:47 ` Will Deacon -1 siblings, 0 replies; 19+ messages in thread From: Will Deacon @ 2015-07-28 8:47 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 28, 2015 at 07:31:20AM +0100, yalin wang wrote: > > > On Jul 24, 2015, at 19:52, yalin wang <yalin.wang2010@gmail.com> wrote: > > > > A little change to patch_map() function, > > use set_fixmap_offset() to make code more clear. > > > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > > --- > > arch/arm64/kernel/insn.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > index dd9671c..f341866 100644 > > --- a/arch/arm64/kernel/insn.c > > +++ b/arch/arm64/kernel/insn.c > > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > > return addr; > > > > BUG_ON(!page); > > - set_fixmap(fixmap, page_to_phys(page)); > > - > > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > > + (uintaddr & ~PAGE_MASK)); > > } > > > > static void __kprobes patch_unmap(int fixmap) > > -- > > 1.9.1 > > > Marinas, > this V2 patch can build without warning even CONFIG_DEBUG_SET_MODULE_RONX enabled, > could you review it. > Thanks. I queued this already for 4.3. There's a (benign) sparse warning about the addr parameter to patch_map being overridden by the local scope of set_fixmap_offset which could be solved with a healthy portion of underscores in the core code. Will ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC V2] arm64:use set_fixmap_offset to make it more clear @ 2015-07-28 8:47 ` Will Deacon 0 siblings, 0 replies; 19+ messages in thread From: Will Deacon @ 2015-07-28 8:47 UTC (permalink / raw) To: yalin wang Cc: Catalin Marinas, zlim.lnx@gmail.com, Punit Agrawal, Marc Zyngier, lauraa@codeaurora.org, wcohen@redhat.com, linux-arm-kernel@lists.infradead.org, open list On Tue, Jul 28, 2015 at 07:31:20AM +0100, yalin wang wrote: > > > On Jul 24, 2015, at 19:52, yalin wang <yalin.wang2010@gmail.com> wrote: > > > > A little change to patch_map() function, > > use set_fixmap_offset() to make code more clear. > > > > Signed-off-by: yalin wang <yalin.wang2010@gmail.com> > > --- > > arch/arm64/kernel/insn.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > index dd9671c..f341866 100644 > > --- a/arch/arm64/kernel/insn.c > > +++ b/arch/arm64/kernel/insn.c > > @@ -101,9 +101,8 @@ static void __kprobes *patch_map(void *addr, int fixmap) > > return addr; > > > > BUG_ON(!page); > > - set_fixmap(fixmap, page_to_phys(page)); > > - > > - return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK)); > > + return (void *)set_fixmap_offset(fixmap, page_to_phys(page) + > > + (uintaddr & ~PAGE_MASK)); > > } > > > > static void __kprobes patch_unmap(int fixmap) > > -- > > 1.9.1 > > > Marinas, > this V2 patch can build without warning even CONFIG_DEBUG_SET_MODULE_RONX enabled, > could you review it. > Thanks. I queued this already for 4.3. There's a (benign) sparse warning about the addr parameter to patch_map being overridden by the local scope of set_fixmap_offset which could be solved with a healthy portion of underscores in the core code. Will ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-07-28 8:47 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-23 11:45 [RFC] arm64:use set_fixmap_offset to make it more clear yalin wang 2015-07-23 11:45 ` yalin wang 2015-07-23 13:03 ` Catalin Marinas 2015-07-23 13:03 ` Catalin Marinas 2015-07-24 3:56 ` yalin wang 2015-07-24 3:56 ` yalin wang 2015-07-24 9:56 ` Mark Rutland 2015-07-24 9:56 ` Mark Rutland 2015-07-24 10:07 ` Mark Rutland 2015-07-24 10:07 ` Mark Rutland 2015-07-24 11:53 ` yalin wang 2015-07-24 11:53 ` yalin wang 2015-07-23 15:29 ` Laura Abbott 2015-07-23 15:29 ` Laura Abbott 2015-07-24 11:52 ` [RFC V2] " yalin wang 2015-07-28 6:31 ` yalin wang 2015-07-28 6:31 ` yalin wang 2015-07-28 8:47 ` Will Deacon 2015-07-28 8:47 ` Will Deacon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.