* [PATCH v10 15/18] KVM: arm64: Remove eager host SVE state saving
From: Alex Bennée @ 2018-05-24 14:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527005119-6842-16-git-send-email-Dave.Martin@arm.com>
Dave Martin <Dave.Martin@arm.com> writes:
> Now that the host SVE context can be saved on demand from Hyp,
> there is no longer any need to save this state in advance before
> entering the guest.
>
> This patch removes the relevant call to
> kvm_fpsimd_flush_cpu_state().
>
> Since the problem that function was intended to solve now no longer
> exists, the function and its dependencies are also deleted.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Acked-by: Christoffer Dall <christoffer.dall@arm.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
> arch/arm/include/asm/kvm_host.h | 3 ---
> arch/arm64/include/asm/kvm_host.h | 10 ----------
> arch/arm64/kernel/fpsimd.c | 21 ---------------------
> virt/kvm/arm/arm.c | 3 ---
> 4 files changed, 37 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 3b85bbb..f079a20 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -312,9 +312,6 @@ static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
>
> -/* All host FP/SIMD state is restored on guest exit, so nothing to save: */
> -static inline void kvm_fpsimd_flush_cpu_state(void) {}
> -
> static inline void kvm_arm_vhe_guest_enter(void) {}
> static inline void kvm_arm_vhe_guest_exit(void) {}
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 06d5a61..ce7ed92 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -457,16 +457,6 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> }
> #endif
>
> -/*
> - * All host FP/SIMD state is restored on guest exit, so nothing needs
> - * doing here except in the SVE case:
> -*/
> -static inline void kvm_fpsimd_flush_cpu_state(void)
> -{
> - if (system_supports_sve())
> - sve_flush_cpu_state();
> -}
> -
> static inline void kvm_arm_vhe_guest_enter(void)
> {
> local_daif_mask();
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index f39d3b0..ea5d780 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -120,7 +120,6 @@
> */
> struct fpsimd_last_state_struct {
> struct user_fpsimd_state *st;
> - bool sve_in_use;
> };
>
> static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
> @@ -1003,7 +1002,6 @@ void fpsimd_bind_task_to_cpu(void)
> this_cpu_ptr(&fpsimd_last_state);
>
> last->st = ¤t->thread.uw.fpsimd_state;
> - last->sve_in_use = test_thread_flag(TIF_SVE);
> current->thread.fpsimd_cpu = smp_processor_id();
>
> if (system_supports_sve()) {
> @@ -1025,7 +1023,6 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
> WARN_ON(!in_softirq() && !irqs_disabled());
>
> last->st = st;
> - last->sve_in_use = false;
> }
>
> /*
> @@ -1086,24 +1083,6 @@ void fpsimd_flush_cpu_state(void)
> set_thread_flag(TIF_FOREIGN_FPSTATE);
> }
>
> -/*
> - * Invalidate any task SVE state currently held in this CPU's regs.
> - *
> - * This is used to prevent the kernel from trying to reuse SVE register data
> - * that is detroyed by KVM guest enter/exit. This function should go away when
> - * KVM SVE support is implemented. Don't use it for anything else.
> - */
> -#ifdef CONFIG_ARM64_SVE
> -void sve_flush_cpu_state(void)
> -{
> - struct fpsimd_last_state_struct const *last =
> - this_cpu_ptr(&fpsimd_last_state);
> -
> - if (last->st && last->sve_in_use)
> - fpsimd_flush_cpu_state();
> -}
> -#endif /* CONFIG_ARM64_SVE */
> -
> #ifdef CONFIG_KERNEL_MODE_NEON
>
> DEFINE_PER_CPU(bool, kernel_neon_busy);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index ce7c6f3..39e7771 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -682,9 +682,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> */
> preempt_disable();
>
> - /* Flush FP/SIMD state that can't survive guest entry/exit */
> - kvm_fpsimd_flush_cpu_state();
> -
> kvm_pmu_flush_hwstate(vcpu);
>
> local_irq_disable();
--
Alex Benn?e
^ permalink raw reply
* [PATCH 6/9] ARM: dts: wheat: Drop MTD partitioning from DT
From: Marek Vasut @ 2018-05-24 14:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdXnNABJSJZ6MLF+aQu8QKpFoMCAvno9Yc-9LOe810NtbA@mail.gmail.com>
On 05/23/2018 08:25 AM, Geert Uytterhoeven wrote:
> Hi Marek,
>
> On Wed, May 23, 2018 at 12:01 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 05/22/2018 04:43 PM, Geert Uytterhoeven wrote:
>>> On Tue, May 22, 2018 at 2:02 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> Drop the MTD partitioning from DT, since it does not describe HW
>>>> and to give way to a more flexible kernel command line partition
>>>> passing.
>>>>
>>>> To retain the original partitioning, assure you have enabled
>>>> CONFIG_MTD_CMDLINE_PARTS in your kernel config and add the
>>>> following to your kernel command line:
>>>>
>>>> mtdparts=spi0.0:256k at 0(loader),4096k(user),-(flash)
>>>
>>> I think the "@0" can be dropped, as it's optional?
>>> 4m?
>>
>> My take on this is that the loader is actually at offset 0x0 of the MTD
>> device and we explicitly state that in the mtdparts to anchor the first
>> partition within the MTD device and all the other partitions are at
>> offset +(sum of the sizes of all partitions listed before the current
>> one) relative to that first partition.
>
> Where is this explicitly states for the first partition?
>
>> Removing the @0 feels fragile at best and it seems to depend on the
>> current behavior of the code.
>
> Better, it also depends on the documented behavior:
>
> Documentation/admin-guide/kernel-parameters.txt refers to
> drivers/mtd/cmdlinepart.c, which states:
>
> * <offset> := standard linux memsize
> * if omitted the part will immediately follow the previous part
> * or 0 if the first part
>
> None of the examples listed there or under the MTD_CMDLINE_PARTS Kconfig
> help text, or in a defconfig bundled with the kernel, use @0 for the first
> partition.
I think this is exceptionally fragile and dangerous to depend on this,
but so be it.
--
Best regards,
Marek Vasut
^ permalink raw reply
* [PATCH v10 14/18] KVM: arm64: Save host SVE context as appropriate
From: Alex Bennée @ 2018-05-24 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527005119-6842-15-git-send-email-Dave.Martin@arm.com>
Dave Martin <Dave.Martin@arm.com> writes:
> This patch adds SVE context saving to the hyp FPSIMD context switch
> path. This means that it is no longer necessary to save the host
> SVE state in advance of entering the guest, when in use.
>
> In order to avoid adding pointless complexity to the code, VHE is
> assumed if SVE is in use. VHE is an architectural prerequisite for
> SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> kernels that support both SVE and KVM.
>
> Historically, software models exist that can expose the
> architecturally invalid configuration of SVE without VHE, so if
> this situation is detected at kvm_init() time then KVM will be
> disabled.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> ---
>
> * Tags stripped since v8, please reconfirm if possible:
>
> Formerly-Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> Formerly-Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Formerly-Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Changes since v9:
>
> Requested by Marc Zyngier:
>
> * Inline check for VHE if SVE is present into kvm_host.h.
>
> The check has been renamed to the more specific
> kvm_arch_check_sve_has_vhe(), and the kvm_pr_unimpl() moved back to
> arm.c (to avoid circular include issues).
>
> arm.c is not single-arch code, but it is all Arm-specific, so
> adding a hook like this doesn't seem too unreasonable.
>
> Changes since v8:
>
> * Add kvm_arch_check_supported() hook, and move arm64-specific check
> for SVE-implies-VHE into arch/arm64/.
>
> Due to circular header dependency problems, it is difficult to get
> the prototype for kvm_pr_*() functions in <asm/kvm_host.h>, so this
> patch puts arm64's kvm_arch_check_supported() hook out of line.
> This is not a hot function.
> ---
> arch/arm/include/asm/kvm_host.h | 1 +
> arch/arm64/Kconfig | 7 +++++++
> arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
> arch/arm64/kvm/fpsimd.c | 1 -
> arch/arm64/kvm/hyp/switch.c | 20 +++++++++++++++++++-
> virt/kvm/arm/arm.c | 7 +++++++
> 6 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index ac870b2..3b85bbb 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -280,6 +280,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>
> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>
> +static inline bool kvm_arch_check_sve_has_vhe(void) { return true; }
> static inline void kvm_arch_hardware_unsetup(void) {}
> static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf49..b0d3820 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1130,6 +1130,7 @@ endmenu
> config ARM64_SVE
> bool "ARM Scalable Vector Extension support"
> default y
> + depends on !KVM || ARM64_VHE
> help
> The Scalable Vector Extension (SVE) is an extension to the AArch64
> execution state which complements and extends the SIMD functionality
> @@ -1155,6 +1156,12 @@ config ARM64_SVE
> booting the kernel. If unsure and you are not observing these
> symptoms, you should assume that it is safe to say Y.
>
> + CPUs that support SVE are architecturally required to support the
> + Virtualization Host Extensions (VHE), so the kernel makes no
> + provision for supporting SVE alongside KVM without VHE enabled.
> + Thus, you will need to enable CONFIG_ARM64_VHE if you want to support
> + KVM in the same kernel image.
> +
> config ARM64_MODULE_PLTS
> bool
> select HAVE_MOD_ARCH_SPECIFIC
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index b3fe730..06d5a61 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -405,6 +405,19 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
> kvm_call_hyp(__kvm_set_tpidr_el2, tpidr_el2);
> }
>
> +static inline bool kvm_arch_check_sve_has_vhe(void)
> +{
> + /*
> + * The Arm architecture specifies that imlpementation of SVE
> + * requires VHE also to be implemented. The KVM code for arm64
> + * relies on this when SVE is present:
> + */
> + if (system_supports_sve())
> + return has_vhe();
> + else
> + return true;
> +}
> +
> static inline void kvm_arch_hardware_unsetup(void) {}
> static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 365933a..dc6ecfa 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -59,7 +59,6 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
> */
> void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> {
> - BUG_ON(system_supports_sve());
> BUG_ON(!current->mm);
>
> vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_HOST_SVE_IN_USE);
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 118f300..a6a8c7d 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -21,6 +21,7 @@
>
> #include <kvm/arm_psci.h>
>
> +#include <asm/cpufeature.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_emulate.h>
> #include <asm/kvm_host.h>
> @@ -28,6 +29,7 @@
> #include <asm/kvm_mmu.h>
> #include <asm/fpsimd.h>
> #include <asm/debug-monitors.h>
> +#include <asm/processor.h>
> #include <asm/thread_info.h>
>
> /* Check whether the FP regs were dirtied while in the host-side run loop: */
> @@ -329,6 +331,8 @@ static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> struct kvm_vcpu *vcpu)
> {
> + struct user_fpsimd_state *host_fpsimd = vcpu->arch.host_fpsimd_state;
> +
> if (has_vhe())
> write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
> cpacr_el1);
> @@ -339,7 +343,21 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> isb();
>
> if (vcpu->arch.flags & KVM_ARM64_FP_HOST) {
> - __fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> + /*
> + * In the SVE case, VHE is assumed: it is enforced by
> + * Kconfig and kvm_arch_init().
> + */
> + if (system_supports_sve() &&
> + (vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE)) {
> + struct thread_struct *thread = container_of(
> + host_fpsimd,
> + struct thread_struct, uw.fpsimd_state);
> +
> + sve_save_state(sve_pffr(thread), &host_fpsimd->fpsr);
> + } else {
> + __fpsimd_save_state(host_fpsimd);
> + }
> +
> vcpu->arch.flags &= ~KVM_ARM64_FP_HOST;
> }
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index bee226c..ce7c6f3 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -16,6 +16,7 @@
> * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> */
>
> +#include <linux/bug.h>
> #include <linux/cpu_pm.h>
> #include <linux/errno.h>
> #include <linux/err.h>
> @@ -41,6 +42,7 @@
> #include <asm/mman.h>
> #include <asm/tlbflush.h>
> #include <asm/cacheflush.h>
> +#include <asm/cpufeature.h>
> #include <asm/virt.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> @@ -1574,6 +1576,11 @@ int kvm_arch_init(void *opaque)
> return -ENODEV;
> }
>
> + if (!kvm_arch_check_sve_has_vhe()) {
> + kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?");
> + return -ENODEV;
> + }
> +
Ahh this is going to be a pain when people want to enable system
emulation for SVE in QEMU given our patchy feature implementation (i.e.
we haven't done VHE yet). However that's totally our problem not yours
;-)
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
> for_each_online_cpu(cpu) {
> smp_call_function_single(cpu, check_kvm_target_cpu, &ret, 1);
> if (ret < 0) {
--
Alex Benn?e
^ permalink raw reply
* [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks
From: Dave Martin @ 2018-05-24 14:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524100659.GO55598@C02W217FHV2R.local>
On Thu, May 24, 2018 at 12:06:59PM +0200, Christoffer Dall wrote:
> On Thu, May 24, 2018 at 10:50:56AM +0100, Dave Martin wrote:
> > On Thu, May 24, 2018 at 10:33:50AM +0200, Christoffer Dall wrote:
[...]
> > > ...with a risk of being a bit over-pedantic and annoying, may I suggest
> > > the following complete commit text:
> > >
> > > ------8<------
> > > Currently the FPSIMD handling code uses the condition task->mm ==
> > > NULL as a hint that task has no FPSIMD register context.
> > >
> > > The ->mm check is only there to filter out tasks that cannot
> > > possibly have FPSIMD context loaded, for optimisation purposes.
> > > However, TIF_FOREIGN_FPSTATE must always be checked anyway before
> > > saving FPSIMD context back to memory. For this reason, the ->mm
> > > checks are not useful, providing that that TIF_FOREIGN_FPSTATE is
> > > maintained properly for kernel threads.
> > >
> > > FPSIMD context is never preserved for kernel threads across a context
> > > switch and therefore TIF_FOREIGN_FPSTATE should always be true for
> >
> > (This refactoring opens up the interesting possibility of making
> > kernel-mode NEON in task context preemptible for kernel threads so
> > that we actually do preserve state... but that's a discussion for
> > another day. There may be code around that relies on
> > kernel_neon_begin() disabling preemption for real.)
> >
> > > kernel threads. This is indeed the case, as the wrong_task and
> >
> > This suggests that TIF_FOREIGN_FPSTATE is always true for kernel
> > threads today. This is not quite because use_mm() can make mm non-
> > NULL.
> >
>
> I was suggesting that it's always true after this patch.
I tend to read the present tense as describing the situation before the
patch, but this convention isn't followed universally.
This was part of the problem with my "true by construction" weasel
words: the described property wasn't true by construction prior to the
patch, and there wasn't sufficient explanation to convince people it's
true afterwards. If people are bring rigorous, it takes a _lot_ of
explanation...
>
> > > wrong_cpu tests in fpsimd_thread_switch() will always yield false for
> > > kernel threads.
> >
> > ("false" -> "true". My bad.)
> >
> > > Further, the context switch logic is already deliberately optimised to
> > > defer reloads of the FPSIMD context until ret_to_user (or sigreturn as a
> > > special case), which kernel threads by definition never reach, and
> > > therefore this change introduces no additional work in the critical
> > > path.
> > >
> > > This patch removes the redundant checks and special-case code.
> > > ------8<------
> >
> > Looking at my existing text, I rather reworded it like this.
> > Does this work any better for you?
> >
> > --8<--
> >
> > Currently the FPSIMD handling code uses the condition task->mm ==
> > NULL as a hint that task has no FPSIMD register context.
> >
> > The ->mm check is only there to filter out tasks that cannot
> > possibly have FPSIMD context loaded, for optimisation purposes.
> > Also, TIF_FOREIGN_FPSTATE must always be checked anyway before
> > saving FPSIMD context back to memory. For these reasons, the ->mm
> > checks are not useful, providing that TIF_FOREIGN_FPSTATE is
> > maintained in a consistent way for kernel threads.
>
> Consistent with what? Without more context or explanation,
Consistent with the handling of user threads (though I admit it's not
explicit in the text.)
> I'm not sure what the reader is to make of that. Do you not mean the
> TIF_FOREIGN_FPSTATE is always true for kernel threads?
Again, this is probably a red herring. TIF_FOREIGN_FPSTATE is always
true for kernel threads prior to the patch, except (randomly) for the
init task.
This change is not really about TIF_FOREIGN_FPSTATE at all, rather
that there is nothing to justify handling kernel threads differently,
or even distinguishing kernel threads from user threads at all in this
code.
Part of the confusion (and I had confused myself) comes from the fact
that TIF_FOREIGN_FPSTATE is really a per-cpu property and doesn't make
sense as a per-task property -- i.e., the flag is meaningless for
scheduled-out tasks and we must explicitly "repair" it when scheduling
a task in anyway. I think it's a thread flag primarily so that it's
convenient to check alongside other thread flags in the ret_to_user
work loop. This is somewhat less of a justification now that loop was
ported to C.
> >
> > The context switch logic is already deliberately optimised to defer
> > reloads of the regs until ret_to_user (or sigreturn as a special
> > case), and save them only if they have been previously loaded.
Does it help to insert the following here?
"These paths are the only places where the wrong_task and wrong_cpu
conditions can be made false, by calling fpsimd_bind_task_to_cpu()."
> > Kernel threads by definition never reach these paths. As a result,
>
> I'm struggling with the "As a result," here. Is this because reloads of
> regs in ret_to_user (or sigreturn) are the only places that can make
> wrong_cpu or wrong_task be false?
See the proposed clarification above. Is that sufficient?
> (I'm actually wanting to understand this, not just bikeshedding the
> commit message, as new corner cases keep coming up on this logic.)
That's a good thing, and I would really like to explain it in a
concise manner. See [*] below for the "concise" explanation -- it may
demonstrate why I've been evasive...
> > the wrong_task and wrong_cpu tests in fpsimd_thread_switch() will
> > always yield true for kernel threads.
> >
> > This patch removes the redundant checks and special-case code, ensuring that TIF_FOREIGN_FPSTATE is set whenever a kernel thread is scheduled in, and ensures that this flag is set for the init
> > task. The fpsimd_flush_task_state() call already present in copy_thread() ensures the same for any new task.
>
> nit: funny formatting
Dang, I was repeatedly pasing between Mutt and git commit terminals,
which doesn't always work as I'd like...
> nit: ensuring that TIF_FOREIGN_FPSTATE *remains* set whenever a kernel
> thread is scheduled in?
Er, yes.
> > With TIF_FOREIGN_FPSTATE always set for kernel threads, this patch
> > ensures that no extra context save work is added for kernel
> > threads, and eliminates the redundant context saving that may
> > currently occur for kernel threads that have acquired an mm via
> > use_mm().
> >
> > -->8--
>
> If you can slightly connect the dots with the "As a result" above, I'm
> fine with your version of the text.
As an aside, the big wall of text before the definition of struct
fpsimd_last_state_struct is looking out of date and could use an
update to cover at least some of what is explained in [*] better.
I'm currently considering that out of scope for this series, but I will
keep it in mind to refresh it in the not too distant future.
Cheers
---Dave
--8<--
[*] The bigger picture:
* Consider a relation (C,T) between cpus C and tasks T, such that
(C,T) means "T's FPSIMD regs are loaded on cpu C".
At a given point of execution of some cpu C, there is at most one task
T for which (C,T) holds.
At a given point of execution of some task T, there is at most one
cpu C for which (C,T) holds.
* (C,T) becomes true whenever T's registers are loaded into cpu C.
* At sched-out, we must ensure that the registers of current are
loaded before writing them to current's thread_struct. Thus, we
must save the registers if and only if (smp_processor_id(), current)
holds@this time.
* Before entering userspace, we must ensure that current's regs
are loaded, and we must only load the regs if they are not loaded
already (since if so, they might have been dirtied by current in
userspace since last loaded).
Thus, when entering userspace, we must load the regs from memory
if and only if (smp_processor_id(), current) does not hold.
* Checking this relation involves per-CPU access and inspection of
current->thread, and was presumably considered too cumbersome for
implemenation an entry.S, particluarly in the ret_to_user work
pending loop (which is where the FPSIMD regs are finally loaded
before entering userspace, if they weren't loaded already).
To mitigate this, the status of the check is cached in a thread flag
TIF_FOREIGN_FPSTATE: with softirqs disabled, (smp_processor_id(),
current) holds if and only if TIF_FOREIGN_FPSTATE is false.
TIF_FOREIGN_FPSTATE is corrected on sched-in by the code in
fpsimd_thread_switch().
[2] Anything that changes the state of the relation for current
requires its TIF_FOREIGN_FPSTATE to be changed to match.
* (smp_processor_id(), current) is established in
fpsimd_bind_task_to_cpu(). This is the only way the relation can be
made to hold between a task and a CPU.
* (C,T) is broken whenever
[1] T is created;
* T's regs are loaded onto a different cpu C2, so (C2,T) becomes
true and (C,T) necessarily becomes false;
* another task's regs are loaded into C, so (C,T2) becomes true
and (C,T) necessarily becomes false;
* the kernel clobbers the regs on C for its own purposes, so
(C,T) becomes false but there is no T2 for which (C,T2) becomes
true as a result. Examples are kernel-mode NEON and loading
the regs for a KVM vcpu;
* T's register context changes via a thread_struct update instead
of running instructions in userspace, requiring the contents of
the hardware regs to be thrown away. Examples are exec() (which
requires the registers to be zeroed), sigreturn (which populates the
regs from the user signal frame) and modification of the registers
via PTRACE_SETREGSET;
As a (probably unnecesary) optimisation, sigreturn immediately
loads the registers and reestablishes (smp_processor_id(), current)
in anticipation of the return to userspace which is likely to
occur soon. This allows the relation breaking logic to be omitted
in fpsimd_update_current_state() which does the work.
* In general, these relation breakings involve an unknown: knowing
either C or T but *not* both, we want to break (C,T). If the
relation were recorded in task_struct only, we would need to scan all
tasks in the "T unknown" case. If the relation were recorded in a
percpu variable only, we would need to scan all CPUs in the "C
unknown" case. As well as having gnarly synchronisation
requirements, these would get expensive in many-tasks or many-cpus
situations.
This is why the relation is recorded in both places, and is only
deemed to hold if the two records match up. This is what
fpsimd_thread_switch() is checking for the task being scheduled in.
The invalidation (breaking) operations are now factored as
fpsimd_flush_task_state(): falsify (C,current) for every cpu C.
This is done by zapping current->thread.fpsimd_cpu with NR_CPUS
(chosen because it cannot match smp_processor_id()).
fpsumd_flush_cpu_state(): falsify (smp_processor_id(),T) for every
task T. This is done by zapping this_cpu(fpsimd_last_state.st)
with NULL (chosen because it cannot match &T->thread.uw.fpsimd_state
for any task).
By [2] above, it is necessary to ensure that TIF_FOREIGN_FPSTATE is
set after calling either of the above functions. Of the two,
fpsimd_flush_cpu_state() now does this implicitly but
fpsimd_flush_task_state() does not: but the caller must do it
instead. I have a vague memory of some refactoring obstacle that
dissuaded me from pulling the set_thread_flag in, but I can't
remember it now. I may review this later.
* Because the (C,T) relation may need to be manipulated by
kernel_neon_{begin,end}() in softirq context, examining or
manipulating for current or the running CPU must be done under
local_bh_disable(). The same goes for TIF_FOREIGN_FPSTATE which is
supposed to represent the same condition but may spontaneously become
stale if softirqs are not masked. (The rule is not quite as strict
as this, but in order to make the code easier to reason about, I skip
the local_bh_disable() only where absolutely necessary --
restore_sve_fpsimd_context() is the only example today.)
Now, imagine that T is a kernel thread, and consider what needs to
be done differently. The observation of this patch is that nothing
needs to be done differently at all.
There is a single anomaly relating to [1] above, in the form of a task
that can run without ever being scheduled in: the init task. Beyond
that, kernel_neon_begin() before the first reschedule would spuriously
save the FPSIMD regs into the init_task's thread struct, even though it
is pointless to do so. This patch fixes those anomalies by updating
INIT_THREAD and INIT_THREAD_INFO to set up the init task so that it
looks the same as some other kernel thread that has been scheduled in.
There is a strong design motivation to avoid unnecessary loads and
saves of the state, so if removing the special-casing of kernel threads
were to add cost it would imply that the code were _already_ suboptimal
for user tasks. This patch does not attempt to address that at all,
but by assuming that the code is already well-optimised, "unnecessary"
save/restore work will not be added. If this were not the case, it
could in any case be fixed independently.
The observation of this _series_ is that we don't need to do very
much in order to be able to generalise the logic to accept KVM vcpus
in place of T.
^ permalink raw reply
* [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-24 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFravSx3h6UnsjNGM2ViZtkxvPMULyVQnoVT9jsEP5az+A@mail.gmail.com>
On 24/05/18 13:17, Ulf Hansson wrote:
> On 24 May 2018 at 11:36, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 24/05/18 08:04, Ulf Hansson wrote:
>>
>> ...
>>
>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>> indicate
>>>> whether the new device should be linked? I think that I prefer the API
>>>> handles it, but I can see there could be instances where drivers may wish
>>>> to
>>>> handle it themselves.
>>>
>>>
>>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>>> case, would benefit from doing the linking themselves, as it needs
>>> different PM domains to be powered on depending on the current use
>>> case - as to avoid wasting power.
>>>
>>> However, I can understand that you prefer some simplicity over
>>> optimizations, as you told us. Then, does it mean that you are
>>> insisting on extending the APIs with a boolean for linking, or are you
>>> fine with the driver to call device_link_add()?
>>
>>
>> I am fine with the driver calling device_link_add(), but I just wonder if we
>> will find a several drivers doing this and then we will end up doing this
>> later anyway.
>
> Okay.
>
>>
>> The current API is called ...
>>
>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> * @dev: Device to attach.
>> * @index: The index of the PM domain.
>>
>> This naming and description is a bit misleading, because really it is not
>> attaching the device that is passed, but creating a new device to attach a
>> PM domain to. So we should consider renaming and changing the description
>> and indicate that users need to link the device.
>
> I picked the name to be consistent with the existing
> genpd_dev_pm_attach(). Do you have a better suggestion?
Well, it appears to get more of a 'get' function and so I don't see why
we could not have 'genpd_dev_get_by_id()' and then we could have a
genpd_dev_put() as well (which would call genpd_dev_pm_detach).
> I agree, some details is missing to the description, let me try to
> improve it. Actually, I was trying to follow existing descriptions
> from genpd_dev_pm_attach(), so perhaps that also needs a little
> update.
>
> However, do note that, neither genpd_dev_pm_attach() or
> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
> rather only by the driver core. So description may not be so
> important.
>
> In regards to good descriptions, for sure the API added in patch9,
> dev_pm_domain_attach_by_id(), needs a good one, as this is what
> drivers should be using.
OK. Same appears to apply here to the description as I mentioned above.
Still seems to be more of a 'get' than an attach. So I wonder if it
should be dev_pm_domain_get_by_id() instead?
>> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
>> detached?
>
> Via the existing genpd_dev_pm_detach(), according to what I have
> described in the change log. I clarify the description in regards to
> this as well.
OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that
you said 'although we need to extend it to cover cleanup of the earlier
registered device, via calling device_unregister().' So if we do this
then that would be fine.
Cheers!
Jon
--
nvpublic
^ permalink raw reply
* [PATCH v9 07/11] arm64: kexec_file: add crash dump support
From: Rob Herring @ 2018-05-24 14:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521101425.GC9887@linaro.org>
On Mon, May 21, 2018 at 5:14 AM, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
> Hi Rob,
>
> On Fri, May 18, 2018 at 10:35:52AM -0500, Rob Herring wrote:
>> On Tue, May 15, 2018 at 06:12:59PM +0100, James Morse wrote:
>> > Hi guys,
>> >
>> > (CC: +RobH, devicetree list)
>>
>> Thanks.
>>
>> > On 25/04/18 07:26, AKASHI Takahiro wrote:
>> > > Enabling crash dump (kdump) includes
>> > > * prepare contents of ELF header of a core dump file, /proc/vmcore,
>> > > using crash_prepare_elf64_headers(), and
>> > > * add two device tree properties, "linux,usable-memory-range" and
>> > > "linux,elfcorehdr", which represent repsectively a memory range
>> > > to be used by crash dump kernel and the header's location
>>
>> BTW, I intend to move existing parsing these out of the arch code.
>> Please don't add more DT handling to arch/ unless it is *really* arch
>> specific. I'd assume that the next arch to add kexec support will use
>> these bindings instead of the powerpc way.
>
> So do you expect all the fdt-related stuff in my current implementation
> for arm64 to be put into libfdt, or at least drivers/of, from the beginning?
Yes.
> I'm not sure how arch-specific the properties here are. For instance,
> it is only arm64 that uses "linux,usable-memory-range" right now but
> if some other arch follows, it is no more arch-specific.
> # I remember that you didn't like this property :)
The question I guess is what will the next arch use. I don't think any
other DT based arch supports crashdump or kexec yet.
>> > > +{
>> > > + void *buf, *prop;
>> > > + size_t buf_size;
>> > > + int result;
>> > > +
>> > > + buf_size = (__dt_root_addr_cells + __dt_root_size_cells) * sizeof(u32);
>> > > + prop = buf = vmalloc(buf_size);
>>
>> This can go on the stack instead (and would be required to to work in
>> libfdt).
>
> Well, I can't agree with you here since we are now in effort, as far as
> I correctly understand, of purging all the variable-sized arrays on a local
> stack out of the kernel code.
You don't need a variable sized array. The array size just needs to
the the maximum size (16 bytes).
Rob
^ permalink raw reply
* [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
Add bindings for CryptoCell instance in the SoC.
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index d842940..3ac75db 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -528,6 +528,15 @@
status = "disabled";
};
+ arm_cc630p: crypto at e6601000 {
+ compatible = "arm,cryptocell-630p-ree";
+ interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x0 0xe6601000 0 0x1000>;
+ clocks = <&cpg CPG_MOD 229>;
+ resets = <&cpg 229>;
+ power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+ };
+
i2c3: i2c at e66d0000 {
#address-cells = <1>;
#size-cells = <0>;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 4/5] clk: renesas: r8a7795: add ccree clock bindings
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
This patch adds the clock used by the CryptoCell 630p instance in the SoC.
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
This patch depends upon the "clk: renesas: r8a7795: Add CR clock" patch
from Geert Uytterhoeven.
drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index e5b1865..a85dd50 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -133,6 +133,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
DEF_MOD("sys-dmac2", 217, R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac1", 218, R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac0", 219, R8A7795_CLK_S0D3),
+ DEF_MOD("sceg-pub", 229, R8A7795_CLK_CR),
DEF_MOD("cmt3", 300, R8A7795_CLK_R),
DEF_MOD("cmt2", 301, R8A7795_CLK_R),
DEF_MOD("cmt1", 302, R8A7795_CLK_R),
--
2.7.4
^ permalink raw reply related
* [PATCH v2 3/5] crypto: ccree: silence debug prints
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
The cache parameter register configuration was being too verbose.
Use dev_dbg() to only provide the information if needed.
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index b266657..1e40e76 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -168,14 +168,14 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe)
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
if (is_probe)
- dev_info(dev, "Cache params previous: 0x%08X\n", val);
+ dev_dbg(dev, "Cache params previous: 0x%08X\n", val);
cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params);
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
if (is_probe)
- dev_info(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
- val, cache_params);
+ dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
+ val, cache_params);
return 0;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/5] crypto: ccree: better clock handling
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
Use managed clock handling, differentiate between no clock (possibly OK)
and clock init failure (never OK) and correctly handle clock detection
being deferred.
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_driver.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f93ce7..b266657 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
u64 dma_mask;
const struct cc_hw_data *hw_rev;
const struct of_device_id *dev_id;
+ struct clk *clk;
int rc = 0;
new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
@@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device *plat_dev)
platform_set_drvdata(plat_dev, new_drvdata);
new_drvdata->plat_dev = plat_dev;
- new_drvdata->clk = of_clk_get(np, 0);
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ switch (PTR_ERR(clk)) {
+ /* Clock is optional so this might be fine */
+ case -ENOENT:
+ break;
+
+ /* Clock not available, let's try again soon */
+ case -EPROBE_DEFER:
+ return -EPROBE_DEFER;
+
+ default:
+ dev_err(dev, "Error getting clock: %ld\n",
+ PTR_ERR(clk));
+ return PTR_ERR(clk);
+ }
+ new_drvdata->clk = clk;
+
new_drvdata->coherent = of_dma_is_coherent(np);
/* Get device resources */
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/5] crypto: ccree: correct host regs offset
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
The product signature and HW revision register have different offset on the
older HW revisions.
This fixes the problem of the driver failing sanity check on silicon
despite working on the FPGA emulation systems.
Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
Cc: stable at vger.kernel.org
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_debugfs.c | 7 +++++--
drivers/crypto/ccree/cc_driver.c | 8 ++++++--
drivers/crypto/ccree/cc_driver.h | 2 ++
drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
4 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
index 08f8db4..5ca184e 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
static struct dentry *cc_debugfs_dir;
static struct debugfs_reg32 debug_regs[] = {
- CC_DEBUG_REG(HOST_SIGNATURE),
+ { .name = "SIGNATURE" }, /* Must be 0th */
+ { .name = "VERSION" }, /* Must be 1st */
CC_DEBUG_REG(HOST_IRR),
CC_DEBUG_REG(HOST_POWER_DOWN_EN),
CC_DEBUG_REG(AXIM_MON_ERR),
@@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
CC_DEBUG_REG(HOST_IMR),
CC_DEBUG_REG(AXIM_CFG),
CC_DEBUG_REG(AXIM_CACHE_PARAMS),
- CC_DEBUG_REG(HOST_VERSION),
CC_DEBUG_REG(GPR_HOST),
CC_DEBUG_REG(AXIM_MON_COMP),
};
@@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
struct debugfs_regset32 *regset;
struct dentry *file;
+ debug_regs[0].offset = drvdata->sig_offset;
+ debug_regs[1].offset = drvdata->ver_offset;
+
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..6f93ce7 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device *plat_dev)
if (hw_rev->rev >= CC_HW_REV_712) {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
+ new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
+ new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
} else {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
+ new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
+ new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
}
platform_set_drvdata(plat_dev, new_drvdata);
@@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
}
/* Verify correct mapping */
- signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
+ signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
if (signature_val != hw_rev->sig) {
dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
signature_val, hw_rev->sig);
@@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
/* Display HW versions */
dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
- hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
+ hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
DRV_MODULE_VERSION);
rc = init_cc_regs(new_drvdata, true);
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index 2048fde..95f82b2 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -129,6 +129,8 @@ struct cc_drvdata {
enum cc_hw_rev hw_rev;
u32 hash_len_sz;
u32 axim_mon_offset;
+ u32 sig_offset;
+ u32 ver_offset;
};
struct cc_crypto_alg {
diff --git a/drivers/crypto/ccree/cc_host_regs.h b/drivers/crypto/ccree/cc_host_regs.h
index f510018..616b2e1 100644
--- a/drivers/crypto/ccree/cc_host_regs.h
+++ b/drivers/crypto/ccree/cc_host_regs.h
@@ -45,7 +45,8 @@
#define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE 0x1UL
#define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT 0x17UL
#define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE 0x1UL
-#define CC_HOST_SIGNATURE_REG_OFFSET 0xA24UL
+#define CC_HOST_SIGNATURE_712_REG_OFFSET 0xA24UL
+#define CC_HOST_SIGNATURE_630_REG_OFFSET 0xAC8UL
#define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT 0x0UL
#define CC_HOST_SIGNATURE_VALUE_BIT_SIZE 0x20UL
#define CC_HOST_BOOT_REG_OFFSET 0xA28UL
@@ -105,7 +106,8 @@
#define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE 0x1UL
#define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SHIFT 0x1EUL
#define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SIZE 0x1UL
-#define CC_HOST_VERSION_REG_OFFSET 0xA40UL
+#define CC_HOST_VERSION_712_REG_OFFSET 0xA40UL
+#define CC_HOST_VERSION_630_REG_OFFSET 0xAD8UL
#define CC_HOST_VERSION_VALUE_BIT_SHIFT 0x0UL
#define CC_HOST_VERSION_VALUE_BIT_SIZE 0x20UL
#define CC_HOST_KFDE0_VALID_REG_OFFSET 0xA60UL
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: linux-arm-kernel
The patch set enables the use of CryptoCell found in some Renesas R-Car
Salvator-X boards and fixes some driver issues uncovered that prevented
to work properly.
Changes from v1:
- Properly fix the bug that caused us to read a bad signature register
rather than dropping the check
- Proper DT fields as indicated by Geert Uytterhoeven.
- Better clock enabling as suggested by Geert Uytterhoeven.
Note! the last two patches in the set depend on the
"clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.
Gilad Ben-Yossef (5):
crypto: ccree: correct host regs offset
crypto: ccree: better clock handling
crypto: ccree: silence debug prints
clk: renesas: r8a7795: add ccree clock bindings
arm64: dts: renesas: r8a7795: add ccree binding
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
drivers/crypto/ccree/cc_debugfs.c | 7 +++++--
drivers/crypto/ccree/cc_driver.c | 34 ++++++++++++++++++++++++++------
drivers/crypto/ccree/cc_driver.h | 2 ++
drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
6 files changed, 49 insertions(+), 10 deletions(-)
--
2.7.4
^ permalink raw reply
* linux-next: build failure after merge of the pm tree
From: Mark Brown @ 2018-05-24 14:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi Rafael,
After merging the pm tree, today's linux-next build (arm64 defconfig)
failed like this:
drivers/acpi/ec.o: In function `acpi_ec_dispatch_gpe':
/home/broonie/next/next/drivers/acpi/ec.c:1040: undefined reference to `acpi_dispatch_gpe'
/home/broonie/next/next/drivers/acpi/ec.c:1040:(.text+0x228c): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `acpi_dispatch_gpe'
Probably caused by commit
8f81c457105b5d4 ("ACPI: EC: Dispatch the EC GPE directly on s2idle wake")
(though the symbol appears to be there and like it should be built and
the relocation warning looks funky...). I've used the PM tree from the
last -next.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180524/fcc5a5bf/attachment.sig>
^ permalink raw reply
* [PATCH v2 08/13] ASoC: pxa: remove the dmaengine compat need
From: Mark Brown @ 2018-05-24 13:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524070703.11901-9-robert.jarzmik@free.fr>
On Thu, May 24, 2018 at 09:06:58AM +0200, Robert Jarzmik wrote:
> As the pxa architecture switched towards the dmaengine slave map, the
> old compatibility mechanism to acquire the dma requestor line number and
> priority are not needed anymore.
Acked-by: Mark Brown <broonie@kernel.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180524/f0c21ccd/attachment-0001.sig>
^ permalink raw reply
* [PATCH v10 4/4] Revert "arm64: Enforce BBM for huge IO/VMAP mappings"
From: Chintan Pandya @ 2018-05-24 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527170250-5291-1-git-send-email-cpandya@codeaurora.org>
This commit 15122ee2c515a ("arm64: Enforce BBM for huge
IO/VMAP mappings") is a temporary work-around until the
issues with CONFIG_HAVE_ARCH_HUGE_VMAP gets fixed.
Revert this change as we have fixes for the issue.
Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
arch/arm64/mm/mmu.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 17d9282..defb3bb 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -935,10 +935,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
pgprot_t sect_prot = __pgprot(PUD_TYPE_SECT |
pgprot_val(mk_sect_prot(prot)));
- /* ioremap_page_range doesn't honour BBM */
- if (pud_present(READ_ONCE(*pudp)))
- return 0;
-
BUG_ON(phys & ~PUD_MASK);
set_pud(pudp, pfn_pud(__phys_to_pfn(phys), sect_prot));
return 1;
@@ -949,10 +945,6 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
pgprot_t sect_prot = __pgprot(PMD_TYPE_SECT |
pgprot_val(mk_sect_prot(prot)));
- /* ioremap_page_range doesn't honour BBM */
- if (pmd_present(READ_ONCE(*pmdp)))
- return 0;
-
BUG_ON(phys & ~PMD_MASK);
set_pmd(pmdp, pfn_pmd(__phys_to_pfn(phys), sect_prot));
return 1;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply related
* [PATCH v10 3/4] arm64: Implement page table free interfaces
From: Chintan Pandya @ 2018-05-24 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527170250-5291-1-git-send-email-cpandya@codeaurora.org>
Implement pud_free_pmd_page() and pmd_free_pte_page().
Implementation requires,
1) Clearing off the current pud/pmd entry
2) Invalidate TLB which could have previously
valid but not stale entry
3) Freeing of the un-used next level page tables
Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
arch/arm64/mm/mmu.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index da98828..17d9282 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -45,6 +45,7 @@
#include <asm/memblock.h>
#include <asm/mmu_context.h>
#include <asm/ptdump.h>
+#include <asm/tlbflush.h>
#define NO_BLOCK_MAPPINGS BIT(0)
#define NO_CONT_MAPPINGS BIT(1)
@@ -973,12 +974,37 @@ int pmd_clear_huge(pmd_t *pmdp)
return 1;
}
-int pud_free_pmd_page(pud_t *pud, unsigned long addr)
+int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
{
- return pud_none(*pud);
+ pmd_t *table;
+ pmd_t pmd;
+
+ pmd = READ_ONCE(*pmdp);
+ if (pmd_present(pmd)) {
+ table = __va(pmd_val(pmd));
+ pmd_clear(pmdp);
+ __flush_tlb_kernel_pgtable(addr);
+ pte_free_kernel(NULL, (pte_t *) table);
+ }
+ return 1;
}
-int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
+int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
{
- return pmd_none(*pmd);
+ pmd_t *table;
+ pud_t pud;
+ int i = 0;
+
+ pud = READ_ONCE(*pudp);
+ if (pud_present(pud)) {
+ table = __va(pud_val(pud));
+ do {
+ pmd_free_pte_page(&table[i], addr + (i * PMD_SIZE));
+ } while (i++, i < PTRS_PER_PMD);
+
+ pud_clear(pudp);
+ __flush_tlb_kernel_pgtable(addr);
+ pmd_free(NULL, table);
+ }
+ return 1;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply related
* [PATCH v10 2/4] arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable
From: Chintan Pandya @ 2018-05-24 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527170250-5291-1-git-send-email-cpandya@codeaurora.org>
Add an interface to invalidate intermediate page tables
from TLB for kernel.
Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
arch/arm64/include/asm/tlbflush.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
index dfc61d7..a4a1901 100644
--- a/arch/arm64/include/asm/tlbflush.h
+++ b/arch/arm64/include/asm/tlbflush.h
@@ -218,6 +218,13 @@ static inline void __flush_tlb_pgtable(struct mm_struct *mm,
dsb(ish);
}
+static inline void __flush_tlb_kernel_pgtable(unsigned long kaddr)
+{
+ unsigned long addr = __TLBI_VADDR(kaddr, 0);
+
+ __tlbi(vaae1is, addr);
+ dsb(ish);
+}
#endif
#endif
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply related
* [PATCH v10 1/4] ioremap: Update pgtable free interfaces with addr
From: Chintan Pandya @ 2018-05-24 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527170250-5291-1-git-send-email-cpandya@codeaurora.org>
From: Chintan Pandya <cpandya@codeaurora.org>
The following kernel panic was observed on ARM64 platform due to a stale
TLB entry.
1. ioremap with 4K size, a valid pte page table is set.
2. iounmap it, its pte entry is set to 0.
3. ioremap the same address with 2M size, update its pmd entry with
a new value.
4. CPU may hit an exception because the old pmd entry is still in TLB,
which leads to a kernel panic.
Commit b6bdb7517c3d ("mm/vmalloc: add interfaces to free unmapped page
table") has addressed this panic by falling to pte mappings in the above
case on ARM64.
To support pmd mappings in all cases, TLB purge needs to be performed
in this case on ARM64.
Add a new arg, 'addr', to pud_free_pmd_page() and pmd_free_pte_page()
so that TLB purge can be added later in seprate patches.
[toshi at hpe.com: merge changes, rewrite patch description]
Fixes: 28ee90fe6048 ("x86/mm: implement free pmd/pte page interfaces")
Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: <stable@vger.kernel.org>
---
arch/arm64/mm/mmu.c | 4 ++--
arch/x86/mm/pgtable.c | 8 +++++---
include/asm-generic/pgtable.h | 8 ++++----
lib/ioremap.c | 4 ++--
4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..da98828 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -973,12 +973,12 @@ int pmd_clear_huge(pmd_t *pmdp)
return 1;
}
-int pud_free_pmd_page(pud_t *pud)
+int pud_free_pmd_page(pud_t *pud, unsigned long addr)
{
return pud_none(*pud);
}
-int pmd_free_pte_page(pmd_t *pmd)
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
{
return pmd_none(*pmd);
}
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index ffc8c13..37e3cba 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -718,11 +718,12 @@ int pmd_clear_huge(pmd_t *pmd)
/**
* pud_free_pmd_page - Clear pud entry and free pmd page.
* @pud: Pointer to a PUD.
+ * @addr: Virtual address associated with pud.
*
* Context: The pud range has been unmaped and TLB purged.
* Return: 1 if clearing the entry succeeded. 0 otherwise.
*/
-int pud_free_pmd_page(pud_t *pud)
+int pud_free_pmd_page(pud_t *pud, unsigned long addr)
{
pmd_t *pmd;
int i;
@@ -733,7 +734,7 @@ int pud_free_pmd_page(pud_t *pud)
pmd = (pmd_t *)pud_page_vaddr(*pud);
for (i = 0; i < PTRS_PER_PMD; i++)
- if (!pmd_free_pte_page(&pmd[i]))
+ if (!pmd_free_pte_page(&pmd[i], addr + (i * PMD_SIZE)))
return 0;
pud_clear(pud);
@@ -745,11 +746,12 @@ int pud_free_pmd_page(pud_t *pud)
/**
* pmd_free_pte_page - Clear pmd entry and free pte page.
* @pmd: Pointer to a PMD.
+ * @addr: Virtual address associated with pmd.
*
* Context: The pmd range has been unmaped and TLB purged.
* Return: 1 if clearing the entry succeeded. 0 otherwise.
*/
-int pmd_free_pte_page(pmd_t *pmd)
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
{
pte_t *pte;
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index f59639a..b081794 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1019,8 +1019,8 @@ static inline int p4d_clear_huge(p4d_t *p4d)
int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
int pud_clear_huge(pud_t *pud);
int pmd_clear_huge(pmd_t *pmd);
-int pud_free_pmd_page(pud_t *pud);
-int pmd_free_pte_page(pmd_t *pmd);
+int pud_free_pmd_page(pud_t *pud, unsigned long addr);
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
static inline int p4d_set_huge(p4d_t *p4d, phys_addr_t addr, pgprot_t prot)
{
@@ -1046,11 +1046,11 @@ static inline int pmd_clear_huge(pmd_t *pmd)
{
return 0;
}
-static inline int pud_free_pmd_page(pud_t *pud)
+static inline int pud_free_pmd_page(pud_t *pud, unsigned long addr)
{
return 0;
}
-static inline int pmd_free_pte_page(pmd_t *pmd)
+static inline int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
{
return 0;
}
diff --git a/lib/ioremap.c b/lib/ioremap.c
index 54e5bba..517f585 100644
--- a/lib/ioremap.c
+++ b/lib/ioremap.c
@@ -92,7 +92,7 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
if (ioremap_pmd_enabled() &&
((next - addr) == PMD_SIZE) &&
IS_ALIGNED(phys_addr + addr, PMD_SIZE) &&
- pmd_free_pte_page(pmd)) {
+ pmd_free_pte_page(pmd, addr)) {
if (pmd_set_huge(pmd, phys_addr + addr, prot))
continue;
}
@@ -119,7 +119,7 @@ static inline int ioremap_pud_range(p4d_t *p4d, unsigned long addr,
if (ioremap_pud_enabled() &&
((next - addr) == PUD_SIZE) &&
IS_ALIGNED(phys_addr + addr, PUD_SIZE) &&
- pud_free_pmd_page(pud)) {
+ pud_free_pmd_page(pud, addr)) {
if (pud_set_huge(pud, phys_addr + addr, prot))
continue;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply related
* [PATCH v10 0/4] Fix issues with huge mapping in ioremap for ARM64
From: Chintan Pandya @ 2018-05-24 13:57 UTC (permalink / raw)
To: linux-arm-kernel
This series of patches re-bring huge vmap back for arm64.
Patch 1/4 has been taken by Toshi in his series of patches
by name "[PATCH v3 0/3] fix free pmd/pte page handlings on x86"
to avoid merge conflict with this series.
Patch 4/4 may conflict with Laura's patch "[PATCHv2] arm64: Make
sure permission updates happen for pmd/pud". It looks to me that,
problem addressed by this patch will also get resolved by below
series of patches. There could be multiple ways to resolve this.
I would let maintainer suggest the best approach, and I will do
the needful.
This patches are tested on 4.16 kernel with Cortex-A75 based SoC.
The test used for verifying these patches is a stress test on
ioremap/unmap which tries to re-use same io-address but changes
size of mapping randomly i.e. 4K to 2M to 16K etc. The same test
used to reproduce 3rd level translation fault without these fixes
(and also of course with Revert "arm64: Enforce BBM for huge IO/VMAP
mappings" being part of the tree).
These patches can also go into '-stable' branch (if accepted)
for 4.6 onwards.
>From V9->V10:
- Updated commit log for patch 1/4 by Toshi
- Addressed review comments by Will on patch 3/4
>From V8->V9:
- Used __TLBI_VADDR macros in new TLB flush API
>From V7->V8:
- Properly fixed compilation issue in x86 file
>From V6->V7:
- Fixed compilation issue in x86 case
- V6 patches were not properly enumarated
>From V5->V6:
- Use __flush_tlb_kernel_pgtable() for both PUD and PMD. Remove
"bool tlb_inv" based variance as it is not need now
- Re-naming for consistency
>From V4->V5:
- Add new API __flush_tlb_kernel_pgtable(unsigned long addr)
for kernel addresses
>From V3->V4:
- Add header for 'addr' in x86 implementation
- Re-order pmd/pud clear and table free
- Avoid redundant TLB invalidatation in one perticular case
>From V2->V3:
- Use the exisiting page table free interface to do arm64
specific things
>From V1->V2:
- Rebased my patches on top of "[PATCH v2 1/2] mm/vmalloc:
Add interfaces to free unmapped page table"
- Honored BBM for ARM64
Chintan Pandya (4):
ioremap: Update pgtable free interfaces with addr
arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable
arm64: Implement page table free interfaces
Revert "arm64: Enforce BBM for huge IO/VMAP mappings"
arch/arm64/include/asm/tlbflush.h | 7 +++++++
arch/arm64/mm/mmu.c | 42 ++++++++++++++++++++++++++++-----------
arch/x86/mm/pgtable.c | 8 +++++---
include/asm-generic/pgtable.h | 8 ++++----
lib/ioremap.c | 4 ++--
5 files changed, 48 insertions(+), 21 deletions(-)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply
* [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Robin Murphy @ 2018-05-24 13:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4b0ec4ab-2d9e-1e4b-e294-c175805f4013@broadcom.com>
On 23/05/18 20:29, Ray Jui wrote:
>
>
> On 5/23/2018 11:59 AM, Rob Herring wrote:
>> On Wed, May 23, 2018 at 09:25:49AM -0700, Ray Jui wrote:
>>>
>>>
>>> On 5/23/2018 3:57 AM, Robin Murphy wrote:
>>>> On 22/05/18 19:47, Ray Jui wrote:
>>>>> Update the SP805 binding document to add optional 'timeout-sec'
>>>>> devicetree property
>>>>>
>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>>> ---
>>>>> ?? Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++
>>>>> ?? 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> index edc4f0e..f898a86 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> @@ -19,6 +19,8 @@ Required properties:
>>>>> ?? Optional properties:
>>>>> ?? - interrupts : Should specify WDT interrupt number.
>>>>> +- timeout-sec : Should specify default WDT timeout in seconds. If
>>>>> unset, the
>>>>> +??????????????? default timeout is 30 seconds
>>>>
>>>> According to the SP805 TRM, the default interval is dependent on the
>>>> rate of WDOGCLK, but would typically be a lot longer than that :/
>>>>
>>>> On a related note, anyone have any idea why we seem to have two subtly
>>>> different SP805 bindings defined?
>>
>> Sigh.
>>
>>> Interesting, I did not even know that until you pointed this out (and
>>> it's
>>> funny that I found that I actually reviewed arm,sp805.txt internally in
>>> Broadcom code review).
>>>
>>> It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the
>>> other
>>> was done by Anup Patel (arm,sp805.txt). Both were merged at the same
>>> time
>>> around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at
>>> around
>>> the same time.
>>>
>>> It sounds like we should definitely remove one of them. Given that
>>> sp805-wdt.txt appears to have more detailed descriptions on the use
>>> of the
>>> clocks, should we remove arm,sp805.txt?
>>
>> Take whichever text you like, but I prefer filenames using the
>> compatible string and the correct string is 'arm,sp805' because '-wdt'
>> is redundant. You can probably safely just update all the dts files with
>> 'arm,sp805' and just remove 'arm,sp805-wdt' because it is not actually
>> used (as the ID registers are).
>
> Okay. I'll consolidate everything into arm,sp805.txt. Will also fix all
> DTS files to use "arm,sp805". The fix for actual DTS files will be in a
> different patch series.
Looking at the current in-tree DTs, for extra fun try to figure out
which binding each instance was following for the clocks. The most
common answer seems to be "neither"... :(
Robin.
^ permalink raw reply
* [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding
From: Geert Uytterhoeven @ 2018-05-24 13:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOtvUMdCSoxYjpyb+fD36EE5+4sV4CjrxwK94RQ7bNKE5rajmQ@mail.gmail.com>
Hi Gilad,
On Thu, May 24, 2018 at 3:20 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
>>>> does not distinguish between the absence of the clock property, and an
>>>> actual error in getting the clock, and never considers any error a failure
>>>> (incl. -PROBE_DEFER).
>>>>
>>>> As of_clk_get() returns -ENOENT for both a missing clock property and a
>>>> missing clock, you should use (devm_)clk_get() instead, and distinguish
>>>> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>>>
>>> I was trying to do as you suggested but I didn't quite get what is the
>>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>>
>> It's the (optional) name of the clock, helpful in case there is more than one.
>> In your case, NULL is fine.
>
> I have assumed as much and tried it, it did not work and so I assumed
> I am missing something and asked you.
> It turns out I was missing the fact I was using the wrong device tree
> file... :-(
>
> So thanks, it works now :-)
Glad to hear that!
> Having said that, while using devm)clk_get() is a better approach, it
> does not seems to distinguish
> between no "clocks" and a failure to clock information - it returns
> ENOENT in both cases as well.
Oh right, I guess I'm too used to not even getting that far due to the PM
Domain code failing to obtain the clock...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip
From: Rob Herring @ 2018-05-24 13:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3307687.qJF5Pr3uHG@phil>
On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko St?bner <heiko@sntech.de> wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw at t-chip.com.cn wrote:
>> >> >>> From: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> Changes in v2:
>> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >> >>>
>> >> >>> Changes in v1:
>> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >> >>> - Add doc rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41
>> >> >>>
>> >> >>> ++++++++++++++++++++++
>> >> >>>
>> >> >>> drivers/gpio/gpio-syscon.c | 30
>> >> >>>
>> >> >>> ++++++++++++++++
>> >> >>>
>> >> >>> 2 files changed, 71 insertions(+)
>> >> >>> create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> + the second cell is used to specify the gpio polarity:
>> >> >>> + 0 = Active high,
>> >> >>> + 1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > + grf: syscon at ff100000 {
>> >> > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.
But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?
> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8
Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a driver, but DT is not a list of drivers to bind.
This is what I don't want to see:
syscon {
compatible = "foo,soc-sysctrl", "syscon", "simple-mfd";
reg = <...>;
clock-controller {
compatible = "foo,soc-sysctrl-clocks";
#clock-cells = <1>;
};
reset-controller {
compatible = "foo,soc-sysctrl-resets";
#reset-cells = <1>;
};
gpio {
compatible = "foo,soc-sysctrl-gpios";
#gpio-cells = <2>;
gpio-controller;
};
};
But rather:
syscon {
compatible = "foo,soc-sysctrl";
reg = <...>;
#clock-cells = <1>;
#reset-cells = <1>;
#gpio-cells = <2>;
gpio-controller;
};
> which also has a real phy-driver behind it and binding against that
> subnode of the GRF simple-mfd.
>
> These are real IP blocks somewhere on the socs, with regular supplies
> like resets, clocks etc in most cases. Only their controlling registers
> got dumped into the GRF for some reason.
I can tell that from your examples, but I can't tell that with this
binding. For this binding, it looks like you are adding a sub-node for
1 register bit. That wouldn't scale if you have 11 page register list.
> And in retrospect it really looks like we're doing something right,
> because it seems these bindings seem quite stable over time.
>
>
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>>
>> Is there really just one GPIO? If it has a defined function, then is
>> it really GP? Can you control direction? I know Linus W doesn't like
>> that kind of abuse of GPIO.
>
> looks like I convinced Linus that we're not abusing anything with this :-) .
Okay, but still my question remains: is it really only 1 GPIO?
Dropping "-mute" would be more future proof if not.
Rob
^ permalink raw reply
* [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
From: Don Brace @ 2018-05-24 13:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524130727.GA85822@bhelgaas-glaptop.roam.corp.google.com>
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas at kernel.org]
> Sent: Thursday, May 24, 2018 8:07 AM
> To: Sinan Kaya <okaya@codeaurora.org>
> Cc: linux-pci at vger.kernel.org; timur at codeaurora.org; ryan at finnie.org; linux-
> arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> stable at vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>; Rafael J.
> Wysocki <rafael.j.wysocki@intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Thomas Gleixner <tglx@linutronix.de>; Kate
> Stewart <kstewart@linuxfoundation.org>; Frederick Lawler
> <fred@fredlawl.com>; Dongdong Liu <liudongdong3@huawei.com>; Mika
> Westerberg <mika.westerberg@linux.intel.com>; open list <linux-
> kernel at vger.kernel.org>; Don Brace <don.brace@microsemi.com>;
> esc.storagedev <esc.storagedev@microsemi.com>; linux-scsi at vger.kernel.org
> Subject: Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
>
> EXTERNAL EMAIL
>
>
> On Thu, May 24, 2018 at 07:43:05AM -0400, Sinan Kaya wrote:
> > On 5/23/2018 6:57 PM, Sinan Kaya wrote:
> > >> The crash seems to indicate that the hpsa device attempted a DMA after
> > >> we cleared the Root Port's PCI_COMMAND_MASTER, which means
> > >> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
> > >> shutdown methods don't disable device DMA, so it's in good company).
> > > All drivers are expected to shutdown DMA and interrupts in their shutdown()
> > > routines. They can skip removing threads, data structures etc. but DMA and
> > > interrupt disabling are required. This is the difference between shutdown()
> > > and remove() callbacks.
> >
> > I found this note yesterday to see why we are not disabling the
> > devices in the PCI core itself.
> >
> > pci_device_remove()
> >
> > /*
> > * We would love to complain here if pci_dev->is_enabled is set, that
> > * the driver should have called pci_disable_device(), but the
> > * unfortunate fact is there are too many odd BIOS and bridge setups
> > * that don't like drivers doing that all of the time.
> > * Oh well, we can dream of sane hardware when we sleep, no matter how
> > * horrible the crap we have to deal with is when we are awake...
> > */
> >
> > Ryan, can you discard the previous patch and test this one instead?
> > remove() path in hpsa driver seems to call pci_disable_device() via
> >
> > hpsa_remove_one()
> > hpsa_free_pci_init()
> >
> > but nothing on the shutdown path.
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 4ed3d26..3823f04 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
> > h->access.set_intr_mask(h, HPSA_INTR_OFF);
> > hpsa_free_irqs(h); /* init_one 4 */
> > hpsa_disable_interrupt_mode(h); /* pci_init 2 */
> > + pci_disable_device(h->pdev);
> > }
>
> I suspect this will make things "work" (if the device can't attempt
> DMA, no Unsupported Request error will occur).
>
> But I'm concerned that the reason for the DMA might that hpsa is
> transferring buffers from system memory to the hpsa device, and if we
> arbitrarily terminate those transfers with pci_disable_device(), we
> may leave the hpsa device in an inconsistent state, e.g., with a dirty
> filesystem.
>
> But we really need guidance from an hpsa expert. I don't know the
> filesystem/SCSI/hpsa details.
>
> Bjorn
It's most likely OCSD traffic that will stop when bus mastering is turned off.
So, I'll run some tests on my end before ACKing your patch.
Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation
^ permalink raw reply
* [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
From: okaya at codeaurora.org @ 2018-05-24 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524130727.GA85822@bhelgaas-glaptop.roam.corp.google.com>
On 2018-05-24 09:07, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 07:43:05AM -0400, Sinan Kaya wrote:
>> On 5/23/2018 6:57 PM, Sinan Kaya wrote:
>> >> The crash seems to indicate that the hpsa device attempted a DMA after
>> >> we cleared the Root Port's PCI_COMMAND_MASTER, which means
>> >> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
>> >> shutdown methods don't disable device DMA, so it's in good company).
>> > All drivers are expected to shutdown DMA and interrupts in their shutdown()
>> > routines. They can skip removing threads, data structures etc. but DMA and
>> > interrupt disabling are required. This is the difference between shutdown()
>> > and remove() callbacks.
>>
>> I found this note yesterday to see why we are not disabling the
>> devices in the PCI core itself.
>>
>> pci_device_remove()
>>
>> /*
>> * We would love to complain here if pci_dev->is_enabled is set, that
>> * the driver should have called pci_disable_device(), but the
>> * unfortunate fact is there are too many odd BIOS and bridge setups
>> * that don't like drivers doing that all of the time.
>> * Oh well, we can dream of sane hardware when we sleep, no matter
>> how
>> * horrible the crap we have to deal with is when we are awake...
>> */
>>
>> Ryan, can you discard the previous patch and test this one instead?
>> remove() path in hpsa driver seems to call pci_disable_device() via
>>
>> hpsa_remove_one()
>> hpsa_free_pci_init()
>>
>> but nothing on the shutdown path.
>>
>> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
>> index 4ed3d26..3823f04 100644
>> --- a/drivers/scsi/hpsa.c
>> +++ b/drivers/scsi/hpsa.c
>> @@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
>> h->access.set_intr_mask(h, HPSA_INTR_OFF);
>> hpsa_free_irqs(h); /* init_one 4 */
>> hpsa_disable_interrupt_mode(h); /* pci_init 2 */
>> + pci_disable_device(h->pdev);
>> }
>
> I suspect this will make things "work" (if the device can't attempt
> DMA, no Unsupported Request error will occur).
>
> But I'm concerned that the reason for the DMA might that hpsa is
> transferring buffers from system memory to the hpsa device, and if we
> arbitrarily terminate those transfers with pci_disable_device(), we
> may leave the hpsa device in an inconsistent state, e.g., with a dirty
> filesystem.
>
> But we really need guidance from an hpsa expert. I don't know the
> filesystem/SCSI/hpsa details.
Agreed,
We can drop shutdown and use the remove callback. Remove is supposed to
do a safe cleanup.
>
> Bjorn
^ permalink raw reply
* [PATCH 04/14] arm64: Add ARCH_WORKAROUND_2 probing
From: Suzuki K Poulose @ 2018-05-24 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180524113933.GA8689@arm.com>
On 24/05/18 12:39, Will Deacon wrote:
> On Thu, May 24, 2018 at 10:58:43AM +0100, Suzuki K Poulose wrote:
>> On 22/05/18 16:06, Marc Zyngier wrote:
>>> As for Spectre variant-2, we rely on SMCCC 1.1 to provide the
>>> discovery mechanism for detecting the SSBD mitigation.
>>>
>>> A new capability is also allocated for that purpose, and a
>>> config option.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>>
>>> +static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,
>>> + int scope)
>>> +{
>>> + struct arm_smccc_res res;
>>> + bool supported = true;
>>> +
>>> + WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
>>> +
>>> + if (psci_ops.smccc_version == SMCCC_VERSION_1_0)
>>> + return false;
>>> +
>>> + /*
>>> + * The probe function return value is either negative
>>> + * (unsupported or mitigated), positive (unaffected), or zero
>>> + * (requires mitigation). We only need to do anything in the
>>> + * last case.
>>> + */
>>> + switch (psci_ops.conduit) {
>>> + case PSCI_CONDUIT_HVC:
>>> + arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
>>> + ARM_SMCCC_ARCH_WORKAROUND_2, &res);
>>> + if ((int)res.a0 != 0)
>>> + supported = false;
>>> + break;
>>> +
>>> + case PSCI_CONDUIT_SMC:
>>> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
>>> + ARM_SMCCC_ARCH_WORKAROUND_2, &res);
>>> + if ((int)res.a0 != 0)
>>> + supported = false;
>>> + break;
>>> +
>>> + default:
>>> + supported = false;
>>> + }
>>> +
>>> + if (supported) {
>>> + __this_cpu_write(arm64_ssbd_callback_required, 1);
>>> + do_ssbd(true);
>>> + }
>>
>>
>> Marc,
>>
>> As discussed, we have minor issue with the "corner case". If a CPU
>> is hotplugged in which requires the mitigation, after the system has
>> finalised the cap to "not available", the CPU could go ahead and
>> do the "work around" as above, while not effectively doing anything
>> about it at runtime for KVM guests (as thats the only place where
>> we rely on the CAP being set).
>>
>> But, yes this is real corner case. There is no easy way to solve it
>> other than
>>
>> 1) Allow late modifications to CPU hwcaps
>>
>> OR
>>
>> 2) Penalise the fastpath to always check per-cpu setting.
>
> Shouldn't we just avoid bring up CPUs that require the mitigation after
> we've finalised the capability to say that it's not required? Assuming this
> is just another issue with maxcpus=, then I don't much care for it.
Ah! Sorry, yes we do kill the CPU. But it is just that it will set the
ssbd_callback_required flag and issue the do_ssbd(), which is not an issue.
Yes this can only be triggered by maxcpus=.
Suzuki
^ 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