* Re: [PATCH] arm64: Allow disabling of the compat vDSO
From: Nick Desaulniers @ 2019-09-26 16:40 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ard Biesheuvel, LKML, Thomas Gleixner, Vincenzo Frascino,
Will Deacon, Linux ARM
In-Reply-To: <20190926155147.GL9689@arrakis.emea.arm.com>
On Thu, Sep 26, 2019 at 8:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Sep 26, 2019 at 08:47:18AM +0100, Catalin Marinas wrote:
> > On Thu, Sep 26, 2019 at 01:06:50AM +0100, Vincenzo Frascino wrote:
> > > On 9/25/19 6:08 PM, Catalin Marinas wrote:
> > > > On Wed, Sep 25, 2019 at 09:53:16AM -0700, Nick Desaulniers wrote:
> > > >> On Wed, Sep 25, 2019 at 6:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > >>> - clean up the headers includes; vDSO should not include kernel-only
> > > >>> headers that may even contain code patched at run-time
> > > >>
> > > >> This is a big one; Clang validates the inline asm constraints for
> > > >> extended inline assembly, GCC does not for dead code. So Clang chokes
> > > >> on the inclusion of arm64 headers using extended inline assembly when
> > > >> being compiled for arm-linux-gnueabi.
This case is very much real (not sure if Vincenzo was asking me or
Catalin), see report at the bottom of this comment:
https://github.com/ClangBuiltLinux/linux/issues/595#issuecomment-509874891
> > > >
> > > > Whether clang or gcc, I'd like this fixed anyway. At some point we may
> > > > inadvertently rely on some code which is patched at boot time for the
> > > > kernel code but not for the vDSO.
> > >
> > > Do we have any code of this kind in header files?
> > >
> > > The vDSO library uses only a subset of the headers (mainly Macros) hence all the
> > > unused symbols should be compiled out. Is your concern only theoretical or do
> > > you have an example on where this could be happening?
> >
> > At the moment it's rather theoretical.
>
> Actually, it's not. The moment the compat vdso Makefile needs the line
> below, we are doing it wrong:
>
> VDSO_CFLAGS += -D__uint128_t='void*'
*yikes*
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: Allow disabling of the compat vDSO
From: Nick Desaulniers @ 2019-09-26 16:38 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ard Biesheuvel, LKML, Thomas Gleixner, Vincenzo Frascino,
Will Deacon, Linux ARM
In-Reply-To: <20190926074717.GA26802@iMac.local>
On Thu, Sep 26, 2019 at 12:47 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Thu, Sep 26, 2019 at 01:06:50AM +0100, Vincenzo Frascino wrote:
> > On 9/25/19 6:08 PM, Catalin Marinas wrote:
> > > On Wed, Sep 25, 2019 at 09:53:16AM -0700, Nick Desaulniers wrote:
> > >> On Wed, Sep 25, 2019 at 6:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > >>> Suggestions for future improvements of the compat vDSO handling:
> > >>>
> > >>> - replace the CROSS_COMPILE_COMPAT prefix with a full COMPATCC; maybe
> > >>> check that it indeed produces 32-bit code
> >
> > CROSS_COMPILE_COMPAT is called like this for symmetry with CROSS_COMPILE.
>
> Actually, what gets in the way is the CONFIG_CROSS_COMPILE_COMPAT_VDSO.
> We can keep CROSS_COMPILE_COMPAT together with COMPATCC initialised to
> $(CROSS_COMPILE_COMPAT)gcc. When we will be able to build the compat
> vDSO with clang, we just pass COMPATCC=clang on the make line and the
> kernel Makefile will figure out the --target option from
> CROSS_COMPILE_COMPAT (see how CLANG_FLAGS is handled).
>
> If we stick only to env variables or make cmd line (without Kconfig) for
> the compiler name, we can add a COMPATCC_IS_CLANG in the Kconfig
> directly and simply not allow the enabling the COMPAT_VDSO if we don't
> have the right compiler. This saves us warnings during build.
Yes, I think this would be a nice approach.
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Linus Torvalds @ 2019-09-26 16:35 UTC (permalink / raw)
To: Pascal Van Leeuwen
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Will Deacon, David Miller, Linux ARM
In-Reply-To: <CH2PR20MB29680F87B32BBF0495720172CA860@CH2PR20MB2968.namprd20.prod.outlook.com>
On Thu, Sep 26, 2019 at 2:40 AM Pascal Van Leeuwen
<pvanleeuwen@verimatrix.com> wrote:
>
> While I agree with the principle of first merging Wireguard without
> hooking it up to the Crypto API and doing the latter in a later,
> separate patch, I DONT'T agree with your bashing of the Crypto API
> or HW crypto acceleration in general.
I'm not bashing hardware crypto acceleration.
But I *am* bashing bad interfaces to it.
Honestly, you need to face a few facts, Pascal.
Really.
First off: availability.
(a) hardware doesn't exist right now outside your lab
This is a fact.
(b) hardware simply WILL NOT exist in any huge number for years,
possibly decades. If ever,
This is just reality, Pascal. Even if you release your hardware
tomorrow, where do you think it will exists? Laptops? PC's? Phones?
No. No. And no.
Phones are likely the strongest argument for power use, but phones
won't really start using it until it's on the SoC, because while they
care deeply about power, they care even more deeply about a lot of
other things a whole lot more. Form factor, price, and customers that
care.
So phones aren't happening. Not for years, and not until it's a big
deal and standard IP that everybody wants.
Laptops and PC's? No. Look at all the crypto acceleration they have today.
That was sarcasm, btw, just to make it clear. It's simply not a market.
End result: even with hardware, the hardware will be very rare. Maybe
routers that want to sell particular features in the near future.
Again, this isn't theory. This is that pesky little thing called
"reality". It sucks, I know.
But even if you *IGNORE* the fact that hardware doesn't exist right
now, and won't be widely available for years (or longer), there's
another little fact that you are ignoring:
The async crypto interfaces are badly designed. Full stop.
Seriously. This isn't rocket science. This is very very basic Computer
Science 101.
Tell me, what's _the_ most important part about optimizing something?
Yeah, it's "you optimize for the common case". But let's ignore that
part, since I already said that hardware isn't the common case but I
promised that I'd ignore that part.
The _other_ most important part of optimization is that you optimize
for the case that _matters_.
And the async crypto case got that completely wrong, and the wireguard
patch shows that issue very clearly.
The fact is, even you admit that a few CPU cycles don't matter for the
async case where you have hardware acceleration, because the real cost
is going to be in the IO - whether it's DMA, cache coherent
interconnects, or some day some SoC special bus. The CPU cycles just
don't matter, they are entirely in the noise.
What does that mean? Think about it.
[ Time passes ]
Ok, I'll tell you: it means that the interface shouldn't be designed
for async hw-assisted crypto. The interface should be designed for the
case that _does_ care about CPU cycles, and then the async hw-assisted
crypto should be hidden by a conditional, and its (extra) data
structures should be the ones that are behind some indirect pointers
etc. Because, as we agreed, the async external hw case really doesn't
care. It it has to traverse a pointer or two, and if it has to have a
*SEPARATE* keystore that has longer lifetimes, then the async code can
set that up on its own, but that should not affect the case that
cares.
Really, this is fundamental, and really, the crypto interfaces got this wrong.
This is in fact _so_ fundamental that the only possible reason you can
disagree is because you don't care about reality or fundamentals, and
you only care about the small particular hardware niche you work in
and nothing else.
You really should think about this a bit.
> However, if you're doing bulk crypto like network packet processing
> (as Wireguard does!) or disk/filesystem encryption, then that cipher
> allocation only happens once every blue moon and the overhead for
> that is totally *irrelevant* as it is amortized over many hours or
> days of runtime.
This is not true. It's not true because the overhead happens ALL THE TIME.
And in 99.9% of all cases there are no upsides from the hw crypto,
because the hardware DOES NOT EXIST.
You think the "common case" is that hardware encryption case, but see
above. It's really not.
And when you _do_ have HW encryption, you could do the indirection.
But that's not an argument for always having the indirection.
What indirection am I talking about?
There's multiple levels of indirection in the existing bad crypto interfaces:
(a) the data structures themselves. This is things like keys and
state storage, both of which are (often) small enough that they would
be better off on a stack, or embedded in the data structures of the
callers.
(b) the calling conventions. This is things like indirection -
usually several levels - just to find the function pointer to call to,
and then an indirect call to that function pointer.
and both of those are actively bad things when you don't have those
hardware accelerators.
When you *do* have them, you don't care. Everybody agrees about that.
But you're ignoring the big white elephant in the room, which is that
the hw really is very rare in the end, even if it were to exist at
all.
> While I generally dislike this whole hype of storing stuff in
> textual formats like XML and JSON and then wasting lots of CPU
> cycles on parsing that, I've learned to appreciate the power of
> these textual Crypto API templates, as they allow a hardware
> accelerator to advertise complex combined operations as single
> atomic calls, amortizing the communication overhead between SW
> and HW. It's actually very flexible and powerful!
BUT IT IS FUNDAMENTALLY BADLY DESIGNED!
Really.
You can get the advantages of hw-accelerated crypto with good designs.
The current crypto layer is not that.
The current crypto layer simply has indirection at the wrong level.
Here's how it should have been done:
- make the interfaces be direct calls to the crypto you know you want.
- make the basic key and state buffer be explicit and let people
allocate them on their own stacks or whatever
"But what about hw acceleration?"
- add a single indirect private pointer that the direct calls can use
for their own state IF THEY HAVE REASON TO
- make the direct crypto calls just have a couple of conditionals
inside of them
Why? Direct calls with a couple of conditionals are really cheap for
the non-accelerated case. MUCH cheaper than the indirection overhead
(both on a state level and on a "crypto description" level) that the
current code has.
Seriously. The hw accelerated crypto won't care about the "call a
static routine" part. The hw accelerated crypto won't care about the
"I need to allocate a copy of the key because I can't have it on
stack, and need to have it in a DMA'able region". The hw accelerated
crypto won't care about the two extra instructions that do "do I have
any extra state at all, or should I just do the synchronous CPU
version" before it gets called through some indirect pointer.
So there is absolutely NO DOWNSIDE for hw accelerated crypto to just
do it right, and use an interface like this:
if (!chacha20poly1305_decrypt_sg(sg, sg, skb->len, NULL, 0,
PACKET_CB(skb)->nonce, key->key,
simd_context))
return false;
because for the hw accelerated case the costs are all elsewhere.
But for synchronous encryption code on the CPU? Avoiding the
indirection can be a huge win. Avoiding allocations, extra cachelines,
all that overhead. Avoiding indirect calls entirely, because doing a
few conditional branches (that will predict perfectly) on the state
will be a lot more efficient, both in direct CPU cycles and in things
like I$ etc.
In contrast, forcing people to use this model:
if (unlikely(crypto_aead_reqsize(key->tfm) > 0)) {
req = aead_request_alloc(key->tfm, GFP_ATOMIC);
if (!req)
return false;
} else {
req = &stackreq;
aead_request_set_tfm(req, key->tfm);
}
aead_request_set_ad(req, 0);
aead_request_set_callback(req, 0, NULL, NULL);
aead_request_set_crypt(req, sg, sg, skb->len,
(u8 *)&PACKET_CB(skb)->ivpad);
err = crypto_aead_decrypt(req);
if (unlikely(req != &stackreq))
aead_request_free(req);
if (err)
return false;
isn't going to help anybody. It sure as hell doesn't help the
CPU-synchronous case, and see above: it doesn't even help the hw
accelerated case. It could have had _all_ that "tfm" work behind a
private pointer that the CPU case never touches except to see "ok,
it's NULL, I don't have any".
See?
The interface *should* be that chacha20poly1305_decrypt_sg() library
interface, just give it a private pointer it can use and update. Then,
*internally* if can do something like
bool chacha20poly1305_decrypt_sg(...)
{
struct cc20p1305_ptr *state = *state_p;
if (state) {
.. do basically the above complex thing ..
return ret; .. or fall back to sw if the hw
queues are full..
}
.. do the CPU only thing..
}
and now you'd have no extra obverhead for the no-hw-accel case, you'd
have a more pleasant interface to use, and you could still use hw
accel if you wanted to.
THIS is why I say that the crypto interface is bad. It was designed
for the wrong objectives. It was designed purely for a SSL-like model
where you do a complex key and algorithm exchange dance, and you
simply don't know ahead of time what crypto you are even using.
And yes, that "I'm doing the SSL thing" used to be a major use of
encryption. I understand why it happened. It was what people did in
the 90's. People thought it was a good idea back then, and it was also
most of the hw acceleration world.
And yes, in that model of "I don't have a clue of what crypto I'm even
using" the model works fine. But honestly, if you can't admit to
yourself that it's wrong for the case where you _do_ know the
algorithm, you have some serious blinders on.
Just from a user standpoint, do you seriously think users _like_
having to do the above 15+ lines of code, vs the single function call?
The crypto interface really isn't pleasant, and you're wrong to
believe that it really helps. The hw acceleration capability could
have been abstracted away, instead of making that indirection be front
and center.
And again - I do realize the historical reasons for it. But
understanding that doesn't magically make it wonderful.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/4] arm64: vdso32: Detect binutils support for dmb ishld
From: Nick Desaulniers @ 2019-09-26 16:23 UTC (permalink / raw)
To: Vincenzo Frascino, Will Deacon
Cc: Catalin Marinas, Thomas Gleixner, LKML, Linux ARM, Ard Biesheuvel
In-Reply-To: <20190926133805.52348-3-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 6:38 AM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> As reported by Will Deacon, older versions of binutils that do not
> support certain types of memory barriers can cause build failure of the
> vdso32 library.
Do we know which specific version of binutils has this problem?
Documentation/process/changes.rst lists 2.21+ as the supported
versions. If it's older than that, it's unsupported.
I triple checked android's 2.27 seems ok.
$ cat bar.s
.text
.globl foo
dmb ish
dmb ishld
dmb ishst
$ aarch64-linux-gnu-as bar.s
$ echo $?
0
$ llvm-objdump -d a.out
a.out: file format ELF64-aarch64-little
Disassembly of section .text:
0000000000000000 $x:
0: bf 3b 03 d5 dmb ish
4: bf 39 03 d5 dmb ishld
8: bf 3a 03 d5 dmb ishst
>
> Add a compilation time mechanism that detects if binutils supports those
> instructions and configure the kernel accordingly.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
> arch/arm64/Kbuild | 6 ++++++
> arch/arm64/include/asm/vdso/compat_barrier.h | 2 +-
> arch/arm64/kernel/vdso32/Makefile | 9 +++++++++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
> index d6465823b281..75cf8c796d0e 100644
> --- a/arch/arm64/Kbuild
> +++ b/arch/arm64/Kbuild
> @@ -4,3 +4,9 @@ obj-$(CONFIG_NET) += net/
> obj-$(CONFIG_KVM) += kvm/
> obj-$(CONFIG_XEN) += xen/
> obj-$(CONFIG_CRYPTO) += crypto/
> +
> +# as-instr-compat
> +# Usage: cflags-y += $(call as-instr-compat,instr,option1,option2)
> +
> +as-instr-compat = $(call try-run,\
> + printf "%b\n" "$(1)" | $(COMPATCC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
> diff --git a/arch/arm64/include/asm/vdso/compat_barrier.h b/arch/arm64/include/asm/vdso/compat_barrier.h
> index fb60a88b5ed4..3fd8fd6d8fc2 100644
> --- a/arch/arm64/include/asm/vdso/compat_barrier.h
> +++ b/arch/arm64/include/asm/vdso/compat_barrier.h
> @@ -20,7 +20,7 @@
>
> #define dmb(option) __asm__ __volatile__ ("dmb " #option : : : "memory")
>
> -#if __LINUX_ARM_ARCH__ >= 8
> +#if __LINUX_ARM_ARCH__ >= 8 && defined(CONFIG_AS_DMB_ISHLD)
> #define aarch32_smp_mb() dmb(ish)
> #define aarch32_smp_rmb() dmb(ishld)
> #define aarch32_smp_wmb() dmb(ishst)
> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> index 1fba0776ed40..22f0d31ea528 100644
> --- a/arch/arm64/kernel/vdso32/Makefile
> +++ b/arch/arm64/kernel/vdso32/Makefile
> @@ -17,6 +17,8 @@ cc32-disable-warning = $(call try-run,\
> $(COMPATCC) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1)))
> cc32-ldoption = $(call try-run,\
> $(COMPATCC) $(1) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
> +cc32-as-instr = $(call try-run,\
> + printf "%b\n" "$(1)" | $(COMPATCC) $(VDSO_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
>
> # We cannot use the global flags to compile the vDSO files, the main reason
> # being that the 32-bit compiler may be older than the main (64-bit) compiler
> @@ -55,6 +57,7 @@ endif
> VDSO_CAFLAGS += -fPIC -fno-builtin -fno-stack-protector
> VDSO_CAFLAGS += -DDISABLE_BRANCH_PROFILING
>
> +
> # Try to compile for ARMv8. If the compiler is too old and doesn't support it,
> # fall back to v7. There is no easy way to check for what architecture the code
> # is being compiled, so define a macro specifying that (see arch/arm/Makefile).
> @@ -91,6 +94,12 @@ VDSO_CFLAGS += -Wno-int-to-pointer-cast
> VDSO_AFLAGS := $(VDSO_CAFLAGS)
> VDSO_AFLAGS += -D__ASSEMBLY__
>
> +# Check for binutils support for dmb ishld
> +dmbinstr := $(call cc32-as-instr,dmb ishld,-DCONFIG_AS_DMB_ISHLD=1)
> +
> +VDSO_CFLAGS += $(dmbinstr)
> +VDSO_AFLAGS += $(dmbinstr)
> +
> VDSO_LDFLAGS := $(VDSO_CPPFLAGS)
> # From arm vDSO Makefile
> VDSO_LDFLAGS += -Wl,-Bsymbolic -Wl,--no-undefined -Wl,-soname=linux-vdso.so.1
> --
> 2.23.0
>
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] soc: amlogic: Add support for Secure power domains controller
From: Kevin Hilman @ 2019-09-26 16:23 UTC (permalink / raw)
To: Jianxin Pan, linux-amlogic
Cc: devicetree, Hanjie Lin, Victor Wan, linux-pm, Martin Blumenstingl,
Neil Armstrong, linux-kernel, Zhiqiang Liang, Rob Herring,
Jian Hu, Xingyu Chen, linux-arm-kernel, Jerome Brunet
In-Reply-To: <3859c748-01f0-4dbd-05d6-20fff31edf11@amlogic.com>
Jianxin Pan <jianxin.pan@amlogic.com> writes:
> Hi Kevin,
>
> Thanks for your review. Please see my comments below.
>
>
> On 2019/9/26 6:41, Kevin Hilman wrote:
>> Hi Jianxin,
>>
>> Jianxin Pan <jianxin.pan@amlogic.com> writes:
>>
>>> Add support for the Amlogic Secure Power controller. In A1/C1 series, power
>>> control registers are in secure domain, and should be accessed by smc.
>>>
>>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
>>> Signed-off-by: Zhiqiang Liang <zhiqiang.liang@amlogic.com>
>>
>> Thanks for the new power domain driver.
>>
>>> ---
>>> drivers/soc/amlogic/Kconfig | 13 +++
>>> drivers/soc/amlogic/Makefile | 1 +
>>> drivers/soc/amlogic/meson-secure-pwrc.c | 182 ++++++++++++++++++++++++++++++++
>>> 3 files changed, 196 insertions(+)
>>> create mode 100644 drivers/soc/amlogic/meson-secure-pwrc.c
>>>
>>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>>> index bc2c912..6cb06e7 100644
>>> --- a/drivers/soc/amlogic/Kconfig
>>> +++ b/drivers/soc/amlogic/Kconfig
>>> @@ -48,6 +48,19 @@ config MESON_EE_PM_DOMAINS
>>> Say yes to expose Amlogic Meson Everything-Else Power Domains as
>>> Generic Power Domains.
>>>
>>> +config MESON_SECURE_PM_DOMAINS
>>> + bool "Amlogic Meson Secure Power Domains driver"
>>> + depends on ARCH_MESON || COMPILE_TEST
>>> + depends on PM && OF
>>> + depends on HAVE_ARM_SMCCC
>>> + default ARCH_MESON
>>> + select PM_GENERIC_DOMAINS
>>> + select PM_GENERIC_DOMAINS_OF
>>> + help
>>> + Support for the power controller on Amlogic A1/C1 series.
>>> + Say yes to expose Amlogic Meson Secure Power Domains as Generic
>>> + Power Domains.
>>> +
>>> config MESON_MX_SOCINFO
>>> bool "Amlogic Meson MX SoC Information driver"
>>> depends on ARCH_MESON || COMPILE_TEST
>>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
>>> index de79d044..7b8c5d3 100644
>>> --- a/drivers/soc/amlogic/Makefile
>>> +++ b/drivers/soc/amlogic/Makefile
>>> @@ -5,3 +5,4 @@ obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>>> obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>>> obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
>>> obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
>>> +obj-$(CONFIG_MESON_SECURE_PM_DOMAINS) += meson-secure-pwrc.o
>>> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
>>> new file mode 100644
>>> index 00000000..00c7232
>>> --- /dev/null
>>> +++ b/drivers/soc/amlogic/meson-secure-pwrc.c
>>> @@ -0,0 +1,182 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> [...]
>>> +
>>> +static bool pwrc_secure_get_power(struct meson_secure_pwrc_domain *pwrc_domain)
>>> +{
>>> + struct arm_smccc_res res;
>>> +
>>> + arm_smccc_smc(SMC_PWRC_GET, pwrc_domain->index, 0,
>>> + 0, 0, 0, 0, 0, &res);
>>> +
>>> + return res.a0 & 0x1;
>>
>> Please use a #define with a readable name for this mask.
>> The return type of this smc is bool. I will remove 0x1 mask in next version.
>
> Another question about smc:
> In this driver, no share memory is needed, and I use arm_smccc_smc() directly.
> Should I add secure-monitor = <&sm> in dtb and use meson_sm_call() from sm driver instead?
Yes, that would be preferred.
>>> +}
>>
>> What does the return value for this function mean? Does true mean
>> "powered off" or "powered on">
> The return vaule for SMC_PWRC_GET :
> 0 -> power on
> 1 -> power off> See the rename I just did on the ee-pwrc driver:
>> https://lore.kernel.org/linux-amlogic/20190925213528.21515-2-khilman@kernel.org/
>> I will follow and rename to _is_off() in the next verson.
>>> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
>>> +{
>>> + struct arm_smccc_res res;
>>> + struct meson_secure_pwrc_domain *pwrc_domain =
> [...]
>>> +
>>> +#define SEC_PD(__name, __flag) \
>>> +{ \
>>> + .name = #__name, \
>>> + .index = PWRC_##__name##_ID, \
>>> + .get_power = pwrc_secure_get_power, \
>>> + .flags = __flag, \
>>> +}
>>> +
>>> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>>> + SEC_PD(DSPA, 0),
>>> + SEC_PD(DSPB, 0),
>>> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON),
>>
>> This flag should only be used for domains where there are no linux
>> drivers.
>>
>> Rather than using this flag, you need to add a 'power-domain' property
>> to the uart driver in DT, and then update the meson_uart driver to use
>> the runtime PM API so that the domain is enabled whenever the UART is in
>> use.
>
> PM_UART Power domain is shared by uart, msr, jtag and cec.
> Uart should keep working in BL31, after kernel suspend and before kernel resume.
OK.
>>
>>> + SEC_PD(DMC, GENPD_FLAG_ALWAYS_ON),
>>
>> Please explain the need for ALWAYS_ON.
>>
> PM_DMC is used for DDR PHY ana/dig and DMC.
> There is no linux drver for them, and it should be always on.
>
> I will add comments for all these always on domains.
OK, thank you.
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq
From: Matthias Kaehlcke @ 2019-09-26 16:04 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan, linux-pm,
Viresh Kumar, NXP Linux Team, Krzysztof Kozlowski, Lukasz Luba,
Kyungmin Park, MyungJoo Ham, Alexandre Bailon, Leonard Crestez,
Georgi Djakov, linux-arm-kernel, Jacky Bai
In-Reply-To: <81b136fc-0613-08f8-cb26-6243e7be621b@samsung.com>
On Thu, Sep 26, 2019 at 10:25:41AM +0900, Chanwoo Choi wrote:
> On 19. 9. 26. 오전 1:45, Matthias Kaehlcke wrote:
> > On Wed, Sep 25, 2019 at 11:41:07AM +0900, Chanwoo Choi wrote:
> >> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
> >>> Switch the handling of min_freq and max_freq from sysfs to use the
> >>> dev_pm_qos_request interface.
> >>>
> >>> Since PM QoS handles frequencies as kHz this change reduces the
> >>> precision of min_freq and max_freq. This shouldn't introduce problems
> >>> because frequencies which are not an integer number of kHz are likely
> >>> not an integer number of Hz either.
> >>>
> >>> Try to ensure compatibility by rounding min values down and rounding
> >>> max values up.
> >>>
> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> >>> ---
> >>> drivers/devfreq/devfreq.c | 46 ++++++++++++++++++++++++---------------
> >>> include/linux/devfreq.h | 9 ++++----
> >>> 2 files changed, 33 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>> index 784f3e40536a..8bb7efd821ab 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq,
> >>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> >>> DEV_PM_QOS_MIN_FREQUENCY);
> >>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq);
> >>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq);
> >>>
> >>> - /* constraints from sysfs */
> >>> - *min_freq = max(*min_freq, devfreq->min_freq);
> >>> - *max_freq = min(*max_freq, devfreq->max_freq);
> >>> -
> >>> /* constraints from OPP interface */
> >>> *min_freq = max(*min_freq, devfreq->scaling_min_freq);
> >>> /* scaling_max_freq can be zero on error */
> >>> if (devfreq->scaling_max_freq)
> >>> *max_freq = min(*max_freq, devfreq->scaling_max_freq);
> >>> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
> >>> DEV_PM_QOS_MIN_FREQUENCY);
> >>>
> >>> if (devfreq->profile->exit)
> >>> devfreq->profile->exit(devfreq->dev.parent);
> >>>
> >>> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> >>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> >>
> >> Please check the return value if error happen, just print the err with dev_err()
> >> without stopping the release steps.
> >
> > I wonder if dev_warn() would be more appropriate, since the current operation
> > is not aborted.
> >
> >>> kfree(devfreq->time_in_state);
> >>> kfree(devfreq->trans_table);
> >>> mutex_destroy(&devfreq->lock);
> >>> kfree(devfreq);
> >>> }
> >>> @@ -747,18 +745,25 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>> devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> >>> if (!devfreq->scaling_min_freq) {
> >>> err = -EINVAL;
> >>> goto err_dev;
> >>> }
> >>> - devfreq->min_freq = devfreq->scaling_min_freq;
> >>>
> >>> devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> >>> if (!devfreq->scaling_max_freq) {
> >>> err = -EINVAL;
> >>> goto err_dev;
> >>> }
> >>> - devfreq->max_freq = devfreq->scaling_max_freq;
> >>> +
> >>> + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> >>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
> >>> + if (err < 0)
> >>> + goto err_dev;
> >>> + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> >>> + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> >>> + if (err < 0)
> >>> + goto err_dev;
> >>>
> >>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> >>> atomic_set(&devfreq->suspend_count, 0);
> >>>
> >>> devfreq->trans_table = kzalloc(
> >>> @@ -843,10 +848,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>> err_dev:
> >>> /*
> >>> * Cleanup path for errors that happen before registration.
> >>> * Otherwise we rely on devfreq_dev_release.
> >>> */
> >>> + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> >>> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> >>
> >> Please check the return value if error happen, just print the err with dev_err()
> >> without stopping the release steps.
> >>
> >> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n");
> >
> > dev_warn() for the same reason as above?
>
> Actually, I think that is not critical error but need to print the error
> So, I think that dev_err() is enough. If this thing is critical,
> better to use dev_warn.
We agree that this is not critical, but not when to use dev_err()
and dev_warn(). You seem to imply that dev_warn() is more severe
than dev_err(), however it's the other way around:
#define KERN_ERR KERN_SOH "3" /* error conditions */
#define KERN_WARNING KERN_SOH "4" /* warning conditions */
#define KERN_NOTICE KERN_SOH "5" /* normal but significant condition */
I think in this case you can go either way, it's certainly an error,
but the driver continues with the normal execution path.
> >
> > I think the message would be better with a slight change:
> >
> > "failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n"
>
> OK.
>
> >
> >>
> >>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> >>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> >>
> >> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n");
> >
> > ditto
> >
> >>> kfree(devfreq->time_in_state);
> >>> kfree(devfreq->trans_table);
> >>> kfree(devfreq);
> >>> err_out:
> >>> return ERR_PTR(err);
> >>> @@ -1407,14 +1416,15 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> >>>
> >>> ret = sscanf(buf, "%lu", &value);
> >>> if (ret != 1)
> >>> return -EINVAL;
> >>>
> >>> - mutex_lock(&df->lock);
> >>> - df->min_freq = value;
> >>> - update_devfreq(df);
> >>> - mutex_unlock(&df->lock);
> >>> + /* round down to kHz for PM QoS */
> >>
> >> I prefer more detailed description as following:
> >>
> >> /*
> >> * Round down to KHz to decide the proper minimum frequency
> >
> > it should be kHz, with a lower-case 'k', as in the original comment.
>
> Good.
>
> >
> >> * which is closed to user request.
> >> */
> >
> > The comment you suggest doesn't provide any information about why the
> > conversion to kHz is done, in this sense the original comment that
> > mentions PM QoS provides more value.
> >
> > With whatever we end up, I suggest to use 'convert' instead of
> > 'round down'.
>
> I agree to use 'convert' instead of 'round down'
> if some expression indicates the correct meaning.
>
> >
> >>> + ret = dev_pm_qos_update_request(&df->user_min_freq_req,
> >>> + value / HZ_PER_KHZ);
> >>> + if (ret < 0)
> >>> + return ret;
> >>>
> >>> return count;
> >>> }
> >>>
> >>> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> >>> @@ -1439,19 +1449,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> >>>
> >>> ret = sscanf(buf, "%lu", &value);
> >>> if (ret != 1)
> >>> return -EINVAL;
> >>>
> >>> - mutex_lock(&df->lock);
> >>> -
> >>> - /* Interpret zero as "don't care" */
> >>> - if (!value)
> >>> - value = ULONG_MAX;
> >>> + /* round up to kHz for PM QoS and interpret zero as "don't care" */
> >>
> >> I think that "don't care" comment style is not good.
> >>
> >> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file.
> >> I prefer more detailed description as following:
> >> /*
> >> * Round up to KHz to decide the proper maximum frequency
> >
> > kHz
> >
> >> * which is closed to user request. If value is zero,
> >> * the user does not care.
> >
> > "the user does not care" is still very casual you didn't like initially.
> > How about "A value of zero is interpreted as 'no limit'."?
> >
> > As for the min freq, I think PM QoS should be mentioned to make clear why
> > the conversion to kHz is needed.
>
> Agree. I expect that Leonard will mention that.
>
> >
> >> */
> >>
> >>
> >>> + if (value)
> >>> + value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> >>> + else
> >>> + value = S32_MAX;
> >>>
> >>> - df->max_freq = value;
> >>> - update_devfreq(df);
> >>> - mutex_unlock(&df->lock);
> >>> + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> >>> + if (ret < 0)
> >>> + return ret;
> >>>
> >>> return count;
> >>> }
> >>> static DEVICE_ATTR_RW(min_freq);
> >>>
> >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> >>> index dac0dffeabb4..7849fe4c666d 100644
> >>> --- a/include/linux/devfreq.h
> >>> +++ b/include/linux/devfreq.h
> >>> @@ -11,10 +11,11 @@
> >>> #define __LINUX_DEVFREQ_H__
> >>>
> >>> #include <linux/device.h>
> >>> #include <linux/notifier.h>
> >>> #include <linux/pm_opp.h>
> >>> +#include <linux/pm_qos.h>
> >>>
> >>> #define DEVFREQ_NAME_LEN 16
> >>>
> >>> /* DEVFREQ governor name */
> >>> #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand"
> >>> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
> >>> * devfreq.nb to the corresponding register notifier call chain.
> >>> * @work: delayed work for load monitoring.
> >>> * @previous_freq: previously configured frequency value.
> >>> * @data: Private data of the governor. The devfreq framework does not
> >>> * touch this.
> >>> - * @min_freq: Limit minimum frequency requested by user (0: none)
> >>> - * @max_freq: Limit maximum frequency requested by user (0: none)
> >>> + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs)
> >>
> >> min -> minimum and then remove parenthesis as following:
> >> PM QoS minimum frequency request by user via sysfs
> >>
> >>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs)
> >>
> >> ditto. max -> maximum
> >> PM QoS maximum frequency request by user via sysfs
> >>
> >>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface
> >>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface
> >>> * @stop_polling: devfreq polling status of a device.
> >>> * @suspend_freq: frequency of a device set during suspend phase.
> >>> * @resume_freq: frequency of a device set in resume phase.
> >>> @@ -161,12 +162,12 @@ struct devfreq {
> >>> unsigned long previous_freq;
> >>> struct devfreq_dev_status last_status;
> >>>
> >>> void *data; /* private data for governors */
> >>>
> >>> - unsigned long min_freq;
> >>> - unsigned long max_freq;
> >>> + struct dev_pm_qos_request user_min_freq_req;
> >>> + struct dev_pm_qos_request user_max_freq_req;
> >>> unsigned long scaling_min_freq;
> >>> unsigned long scaling_max_freq;
> >>> bool stop_polling;
> >>>
> >>> unsigned long suspend_freq;
> >>>
> >>
> >>
> >> --
> >> Best Regards,
> >> Chanwoo Choi
> >> Samsung Electronics
> >
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/4] arm64: vdso32: Detect binutils support for dmb ishld
From: Catalin Marinas @ 2019-09-26 15:59 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926133805.52348-3-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 02:38:03PM +0100, Vincenzo Frascino wrote:
> arch/arm64/kernel/vdso32/Makefile | 9 +++++++++
Could you please also remove the unnecessary gcc-goto.sh check in this
file? We don't use jump labels in the vdso (can't run-time patch them).
I found it while forcing COMPATCC=clang with my additional diff and I
get the warning on 'make clean'.
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 22f0d31ea528..038357a1e835 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -40,9 +38,6 @@ VDSO_CAFLAGS += $(call cc32-option,-fno-PIE)
ifdef CONFIG_DEBUG_INFO
VDSO_CAFLAGS += -g
endif
-ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(COMPATCC)), y)
-VDSO_CAFLAGS += -DCC_HAVE_ASM_GOTO
-endif
# From arm Makefile
VDSO_CAFLAGS += $(call cc32-option,-fno-dwarf2-cfi-asm)
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH] staging: media: imx: Use devm_platform_ioremap_resource().
From: Jeeeun Evans @ 2019-09-26 15:55 UTC (permalink / raw)
To: rmfrfs, slongerbeam, p.zabel, mchehab, gregkh, shawnguo, s.hauer,
kernel, festevam, linux-imx
Cc: devel, linux-kernel, linux-arm-kernel, linux-media
This patch fixes a warning by coccicheck:
drivers/staging/media/imx/imx7-mipi-csis.c:973:1-12: WARNING: Use devm_platform_ioremap_resource for state -> regs
Use devm_platform_ioremap_resource helper which wraps platform_get_resource()
and devm_ioremap_resource() together.
Signed-off-by: Jeeeun Evans <jeeeunevans@gmail.com>
---
drivers/staging/media/imx/imx7-mipi-csis.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 73d8354e618c..bf21db38441f 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -947,7 +947,6 @@ static void mipi_csis_debugfs_exit(struct csi_state *state)
static int mipi_csis_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct resource *mem_res;
struct csi_state *state;
int ret;
@@ -969,8 +968,7 @@ static int mipi_csis_probe(struct platform_device *pdev)
mipi_csis_phy_init(state);
mipi_csis_phy_reset(state);
- mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- state->regs = devm_ioremap_resource(dev, mem_res);
+ state->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(state->regs))
return PTR_ERR(state->regs);
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] arm64: Allow disabling of the compat vDSO
From: Catalin Marinas @ 2019-09-26 15:51 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: Ard Biesheuvel, Nick Desaulniers, LKML, Thomas Gleixner,
Will Deacon, Linux ARM
In-Reply-To: <20190926074717.GA26802@iMac.local>
On Thu, Sep 26, 2019 at 08:47:18AM +0100, Catalin Marinas wrote:
> On Thu, Sep 26, 2019 at 01:06:50AM +0100, Vincenzo Frascino wrote:
> > On 9/25/19 6:08 PM, Catalin Marinas wrote:
> > > On Wed, Sep 25, 2019 at 09:53:16AM -0700, Nick Desaulniers wrote:
> > >> On Wed, Sep 25, 2019 at 6:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > >>> - clean up the headers includes; vDSO should not include kernel-only
> > >>> headers that may even contain code patched at run-time
> > >>
> > >> This is a big one; Clang validates the inline asm constraints for
> > >> extended inline assembly, GCC does not for dead code. So Clang chokes
> > >> on the inclusion of arm64 headers using extended inline assembly when
> > >> being compiled for arm-linux-gnueabi.
> > >
> > > Whether clang or gcc, I'd like this fixed anyway. At some point we may
> > > inadvertently rely on some code which is patched at boot time for the
> > > kernel code but not for the vDSO.
> >
> > Do we have any code of this kind in header files?
> >
> > The vDSO library uses only a subset of the headers (mainly Macros) hence all the
> > unused symbols should be compiled out. Is your concern only theoretical or do
> > you have an example on where this could be happening?
>
> At the moment it's rather theoretical.
Actually, it's not. The moment the compat vdso Makefile needs the line
below, we are doing it wrong:
VDSO_CFLAGS += -D__uint128_t='void*'
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCHv3-next 0/3] Odroid c2 missing regulator linking
From: Anand Moon @ 2019-09-26 15:38 UTC (permalink / raw)
To: Rob Herring, Martin Blumenstingl, Jerome Brunet, Neil Armstrong,
Kevin Hilman
Cc: devicetree, Linux Kernel, linux-arm-kernel, linux-amlogic
In-Reply-To: <20190906122044.372-1-linux.amoon@gmail.com>
Hi Kevin,
On Fri, 6 Sep 2019 at 17:50, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Below small changes help re-configure or fix missing inter linking
> of regulator node.
>
> Re-based on linux-next-20190904
> Changes from previous patch's series.
> Build using Cross Compiler.
>
> Added missing Reviewed-by Neil's and Martin.
> Added few suggestion from martin for rename of node.
>
> Dependencies:
> Changes based top on my previous usb fix series patch's.
>
> [0] https://patchwork.kernel.org/patch/11113095/
> [1] https://patchwork.kernel.org/patch/11113099/
> [3] https://patchwork.kernel.org/patch/11113103/
>
> Hope this series get picked up for 5.4-rc1, finger crossed.
>
> Changes for previous changes.
> Fix some typo.
> Updated few patches as per Martin's suggestion.
>
> I will try to commit less mistake in the future.
>
> Best Regards
> -Anand
>
Gentle ping for this series.
Best Regards
-Anand
> Anand Moon (3):
> arm64: dts: meson: odroid-c2: Add missing regulator linked to P5V0
> regulator
> arm64: dts: meson: odroid-c2: Add missing regulator linked to
> VDDIO_AO3V3 regulator
> arm64: dts: meson: odroid-c2: Add missing regulator linked to HDMI
> supply
>
> .../boot/dts/amlogic/meson-gxbb-odroidc2.dts | 53 +++++++++++++++++--
> 1 file changed, 50 insertions(+), 3 deletions(-)
>
> --
> 2.23.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5] gpio/mpc8xxx: change irq handler from chained to normal
From: Russell King - ARM Linux admin @ 2019-09-26 15:35 UTC (permalink / raw)
To: Hui Song
Cc: Mark Rutland, devicetree, linux-gpio, Linus Walleij, linux-kernel,
Li Yang, Bartosz Golaszewski, Rob Herring, Shawn Guo,
linux-arm-kernel
In-Reply-To: <20190916055817.43425-1-hui.song_1@nxp.com>
On Mon, Sep 16, 2019 at 01:58:17PM +0800, Hui Song wrote:
> From: Song Hui <hui.song_1@nxp.com>
>
> More than one gpio controllers can share one interrupt, change the
> driver to request shared irq.
>
> Signed-off-by: Laurentiu Tudor <Laurentiu.Tudor@nxp.com>
> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
While this will work, it will mess up userspace accounting of the
number of interrupts per second in tools such as vmstat. The reason
is that for every GPIO interrupt, /proc/interrupts records the count
against GIC interrupt 68 or 69, as well as the GPIO itself. So, for
every GPIO interrupt, the total number of interrupts that the system
has seen increments by two.
If we don't care about accurate interrupt statistics, then this is
fine, but I think it should be mentioned in the commit message.
> ---
> Changes in v5:
> - add traverse every bit function.
> Changes in v4:
> - convert 'pr_err' to 'dev_err'.
> Changes in v3:
> - update the patch description.
> Changes in v2:
> - delete the compatible of ls1088a.
> drivers/gpio/gpio-mpc8xxx.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 16a47de..3a06ca9 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -22,6 +22,7 @@
> #include <linux/irq.h>
> #include <linux/gpio/driver.h>
> #include <linux/bitops.h>
> +#include <linux/interrupt.h>
>
> #define MPC8XXX_GPIO_PINS 32
>
> @@ -127,20 +128,20 @@ static int mpc8xxx_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> return -ENXIO;
> }
>
> -static void mpc8xxx_gpio_irq_cascade(struct irq_desc *desc)
> +static irqreturn_t mpc8xxx_gpio_irq_cascade(int irq, void *data)
> {
> - struct mpc8xxx_gpio_chip *mpc8xxx_gc = irq_desc_get_handler_data(desc);
> - struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct mpc8xxx_gpio_chip *mpc8xxx_gc = (struct mpc8xxx_gpio_chip *)data;
> struct gpio_chip *gc = &mpc8xxx_gc->gc;
> unsigned int mask;
> + int i;
>
> mask = gc->read_reg(mpc8xxx_gc->regs + GPIO_IER)
> & gc->read_reg(mpc8xxx_gc->regs + GPIO_IMR);
> - if (mask)
> + for_each_set_bit(i, &mask, 32)
> generic_handle_irq(irq_linear_revmap(mpc8xxx_gc->irq,
> - 32 - ffs(mask)));
> - if (chip->irq_eoi)
> - chip->irq_eoi(&desc->irq_data);
> + 31 - i));
> +
> + return IRQ_HANDLED;
> }
>
> static void mpc8xxx_irq_unmask(struct irq_data *d)
> @@ -388,8 +389,8 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>
> ret = gpiochip_add_data(gc, mpc8xxx_gc);
> if (ret) {
> - pr_err("%pOF: GPIO chip registration failed with status %d\n",
> - np, ret);
> + dev_err(&pdev->dev, "%pOF: GPIO chip registration failed with status %d\n",
> + np, ret);
> goto err;
> }
>
> @@ -409,8 +410,15 @@ static int mpc8xxx_probe(struct platform_device *pdev)
> if (devtype->gpio_dir_in_init)
> devtype->gpio_dir_in_init(gc);
>
> - irq_set_chained_handler_and_data(mpc8xxx_gc->irqn,
> - mpc8xxx_gpio_irq_cascade, mpc8xxx_gc);
> + ret = request_irq(mpc8xxx_gc->irqn, mpc8xxx_gpio_irq_cascade,
> + IRQF_NO_THREAD | IRQF_SHARED, "gpio-cascade",
> + mpc8xxx_gc);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: failed to request_irq(%d), ret = %d\n",
> + np->full_name, mpc8xxx_gc->irqn, ret);
> + goto err;
> + }
> +
> return 0;
> err:
> iounmap(mpc8xxx_gc->regs);
> --
> 2.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 0/4] arm64: vdso32: Address various issues
From: Catalin Marinas @ 2019-09-26 15:31 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926133805.52348-1-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 02:38:01PM +0100, Vincenzo Frascino wrote:
> this patch series is meant to address the various compilation issues you
> reported about arm64 vdso32. (This time for real I hope ;))
>
> Please let me know if there is still something missing.
Apart from the diff I posted and some nitpicks, the series looks fine to
me. If you post an update, I'll ack it (and a tested-by).
In addition to this series I'd still prefer to have my Kconfig option to
disable the compat vDSO if something else fails in the future (at least
until we complete the headers clean-up). But I'm fine with a default y
and removing EXPERT.
Thanks for the quick turnaround.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/4] arm64: vdso32: Introduce COMPAT_CC_IS_GCC
From: Catalin Marinas @ 2019-09-26 15:23 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <0c5816ba-4393-07f7-23ec-38ebde0e447f@arm.com>
On Thu, Sep 26, 2019 at 12:02:38PM +0100, Vincenzo Frascino wrote:
> On 9/26/19 11:56 AM, Vincenzo Frascino wrote:
> > On 9/26/19 9:06 AM, Catalin Marinas wrote:
> >> Has CONFIG_CROSS_COMPILE_COMPAT_VDSO actually been removed from
> >> lib/vdso/Kconfig? (I haven't checked the subsequent patches).
>
> Missed this, I have the patch ready for that. When this series will be merged,
> no more architectures will use the macro hence I will send a separate patch to
> remove it from the common code.
Since arm64 was the only user, can you send it together with this
series? I find it strange that Kbuild prompts me to enter this option
when it wouldn't have any effect.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 3/4] arm64: vdso32: Fix compilation warning
From: Catalin Marinas @ 2019-09-26 15:22 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926133805.52348-4-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 02:38:04PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b61b50bf68b1..cfa9cd87af14 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -228,11 +228,16 @@ static inline unsigned long kaslr_offset(void)
> #define __tag_get(addr) 0
> #endif /* CONFIG_KASAN_SW_TAGS */
>
> +#ifdef __aarch64__
> static inline const void *__tag_set(const void *addr, u8 tag)
> {
> u64 __addr = (u64)addr & ~__tag_shifted(0xff);
> return (const void *)(__addr | __tag_shifted(tag));
> }
> +#else
> +/* Unused in compat vdso */
> +#define __tag_set(addr, tag) 0
> +#endif
Do we actually need an #else block here? I think the #ifdef is
sufficient, with a comment along the lines of "Do not attempt to compile
when included in the compat vdso" (or pick some better wording).
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/4] arm64: vdso32: Introduce COMPAT_CC_IS_GCC
From: Catalin Marinas @ 2019-09-26 15:17 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <ad90f9bb-aa39-615c-3ae5-bea394bd787c@arm.com>
On Thu, Sep 26, 2019 at 01:22:01PM +0100, Vincenzo Frascino wrote:
> On 9/26/19 11:56 AM, Vincenzo Frascino wrote:
> > On 9/26/19 9:06 AM, Catalin Marinas wrote:
> >> Now, could we not generate a COMPATCC in the Makefile and use
> >> $(COMPATCC) here instead of $(CROSS_COMPILE_COMPAT)gcc? It really
> >> doesn't make sense to check that gcc is gcc.
> >>
> >
> > All right, COMPATCC is already in the makefile, I will use it in here.
>
> What you are proposing seems not possible because Kconfig runs first and then
> the arch Makefile, hence compatcc does not take effect on the Kconfig. I will
> post v2 with what I proposed, please feel free to comment if you have a better idea.
I think it works as long as you export COMPATCC from the
arch/arm64/Makefile. The arch Makefile is used in the config step
AFAICT. See the diff I posted in my reply to your v2.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v10 0/6] mm / virtio: Provide support for unused page reporting
From: Alexander Duyck @ 2019-09-26 15:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Yang Zhang, Pankaj Gupta, kvm list, David Hildenbrand,
lcapitulino, linux-mm, Alexander Duyck, Andrea Arcangeli,
virtio-dev, Michael S. Tsirkin, Matthew Wilcox, Wang, Wei W,
Vlastimil Babka, Rik van Riel, Dan Williams, linux-arm-kernel,
Oscar Salvador, Nitesh Narayan Lal, Konrad Rzeszutek Wilk,
Dave Hansen, LKML, Paolo Bonzini, Andrew Morton, Mel Gorman
In-Reply-To: <20190926122208.GI20255@dhcp22.suse.cz>
On Thu, Sep 26, 2019 at 5:22 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Tue 24-09-19 08:20:22, Alexander Duyck wrote:
> > On Tue, Sep 24, 2019 at 7:23 AM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Wed 18-09-19 10:52:25, Alexander Duyck wrote:
> > > [...]
> > > > In order to try and keep the time needed to find a non-reported page to
> > > > a minimum we maintain a "reported_boundary" pointer. This pointer is used
> > > > by the get_unreported_pages iterator to determine at what point it should
> > > > resume searching for non-reported pages. In order to guarantee pages do
> > > > not get past the scan I have modified add_to_free_list_tail so that it
> > > > will not insert pages behind the reported_boundary.
> > > >
> > > > If another process needs to perform a massive manipulation of the free
> > > > list, such as compaction, it can either reset a given individual boundary
> > > > which will push the boundary back to the list_head, or it can clear the
> > > > bit indicating the zone is actively processing which will result in the
> > > > reporting process resetting all of the boundaries for a given zone.
> > >
> > > Is this any different from the previous version? The last review
> > > feedback (both from me and Mel) was that we are not happy to have an
> > > externally imposed constrains on how the page allocator is supposed to
> > > maintain its free lists.
> >
> > The main change for v10 versus v9 is that I allow the page reporting
> > boundary to be overridden. Specifically there are two approaches that
> > can be taken.
> >
> > The first is to simply reset the iterator for whatever list is
> > updated. What this will do is reset the iterator back to list_head and
> > then you can do whatever you want with that specific list.
>
> OK, this is slightly better than pushing the allocator to the corner.
> The allocator really has to be under control of its data structures.
> I would still be happier if the allocator wouldn't really have to bother
> about somebody snooping its internal state to do its own thing. So
> please make sure to describe why and how much this really matters.
Okay I can try to do that. I suppose if nothing else I can put
together a test patch that reverts these bits and can add
documentation on the amount of regression seen without those bits. I
should be able to get that taken care of and a v11 out in the next few
days.
> > The other option is to simply clear the ZONE_PAGE_REPORTING_ACTIVE
> > bit. That will essentially notify the page reporting code that any/all
> > hints that were recorded have been discarded and that it needs to
> > start over.
> >
> > All I am trying to do with this approach is reduce the work. Without
> > doing this the code has to walk the entire free page list for the
> > higher orders every iteration and that will not be cheap.
>
> How expensive this will be?
Well without this I believe the work goes from being O(n) to O(n^2) as
we would have to walk the list every time we pull the batch of pages,
so without the iterator we end up having walk the page list
repeatedly. I suspect it becomes more expensive the more memory we
have. I'll be able to verify it later today once I can generate some
numbers.
> > Admittedly
> > it is a bit more invasive than the cut/splice logic used in compaction
> > which is taking the pages it has already processed and moving them to
> > the other end of the list. However, I have reduced things so that we
> > only really are limiting where add_to_free_list_tail can place pages,
> > and we are having to check/push back the boundaries if a reported page
> > is removed from a free_list.
> >
> > > If this is really the only way to go forward then I would like to hear
> > > very convincing arguments about other approaches not being feasible.
> > > There are none in this cover letter unfortunately. This will be really a
> > > hard sell without them.
> >
> > So I had considered several different approaches.
>
> Thanks this is certainly useful and it would have been even more so if
> you gave some rough numbers to quantify how much overhead for different
> solutions we are talking about here.
I'll see what I can do. As far as the bitmap solution I think Nitesh
has numbers for what he has been able to get out of it. At this point
I would assume his solution for the virtio/QEMU bits is probably
identical to mine so it should be easier to get an apples to apples
comparison.
Thanks.
- Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/4] arm64: vdso32: Introduce COMPAT_CC_IS_GCC
From: Catalin Marinas @ 2019-09-26 15:13 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926133805.52348-2-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 02:38:02PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 37c610963eee..13e2d2e16af7 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -110,7 +110,7 @@ config ARM64
> select GENERIC_STRNLEN_USER
> select GENERIC_TIME_VSYSCALL
> select GENERIC_GETTIMEOFDAY
> - select GENERIC_COMPAT_VDSO if (!CPU_BIG_ENDIAN && COMPAT)
> + select GENERIC_COMPAT_VDSO if (!CPU_BIG_ENDIAN && COMPAT && COMPATCC_IS_GCC)
> select HANDLE_DOMAIN_IRQ
> select HARDIRQS_SW_RESEND
> select HAVE_PCI
> @@ -313,6 +313,9 @@ config KASAN_SHADOW_OFFSET
> default 0xeffffff900000000 if ARM64_VA_BITS_36 && KASAN_SW_TAGS
> default 0xffffffffffffffff
>
> +config COMPATCC_IS_GCC
> + def_bool $(success,$(CROSS_COMPILE_COMPAT)gcc --version | head -n 1 | grep -q arm)
Nitpick: COMPAT_CC_IS_ARM_GCC together with 'grep -q "arm-.*-gcc"'
The patch below allows me to use $(COMPATCC) directly here and also set
either CROSS_COMPILE_COMPAT or COMPATCC on the command line:
----------8<----------------------------------
diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
index 75cf8c796d0e..d6465823b281 100644
--- a/arch/arm64/Kbuild
+++ b/arch/arm64/Kbuild
@@ -4,9 +4,3 @@ obj-$(CONFIG_NET) += net/
obj-$(CONFIG_KVM) += kvm/
obj-$(CONFIG_XEN) += xen/
obj-$(CONFIG_CRYPTO) += crypto/
-
-# as-instr-compat
-# Usage: cflags-y += $(call as-instr-compat,instr,option1,option2)
-
-as-instr-compat = $(call try-run,\
- printf "%b\n" "$(1)" | $(COMPATCC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5d4be0b4f980..811c89fa2b71 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -110,7 +110,7 @@ config ARM64
select GENERIC_STRNLEN_USER
select GENERIC_TIME_VSYSCALL
select GENERIC_GETTIMEOFDAY
- select GENERIC_COMPAT_VDSO if (!CPU_BIG_ENDIAN && COMPAT && COMPATCC_IS_GCC)
+ select GENERIC_COMPAT_VDSO if (!CPU_BIG_ENDIAN && COMPAT && COMPATCC_IS_ARM_GCC)
select HANDLE_DOMAIN_IRQ
select HARDIRQS_SW_RESEND
select HAVE_PCI
@@ -313,8 +313,8 @@ config KASAN_SHADOW_OFFSET
default 0xeffffff900000000 if ARM64_VA_BITS_36 && KASAN_SW_TAGS
default 0xffffffffffffffff
-config COMPATCC_IS_GCC
- def_bool $(success,$(CROSS_COMPILE_COMPAT)gcc --version | head -n 1 | grep -q arm)
+config COMPATCC_IS_ARM_GCC
+ def_bool $(success,$(COMPATCC) --version | head -n 1 | grep -q "arm-.*-gcc")
source "arch/arm64/Kconfig.platforms"
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 34f53eb11878..6b4eb5c444c5 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -53,6 +53,9 @@ $(warning Detected assembler with broken .inst; disassembly will be unreliable)
endif
endif
+COMPATCC ?= $(CROSS_COMPILE_COMPAT)gcc
+export COMPATCC
+
ifeq ($(CONFIG_GENERIC_COMPAT_VDSO), y)
export CONFIG_COMPAT_VDSO := y
compat_vdso := -DCONFIG_COMPAT_VDSO=1
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 22f0d31ea528..77aa61340374 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -8,8 +8,6 @@
ARCH_REL_TYPE_ABS := R_ARM_JUMP_SLOT|R_ARM_GLOB_DAT|R_ARM_ABS32
include $(srctree)/lib/vdso/Makefile
-COMPATCC := $(CROSS_COMPILE_COMPAT)gcc
-
# Same as cc-*option, but using COMPATCC instead of CC
cc32-option = $(call try-run,\
$(COMPATCC) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* RE: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
From: Pascal Van Leeuwen @ 2019-09-26 15:04 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A. Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Eric Biggers, Greg KH, Samuel Neves, Will Deacon,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Linus Torvalds, David Miller, linux-arm-kernel
In-Reply-To: <CAKv+Gu-P58Uar2jUNdN5VvG1g45=V_+3FMXCD+0sFY7y2RPeag@mail.gmail.com>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> Of Ard Biesheuvel
> Sent: Thursday, September 26, 2019 4:53 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>
> Subject: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
>
> On Thu, 26 Sep 2019 at 16:03, Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com> wrote:
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Sent: Thursday, September 26, 2019 3:16 PM
> > > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> > > Cc: Jason A. Donenfeld <Jason@zx2c4.com>; Linux Crypto Mailing List <linux-
> > > crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> > > Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg
> KH
> > > <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> > > Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> > > <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski
> <luto@kernel.org>;
> > > Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> > > <catalin.marinas@arm.com>
> > > Subject: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
> > >
> > > On Thu, 26 Sep 2019 at 15:06, Pascal Van Leeuwen
> > > <pvanleeuwen@verimatrix.com> wrote:
> > > ...
> > > > >
> > > > > My preference would be to address this by permitting per-request keys
> > > > > in the AEAD layer. That way, we can instantiate the transform only
> > > > > once, and just invoke it with the appropriate key on the hot path (and
> > > > > avoid any per-keypair allocations)
> > > > >
> > > > This part I do not really understand. Why would you need to allocate a
> > > > new transform if you change the key? Why can't you just call setkey()
> > > > on the already allocated transform?
> > > >
> > >
> > > Because the single transform will be shared between all users running
> > > on different CPUs etc, and so the key should not be part of the TFM
> > > state but of the request state.
> > >
> > So you need a transform per user, such that each user can have his own
> > key. But you shouldn't need to reallocate it when the user changes his
> > key. I also don't see how the "different CPUs" is relevant here? I can
> > share a single key across multiple CPUs here just fine ...
> >
>
> We need two transforms per connection, one for each direction. That is
> how I currently implemented it, and it seems to me that, if
> allocating/freeing those on the same path as where the keypair object
> itself is allocated is too costly, I wonder why allocating the keypair
> object itself is fine.
>
I guess that keypair object is a Wireguard specific thing?
In that case it may not make a difference performance wise.
I just would not expect a new (pair of) transforms to be allocated
just for a rekey, only when a new connection is made.
Thinking about this some more:
Allocating a transform is about more than just allocating the
object, there may be all kinds of side-effects like fallback
ciphers being allocated, specific HW initialization being done, etc.
I just feel that if you only need to change the key, you should
only change the key. As that's what the driver would be optimized
for.
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
From: Ard Biesheuvel @ 2019-09-26 14:52 UTC (permalink / raw)
To: Pascal Van Leeuwen
Cc: Jason A. Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Eric Biggers, Greg KH, Samuel Neves, Will Deacon,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Linus Torvalds, David Miller, linux-arm-kernel
In-Reply-To: <MN2PR20MB297313B598D8EBBE06477B1CCA860@MN2PR20MB2973.namprd20.prod.outlook.com>
On Thu, 26 Sep 2019 at 16:03, Pascal Van Leeuwen
<pvanleeuwen@verimatrix.com> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Thursday, September 26, 2019 3:16 PM
> > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> > Cc: Jason A. Donenfeld <Jason@zx2c4.com>; Linux Crypto Mailing List <linux-
> > crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> > Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> > <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> > Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> > <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> > Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> > <catalin.marinas@arm.com>
> > Subject: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
> >
> > On Thu, 26 Sep 2019 at 15:06, Pascal Van Leeuwen
> > <pvanleeuwen@verimatrix.com> wrote:
> > ...
> > > >
> > > > My preference would be to address this by permitting per-request keys
> > > > in the AEAD layer. That way, we can instantiate the transform only
> > > > once, and just invoke it with the appropriate key on the hot path (and
> > > > avoid any per-keypair allocations)
> > > >
> > > This part I do not really understand. Why would you need to allocate a
> > > new transform if you change the key? Why can't you just call setkey()
> > > on the already allocated transform?
> > >
> >
> > Because the single transform will be shared between all users running
> > on different CPUs etc, and so the key should not be part of the TFM
> > state but of the request state.
> >
> So you need a transform per user, such that each user can have his own
> key. But you shouldn't need to reallocate it when the user changes his
> key. I also don't see how the "different CPUs" is relevant here? I can
> share a single key across multiple CPUs here just fine ...
>
We need two transforms per connection, one for each direction. That is
how I currently implemented it, and it seems to me that, if
allocating/freeing those on the same path as where the keypair object
itself is allocated is too costly, I wonder why allocating the keypair
object itself is fine.
But what I am suggesting is to use a single TFM which gets shared by
all the connections, where the key for each operation is provided
per-request. That TFM cannot have a key set, because each user may use
a different key.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/4] arm64: vdso32: Detect binutils support for dmb ishld
From: Catalin Marinas @ 2019-09-26 14:26 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926133805.52348-3-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 02:38:03PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
> index d6465823b281..75cf8c796d0e 100644
> --- a/arch/arm64/Kbuild
> +++ b/arch/arm64/Kbuild
> @@ -4,3 +4,9 @@ obj-$(CONFIG_NET) += net/
> obj-$(CONFIG_KVM) += kvm/
> obj-$(CONFIG_XEN) += xen/
> obj-$(CONFIG_CRYPTO) += crypto/
> +
> +# as-instr-compat
> +# Usage: cflags-y += $(call as-instr-compat,instr,option1,option2)
> +
> +as-instr-compat = $(call try-run,\
> + printf "%b\n" "$(1)" | $(COMPATCC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
This doesn't seem to be used anywhere. Was it meant to be replaced by
cc32-as-instr?
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 00/11] PCI dma-ranges parsing consolidation
From: Marc Gonzalez @ 2019-09-26 14:09 UTC (permalink / raw)
To: Andrew Murray, Rob Herring
Cc: Mans Rullgard, Ard Biesheuvel, Marc Zyngier, Lorenzo Pieralisi,
Bjorn Helgaas, PCI, Robin Murphy, Linux ARM
In-Reply-To: <20190926133849.GF9720@e119886-lin.cambridge.arm.com>
On 26/09/2019 15:38, Andrew Murray wrote:
> On Thu, Sep 26, 2019 at 08:11:16AM -0500, Rob Herring wrote:
>
>> On Thu, Sep 26, 2019 at 6:20 AM Marc Gonzalez wrote:
>>>
>>> On 26/09/2019 10:49, Andrew Murray wrote:
>>>
>>>> On Tue, Sep 24, 2019 at 04:46:19PM -0500, Rob Herring wrote:
>>>>
>>>>> pci-rcar-gen2 is the only remaining driver doing its own dma-ranges
>>>>> handling as it is still using the old ARM PCI functions. Looks like it
>>>>> is the last one (in drivers/pci/).
>>>>
>>>> It also seems that pcie-tango is using of_pci_dma_range_parser_init
>>>> and so parsing dma-ranges. Though it's using the dma_ranges for a
>>>> slightly different purpose.
>>
>> Seems I missed that as I only grep'ed for for_each_of_pci_range...
>>
>>> The rationale for that code can be found here:
>>>
>>> https://patchwork.kernel.org/patch/9915469/
>>>
>>> NB: 1) The PCIE_TANGO_SMP8759 Kconfig symbol is marked "depends on BROKEN",
>>> and 2) The driver adds TAINT_CRAP,
>>> and 3) The maker of the tango platform is dead.
>
> Thanks for the context Marc, much appreciated.
>
> Is there a path to make this driver not BROKEN? Or is this likely to bit rot?
It is not the device driver that is BROKEN, it is the device being driven ;-)
I don't know how many smp8759 boards exist in the wild. Mans might have one.
I didn't keep mine.
Regards.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] KVM: arm/arm64: Allow user injection of external data aborts
From: Marc Zyngier @ 2019-09-26 14:09 UTC (permalink / raw)
To: Christoffer Dall, kvmarm
Cc: Peter Maydell, Daniel P. Berrangé, Suzuki K Poulose,
Heinrich Schuchardt, Alexander Graf, Julien Thierry,
Stefan Hajnoczi, James Morse, linux-arm-kernel
In-Reply-To: <20190909121337.27287-3-christoffer.dall@arm.com>
On 09/09/2019 13:13, Christoffer Dall wrote:
> In some scenarios, such as buggy guest or incorrect configuration of the
> VMM and firmware description data, userspace will detect a memory access
> to a portion of the IPA, which is not mapped to any MMIO region.
>
> For this purpose, the appropriate action is to inject an external abort
> to the guest. The kernel already has functionality to inject an
> external abort, but we need to wire up a signal from user space that
> lets user space tell the kernel to do this.
>
> It turns out, we already have the set event functionality which we can
> perfectly reuse for this.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
> Documentation/virt/kvm/api.txt | 15 ++++++++++++++-
> arch/arm/include/uapi/asm/kvm.h | 3 ++-
> arch/arm/kvm/guest.c | 3 +++
> arch/arm64/include/uapi/asm/kvm.h | 3 ++-
> arch/arm64/kvm/guest.c | 3 +++
> arch/arm64/kvm/inject_fault.c | 4 ++--
> include/uapi/linux/kvm.h | 1 +
> virt/kvm/arm/arm.c | 1 +
> 8 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt
> index 02501333f746..edd6cdc470ca 100644
> --- a/Documentation/virt/kvm/api.txt
> +++ b/Documentation/virt/kvm/api.txt
> @@ -955,6 +955,8 @@ The following bits are defined in the flags field:
>
> ARM/ARM64:
>
> +User space may need to inject several types of events to the guest.
> +
> If the guest accesses a device that is being emulated by the host kernel in
> such a way that a real device would generate a physical SError, KVM may make
> a virtual SError pending for that VCPU. This system error interrupt remains
> @@ -989,12 +991,23 @@ Specifying exception.has_esr on a system that does not support it will return
> -EINVAL. Setting anything other than the lower 24bits of exception.serror_esr
> will return -EINVAL.
>
> +If the guest performed an access to I/O memory which could not be handled by
> +user space, for example because of missing instruction syndrome decode
> +information or because there is no device mapped at the accessed IPA, then
> +user space can ask the kernel to inject an external abort using the address
> +from the exiting fault on the VCPU. It is a programming error to set
> +ext_dabt_pending at the same time as any of the serror fields, or to set
> +ext_dabt_pending on an exit which was not either KVM_EXIT_MMIO or
... on *re-entry from* an exit?
> +KVM_EXIT_ARM_NISV. This feature is only available if the system supports
> +KVM_CAP_ARM_INJECT_EXT_DABT;
s/;/./
Can we add some wording to the fact that this is only a helper for the
most common case? Most of the ARM exceptions can otherwise be
constructed/injected using the SET_ONE_REG API.
> +
> struct kvm_vcpu_events {
> struct {
> __u8 serror_pending;
> __u8 serror_has_esr;
> + __u8 ext_dabt_pending;
> /* Align it to 8 bytes */
> - __u8 pad[6];
> + __u8 pad[5];
> __u64 serror_esr;
> } exception;
> __u32 reserved[12];
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index a4217c1a5d01..d2449a5bf8d5 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -131,8 +131,9 @@ struct kvm_vcpu_events {
> struct {
> __u8 serror_pending;
> __u8 serror_has_esr;
> + __u8 ext_dabt_pending;
> /* Align it to 8 bytes */
> - __u8 pad[6];
> + __u8 pad[5];
> __u64 serror_esr;
> } exception;
> __u32 reserved[12];
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 684cf64b4033..4154c5589501 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -263,11 +263,14 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> {
> bool serror_pending = events->exception.serror_pending;
> bool has_esr = events->exception.serror_has_esr;
> + bool has_ext_dabt_pending = events->exception.ext_dabt_pending;
>
> if (serror_pending && has_esr)
> return -EINVAL;
> else if (serror_pending)
> kvm_inject_vabt(vcpu);
> + else if (has_ext_dabt_pending)
> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
>
> return 0;
> }
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 9a507716ae2f..7729efdb1c0c 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -164,8 +164,9 @@ struct kvm_vcpu_events {
> struct {
> __u8 serror_pending;
> __u8 serror_has_esr;
> + __u8 ext_dabt_pending;
> /* Align it to 8 bytes */
> - __u8 pad[6];
> + __u8 pad[5];
> __u64 serror_esr;
> } exception;
> __u32 reserved[12];
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index dfd626447482..10e6e2144dca 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -720,6 +720,7 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> {
> bool serror_pending = events->exception.serror_pending;
> bool has_esr = events->exception.serror_has_esr;
> + bool has_ext_dabt_pending = events->exception.ext_dabt_pending;
>
> if (serror_pending && has_esr) {
> if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
> @@ -731,6 +732,8 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> return -EINVAL;
> } else if (serror_pending) {
> kvm_inject_vabt(vcpu);
> + } else if (has_ext_dabt_pending) {
> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> }
>
> return 0;
> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> index a9d25a305af5..ccdb6a051ab2 100644
> --- a/arch/arm64/kvm/inject_fault.c
> +++ b/arch/arm64/kvm/inject_fault.c
> @@ -109,7 +109,7 @@ static void inject_undef64(struct kvm_vcpu *vcpu)
>
> /**
> * kvm_inject_dabt - inject a data abort into the guest
> - * @vcpu: The VCPU to receive the undefined exception
> + * @vcpu: The VCPU to receive the data abort
> * @addr: The address to report in the DFAR
> *
> * It is assumed that this code is called from the VCPU thread and that the
> @@ -125,7 +125,7 @@ void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr)
>
> /**
> * kvm_inject_pabt - inject a prefetch abort into the guest
> - * @vcpu: The VCPU to receive the undefined exception
> + * @vcpu: The VCPU to receive the prefetch abort
> * @addr: The address to report in the DFAR
> *
> * It is assumed that this code is called from the VCPU thread and that the
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index dd79235b6435..a80ee820e700 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1003,6 +1003,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
> #define KVM_CAP_PMU_EVENT_FILTER 173
> #define KVM_CAP_ARM_NISV_TO_USER 174
> +#define KVM_CAP_ARM_INJECT_EXT_DABT 175
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 7153504bb106..56a97dd9b292 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -217,6 +217,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_IMMEDIATE_EXIT:
> case KVM_CAP_VCPU_EVENTS:
> case KVM_CAP_ARM_NISV_TO_USER:
> + case KVM_CAP_ARM_INJECT_EXT_DABT:
> r = 1;
> break;
> case KVM_CAP_ARM_SET_DEVICE_ADDR:
>
Otherwise looks good to me. If you respin the series, and unless anyone
shouts, I'll queue it. No hurry though, I'm going to take slow(er) the
following two weeks.
Thanks,
M.
--
Jazz is not dead, it just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/6] PM / devfreq: Introduce get_freq_range helper
From: Leonard Crestez @ 2019-09-26 14:03 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Matthias Kaehlcke,
Viresh Kumar
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, dl-linux-imx, Krzysztof Kozlowski,
Lukasz Luba, Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <e8ec6c2e-8536-b926-c1fc-468f9e9adca7@samsung.com>
On 2019-09-26 4:02 AM, Chanwoo Choi wrote:
> Hi,
>
> On 19. 9. 26. 오전 5:55, Leonard Crestez wrote:
>> On 25.09.2019 04:32, Chanwoo Choi wrote:
>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>>>> Moving handling of min/max freq to a single function and call it from
>>>> update_devfreq and for printing min/max freq values in sysfs.
>>>>
>>>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>>>> is now done at evaluation time. This means that if an out-of-range
>>>> constraint is imposed by sysfs and it later becomes valid then it will
>>>> be enforced.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>> drivers/devfreq/devfreq.c | 112 ++++++++++++++++++++++----------------
>>>> 1 file changed, 64 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 4a878baa809e..eee403e70c84 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>>> dev_pm_opp_put(opp);
>>>>
>>>> return max_freq;
>>>> }
>>>>
>>>> +/**
>>>> + * get_freq_range() - Get the current freq range
>>>> + * @devfreq: the devfreq instance
>>>> + * @min_freq: the min frequency
>>>> + * @max_freq: the max frequency
>>>> + *
>>>> + * This takes into consideration all constraints.
>>>> + */
>>>> +static void get_freq_range(struct devfreq *devfreq,
>>>> + unsigned long *min_freq,
>>>> + unsigned long *max_freq)
>>>> +{
>>>> + unsigned long *freq_table = devfreq->profile->freq_table;
>>>> +
>>>> + lockdep_assert_held(&devfreq->lock);
>>>> +
>>>> + /*
>>>> + * Init min/max frequency from freq table.
>>>
>>> Init -> Initialize
>>> min/max -> minimum/maximum
>>>
>>>> + * Drivers can initialize this in either ascending or descending order
>>>
>>> Drivers -> devfreq drivers
>>>
>>>> + * and devfreq core supports both.
>>>> + */
>>>
>>>
>>> In result, I prefer to change the comments as following:
>>> /*
>>> * Initialize the minimum/maximum frequency from freq_table.
>>> * The devfreq drivers can initialize freq_table in either
>>> * ascending or descending order and devfreq core supports both.
>>> */
>>
>> OK
>>
>>>> + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
>>>> + *min_freq = freq_table[0];
>>>> + *max_freq = freq_table[devfreq->profile->max_state - 1];
>>>> + } else {
>>>> + *min_freq = freq_table[devfreq->profile->max_state - 1];
>>>> + *max_freq = freq_table[0];
>>>> + }
>>>> +
>>>> + /* constraints from sysfs */
>>>
>>> 'constraints' -> Constraint because first verb have to be used
>>> as the sigular verbs. Also, I think that have to enhance the comments
>>> I prefer to use following comments:
>>>
>>> /* Constraint minimum/maximum frequency from user input via sysfs */
>>>
>>>
>>>
>>>> + *min_freq = max(*min_freq, devfreq->min_freq);
>>>> + *max_freq = min(*max_freq, devfreq->max_freq);
>>>> +
>>>> + /* constraints from OPP interface */
>>>
>>> ditto. I prefer to use following comments:
>>>
>>> /* Constraint minimum/maximum frequency from OPP interface */
>>>
>>>
>>>> + *min_freq = max(*min_freq, devfreq->scaling_min_freq);
>>>> + /* scaling_max_freq can be zero on error */
>>>
>>> Please remove it.
>>>
>>>> + if (devfreq->scaling_max_freq)
>>>
>>> As I knew, devfreq->scaling_max_freq is never zero.
>>> So, it is always true. This if statement is needed.
>>
>> It can happen if find_available_max_freq encounters an error when called
>> from devfreq_notifier_call.
>
> If you are wondering this case, I think that have to fix
> the possible issue on there instead of this point.
>
>>
>> Maybe that should be separately fixed to set scaling_max_freq to a
>> neutral value such as "ULONG_MAX" instead?
>
> OK.
>
>>
>> BTW: the devfreq_notifier_call function returns -EINVAL on error but it
>> should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP
>> framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates
>> as true so other notifiers will be skipped unintentionally.
>
> I agree. It is needed to fix the return value type.
Will include as separate patches in v9.
>>>> + *max_freq = min(*max_freq, devfreq->scaling_max_freq);
>>>> +
>>>> + /* max_freq takes precedence over min_freq */
>>>
>>> As I said, almost people know that min_freq have be under than max_freq.
>>> Please remove it. And until finishing the discussion on mailing list,
>>> please don't send the next version. If you just replied from my comment
>>> and then wait my next comment, we can save the time without replying
>>> the repetitive and same comment for same point.
>>
>> This series makes it possible to set a min_freq higher than max_freq
>> (for example via PM QoS from various devices).
>>
>> It is not obvious that min_freq takes precedence over max_freq but the
>> code is self-evident so I will remove the comment.
>>
>>>> + if (*min_freq > *max_freq)
>>>> + *min_freq = *max_freq;
>>>> +}
>>>> +
>>>> /**
>>>> * devfreq_get_freq_level() - Lookup freq_table for the frequency
>>>> * @devfreq: the devfreq instance
>>>> * @freq: the target frequency
>>>> */
>>>> @@ -349,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq)
>>>>
>>>> /* Reevaluate the proper frequency */
>>>> err = devfreq->governor->get_target_freq(devfreq, &freq);
>>>> if (err)
>>>> return err;
>>>> -
>>>> - /*
>>>> - * Adjust the frequency with user freq, QoS and available freq.
>>>> - *
>>>> - * List from the highest priority
>>>> - * max_freq
>>>> - * min_freq
>>>> - */
>>>> - max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
>>>> - min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
>>>> + get_freq_range(devfreq, &min_freq, &max_freq);
>>>>
>>>> if (freq < min_freq) {
>>>> freq = min_freq;
>>>> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>>> }
>>>> @@ -1298,40 +1333,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>> ret = sscanf(buf, "%lu", &value);
>>>> if (ret != 1)
>>>> return -EINVAL;
>>>>
>>>> mutex_lock(&df->lock);
>>>> -
>>>> - if (value) {
>>>> - if (value > df->max_freq) {
>>>> - ret = -EINVAL;
>>>> - goto unlock;
>>>> - }
>>>> - } else {
>>>> - unsigned long *freq_table = df->profile->freq_table;
>>>> -
>>>> - /* Get minimum frequency according to sorting order */
>>>> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>>> - value = freq_table[0];
>>>> - else
>>>> - value = freq_table[df->profile->max_state - 1];
>>>> - }
>>>> -
>>>> df->min_freq = value;
>>>> update_devfreq(df);
>>>> - ret = count;
>>>> -unlock:
>>>> mutex_unlock(&df->lock);
>>>> - return ret;
>>>> +
>>>> + return count;
>>>> }
>>>>
>>>> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>>> char *buf)
>>>> {
>>>> struct devfreq *df = to_devfreq(dev);
>>>> + unsigned long min_freq, max_freq;
>>>> +
>>>> + mutex_lock(&df->lock);
>>>> + get_freq_range(df, &min_freq, &max_freq);
>>>> + mutex_unlock(&df->lock);
>>>>
>>>> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
>>>> + return sprintf(buf, "%lu\n", min_freq);
>>>> }
>>>>
>>>> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>> const char *buf, size_t count)
>>>> {
>>>> @@ -1343,40 +1366,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>> if (ret != 1)
>>>> return -EINVAL;
>>>>
>>>> mutex_lock(&df->lock);
>>>>
>>>> - if (value) {
>>>> - if (value < df->min_freq) {
>>>> - ret = -EINVAL;
>>>> - goto unlock;
>>>> - }
>>>> - } else {
>>>> - unsigned long *freq_table = df->profile->freq_table;
>>>> -
>>>> - /* Get maximum frequency according to sorting order */
>>>> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>>> - value = freq_table[df->profile->max_state - 1];
>>>> - else
>>>> - value = freq_table[0];
>>>> - }
>>>> + /* Interpret zero as "don't care" */
>>>
>>> Please remove it. Also, the detailed comment for user have to add
>>> the documentation file.
>>
>> OK
>>
>>>
>>>> + if (!value)
>>>> + value = ULONG_MAX;
>>>>
>>>> df->max_freq = value;
>>>> update_devfreq(df);
>>>> - ret = count;
>>>> -unlock:
>>>> mutex_unlock(&df->lock);
>>>> - return ret;
>>>> +
>>>> + return count;
>>>> }
>>>> static DEVICE_ATTR_RW(min_freq);
>>>>
>>>> static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>>> char *buf)
>>>> {
>>>> struct devfreq *df = to_devfreq(dev);
>>>> + unsigned long min_freq, max_freq;
>>>> +
>>>> + mutex_lock(&df->lock);
>>>> + get_freq_range(df, &min_freq, &max_freq);
>>>> + mutex_unlock(&df->lock);
>>>>
>>>> - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
>>>> + return sprintf(buf, "%lu\n", max_freq);
>>>> }
>>>> static DEVICE_ATTR_RW(max_freq);
>>>>
>>>> static ssize_t available_frequencies_show(struct device *d,
>>>> struct device_attribute *attr,
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
From: Pascal Van Leeuwen @ 2019-09-26 14:03 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A. Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Eric Biggers, Greg KH, Samuel Neves, Will Deacon,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Linus Torvalds, David Miller, linux-arm-kernel
In-Reply-To: <CAKv+Gu_eNK1HFxTY379kpCpF8FQQFHEdC1Th=s5f7Fy3bebOjQ@mail.gmail.com>
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, September 26, 2019 3:16 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>
> Subject: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API
>
> On Thu, 26 Sep 2019 at 15:06, Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com> wrote:
> ...
> > >
> > > My preference would be to address this by permitting per-request keys
> > > in the AEAD layer. That way, we can instantiate the transform only
> > > once, and just invoke it with the appropriate key on the hot path (and
> > > avoid any per-keypair allocations)
> > >
> > This part I do not really understand. Why would you need to allocate a
> > new transform if you change the key? Why can't you just call setkey()
> > on the already allocated transform?
> >
>
> Because the single transform will be shared between all users running
> on different CPUs etc, and so the key should not be part of the TFM
> state but of the request state.
>
So you need a transform per user, such that each user can have his own
key. But you shouldn't need to reallocate it when the user changes his
key. I also don't see how the "different CPUs" is relevant here? I can
share a single key across multiple CPUs here just fine ...
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: chapoly acceleration hardware [Was: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API]
From: Pascal Van Leeuwen @ 2019-09-26 13:52 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Toke Høiland-Jørgensen, Catalin Marinas, Herbert Xu,
Arnd Bergmann, Ard Biesheuvel, Greg KH, Eric Biggers, Dave Taht,
Willy Tarreau, Samuel Neves, Will Deacon, Netdev,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Linus Torvalds, David Miller, linux-arm-kernel
In-Reply-To: <CAHmME9r5m7D-oMU6Lv_ZhEyWmrNscMr5HokzdK0wg2Ayzzbeow@mail.gmail.com>
> -----Original Message-----
> From: Jason A. Donenfeld <Jason@zx2c4.com>
> Sent: Thursday, September 26, 2019 1:07 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>; Willy Tarreau <w@1wt.eu>; Netdev <netdev@vger.kernel.org>;
> Toke Høiland-Jørgensen <toke@toke.dk>; Dave Taht <dave.taht@gmail.com>
> Subject: chapoly acceleration hardware [Was: Re: [RFC PATCH 00/18] crypto: wireguard
> using the existing crypto API]
>
> [CC +willy, toke, dave, netdev]
>
> Hi Pascal
>
> On Thu, Sep 26, 2019 at 12:19 PM Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com> wrote:
> > Actually, that assumption is factually wrong. I don't know if anything
> > is *publicly* available, but I can assure you the silicon is running in
> > labs already. And something will be publicly available early next year
> > at the latest. Which could nicely coincide with having Wireguard support
> > in the kernel (which I would also like to see happen BTW) ...
> >
> > Not "at some point". It will. Very soon. Maybe not in consumer or server
> > CPUs, but definitely in the embedded (networking) space.
> > And it *will* be much faster than the embedded CPU next to it, so it will
> > be worth using it for something like bulk packet encryption.
>
> Super! I was wondering if you could speak a bit more about the
> interface. My biggest questions surround latency. Will it be
> synchronous or asynchronous?
>
The hardware being external to the CPU and running in parallel with it,
obviously asynchronous.
> If the latter, why?
>
Because, as you probably already guessed, the round-trip latency is way
longer than the actual processing time, at least for small packets.
Partly because the only way to communicate between the CPU and the HW
accelerator (whether that is crypto, a GPU, a NIC, etc.) that doesn't
keep the CPU busy moving data is through memory, with the HW doing DMA.
And, as any programmer should now, round trip times to memory are huge
relative to the processing speed.
And partly because these accelerators are very similar to CPU's in
terms of architecture, doing pipelined processing and having multiple
of such pipelines in parallel. Except that these pipelines are not
working on low-level instructions but on full packets/blocks. So they
need to have many packets in flight to keep those pipelines fully
occupied. And packets need to move through the various pipeline stages,
so they incur the time needed to process them multiple times. (just
like e.g. a multiply instruction with a throughput of 1 per cycle
actually may need 4 or more cycles to actually provide its result)
Could you do that from a synchronous interface? In theory, probably,
if you would spawn a new thread for every new packet arriving and
rely on the scheduler to preempt the waiting threads. But you'd need
as many threads as the HW accelerator can have packets in flight,
while an async would need only 2 threads: one to handle the input to
the accelerator and one to handle the output (or at most one thread
per CPU, if you want to divide the workload)
Such a many-thread approach seems very inefficient to me.
> What will its latencies be?
>
Depends very much on the specific integration scenario (i.e. bus
speed, bus hierarchy, cache hierarchy, memory speed, etc.) but on
the order of a few thousand CPU clocks is not unheard of.
Which is an eternity for the CPU, but still only a few uSec in
human time. Not a problem unless you're a high-frequency trader and
every ns counts ...
It's not like the CPU would process those packets in zero time.
> How deep will its buffers be?
>
That of course depends on the specific accelerator implementation,
but possibly dozens of small packets in our case, as you'd need
at least width x depth packets in there to keep the pipes busy.
Just like a modern CPU needs hundreds of instructions in flight
to keep all its resources busy.
> The reason I ask is that a
> lot of crypto acceleration hardware of the past has been fast and
> having very deep buffers, but at great expense of latency.
>
Define "great expense". Everything is relative. The latency is very
high compared to per-packet processing time but at the same time it's
only on the order of a few uSec. Which may not even be significant on
the total time it takes for the packet to travel from input MAC to
output MAC, considering the CPU will still need to parse and classify
it and do pre- and postprocessing on it.
> In the networking context, keeping latency low is pretty important.
>
I've been doing this for IPsec for nearly 20 years now and I've never
heard anyone complain about our latency, so it must be OK.
We're also doing (fully inline, no CPU involved) MACsec cores, which
operate at layer 2 and I know it's a concern there for very specific
use cases (high frequency trading, precision time protocol, ...).
For "normal" VPN's though, a few uSec more or less should be a non-issue.
> Already
> WireGuard is multi-threaded which isn't super great all the time for
> latency (improvements are a work in progress). If you're involved with
> the design of the hardware, perhaps this is something you can help
> ensure winds up working well? For example, AES-NI is straightforward
> and good, but Intel can do that because they are the CPU. It sounds
> like your silicon will be adjacent. How do you envision this working
> in a low latency environment?
>
Depends on how low low-latency is. If you really need minimal latency,
you need an inline implementation. Which we can also provide, BTW :-)
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox