From: "kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com> To: Robin Murphy <robin.murphy@arm.com>, Russell King - ARM Linux <linux@armlinux.org.uk>, Catalin Marinas <catalin.marinas@arm.com>, "Will Deacon" <will.deacon@arm.com>, Mark Rutland <mark.rutland@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Cc: "hyunchul3.kim@sk.com" <hyunchul3.kim@sk.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "woosuk.chung@sk.com" <woosuk.chung@sk.com> Subject: RE: [PATCH v2] arm64: mm: convert __dma_* routines to use start, size Date: Thu, 28 Jul 2016 00:08:09 +0000 [thread overview] Message-ID: <b0d9ef6b59364c4587f8d7c2493160ed@nmail01.hynixad.com> (raw) In-Reply-To: <c355debc-0238-ff51-ca90-6e0c8085366b@arm.com> > -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: Wednesday, July 27, 2016 6:56 PM > To: 이광우(LEE KWANGWOO) MS SW; Russell King - ARM Linux; Catalin Marinas; Will Deacon; Mark Rutland; > linux-arm-kernel@lists.infradead.org > Cc: 김현철(KIM HYUNCHUL) MS SW; linux-kernel@vger.kernel.org; 정우석(CHUNG WOO SUK) MS SW > Subject: Re: [PATCH v2] arm64: mm: convert __dma_* routines to use start, size > > On 27/07/16 02:55, kwangwoo.lee@sk.com wrote: > [...] > >>> /* > >>> - * __dma_clean_range(start, end) > >>> + * __dma_clean_area(start, size) > >>> * - start - virtual start address of region > >>> - * - end - virtual end address of region > >>> + * - size - size in question > >>> */ > >>> -__dma_clean_range: > >>> - dcache_line_size x2, x3 > >>> - sub x3, x2, #1 > >>> - bic x0, x0, x3 > >>> -1: > >>> +__dma_clean_area: > >>> alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE > >>> - dc cvac, x0 > >>> + dcache_by_line_op cvac, sy, x0, x1, x2, x3 > >>> alternative_else > >>> - dc civac, x0 > >>> + dcache_by_line_op civac, sy, x0, x1, x2, x3 > >> > >> dcache_by_line_op is a relatively large macro - is there any way we can > >> still apply the alternative to just the one instruction which needs it, > >> as opposed to having to patch the entire mostly-identical routine? > > > > I agree with your opinion. Then, how do you think about using CONFIG_* options > > like below? I think that alternative_* macros seems to keep the space for > > unused instruction. Is it necessary? Please, share your thought about the > > space. Thanks! > > > > +__dma_clean_area: > > +#if defined(CONFIG_ARM64_ERRATUM_826319) || \ > > + defined(CONFIG_ARM64_ERRATUM_827319) || \ > > + defined(CONFIG_ARM64_ERRATUM_824069) || \ > > + defined(CONFIG_ARM64_ERRATUM_819472) > > + dcache_by_line_op civac, sy, x0, x1, x2, x3 > > +#else > > + dcache_by_line_op cvac, sy, x0, x1, x2, x3 > > +#endif > > That's not ideal, because we still only really want to use the > workaround if we detect a CPU which needs it, rather than baking it in > at compile time. I was thinking more along the lines of pushing the > alternative down into dcache_by_line_op, something like the idea below > (compile-tested only, may not actually be viable). OK. Using the capability of CPU features seems to be preferred. > Robin. > > -----8<----- > diff --git a/arch/arm64/include/asm/assembler.h > b/arch/arm64/include/asm/assembler.h > index 10b017c4bdd8..1c005c90387e 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -261,7 +261,16 @@ lr .req x30 // link register > add \size, \kaddr, \size > sub \tmp2, \tmp1, #1 > bic \kaddr, \kaddr, \tmp2 > -9998: dc \op, \kaddr > +9998: > + .ifeqs "\op", "cvac" > +alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE > + dc cvac, \kaddr > +alternative_else > + dc civac, \kaddr > +alternative_endif > + .else > + dc \op, \kaddr > + .endif > add \kaddr, \kaddr, \tmp1 > cmp \kaddr, \size > b.lo 9998b I agree that it looks not viable because it makes the macro bigger and conditional specifically with CVAC op. Then.. if the number of the usage of alternative_* macros for erratum is few (just one in this case for cache clean), I think only small change like below seems to be optimal and there is no need to create a variant macro of dcache_cache_by_line_op. How do you think about it? /* - * __dma_clean_range(start, end) + * __clean_dcache_area_poc(kaddr, size) + * + * Ensure that any D-cache lines for the interval [kaddr, kaddr+size) + * are cleaned to the PoC. + * + * - kaddr - kernel address + * - size - size in question + */ +ENTRY(__clean_dcache_area_poc) + /* FALLTHROUGH */ + +/* + * __dma_clean_area(start, size) * - start - virtual start address of region - * - end - virtual end address of region + * - size - size in question */ -__dma_clean_range: +__dma_clean_area: + add x1, x1, x0 dcache_line_size x2, x3 sub x3, x2, #1 bic x0, x0, x3 @@ -158,24 +172,21 @@ alternative_endif b.lo 1b dsb sy ret -ENDPROC(__dma_clean_range) +ENDPIPROC(__clean_dcache_area_poc) +ENDPROC(__dma_clean_area) Regards, Kwangwoo Lee
WARNING: multiple messages have this Message-ID (diff)
From: kwangwoo.lee@sk.com (kwangwoo.lee at sk.com) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] arm64: mm: convert __dma_* routines to use start, size Date: Thu, 28 Jul 2016 00:08:09 +0000 [thread overview] Message-ID: <b0d9ef6b59364c4587f8d7c2493160ed@nmail01.hynixad.com> (raw) In-Reply-To: <c355debc-0238-ff51-ca90-6e0c8085366b@arm.com> > -----Original Message----- > From: Robin Murphy [mailto:robin.murphy at arm.com] > Sent: Wednesday, July 27, 2016 6:56 PM > To: ???(LEE KWANGWOO) MS SW; Russell King - ARM Linux; Catalin Marinas; Will Deacon; Mark Rutland; > linux-arm-kernel at lists.infradead.org > Cc: ???(KIM HYUNCHUL) MS SW; linux-kernel at vger.kernel.org; ???(CHUNG WOO SUK) MS SW > Subject: Re: [PATCH v2] arm64: mm: convert __dma_* routines to use start, size > > On 27/07/16 02:55, kwangwoo.lee at sk.com wrote: > [...] > >>> /* > >>> - * __dma_clean_range(start, end) > >>> + * __dma_clean_area(start, size) > >>> * - start - virtual start address of region > >>> - * - end - virtual end address of region > >>> + * - size - size in question > >>> */ > >>> -__dma_clean_range: > >>> - dcache_line_size x2, x3 > >>> - sub x3, x2, #1 > >>> - bic x0, x0, x3 > >>> -1: > >>> +__dma_clean_area: > >>> alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE > >>> - dc cvac, x0 > >>> + dcache_by_line_op cvac, sy, x0, x1, x2, x3 > >>> alternative_else > >>> - dc civac, x0 > >>> + dcache_by_line_op civac, sy, x0, x1, x2, x3 > >> > >> dcache_by_line_op is a relatively large macro - is there any way we can > >> still apply the alternative to just the one instruction which needs it, > >> as opposed to having to patch the entire mostly-identical routine? > > > > I agree with your opinion. Then, how do you think about using CONFIG_* options > > like below? I think that alternative_* macros seems to keep the space for > > unused instruction. Is it necessary? Please, share your thought about the > > space. Thanks! > > > > +__dma_clean_area: > > +#if defined(CONFIG_ARM64_ERRATUM_826319) || \ > > + defined(CONFIG_ARM64_ERRATUM_827319) || \ > > + defined(CONFIG_ARM64_ERRATUM_824069) || \ > > + defined(CONFIG_ARM64_ERRATUM_819472) > > + dcache_by_line_op civac, sy, x0, x1, x2, x3 > > +#else > > + dcache_by_line_op cvac, sy, x0, x1, x2, x3 > > +#endif > > That's not ideal, because we still only really want to use the > workaround if we detect a CPU which needs it, rather than baking it in > at compile time. I was thinking more along the lines of pushing the > alternative down into dcache_by_line_op, something like the idea below > (compile-tested only, may not actually be viable). OK. Using the capability of CPU features seems to be preferred. > Robin. > > -----8<----- > diff --git a/arch/arm64/include/asm/assembler.h > b/arch/arm64/include/asm/assembler.h > index 10b017c4bdd8..1c005c90387e 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -261,7 +261,16 @@ lr .req x30 // link register > add \size, \kaddr, \size > sub \tmp2, \tmp1, #1 > bic \kaddr, \kaddr, \tmp2 > -9998: dc \op, \kaddr > +9998: > + .ifeqs "\op", "cvac" > +alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE > + dc cvac, \kaddr > +alternative_else > + dc civac, \kaddr > +alternative_endif > + .else > + dc \op, \kaddr > + .endif > add \kaddr, \kaddr, \tmp1 > cmp \kaddr, \size > b.lo 9998b I agree that it looks not viable because it makes the macro bigger and conditional specifically with CVAC op. Then.. if the number of the usage of alternative_* macros for erratum is few (just one in this case for cache clean), I think only small change like below seems to be optimal and there is no need to create a variant macro of dcache_cache_by_line_op. How do you think about it? /* - * __dma_clean_range(start, end) + * __clean_dcache_area_poc(kaddr, size) + * + * Ensure that any D-cache lines for the interval [kaddr, kaddr+size) + * are cleaned to the PoC. + * + * - kaddr - kernel address + * - size - size in question + */ +ENTRY(__clean_dcache_area_poc) + /* FALLTHROUGH */ + +/* + * __dma_clean_area(start, size) * - start - virtual start address of region - * - end - virtual end address of region + * - size - size in question */ -__dma_clean_range: +__dma_clean_area: + add x1, x1, x0 dcache_line_size x2, x3 sub x3, x2, #1 bic x0, x0, x3 @@ -158,24 +172,21 @@ alternative_endif b.lo 1b dsb sy ret -ENDPROC(__dma_clean_range) +ENDPIPROC(__clean_dcache_area_poc) +ENDPROC(__dma_clean_area) Regards, Kwangwoo Lee
next prev parent reply other threads:[~2016-07-28 0:08 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-26 7:34 [PATCH v2] arm64: mm: convert __dma_* routines to use start, size Kwangwoo Lee 2016-07-26 7:34 ` Kwangwoo Lee 2016-07-26 10:43 ` Robin Murphy 2016-07-26 10:43 ` Robin Murphy 2016-07-27 1:55 ` kwangwoo.lee 2016-07-27 1:55 ` kwangwoo.lee at sk.com 2016-07-27 9:56 ` Robin Murphy 2016-07-27 9:56 ` Robin Murphy 2016-07-28 0:08 ` kwangwoo.lee [this message] 2016-07-28 0:08 ` kwangwoo.lee at sk.com 2016-07-29 17:06 ` Robin Murphy 2016-07-29 17:06 ` Robin Murphy 2016-07-31 23:45 ` kwangwoo.lee 2016-07-31 23:45 ` kwangwoo.lee at sk.com 2016-08-01 13:36 ` Robin Murphy 2016-08-01 13:36 ` Robin Murphy 2016-08-01 13:53 ` Robin Murphy 2016-08-01 13:53 ` Robin Murphy 2016-08-01 23:24 ` kwangwoo.lee 2016-08-01 23:24 ` kwangwoo.lee at sk.com
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b0d9ef6b59364c4587f8d7c2493160ed@nmail01.hynixad.com \ --to=kwangwoo.lee@sk.com \ --cc=catalin.marinas@arm.com \ --cc=hyunchul3.kim@sk.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=robin.murphy@arm.com \ --cc=will.deacon@arm.com \ --cc=woosuk.chung@sk.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.