[GIT PULL] drm/arc: Minor improvements
Hi Dave, Daniel, The following changes since commit 7aaddd96d5febcf5b24357a326b3038d49a20532: drm/modes: Don't apply cmdline's rotation if it wasn't specified (2019-07-16 10:34:38 +0200) are available in the Git repository at: g...@github.com:abrodkin/linux.git tags/arcpgu-updates-2019.07.18 for you to fetch changes up to cee17a71656e9e1b5ffc01767844026550d5f4a9: drm/arcpgu: rework encoder search (2019-07-17 23:36:56 +0300) This is a pretty simple improvement that allows to find encoder as the one and only (ARC PGU doesn't support more than one) endpoint instead of using non-standard "encoder-slave" property. Eugeniy Paltsev (1): drm/arcpgu: rework encoder search drivers/gpu/drm/arc/arcpgu_drv.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) Regards, Alexey ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: ARCv2: jump label: implement jump label patching
On Wed, 2019-07-17 at 17:45 +, Vineet Gupta wrote:
> On 7/17/19 8:09 AM, Eugeniy Paltsev wrote:
> > > > +/* Halt system on fatal error to make debug easier */
> > > > +#define arc_jl_fatal(format...)
> > > > \
> > > > +({
> > > > \
> > > > + pr_err(JUMPLABEL_ERR format);
> > > > \
> > > > + BUG();
> > > > \
> > > Does it make sense to bring down the whole system vs. failing and
> > > returning.
> > > I see there is no error propagation to core code, but still.
> > I totally agree with Peter, and I prefer to stop the system on this errors.
> > Here is few arguments:
> > All this checks can't be toggle in system operating normally and only may
> > be caused by bad code generation (or some code/data corruption):
> > 1) We got our instruction to patch unaligned by 4 bytes (despite the fact
> > that we used '.balign 4' to align it)
> > 2) We got branch offset unaligned (which means that we calculate it wrong
> > at build time or corrupt it in run time)
> > 3) We got branch offset which not fits into s25. As this is offset inside
> > one function (inside one 'if' statement actually) we newer get such huge
> > offset in real life if code generation is ok.
>
> I understand that. But AFAIKR in implementation arc_jl_fatal() gets called
> before
> we have done the actual code patching and/or flushing the caches
Correct.
> to that effect.
> So harm has been done just yet. We just need to make sure that any
> book-keeping of
> true/false is not yet done or unrolled.
Can you describe your proposal in more detail?
Be noted that in case (2) or (3) we know that we are not able to generate
correct instruction for replace existing one. And we don't know anything
about the real cause.
So what we can do here besides that we warn about it?
-We can halt the system to make debug easy (which I propose)
-We can generate invalid instruction.
-We can just skip this patching. In that case we'll end with some 'if'
statements [in kernel code] working incorrectly. I really don't want to debug
this.
Given that we'll never face with this asserts in any consistent state I don't
see any reason why we shouldn't simply call BUG() here.
>
> > If we only warn to log and return we will face with compromised kernel flow
> > later.
> > I.E. it could be 'if' statement in kernel text which is switched to wrong
> > state: the condition is true but we take the false branch.
> > And It will be the issue which is much more difficult to debug.
> >
> > Does it sound reasonably?
>
> I'm still not convinced that by hitting the _fatal() we are in some
> inconsistent
> state already. But if u feel strongly lets keep it.
>
> > If you really don't want to have BUG here, I can make it conditionally
> > enabled
>
> Not a good idea. It is unconditionally present or not. Not something in
> between.
>
> > in depend on CONFIG_ARC_STATIC_KEYS_DEBUG as I want to have it enabled at
> > least on ARC development platforms.
>
>
--
Eugeniy Paltsev
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARCv2: Don't pretend we may set U & DE bits in STATUS32 with kflag
On 7/16/19 1:51 PM, Alexey Brodkin wrote:
> As per PRM "kflag" instruction doesn't change state of
> DE-flag ("Delayed branch is pending") and U-flag ("User mode")
> in STATUS32 register so let's not act as if we can affect those bits.
I understand the motivation and indeed bits not writable by kflag should be
removed. But what if we do need to clear those bits out from status32 (assuming
they exist there in first place) and that kflag might not be the right
instruction
to do that.
So the question to ask is do we need to clear U and DE bits from status32 and
answer from reading the PRM is no, we don't have to, as those are cleared
already
when an exception is taken.
The likely reason this code is because back in arc700 days we used to have a
different version of this macro which atleast in original code relied on using
the
pre-exception status32 (in pt_regs) - and that could easily have U and/or DE set
based hence needed clearing.
.macro FAKE_RET_FROM_EXCPN reg
ld \reg, [sp, PT_status32]
and \reg, \reg, ~(STATUS_U_MASK|STATUS_DE_MASK)
or \reg, \reg, STATUS_L_MASK
sr \reg, [erstatus]
mov \reg, 55f
sr \reg, [eret]
rtie
55:
.endm
This is not needed (even in arc700) if we are using the current "in-exception"
status32 for doing the early return.
Long story short, your patch is correct but we need to explain better why it is.
I've applied it locally with slight tweak to changelog to that effect.
> Signed-off-by: Alexey Brodkin
> ---
> arch/arc/include/asm/entry-arcv2.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arc/include/asm/entry-arcv2.h
> b/arch/arc/include/asm/entry-arcv2.h
> index 225e7df2d8ed..6558e2edb583 100644
> --- a/arch/arc/include/asm/entry-arcv2.h
> +++ b/arch/arc/include/asm/entry-arcv2.h
> @@ -237,7 +237,7 @@
>
> .macro FAKE_RET_FROM_EXCPN
> lr r9, [status32]
> - bic r9, r9, (STATUS_U_MASK|STATUS_DE_MASK|STATUS_AE_MASK)
> + bic r9, r9, STATUS_AE_MASK
> or r9, r9, STATUS_IE_MASK
> kflag r9
> .endm
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: ARCv2: jump label: implement jump label patching
On 7/17/19 8:09 AM, Eugeniy Paltsev wrote:
>>> +/* Halt system on fatal error to make debug easier */
>>> +#define arc_jl_fatal(format...)
>>> \
>>> +({ \
>>> + pr_err(JUMPLABEL_ERR format); \
>>> + BUG(); \
>> Does it make sense to bring down the whole system vs. failing and returning.
>> I see there is no error propagation to core code, but still.
> I totally agree with Peter, and I prefer to stop the system on this errors.
> Here is few arguments:
> All this checks can't be toggle in system operating normally and only may be
> caused by bad code generation (or some code/data corruption):
> 1) We got our instruction to patch unaligned by 4 bytes (despite the fact
> that we used '.balign 4' to align it)
> 2) We got branch offset unaligned (which means that we calculate it wrong at
> build time or corrupt it in run time)
> 3) We got branch offset which not fits into s25. As this is offset inside one
> function (inside one 'if' statement actually) we newer get such huge
> offset in real life if code generation is ok.
I understand that. But AFAIKR in implementation arc_jl_fatal() gets called
before
we have done the actual code patching and/or flushing the caches to that effect.
So harm has been done just yet. We just need to make sure that any book-keeping
of
true/false is not yet done or unrolled.
> If we only warn to log and return we will face with compromised kernel flow
> later.
> I.E. it could be 'if' statement in kernel text which is switched to wrong
> state: the condition is true but we take the false branch.
> And It will be the issue which is much more difficult to debug.
>
> Does it sound reasonably?
I'm still not convinced that by hitting the _fatal() we are in some inconsistent
state already. But if u feel strongly lets keep it.
>
> If you really don't want to have BUG here, I can make it conditionally enabled
Not a good idea. It is unconditionally present or not. Not something in between.
> in depend on CONFIG_ARC_STATIC_KEYS_DEBUG as I want to have it enabled at
> least on ARC development platforms.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: ARCv2: jump label: implement jump label patching
Hi Vineet,
I'm finally back, so see my comments below.
On Tue, 2019-06-18 at 16:16 +, Vineet Gupta wrote:
> On 6/14/19 9:41 AM, Eugeniy Paltsev wrote:
> > Implement jump label patching for ARC. Jump labels provide
> > an interface to generate dynamic branches using
> > self-modifying code.
> >
> > This allows us to implement conditional branches where
> > changing branch direction is expensive but branch selection
> > is basically 'free'
> >
> > This implementation uses 32-bit NOP and BRANCH instructions
> > which forced to be aligned by 4 to guarantee that they don't
> > cross L1 cache line and can be update atomically.
> >
> > Signed-off-by: Eugeniy Paltsev
>
> LGTM overall - nits below.
>
> > ---
> > arch/arc/Kconfig | 8 ++
> > arch/arc/include/asm/jump_label.h | 68
> > arch/arc/kernel/Makefile | 1 +
> > arch/arc/kernel/jump_label.c | 168 ++
> > 4 files changed, 245 insertions(+)
> > create mode 100644 arch/arc/include/asm/jump_label.h
> > create mode 100644 arch/arc/kernel/jump_label.c
> >
> > diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
> > index c781e45d1d99..b1313e016c54 100644
> > --- a/arch/arc/Kconfig
> > +++ b/arch/arc/Kconfig
> > @@ -47,6 +47,7 @@ config ARC
> > select OF_EARLY_FLATTREE
> > select PCI_SYSCALL if PCI
> > select PERF_USE_VMALLOC if ARC_CACHE_VIPT_ALIASING
> > + select HAVE_ARCH_JUMP_LABEL if ISA_ARCV2 && !CPU_ENDIAN_BE32
> >
> > config ARCH_HAS_CACHE_LINE_SIZE
> > def_bool y
> > @@ -529,6 +530,13 @@ config ARC_DW2_UNWIND
> > config ARC_DBG_TLB_PARANOIA
> > bool "Paranoia Checks in Low Level TLB Handlers"
> >
> > +config ARC_DBG_STATIC_KEYS
> > + bool "Paranoid checks in Static Keys code"
> > + depends on JUMP_LABEL
> > + select STATIC_KEYS_SELFTEST
> > + help
> > + Enable paranoid checks and self-test of both ARC-specific and generic
> > + part of static-keys-related code.
>
> Why can't we just enable this if STATIC_KEYS_SELFTEST
As we extent STATIC_KEYS_SELFTEST option such dependency looks more reasonable
for me
(we enable our checks -> lets enable corresponding generic ones)
I don't insist, though.
> > endif
> >
> > config ARC_BUILTIN_DTB_NAME
> > diff --git a/arch/arc/include/asm/jump_label.h
> > b/arch/arc/include/asm/jump_label.h
> > new file mode 100644
> > index ..8971d0608f2c
> > --- /dev/null
> > +++ b/arch/arc/include/asm/jump_label.h
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_ARC_JUMP_LABEL_H
> > +#define _ASM_ARC_JUMP_LABEL_H
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include
> > +
> > +#define JUMP_LABEL_NOP_SIZE 4
> > +
> > +/*
> > + * To make atomic update of patched instruction available we need to
> > guarantee
> > + * that this instruction doesn't cross L1 cache line boundary.
> > + *
> > + * As of today we simply align instruction which can be patched by 4 byte
> > using
> > + * ".balign 4" directive. In that case patched instruction is aligned with
> > one
> > + * 16-bit NOP_S if this is required.
> > + * However 'align by 4' directive is much stricter than it actually
> > required.
> > + * It's enough that our 32-bit instruction don't cross l1 cache line
> > boundary
> > + * which can be achieved by using ".bundle_align_mode" directive. That
> > will save
> > + * us from adding useless NOP_S padding in most of the cases.
> > + *
> > + * TODO: switch to ".bundle_align_mode" directive using whin it will be
> > + * supported by ARC toolchain.
> > + */
> > +
>
> So block comments on top of a function imply summary of function etc.
> What you are doing here is calling out the need for .balign quirk. So better
> to
> phrase it is as "Note about .balign" and then describe the thing
Ok, will fix in v2.
> > +static __always_inline bool arch_static_branch(struct static_key *key,
> > + bool branch)
> > +{
> > + asm_volatile_goto(".balign 4\n"
> > +"1:\n"
> > +"nop \n"
> > +".pushsection __jump_table, \"aw\" \n"
> > +".word 1b, %l[l_yes], %c0 \n"
> > +".popsection \n"
> > +: : "i" (&((char *)key)[branch]) : : l_yes);
> > +
> > + return false;
> > +l_yes:
> > + return true;
> > +}
> > +
> > +static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > + bool branch)
> > +{
> > + asm_volatile_goto(".balign 4\n"
> > +"1:\n"
> > +"b %l[l_yes] \n"
> > +".pushsection __jump_table, \"aw\" \n"
> > +".word 1b, %l[l_yes], %c0 \n"
> > +".popsection \n"
> > +: :
