* [PATCHv4 01/12] cma: Store a name in the cma structure
From: Laura Abbott @ 2017-04-18 18:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492540034-5466-1-git-send-email-labbott@redhat.com>
Frameworks that may want to enumerate CMA heaps (e.g. Ion) will find it
useful to have an explicit name attached to each region. Store the name
in each CMA structure.
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
arch/powerpc/kvm/book3s_hv_builtin.c | 3 ++-
drivers/base/dma-contiguous.c | 5 +++--
include/linux/cma.h | 4 +++-
mm/cma.c | 17 +++++++++++++++--
mm/cma.h | 1 +
mm/cma_debug.c | 2 +-
6 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
index 4d6c64b..b739ff8 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -100,7 +100,8 @@ void __init kvm_cma_reserve(void)
(unsigned long)selected_size / SZ_1M);
align_size = HPT_ALIGN_PAGES << PAGE_SHIFT;
cma_declare_contiguous(0, selected_size, 0, align_size,
- KVM_CMA_CHUNK_ORDER - PAGE_SHIFT, false, &kvm_cma);
+ KVM_CMA_CHUNK_ORDER - PAGE_SHIFT, false, "kvm_cma",
+ &kvm_cma);
}
}
diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index b55804c..ea9726e 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -165,7 +165,8 @@ int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
{
int ret;
- ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed, res_cma);
+ ret = cma_declare_contiguous(base, size, limit, 0, 0, fixed,
+ "reserved", res_cma);
if (ret)
return ret;
@@ -258,7 +259,7 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
return -EINVAL;
}
- err = cma_init_reserved_mem(rmem->base, rmem->size, 0, &cma);
+ err = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name, &cma);
if (err) {
pr_err("Reserved memory: unable to setup CMA region\n");
return err;
diff --git a/include/linux/cma.h b/include/linux/cma.h
index 03f32d0..d41d1f8 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -21,13 +21,15 @@ struct cma;
extern unsigned long totalcma_pages;
extern phys_addr_t cma_get_base(const struct cma *cma);
extern unsigned long cma_get_size(const struct cma *cma);
+extern const char *cma_get_name(const struct cma *cma);
extern int __init cma_declare_contiguous(phys_addr_t base,
phys_addr_t size, phys_addr_t limit,
phys_addr_t alignment, unsigned int order_per_bit,
- bool fixed, struct cma **res_cma);
+ bool fixed, const char *name, struct cma **res_cma);
extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
unsigned int order_per_bit,
+ const char *name,
struct cma **res_cma);
extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
gfp_t gfp_mask);
diff --git a/mm/cma.c b/mm/cma.c
index a6033e3..43c1b2c 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -53,6 +53,11 @@ unsigned long cma_get_size(const struct cma *cma)
return cma->count << PAGE_SHIFT;
}
+const char *cma_get_name(const struct cma *cma)
+{
+ return cma->name ? cma->name : "(undefined)";
+}
+
static unsigned long cma_bitmap_aligned_mask(const struct cma *cma,
int align_order)
{
@@ -168,6 +173,7 @@ core_initcall(cma_init_reserved_areas);
*/
int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
unsigned int order_per_bit,
+ const char *name,
struct cma **res_cma)
{
struct cma *cma;
@@ -198,6 +204,13 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
* subsystems (like slab allocator) are available.
*/
cma = &cma_areas[cma_area_count];
+ if (name) {
+ cma->name = name;
+ } else {
+ cma->name = kasprintf(GFP_KERNEL, "cma%d\n", cma_area_count);
+ if (!cma->name)
+ return -ENOMEM;
+ }
cma->base_pfn = PFN_DOWN(base);
cma->count = size >> PAGE_SHIFT;
cma->order_per_bit = order_per_bit;
@@ -229,7 +242,7 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
int __init cma_declare_contiguous(phys_addr_t base,
phys_addr_t size, phys_addr_t limit,
phys_addr_t alignment, unsigned int order_per_bit,
- bool fixed, struct cma **res_cma)
+ bool fixed, const char *name, struct cma **res_cma)
{
phys_addr_t memblock_end = memblock_end_of_DRAM();
phys_addr_t highmem_start;
@@ -335,7 +348,7 @@ int __init cma_declare_contiguous(phys_addr_t base,
base = addr;
}
- ret = cma_init_reserved_mem(base, size, order_per_bit, res_cma);
+ ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
if (ret)
goto err;
diff --git a/mm/cma.h b/mm/cma.h
index 17c75a4..4986128 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -11,6 +11,7 @@ struct cma {
struct hlist_head mem_head;
spinlock_t mem_head_lock;
#endif
+ const char *name;
};
extern struct cma cma_areas[MAX_CMA_AREAS];
diff --git a/mm/cma_debug.c b/mm/cma_debug.c
index ffc0c3d..595b757 100644
--- a/mm/cma_debug.c
+++ b/mm/cma_debug.c
@@ -167,7 +167,7 @@ static void cma_debugfs_add_one(struct cma *cma, int idx)
char name[16];
int u32s;
- sprintf(name, "cma-%d", idx);
+ sprintf(name, "cma-%s", cma->name);
tmp = debugfs_create_dir(name, cma_debugfs_root);
--
2.7.4
^ permalink raw reply related
* [PATCHv4 00/12] Ion cleanup in preparation for moving out of staging
From: Laura Abbott @ 2017-04-18 18:27 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
This is v4 of the series to cleanup to Ion. Greg took some of the patches
that weren't CMA related already. There was a minor bisectability problem
with the CMA APIs so this is a new version to address that. I also
addressed some minor comments on the patch to collapse header files.
Thanks,
Laura
Laura Abbott (12):
cma: Store a name in the cma structure
cma: Introduce cma_for_each_area
staging: android: ion: Use CMA APIs directly
staging: android: ion: Stop butchering the DMA address
staging: android: ion: Break the ABI in the name of forward progress
staging: android: ion: Get rid of ion_phys_addr_t
staging: android: ion: Collapse internal header files
staging: android: ion: Rework heap registration/enumeration
staging: android: ion: Drop ion_map_kernel interface
staging: android: ion: Remove ion_handle and ion_client
staging: android: ion: Set query return value
staging/android: Update Ion TODO list
arch/powerpc/kvm/book3s_hv_builtin.c | 3 +-
drivers/base/dma-contiguous.c | 5 +-
drivers/staging/android/TODO | 21 +-
drivers/staging/android/ion/Kconfig | 32 +
drivers/staging/android/ion/Makefile | 11 +-
drivers/staging/android/ion/compat_ion.c | 152 -----
drivers/staging/android/ion/compat_ion.h | 29 -
drivers/staging/android/ion/ion-ioctl.c | 55 +-
drivers/staging/android/ion/ion.c | 812 ++----------------------
drivers/staging/android/ion/ion.h | 386 ++++++++---
drivers/staging/android/ion/ion_carveout_heap.c | 21 +-
drivers/staging/android/ion/ion_chunk_heap.c | 16 +-
drivers/staging/android/ion/ion_cma_heap.c | 120 ++--
drivers/staging/android/ion/ion_heap.c | 68 --
drivers/staging/android/ion/ion_page_pool.c | 3 +-
drivers/staging/android/ion/ion_priv.h | 453 -------------
drivers/staging/android/ion/ion_system_heap.c | 39 +-
drivers/staging/android/uapi/ion.h | 36 +-
include/linux/cma.h | 6 +-
mm/cma.c | 31 +-
mm/cma.h | 1 +
mm/cma_debug.c | 2 +-
22 files changed, 524 insertions(+), 1778 deletions(-)
delete mode 100644 drivers/staging/android/ion/compat_ion.c
delete mode 100644 drivers/staging/android/ion/compat_ion.h
delete mode 100644 drivers/staging/android/ion/ion_priv.h
--
2.7.4
^ permalink raw reply
* [PATCHv3 08/14] drivers/perf: arm_pmu: split cpu-local irq request/free
From: Geert Uytterhoeven @ 2017-04-18 18:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdVV4qMyYTYN92TE4fsrxo66WK-Gd001YbJZok2txOH4ig@mail.gmail.com>
On Tue, Apr 18, 2017 at 7:25 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Apr 11, 2017 at 10:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Currently we have functions to request/free all IRQs for a given PMU.
>> While this works today, this won't work for ACPI, where we don't know
>> the full set of IRQs up front, and need to request them separately.
>>
>> To enable supporting ACPI, this patch splits out the cpu-local
>> request/free into new functions, allowing us to request/free individual
>> IRQs.
>>
>> As this makes it possible/necessary to request a PPI once per cpu, an
>> additional check is added to detect mismatched PPIs. This shouldn't
>> matter for the DT / platform case, as we check this when parsing.
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>
> This patch causes warnings during PSCI system suspend on R-Car Gen3.
Also on SH-Mobile AG5 (dual CA9).
It does not happen on R-Car M2-W (dual CA15).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] Revert "arm64: Increase the max granular size"
From: Chalamarla, Tirumalesh @ 2017-04-18 18:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <93d2819a-95b1-6606-74d4-0bc0a64db29e@codeaurora.org>
On 4/17/17, 12:35 AM, "Imran Khan" <kimran@codeaurora.org> wrote:
On 4/12/2017 7:30 PM, Chalamarla, Tirumalesh wrote:
>
>
> On 4/11/17, 10:13 PM, "linux-arm-kernel on behalf of Imran Khan" <linux-arm-kernel-bounces at lists.infradead.org on behalf of kimran@codeaurora.org> wrote:
>
> On 4/7/2017 7:36 AM, Ganesh Mahendran wrote:
> > 2017-04-06 23:58 GMT+08:00 Catalin Marinas <catalin.marinas@arm.com>:
> >> On Thu, Apr 06, 2017 at 12:52:13PM +0530, Imran Khan wrote:
> >>> On 4/5/2017 10:13 AM, Imran Khan wrote:
> >>>>> We may have to revisit this logic and consider L1_CACHE_BYTES the
> >>>>> _minimum_ of cache line sizes in arm64 systems supported by the kernel.
> >>>>> Do you have any benchmarks on Cavium boards that would show significant
> >>>>> degradation with 64-byte L1_CACHE_BYTES vs 128?
> >>>>>
> >>>>> For non-coherent DMA, the simplest is to make ARCH_DMA_MINALIGN the
> >>>>> _maximum_ of the supported systems:
> >>>>>
> >>>>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
> >>>>> index 5082b30bc2c0..4b5d7b27edaf 100644
> >>>>> --- a/arch/arm64/include/asm/cache.h
> >>>>> +++ b/arch/arm64/include/asm/cache.h
> >>>>> @@ -18,17 +18,17 @@
> >>>>>
> >>>>> #include <asm/cachetype.h>
> >>>>>
> >>>>> -#define L1_CACHE_SHIFT 7
> >>>>> +#define L1_CACHE_SHIFT 6
> >>>>> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> >>>>>
> >>>>> /*
> >>>>> * Memory returned by kmalloc() may be used for DMA, so we must make
> >>>>> - * sure that all such allocations are cache aligned. Otherwise,
> >>>>> - * unrelated code may cause parts of the buffer to be read into the
> >>>>> - * cache before the transfer is done, causing old data to be seen by
> >>>>> - * the CPU.
> >>>>> + * sure that all such allocations are aligned to the maximum *known*
> >>>>> + * cache line size on ARMv8 systems. Otherwise, unrelated code may cause
> >>>>> + * parts of the buffer to be read into the cache before the transfer is
> >>>>> + * done, causing old data to be seen by the CPU.
> >>>>> */
> >>>>> -#define ARCH_DMA_MINALIGN L1_CACHE_BYTES
> >>>>> +#define ARCH_DMA_MINALIGN (128)
> >>>>>
> >>>>> #ifndef __ASSEMBLY__
> >>>>>
> >>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>>>> index 392c67eb9fa6..30bafca1aebf 100644
> >>>>> --- a/arch/arm64/kernel/cpufeature.c
> >>>>> +++ b/arch/arm64/kernel/cpufeature.c
> >>>>> @@ -976,9 +976,9 @@ void __init setup_cpu_features(void)
> >>>>> if (!cwg)
> >>>>> pr_warn("No Cache Writeback Granule information, assuming
> >>>>> cache line size %d\n",
> >>>>> cls);
> >>>>> - if (L1_CACHE_BYTES < cls)
> >>>>> - pr_warn("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n",
> >>>>> - L1_CACHE_BYTES, cls);
> >>>>> + if (ARCH_DMA_MINALIGN < cls)
> >>>>> + pr_warn("ARCH_DMA_MINALIGN smaller than the Cache Writeback Granule (%d < %d)\n",
> >>>>> + ARCH_DMA_MINALIGN, cls);
> >>>>> }
> >>>>>
> >>>>> static bool __maybe_unused
> >>>>
> >>>> This change was discussed at: [1] but was not concluded as apparently no one
> >>>> came back with test report and numbers. After including this change in our
> >>>> local kernel we are seeing significant throughput improvement. For example with:
> >>>>
> >>>> iperf -c 192.168.1.181 -i 1 -w 128K -t 60
> >>>>
> >>>> The average throughput is improving by about 30% (230Mbps from 180Mbps).
> >>>> Could you please let us know if this change can be included in upstream kernel.
> >>>>
> >>>> [1]: https://groups.google.com/forum/#!topic/linux.kernel/P40yDB90ePs
> >>>
> >>> Could you please provide some feedback about the above mentioned query ?
> >>
> >> Do you have an explanation on the performance variation when
> >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
> >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
> >> non-coherent DMA?).
> >
> > network stack use SKB_DATA_ALIGN to align.
> > ---
> > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
> > ~(SMP_CACHE_BYTES - 1))
> >
> > #define SMP_CACHE_BYTES L1_CACHE_BYTES
> > ---
> > I think this is the reason of performance regression.
> >
>
> Yes this is the reason for performance regression. Due to increases L1 cache alignment the
> object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
> 4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
>
> We tried different benchmarks and found none which really affects with Cache line change. If there is no correctness issue,
> I think we are fine with reverting the patch.
>
So, can we revert the patch that makes L1_CACHE_SHIFT 7 or should the patch suggested by Catalin should be mainlined.
We have verified the throughput degradation on 3.18 and 4.4 but I am afraid that this issue will be seen on other
kernels too.
> Though I still think it is beneficiary to do some more investigation for the perf loss, who knows 32 bit align or no align might
> Give even more perf benefit.
>
Which perf loss you are referring to here. Did you mean throughput loss here or some other perf benchmarking ?
The iperf issue mentioning here, looks to me as incomplete.
Thanks,
Imran
>
> Thanks,
> Tirumalesh.
> >>
> >> The Cavium guys haven't shown any numbers (IIUC) to back the
> >> L1_CACHE_BYTES performance improvement but I would not revert the
> >> original commit since ARCH_DMA_MINALIGN definitely needs to cover the
> >> maximum available cache line size, which is 128 for them.
> >
> > how about define L1_CACHE_SHIFT like below:
> > ---
> > #ifdef CONFIG_ARM64_L1_CACHE_SHIFT
> > #define L1_CACHE_SHIFT CONFIG_ARM64_L1_CACHE_SHIFT
> > #else
> > #define L1_CACHE_SHIFT 7
> > endif
> > ---
> >
> > Thanks
> >
> >>
> >> --
> >> Catalin
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* [PATCH 4/5] Hot-remove implementation for arm64
From: Mark Rutland @ 2017-04-18 18:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170414140158.GA17950@samekh>
On Fri, Apr 14, 2017 at 03:01:58PM +0100, Andrea Reale wrote:
> I guess it is likely that I might have made assumptions that are true
> for x86_64 but do not hold for arm64. Whenever you feel this is the
> case, I would be really grateful if you could help identify them.
Sure thing.
> On Tue, Apr 11, 2017 at 06:12:11PM +0100, Mark Rutland wrote:
> > On Tue, Apr 11, 2017 at 03:55:42PM +0100, Andrea Reale wrote:
> > > +static void free_pagetable(struct page *page, int order, bool direct)
> >
> > This "direct" parameter needs a better name, and a description in a
> > comment block above this function.
>
> The name direct is inherited directly from the x86_64 hot remove code.
> It serves to distinguish if we are removing either a pagetable page that
> is mapping to the direct mapping space (I think it is called also linear
> mapping area somewhere else) or a pagetable page or a data page
> from vmemmap.
FWIW, I've largely heard the folk call that the "linear mapping", and
x86 folk call that the "direct mapping". The two are interchangeable.
> In this specific set of functions, the flag is needed because the various
> alloc_init_p*d used for allocating entries for direct memory mapping
> rely on pgd_pgtable_alloc, which in its turn calls pgtable_page_ctor;
> hence, we need to call the dtor.
AFAICT, that's not true for the arm64 linear map, since that's created
with our early_pgtable_alloc(), which doesn't call pgtable_page_ctor().
Judging by commit:
1378dc3d4ba07ccd ("arm64: mm: run pgtable_page_ctor() on non-swapper
translation table pages")
... we only do this for non-swapper page tables.
> On the contrary, vmemmap entries are created using vmemmap_alloc_block
> (from within vmemmap_populate), which does not call pgtable_page_ctor
> when allocating pages.
>
> I am not sure I understand why the pgtable_page_ctor is not called when
> allocating vmemmap page tables, but that's the current situation.
>From a quick scan, I see that it's necessary to use pgtable_page_ctor()
for pages that will be used for userspace page tables, but it's not
clear to me if it's ever necessary for pages used for kernel page
tables.
If it is, we appear to have a bug on arm64.
Laura, Ard, thoughts?
> > > +{
> > > + unsigned long magic;
> > > + unsigned int nr_pages = 1 << order;
> > > + struct vmem_altmap *altmap = to_vmem_altmap((unsigned long) page);
> > > +
> > > + if (altmap) {
> > > + vmem_altmap_free(altmap, nr_pages);
> > > + return;
> > > + }
> > > +
> > > + /* bootmem page has reserved flag */
> > > + if (PageReserved(page)) {
> > > + __ClearPageReserved(page);
> > > +
> > > + magic = (unsigned long)page->lru.next;
> > > + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
> > > + while (nr_pages--)
> > > + put_page_bootmem(page++);
> > > + } else {
> > > + while (nr_pages--)
> > > + free_reserved_page(page++);
> > > + }
> > > + } else {
> > > + /*
> > > + * Only direct pagetable allocation (those allocated via
> > > + * hotplug) call the pgtable_page_ctor; vmemmap pgtable
> > > + * allocations don't.
> > > + */
> > > + if (direct)
> > > + pgtable_page_dtor(page);
> > > +
> > > + free_pages((unsigned long)page_address(page), order);
> > > + }
> > > +}
> >
> > This largely looks like a copy of the x86 code. Why can that not be
> > factored out to an arch-neutral location?
>
> Yes it probably can - the only difference being calling
> pgtable_page_dtor when it needs to - but I am not confident enough to
> say that it would really be architecture neutral or just specific to
> only arm64 and x86. For example, I don't see this used anywhere else
> for hot-removing memory.
Mhmmm. As above, it's also not clear to me if the ctor()/dtor() dance is
necessary in the first place.
I'm also not clear on why x86_64 and powerpc are the only architectures
that appear to clear up their page tables after __remove_pages(). Do
other architectures not have "real" hot-remove?
> (Actually, also a large part of remove_*_table and free_*_table could
> probably be factored, but I wouldn't be sure how to deal with the
> differences in the pgtable.h macros used throughout)
Let's figure out what's necessary first. Then we'll know if/how we can
align on a common pattern.
[...]
>> > + page = pmd_page(*pmd);
> > > +
> > > + free_pagetable(page, 0, direct);
> >
> > The page has been freed here, and may be subject to arbitrary
> > modification...
> >
> > > +
> > > + /*
> > > + * This spin lock could be only taken in _pte_aloc_kernel
> > > + * in mm/memory.c and nowhere else (for arm64). Not sure if
> > > + * the function above can be called concurrently. In doubt,
> > > + * I am living it here for now, but it probably can be removed
> > > + */
> > > + spin_lock(&init_mm.page_table_lock);
> > > + pmd_clear(pmd);
> >
> > ... but we only remove it from the page tables here, so the page table
> > walkers can see junk in the tables, were the page reused in the mean
> > timer.
> >
> > After clearing the PMD, it needs to be cleared from TLBs. We allow
> > partial walks to be cached, so the TLBs may still start walking this
> > entry or beyond.
>
> I guess that the safe approach would be something along the lines:
> 1. clear the page table
> 2. flush the tlbs
> 3. free the page
Yes. That's the sequence we follow elsewhere.
> When I am flushing intermediate p*d entries, would it be
> more appropriate to use something like __flush_tlb_pgtable() to clear
> cached partial walks rather than flushing the whole table? I mean,
> hot-remove is not going to be a frequent operation anyway, so I don't
> think that flushing the whole tlb would be a great deal of harm
> anyway.
Using __flush_tlb_pgtable() sounds sane to me. That's what we do when
tearing down user mappings.
> My question at this point would be: how come that the code structure above
> works for x86_64 hot-remove?
I don't know enough about x86 to say.
> My assumption, when I was writing this, was
> that there would be no problem since this code is called when we are sure
> that all the memory mapped by these entries has been already offlined,
> so nobody should be using those VAs anyway (also considering that there
> cannot be any two mem hot-plug/remove actions running concurrently).
> Is that correct?
The problem is that speculation, Out-of-Order execution, HW prefetching,
and other such things *can* result in those VAs being accessed,
regardless of what code explicitly does in a sequential execution.
If any table relevant to one of those VAs has been freed (and
potentially modified by the allocator, or in another thread), it's
possible that the CPU performs a page table walk and sees junk as a
valid page table entry. As a result, a number of bad things can happen.
If the entry was a pgd, pud, or pmd, the CPU may try to continue to walk
to the relevant pte, accessing a junk address. That could change the
state of MMIO, trigger an SError, etc, or allocate more junk into the
TLBs.
For any level, the CPU might allocate the entry into a TLB, regardless
of whether an existing entry existed. The new entry can conflict with
the existing one, either leading to a TLB conflict abort, or to the TLB
returning junk for that address. Speculation and so on can now access
junk based on that, etc.
[...]
> > > +static void remove_pte_table(pte_t *pte, unsigned long addr,
> > > + unsigned long end, bool direct)
> > > +{
> > > + unsigned long next;
> > > + void *page_addr;
> > > +
> > > + for (; addr < end; addr = next, pte++) {
> > > + next = (addr + PAGE_SIZE) & PAGE_MASK;
> > > + if (next > end)
> > > + next = end;
> >
> > Please use the usual p*d_addr_end() functions. See alloc_init_pmd() and
> > friends in arch/arm64/mm/mmu.c for examples of how to use them.
>
> we used the p*d_addr_end family of functions when dealing with p*d(s). I
> cannot identify an equivalent for pte entries.
Ah; my bad.
I guess we should follow the same pattern as alloc_init_pte() does here,
assuming we use p*d_addr_end() for all the levels above (as for
alloc_init_p*d()).
> Would you recommend adding a pte_addr_end macro in pgtable.h?
That shouldn't be necessary. Sorry for the confusion.
> > > +
> > > + if (!pte_present(*pte))
> > > + continue;
> > > +
> > > + if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {
> >
> > When would those addresses *not* be page-aligned? By construction, next
> > must be. Unplugging partial pages of memory makes no sense, so surely
> > addr is page-aligned when passed in?
>
> The issue here is that this function is called in one of two cases:
> 1. to clear pagetables of directly mapped (linear) memory
> 2. Pagetables (and corresponding data pages) for vmemmap.
>
> It is my understanding that, in the second case, we might be clearing
> only part of the page content (i.e, only a few struct pages). Note that
> next is page aligned by construction only if next <= end.
Ok. A comment to that effect immediately above this check would be
helpful.
> > > + /*
> > > + * Do not free direct mapping pages since they were
> > > + * freed when offlining, or simplely not in use.
> > > + */
> > > + if (!direct)
> > > + free_pagetable(pte_page(*pte), 0, direct);
> > > +
> > > + /*
> > > + * This spin lock could be only
> > > + * taken in _pte_aloc_kernel in
> > > + * mm/memory.c and nowhere else
> > > + * (for arm64). Not sure if the
> > > + * function above can be called
> > > + * concurrently. In doubt,
> > > + * I am living it here for now,
> > > + * but it probably can be removed.
> > > + */
> > > + spin_lock(&init_mm.page_table_lock);
> > > + pte_clear(&init_mm, addr, pte);
> > > + spin_unlock(&init_mm.page_table_lock);
> > > + } else {
> > > + /*
> > > + * If we are here, we are freeing vmemmap pages since
> > > + * direct mapped memory ranges to be freed are aligned.
> > > + *
> > > + * If we are not removing the whole page, it means
> > > + * other page structs in this page are being used and
> > > + * we canot remove them. So fill the unused page_structs
> > > + * with 0xFD, and remove the page when it is wholly
> > > + * filled with 0xFD.
> > > + */
> > > + memset((void *)addr, PAGE_INUSE, next - addr);
> >
> > What's special about 0xFD?
>
> Just used it as a constant symmetrically to x86_64 code.
>
> > Why do we need to mess with the page array in this manner? Why can't we
> > detect when a range is free by querying memblock, for example?
>
> I am not sure I get your suggestion. I guess that the logic here
> is that I cannot be sure that I can free the full page because other
> entries might be in use for active vmemmap mappings. So we just "mark"
> the unused once and only free the page when all of it is marked. See
> again commit ae9aae9eda2db71, where all this comes from.
I understood that this is deferring freeing until a whole page of struct
pages has been freed.
My concern is that filling the unused memory with an array of junk chars
feels like a hack. We don't do this at the edges when we allocate
memblock today, AFAICT, so this doesn't seem complete.
Is there no "real" datastructure we can use to keep track of what memory
is present? e.g. memblock?
[...]
> > > +static void remove_pmd_table(pmd_t *pmd, unsigned long addr,
> > > + unsigned long end, bool direct)
> > > +{
> > > + unsigned long next;
> > > + void *page_addr;
> > > + pte_t *pte;
> > > +
> > > + for (; addr < end; addr = next, pmd++) {
> > > + next = pmd_addr_end(addr, end);
> > > +
> > > + if (!pmd_present(*pmd))
> > > + continue;
> > > +
> > > + // check if we are using 2MB section mappings
> > > + if (pmd_sect(*pmd)) {
> > > + if (PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)) {
> >
> > Surely you're intending to check if you can free the whole pmd? i.e.
> > that addr and next are pmd-aligned?
>
> Indeed, that's a mistake. It should have been IS_ALIGNED(addr, PMD_SIZE).
Ok.
> > Can we ever be in a situation where we're requested to free a partial
> > pmd that could be section mapped?
>
> Yes, as I said above, for vmemmap mappings.
Ok.
> > If that's the case, we'll *need* to split the pmd, which we can't do on
> > live page tables.
>
> Please, see below.
>
> > > + if (!direct) {
> > > + free_pagetable(pmd_page(*pmd),
> > > + get_order(PMD_SIZE), direct);
> > > + }
> > > + /*
> > > + * This spin lock could be only
> > > + * taken in _pte_aloc_kernel in
> > > + * mm/memory.c and nowhere else
> > > + * (for arm64). Not sure if the
> > > + * function above can be called
> > > + * concurrently. In doubt,
> > > + * I am living it here for now,
> > > + * but it probably can be removed.
> > > + */
> > > + spin_lock(&init_mm.page_table_lock);
> > > + pmd_clear(pmd);
> > > + spin_unlock(&init_mm.page_table_lock);
> > > + } else {
> > > + /* If here, we are freeing vmemmap pages. */
> > > + memset((void *)addr, PAGE_INUSE, next - addr);
> > > +
> > > + page_addr = page_address(pmd_page(*pmd));
> > > + if (!memchr_inv(page_addr, PAGE_INUSE,
> > > + PMD_SIZE)) {
> > > + free_pagetable(pmd_page(*pmd),
> > > + get_order(PMD_SIZE), direct);
> > > +
> > > + /*
> > > + * This spin lock could be only
> > > + * taken in _pte_aloc_kernel in
> > > + * mm/memory.c and nowhere else
> > > + * (for arm64). Not sure if the
> > > + * function above can be called
> > > + * concurrently. In doubt,
> > > + * I am living it here for now,
> > > + * but it probably can be removed.
> > > + */
> > > + spin_lock(&init_mm.page_table_lock);
> > > + pmd_clear(pmd);
> > > + spin_unlock(&init_mm.page_table_lock);
> > > + }
> >
> > I don't think this is correct.
> >
> > If we're getting rid of a partial pmd, we *must* split the pmd.
> > Otherwise, we're leaving bits mapped that should not be. If we split the
> > pmd, we can free the individual pages as we would for a non-section
> > mapping.
> >
> > As above, we can't split block entries within live tables, so that will
> > be painful at best.
> >
> > If we can't split a pmd, hen we cannot free a partial pmd, and will need
> > to reject request to do so.
> >
> > The same comments (with s/pmu/pud/, etc) apply for the higher level
> > remove_p*d_table functions.
> >
> > [...]
>
> This only happens when we are clearing vmemmap memory.
Is that definitely the case?
Currently, I can't see what prevents adding 2M of memory, and then
removing the first 4K of that. We'll use a 2M section for the linear map
of that, but won't unmap the 4K when removing.
Likewise for the next level up, with s/2M/1G/ and s/4K/2M/.
> My understanding
> is that the whole hack of marking the content of partially unused areas
> with the 0xFD constant is exactly to avoid splitting the PMD, but instead
> to only clear the full area when we realize that there's no valid struct
> page in it anymore. When would this kind of use be source of problems?
I understand what's going on for the vmemmap. So long as we don't use
hotpluggable memory to back the vmemmap, that's fine.
As above, my concern is whether splitting a section can ever occur for
the linear map.
Thanks,
Mark.
^ permalink raw reply
* arm64: Question about warnings due to unspecified ASM operand width
From: Matthias Kaehlcke @ 2017-04-18 18:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418113849.GC27592@e104818-lin.cambridge.arm.com>
Hi Catalin,
El Tue, Apr 18, 2017 at 12:38:49PM +0100 Catalin Marinas ha dit:
> On Mon, Apr 17, 2017 at 06:31:56PM -0700, Matthias Kaehlcke wrote:
> > During my work on improving support for kernel builds with clang I
> > came across a bunch of warnings on arm64 builds about the width of
> > operands in assembly not being specified:
> >
> > arch/arm64/include/asm/arch_timer.h:92:46: error: value size does
> > not match register size specified by the constraint and modifier [-Werror,-Wasm-operand-widths]
> > asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
>
> I think the first step would be to test it against a newer kernel. The
> above code disappeared in 4.9 in favour of dedicated read_sysreg()
> macros which shouldn't give this warning (u64 __val).
Sure, it was just one example from the 4.4 kernel my project currently
uses. There are others in more recent kernel versions.
> > I understand that this is usually not a problem and might even be
> > desired to give the compiler more flexiblity in the use of the
> > available registers.
>
> I don't think arm64 would benefit from such ambiguity, so we should
> rather fix them. In practice there is no issue since the compiler cannot
> allocate two 32-bit variables in a 64-bit register.
Thanks for the clarification!
According to Ard there is the case of the msr/mrs instructions to
consider, but lets talk about this in the subthread of his reply.
Matthias
^ permalink raw reply
* [PATCH 2/2] arm64: dts: juno: add information about L1 and L2 caches
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492538279-16405-1-git-send-email-sudeep.holla@arm.com>
Commit a8d4636f96ad ("arm64: cacheinfo: Remove CCSIDR-based cache
information probing") removed mechanism to extract cache information
based on CCSIDR register as the architecture explicitly states no
inference about the actual sizes of caches based on CCSIDR registers.
Commit 9a802431c527 ("arm64: cacheinfo: add support to override cache
levels via device tree") had already provided options to override cache
information from the device tree.
This patch adds the information about L1 and L2 caches on all variants
of Juno platform.
Cc: Will Deacon <will.deacon@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
arch/arm64/boot/dts/arm/juno-r1.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno-r2.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno.dts | 42 +++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts b/arch/arm64/boot/dts/arm/juno-r1.dts
index 0033c59a64b5..0e8943ab94d7 100644
--- a/arch/arm64/boot/dts/arm/juno-r1.dts
+++ b/arch/arm64/boot/dts/arm/juno-r1.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts b/arch/arm64/boot/dts/arm/juno-r2.dts
index 218d0e4736a8..405e2fba025b 100644
--- a/arch/arm64/boot/dts/arm/juno-r2.dts
+++ b/arch/arm64/boot/dts/arm/juno-r2.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A72_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index bb2820ef3d5b..0220494c9b80 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -88,6 +88,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -99,6 +105,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -110,6 +122,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -121,6 +139,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -132,6 +156,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -143,6 +173,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -151,10 +187,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] arm64: dts: juno: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes the following set of warnings on juno.
smb at 08000000 unit name should not have leading 0s
sysctl at 020000 simple-bus unit address format error, expected "20000"
apbregs at 010000 simple-bus unit address format error, expected "10000"
mmci at 050000 simple-bus unit address format error, expected "50000"
kmi at 060000 simple-bus unit address format error, expected "60000"
kmi at 070000 simple-bus unit address format error, expected "70000"
wdt at 0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +-
arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index 8ffaff2043d0..bfe7d683a42e 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -699,7 +699,7 @@
<0x00000008 0x80000000 0x1 0x80000000>;
};
- smb at 08000000 {
+ smb at 8000000 {
compatible = "simple-bus";
#address-cells = <2>;
#size-cells = <1>;
diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
index 098601657f82..2ac43221ddb6 100644
--- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
@@ -137,7 +137,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysctl: sysctl at 020000 {
+ v2m_sysctl: sysctl at 20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&mb_clk24mhz>;
@@ -148,7 +148,7 @@
assigned-clock-parents = <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>;
};
- apbregs at 010000 {
+ apbregs at 10000 {
compatible = "syscon", "simple-mfd";
reg = <0x010000 0x1000>;
@@ -216,7 +216,7 @@
};
};
- mmci at 050000 {
+ mmci at 50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <5>;
@@ -228,7 +228,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi at 060000 {
+ kmi at 60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <8>;
@@ -236,7 +236,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi at 070000 {
+ kmi at 70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <8>;
@@ -244,7 +244,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- wdt at 0f0000 {
+ wdt at f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x10000>;
interrupts = <7>;
--
2.7.4
^ permalink raw reply related
* [PATCH] ARM: dts: vexpress: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:45 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes the following set of warnings on vexpress platforms:
sysreg at 010000 simple-bus unit address format error, expected "10000"
sysctl at 020000 simple-bus unit address format error, expected "20000"
i2c at 030000 simple-bus unit address format error, expected "30000"
aaci at 040000 simple-bus unit address format error, expected "40000"
mmci at 050000 simple-bus unit address format error, expected "50000"
kmi at 060000 simple-bus unit address format error, expected "60000"
kmi at 070000 simple-bus unit address format error, expected "70000"
uart at 090000 simple-bus unit address format error, expected "90000"
uart at 0a0000 simple-bus unit address format error, expected "a0000"
uart at 0b0000 simple-bus unit address format error, expected "b0000"
uart at 0c0000 simple-bus unit address format error, expected "c0000"
wdt at 0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2m.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts | 18 +++++++++---------
arch/arm/boot/dts/vexpress-v2p-ca5s.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca9.dts | 2 +-
6 files changed, 36 insertions(+), 36 deletions(-)
Hi,
I observed few warning in linux-next due to the enhanced DTC checks
introduced with DTC upgrade in linux-next. The patch fixes few warnings
Regards,
Sudeep
diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index 3086efacd00e..35714ff6f467 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -71,7 +71,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysreg: sysreg at 010000 {
+ v2m_sysreg: sysreg at 10000 {
compatible = "arm,vexpress-sysreg";
reg = <0x010000 0x1000>;
@@ -94,7 +94,7 @@
};
};
- v2m_sysctl: sysctl at 020000 {
+ v2m_sysctl: sysctl at 20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -106,7 +106,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c at 030000 {
+ v2m_i2c_pcie: i2c at 30000 {
compatible = "arm,versatile-i2c";
reg = <0x030000 0x1000>;
@@ -119,7 +119,7 @@
};
};
- aaci at 040000 {
+ aaci at 40000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x040000 0x1000>;
interrupts = <11>;
@@ -127,7 +127,7 @@
clock-names = "apb_pclk";
};
- mmci at 050000 {
+ mmci at 50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <9 10>;
@@ -139,7 +139,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi at 060000 {
+ kmi at 60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <12>;
@@ -147,7 +147,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi at 070000 {
+ kmi at 70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <13>;
@@ -155,7 +155,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart at 090000 {
+ v2m_serial0: uart at 90000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x090000 0x1000>;
interrupts = <5>;
@@ -163,7 +163,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart at 0a0000 {
+ v2m_serial1: uart at a0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a0000 0x1000>;
interrupts = <6>;
@@ -171,7 +171,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart at 0b0000 {
+ v2m_serial2: uart at b0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b0000 0x1000>;
interrupts = <7>;
@@ -179,7 +179,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart at 0c0000 {
+ v2m_serial3: uart at c0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c0000 0x1000>;
interrupts = <8>;
@@ -187,7 +187,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt at 0f0000 {
+ wdt at f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index c6393d3f1719..1b6f6393be93 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -70,7 +70,7 @@
#size-cells = <1>;
ranges = <0 7 0 0x20000>;
- v2m_sysreg: sysreg at 00000 {
+ v2m_sysreg: sysreg at 0 {
compatible = "arm,vexpress-sysreg";
reg = <0x00000 0x1000>;
@@ -93,7 +93,7 @@
};
};
- v2m_sysctl: sysctl at 01000 {
+ v2m_sysctl: sysctl at 1000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x01000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -105,7 +105,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c at 02000 {
+ v2m_i2c_pcie: i2c at 2000 {
compatible = "arm,versatile-i2c";
reg = <0x02000 0x1000>;
@@ -118,7 +118,7 @@
};
};
- aaci at 04000 {
+ aaci at 4000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x04000 0x1000>;
interrupts = <11>;
@@ -126,7 +126,7 @@
clock-names = "apb_pclk";
};
- mmci at 05000 {
+ mmci at 5000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x05000 0x1000>;
interrupts = <9 10>;
@@ -138,7 +138,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi at 06000 {
+ kmi at 6000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x06000 0x1000>;
interrupts = <12>;
@@ -146,7 +146,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi at 07000 {
+ kmi at 7000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x07000 0x1000>;
interrupts = <13>;
@@ -154,7 +154,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart at 09000 {
+ v2m_serial0: uart at 9000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x09000 0x1000>;
interrupts = <5>;
@@ -162,7 +162,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart at 0a000 {
+ v2m_serial1: uart at a000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a000 0x1000>;
interrupts = <6>;
@@ -170,7 +170,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart at 0b000 {
+ v2m_serial2: uart at b000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b000 0x1000>;
interrupts = <7>;
@@ -178,7 +178,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart at 0c000 {
+ v2m_serial3: uart at c000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c000 0x1000>;
interrupts = <8>;
@@ -186,7 +186,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt at 0f000 {
+ wdt at f000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
index 15f4fd3f4695..0c8de0ca73ee 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
@@ -220,7 +220,7 @@
};
};
- smb at 08000000 {
+ smb at 8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
index bd107c5a0226..65ecf206388c 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
@@ -385,7 +385,7 @@
};
};
- etb at 0,20010000 {
+ etb at 20010000 {
compatible = "arm,coresight-etb10", "arm,primecell";
reg = <0 0x20010000 0 0x1000>;
@@ -399,7 +399,7 @@
};
};
- tpiu at 0,20030000 {
+ tpiu at 20030000 {
compatible = "arm,coresight-tpiu", "arm,primecell";
reg = <0 0x20030000 0 0x1000>;
@@ -449,7 +449,7 @@
};
};
- funnel at 0,20040000 {
+ funnel at 20040000 {
compatible = "arm,coresight-funnel", "arm,primecell";
reg = <0 0x20040000 0 0x1000>;
@@ -513,7 +513,7 @@
};
};
- ptm at 0,2201c000 {
+ ptm at 2201c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201c000 0 0x1000>;
@@ -527,7 +527,7 @@
};
};
- ptm at 0,2201d000 {
+ ptm at 2201d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201d000 0 0x1000>;
@@ -541,7 +541,7 @@
};
};
- etm at 0,2203c000 {
+ etm at 2203c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203c000 0 0x1000>;
@@ -555,7 +555,7 @@
};
};
- etm at 0,2203d000 {
+ etm at 2203d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203d000 0 0x1000>;
@@ -569,7 +569,7 @@
};
};
- etm at 0,2203e000 {
+ etm at 2203e000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203e000 0 0x1000>;
@@ -583,7 +583,7 @@
};
};
- smb at 08000000 {
+ smb at 8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
index 1acecaf4b13d..6e69b8e6c1a7 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
@@ -190,7 +190,7 @@
};
};
- smb at 08000000 {
+ smb at 8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index b608a03ee02f..c9305b58afc2 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -300,7 +300,7 @@
};
};
- smb at 04000000 {
+ smb at 4000000 {
compatible = "simple-bus";
#address-cells = <2>;
--
2.7.4
^ permalink raw reply related
* [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-18 17:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan@linaro.org>
On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
This is coming along well - a few comment below. In your next revision please
add GKH to the 'To' list.
Thanks,
Mathieu
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> coresight-etm4x-sysfs.o
> obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/amba/bus.h>
> +#include <linux/coresight.h>
> +#include <linux/cpu.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR 0x0A0
> +#define EDCIDSR 0x0A4
> +#define EDVIDSR 0x0A8
> +#define EDPCSR_HI 0x0AC
> +#define EDOSLAR 0x300
> +#define EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* bits definition for EDPRSR */
> +#define EDPRSR_DLK BIT(6)
> +#define EDPRSR_PU BIT(0)
> +
> +/* bits definition for EDVIDSR */
> +#define EDVIDSR_NS BIT(31)
> +#define EDVIDSR_E2 BIT(30)
> +#define EDVIDSR_E3 BIT(29)
> +#define EDVIDSR_HV BIT(28)
> +#define EDVIDSR_VMID GENMASK(7, 0)
> +
> +/*
> + * bits definition for EDDEVID1:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> e struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
For this driver we have a debugFS interface so I question the validity of a
kernel module parameter. Other than adding complexity to the code it offers no
real added value. If a user is to insmod a module, it is just as easy to switch
on the functionality using debugFS in a second step.
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
Here the wmb is to make sure reordering (either at compile time or in the CPU)
doesn't happend. You need a comment here otherwise checkpatch will complain.
Speaking of which, did you run this through checkpatch?
> + wmb();
> +}
> +
> +/*
> + * According to ARM DDI 0487A.k, before access external debug
> + * registers should firstly check the access permission; if any
> + * below condition has been met then cannot access debug
> + * registers to avoid lockup issue:
> + *
> + * - CPU power domain is powered off;
> + * - The OS Double Lock is locked;
> + *
> + * By checking EDPRSR can get to know if meet these conditions.
> + */
> +static bool debug_access_permitted(struct debug_drvdata *drvdata)
> +{
> + /* CPU is powered off */
> + if (!(drvdata->edprsr & EDPRSR_PU))
> + return false;
> +
> + /* The OS Double Lock is locked */
> + if (drvdata->edprsr & EDPRSR_DLK)
> + return false;
> +
> + return true;
> +}
> +
> +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
/* The core power domain got switched off on use, try again */
if (unlikely(!drvdata->edprsr & EDPRSR_PU))
goto try_again;
I understand you don't want to introduce a infinite loop but if that happens
here, something else has gone very wrong. The above readx_poll_timeout_atomic
loop should take care of bailing out in case of problems. That way you also get
to rid of the retried variable and the code is more simple.
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> +
> + /*
> + * As described in ARM DDI 0487A.k, if the processing
> + * element (PE) is in debug state, or sample-based
> + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> + * UNKNOWN state. So directly bail out for this case.
> + */
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED)
> + goto out;
> +
> + /*
> + * A read of the EDPCSR normally has the side-effect of
> + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> + * at this point it's safe to read value from them.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + arm_inst_offset = 8;
> + thumb_inst_offset = 4;
> + }
> +
> + /* Handle thumb instruction */
> + if (pc & EDPCSR_THUMB) {
> + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> + return pc;
> + }
> +
> + /*
> + * Handle arm instruction offset, if the arm instruction
> + * is not 4 byte alignment then it's possible the case
> + * for implementation defined; keep original value for this
> + * case and print info for notice.
> + */
> + if (pc & BIT(1))
> + pr_emerg("Instruction offset is implementation defined\n");
> + else
> + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> +
> + return pc;
> +}
> +
> +static void debug_dump_regs(struct debug_drvdata *drvdata)
> +{
> + unsigned long pc;
> +
> + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> +
> + if (!debug_access_permitted(drvdata)) {
> + pr_emerg("No permission to access debug registers!\n");
> + return;
> + }
> +
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> + return;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
> + drvdata->edvidsr,
> + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
I don't think you need to install the handler every time the functionality is
switched on/off. I suggest to install the handler at boot time (or module
insertion time) and in the notifier handler, check the debug_enable flag before
moving on with the output.
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
Using a true/false value is probably better here. That way you don't end up
with miscellaneous values in debugFS.
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
snprintf()
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
Please align this properly.
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> + void __iomem *base;
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata;
> + struct resource *res = &adev->res;
> + struct device_node *np = adev->dev.of_node;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, drvdata);
> +
> + /* Validity for the resource is already checked by the AMBA core */
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + drvdata->base = base;
> +
> + get_online_cpus();
> + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT
From: Matthias Kaehlcke @ 2017-04-18 17:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu_DHBT=2YA9h+ao3pZXgEV5KsO9ia+J90wtLuDX=a4+eg@mail.gmail.com>
El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> The operand is an integer constant, make the constness explicit by
> >> adding the modifier. This is needed for clang to generate valid code
> >> and also works with gcc.
> >
> > Actually it doesn't work with all gcc. I've got an older arm64 toolchain that I
> > only use for syntax checking (and hence I don't care if it is the latest and
> > greatest) and this commit breaks it:
> >
> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> > operand prefix '%c'
> > asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
Sorry about that :(
> > I'm currently reverting this change locally so I can continue to use the old
> > toolchain:
> >
> > $ aarch64-linux-gnu-gcc --version
> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> > Copyright (C) 2013 Free Software Foundation, Inc.
> >
> > $ aarch64-linux-gnu-as --version
> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> > 2013.11) 2.24.0.20131220
> > Copyright 2013 Free Software Foundation, Inc.
> >
> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >
>
> Thanks for the report. I think we care more about GCC 4.8 than about
> Clang, which argues for reverting this patch.
I agree, we certainly shouldn't break GCC.
> I understand these issues must be frustrating if you are working on
> this stuff, but to me, it is not entirely obvious why we want to
> support Clang in the first place (i.e., what does it buy you if your
> distro/environment is not already using Clang for userland),
There are environments that use Clang for userland, like Chrome OS or
Android. Debian releases with GCC, but also does Clang builds of most
of its repository: http://clang.debian.net/
I would also argue that Linux directly benefits from additional error
checks provided by Clang.
> and why the burden is on Linux to make modifications to support
> Clang, especially when it comes to GCC extensions such as inline
> assembly syntax.
Personally I'm on the pragmatic side. 99.9x% of the kernel already
builds with Clang (at least for arm64 and x86), only a very tiny
fraction of the code needs adaptation, which should ideally result in
the same code for gcc and clang. And it's not like Clang is just
another proprietary or niche compiler, it's the other big open source
compiler out there, since the effort is relatively small it seems
desirable to support it without out-of-tree patches. In parallel there
is also work ongoing with upstream Clang to improve compatiblity.
> It is ultimately up to the maintainers to decide what to do with this
> patch, but my vote would be to revert it, especially given that the %c
> placeholder prefix is not documented anywhere, and appears to simply
> trigger some GCC internals that happen to do the right thing in this
> case.
>
> However, the I -> i change is arguably an improvement, and considering
> that the following
>
> asm("foo: .long %0" :: "i"(some value))
>
> doesn't compile with clang either, I suggest you (Matthias) file a bug
> against Clang to get this fixed, and we can propose another patch just
> for the I->i change.
Ok, I will see if we can get this fixed in Clang.
Thanks
Matthias
^ permalink raw reply
* [PATCHv3 08/14] drivers/perf: arm_pmu: split cpu-local irq request/free
From: Geert Uytterhoeven @ 2017-04-18 17:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1491899997-32210-9-git-send-email-mark.rutland@arm.com>
Hi Mark,
On Tue, Apr 11, 2017 at 10:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Currently we have functions to request/free all IRQs for a given PMU.
> While this works today, this won't work for ACPI, where we don't know
> the full set of IRQs up front, and need to request them separately.
>
> To enable supporting ACPI, this patch splits out the cpu-local
> request/free into new functions, allowing us to request/free individual
> IRQs.
>
> As this makes it possible/necessary to request a PPI once per cpu, an
> additional check is added to detect mismatched PPIs. This shouldn't
> matter for the DT / platform case, as we check this when parsing.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
This patch causes warnings during PSCI system suspend on R-Car Gen3.
On R-Car M3-W (Dual CA57):
Disabling non-boot CPUs ...
+IRQ15 no longer affine to CPU1
CPU1: shutdown
psci: CPU1 killed.
On R-Car H3 (Quad CA57):
Disabling non-boot CPUs ...
+IRQ15 no longer affine to CPU1
CPU1: shutdown
psci: CPU1 killed.
+IRQ16 no longer affine to CPU2
CPU2: shutdown
psci: CPU2 killed.
+IRQ17 no longer affine to CPU3
CPU3: shutdown
psci: CPU3 killed.
Unfortunately it can't be reverted easily.
Do you have any clue?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v24 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver
From: Lorenzo Pieralisi @ 2017-04-18 17:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170414184014.8524-10-fu.wei@linaro.org>
On Sat, Apr 15, 2017 at 02:40:12AM +0800, fu.wei at linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> On platforms booting with ACPI, architected memory-mapped timers'
> configuration data is provided by firmware through the ACPI GTDT
> static table.
>
> The clocksource architected timer kernel driver requires a firmware
> interface to collect timer configuration and configure its driver.
> this infrastructure is present for device tree systems, but it is
> missing on systems booting with ACPI.
>
> Implement the kernel infrastructure required to parse the static
> ACPI GTDT table so that the architected timer clocksource driver can
> make use of it on systems booting with ACPI, therefore enabling
> the corresponding timers configuration.
>
> Signed-off-by: Fu Wei <fu.wei@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> drivers/acpi/arm64/gtdt.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/acpi.h | 1 +
> 2 files changed, 145 insertions(+)
>
> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> index 3d95af8..c9ef9c2 100644
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -13,6 +13,7 @@
>
> #include <linux/acpi.h>
> #include <linux/init.h>
> +#include <linux/irqdomain.h>
> #include <linux/kernel.h>
>
> #include <clocksource/arm_arch_timer.h>
> @@ -37,6 +38,28 @@ struct acpi_gtdt_descriptor {
>
> static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>
> +static inline void *next_platform_timer(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + platform_timer += gh->length;
> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> + return platform_timer;
> +
> + return NULL;
> +}
> +
> +#define for_each_platform_timer(_g) \
> + for (_g = acpi_gtdt_desc.platform_timer; _g; \
> + _g = next_platform_timer(_g))
> +
> +static inline bool is_timer_block(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
> +}
> +
> static int __init map_gt_gsi(u32 interrupt, u32 flags)
> {
> int trigger, polarity;
> @@ -155,3 +178,124 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
>
> return 0;
> }
> +
> +static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block *block,
> + struct arch_timer_mem *timer_mem)
> +{
> + int i;
> + struct arch_timer_mem_frame *frame;
> + struct acpi_gtdt_timer_entry *gtdt_frame;
> +
> + if (!block->timer_count) {
> + pr_err(FW_BUG "GT block present, but frame count is zero.");
> + return -ENODEV;
> + }
> +
> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 8\n",
> + block->timer_count);
> + return -EINVAL;
> + }
> +
> + timer_mem->cntctlbase = (phys_addr_t)block->block_address;
> + /*
> + * The CNTCTLBase frame is 4KB (register offsets 0x000 - 0xFFC).
> + * See ARM DDI 0487A.k_iss10775, page I1-5129, Table I1-3
> + * "CNTCTLBase memory map".
> + */
> + timer_mem->size = SZ_4K;
> +
> + gtdt_frame = (void *)block + block->timer_offset;
> + if (gtdt_frame + block->timer_count != (void *)block + block->header.length)
> + return -EINVAL;
> +
> + /*
> + * Get the GT timer Frame data for every GT Block Timer
> + */
> + for (i = 0; i < block->timer_count; i++, gtdt_frame++) {
> + if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER)
> + continue;
> +
> + if (!gtdt_frame->base_address || !gtdt_frame->timer_interrupt)
> + goto error;
> +
> + frame = &timer_mem->frame[gtdt_frame->frame_number];
> + frame->phys_irq = map_gt_gsi(gtdt_frame->timer_interrupt,
> + gtdt_frame->timer_flags);
> + if (frame->phys_irq <= 0) {
> + pr_warn("failed to map physical timer irq in frame %d.\n",
> + gtdt_frame->frame_number);
> + goto error;
> + }
> +
> + if (gtdt_frame->virtual_timer_interrupt) {
> + frame->virt_irq =
> + map_gt_gsi(gtdt_frame->virtual_timer_interrupt,
> + gtdt_frame->virtual_timer_flags);
> + if (frame->virt_irq <= 0) {
> + pr_warn("failed to map virtual timer irq in frame %d.\n",
> + gtdt_frame->frame_number);
> + goto error;
> + }
> + } else {
> + frame->virt_irq = 0;
> + pr_debug("virtual timer in frame %d not implemented.\n",
> + gtdt_frame->frame_number);
> + }
> +
> + frame->cntbase = gtdt_frame->base_address;
> + /*
> + * The CNTBaseN frame is 4KB (register offsets 0x000 - 0xFFC).
> + * See ARM DDI 0487A.k_iss10775, page I1-5130, Table I1-4
> + * "CNTBaseN memory map".
> + */
> + frame->size = SZ_4K;
> + frame->valid = true;
> + }
> +
> + return 0;
> +
> +error:
> + for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
> + frame = &timer_mem->frame[i];
> + if (frame->phys_irq > 0)
> + acpi_unregister_irq(frame->phys_irq);
> + if (frame->virt_irq > 0)
> + acpi_unregister_irq(frame->virt_irq);
> + }
There are three error paths, none of them reset [i,gtdt_frame],
correct ?
If yes, why can't it simply be written like this ?
for (; i >= 0; i--, gtdt_frame--) {
frame = &timer_mem->frame[gtdt_frame->frame_number];
/* not sure this check is actually needed */
if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER)
continue;
if (frame->phys_irq > 0)
acpi_unregister_gsi(gtdt_frame->timer_interrupt);
if (frame->virt_irq > 0)
acpi_unregister_gsi(gtdt_frame->virtual_timer_interrupt);
}
It is time we merged this code, that's for certain so if for any reason
loop above does not work this current series is ok for me, go ahead, I
just don't see the point of adding another ACPI IRQ function
acpi_unregister_irq() (which does not mean acpi_unregister_gsi() is sane
- we just lived with it when ACPI ARM64 was merged because changing its
API requires reworking x86 legacy code that I don't understand).
Thanks,
Lorenzo
> + return -EINVAL;
> +}
> +
> +/**
> + * acpi_arch_timer_mem_init() - Get the info of all GT blocks in GTDT table.
> + * @timer_mem: The pointer to the array of struct arch_timer_mem for returning
> + * the result of parsing. The element number of this array should
> + * be platform_timer_count(the total number of platform timers).
> + * @timer_count: It points to a integer variable which is used for storing the
> + * number of GT blocks we have parsed.
> + *
> + * Return: 0 if success, -EINVAL/-ENODEV if error.
> + */
> +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
> + int *timer_count)
> +{
> + int ret;
> + void *platform_timer;
> +
> + *timer_count = 0;
> + for_each_platform_timer(platform_timer) {
> + if (is_timer_block(platform_timer)) {
> + ret = gtdt_parse_timer_block(platform_timer, timer_mem);
> + if (ret)
> + return ret;
> + timer_mem++;
> + (*timer_count)++;
> + }
> + }
> +
> + if (*timer_count)
> + pr_info("found %d memory-mapped timer block(s).\n",
> + *timer_count);
> +
> + return 0;
> +}
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 728d1ed..4b1ab65 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -606,6 +606,7 @@ int acpi_reconfig_notifier_unregister(struct notifier_block *nb);
> int acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count);
> int acpi_gtdt_map_ppi(int type);
> bool acpi_gtdt_c3stop(int type);
> +int acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem, int *timer_count);
> #endif
>
> #else /* !CONFIG_ACPI */
> --
> 2.9.3
>
^ permalink raw reply
* Panic in arch_hw_breakpoint_init() on Cortex-A9 (SPEAr1340)
From: Lubomir Rintel @ 2017-04-18 17:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418130326.GE17866@leverpostej>
On Tue, 2017-04-18 at 14:03 +0100, Mark Rutland wrote:
> On Tue, Apr 18, 2017 at 12:44:28PM +0200, Lubomir Rintel wrote:
> > Hi,
>
> Hi,
>
> > I'm getting a crash that looks awfully lot like what ddc37832a1 [ARM:
> > 8634/1: hw_breakpoint: blacklist Scorpion CPUs] aims to fix.
>
> Just to check, have you successfully booted a kernel on this board
> before? i.e. is this a new crash?
I've now checked, and it doesn't seem like it ever worked when
PERF_EVENTS (and in turn HAVE_HW_BREAKPOINT) is enabled. The crash look
the same up to 4.7; the previous versions also crash as soon as I
enable the options but for some reason I don't get an oops on the
console even with earlyprintk enabled (the machine reboots with panic=
argument, so there's just possibly just something wrong with the
console).
> > My hardware doesn't use a Scorpion CPU though -- it uses a Cortex-A9
> > (cpuid=0x412fc091).
> >
> > [????0.157000] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
> > [????0.157000] Modules linked in:
> > [????0.157000] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-0.rc3.git0.2.spear2.fc26.armv7hl #1
> > [????0.157000] Hardware name: ST SPEAr1340 SoC with Flattened Device Tree
> > [????0.157000] task: ef102b80 task.stack: ef0fa000
> > [????0.157000] PC is at arch_hw_breakpoint_init+0x130/0x2a0
> > [????0.157000] LR is at arch_hw_breakpoint_init+0x7c/0x2a0
> > [????0.157000] pc : [<c0e0538c>]????lr : [<c0e052d8>]????psr: 60000013
>
> It would be helpful to know which specific access that is.
>
> Can you figure that out with addr2line, e.g.
>
> ?$ addr2line -ife vmlinux c0e0538c
Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc7 #21
Hardware name: ST SPEAr1340 SoC with Flattened Device Tree
task: ef058000 task.stack: ef04c000
PC is at arch_hw_breakpoint_init+0x9c/0x2a4
LR is at arch_hw_breakpoint_init+0x84/0x2a4
pc : [<c0905504>]????lr : [<c09054ec>]????psr: 60000013
$ addr2line -ife vmlinux c0905504
core_has_os_save_restore
/home/lkundrak/spear/linux/arch/arm/kernel/hw_breakpoint.c:920
arch_hw_breakpoint_init
/home/lkundrak/spear/linux/arch/arm/kernel/hw_breakpoint.c:1082
>
> Thanks,
> Mark
^ permalink raw reply
* [PATCH] Revert "arm64: Increase the max granular size"
From: Sunil Kovvuri @ 2017-04-18 17:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418144839.GF27592@e104818-lin.cambridge.arm.com>
On Tue, Apr 18, 2017 at 8:18 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Mon, Apr 17, 2017 at 04:08:52PM +0530, Sunil Kovvuri wrote:
>> >> >> Do you have an explanation on the performance variation when
>> >> >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>> >> >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>> >> >> non-coherent DMA?).
>> >> >
>> >> > network stack use SKB_DATA_ALIGN to align.
>> >> > ---
>> >> > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
>> >> > ~(SMP_CACHE_BYTES - 1))
>> >> >
>> >> > #define SMP_CACHE_BYTES L1_CACHE_BYTES
>> >> > ---
>> >> > I think this is the reason of performance regression.
>> >> >
>> >>
>> >> Yes this is the reason for performance regression. Due to increases L1 cache alignment the
>> >> object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
>> >> 4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
>>
>> With what traffic did you check 'skb->truesize' ?
>> Increase from 2304 to 4352 bytes doesn't seem to be real. I checked
>> with ICMP pkts with maximum
>> size possible with 1500byte MTU and I don't see such a bump. If the
>> bump is observed with Iperf
>> sending TCP packets then I suggest to check if TSO is playing a part over here.
>
> I haven't checked truesize but I added some printks to __alloc_skb() (on
> a Juno platform) and the size argument to this function is 1720 on many
> occasions. With sizeof(struct skb_shared_info) of 320, the actual data
> allocation is exactly 2048 when using 64 byte L1_CACHE_SIZE. With a
> 128 byte cache size, it goes slightly over 2K, hence the 4K slab
> allocation.
Understood but still in my opinion this '4K slab allocation' cannot be
considered as
an issue with cache line size, there are many network drivers out
there which do
receive buffer or page recycling to minimize (sometimes almost to
zero) the cost
of buffer allocation.
>The 1720 figure surprised me a bit as well since I was
> expecting something close to 1500.
>
> The thing that worries me is that skb->data may be used as a buffer to
> DMA into. If that's the case, skb_shared_info is wrongly aligned based
> on SMP_CACHE_BYTES only and can lead to corruption on a non-DMA-coherent
> platform. It should really be ARCH_DMA_MINALIGN.
I didn't get this, if you see __alloc_skb()
229 size = SKB_DATA_ALIGN(size);
230 size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
both DMA buffer and skb_shared_info are aligned to a cacheline separately,
considering 128byte alignment guarantees 64byte alignment as well, how
will this
lead to corruption ?
And if platform is non-DMA-coherent then again it's the driver which
should take
of coherency by using appropriate map/unmap APIs and should avoid any cacheline
sharing btw DMA buffer and skb_shared_info.
>
> IIUC, the Cavium platform has coherent DMA, so it shouldn't be an issue
> if we go back to 64 byte cache lines.
Yes, Cavium platform is DMA coherent and there is no issue with reverting back
to 64byte cachelines. But do we want to do this because some platform has a
performance issue and this is an easy way to solve it. IMHO there seems
to be many ways to solve performance degradation within the driver itself, and
if those doesn't work then probably it makes sense to revert this.
>However, we don't really have an
> easy way to check (maybe taint the kernel if CWG is different from
> ARCH_DMA_MINALIGN *and* the non-coherent DMA API is called).
>
> --
> Catalin
^ permalink raw reply
* [patch V2 11/24] ARM/hw_breakpoint: Use cpuhp_setup_state_cpuslocked()
From: Thomas Gleixner @ 2017-04-18 17:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418170442.665445272@linutronix.de>
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
arch_hw_breakpoint_init() holds get_online_cpus() while registerring the
hotplug callbacks.
cpuhp_setup_state() invokes get_online_cpus() as well. This is correct, but
prevents the conversion of the hotplug locking to a percpu rwsem.
Use cpuhp_setup_state_cpuslocked() to avoid the nested call.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org
---
arch/arm/kernel/hw_breakpoint.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -1098,8 +1098,9 @@ static int __init arch_hw_breakpoint_ini
* assume that a halting debugger will leave the world in a nice state
* for us.
*/
- ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
- dbg_reset_online, NULL);
+ ret = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
+ "arm/hw_breakpoint:online",
+ dbg_reset_online, NULL);
unregister_undef_hook(&debug_reg_hook);
if (WARN_ON(ret < 0) || !cpumask_empty(&debug_err_mask)) {
core_num_brps = 0;
^ permalink raw reply
* [patch V2 09/24] hwtracing/coresight-etm4x: Use cpuhp_setup_state_nocalls_cpuslocked()
From: Thomas Gleixner @ 2017-04-18 17:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418170442.665445272@linutronix.de>
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
etm_probe4() holds get_online_cpus() while invoking
cpuhp_setup_state_nocalls().
cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
correct, but prevents the conversion of the hotplug locking to a percpu
rwsem.
Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org
---
drivers/hwtracing/coresight/coresight-etm4x.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -990,12 +990,12 @@ static int etm4_probe(struct amba_device
dev_err(dev, "ETM arch init failed\n");
if (!etm4_count++) {
- cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
- "arm/coresight4:starting",
- etm4_starting_cpu, etm4_dying_cpu);
- ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
- "arm/coresight4:online",
- etm4_online_cpu, NULL);
+ cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
+ "arm/coresight4:starting",
+ etm4_starting_cpu, etm4_dying_cpu);
+ ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+ "arm/coresight4:online",
+ etm4_online_cpu, NULL);
if (ret < 0)
goto err_arch_supported;
hp_online = ret;
^ permalink raw reply
* [patch V2 08/24] hwtracing/coresight-etm3x: Use cpuhp_setup_state_nocalls_cpuslocked()
From: Thomas Gleixner @ 2017-04-18 17:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418170442.665445272@linutronix.de>
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
etm_probe() holds get_online_cpus() while invoking
cpuhp_setup_state_nocalls().
cpuhp_setup_state_nocalls() invokes get_online_cpus() as well. This is
correct, but prevents the conversion of the hotplug locking to a percpu
rwsem.
Use cpuhp_setup_state_nocalls_cpuslocked() to avoid the nested call.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: linux-arm-kernel at lists.infradead.org
---
drivers/hwtracing/coresight/coresight-etm3x.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x.c
@@ -803,12 +803,12 @@ static int etm_probe(struct amba_device
dev_err(dev, "ETM arch init failed\n");
if (!etm_count++) {
- cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
- "arm/coresight:starting",
- etm_starting_cpu, etm_dying_cpu);
- ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
- "arm/coresight:online",
- etm_online_cpu, NULL);
+ cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
+ "arm/coresight:starting",
+ etm_starting_cpu, etm_dying_cpu);
+ ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
+ "arm/coresight:online",
+ etm_online_cpu, NULL);
if (ret < 0)
goto err_arch_supported;
hp_online = ret;
^ permalink raw reply
* [PATCH] fs: Preventing READ_IMPLIES_EXEC Propagation
From: Catalin Marinas @ 2017-04-18 17:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2414e3b3-03f6-bd6c-5aa4-ad58c66b5aa5@huawei.com>
On Thu, Apr 13, 2017 at 08:33:52PM +0800, dongbo (E) wrote:
> From: Dong Bo <dongbo4@huawei.com>
>
> In load_elf_binary(), once the READ_IMPLIES_EXEC flag is set,
> the flag is propagated to its child processes, even the elf
> files are marked as not requiring executable stack. It may
> cause superfluous operations on some arch, e.g.
> __sync_icache_dcache on aarch64 due to a PROT_READ mmap is
> also marked as PROT_EXEC.
>
> Signed-off-by: Dong Bo <dongbo4@huawei.com>
> ---
> fs/binfmt_elf.c | 2 ++
> fs/binfmt_elf_fdpic.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5075fd5..c52e670 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -863,6 +863,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
> SET_PERSONALITY2(loc->elf_ex, &arch_state);
> if (elf_read_implies_exec(loc->elf_ex, executable_stack))
> current->personality |= READ_IMPLIES_EXEC;
> + else
> + current->personality &= ~READ_IMPLIES_EXEC;
> if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> current->flags |= PF_RANDOMIZE;
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index cf93a4f..c4bc4d0 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -354,6 +354,8 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
> set_personality(PER_LINUX);
> if (elf_read_implies_exec(&exec_params.hdr, executable_stack))
> current->personality |= READ_IMPLIES_EXEC;
> + else
> + current->personality &= ~READ_IMPLIES_EXEC;
> setup_new_exec(bprm);
That's affecting most architectures with a risk of ABI breakage. We
could do it on arm64 only, though I'm not yet clear on the ABI
implications (at a first look, there shouldn't be any). This follows the
x86_64 approach but unfortunately we haven't done it on arm64 from the
start:
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 5d1700425efe..5941e7f6ae60 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -142,6 +142,7 @@ typedef struct user_fpsimd_state elf_fpregset_t;
({ \
clear_bit(TIF_32BIT, ¤t->mm->context.flags); \
clear_thread_flag(TIF_32BIT); \
+ current->personality &= ~READ_IMPLIES_EXEC; \
})
/* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
--
Catalin
^ permalink raw reply related
* [PATCH v3 04/32] asm-generic: add ioremap_nopost() remap interface
From: Bjorn Helgaas @ 2017-04-18 16:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170418154937.GA1006@red-moon>
On Tue, Apr 18, 2017 at 10:49 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Apr 12, 2017 at 12:20:22PM +0100, Russell King - ARM Linux wrote:
>> On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote:
>> > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote:
>> > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset,
>> > > size_t size)
>> > > +{
>> > > + return ioremap_nocache(offset, size);
>> > > +}
>> > > +
>> >
>> > No this is wrong as I explained.
>> >
>> > This is a semantic that simply *cannot* be generically provided accross
>> > architectures as a mapping attribute.
>> >
>> > The solution to your problem lies elsewhere.
>>
>> I disagree. Sure, it may not be supportable across all architectures,
>> but we're not introducing something brand new here.
>>
>> What we're trying to do is to fix some _existing_ drivers that are
>> already using ioremap() to map the PCI IO and configuration spaces.
>> Maybe it would help if those drivers were part of this patch set,
>> rather than the patch set just introducing a whole new architecture
>> interface without really showing where the problem drivers are.
>>
>> The issue here is that if we make this new ioremap_nopost() fail by
>> returning NULL if an architecture does not provide an implementation,
>> then these drivers are going to start failing on such architectures.
>>
>> It is only safe to do that where we know for certain that the drivers
>> are not used - but I don't think that's the case here (it's difficult
>> to tell, because no drivers are updated in this series, so we don't
>> know which are going to be affected.)
>>
>> So, the question is:
>>
>> - is it better to provide a default implementation which provides the
>> functionality that existing drivers are _already_ using, albiet not
>> entirely correctly.
>>
>> or:
>>
>> - is it better to break drivers on architectures when they're converted
>> to fix up this issue.
>>
>> What, I suppose, we could do is not bother with a default implementation,
>> but instead litter drivers with:
>>
>> +#ifdef ioremap_post
>> + base = ioremap_post(...);
>> +#else
>> base = ioremap(...);
>> +#endif
>>
>> which gets around your objection - not providing a default that's weaker
>> than required, but also not breaking the drivers. The problem is that
>> we end up littering drivers with ifdefs.
>>
>> However, I'm willing to bet that the architectures that you're saying
>> will not be able to support the weaker semantic won't have any need to
>> use these drivers, or if they do, the drivers will need the method of
>> accessing stuff like PCI IO and configuration spaces radically altered.
>>
>> So, maybe the best solution is to not provide any kind of default
>> implementation, add a HAVE_IOREMAP_POST Kconfig symbol, have architectures
>> select that when they do provide ioremap_post(), and make the drivers
>> depend on that (so we don't end up trying to build these drivers on
>> architectures where they can never work.) Down side to that is reduced
>> build coverage.
>
> I can do that yes, which already means I have to know if eg microblaze
> (drivers/pci/host/pcie-xilinx.c) can provide a mapping with nonposted
> writes semantics, otherwise it is a dead-end.
>
> Another option would be going back to what v1 did, namely, to implement
> a pci_remap_cfgspace() interface (it is the _nopost() suffix that stirred
> debate - nobody would object to having a default pci_remap_cfgspace()
> implementation that defaults to ioremap_nocache(), I know Bjorn does not
> like it to be PCI specific, just adding an option on the table to make
> progress).
The reason I objected to pci_remap_cfgspace() was that I thought
ioremap_nopost() was implementable across arches. That turned out not
to be true, so I'm fine with calling it pci_remap_cfgspace(). Maybe
it's worth a note in the default implementation that arches should
override it with a non-postable version if they can.
Bjorn
^ permalink raw reply
* [PATCH v24 08/11] acpi: Introduce acpi_unregister_irq function
From: Mark Rutland @ 2017-04-18 15:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170414184014.8524-9-fu.wei@linaro.org>
On Sat, Apr 15, 2017 at 02:40:11AM +0800, fu.wei at linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> This patch introduces acpi_unregister_irq function to free a
> linux IRQ number<->GSI mapping by a given linux IRQ number.
>
> Even we have successfully registered the GSI, when some error occurs, we
> may need to unmap it for freeing the IRQ resource. But in some cases, we
> only have IRQ, but not GSI.
>
> This patch is the preparation for memory-mapped timer support in GTDT
> driver.
>
> Signed-off-by: Fu Wei <fu.wei@linaro.org>
> ---
> drivers/acpi/irq.c | 10 ++++++++++
> include/linux/acpi.h | 7 +++++++
> 2 files changed, 17 insertions(+)
This will need comments/acks from the ACPI folk.
Lorenzo, do you prefer this to direct use of irq_dispose_mapping() in
the GTDT parsing code?
Thanks,
Mark.
> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> index 830299a..59de777 100644
> --- a/drivers/acpi/irq.c
> +++ b/drivers/acpi/irq.c
> @@ -85,6 +85,16 @@ void acpi_unregister_gsi(u32 gsi)
> EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
>
> /**
> + * acpi_unregister_irq() - Free a linux IRQ number<->GSI mapping
> + * @irq: linux IRQ number
> + */
> +void acpi_unregister_irq(int irq)
> +{
> + irq_dispose_mapping(irq);
> +}
> +EXPORT_SYMBOL_GPL(acpi_unregister_irq);
> +
> +/**
> * acpi_get_irq_source_fwhandle() - Retrieve fwhandle from IRQ resource source.
> * @source: acpi_resource_source to use for the lookup.
> *
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4b5c146..728d1ed 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -336,6 +336,13 @@ extern int acpi_get_override_irq(u32 gsi, int *trigger, int *polarity);
> */
> void acpi_unregister_gsi (u32 gsi);
>
> +/*
> + * This function undoes the effect of one call to acpi_register_gsi().
> + * The irq should be the return value of acpi_register_gsi().
> + * If the irq is valid, free a linux IRQ number<->GSI mapping.
> + */
> +void acpi_unregister_irq(int irq);
> +
> struct pci_dev;
>
> int acpi_pci_irq_enable (struct pci_dev *dev);
> --
> 2.9.3
>
^ permalink raw reply
* [PATCH] Remove ARM errata Workarounds 458693 and 460075
From: Catalin Marinas @ 2017-04-18 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170416080446.GI17774@n2100.armlinux.org.uk>
On Sun, Apr 16, 2017 at 09:04:46AM +0100, Russell King - ARM Linux wrote:
> On Sat, Apr 15, 2017 at 07:06:06PM -0500, Nisal Menuka wrote:
> > According to ARM, these errata exist only in a version of Cortex-A8
> > (r2p0) which was never built. Therefore, I believe there are no platforms
> > where this workaround should be enabled.
> > link :http://infocenter.arm.com/help/index.jsp?topic=
> > /com.arm.doc.faqs/ka15634.html
>
> These were submitted by ARM Ltd back in 2009 - if the silicon was never
> built, there would've been no reason to submit them. Maybe Catalin can
> shed some light on this, being the commit author who introduced these?
We normally try not to submit errata workarounds for revisions that are
not going to be built/deployed. It's possible that at the time there
were plans for r2p0 to be licensed and built (not just FPGA) but I don't
really remember the details. The A8 errata document indeed states that
r1p0 and r2p0 are obsolete but this can mean many things (like not
available to license).
I'll try to see if any of the A8 past product managers know anything
about this. In the meantime, I would leave them in (no run-time
overhead).
--
Catalin
^ permalink raw reply
* [PATCH v3 04/32] asm-generic: add ioremap_nopost() remap interface
From: Lorenzo Pieralisi @ 2017-04-18 15:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170412112022.GY17774@n2100.armlinux.org.uk>
On Wed, Apr 12, 2017 at 12:20:22PM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 11, 2017 at 11:39:43PM +1000, Benjamin Herrenschmidt wrote:
> > On Tue, 2017-04-11 at 13:28 +0100, Lorenzo Pieralisi wrote:
> > > +static inline void __iomem *ioremap_nopost(phys_addr_t offset,
> > > size_t size)
> > > +{
> > > +???????return ioremap_nocache(offset, size);
> > > +}
> > > +
> >
> > No this is wrong as I explained.
> >
> > This is a semantic that simply *cannot* be generically provided accross
> > architectures as a mapping attribute.
> >
> > The solution to your problem lies elsewhere.
>
> I disagree. Sure, it may not be supportable across all architectures,
> but we're not introducing something brand new here.
>
> What we're trying to do is to fix some _existing_ drivers that are
> already using ioremap() to map the PCI IO and configuration spaces.
> Maybe it would help if those drivers were part of this patch set,
> rather than the patch set just introducing a whole new architecture
> interface without really showing where the problem drivers are.
>
> The issue here is that if we make this new ioremap_nopost() fail by
> returning NULL if an architecture does not provide an implementation,
> then these drivers are going to start failing on such architectures.
>
> It is only safe to do that where we know for certain that the drivers
> are not used - but I don't think that's the case here (it's difficult
> to tell, because no drivers are updated in this series, so we don't
> know which are going to be affected.)
>
> So, the question is:
>
> - is it better to provide a default implementation which provides the
> functionality that existing drivers are _already_ using, albiet not
> entirely correctly.
>
> or:
>
> - is it better to break drivers on architectures when they're converted
> to fix up this issue.
>
> What, I suppose, we could do is not bother with a default implementation,
> but instead litter drivers with:
>
> +#ifdef ioremap_post
> + base = ioremap_post(...);
> +#else
> base = ioremap(...);
> +#endif
>
> which gets around your objection - not providing a default that's weaker
> than required, but also not breaking the drivers. The problem is that
> we end up littering drivers with ifdefs.
>
> However, I'm willing to bet that the architectures that you're saying
> will not be able to support the weaker semantic won't have any need to
> use these drivers, or if they do, the drivers will need the method of
> accessing stuff like PCI IO and configuration spaces radically altered.
>
> So, maybe the best solution is to not provide any kind of default
> implementation, add a HAVE_IOREMAP_POST Kconfig symbol, have architectures
> select that when they do provide ioremap_post(), and make the drivers
> depend on that (so we don't end up trying to build these drivers on
> architectures where they can never work.) Down side to that is reduced
> build coverage.
I can do that yes, which already means I have to know if eg microblaze
(drivers/pci/host/pcie-xilinx.c) can provide a mapping with nonposted
writes semantics, otherwise it is a dead-end.
Another option would be going back to what v1 did, namely, to implement
a pci_remap_cfgspace() interface (it is the _nopost() suffix that stirred
debate - nobody would object to having a default pci_remap_cfgspace()
implementation that defaults to ioremap_nocache(), I know Bjorn does not
like it to be PCI specific, just adding an option on the table to make
progress).
Thanks,
Lorenzo
^ permalink raw reply
* [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT
From: Ard Biesheuvel @ 2017-04-18 15:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAP=VYLpejcYwXUMtCCkXmjXw=T1tBJLn21aVi-9v_VGmEinjRg@mail.gmail.com>
On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> The operand is an integer constant, make the constness explicit by
>> adding the modifier. This is needed for clang to generate valid code
>> and also works with gcc.
>
> Actually it doesn't work with all gcc. I've got an older arm64 toolchain that I
> only use for syntax checking (and hence I don't care if it is the latest and
> greatest) and this commit breaks it:
>
> arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> operand prefix '%c'
> asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
> I'm currently reverting this change locally so I can continue to use the old
> toolchain:
>
> $ aarch64-linux-gnu-gcc --version
> aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> GCC 2013.11) 4.8.3 20131202 (prerelease)
> Copyright (C) 2013 Free Software Foundation, Inc.
>
> $ aarch64-linux-gnu-as --version
> GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> 2013.11) 2.24.0.20131220
> Copyright 2013 Free Software Foundation, Inc.
>
> Maybe it is finally too old and nobody cares, but I thought it worth a mention.
>
Thanks for the report. I think we care more about GCC 4.8 than about
Clang, which argues for reverting this patch.
I understand these issues must be frustrating if you are working on
this stuff, but to me, it is not entirely obvious why we want to
support Clang in the first place (i.e., what does it buy you if your
distro/environment is not already using Clang for userland), and why
the burden is on Linux to make modifications to support Clang,
especially when it comes to GCC extensions such as inline assembly
syntax.
It is ultimately up to the maintainers to decide what to do with this
patch, but my vote would be to revert it, especially given that the %c
placeholder prefix is not documented anywhere, and appears to simply
trigger some GCC internals that happen to do the right thing in this
case.
However, the I -> i change is arguably an improvement, and considering
that the following
asm("foo: .long %0" :: "i"(some value))
doesn't compile with clang either, I suggest you (Matthias) file a bug
against Clang to get this fixed, and we can propose another patch just
for the I->i change.
--
Ard.
>>
>> Also change the constraint of the operand from 'I' ("Integer constant
>> that is valid as an immediate operand in an ADD instruction", AArch64)
>> to 'i' ("An immediate integer operand").
>>
>> Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> Changes in v2:
>> - Changed operand constraint from I to i
>> - Updated commit message
>> - Changed 'From' tag to 'Based-on-patch-from'
>>
>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>> index aefda9868627..6b520e3f3ab1 100644
>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>>
>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>> index 7cd587564a41..e3abe11de48c 100644
>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>>
>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> --
>> 2.12.2.715.g7642488e1d-goog
>>
^ permalink raw reply
* [PATCH v6 5/8] coresight: use const for device_node structures
From: Mathieu Poirier @ 2017-04-18 15:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1491485461-22800-6-git-send-email-leo.yan@linaro.org>
On Thu, Apr 06, 2017 at 09:30:58PM +0800, Leo Yan wrote:
> Almost low level functions from open firmware have used const to
> qualify device_node structures, so add const for device_node
> parameters in of_coresight related functions.
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
I agree with these changes but the patch needs to be split up - please see
below.
> ---
> drivers/hwtracing/coresight/of_coresight.c | 6 +++---
> include/linux/coresight.h | 8 ++++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 78d2399..46eec0f 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -52,7 +52,7 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> endpoint, of_dev_node_match);
> }
>
> -static void of_coresight_get_ports(struct device_node *node,
> +static void of_coresight_get_ports(const struct device_node *node,
> int *nr_inport, int *nr_outport)
Move this to a patch by itself as it is not related to this driver.
> {
> struct device_node *ep = NULL;
> @@ -101,7 +101,7 @@ static int of_coresight_alloc_memory(struct device *dev,
> return 0;
> }
>
> -int of_coresight_get_cpu(struct device_node *node)
> +int of_coresight_get_cpu(const struct device_node *node)
Move this to the previous patch in this set. There is not need to undo what you
just did there.
> {
> int cpu;
> bool found;
> @@ -128,7 +128,7 @@ int of_coresight_get_cpu(struct device_node *node)
> EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>
> struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node)
> + struct device *dev, const struct device_node *node)
Same here, move this to a new patch (the same one as of_coresight_get_ports()).
> {
> int i = 0, ret = 0;
> struct coresight_platform_data *pdata;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index bf96678..4915254 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -263,13 +263,13 @@ static inline int coresight_timeout(void __iomem *addr, u32 offset,
> #endif
>
> #ifdef CONFIG_OF
> -extern int of_coresight_get_cpu(struct device_node *node);
> +extern int of_coresight_get_cpu(const struct device_node *node);
> extern struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node);
> + struct device *dev, const struct device_node *node);
> #else
> -static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
> +static inline int of_coresight_get_cpu(const struct device_node *node) { return 0; }
> static inline struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node) { return NULL; }
> + struct device *dev, const struct device_node *node) { return NULL; }
> #endif
>
> #ifdef CONFIG_PID_NS
> --
> 2.7.4
>
^ 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