* Re: [PATCH 0/2] Let pci_fixup_final access iommu_fwnode
From: Zhangfei Gao @ 2020-05-26 12:07 UTC (permalink / raw)
To: Joerg Roedel
Cc: jean-philippe, Lorenzo Pieralisi, Herbert Xu, Greg Kroah-Hartman,
Hanjun Guo, Rafael J. Wysocki, linux-kernel, iommu, linux-acpi,
Wangzhou, linux-crypto, Sudeep Holla, kenneth-lee-2012,
linux-arm-kernel, Len Brown
In-Reply-To: <20200525134318.GB5221@8bytes.org>
On 2020/5/25 下午9:43, Joerg Roedel wrote:
> On Tue, May 12, 2020 at 12:08:29PM +0800, Zhangfei Gao wrote:
>> Some platform devices appear as PCI but are
>> actually on the AMBA bus, and they need fixup in
>> drivers/pci/quirks.c handling iommu_fwnode.
>> So calling pci_fixup_final after iommu_fwnode is allocated.
>>
>> For example:
>> Hisilicon platform device need fixup in
>> drivers/pci/quirks.c
>>
>> +static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
>> +{
>> + struct iommu_fwspec *fwspec;
>> +
>> + pdev->eetlp_prefix_path = 1;
>> + fwspec = dev_iommu_fwspec_get(&pdev->dev);
>> + if (fwspec)
>> + fwspec->can_stall = 1;
>> +}
>> +
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva);
> I don't think it is a great idea to hook this into PCI_FIXUP_FINAL. The
> fixup list needs to be processed for every device, which will slow down
> probing.
>
> So either we introduce something like PCI_FIXUP_IOMMU, if this is
> entirely PCI specific. If it needs to be generic we need some fixup
> infrastructure in the IOMMU code itself.
Thanks Joerg for the good suggestion.
I am trying to introduce PCI_FIXUP_IOMMU in
https://lkml.org/lkml/2020/5/26/366
Thanks
_______________________________________________
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/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()
From: Catalin Marinas @ 2020-05-26 12:19 UTC (permalink / raw)
To: Anshuman Khandual
Cc: mark.rutland, Suzuki K Poulose, linux-kernel, Mark Brown,
Will Deacon, linux-arm-kernel
In-Reply-To: <aa6436d4-c724-4933-1341-6ca417ce40ed@arm.com>
On Mon, May 25, 2020 at 05:22:23AM +0530, Anshuman Khandual wrote:
> On 05/21/2020 10:29 PM, Catalin Marinas wrote:
> > On Thu, May 21, 2020 at 05:22:15PM +0100, Will Deacon wrote:
> >> On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
> >>> The existing code has BUG_ON() in three different callers doing exactly the
> >>> same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
> >>> mentioned before an enum variable (as preferred - over a bool) can still
> >>> preserve the existing behavior for emulate_sys_reg().
> >>>
> >>> IMHO these are very good reasons for us to change the code which will make
> >>> it cleaner while also removing three redundant BUG_ON() instances. Hence I
> >>> will request you to please reconsider this proposal.
> >>
> >> Hmm, then how about trying my proposal with the WARN_ON(), but having a
> >> get_arm64_ftr_reg_nowarn() variant for the user emulation case?
[...]
> > read_sanitised_ftr_reg() would need to return something though. Would
> > all 0s be ok? I think it works as long as we don't have negative CPUID
> > fields.
>
> Just trying to understand. If get_arm64_ftr_reg() returns NULL, then
> read_sanitised_ftr_reg() should also return 0 for that non existent
> register (already been warned in get_arm64_ftr_reg).
>
> @@ -961,8 +972,8 @@ u64 read_sanitised_ftr_reg(u32 id)
> {
> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);
>
> - /* We shouldn't get a request for an unsupported register */
> - BUG_ON(!regp);
> + if (!regp)
> + return 0;
> return regp->sys_val;
> }
Yes, as long as we also get the WARN_ON().
--
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 RFCv2 7/9] kvm/arm64: Support async page fault
From: Mark Rutland @ 2020-05-26 12:34 UTC (permalink / raw)
To: Gavin Shan
Cc: aarcange, drjones, suzuki.poulose, catalin.marinas, linux-kernel,
eric.auger, james.morse, shan.gavin, maz, will, kvmarm,
linux-arm-kernel
In-Reply-To: <20200508032919.52147-8-gshan@redhat.com>
On Fri, May 08, 2020 at 01:29:17PM +1000, Gavin Shan wrote:
> There are two stages of fault pages and the stage one page fault is
> handled by guest itself. The guest is trapped to host when the page
> fault is caused by stage 2 page table, for example missing. The guest
> is suspended until the requested page is populated. To populate the
> requested page can be related to IO activities if the page was swapped
> out previously. In this case, the guest has to suspend for a few of
> milliseconds at least, regardless of the overall system load. There
> is no useful work done during the suspended period from guest's view.
This is a bit difficult to read. How about:
| When a vCPU triggers a Stage-2 fault (e.g. when accessing a page that
| is not mapped at Stage-2), the vCPU is suspended until the host has
| handled the fault. It can take the host milliseconds or longer to
| handle the fault as this may require IO, and when the system load is
| low neither the host nor guest perform useful work during such
| periods.
>
> This adds asychornous page fault to improve the situation. A signal
Nit: typo for `asynchronous` here, and there are a few other typos in
the patch itself. It would be nice if you could run a spellcheck over
that.
> (PAGE_NOT_PRESENT) is sent to guest if the requested page needs some time
> to be populated. Guest might reschedule to another running process if
> possible. Otherwise, the vCPU is put into power-saving mode, which is
> actually to cause vCPU reschedule from host's view. A followup signal
> (PAGE_READY) is sent to guest once the requested page is populated.
> The suspended task is waken up or scheduled when guest receives the
> signal. With this mechanism, the vCPU won't be stuck when the requested
> page is being populated by host.
It would probably be best to say 'notification' rather than 'signal'
here, and say 'the guest is notified', etc. As above, it seems that this
is per-vCPU, so it's probably better to say 'vCPU' rather than guest, to
make it clear which context this applies to.
>
> There are more details highlighted as below. Note the implementation is
> similar to what x86 has to some extent:
>
> * A dedicated SMCCC ID is reserved to enable, disable or configure
> the functionality. The only 64-bits parameter is conveyed by two
> registers (w2/w1). Bits[63:56] is the bitmap used to specify the
> operated functionality like enabling/disabling/configuration. The
> bits[55:6] is the physical address of control block or external
> data abort injection disallowed region. Bit[5:0] are used to pass
> control flags.
>
> * Signal (PAGE_NOT_PRESENT) is sent to guest if the requested page
> isn't ready. In the mean while, a work is started to populate the
> page asynchronously in background. The stage 2 page table entry is
> updated accordingly and another signal (PAGE_READY) is fired after
> the request page is populted. The signals is notified by injected
> data abort fault.
>
> * The signals are fired and consumed in sequential fashion. It means
> no more signals will be fired if there is pending one, awaiting the
> guest to consume. It's because the injected data abort faults have
> to be done in sequential fashion.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 43 ++++
> arch/arm64/include/asm/kvm_para.h | 27 ++
> arch/arm64/include/uapi/asm/Kbuild | 2 -
> arch/arm64/include/uapi/asm/kvm_para.h | 22 ++
> arch/arm64/kvm/Kconfig | 1 +
> arch/arm64/kvm/Makefile | 2 +
> include/linux/arm-smccc.h | 6 +
> virt/kvm/arm/arm.c | 36 ++-
> virt/kvm/arm/async_pf.c | 335 +++++++++++++++++++++++++
> virt/kvm/arm/hypercalls.c | 8 +
> virt/kvm/arm/mmu.c | 29 ++-
> 11 files changed, 506 insertions(+), 5 deletions(-)
> create mode 100644 arch/arm64/include/asm/kvm_para.h
> create mode 100644 arch/arm64/include/uapi/asm/kvm_para.h
> create mode 100644 virt/kvm/arm/async_pf.c
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f77c706777ec..a207728d6f3f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -250,6 +250,23 @@ struct vcpu_reset_state {
> bool reset;
> };
>
> +#ifdef CONFIG_KVM_ASYNC_PF
> +
> +/* Should be a power of two number */
> +#define ASYNC_PF_PER_VCPU 64
What exactly is this number?
> +
> +/*
> + * The association of gfn and token. The token will be sent to guest as
> + * page fault address. Also, the guest could be in aarch32 mode. So its
> + * length should be 32-bits.
> + */
The length of what should be 32-bit? The token?
The guest sees the token as the fault address? How exactly is that
exposed to the guest, is that via a synthetic S1 fault?
> +struct kvm_arch_async_pf {
> + u32 token;
> + gfn_t gfn;
> + u32 esr;
> +};
> +#endif /* CONFIG_KVM_ASYNC_PF */
> +
> struct kvm_vcpu_arch {
> struct kvm_cpu_context ctxt;
> void *sve_state;
> @@ -351,6 +368,17 @@ struct kvm_vcpu_arch {
> u64 last_steal;
> gpa_t base;
> } steal;
> +
> +#ifdef CONFIG_KVM_ASYNC_PF
> + struct {
> + struct gfn_to_hva_cache cache;
> + gfn_t gfns[ASYNC_PF_PER_VCPU];
> + u64 control_block;
> + u16 id;
> + bool send_user_only;
> + u64 no_fault_inst_range;
What are all of these fields? This implies functionality not covered
in the commit message, and it's not at all clear what these are.
For example, what exactly is `no_fault_inst_range`? If it's a range,
surely that needs a start/end or base/size pair rather than a single
value?
> + } apf;
> +#endif
> };
>
> /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> @@ -604,6 +632,21 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>
> static inline void __cpu_init_stage2(void) {}
>
> +#ifdef CONFIG_KVM_ASYNC_PF
> +bool kvm_async_pf_hash_find(struct kvm_vcpu *vcpu, gfn_t gfn);
> +bool kvm_arch_can_inject_async_page_not_present(struct kvm_vcpu *vcpu);
> +bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu);
> +int kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, u32 esr,
> + gpa_t gpa, gfn_t gfn);
> +void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work);
> +void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work);
> +void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work);
> +long kvm_async_pf_hypercall(struct kvm_vcpu *vcpu);
> +#endif /* CONFIG_KVM_ASYNC_PF */
> +
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/include/asm/kvm_para.h b/arch/arm64/include/asm/kvm_para.h
> new file mode 100644
> index 000000000000..0ea481dd1c7a
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_para.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM_KVM_PARA_H
> +#define _ASM_ARM_KVM_PARA_H
> +
> +#include <uapi/asm/kvm_para.h>
> +
> +static inline bool kvm_check_and_clear_guest_paused(void)
> +{
> + return false;
> +}
> +
> +static inline unsigned int kvm_arch_para_features(void)
> +{
> + return 0;
> +}
> +
> +static inline unsigned int kvm_arch_para_hints(void)
> +{
> + return 0;
> +}
> +
> +static inline bool kvm_para_available(void)
> +{
> + return false;
> +}
> +
> +#endif /* _ASM_ARM_KVM_PARA_H */
> diff --git a/arch/arm64/include/uapi/asm/Kbuild b/arch/arm64/include/uapi/asm/Kbuild
> index 602d137932dc..f66554cd5c45 100644
> --- a/arch/arm64/include/uapi/asm/Kbuild
> +++ b/arch/arm64/include/uapi/asm/Kbuild
> @@ -1,3 +1 @@
> # SPDX-License-Identifier: GPL-2.0
> -
> -generic-y += kvm_para.h
> diff --git a/arch/arm64/include/uapi/asm/kvm_para.h b/arch/arm64/include/uapi/asm/kvm_para.h
> new file mode 100644
> index 000000000000..e0bd0e579b9a
> --- /dev/null
> +++ b/arch/arm64/include/uapi/asm/kvm_para.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_ASM_ARM_KVM_PARA_H
> +#define _UAPI_ASM_ARM_KVM_PARA_H
> +
> +#include <linux/types.h>
> +
> +#define KVM_FEATURE_ASYNC_PF 0
> +
> +/* Async PF */
> +#define KVM_ASYNC_PF_ENABLED (1 << 0)
> +#define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1)
> +
> +#define KVM_PV_REASON_PAGE_NOT_PRESENT 1
> +#define KVM_PV_REASON_PAGE_READY 2
> +
> +struct kvm_vcpu_pv_apf_data {
> + __u32 reason;
> + __u8 pad[60];
> + __u32 enabled;
> +};
What's all the padding for?
> +
> +#endif /* _UAPI_ASM_ARM_KVM_PARA_H */
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 449386d76441..1053e16b1739 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -34,6 +34,7 @@ config KVM
> select KVM_VFIO
> select HAVE_KVM_EVENTFD
> select HAVE_KVM_IRQFD
> + select KVM_ASYNC_PF
> select KVM_ARM_PMU if HW_PERF_EVENTS
> select HAVE_KVM_MSI
> select HAVE_KVM_IRQCHIP
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 5ffbdc39e780..3be24c1e401f 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -37,3 +37,5 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-debug.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> +kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o
> +kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/arm/async_pf.o
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index bdc0124a064a..22007dd3b9f0 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -94,6 +94,7 @@
>
> /* KVM "vendor specific" services */
> #define ARM_SMCCC_KVM_FUNC_FEATURES 0
> +#define ARM_SMCCC_KVM_FUNC_APF 1
> #define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
> #define ARM_SMCCC_KVM_NUM_FUNCS 128
>
> @@ -102,6 +103,11 @@
> ARM_SMCCC_SMC_32, \
> ARM_SMCCC_OWNER_VENDOR_HYP, \
> ARM_SMCCC_KVM_FUNC_FEATURES)
> +#define ARM_SMCCC_VENDOR_HYP_KVM_APF_FUNC_ID \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_VENDOR_HYP, \
> + ARM_SMCCC_KVM_FUNC_APF)
>
> #ifndef __ASSEMBLY__
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 2cbb57485760..3f62899cef13 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -222,6 +222,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> */
> r = 1;
> break;
> +#ifdef CONFIG_KVM_ASYNC_PF
> + case KVM_CAP_ASYNC_PF:
> + r = 1;
> + break;
> +#endif
> default:
> r = kvm_arch_vm_ioctl_check_extension(kvm, ext);
> break;
> @@ -269,6 +274,10 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> /* Force users to call KVM_ARM_VCPU_INIT */
> vcpu->arch.target = -1;
> bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> +#ifdef CONFIG_KVM_ASYNC_PF
> + vcpu->arch.apf.control_block = 0UL;
> + vcpu->arch.apf.no_fault_inst_range = 0x800;
Where has this magic number come from?
> +#endif
>
> /* Set up the timer */
> kvm_timer_vcpu_init(vcpu);
> @@ -426,8 +435,27 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> {
> bool irq_lines = *vcpu_hcr(v) & (HCR_VI | HCR_VF);
> - return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
> - && !v->arch.power_off && !v->arch.pause);
> +
> + if ((irq_lines || kvm_vgic_vcpu_pending_irq(v)) &&
> + !v->arch.power_off && !v->arch.pause)
> + return true;
> +
> +#ifdef CONFIG_KVM_ASYNC_PF
> + if (v->arch.apf.control_block & KVM_ASYNC_PF_ENABLED) {
> + u32 val;
> + int ret;
> +
> + if (!list_empty_careful(&v->async_pf.done))
> + return true;
> +
> + ret = kvm_read_guest_cached(v->kvm, &v->arch.apf.cache,
> + &val, sizeof(val));
> + if (ret || val)
> + return true;
> + }
> +#endif
> +
> + return false;
> }
>
> bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> @@ -683,6 +711,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>
> check_vcpu_requests(vcpu);
>
> +#ifdef CONFIG_KVM_ASYNC_PF
> + kvm_check_async_pf_completion(vcpu);
> +#endif
Rather than adding ifdeffery like this, please add an empty stub for
when CONFIG_KVM_ASYNC_PF isn't selected, so that this can be used
unconditionally.
> +
> /*
> * Preparing the interrupts to be injected also
> * involves poking the GIC, which must be done in a
> diff --git a/virt/kvm/arm/async_pf.c b/virt/kvm/arm/async_pf.c
> new file mode 100644
> index 000000000000..5be49d684de3
> --- /dev/null
> +++ b/virt/kvm/arm/async_pf.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Asynchronous Page Fault Support
> + *
> + * Copyright (C) 2020 Red Hat, Inc., Gavin Shan
> + *
> + * Based on arch/x86/kernel/kvm.c
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_emulate.h>
> +#include <kvm/arm_hypercalls.h>
> +
> +static inline u32 kvm_async_pf_hash_fn(gfn_t gfn)
> +{
> + return hash_32(gfn & 0xffffffff, order_base_2(ASYNC_PF_PER_VCPU));
> +}
> +
> +static inline u32 kvm_async_pf_hash_next(u32 key)
> +{
> + return (key + 1) & (ASYNC_PF_PER_VCPU - 1);
> +}
> +
> +static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
> +{
> + int i;
> +
> + for (i = 0; i < ASYNC_PF_PER_VCPU; i++)
> + vcpu->arch.apf.gfns[i] = ~0;
> +}
> +
> +/*
> + * Add gfn to the hash table. It's ensured there is a free entry
> + * when this function is called.
> + */
> +static void kvm_async_pf_hash_add(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> + u32 key = kvm_async_pf_hash_fn(gfn);
> +
> + while (vcpu->arch.apf.gfns[key] != ~0)
> + key = kvm_async_pf_hash_next(key);
> +
> + vcpu->arch.apf.gfns[key] = gfn;
> +}
> +
> +static u32 kvm_async_pf_hash_slot(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> + u32 key = kvm_async_pf_hash_fn(gfn);
> + int i;
> +
> + for (i = 0; i < ASYNC_PF_PER_VCPU; i++) {
> + if (vcpu->arch.apf.gfns[key] == gfn ||
> + vcpu->arch.apf.gfns[key] == ~0)
> + break;
> +
> + key = kvm_async_pf_hash_next(key);
> + }
> +
> + return key;
> +}
> +
> +static void kvm_async_pf_hash_remove(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> + u32 i, j, k;
> +
> + i = j = kvm_async_pf_hash_slot(vcpu, gfn);
> + while (true) {
> + vcpu->arch.apf.gfns[i] = ~0;
> +
> + do {
> + j = kvm_async_pf_hash_next(j);
> + if (vcpu->arch.apf.gfns[j] == ~0)
> + return;
> +
> + k = kvm_async_pf_hash_fn(vcpu->arch.apf.gfns[j]);
> + /*
> + * k lies cyclically in ]i,j]
> + * | i.k.j |
> + * |....j i.k.| or |.k..j i...|
> + */
> + } while ((i <= j) ? (i < k && k <= j) : (i < k || k <= j));
> +
> + vcpu->arch.apf.gfns[i] = vcpu->arch.apf.gfns[j];
> + i = j;
> + }
> +}
This looks like a copy-paste of code under arch/x86.
This looks like something that should be factored into common code
rather than duplicated. Do we not have an existing common hash table
implementation that we can use rather than building one specific to KVM
async page faults?
> +
> +bool kvm_async_pf_hash_find(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> + u32 key = kvm_async_pf_hash_slot(vcpu, gfn);
> +
> + return vcpu->arch.apf.gfns[key] == gfn;
> +}
> +
> +static inline int kvm_async_pf_read_cache(struct kvm_vcpu *vcpu, u32 *val)
> +{
> + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.apf.cache,
> + val, sizeof(*val));
> +}
> +
> +static inline int kvm_async_pf_write_cache(struct kvm_vcpu *vcpu, u32 val)
> +{
> + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.apf.cache,
> + &val, sizeof(val));
> +}
> +
> +bool kvm_arch_can_inject_async_page_not_present(struct kvm_vcpu *vcpu)
> +{
> + u64 vbar, pc;
> + u32 val;
> + int ret;
> +
> + if (!(vcpu->arch.apf.control_block & KVM_ASYNC_PF_ENABLED))
> + return false;
> +
> + if (vcpu->arch.apf.send_user_only && vcpu_mode_priv(vcpu))
> + return false;
> +
> + /* Pending page fault, which ins't acknowledged by guest */
> + ret = kvm_async_pf_read_cache(vcpu, &val);
> + if (ret || val)
> + return false;
> +
> + /*
> + * Events can't be injected through data abort because it's
> + * going to clobber ELR_EL1, which might not consued (or saved)
> + * by guest yet.
> + */
> + vbar = vcpu_read_sys_reg(vcpu, VBAR_EL1);
> + pc = *vcpu_pc(vcpu);
> + if (pc >= vbar && pc < (vbar + vcpu->arch.apf.no_fault_inst_range))
> + return false;
Ah, so that's when this `no_fault_inst_range` is for.
As-is this is not sufficient, and we'll need t be extremely careful
here.
The vectors themselves typically only have a small amount of stub code,
and the bulk of the non-reentrant exception entry work happens
elsewhere, in a mixture of assembly and C code that isn't even virtually
contiguous with either the vectors or itself.
It's possible in theory that code in modules (or perhaps in eBPF JIT'd
code) that isn't safe to take a fault from, so even having a contiguous
range controlled by the kernel isn't ideal.
How does this work on x86?
> +
> + return true;
> +}
> +
> +/*
> + * We need deliver the page present signal as quick as possible because
> + * it's performance critical. So the signal is delivered no matter which
> + * privilege level the guest has. It's possible the signal can't be handled
> + * by the guest immediately. However, host doesn't contribute the delay
> + * anyway.
> + */
> +bool kvm_arch_can_inject_async_page_present(struct kvm_vcpu *vcpu)
> +{
> + u64 vbar, pc;
> + u32 val;
> + int ret;
> +
> + if (!(vcpu->arch.apf.control_block & KVM_ASYNC_PF_ENABLED))
> + return true;
> +
> + /* Pending page fault, which ins't acknowledged by guest */
> + ret = kvm_async_pf_read_cache(vcpu, &val);
> + if (ret || val)
> + return false;
> +
> + /*
> + * Events can't be injected through data abort because it's
> + * going to clobber ELR_EL1, which might not consued (or saved)
> + * by guest yet.
> + */
> + vbar = vcpu_read_sys_reg(vcpu, VBAR_EL1);
> + pc = *vcpu_pc(vcpu);
> + if (pc >= vbar && pc < (vbar + vcpu->arch.apf.no_fault_inst_range))
> + return false;
> +
> + return true;
> +}
Much of this is identical to the not_present case, so the same comments
apply. The common bits should probably be factored out into a helper.
> +
> +int kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, u32 esr,
> + gpa_t gpa, gfn_t gfn)
> +{
> + struct kvm_arch_async_pf arch;
> + unsigned long hva = kvm_vcpu_gfn_to_hva(vcpu, gfn);
> +
> + arch.token = (vcpu->arch.apf.id++ << 16) | vcpu->vcpu_id;
> + arch.gfn = gfn;
> + arch.esr = esr;
> +
> + return kvm_setup_async_pf(vcpu, gpa, hva, &arch);
> +}
> +
> +/*
> + * It's garanteed that no pending asynchronous page fault when this is
> + * called. It means all previous issued asynchronous page faults have
> + * been acknoledged.
> + */
> +void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work)
> +{
> + int ret;
> +
> + kvm_async_pf_hash_add(vcpu, work->arch.gfn);
> + ret = kvm_async_pf_write_cache(vcpu, KVM_PV_REASON_PAGE_NOT_PRESENT);
> + if (ret) {
> + kvm_err("%s: Error %d writing cache\n", __func__, ret);
> + kvm_async_pf_hash_remove(vcpu, work->arch.gfn);
> + return;
> + }
> +
> + kvm_inject_dabt(vcpu, work->arch.token);
> +}
> +
> +/*
> + * It's garanteed that no pending asynchronous page fault when this is
> + * called. It means all previous issued asynchronous page faults have
> + * been acknoledged.
> + */
> +void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work)
> +{
> + int ret;
> +
> + /* Broadcast wakeup */
> + if (work->wakeup_all)
> + work->arch.token = ~0;
> + else
> + kvm_async_pf_hash_remove(vcpu, work->arch.gfn);
> +
> + ret = kvm_async_pf_write_cache(vcpu, KVM_PV_REASON_PAGE_READY);
> + if (ret) {
> + kvm_err("%s: Error %d writing cache\n", __func__, ret);
> + return;
> + }
> +
> + kvm_inject_dabt(vcpu, work->arch.token);
So the guest sees a fake S1 abort with a fake address?
How is the guest expected to distinguish this from a real S1 fault?
> +}
> +
> +void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu,
> + struct kvm_async_pf *work)
> +{
> + struct kvm_memory_slot *memslot;
> + unsigned int esr = work->arch.esr;
> + phys_addr_t gpa = work->cr2_or_gpa;
> + gfn_t gfn = gpa >> PAGE_SHIFT;
Perhaps:
gfn_t gfn = gpa_to_gfn(gpa);
> + unsigned long hva;
> + bool write_fault, writable;
> + int idx;
> +
> + /*
> + * We shouldn't issue prefault for special work to wake up
> + * all pending tasks because the associated token (address)
> + * is invalid.
> + */
I'm not sure what this comment is trying to say.
> + if (work->wakeup_all)
> + return;
> +
> + /*
> + * The gpa was validated before the work is started. However, the
> + * memory slots might be changed since then. So we need to redo the
> + * validatation here.
> + */
> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
> + write_fault = kvm_is_write_fault(esr);
> + memslot = gfn_to_memslot(vcpu->kvm, gfn);
> + hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
> + if (kvm_is_error_hva(hva) || (write_fault && !writable))
> + goto out;
> +
> + kvm_handle_user_mem_abort(vcpu, esr, memslot, gpa, hva, true);
> +
> +out:
> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +}
> +
> +static long kvm_async_pf_update_enable_reg(struct kvm_vcpu *vcpu, u64 data)
> +{
> + bool enabled, enable;
> + gpa_t gpa = (data & ~0x3F);
What exactly is going on here? Why are the low 7 bits of data not valid?
This will also truncate the value to 32 bits; did you mean to do that?
> + int ret;
> +
> + enabled = !!(vcpu->arch.apf.control_block & KVM_ASYNC_PF_ENABLED);
> + enable = !!(data & KVM_ASYNC_PF_ENABLED);
> + if (enable == enabled) {
> + kvm_debug("%s: Async PF has been %s (0x%llx -> 0x%llx)\n",
> + __func__, enabled ? "enabled" : "disabled",
> + vcpu->arch.apf.control_block, data);
> + return SMCCC_RET_NOT_REQUIRED;
> + }
> +
> + if (enable) {
> + ret = kvm_gfn_to_hva_cache_init(
> + vcpu->kvm, &vcpu->arch.apf.cache,
> + gpa + offsetof(struct kvm_vcpu_pv_apf_data, reason),
> + sizeof(u32));
> + if (ret) {
> + kvm_err("%s: Error %d initializing cache on 0x%llx\n",
> + __func__, ret, data);
> + return SMCCC_RET_NOT_SUPPORTED;
> + }
> +
> + kvm_async_pf_hash_reset(vcpu);
> + vcpu->arch.apf.send_user_only =
> + !(data & KVM_ASYNC_PF_SEND_ALWAYS);
> + kvm_async_pf_wakeup_all(vcpu);
> + vcpu->arch.apf.control_block = data;
> + } else {
> + kvm_clear_async_pf_completion_queue(vcpu);
> + vcpu->arch.apf.control_block = data;
> + }
> +
> + return SMCCC_RET_SUCCESS;
> +}
> +
> +long kvm_async_pf_hypercall(struct kvm_vcpu *vcpu)
> +{
> + u64 data, func, val, range;
> + long ret = SMCCC_RET_SUCCESS;
> +
> + data = (smccc_get_arg2(vcpu) << 32) | smccc_get_arg1(vcpu);
What prevents the high bits being set in arg1?
> + func = data & (0xfful << 56);
> + val = data & ~(0xfful << 56);
> + switch (func) {
> + case BIT(63):
> + ret = kvm_async_pf_update_enable_reg(vcpu, val);
Please give BIT(63) a mnemonic.
> + break;
> + case BIT(62):
Likewise.
> + if (vcpu->arch.apf.control_block & KVM_ASYNC_PF_ENABLED) {
> + ret = SMCCC_RET_NOT_SUPPORTED;
> + break;
> + }
> +
> + range = vcpu->arch.apf.no_fault_inst_range;
> + vcpu->arch.apf.no_fault_inst_range = max(range, val);
Huh? How is the `no_fault_inst_range` set by he guest?
Thanks,
Mark.
> + break;
> + default:
> + kvm_err("%s: Unrecognized function 0x%llx\n", __func__, func);
> + ret = SMCCC_RET_NOT_SUPPORTED;
> + }
> +
> + return ret;
> +}
> diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
> index db6dce3d0e23..a7e0fe17e2f1 100644
> --- a/virt/kvm/arm/hypercalls.c
> +++ b/virt/kvm/arm/hypercalls.c
> @@ -70,7 +70,15 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> break;
> case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> +#ifdef CONFIG_KVM_ASYNC_PF
> + val[0] |= BIT(ARM_SMCCC_KVM_FUNC_APF);
> +#endif
> break;
> +#ifdef CONFIG_KVM_ASYNC_PF
> + case ARM_SMCCC_VENDOR_HYP_KVM_APF_FUNC_ID:
> + val[0] = kvm_async_pf_hypercall(vcpu);
> + break;
> +#endif
> default:
> return kvm_psci_call(vcpu);
> }
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index 95aaabb2b1fc..a303815845a2 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -1656,6 +1656,30 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot,
> (hva & ~(map_size - 1)) + map_size <= uaddr_end;
> }
>
> +static bool try_async_pf(struct kvm_vcpu *vcpu, u32 esr, gpa_t gpa,
> + gfn_t gfn, kvm_pfn_t *pfn, bool write,
> + bool *writable, bool prefault)
> +{
> + struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
> +#ifdef CONFIG_KVM_ASYNC_PF
> + bool async = false;
> +
> + /* Bail if *pfn has correct page */
> + *pfn = __gfn_to_pfn_memslot(slot, gfn, false, &async, write, writable);
> + if (!async)
> + return false;
> +
> + if (!prefault && kvm_arch_can_inject_async_page_not_present(vcpu)) {
> + if (kvm_async_pf_hash_find(vcpu, gfn) ||
> + kvm_arch_setup_async_pf(vcpu, esr, gpa, gfn))
> + return true;
> + }
> +#endif
> +
> + *pfn = __gfn_to_pfn_memslot(slot, gfn, false, NULL, write, writable);
> + return false;
> +}
> +
> int kvm_handle_user_mem_abort(struct kvm_vcpu *vcpu, unsigned int esr,
> struct kvm_memory_slot *memslot,
> phys_addr_t fault_ipa, unsigned long hva,
> @@ -1737,7 +1761,10 @@ int kvm_handle_user_mem_abort(struct kvm_vcpu *vcpu, unsigned int esr,
> */
> smp_rmb();
>
> - pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
> + if (try_async_pf(vcpu, esr, fault_ipa, gfn, &pfn,
> + write_fault, &writable, prefault))
> + return 1;
> +
> if (pfn == KVM_PFN_ERR_HWPOISON) {
> kvm_send_hwpoison_signal(hva, vma_shift);
> return 0;
> --
> 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 RFCv2 9/9] arm64: Support async page fault
From: Mark Rutland @ 2020-05-26 12:56 UTC (permalink / raw)
To: Gavin Shan
Cc: aarcange, drjones, suzuki.poulose, catalin.marinas, linux-kernel,
eric.auger, james.morse, shan.gavin, maz, will, kvmarm,
linux-arm-kernel
In-Reply-To: <20200508032919.52147-10-gshan@redhat.com>
On Fri, May 08, 2020 at 01:29:19PM +1000, Gavin Shan wrote:
> This supports asynchronous page fault for the guest. The design is
> similar to what x86 has: on receiving a PAGE_NOT_PRESENT signal from
> the host, the current task is either rescheduled or put into power
> saving mode. The task will be waken up when PAGE_READY signal is
> received. The PAGE_READY signal might be received in the context
> of the suspended process, to be waken up. That means the suspended
> process has to wake up itself, but it's not safe and prone to cause
> dead-lock on CPU runqueue lock. So the wakeup is delayed on returning
> from kernel space to user space or idle process is picked for running.
>
> The signals are conveyed through the async page fault control block,
> which was passed to host on enabling the functionality. On each page
> fault, the control block is checked and switch to the async page fault
> handling flow if any signals exist.
>
> The feature is put into the CONFIG_KVM_GUEST umbrella, which is added
> by this patch. So we have inline functions implemented in kvm_para.h,
> like other architectures do, to check if async page fault (one of the
> KVM para-virtualized features) is available. Also, the kernel boot
> parameter "no-kvmapf" can be specified to disable the feature.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/Kconfig | 11 +
> arch/arm64/include/asm/exception.h | 3 +
> arch/arm64/include/asm/kvm_para.h | 27 +-
> arch/arm64/kernel/entry.S | 33 +++
> arch/arm64/kernel/process.c | 4 +
> arch/arm64/mm/fault.c | 434 +++++++++++++++++++++++++++++
> 6 files changed, 505 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 40fb05d96c60..2d5e5ee62d6d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1045,6 +1045,17 @@ config PARAVIRT
> under a hypervisor, potentially improving performance significantly
> over full virtualization.
>
> +config KVM_GUEST
> + bool "KVM Guest Support"
> + depends on PARAVIRT
> + default y
> + help
> + This option enables various optimizations for running under the KVM
> + hypervisor. Overhead for the kernel when not running inside KVM should
> + be minimal.
> +
> + In case of doubt, say Y
> +
> config PARAVIRT_TIME_ACCOUNTING
> bool "Paravirtual steal time accounting"
> select PARAVIRT
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index 7a6e81ca23a8..d878afa42746 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -46,4 +46,7 @@ void bad_el0_sync(struct pt_regs *regs, int reason, unsigned int esr);
> void do_cp15instr(unsigned int esr, struct pt_regs *regs);
> void do_el0_svc(struct pt_regs *regs);
> void do_el0_svc_compat(struct pt_regs *regs);
> +#ifdef CONFIG_KVM_GUEST
> +void kvm_async_pf_delayed_wake(void);
> +#endif
> #endif /* __ASM_EXCEPTION_H */
> diff --git a/arch/arm64/include/asm/kvm_para.h b/arch/arm64/include/asm/kvm_para.h
> index 0ea481dd1c7a..b2f8ef243df7 100644
> --- a/arch/arm64/include/asm/kvm_para.h
> +++ b/arch/arm64/include/asm/kvm_para.h
> @@ -3,6 +3,20 @@
> #define _ASM_ARM_KVM_PARA_H
>
> #include <uapi/asm/kvm_para.h>
> +#include <linux/of.h>
> +#include <asm/hypervisor.h>
> +
> +#ifdef CONFIG_KVM_GUEST
> +static inline int kvm_para_available(void)
> +{
> + return 1;
> +}
> +#else
> +static inline int kvm_para_available(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_KVM_GUEST */
Please make these bool, and return true/false, as was the case with the
existing stub.
>
> static inline bool kvm_check_and_clear_guest_paused(void)
> {
> @@ -11,17 +25,16 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>
> static inline unsigned int kvm_arch_para_features(void)
> {
> - return 0;
> + unsigned int features = 0;
> +
> + if (kvm_arm_hyp_service_available(ARM_SMCCC_KVM_FUNC_APF))
> + features |= (1 << KVM_FEATURE_ASYNC_PF);
> +
> + return features;
> }
>
> static inline unsigned int kvm_arch_para_hints(void)
> {
> return 0;
> }
> -
> -static inline bool kvm_para_available(void)
> -{
> - return false;
> -}
> -
> #endif /* _ASM_ARM_KVM_PARA_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index ddcde093c433..15efd57129ff 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -751,12 +751,45 @@ finish_ret_to_user:
> enable_step_tsk x1, x2
> #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> bl stackleak_erase
> +#endif
> +#ifdef CONFIG_KVM_GUEST
> + bl kvm_async_pf_delayed_wake
> #endif
Yuck. I am very much not keen on this living in the entry assembly.
What precisely is this needed for?
> kernel_exit 0
> ENDPROC(ret_to_user)
>
> .popsection // .entry.text
>
> +#ifdef CONFIG_KVM_GUEST
> + .pushsection ".rodata", "a"
> +SYM_DATA_START(__exception_handlers_offset)
> + .quad 0
> + .quad 0
> + .quad 0
> + .quad 0
> + .quad el1_sync - vectors
> + .quad el1_irq - vectors
> + .quad 0
> + .quad el1_error - vectors
> + .quad el0_sync - vectors
> + .quad el0_irq - vectors
> + .quad 0
> + .quad el0_error - vectors
> +#ifdef CONFIG_COMPAT
> + .quad el0_sync_compat - vectors
> + .quad el0_irq_compat - vectors
> + .quad 0
> + .quad el0_error_compat - vectors
> +#else
> + .quad 0
> + .quad 0
> + .quad 0
> + .quad 0
> +#endif
> +SYM_DATA_END(__exception_handlers_offset)
> + .popsection // .rodata
> +#endif /* CONFIG_KVM_GUEST */
This looks scary, and needs an introduction in the commit message.
> +
> #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> /*
> * Exception vectors trampoline.
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 56be4cbf771f..5e7ee553566d 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -53,6 +53,7 @@
> #include <asm/processor.h>
> #include <asm/pointer_auth.h>
> #include <asm/stacktrace.h>
> +#include <asm/exception.h>
>
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> #include <linux/stackprotector.h>
> @@ -70,6 +71,9 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>
> static void __cpu_do_idle(void)
> {
> +#ifdef CONFIG_KVM_GUEST
> + kvm_async_pf_delayed_wake();
> +#endif
> dsb(sy);
> wfi();
> }
This is meant to be a very low-level helper, so I don't think this
should live here.
If nothing else, this needs to have no overhead when async page faults
are not in use, so this probably needs an inline with a static key.
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index c9cedc0432d2..cbf8b52135c9 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -19,10 +19,12 @@
> #include <linux/page-flags.h>
> #include <linux/sched/signal.h>
> #include <linux/sched/debug.h>
> +#include <linux/swait.h>
> #include <linux/highmem.h>
> #include <linux/perf_event.h>
> #include <linux/preempt.h>
> #include <linux/hugetlb.h>
> +#include <linux/kvm_para.h>
>
> #include <asm/acpi.h>
> #include <asm/bug.h>
> @@ -48,8 +50,31 @@ struct fault_info {
> const char *name;
> };
>
> +#ifdef CONFIG_KVM_GUEST
> +struct kvm_task_sleep_node {
> + struct hlist_node link;
> + struct swait_queue_head wq;
> + u32 token;
> + struct task_struct *task;
> + int cpu;
> + bool halted;
> + bool delayed;
> +};
> +
> +struct kvm_task_sleep_head {
> + raw_spinlock_t lock;
> + struct hlist_head list;
> +};
> +#endif /* CONFIG_KVM_GUEST */
> +
> static const struct fault_info fault_info[];
> static struct fault_info debug_fault_info[];
> +#ifdef CONFIG_KVM_GUEST
> +extern char __exception_handlers_offset[];
> +static bool async_pf_available = true;
> +static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_data) __aligned(64);
> +static DEFINE_PER_CPU(struct kvm_task_sleep_head, apf_head);
> +#endif
>
> static inline const struct fault_info *esr_to_fault_info(unsigned int esr)
> {
> @@ -717,10 +742,281 @@ static const struct fault_info fault_info[] = {
> { do_bad, SIGKILL, SI_KERNEL, "unknown 63" },
> };
>
> +#ifdef CONFIG_KVM_GUEST
> +static inline unsigned int kvm_async_pf_read_enabled(void)
> +{
> + return __this_cpu_read(apf_data.enabled);
> +}
> +
> +static inline void kvm_async_pf_write_enabled(unsigned int val)
> +{
> + __this_cpu_write(apf_data.enabled, val);
> +}
> +
> +static inline unsigned int kvm_async_pf_read_reason(void)
> +{
> + return __this_cpu_read(apf_data.reason);
> +}
> +
> +static inline void kvm_async_pf_write_reason(unsigned int val)
> +{
> + __this_cpu_write(apf_data.reason, val);
> +}
> +
> +#define kvm_async_pf_lock(b, flags) \
> + raw_spin_lock_irqsave(&(b)->lock, (flags))
> +#define kvm_async_pf_trylock(b, flags) \
> + raw_spin_trylock_irqsave(&(b)->lock, (flags))
> +#define kvm_async_pf_unlock(b, flags) \
> + raw_spin_unlock_irqrestore(&(b)->lock, (flags))
> +#define kvm_async_pf_unlock_and_clear(b, flags) \
> + do { \
> + raw_spin_unlock_irqrestore(&(b)->lock, (flags)); \
> + kvm_async_pf_write_reason(0); \
> + } while (0)
> +
> +static struct kvm_task_sleep_node *kvm_async_pf_find(
> + struct kvm_task_sleep_head *b, u32 token)
> +{
> + struct kvm_task_sleep_node *n;
> + struct hlist_node *p;
> +
> + hlist_for_each(p, &b->list) {
> + n = hlist_entry(p, typeof(*n), link);
> + if (n->token == token)
> + return n;
> + }
> +
> + return NULL;
> +}
> +
> +static void kvm_async_pf_wait(u32 token, int in_kernel)
> +{
> + struct kvm_task_sleep_head *b = this_cpu_ptr(&apf_head);
> + struct kvm_task_sleep_node n, *e;
> + DECLARE_SWAITQUEUE(wait);
> + unsigned long flags;
> +
> + kvm_async_pf_lock(b, flags);
> + e = kvm_async_pf_find(b, token);
> + if (e) {
> + /* dummy entry exist -> wake up was delivered ahead of PF */
> + hlist_del(&e->link);
> + kfree(e);
> + kvm_async_pf_unlock_and_clear(b, flags);
> +
> + return;
> + }
> +
> + n.token = token;
> + n.task = current;
> + n.cpu = smp_processor_id();
> + n.halted = is_idle_task(current) ||
> + (IS_ENABLED(CONFIG_PREEMPT_COUNT) ?
> + preempt_count() > 1 || rcu_preempt_depth() : in_kernel);
> + n.delayed = false;
> + init_swait_queue_head(&n.wq);
> + hlist_add_head(&n.link, &b->list);
> + kvm_async_pf_unlock_and_clear(b, flags);
> +
> + for (;;) {
> + if (!n.halted) {
> + prepare_to_swait_exclusive(&n.wq, &wait,
> + TASK_UNINTERRUPTIBLE);
> + }
> +
> + if (hlist_unhashed(&n.link))
> + break;
> +
> + if (!n.halted) {
> + schedule();
> + } else {
> + dsb(sy);
> + wfi();
> + }
Please don't open-code idle sequences. I strongly suspect this won't
work with pseudo-nmi, and regardless we don't want to duplicate this.
> + }
> +
> + if (!n.halted)
> + finish_swait(&n.wq, &wait);
> +}
> +
> +/*
> + * There are two cases the suspended processed can't be waken up
> + * immediately: The waker is exactly the suspended process, or
> + * the current CPU runqueue has been locked. Otherwise, we might
> + * run into dead-lock.
> + */
> +static inline void kvm_async_pf_wake_one(struct kvm_task_sleep_node *n)
> +{
> + if (n->task == current ||
> + cpu_rq_is_locked(smp_processor_id())) {
> + n->delayed = true;
> + return;
> + }
> +
> + hlist_del_init(&n->link);
> + if (n->halted)
> + smp_send_reschedule(n->cpu);
> + else
> + swake_up_one(&n->wq);
> +}
> +
> +void kvm_async_pf_delayed_wake(void)
> +{
> + struct kvm_task_sleep_head *b;
> + struct kvm_task_sleep_node *n;
> + struct hlist_node *p, *next;
> + unsigned int reason;
> + unsigned long flags;
> +
> + if (!kvm_async_pf_read_enabled())
> + return;
> +
> + /*
> + * We're running in the edging context, we need to complete
> + * the work as quick as possible. So we have a preliminary
> + * check without holding the lock.
> + */
What is 'the edging context'?
> + b = this_cpu_ptr(&apf_head);
> + if (hlist_empty(&b->list))
> + return;
> +
> + /*
> + * Set the async page fault reason to something to avoid
> + * receiving the signals, which might cause lock contention
> + * and possibly dead-lock. As we're in guest context, it's
> + * safe to set the reason here.
> + *
> + * There might be pending signals. For that case, we needn't
> + * do anything. Otherwise, the pending signal will be lost.
> + */
> + reason = kvm_async_pf_read_reason();
> + if (!reason) {
> + kvm_async_pf_write_reason(KVM_PV_REASON_PAGE_NOT_PRESENT +
> + KVM_PV_REASON_PAGE_READY);
> + }
Huh? Are we doing this to prevent the host from writing tho this area?
> +
> + if (!kvm_async_pf_trylock(b, flags))
> + goto done;
> +
> + hlist_for_each_safe(p, next, &b->list) {
> + n = hlist_entry(p, typeof(*n), link);
> + if (n->cpu != smp_processor_id())
> + continue;
> + if (!n->delayed)
> + continue;
> +
> + kvm_async_pf_wake_one(n);
> + }
> +
> + kvm_async_pf_unlock(b, flags);
> +
> +done:
> + if (!reason)
> + kvm_async_pf_write_reason(0);
> +}
> +NOKPROBE_SYMBOL(kvm_async_pf_delayed_wake);
> +
> +static void kvm_async_pf_wake_all(void)
> +{
> + struct kvm_task_sleep_head *b;
> + struct kvm_task_sleep_node *n;
> + struct hlist_node *p, *next;
> + unsigned long flags;
> +
> + b = this_cpu_ptr(&apf_head);
> + kvm_async_pf_lock(b, flags);
> +
> + hlist_for_each_safe(p, next, &b->list) {
> + n = hlist_entry(p, typeof(*n), link);
> + kvm_async_pf_wake_one(n);
> + }
> +
> + kvm_async_pf_unlock(b, flags);
> +
> + kvm_async_pf_write_reason(0);
> +}
> +
> +static void kvm_async_pf_wake(u32 token)
> +{
> + struct kvm_task_sleep_head *b = this_cpu_ptr(&apf_head);
> + struct kvm_task_sleep_node *n;
> + unsigned long flags;
> +
> + if (token == ~0) {
> + kvm_async_pf_wake_all();
> + return;
> + }
> +
> +again:
> + kvm_async_pf_lock(b, flags);
> +
> + n = kvm_async_pf_find(b, token);
> + if (!n) {
> + /*
> + * Async PF was not yet handled. Add dummy entry
> + * for the token. Busy wait until other CPU handles
> + * the async PF on allocation failure.
> + */
> + n = kzalloc(sizeof(*n), GFP_ATOMIC);
> + if (!n) {
> + kvm_async_pf_unlock(b, flags);
> + cpu_relax();
> + goto again;
> + }
> + n->token = token;
> + n->task = current;
> + n->cpu = smp_processor_id();
> + n->halted = false;
> + n->delayed = false;
> + init_swait_queue_head(&n->wq);
> + hlist_add_head(&n->link, &b->list);
> + } else {
> + kvm_async_pf_wake_one(n);
> + }
> +
> + kvm_async_pf_unlock_and_clear(b, flags);
> +}
> +
> +static bool do_async_pf(unsigned long addr, unsigned int esr,
> + struct pt_regs *regs)
> +{
> + u32 reason;
> +
> + if (!kvm_async_pf_read_enabled())
> + return false;
> +
> + reason = kvm_async_pf_read_reason();
> + if (!reason)
> + return false;
> +
> + switch (reason) {
> + case KVM_PV_REASON_PAGE_NOT_PRESENT:
> + kvm_async_pf_wait((u32)addr, !user_mode(regs));
> + break;
> + case KVM_PV_REASON_PAGE_READY:
> + kvm_async_pf_wake((u32)addr);
> + break;
> + default:
> + if (reason) {
> + pr_warn("%s: Illegal reason %d\n", __func__, reason);
> + kvm_async_pf_write_reason(0);
> + }
> + }
> +
> + return true;
> +}
> +#endif /* CONFIG_KVM_GUEST */
> +
> void do_mem_abort(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> {
> const struct fault_info *inf = esr_to_fault_info(esr);
>
> +#ifdef CONFIG_KVM_GUEST
> + if (do_async_pf(addr, esr, regs))
> + return;
> +#endif
> +
> if (!inf->fn(addr, esr, regs))
> return;
>
> @@ -878,3 +1174,141 @@ void do_debug_exception(unsigned long addr_if_watchpoint, unsigned int esr,
> debug_exception_exit(regs);
> }
> NOKPROBE_SYMBOL(do_debug_exception);
> +
> +#ifdef CONFIG_KVM_GUEST
> +static int __init kvm_async_pf_available(char *arg)
> +{
> + async_pf_available = false;
> + return 0;
> +}
> +early_param("no-kvmapf", kvm_async_pf_available);
> +
> +static void kvm_async_pf_enable(bool enable)
> +{
> + struct arm_smccc_res res;
> + unsigned long *offsets = (unsigned long *)__exception_handlers_offset;
> + u32 enabled = kvm_async_pf_read_enabled();
> + u64 val;
> + int i;
> +
> + if (enable == enabled)
> + return;
> +
> + if (enable) {
> + /*
> + * Asychonous page faults will be prohibited when CPU runs
> + * instructions between the vector base and the maximal
> + * offset, plus 4096. The 4096 is the assumped maximal
> + * length for individual handler. The hardware registers
> + * should be saved to stack at the beginning of the handlers,
> + * so 4096 shuld be safe enough.
> + */
> + val = 0;
> + for (i = 0; i < 16; i++) {
> + if (offsets[i] > val)
> + val = offsets[i];
> + }
> +
> + val += 4096;
NAK. This assumption is not true, and regardless we should not make any
assumptions of this sort; we should derive this from the code
explicitly. Guessing is not ok.
Given that non-reentrant exception handling code is scattered across at
least:
* kernel/debug-monitors.c
* kernel/entry.S
* kernel/entry-common.S
* kernel/traps.c
* mm/fault.c
... we *cannot* assume that fault handling code is virtually contiguous,
and certainly cannot assume where this falls w.r.t. the architectural
vectors that VBAR_ELx points to.
How does x86 handle this?
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Re: [PATCH] i2c: stm32f7: Fix runtime PM imbalance in stm32f7_i2c_reg_slave
From: dinghao.liu @ 2020-05-26 13:00 UTC (permalink / raw)
To: Alain Volmat
Cc: Alexandre Torgue, kjlu, linux-kernel, Pierre-Yves MORDRET,
linux-i2c, Maxime Coquelin, linux-stm32, linux-arm-kernel
In-Reply-To: <20200526083400.GC10725@gnbcxd0016.gnb.st.com>
> Overall, there are several other calls to pm_runtime_get_sync within this
> driver, would you like to fix them all at once ?
>
Sure, I will send a new patch to merge them all.
> On Thu, May 21, 2020 at 03:05:07PM +0800, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
> >
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > ---
> > drivers/i2c/busses/i2c-stm32f7.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> > index 330ffed011e0..602cf35649c8 100644
> > --- a/drivers/i2c/busses/i2c-stm32f7.c
> > +++ b/drivers/i2c/busses/i2c-stm32f7.c
> > @@ -1767,8 +1767,10 @@ static int stm32f7_i2c_reg_slave(struct i2c_client *slave)
> > return ret;
> >
> > ret = pm_runtime_get_sync(dev);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + pm_runtime_put_autosuspend(dev);
>
> Considering that if we fail here there is a very good chance that this is due
> to the resume failing, pm_runtime_put_noidle would probably make more sense
> since pm_runtime_put_autosuspend will most probably fail as well.
>
Agree. Thank you for your advice!
Regards.
Dinghao
_______________________________________________
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 v6] arm64: Expose FAR_EL1 tag bits in sigcontext
From: Dave Martin @ 2020-05-26 13:03 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Will Deacon, Andrey Konovalov, Kevin Brodsky, Oleg Nesterov,
Kostya Serebryany, Evgenii Stepanov, Catalin Marinas,
Vincenzo Frascino, Peter Collingbourne, Linux ARM,
Richard Henderson
In-Reply-To: <874ks9drb6.fsf@x220.int.ebiederm.org>
On Thu, May 21, 2020 at 02:24:45PM -0500, Eric W. Biederman wrote:
> Peter Collingbourne <pcc@google.com> writes:
>
> > On Thu, May 21, 2020 at 5:39 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Peter Collingbourne <pcc@google.com> writes:
> >>
> >> > On Wed, May 20, 2020 at 2:26 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >> >>
> >> >> On Wed, May 20, 2020 at 09:55:03AM +0100, Will Deacon wrote:
> >> >> > On Tue, May 19, 2020 at 03:00:12PM -0700, Peter Collingbourne wrote:
> >> >> > > On Mon, May 18, 2020 at 2:53 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >> >> > > > On Thu, May 14, 2020 at 05:58:21PM -0700, Peter Collingbourne wrote:
> >> >> > > > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> >> >> > > > > index baa88dc02e5c..5867f2fdbe64 100644
> >> >> > > > > --- a/arch/arm64/kernel/signal.c
> >> >> > > > > +++ b/arch/arm64/kernel/signal.c
> >> >> > > > > @@ -648,6 +648,7 @@ static int setup_sigframe(struct
> >> >> > > > > rt_sigframe_user_layout *user,
> >> >> > > > > __put_user_error(ESR_MAGIC, &esr_ctx->head.magic, err);
> >> >> > > > > __put_user_error(sizeof(*esr_ctx), &esr_ctx->head.size, err);
> >> >> > > > > __put_user_error(current->thread.fault_code,
> >> >> > > > > &esr_ctx->esr, err);
> >> >> > > > > + current->thread.fault_code = 0;
> >> >> > > >
> >> >> > > > Perhaps, but we'd need to be careful. For example, can we run out of
> >> >> > > > user stack before this and deliver a SIGSEGV, but with the old
> >> >> > > > fault_code still set? Then we'd emit the old fault code with the
> >> >> > > > new "can't deliver signal" signal, which doesn't make sense.
> >> >> > > >
> >> >> > > > Stuff may also go wrong with signal prioritisation.
> >> >> > > >
> >> >> > > > If a higher-priority signal (say SIGINT) comes in after a data abort
> >> >> > > > enters the kernel but before the resulting SIGSEGV is dequeued for
> >> >> > > > delivery, wouldn't we deliver SIGINT first, with the bogus fault code?
> >> >> > > > With your change we'd then have cleared the fault code by the time we
> >> >> > > > deliver the SIGSEGV it actually relates to, if I've understood right.
> >> >> > > >
> >> >> > > > Today, I think we just attach that fault code to every signal that's
> >> >> > > > delivered until something overwrites or resets it, which means that
> >> >> > > > a signal that needs fault_code gets it, at the expense of attaching
> >> >> > > > it to a bunch of other random signals too.
> >> >> > > >
> >> >> > > >
> >> >> > > > Checking the signal number and si_code might help us to know what we
> >> >> > > > should be doing with fault_code. We need to have sure userspace can't
> >> >> > > > trick us with a non kernel generated signal here. It would also be
> >> >> > > > necessary to check how PTRACE_SETSIGINFO interacts with this.
> >> >> > >
> >> >> > > With these possible interactions in mind I think we should store the
> >> >> > > fault code and fault address in kernel_siginfo instead of
> >> >> > > thread_struct (and clear these fields when we receive a siginfo from
> >> >> > > userspace, i.e. in copy_siginfo_from_user which is used by
> >> >> > > ptrace(PTRACE_SETSIGINFO) among other places). That way, the
> >> >> > > information is clearly associated with the signal itself and not the
> >> >> > > thread, so we don't need to worry about our signal being delivered out
> >> >> > > of order.
> >> >> >
> >> >> > Hmm, I can't see a way to do that that isn't horribly invasive in the core
> >> >> > signal code. Can you?
> >> >
> >> > I think I've come up with a way that doesn't seem to be too invasive.
> >> > See patch #1 of the series that I'm about to send out.
> >> >
> >> >> > But generally, I agree: the per-thread handling of fault_address and
> >> >> > fault_code appears to be quite broken in the face of signal prioritisation
> >> >> > and signals that don't correspond directly to hardware trap. It would be
> >> >> > nice to have some tests for this...
> >> >> >
> >> >> > If we want to pile on more bodges, perhaps we could stash the signal number
> >> >> > to which the fault_{address,code} relate, and then check that at delivery
> >> >> > and clear on a match. I hate it.
> >> >>
> >> >> I agree with Daniel's suggestion in principle, but I was also concerned
> >> >> about whether it would be too invasive elsewhere.
> >> >>
> >> >> Question though: does the core code take special care to make sure that
> >> >> a force_sig cannot be outprioritised by a regular signal? If so,
> >> >> perhaps we get away with it. I ask this, because the same same issue
> >> >> may be hitting other arches otherwise.
> >> >
> >> > Not as far as I can tell. There does appear to be prioritisation for
> >> > synchronous signals [1] but as far as I can tell nothing to
> >> > distinguish one of these signals from one with the same signal number
> >> > sent from userspace (e.g. via kill(2)).
> >>
> >> The si_code will differ between signals generated between userspace
> >> and signals generated by the kernel.
> >>
> >> We do allow a little bit of ptrace and sending to yourself to spoof
> >> kernel generated signals, for reasons of debugging and process migration
> >> where an existing process needs to be reconstructed. But the defenses
> >> should be strong enough you can assume that we reliably distinguish
> >> between a signal from userspace and a signal from the kernel.
> >
> > So check for SIGBUS || SIGSEGV and one of the below si_codes, and only
> > add the context in that case? Seems fragile to me, but I suppose I
> > could live with it.
> >
> >> I don't fully follow what you are doing but this feels like the
> >> kind of case where a new si_code has been defined as well as additional
> >> fields in siginfo.
> >
> > There is no new si_code for this, the information will be exposed for
> > several existing si_code types (BUS_ADRERR, BUS_ADRALN, BUS_MCEERR_AR,
> > SEGV_ACCERR, SEGV_MAPERR), and possibly others in the future
> > (particularly SEGV_MTESERR, which is part of the proposed MTE patch
> > set). Note that we already have a union field for BUS_MCEERR_AR, and
> > we may want to expose it for the other si_codes that already have
> > union fields as well.
> >
> > That being said, taking a closer look at siginfo, I think we are in
> > luck and we might be able to make this work in a reasonable way by
> > reusing padding (see below).
> >
> >> In your patchset I really hate that you were going back to
> >> force_sig_info, and filling out struct siginfo by hand. That is an
> >> error prone pattern, and I have fixed enough bugs in the kernel to prove
> >> that.
> >
> > To be fair, most of the callers are in helper functions that take
> > explicit parameters similar to force_sig_fault et al, and the SIGILL
> > one could easily be made that way as well.
> >
> >> I take exception to the idea that including the full address might break
> >> userspace. That means typically means someone has been too lazy to look
> >> and see what userspace is doing. When that userspace that might break
> >> is the same userspace you are changing the kernel to serve that makes me
> >> nervous. AKA the userspace that cares about this signal and how it is
> >> represented in siginfo.
> >
> > It's not a matter of being lazy. This behaviour isn't just an accident
> > but has been explicitly documented for years (see the
> > tagged-pointers.rst file that I changed: "Non-zero tags are not
> > preserved when delivering signals."), so users can reasonably rely on
> > it. Furthermore we simply don't have visibility into the majority of
> > userspace. For example, there are a lot of closed source Android apps
> > out there, and who knows what signal handlers they're installing and
> > how they're making use of the si_addr field on e.g. SEGV_MAPERR. We
> > can't just change the documented semantics under their feet.
> >
> > It's also not the same userspace either. The userspace that's
> > initially going to be consuming the new fields is in a part of the
> > Android system that handles and reports crashes, and that's something
> > that we control unlike all the apps.
> >
> > Finally, the userspace may need to know whether the tag bits were
> > actually zero or whether they were just unavailable, otherwise
> > userspace could for example produce a misleading crash report. Simply
> > having the kernel set the top bits of si_addr wouldn't accomplish that
> > due to the kernel's previous behaviour, hence the mask to let
> > userspace know which bits are accurate.
> >
> >> A fix of one instance of SIGILL should not be included with a patch that
> >> does something else, and really should come before everything else if
> >> possible.
> >
> > Fair point. I can see if I can split that part out.
> >
> >> If this information really belongs in struct siginfo (as it sounds like)
> >> please actually put the information in siginfo, and let userspace look
> >> in siginfo to find it. struct siginfo is a union with plenty of space,
> >> and plenty of si_codes.
> >>
> >> If this applies to multiple cases then it might be trickier but please
> >> dig into the details, don't toss things into sigcontext just because
> >> you can't figure out a clean design for reporting this.
> >
> > If we wanted this in siginfo, one idea that I had was to revert commit
> > b68a68d3dcc15ebbf23cbe91af1abf57591bd96b and add unsigned char fields
> > _addr_top_byte and _addr_top_byte_mask in the padding between
> > _addr_lsb and the union (with comments on all the fields of course to
> > say when they are filled in). I think that would work since we are
> > already clearing padding in siginfo, one nice property of the new
> > fields is that the zero values are correct in the case where the
> > information isn't being exposed (so old kernels would already have the
> > correct behaviour). That would only work on certain architectures
> > (i.e. at least alignof(void*) >= 4) so I suppose it could have an
> > #ifdef __aarch64__ around it.
>
> Perhaps add a 4th padding member to the union inside of _sigfault, that
> adds something like 4 unsigned long's worth of data, and then have your
> fields after the union.
>
> Is it quite a bit of work to gather that information from the
> instructions that faulted? I am just checking that this work is really
> makes sense.
>
> What I really don't understand is how well this problem generalizes to
> other architectures to tell if this is something other people need to
> solve at some point as well.
The broad issue here is how arch-specific fault diagnostics make it into
the signal frame, and whether this is needed at all.
The address tag bits are one case, but the same basic mechanism is also
used to report the type of failed access (read versus write) for
SIGSEGV on arm64. (IIRC qemu relies on this for tracking page use /
dirtiness in userspace.)
Having a way to associate arch metadata of this sort with the
specific signal it relates to seems a good idea. That way, we're not
relying on internal details of the signal common code such as the
precise order signals get delivered in.
This concept is certainly applicable to other arches, but I don't know
the extent to which they actually depend on it.
Ideally, there would be a si_flags field to add simple arch_specific
attributes in, but there seems no backwards compatible way to add such a
thing for existing signals. (Or is there?)
Cheers
---Dave
_______________________________________________
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] iio: stm32-adc: remove usage of iio_priv_to_dev() helper
From: Ardelean, Alexandru @ 2020-05-26 13:05 UTC (permalink / raw)
To: linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, fabrice.gasnier@st.com,
linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Cc: olivier.moysan@st.com, mcoquelin.stm32@gmail.com,
jic23@kernel.org, alexandre.torgue@st.com
In-Reply-To: <447a0db3-0c20-859c-b5f2-7716c57a7e0e@st.com>
On Mon, 2020-05-25 at 18:27 +0200, Fabrice Gasnier wrote:
> [External]
>
> On 5/25/20 11:07 AM, Alexandru Ardelean wrote:
> > We may want to get rid of the iio_priv_to_dev() helper. The reason is
> > that
> > we will hide some of the members of the iio_dev structure (to prevent
> > drivers from accessing them directly), and that will also mean hiding
> > the
> > implementation of the iio_priv_to_dev() helper inside the IIO core.
> >
> > Hiding the implementation of iio_priv_to_dev() implies that some fast-
> > paths
> > may not be fast anymore, so a general idea is to try to get rid of the
> > iio_priv_to_dev() altogether.
> > The iio_priv() helper won't be affected by the rework, as the iio_dev
> > struct will keep a reference to the private information.
> >
> > For this driver, not using iio_priv_to_dev(), means reworking some
> > paths to
> > pass the iio device and using iio_priv() to access the private
> > information.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/iio/adc/stm32-adc.c | 108 +++++++++++++++++++-----------------
> > 1 file changed, 58 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index ae622ee6d08c..9428c5c22712 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -162,10 +162,10 @@ struct stm32_adc_cfg {
> > struct stm32_adc_trig_info *trigs;
> > bool clk_required;
> > bool has_vregready;
> > - int (*prepare)(struct stm32_adc *);
> > - void (*start_conv)(struct stm32_adc *, bool dma);
> > - void (*stop_conv)(struct stm32_adc *);
> > - void (*unprepare)(struct stm32_adc *);
> > + int (*prepare)(struct iio_dev *);
> > + void (*start_conv)(struct iio_dev *, bool dma);
> > + void (*stop_conv)(struct iio_dev *);
> > + void (*unprepare)(struct iio_dev *);
> > const unsigned int *smp_cycles;
> > };
> >
> > @@ -538,10 +538,11 @@ static void stm32_adc_set_res(struct stm32_adc
> > *adc)
> >
> > static int stm32_adc_hw_stop(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> >
> > if (adc->cfg->unprepare)
> > - adc->cfg->unprepare(adc);
> > + adc->cfg->unprepare(indio_dev);
> >
> > if (adc->clk)
> > clk_disable_unprepare(adc->clk);
> > @@ -551,7 +552,8 @@ static int stm32_adc_hw_stop(struct device *dev)
> >
> > static int stm32_adc_hw_start(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> >
> > if (adc->clk) {
> > @@ -563,7 +565,7 @@ static int stm32_adc_hw_start(struct device *dev)
> > stm32_adc_set_res(adc);
> >
> > if (adc->cfg->prepare) {
> > - ret = adc->cfg->prepare(adc);
> > + ret = adc->cfg->prepare(indio_dev);
> > if (ret)
> > goto err_clk_dis;
> > }
> > @@ -587,8 +589,10 @@ static int stm32_adc_hw_start(struct device *dev)
> > * conversions, in IIO buffer modes. Otherwise, use ADC interrupt with
> > direct
> > * DR read instead (e.g. read_raw, or triggered buffer mode without
> > DMA).
> > */
> > -static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
> > +static void stm32f4_adc_start_conv(struct iio_dev *indio_dev, bool
> > dma)
>
> Hi Alexandru,
>
> I've tested your patch. I've no objection, but found few build warnings
> (some of these routines have kernel-doc style).
>
> Building with W=1 makes warnings appear, like:
> drivers/iio/adc/stm32-adc.c:593: warning: Function parameter or member
> 'indio_dev' not described in 'stm32f4_adc_start_conv'
> drivers/iio/adc/stm32-adc.c:593: warning: Excess function parameter
> 'adc' description in 'stm32f4_adc_start_conv'
> ...
>
> Could you update routine's doc as well ?
Thanks.
Will send a V3 shortly.
The W=1 option looks interesting.
We might give this to interns/new-people to get them started.
>
> e.g. something like:
> - * @adc: stm32 adc instance
> + * @indio_dev: IIO device
>
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
> >
> > if (dma)
> > @@ -605,8 +609,10 @@ static void stm32f4_adc_start_conv(struct
> > stm32_adc *adc, bool dma)
> > stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_SWSTART);
> > }
> >
> > -static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
> > +static void stm32f4_adc_stop_conv(struct iio_dev *indio_dev)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_EXTEN_MASK);
> > stm32_adc_clr_bits(adc, STM32F4_ADC_SR, STM32F4_STRT);
> >
> > @@ -615,8 +621,9 @@ static void stm32f4_adc_stop_conv(struct stm32_adc
> > *adc)
> > STM32F4_ADON | STM32F4_DMA | STM32F4_DDS);
> > }
> >
> > -static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
> > +static void stm32h7_adc_start_conv(struct iio_dev *indio_dev, bool
> > dma)
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > enum stm32h7_adc_dmngt dmngt;
> > unsigned long flags;
> > u32 val;
> > @@ -635,9 +642,9 @@ static void stm32h7_adc_start_conv(struct stm32_adc
> > *adc, bool dma)
> > stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
> > }
> >
> > -static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
> > +static void stm32h7_adc_stop_conv(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -652,9 +659,9 @@ static void stm32h7_adc_stop_conv(struct stm32_adc
> > *adc)
> > stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32H7_DMNGT_MASK);
> > }
> >
> > -static int stm32h7_adc_exit_pwr_down(struct stm32_adc *adc)
> > +static int stm32h7_adc_exit_pwr_down(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -690,9 +697,9 @@ static void stm32h7_adc_enter_pwr_down(struct
> > stm32_adc *adc)
> > stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_DEEPPWD);
> > }
> >
> > -static int stm32h7_adc_enable(struct stm32_adc *adc)
> > +static int stm32h7_adc_enable(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -713,9 +720,9 @@ static int stm32h7_adc_enable(struct stm32_adc
> > *adc)
> > return ret;
> > }
> >
> > -static void stm32h7_adc_disable(struct stm32_adc *adc)
> > +static void stm32h7_adc_disable(struct iio_dev *indio_dev)
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -733,9 +740,9 @@ static void stm32h7_adc_disable(struct stm32_adc
> > *adc)
> > * @adc: stm32 adc instance
> > * Note: Must be called once ADC is enabled, so LINCALRDYW[1..6] are
> > writable
> > */
> > -static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_read_selfcalib(struct iio_dev *indio_dev)
>
> Same here.
>
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int i, ret;
> > u32 lincalrdyw_mask, val;
> >
> > @@ -777,9 +784,9 @@ static int stm32h7_adc_read_selfcalib(struct
> > stm32_adc *adc)
> > * @adc: stm32 adc instance
> > * Note: ADC must be enabled, with no on-going conversions.
> > */
> > -static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_restore_selfcalib(struct iio_dev *indio_dev)
>
> Same here.
>
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int i, ret;
> > u32 lincalrdyw_mask, val;
> >
> > @@ -850,9 +857,9 @@ static int stm32h7_adc_restore_selfcalib(struct
> > stm32_adc *adc)
> > * @adc: stm32 adc instance
> > * Note: Must be called once ADC is out of power down.
> > */
> > -static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> > +static int stm32h7_adc_selfcalib(struct iio_dev *indio_dev)
>
> Same here
>
> > {
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int ret;
> > u32 val;
> >
> > @@ -912,30 +919,31 @@ static int stm32h7_adc_selfcalib(struct stm32_adc
> > *adc)
> > * - Only one input is selected for single ended (e.g. 'vinp')
> > * - Two inputs are selected for differential channels (e.g. 'vinp' &
> > 'vinn')
> > */
> > -static int stm32h7_adc_prepare(struct stm32_adc *adc)
> > +static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
>
> Same here.
>
> With the comments updated, you can add my:
>
> Acked-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>
> Thanks for the patch,
> Fabrice
>
> > {
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > int calib, ret;
> >
> > - ret = stm32h7_adc_exit_pwr_down(adc);
> > + ret = stm32h7_adc_exit_pwr_down(indio_dev);
> > if (ret)
> > return ret;
> >
> > - ret = stm32h7_adc_selfcalib(adc);
> > + ret = stm32h7_adc_selfcalib(indio_dev);
> > if (ret < 0)
> > goto pwr_dwn;
> > calib = ret;
> >
> > stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
> >
> > - ret = stm32h7_adc_enable(adc);
> > + ret = stm32h7_adc_enable(indio_dev);
> > if (ret)
> > goto pwr_dwn;
> >
> > /* Either restore or read calibration result for future reference
> > */
> > if (calib)
> > - ret = stm32h7_adc_restore_selfcalib(adc);
> > + ret = stm32h7_adc_restore_selfcalib(indio_dev);
> > else
> > - ret = stm32h7_adc_read_selfcalib(adc);
> > + ret = stm32h7_adc_read_selfcalib(indio_dev);
> > if (ret)
> > goto disable;
> >
> > @@ -944,16 +952,18 @@ static int stm32h7_adc_prepare(struct stm32_adc
> > *adc)
> > return 0;
> >
> > disable:
> > - stm32h7_adc_disable(adc);
> > + stm32h7_adc_disable(indio_dev);
> > pwr_dwn:
> > stm32h7_adc_enter_pwr_down(adc);
> >
> > return ret;
> > }
> >
> > -static void stm32h7_adc_unprepare(struct stm32_adc *adc)
> > +static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> > {
> > - stm32h7_adc_disable(adc);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > +
> > + stm32h7_adc_disable(indio_dev);
> > stm32h7_adc_enter_pwr_down(adc);
> > }
> >
> > @@ -1160,7 +1170,7 @@ static int stm32_adc_single_conv(struct iio_dev
> > *indio_dev,
> >
> > stm32_adc_conv_irq_enable(adc);
> >
> > - adc->cfg->start_conv(adc, false);
> > + adc->cfg->start_conv(indio_dev, false);
> >
> > timeout = wait_for_completion_interruptible_timeout(
> > &adc->completion,
> > STM32_ADC_TIMEOUT);
> > @@ -1173,7 +1183,7 @@ static int stm32_adc_single_conv(struct iio_dev
> > *indio_dev,
> > ret = IIO_VAL_INT;
> > }
> >
> > - adc->cfg->stop_conv(adc);
> > + adc->cfg->stop_conv(indio_dev);
> >
> > stm32_adc_conv_irq_disable(adc);
> >
> > @@ -1227,8 +1237,8 @@ static int stm32_adc_read_raw(struct iio_dev
> > *indio_dev,
> >
> > static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
> > {
> > - struct stm32_adc *adc = data;
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = data;
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > const struct stm32_adc_regspec *regs = adc->cfg->regs;
> > u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
> >
> > @@ -1240,8 +1250,8 @@ static irqreturn_t stm32_adc_threaded_isr(int
> > irq, void *data)
> >
> > static irqreturn_t stm32_adc_isr(int irq, void *data)
> > {
> > - struct stm32_adc *adc = data;
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = data;
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> > const struct stm32_adc_regspec *regs = adc->cfg->regs;
> > u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
> >
> > @@ -1514,7 +1524,7 @@ static int __stm32_adc_buffer_postenable(struct
> > iio_dev *indio_dev)
> > if (!adc->dma_chan)
> > stm32_adc_conv_irq_enable(adc);
> >
> > - adc->cfg->start_conv(adc, !!adc->dma_chan);
> > + adc->cfg->start_conv(indio_dev, !!adc->dma_chan);
> >
> > return 0;
> >
> > @@ -1547,7 +1557,7 @@ static void __stm32_adc_buffer_predisable(struct
> > iio_dev *indio_dev)
> > struct stm32_adc *adc = iio_priv(indio_dev);
> > struct device *dev = indio_dev->dev.parent;
> >
> > - adc->cfg->stop_conv(adc);
> > + adc->cfg->stop_conv(indio_dev);
> > if (!adc->dma_chan)
> > stm32_adc_conv_irq_disable(adc);
> >
> > @@ -1891,7 +1901,7 @@ static int stm32_adc_probe(struct platform_device
> > *pdev)
> > indio_dev->info = &stm32_adc_iio_info;
> > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
> >
> > - platform_set_drvdata(pdev, adc);
> > + platform_set_drvdata(pdev, indio_dev);
> >
> > ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset);
> > if (ret != 0) {
> > @@ -1905,7 +1915,7 @@ static int stm32_adc_probe(struct platform_device
> > *pdev)
> >
> > ret = devm_request_threaded_irq(&pdev->dev, adc->irq,
> > stm32_adc_isr,
> > stm32_adc_threaded_isr,
> > - 0, pdev->name, adc);
> > + 0, pdev->name, indio_dev);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to request IRQ\n");
> > return ret;
> > @@ -1989,8 +1999,8 @@ static int stm32_adc_probe(struct platform_device
> > *pdev)
> >
> > static int stm32_adc_remove(struct platform_device *pdev)
> > {
> > - struct stm32_adc *adc = platform_get_drvdata(pdev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > + struct stm32_adc *adc = iio_priv(indio_dev);
> >
> > pm_runtime_get_sync(&pdev->dev);
> > iio_device_unregister(indio_dev);
> > @@ -2012,8 +2022,7 @@ static int stm32_adc_remove(struct
> > platform_device *pdev)
> > #if defined(CONFIG_PM_SLEEP)
> > static int stm32_adc_suspend(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >
> > if (iio_buffer_enabled(indio_dev))
> > __stm32_adc_buffer_predisable(indio_dev);
> > @@ -2023,8 +2032,7 @@ static int stm32_adc_suspend(struct device *dev)
> >
> > static int stm32_adc_resume(struct device *dev)
> > {
> > - struct stm32_adc *adc = dev_get_drvdata(dev);
> > - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > int ret;
> >
> > ret = pm_runtime_force_resume(dev);
> >
_______________________________________________
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] ACPI/IORT: Remove the unused __get_pci_rid()
From: Lorenzo Pieralisi @ 2020-05-26 13:07 UTC (permalink / raw)
To: Zenghui Yu, will
Cc: guohanjun, rjw, linux-kernel, linux-acpi, sudeep.holla,
wanghaibin.wang, linux-arm-kernel, lenb
In-Reply-To: <20200509093430.1983-1-yuzenghui@huawei.com>
On Sat, May 09, 2020 at 05:34:30PM +0800, Zenghui Yu wrote:
> Since commit bc8648d49a95 ("ACPI/IORT: Handle PCI aliases properly for
> IOMMUs"), __get_pci_rid() has become actually unused and can be removed.
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> drivers/acpi/arm64/iort.c | 9 ---------
> 1 file changed, 9 deletions(-)
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
If we miss v5.8 (which I think it is likely, even though this patch is
just removing dead code so it is safe at this stage) I will resend it
for v5.9 - not a problem.
Lorenzo
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 7d04424189df..ec94dbb60c7a 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -789,15 +789,6 @@ void acpi_configure_pmsi_domain(struct device *dev)
> dev_set_msi_domain(dev, msi_domain);
> }
>
> -static int __maybe_unused __get_pci_rid(struct pci_dev *pdev, u16 alias,
> - void *data)
> -{
> - u32 *rid = data;
> -
> - *rid = alias;
> - return 0;
> -}
> -
> #ifdef CONFIG_IOMMU_API
> static struct acpi_iort_node *iort_get_msi_resv_iommu(struct device *dev)
> {
> --
> 2.19.1
>
_______________________________________________
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 RFCv2 0/9] kvm/arm64: Support Async Page Fault
From: Mark Rutland @ 2020-05-26 13:09 UTC (permalink / raw)
To: Gavin Shan
Cc: aarcange, drjones, suzuki.poulose, catalin.marinas, linux-kernel,
eric.auger, james.morse, shan.gavin, maz, will, kvmarm,
linux-arm-kernel
In-Reply-To: <20200508032919.52147-1-gshan@redhat.com>
Hi Gavin,
At a high-level I'm rather fearful of this series. I can see many ways
that this can break, and I can also see that even if/when we get things
into a working state, constant vigilance will be requried for any
changes to the entry code.
I'm not keen on injecting non-architectural exceptions in this way, and
I'm also not keen on how deep the PV hooks are injected currently (e.g.
in the ret_to_user path).
I see a few patches have preparator cleanup that I think would be
worthwhile regardless of this series; if you could factor those out and
send them on their own it would get that out of the way and make it
easier to review the series itself. Similarly, there's some duplication
of code from arch/x86 which I think can be factored out to virt/kvm
instead as preparatory work.
Generally, I also think that you need to spend some time on commit
messages and/or documentation to better explain the concepts and
expected usage. I had to reverse-engineer the series by reviewing it in
entirety before I had an idea as to how basic parts of it strung
together, and a more thorough conceptual explanation would make it much
easier to critique the approach rather than the individual patches.
On Fri, May 08, 2020 at 01:29:10PM +1000, Gavin Shan wrote:
> Testing
> =======
> The tests are carried on the following machine. A guest with single vCPU
> and 4GB memory is started. Also, the QEMU process is put into memory cgroup
> (v1) whose memory limit is set to 2GB. In the guest, there are two threads,
> which are memory bound and CPU bound separately. The memory bound thread
> allocates all available memory, accesses and them free them. The CPU bound
> thread simply executes block of "nop".
I appreciate this is a microbenchmark, but that sounds far from
realistic.
Is there a specitic real workload that's expected to be representative
of?
Can you run tests with a real workload? For example, a kernel build
inside the VM?
> The test is carried out for 5 time
> continuously and the average number (per minute) of executed blocks in the
> CPU bound thread is taken as indicator of improvement.
>
> Vendor: GIGABYTE CPU: 224 x Cavium ThunderX2(R) CPU CN9975 v2.2 @ 2.0GHz
> Memory: 32GB Disk: Fusion-MPT SAS-3 (PCIe3.0 x8)
>
> Without-APF: 7029030180/minute = avg(7559625120 5962155840 7823208540
> 7629633480 6170527920)
> With-APF: 8286827472/minute = avg(8464584540 8177073360 8262723180
> 8095084020 8434672260)
> Outcome: +17.8%
>
> Another test case is to measure the time consumed by the application, but
> with the CPU-bound thread disabled.
>
> Without-APF: 40.3s = avg(40.6 39.3 39.2 41.6 41.2)
> With-APF: 40.8s = avg(40.6 41.1 40.9 41.0 40.7)
> Outcome: +1.2%
So this is pure overhead in that case?
I think we need to see a real workload that this benefits. As it stands
it seems that this is a lot of complexity to game a synthetic benchmark.
Thanks,
Mark.
> I also have some code in the host to capture the number of async page faults,
> time used to do swapin and its maximal/minimal values when async page fault
> is enabled. During the test, the CPU-bound thread is disabled. There is about
> 30% of the time used to do swapin.
>
> Number of async page fault: 7555 times
> Total time used by application: 42.2 seconds
> Total time used by swapin: 12.7 seconds (30%)
> Minimal swapin time: 36.2 us
> Maximal swapin time: 55.7 ms
>
> Changelog
> =========
> RFCv1 -> RFCv2
> * Rebase to 5.7.rc3
> * Performance data (Marc Zyngier)
> * Replace IMPDEF system register with KVM vendor specific hypercall (Mark Rutland)
> * Based on Will's KVM vendor hypercall probe mechanism (Will Deacon)
> * Don't use IMPDEF DFSC (0x43). Async page fault reason is conveyed
> by the control block (Mark Rutland)
> * Delayed wakeup mechanism in guest kernel (Gavin Shan)
> * Stability improvement in the guest kernel: delayed wakeup mechanism,
> external abort disallowed region, lazily clear async page fault,
> disabled interrupt on acquiring the head's lock and so on (Gavin Shan)
> * Stability improvement in the host kernel: serialized async page
> faults etc. (Gavin Shan)
> * Performance improvement in guest kernel: percpu sleeper head (Gavin Shan)
>
> Gavin Shan (7):
> kvm/arm64: Rename kvm_vcpu_get_hsr() to kvm_vcpu_get_esr()
> kvm/arm64: Detach ESR operator from vCPU struct
> kvm/arm64: Replace hsr with esr
> kvm/arm64: Export kvm_handle_user_mem_abort() with prefault mode
> kvm/arm64: Support async page fault
> kernel/sched: Add cpu_rq_is_locked()
> arm64: Support async page fault
>
> Will Deacon (2):
> arm64: Probe for the presence of KVM hypervisor services during boot
> arm/arm64: KVM: Advertise KVM UID to guests via SMCCC
>
> arch/arm64/Kconfig | 11 +
> arch/arm64/include/asm/exception.h | 3 +
> arch/arm64/include/asm/hypervisor.h | 11 +
> arch/arm64/include/asm/kvm_emulate.h | 83 +++--
> arch/arm64/include/asm/kvm_host.h | 47 +++
> arch/arm64/include/asm/kvm_para.h | 40 +++
> arch/arm64/include/uapi/asm/Kbuild | 2 -
> arch/arm64/include/uapi/asm/kvm_para.h | 22 ++
> arch/arm64/kernel/entry.S | 33 ++
> arch/arm64/kernel/process.c | 4 +
> arch/arm64/kernel/setup.c | 35 ++
> arch/arm64/kvm/Kconfig | 1 +
> arch/arm64/kvm/Makefile | 2 +
> arch/arm64/kvm/handle_exit.c | 48 +--
> arch/arm64/kvm/hyp/switch.c | 33 +-
> arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 7 +-
> arch/arm64/kvm/inject_fault.c | 4 +-
> arch/arm64/kvm/sys_regs.c | 38 +-
> arch/arm64/mm/fault.c | 434 +++++++++++++++++++++++
> include/linux/arm-smccc.h | 32 ++
> include/linux/sched.h | 1 +
> kernel/sched/core.c | 8 +
> virt/kvm/arm/arm.c | 40 ++-
> virt/kvm/arm/async_pf.c | 335 +++++++++++++++++
> virt/kvm/arm/hyp/aarch32.c | 4 +-
> virt/kvm/arm/hyp/vgic-v3-sr.c | 7 +-
> virt/kvm/arm/hypercalls.c | 37 +-
> virt/kvm/arm/mmio.c | 27 +-
> virt/kvm/arm/mmu.c | 69 +++-
> 29 files changed, 1264 insertions(+), 154 deletions(-)
> create mode 100644 arch/arm64/include/asm/kvm_para.h
> create mode 100644 arch/arm64/include/uapi/asm/kvm_para.h
> create mode 100644 virt/kvm/arm/async_pf.c
>
> --
> 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
* [GIT PULL] tee subsystem pin_user_pages for v5.8
From: Jens Wiklander @ 2020-05-26 13:12 UTC (permalink / raw)
To: arm, soc; +Cc: tee-dev, John Hubbard, Linux Kernel Mailing List, Linux ARM
Hello arm-soc maintainers,
Please pull this small patch converting the tee subsystem to use
pin_user_pages() instead of get_user_pages().
Thanks,
Jens
The following changes since commit ae83d0b416db002fe95601e7f97f64b59514d936:
Linux 5.7-rc2 (2020-04-19 14:35:30 -0700)
are available in the Git repository at:
git://git.linaro.org:/people/jens.wiklander/linux-tee.git tags/tee-pin-user-pages-for-5.8
for you to fetch changes up to 37f6b4d5f47b600ec4ab6682c005a44a1bfca530:
tee: convert get_user_pages() --> pin_user_pages() (2020-05-26 10:42:41 +0200)
----------------------------------------------------------------
Converts tee subsystem to use pin_user_pages() instead of get_user_pages()
----------------------------------------------------------------
John Hubbard (1):
tee: convert get_user_pages() --> pin_user_pages()
drivers/tee/tee_shm.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] ARM: add arch/arm/Kbuild
From: Masahiro Yamada @ 2020-05-26 13:20 UTC (permalink / raw)
To: patches
Cc: Arnd Bergmann, Christian Lamparter, Marc Zyngier, Masahiro Yamada,
Russell King, linux-kernel, Nathan Huckleberry, Nathan Chancellor,
Will Deacon, Ard Biesheuvel, linux-arm-kernel
Use the standard obj-y form to specify the sub-directories under
arch/arm/. No functional change intended.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
KernelVersion: v5.7-rc1
arch/arm/Kbuild | 11 +++++++++++
arch/arm/Makefile | 12 +-----------
2 files changed, 12 insertions(+), 11 deletions(-)
create mode 100644 arch/arm/Kbuild
diff --git a/arch/arm/Kbuild b/arch/arm/Kbuild
new file mode 100644
index 000000000000..5208f7061524
--- /dev/null
+++ b/arch/arm/Kbuild
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_FPE_NWFPE) += nwfpe/
+# Put arch/arm/fastfpe/ to use this.
+obj-$(CONFIG_FPE_FASTFPE) += $(patsubst $(srctree)/$(src)/%,%,$(wildcard $(srctree)/$(src)/fastfpe/))
+obj-$(CONFIG_VFP) += vfp/
+obj-$(CONFIG_XEN) += xen/
+obj-$(CONFIG_VDSO) += vdso/
+obj-y += kernel/ mm/ common/
+obj-y += probes/
+obj-y += net/
+obj-y += crypto/
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index cd28211f1418..5f617fb1782e 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -272,18 +272,8 @@ endif
export TEXT_OFFSET GZFLAGS MMUEXT
-core-$(CONFIG_FPE_NWFPE) += arch/arm/nwfpe/
-# Put arch/arm/fastfpe/ to use this.
-core-$(CONFIG_FPE_FASTFPE) += $(patsubst $(srctree)/%,%,$(wildcard $(srctree)/arch/arm/fastfpe/))
-core-$(CONFIG_VFP) += arch/arm/vfp/
-core-$(CONFIG_XEN) += arch/arm/xen/
-core-$(CONFIG_VDSO) += arch/arm/vdso/
-
+core-y += arch/arm/
# If we have a machine-specific directory, then include it in the build.
-core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/
-core-y += arch/arm/probes/
-core-y += arch/arm/net/
-core-y += arch/arm/crypto/
core-y += $(machdirs) $(platdirs)
# For cleaning
--
2.25.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 17/21] drm/stm: Use GEM CMA object functions
From: Philippe CORNU @ 2020-05-26 13:22 UTC (permalink / raw)
To: Thomas Zimmermann, abrodkin@synopsys.com, airlied@linux.ie,
daniel@ffwll.ch, james.qian.wang@arm.com, liviu.dudau@arm.com,
mihail.atanassov@arm.com, brian.starkey@arm.com, joel@jms.id.au,
andrew@aj.id.au, sam@ravnborg.org, bbrezillon@kernel.org,
nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
ludovic.desroches@microchip.com,
maarten.lankhorst@linux.intel.com, mripard@kernel.org,
stefan@agner.ch, alison.wang@nxp.com, xinliang.liu@linaro.org,
zourongrong@gmail.com, john.stultz@linaro.org,
kong.kongxinwei@hisilicon.com, puck.chen@hisilicon.com,
p.zabel@pengutronix.de, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-imx@nxp.com, paul@crapouillou.net, linus.walleij@linaro.org,
narmstrong@baylibre.com, khilman@baylibre.com, marex@denx.de,
laurent.pinchart@ideasonboard.com,
kieran.bingham+renesas@ideasonboard.com,
benjamin.gaignard@linaro.org, Vincent ABRIOU, Yannick FERTRE,
mcoquelin.stm32@gmail.com, Alexandre TORGUE, wens@csie.org,
jsarha@ti.com, tomi.valkeinen@ti.com, noralf@tronnes.org
Cc: linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org, dri-devel@lists.freedesktop.org
In-Reply-To: <20200522135246.10134-18-tzimmermann@suse.de>
On 5/22/20 3:52 PM, Thomas Zimmermann wrote:
> The stm driver uses the default implementation for CMA functions; except
> for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets
> these defaults and .dumb_create in struct drm_driver. All remaining
> operations are provided by CMA GEM object functions.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/stm/drv.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 0f85dd86cafa7..d6238c71fabf6 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -62,16 +62,7 @@ static struct drm_driver drv_driver = {
> .minor = 0,
> .patchlevel = 0,
> .fops = &drv_driver_fops,
> - .dumb_create = stm_gem_cma_dumb_create,
> - .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> - .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_free_object_unlocked = drm_gem_cma_free_object,
> - .gem_vm_ops = &drm_gem_cma_vm_ops,
> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> - .gem_prime_vmap = drm_gem_cma_prime_vmap,
> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> - .gem_prime_mmap = drm_gem_cma_prime_mmap,
> + __DRM_GEM_CMA_DRIVER_OPS(stm_gem_cma_dumb_create),
> };
>
> static int drv_load(struct drm_device *ddev)
>
Hi Thomas,
Thank you for the patch.
Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
Philippe :-)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Re: [PATCH] i2c: stm32f7: Fix runtime PM imbalance in stm32f7_i2c_reg_slave
From: Alain Volmat @ 2020-05-26 13:25 UTC (permalink / raw)
To: dinghao.liu
Cc: Alexandre Torgue, kjlu, linux-kernel, Pierre-Yves MORDRET,
linux-i2c, Maxime Coquelin, linux-stm32, linux-arm-kernel
In-Reply-To: <7548c995.d205f.1725111d7c4.Coremail.dinghao.liu@zju.edu.cn>
On Tue, May 26, 2020 at 09:00:23PM +0800, dinghao.liu@zju.edu.cn wrote:
>
> > Overall, there are several other calls to pm_runtime_get_sync within this
> > driver, would you like to fix them all at once ?
> >
>
> Sure, I will send a new patch to merge them all.
Thanks, you might want to add a Fixes: tag in your commit message as well.
>
> > On Thu, May 21, 2020 at 03:05:07PM +0800, Dinghao Liu wrote:
> > > pm_runtime_get_sync() increments the runtime PM usage counter even
> > > the call returns an error code. Thus a pairing decrement is needed
> > > on the error handling path to keep the counter balanced.
> > >
> > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > > ---
> > > drivers/i2c/busses/i2c-stm32f7.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> > > index 330ffed011e0..602cf35649c8 100644
> > > --- a/drivers/i2c/busses/i2c-stm32f7.c
> > > +++ b/drivers/i2c/busses/i2c-stm32f7.c
> > > @@ -1767,8 +1767,10 @@ static int stm32f7_i2c_reg_slave(struct i2c_client *slave)
> > > return ret;
> > >
> > > ret = pm_runtime_get_sync(dev);
> > > - if (ret < 0)
> > > + if (ret < 0) {
> > > + pm_runtime_put_autosuspend(dev);
> >
> > Considering that if we fail here there is a very good chance that this is due
> > to the resume failing, pm_runtime_put_noidle would probably make more sense
> > since pm_runtime_put_autosuspend will most probably fail as well.
> >
>
> Agree. Thank you for your advice!
>
> Regards.
> Dinghao
_______________________________________________
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 v4 1/2] dt-bindings: PCI: Add UniPhier PCIe endpoint controller description
From: Lorenzo Pieralisi @ 2020-05-26 13:34 UTC (permalink / raw)
To: Kunihiko Hayashi, robh+dt
Cc: devicetree, Masami Hiramatsu, Jassi Brar, linux-pci, linux-kernel,
Masahiro Yamada, Bjorn Helgaas, linux-arm-kernel
In-Reply-To: <1589457801-12796-2-git-send-email-hayashi.kunihiko@socionext.com>
On Thu, May 14, 2020 at 09:03:20PM +0900, Kunihiko Hayashi wrote:
> Add DT bindings for PCIe controller implemented in UniPhier SoCs
> when configured in endpoint mode. This controller is based on
> the DesignWare PCIe core.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> .../bindings/pci/socionext,uniphier-pcie-ep.yaml | 92 ++++++++++++++++++++++
> MAINTAINERS | 2 +-
> 2 files changed, 93 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
Hi Rob,
are you OK with this patch ? Please let me know, I'd like to pull
the series, thanks.
Lorenzo
> diff --git a/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
> new file mode 100644
> index 0000000..f0558b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/socionext,uniphier-pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Socionext UniPhier PCIe endpoint controller
> +
> +description: |
> + UniPhier PCIe endpoint controller is based on the Synopsys DesignWare
> + PCI core. It shares common features with the PCIe DesignWare core and
> + inherits common properties defined in
> + Documentation/devicetree/bindings/pci/designware-pcie.txt.
> +
> +maintainers:
> + - Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> +
> +allOf:
> + - $ref: "pci-ep.yaml#"
> +
> +properties:
> + compatible:
> + const: socionext,uniphier-pro5-pcie-ep
> +
> + reg:
> + maxItems: 4
> +
> + reg-names:
> + items:
> + - const: dbi
> + - const: dbi2
> + - const: link
> + - const: addr_space
> +
> + clocks:
> + maxItems: 2
> +
> + clock-names:
> + items:
> + - const: gio
> + - const: link
> +
> + resets:
> + maxItems: 2
> +
> + reset-names:
> + items:
> + - const: gio
> + - const: link
> +
> + num-ib-windows:
> + const: 16
> +
> + num-ob-windows:
> + const: 16
> +
> + num-lanes: true
> +
> + phys:
> + maxItems: 1
> +
> + phy-names:
> + const: pcie-phy
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - clocks
> + - clock-names
> + - resets
> + - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + pcie_ep: pcie-ep@66000000 {
> + compatible = "socionext,uniphier-pro5-pcie-ep";
> + reg-names = "dbi", "dbi2", "link", "addr_space";
> + reg = <0x66000000 0x1000>, <0x66001000 0x1000>,
> + <0x66010000 0x10000>, <0x67000000 0x400000>;
> + clock-names = "gio", "link";
> + clocks = <&sys_clk 12>, <&sys_clk 24>;
> + reset-names = "gio", "link";
> + resets = <&sys_rst 12>, <&sys_rst 24>;
> + num-ib-windows = <16>;
> + num-ob-windows = <16>;
> + num-lanes = <4>;
> + phy-names = "pcie-phy";
> + phys = <&pcie_phy>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92657a1..7f26748 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13211,7 +13211,7 @@ PCIE DRIVER FOR SOCIONEXT UNIPHIER
> M: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> L: linux-pci@vger.kernel.org
> S: Maintained
> -F: Documentation/devicetree/bindings/pci/uniphier-pcie.txt
> +F: Documentation/devicetree/bindings/pci/uniphier-pcie*
> F: drivers/pci/controller/dwc/pcie-uniphier.c
>
> PCIE DRIVER FOR ST SPEAR13XX
> --
> 2.7.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH V2] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()
From: Anshuman Khandual @ 2020-05-26 13:39 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, Suzuki K Poulose, Catalin Marinas,
Anshuman Khandual, linux-kernel, Mark Brown, Will Deacon
There is no way to proceed when requested register could not be searched in
arm64_ftr_reg[]. Requesting for a non present register would be an error as
well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg()
rather than checking for return value and doing a BUG_ON() instead in some
individual callers. But there are also caller instances that dont error out
when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for
such cases.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Changes in V2:
- Added get_arm64_ftr_reg_nowarn() per Will
- read_sanitised_ftr_reg() returns 0 when register search fails per Catalin
Changes in V1: (https://patchwork.kernel.org/patch/11559083/)
arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index bc5048f152c1..f4555b9d145c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
}
/*
- * get_arm64_ftr_reg - Lookup a feature register entry using its
- * sys_reg() encoding. With the array arm64_ftr_regs sorted in the
- * ascending order of sys_id , we use binary search to find a matching
+ * get_arm64_ftr_reg_nowarn - Looks up a feature register entry using
+ * its sys_reg() encoding. With the array arm64_ftr_regs sorted in the
+ * ascending order of sys_id, we use binary search to find a matching
* entry.
*
* returns - Upon success, matching ftr_reg entry for id.
* - NULL on failure. It is upto the caller to decide
* the impact of a failure.
*/
-static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
+static struct arm64_ftr_reg *get_arm64_ftr_reg_nowarn(u32 sys_id)
{
const struct __ftr_reg_entry *ret;
@@ -571,6 +571,28 @@ static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
return NULL;
}
+/*
+ * get_arm64_ftr_reg - Looks up a feature register entry using
+ * its sys_reg() encoding. This calls get_arm64_ftr_reg_nowarn().
+ *
+ * returns - Upon success, matching ftr_reg entry for id.
+ * - NULL on failure but with an WARN_ON().
+ */
+static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
+{
+ struct arm64_ftr_reg *reg;
+
+ reg = get_arm64_ftr_reg_nowarn(sys_id);
+
+ /*
+ * Can not really proceed when the search fails here.
+ * Requesting for a non existent register search will
+ * be an error. Warn but let it continue for now.
+ */
+ WARN_ON(!reg);
+ return reg;
+}
+
static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
s64 ftr_val)
{
@@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
const struct arm64_ftr_bits *ftrp;
struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
- BUG_ON(!reg);
-
for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
u64 ftr_mask = arm64_ftr_mask(ftrp);
s64 ftr_new = arm64_ftr_value(ftrp, new);
@@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
{
struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
- BUG_ON(!regp);
update_cpu_ftr_reg(regp, val);
if ((boot & regp->strict_mask) == (val & regp->strict_mask))
return 0;
@@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
const struct arm64_ftr_bits *ftrp;
struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
- if (WARN_ON(!regp))
- return;
-
for (ftrp = regp->ftr_bits; ftrp->width; ftrp++) {
if (ftrp->shift == field) {
regp->strict_mask &= ~arm64_ftr_mask(ftrp);
@@ -961,8 +977,8 @@ u64 read_sanitised_ftr_reg(u32 id)
{
struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);
- /* We shouldn't get a request for an unsupported register */
- BUG_ON(!regp);
+ if (!regp)
+ return 0;
return regp->sys_val;
}
@@ -2565,7 +2581,7 @@ static int emulate_sys_reg(u32 id, u64 *valp)
if (sys_reg_CRm(id) == 0)
return emulate_id_reg(id, valp);
- regp = get_arm64_ftr_reg(id);
+ regp = get_arm64_ftr_reg_nowarn(id);
if (regp)
*valp = arm64_ftr_reg_user_value(regp);
else
--
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
* [PATCH v3] iio: stm32-adc: remove usage of iio_priv_to_dev() helper
From: Alexandru Ardelean @ 2020-05-26 13:44 UTC (permalink / raw)
To: linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
Cc: olivier.moysan, alexandre.torgue, mcoquelin.stm32,
Alexandru Ardelean, fabrice.gasnier, jic23
In-Reply-To: <20200525090720.72696-1-alexandru.ardelean@analog.com>
We may want to get rid of the iio_priv_to_dev() helper. The reason is that
we will hide some of the members of the iio_dev structure (to prevent
drivers from accessing them directly), and that will also mean hiding the
implementation of the iio_priv_to_dev() helper inside the IIO core.
Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
may not be fast anymore, so a general idea is to try to get rid of the
iio_priv_to_dev() altogether.
The iio_priv() helper won't be affected by the rework, as the iio_dev
struct will keep a reference to the private information.
For this driver, not using iio_priv_to_dev(), means reworking some paths to
pass the iio device and using iio_priv() to access the private information.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
Changelog v2 -> v3:
- update doc-strings; warnings show-up during build with W=1 arg
Changelog v1 -> v2:
- converted to pass reference to IIO device in function hooks (vs
reference
to adc private data)
drivers/iio/adc/stm32-adc.c | 118 +++++++++++++++++++-----------------
1 file changed, 63 insertions(+), 55 deletions(-)
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index ae622ee6d08c..1dd97ec5571c 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -162,10 +162,10 @@ struct stm32_adc_cfg {
struct stm32_adc_trig_info *trigs;
bool clk_required;
bool has_vregready;
- int (*prepare)(struct stm32_adc *);
- void (*start_conv)(struct stm32_adc *, bool dma);
- void (*stop_conv)(struct stm32_adc *);
- void (*unprepare)(struct stm32_adc *);
+ int (*prepare)(struct iio_dev *);
+ void (*start_conv)(struct iio_dev *, bool dma);
+ void (*stop_conv)(struct iio_dev *);
+ void (*unprepare)(struct iio_dev *);
const unsigned int *smp_cycles;
};
@@ -538,10 +538,11 @@ static void stm32_adc_set_res(struct stm32_adc *adc)
static int stm32_adc_hw_stop(struct device *dev)
{
- struct stm32_adc *adc = dev_get_drvdata(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct stm32_adc *adc = iio_priv(indio_dev);
if (adc->cfg->unprepare)
- adc->cfg->unprepare(adc);
+ adc->cfg->unprepare(indio_dev);
if (adc->clk)
clk_disable_unprepare(adc->clk);
@@ -551,7 +552,8 @@ static int stm32_adc_hw_stop(struct device *dev)
static int stm32_adc_hw_start(struct device *dev)
{
- struct stm32_adc *adc = dev_get_drvdata(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
if (adc->clk) {
@@ -563,7 +565,7 @@ static int stm32_adc_hw_start(struct device *dev)
stm32_adc_set_res(adc);
if (adc->cfg->prepare) {
- ret = adc->cfg->prepare(adc);
+ ret = adc->cfg->prepare(indio_dev);
if (ret)
goto err_clk_dis;
}
@@ -579,7 +581,7 @@ static int stm32_adc_hw_start(struct device *dev)
/**
* stm32f4_adc_start_conv() - Start conversions for regular channels.
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device instance
* @dma: use dma to transfer conversion result
*
* Start conversions for regular channels.
@@ -587,8 +589,10 @@ static int stm32_adc_hw_start(struct device *dev)
* conversions, in IIO buffer modes. Otherwise, use ADC interrupt with direct
* DR read instead (e.g. read_raw, or triggered buffer mode without DMA).
*/
-static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
+static void stm32f4_adc_start_conv(struct iio_dev *indio_dev, bool dma)
{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
if (dma)
@@ -605,8 +609,10 @@ static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_SWSTART);
}
-static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
+static void stm32f4_adc_stop_conv(struct iio_dev *indio_dev)
{
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_EXTEN_MASK);
stm32_adc_clr_bits(adc, STM32F4_ADC_SR, STM32F4_STRT);
@@ -615,8 +621,9 @@ static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
STM32F4_ADON | STM32F4_DMA | STM32F4_DDS);
}
-static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
+static void stm32h7_adc_start_conv(struct iio_dev *indio_dev, bool dma)
{
+ struct stm32_adc *adc = iio_priv(indio_dev);
enum stm32h7_adc_dmngt dmngt;
unsigned long flags;
u32 val;
@@ -635,9 +642,9 @@ static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
}
-static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
+static void stm32h7_adc_stop_conv(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
u32 val;
@@ -652,9 +659,9 @@ static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32H7_DMNGT_MASK);
}
-static int stm32h7_adc_exit_pwr_down(struct stm32_adc *adc)
+static int stm32h7_adc_exit_pwr_down(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
u32 val;
@@ -690,9 +697,9 @@ static void stm32h7_adc_enter_pwr_down(struct stm32_adc *adc)
stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_DEEPPWD);
}
-static int stm32h7_adc_enable(struct stm32_adc *adc)
+static int stm32h7_adc_enable(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
u32 val;
@@ -713,9 +720,9 @@ static int stm32h7_adc_enable(struct stm32_adc *adc)
return ret;
}
-static void stm32h7_adc_disable(struct stm32_adc *adc)
+static void stm32h7_adc_disable(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
u32 val;
@@ -730,12 +737,12 @@ static void stm32h7_adc_disable(struct stm32_adc *adc)
/**
* stm32h7_adc_read_selfcalib() - read calibration shadow regs, save result
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device instance
* Note: Must be called once ADC is enabled, so LINCALRDYW[1..6] are writable
*/
-static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
+static int stm32h7_adc_read_selfcalib(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int i, ret;
u32 lincalrdyw_mask, val;
@@ -774,12 +781,12 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
/**
* stm32h7_adc_restore_selfcalib() - Restore saved self-calibration result
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device instance
* Note: ADC must be enabled, with no on-going conversions.
*/
-static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
+static int stm32h7_adc_restore_selfcalib(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int i, ret;
u32 lincalrdyw_mask, val;
@@ -847,12 +854,12 @@ static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
/**
* stm32h7_adc_selfcalib() - Procedure to calibrate ADC
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device instance
* Note: Must be called once ADC is out of power down.
*/
-static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
+static int stm32h7_adc_selfcalib(struct iio_dev *indio_dev)
{
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
int ret;
u32 val;
@@ -903,7 +910,7 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
/**
* stm32h7_adc_prepare() - Leave power down mode to enable ADC.
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device instance
* Leave power down mode.
* Configure channels as single ended or differential before enabling ADC.
* Enable ADC.
@@ -912,30 +919,31 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
* - Only one input is selected for single ended (e.g. 'vinp')
* - Two inputs are selected for differential channels (e.g. 'vinp' & 'vinn')
*/
-static int stm32h7_adc_prepare(struct stm32_adc *adc)
+static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
{
+ struct stm32_adc *adc = iio_priv(indio_dev);
int calib, ret;
- ret = stm32h7_adc_exit_pwr_down(adc);
+ ret = stm32h7_adc_exit_pwr_down(indio_dev);
if (ret)
return ret;
- ret = stm32h7_adc_selfcalib(adc);
+ ret = stm32h7_adc_selfcalib(indio_dev);
if (ret < 0)
goto pwr_dwn;
calib = ret;
stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
- ret = stm32h7_adc_enable(adc);
+ ret = stm32h7_adc_enable(indio_dev);
if (ret)
goto pwr_dwn;
/* Either restore or read calibration result for future reference */
if (calib)
- ret = stm32h7_adc_restore_selfcalib(adc);
+ ret = stm32h7_adc_restore_selfcalib(indio_dev);
else
- ret = stm32h7_adc_read_selfcalib(adc);
+ ret = stm32h7_adc_read_selfcalib(indio_dev);
if (ret)
goto disable;
@@ -944,16 +952,18 @@ static int stm32h7_adc_prepare(struct stm32_adc *adc)
return 0;
disable:
- stm32h7_adc_disable(adc);
+ stm32h7_adc_disable(indio_dev);
pwr_dwn:
stm32h7_adc_enter_pwr_down(adc);
return ret;
}
-static void stm32h7_adc_unprepare(struct stm32_adc *adc)
+static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
{
- stm32h7_adc_disable(adc);
+ struct stm32_adc *adc = iio_priv(indio_dev);
+
+ stm32h7_adc_disable(indio_dev);
stm32h7_adc_enter_pwr_down(adc);
}
@@ -1160,7 +1170,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
stm32_adc_conv_irq_enable(adc);
- adc->cfg->start_conv(adc, false);
+ adc->cfg->start_conv(indio_dev, false);
timeout = wait_for_completion_interruptible_timeout(
&adc->completion, STM32_ADC_TIMEOUT);
@@ -1173,7 +1183,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
ret = IIO_VAL_INT;
}
- adc->cfg->stop_conv(adc);
+ adc->cfg->stop_conv(indio_dev);
stm32_adc_conv_irq_disable(adc);
@@ -1227,8 +1237,8 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
{
- struct stm32_adc *adc = data;
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct iio_dev *indio_dev = data;
+ struct stm32_adc *adc = iio_priv(indio_dev);
const struct stm32_adc_regspec *regs = adc->cfg->regs;
u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
@@ -1240,8 +1250,8 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
static irqreturn_t stm32_adc_isr(int irq, void *data)
{
- struct stm32_adc *adc = data;
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct iio_dev *indio_dev = data;
+ struct stm32_adc *adc = iio_priv(indio_dev);
const struct stm32_adc_regspec *regs = adc->cfg->regs;
u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
@@ -1514,7 +1524,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
if (!adc->dma_chan)
stm32_adc_conv_irq_enable(adc);
- adc->cfg->start_conv(adc, !!adc->dma_chan);
+ adc->cfg->start_conv(indio_dev, !!adc->dma_chan);
return 0;
@@ -1547,7 +1557,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
struct stm32_adc *adc = iio_priv(indio_dev);
struct device *dev = indio_dev->dev.parent;
- adc->cfg->stop_conv(adc);
+ adc->cfg->stop_conv(indio_dev);
if (!adc->dma_chan)
stm32_adc_conv_irq_disable(adc);
@@ -1891,7 +1901,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
indio_dev->info = &stm32_adc_iio_info;
indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
- platform_set_drvdata(pdev, adc);
+ platform_set_drvdata(pdev, indio_dev);
ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset);
if (ret != 0) {
@@ -1905,7 +1915,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
stm32_adc_threaded_isr,
- 0, pdev->name, adc);
+ 0, pdev->name, indio_dev);
if (ret) {
dev_err(&pdev->dev, "failed to request IRQ\n");
return ret;
@@ -1989,8 +1999,8 @@ static int stm32_adc_probe(struct platform_device *pdev)
static int stm32_adc_remove(struct platform_device *pdev)
{
- struct stm32_adc *adc = platform_get_drvdata(pdev);
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+ struct stm32_adc *adc = iio_priv(indio_dev);
pm_runtime_get_sync(&pdev->dev);
iio_device_unregister(indio_dev);
@@ -2012,8 +2022,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
#if defined(CONFIG_PM_SLEEP)
static int stm32_adc_suspend(struct device *dev)
{
- struct stm32_adc *adc = dev_get_drvdata(dev);
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
if (iio_buffer_enabled(indio_dev))
__stm32_adc_buffer_predisable(indio_dev);
@@ -2023,8 +2032,7 @@ static int stm32_adc_suspend(struct device *dev)
static int stm32_adc_resume(struct device *dev)
{
- struct stm32_adc *adc = dev_get_drvdata(dev);
- struct iio_dev *indio_dev = iio_priv_to_dev(adc);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
int ret;
ret = pm_runtime_force_resume(dev);
--
2.17.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 V2] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()
From: Suzuki K Poulose @ 2020-05-26 14:04 UTC (permalink / raw)
To: anshuman.khandual, linux-arm-kernel
Cc: mark.rutland, catalin.marinas, broonie, will, linux-kernel
In-Reply-To: <1590500353-28082-1-git-send-email-anshuman.khandual@arm.com>
On 05/26/2020 02:39 PM, Anshuman Khandual wrote:
> There is no way to proceed when requested register could not be searched in
> arm64_ftr_reg[]. Requesting for a non present register would be an error as
> well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg()
> rather than checking for return value and doing a BUG_ON() instead in some
> individual callers. But there are also caller instances that dont error out
> when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for
> such cases.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V2:
>
> - Added get_arm64_ftr_reg_nowarn() per Will
> - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin
>
> Changes in V1: (https://patchwork.kernel.org/patch/11559083/)
>
> arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index bc5048f152c1..f4555b9d145c 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
> }
>
...
> static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
> s64 ftr_val)
> {
> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
> const struct arm64_ftr_bits *ftrp;
> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>
> - BUG_ON(!reg);
> -
> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
> u64 ftr_mask = arm64_ftr_mask(ftrp);
> s64 ftr_new = arm64_ftr_value(ftrp, new);
> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
> {
> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>
> - BUG_ON(!regp);
> update_cpu_ftr_reg(regp, val);
> if ((boot & regp->strict_mask) == (val & regp->strict_mask))
> return 0;
> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
> const struct arm64_ftr_bits *ftrp;
> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>
> - if (WARN_ON(!regp))
> - return;
> -
You need to return here, on !regp. Rest looks fine to me.
Suzuki
_______________________________________________
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] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()
From: Anshuman Khandual @ 2020-05-26 14:03 UTC (permalink / raw)
To: Suzuki K Poulose, linux-arm-kernel
Cc: mark.rutland, catalin.marinas, broonie, will, linux-kernel
In-Reply-To: <0726a5b9-fff6-a15c-e705-db7abd4b1abd@arm.com>
On 05/26/2020 07:34 PM, Suzuki K Poulose wrote:
> On 05/26/2020 02:39 PM, Anshuman Khandual wrote:
>> There is no way to proceed when requested register could not be searched in
>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>> well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg()
>> rather than checking for return value and doing a BUG_ON() instead in some
>> individual callers. But there are also caller instances that dont error out
>> when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for
>> such cases.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Changes in V2:
>>
>> - Added get_arm64_ftr_reg_nowarn() per Will
>> - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin
>>
>> Changes in V1: (https://patchwork.kernel.org/patch/11559083/)
>>
>> arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++-----------
>> 1 file changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index bc5048f152c1..f4555b9d145c 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>> }
>>
>
> ...
>
>> static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
>> s64 ftr_val)
>> {
>> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>> const struct arm64_ftr_bits *ftrp;
>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>> - BUG_ON(!reg);
>> -
>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>> {
>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>> - BUG_ON(!regp);
>> update_cpu_ftr_reg(regp, val);
>> if ((boot & regp->strict_mask) == (val & regp->strict_mask))
>> return 0;
>> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
>> const struct arm64_ftr_bits *ftrp;
>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>> - if (WARN_ON(!regp))
>> - return;
>> -
>
> You need to return here, on !regp. Rest looks fine to me.
Catalin had suggested and agreed on for this change in behavior here.
If the register is not found, there is already some problem.
_______________________________________________
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] ACPI: GED: add support for _Exx / _Lxx handler methods
From: Rafael J. Wysocki @ 2020-05-26 14:04 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux Kernel Mailing List,
Stable, ACPI Devel Maling List, Linux ARM, Len Brown
In-Reply-To: <CAMj1kXFVYOoX=pe9uVY-j_o8YhhE_Fef6q6jc8S9nzBLYSBb=g@mail.gmail.com>
On Tue, May 26, 2020 at 1:12 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hello Rafael,
>
> I spotted an issue with this patch. Please see below.
>
>
> On Fri, 15 May 2020 at 18:32, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, May 15, 2020 at 11:37 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > Per the ACPI spec, interrupts in the range [0, 255] may be handled
> > > in AML using individual methods whose naming is based on the format
> > > _Exx or _Lxx, where xx is the hex representation of the interrupt
> > > index.
> > >
> > > Add support for this missing feature to our ACPI GED driver.
> > >
> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Cc: linux-acpi@vger.kernel.org
> > > Cc: <stable@vger.kernel.org> # v4.9+
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > drivers/acpi/evged.c | 22 +++++++++++++++++---
> > > 1 file changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
> > > index aba0d0027586..6d7a522952bf 100644
> > > --- a/drivers/acpi/evged.c
> > > +++ b/drivers/acpi/evged.c
> > > @@ -79,6 +79,8 @@ static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
> > > struct resource r;
> > > struct acpi_resource_irq *p = &ares->data.irq;
> > > struct acpi_resource_extended_irq *pext = &ares->data.extended_irq;
> > > + char ev_name[5];
> > > + u8 trigger;
> > >
> > > if (ares->type == ACPI_RESOURCE_TYPE_END_TAG)
> > > return AE_OK;
> > > @@ -87,14 +89,28 @@ static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
> > > dev_err(dev, "unable to parse IRQ resource\n");
> > > return AE_ERROR;
> > > }
> > > - if (ares->type == ACPI_RESOURCE_TYPE_IRQ)
> > > + if (ares->type == ACPI_RESOURCE_TYPE_IRQ) {
> > > gsi = p->interrupts[0];
> > > - else
> > > + trigger = p->triggering;
> > > + } else {
> > > gsi = pext->interrupts[0];
> > > + trigger = p->triggering;
>
> This should be 'pext->triggering' instead.
>
> In practice, it doesn't matter, since p and pext point to the same
> union, and the 'triggering' field happens to be at the same offset.
> But it should still be fixed, of course.
>
> Would you prefer a followup patch? Or can you fix it locally?
A followup, please.
_______________________________________________
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 v3 7/7] [not for merge] netstats: example use of stats_fs API
From: Andrew Lunn @ 2020-05-26 14:16 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: linux-s390, kvm, linux-doc, netdev, Emanuele Giuseppe Esposito,
linux-kernel, kvm-ppc, Jonathan Adams, Christian Borntraeger,
Alexander Viro, David Rientjes, linux-fsdevel, Paolo Bonzini,
linux-mips, linuxppc-dev, linux-arm-kernel, Jim Mattson
In-Reply-To: <20200526110318.69006-8-eesposit@redhat.com>
On Tue, May 26, 2020 at 01:03:17PM +0200, Emanuele Giuseppe Esposito wrote:
> Apply stats_fs on the networking statistics subsystem.
>
> Currently it only works with disabled network namespace
> (CONFIG_NET_NS=n), because multiple namespaces will have the same
> device name under the same root source that will cause a conflict in
> stats_fs.
Hi Emanuele
How do you atomically get and display a group of statistics?
If you look at how the netlink socket works, you will see code like:
do {
start = u64_stats_fetch_begin_irq(&cpu_stats->syncp);
rx_packets = cpu_stats->rx_packets;
rx_bytes = cpu_stats->rx_bytes;
....
} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, start));
It will ensure that rx_packets and rx_bytes are consistent with each
other. If the value of the sequence counter changes while inside the
loop, the loop so repeated until it does not change.
In general, hardware counters in NICs are the same. You tell it to
take a snapshot of the statistics counters, and then read them all
back, to give a consistent view across all the statistics.
I've not looked at this new code in detail, but it looks like you have
one file per statistic, and assume each statistic is independent of
every other statistic. This independence can limit how you use the
values, particularly when debugging. The netlink interface we use does
not have this limitation.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] ARM: mm: Simplify act_mm macro
From: Linus Walleij @ 2020-05-26 14:32 UTC (permalink / raw)
To: Russell King; +Cc: Linus Walleij, linux-arm-kernel
The act_mm assembly macro is actually partly reimplementing
get_thread_info so let's just use that.
Suggested-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
arch/arm/mm/proc-macros.S | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index 60ac7c5999a9..65eaea85d3d6 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -5,7 +5,6 @@
* VMA_VM_FLAGS
* VM_EXEC
*/
-#include <linux/const.h>
#include <asm/asm-offsets.h>
#include <asm/thread_info.h>
@@ -31,9 +30,7 @@
* act_mm - get current->active_mm
*/
.macro act_mm, rd
- bic \rd, sp, #(THREAD_SIZE - 1) & ~63
- bic \rd, \rd, #63
- ldr \rd, [\rd, #TI_TASK]
+ get_thread_info \rd
.if (TSK_ACTIVE_MM > IMM12_MASK)
add \rd, \rd, #TSK_ACTIVE_MM & ~IMM12_MASK
.endif
--
2.25.4
_______________________________________________
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] ARM: mm: Simplify act_mm macro
From: Russell King - ARM Linux admin @ 2020-05-26 14:37 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-arm-kernel
In-Reply-To: <20200526143249.68202-1-linus.walleij@linaro.org>
On Tue, May 26, 2020 at 04:32:49PM +0200, Linus Walleij wrote:
> The act_mm assembly macro is actually partly reimplementing
> get_thread_info so let's just use that.
>
> Suggested-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> arch/arm/mm/proc-macros.S | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> index 60ac7c5999a9..65eaea85d3d6 100644
> --- a/arch/arm/mm/proc-macros.S
> +++ b/arch/arm/mm/proc-macros.S
> @@ -5,7 +5,6 @@
> * VMA_VM_FLAGS
> * VM_EXEC
> */
> -#include <linux/const.h>
> #include <asm/asm-offsets.h>
> #include <asm/thread_info.h>
>
> @@ -31,9 +30,7 @@
> * act_mm - get current->active_mm
> */
> .macro act_mm, rd
> - bic \rd, sp, #(THREAD_SIZE - 1) & ~63
> - bic \rd, \rd, #63
> - ldr \rd, [\rd, #TI_TASK]
> + get_thread_info \rd
This is not quite the same thing.
get_thread_info loads into \rd the address of the thread_info structure.
That's what the two bic instructions are doing. The LDR is then loading
the address of the task_struct into \rd.
> .if (TSK_ACTIVE_MM > IMM12_MASK)
> add \rd, \rd, #TSK_ACTIVE_MM & ~IMM12_MASK
> .endif
So this change alters which structure \rd is pointing to.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps 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 v4 1/5] coresight: Fix comment in main header file.
From: Suzuki K Poulose @ 2020-05-26 14:49 UTC (permalink / raw)
To: mike.leach, linux-arm-kernel, coresight, mathieu.poirier; +Cc: acme
In-Reply-To: <20200526104642.9526-2-mike.leach@linaro.org>
On 05/26/2020 11:46 AM, Mike Leach wrote:
> Comment for an elemnt in the coresight_device structure appears to have
> been corrupted & makes no sense. Fix this before making further changes.
>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.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: [PATCH 13/14] prctl.2: Add SVE prctls (arm64)
From: Dave Martin @ 2020-05-26 14:45 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arch, linux-man, Michael Kerrisk (man-pages),
linux-arm-kernel, Catalin Marinas
In-Reply-To: <20200513211153.GB28594@willie-the-truck>
On Wed, May 13, 2020 at 10:11:54PM +0100, Will Deacon wrote:
> On Wed, May 13, 2020 at 03:02:00PM +0100, Dave Martin wrote:
> > On Wed, May 13, 2020 at 01:01:12PM +0200, Michael Kerrisk (man-pages) wrote:
> > > On 5/13/20 12:46 PM, Dave Martin wrote:
> > > > On Wed, May 13, 2020 at 09:43:52AM +0100, Will Deacon wrote:
> > > >> On Tue, May 12, 2020 at 05:36:58PM +0100, Dave Martin wrote:
[...]
> > > >>> +If
> > > >>> +.B PR_SVE_VL_INHERIT
> > > >>> +is also included in
> > > >>> +.IR arg2 ,
> > > >>> +it takes effect
> > > >>> +.I after
> > > >>> +this deferred change.
> > > >>
> > > >> I find this a bit hard to follow, since it's not clear to me whether the
> > > >> INHERIT flag is effectively set before or after the next execve(). In other
> > > >> words, if both PR_SVE_SET_VL_ONEXEC and PR_SVE_VL_INHERIT are specified,
> > > >> is the vector length preserved or reset on the next execve()?
> > > >
> > > > It makes no difference, because the ONEXEC handling takes priority over
> > > > the INHERIT handling. But either way INHERIT is never cleared by execve()
> > > > and will apply at subsequent execs().
> > > >
> > > > Explaining all this properly seems out of scope here. Maybe this should
> > > > be trimmed down rather than elaborated? Or perhaps just explain it in
> > > > terms of what the kernel does instead of futile attempts to make it
> > > > intuitive?
>
> Hmm, if we don't explain it in the man page then we should at least point
> people to somewhere where they can get the gory details, because I think
> they're necessary in order to use the prctl() request correctly. I'm still
> not confident that I understand the semantics of setting both
> PR_SVE_SET_VL_ONEXEC and PR_SVE_VL_INHERIT without reading the code, which
> may change.
On this point, can you review the following wording?
I simply enumerate the possible flag combinations now, rather than tying
myself in knots trying to describe the two flags independently.
Cheers
---Dave
--8<--
PR_SVE_SET_VL (since Linux 4.15, only on arm64)
Configure the thread's SVE vector length, as specified by (int)
arg2. Arguments arg3, arg4 and arg5 are ignored.
The bits of arg2 corresponding to PR_SVE_VL_LEN_MASK must be set
to the desired vector length in bytes. This is interpreted as
an upper bound: the kernel will select the greatest available
vector length that does not exceed the value specified. In par-
ticular, specifying SVE_VL_MAX (defined in <asm/sigcontext.h>)
for the PR_SVE_VL_LEN_MASK bits requests the maximum supported
vector length.
In addition, arg2 may include the following combinations of
flags:
0 Perform the change immediately. At the next execve(2) in
the thread, the vector length will be reset to the value
configured in /proc/sys/abi/sve_default_vector_length.
PR_SVE_VL_INHERIT
Perform the change immediately. Subsequent execve(2)
calls will preserve the new vector length.
PR_SVE_SET_VL_ONEXEC
Defer the change, so that it is performed at the next
execve(2) in the thread. Further execve(2) calls will
reset the vector length to the value configured in
/proc/sys/abi/sve_default_vector_length.
PR_SVE_SET_VL_ONEXEC | PR_SVE_VL_INHERIT
Defer the change, so that it is performed at the next
execve(2) in the thread. Further execve(2) calls will
preserve the new vector length.
In all cases, any previously pending deferred change is can-
celed.
The call fails with error EINVAL if SVE is not supported on the
platform, if arg2 is unrecognized or invalid, or the value in
the bits of arg2 corresponding to PR_SVE_VL_LEN_MASK is outside
the range SVE_VL_MIN..SVE_VL_MAX, or is not a multiple of 16.
On success, a nonnegative value is returned that describes the
selected configuration, which may differ from the current con-
figuration if PR_SVE_SET_VL_ONEXEC was specified. The value is
encoded in the same way as the return value of PR_SVE_GET_VL.
The configuration (including any pending deferred change) is
inherited across fork(2) and clone(2).
For more information, see the kernel source file Documenta-
tion/arm64/sve.rst (or Documentation/arm64/sve.txt before Linux
5.3).
Warning: Because the compiler or run-time environment may be
using SVE, using this call without the PR_SVE_SET_VL_ONEXEC flag
can lead to unpredicable behaviour in the calling process. The
conditions for using it safely are complex and system-dependent.
Don't use it unless you really know what you are doing.
-->8--
_______________________________________________
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/2] PCI: Introduce PCI_FIXUP_IOMMU
From: Christoph Hellwig @ 2020-05-26 14:46 UTC (permalink / raw)
To: Zhangfei Gao
Cc: jean-philippe, Lorenzo Pieralisi, Herbert Xu, Arnd Bergmann,
linux-pci, Greg Kroah-Hartman, Joerg Roedel, Hanjun Guo,
Rafael J. Wysocki, linux-kernel, iommu, linux-acpi, Wangzhou,
linux-crypto, Sudeep Holla, Bjorn Helgaas, kenneth-lee-2012,
linux-arm-kernel, Len Brown
In-Reply-To: <1590493749-13823-2-git-send-email-zhangfei.gao@linaro.org>
On Tue, May 26, 2020 at 07:49:08PM +0800, Zhangfei Gao wrote:
> Some platform devices appear as PCI but are actually on the AMBA bus,
> and they need fixup in drivers/pci/quirks.c handling iommu_fwnode.
> Here introducing PCI_FIXUP_IOMMU, which is called after iommu_fwnode
> is allocated, instead of reusing PCI_FIXUP_FINAL since it will slow
> down iommu probing as all devices in fixup final list will be
> reprocessed.
Who is going to use this? I don't see a single user in the series.
_______________________________________________
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