* Re: [PATCH v6 3/4] dt-bindings: arm: fsl: Add Kontron i.MX6UL N6310 compatibles
From: Krzysztof Kozlowski @ 2019-08-21 17:54 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer,
linux-kernel@vger.kernel.org, Schrempf Frieder, NXP Linux Team,
Pengutronix Kernel Team, Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_JsqKBWB2FiVjYo9O7DPw1JYJvan7uRgbR0VBG=FfHDVYdZQ@mail.gmail.com>
On Tue, Aug 20, 2019 at 03:27:39PM -0500, Rob Herring wrote:
> > I see. If I understand the schema correctly, this should look like:
>
> Looks correct, but a couple of comments.
>
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> > index 7294ac36f4c0..eb263d1ccf13 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > @@ -161,6 +161,22 @@ properties:
> > items:
> > - enum:
> > - fsl,imx6ul-14x14-evk # i.MX6 UltraLite 14x14 EVK Board
> > + - kontron,imx6ul-n6310-som # Kontron N6310 SOM
>
> Is the SOM ever used alone? If not, then no point in listing this here.
SoM alone: no, because it requires some type of base board. However it
will be used by some customer designs with some amount of
changes/addons.
Looking at other aproaches, usually SoMs have their own compatible. In
such case - I should document it somewhere.
>
> > + - const: fsl,imx6ul
> > +
> > + - description: Kontron N6310 S Board
> > + items:
> > + - enum:
> > + - kontron,imx6ul-n6310-s
>
> This could be a 'const' instead. It depends if you think there will
> ever be more than one entry.
Indeed, I'll make this and entry below for S 43 board const.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RESEND PATCH] arm64: dts: rockchip: add rk3328 VPU node
From: Jonas Karlman @ 2019-08-21 17:54 UTC (permalink / raw)
To: Heiko Stuebner
Cc: devicetree@vger.kernel.org, Jonas Karlman,
linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
Rob Herring, linux-arm-kernel@lists.infradead.org
This patch add a VPU device node for rk3328.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
It would be great if this can be considered for v5.4,
related dt-bindings commit has been merged in media tree for v5.4.
Decoding using hantro driver has been tested on a Pine64 Rock64 RK3328 device.
---
arch/arm64/boot/dts/rockchip/rk3328.dtsi | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index e9fefd8a7e02..4a175fff2861 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -278,6 +278,7 @@
};
pd_vpu@RK3328_PD_VPU {
reg = <RK3328_PD_VPU>;
+ clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
};
};
@@ -596,6 +597,17 @@
status = "disabled";
};
+ vpu: video-codec@ff350000 {
+ compatible = "rockchip,rk3328-vpu";
+ reg = <0x0 0xff350000 0x0 0x800>;
+ interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "vdpu";
+ clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
+ clock-names = "aclk", "hclk";
+ iommus = <&vpu_mmu>;
+ power-domains = <&power RK3328_PD_VPU>;
+ };
+
vpu_mmu: iommu@ff350800 {
compatible = "rockchip,iommu";
reg = <0x0 0xff350800 0x0 0x40>;
@@ -604,7 +616,7 @@
clocks = <&cru ACLK_VPU>, <&cru HCLK_VPU>;
clock-names = "aclk", "iface";
#iommu-cells = <0>;
- status = "disabled";
+ power-domains = <&power RK3328_PD_VPU>;
};
rkvdec_mmu: iommu@ff360480 {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v4] kasan: add memory corruption identification for software tag-based mode
From: Andrey Ryabinin @ 2019-08-21 17:52 UTC (permalink / raw)
To: Walter Wu
Cc: wsd_upstream, Vasily Gorbik, Arnd Bergmann, linux-mm,
Andrey Konovalov, linux-kernel, kasan-dev, Martin Schwidefsky,
Miles Chen, Alexander Potapenko, linux-arm-kernel,
Matthias Brugger, linux-mediatek, Andrew Morton, Thomas Gleixner,
Dmitry Vyukov
In-Reply-To: <1566279478.9993.21.camel@mtksdccf07>
On 8/20/19 8:37 AM, Walter Wu wrote:
> On Tue, 2019-08-06 at 13:43 +0800, Walter Wu wrote:
>> This patch adds memory corruption identification at bug report for
>> software tag-based mode, the report show whether it is "use-after-free"
>> or "out-of-bound" error instead of "invalid-access" error. This will make
>> it easier for programmers to see the memory corruption problem.
>>
>> We extend the slab to store five old free pointer tag and free backtrace,
>> we can check if the tagged address is in the slab record and make a
>> good guess if the object is more like "use-after-free" or "out-of-bound".
>> therefore every slab memory corruption can be identified whether it's
>> "use-after-free" or "out-of-bound".
>>
>> ====== Changes
>> Change since v1:
>> - add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
>> - change QUARANTINE_FRACTION to reduce quarantine size.
>> - change the qlist order in order to find the newest object in quarantine
>> - reduce the number of calling kmalloc() from 2 to 1 time.
>> - remove global variable to use argument to pass it.
>> - correct the amount of qobject cache->size into the byes of qlist_head.
>> - only use kasan_cache_shrink() to shink memory.
>>
>> Change since v2:
>> - remove the shinking memory function kasan_cache_shrink()
>> - modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
>> - optimize the quarantine_find_object() and qobject_free()
>> - fix the duplicating function name 3 times in the header.
>> - modify the function name set_track() to kasan_set_track()
>>
>> Change since v3:
>> - change tag-based quarantine to extend slab to identify memory corruption
>
> Hi,Andrey,
>
> Would you review the patch,please?
I didn't notice anything fundamentally wrong, but I find there are some
questionable implementation choices that makes code look weirder than necessary
and harder to understand. So I ended up with cleaning it up, see the diff bellow.
I'll send v5 with that diff folded.
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 26cb3bcc9258..6c9682ce0254 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -140,7 +140,7 @@ config KASAN_SW_TAGS_IDENTIFY
help
This option enables best-effort identification of bug type
(use-after-free or out-of-bounds) at the cost of increased
- memory consumption for slab extending.
+ memory consumption.
config TEST_KASAN
tristate "Module for testing KASAN for bug detection"
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 2cdcb16b9c2d..6814d6d6a023 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -71,7 +71,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);
}
-void kasan_set_track(struct kasan_track *track, gfp_t flags)
+static inline void set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = save_stack(flags);
@@ -304,8 +304,6 @@ size_t kasan_metadata_size(struct kmem_cache *cache)
struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
const void *object)
{
- if (!IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);
return (void *)object + cache->kasan_info.alloc_meta_offset;
}
@@ -316,6 +314,24 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
return (void *)object + cache->kasan_info.free_meta_offset;
}
+
+static void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ u8 idx = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ idx = alloc_meta->free_track_idx;
+ alloc_meta->free_pointer_tag[idx] = tag;
+ alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
+#endif
+
+ set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
+}
+
void kasan_poison_slab(struct page *page)
{
unsigned long i;
@@ -452,11 +468,8 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
unlikely(!(cache->flags & SLAB_KASAN)))
return false;
- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- kasan_set_free_info(cache, object, tag);
- else
- kasan_set_track(&get_alloc_info(cache, object)->free_track,
- GFP_NOWAIT);
+ kasan_set_free_info(cache, object, tag);
+
quarantine_put(get_free_info(cache, object), cache);
return IS_ENABLED(CONFIG_KASAN_GENERIC);
@@ -494,8 +507,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
KASAN_KMALLOC_REDZONE);
if (cache->flags & SLAB_KASAN)
- kasan_set_track(&get_alloc_info(cache, object)->alloc_track,
- flags);
+ set_track(&get_alloc_info(cache, object)->alloc_track, flags);
return set_tag(object, tag);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 531a5823e8c6..35cff6bbb716 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -96,21 +96,17 @@ struct kasan_track {
};
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-#define KASAN_EXTRA_FREE_INFO_COUNT 4
-#define KASAN_TOTAL_FREE_INFO_COUNT (KASAN_EXTRA_FREE_INFO_COUNT + 1)
-struct extra_free_info {
- /* Round-robin FIFO array. */
- struct kasan_track free_track[KASAN_EXTRA_FREE_INFO_COUNT];
- u8 free_pointer_tag[KASAN_TOTAL_FREE_INFO_COUNT];
- u8 free_track_tail;
-};
+#define KASAN_NR_FREE_STACKS 5
+#else
+#define KASAN_NR_FREE_STACKS 1
#endif
struct kasan_alloc_meta {
struct kasan_track alloc_track;
- struct kasan_track free_track;
+ struct kasan_track free_track[KASAN_NR_FREE_STACKS];
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- struct extra_free_info free_info;
+ u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
+ u8 free_track_idx;
#endif
};
@@ -160,28 +156,7 @@ void kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);
-struct page *addr_to_page(const void *addr);
-
-void kasan_set_track(struct kasan_track *track, gfp_t flags);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag);
-char *kasan_get_corruption_type(void *addr);
-#else
-static inline void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag) { }
-static inline struct kasan_track *kasan_get_free_track(
- struct kmem_cache *cache, void *object, u8 tag)
-{
- return NULL;
-}
-static inline char *kasan_get_corruption_type(void *addr)
-{
- return NULL;
-}
-#endif
+struct page *kasan_addr_to_page(const void *addr);
#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 9ea7a4265b42..621782100eaa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -111,6 +111,14 @@ static void print_track(struct kasan_track *track, const char *prefix)
}
}
+struct page *kasan_addr_to_page(const void *addr)
+{
+ if ((addr >= (void *)PAGE_OFFSET) &&
+ (addr < high_memory))
+ return virt_to_head_page(addr);
+ return NULL;
+}
+
static void describe_object_addr(struct kmem_cache *cache, void *object,
const void *addr)
{
@@ -143,28 +151,42 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
(void *)(object_addr + cache->object_size));
}
+static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ int i = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ break;
+ }
+ if (i == KASAN_NR_FREE_STACKS)
+ i = alloc_meta->free_track_idx;
+#endif
+
+ return &alloc_meta->free_track[i];
+}
+
static void describe_object(struct kmem_cache *cache, void *object,
- const void *tagged_addr)
+ const void *addr, u8 tag)
{
- void *untagged_addr = reset_tag(tagged_addr);
struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
if (cache->flags & SLAB_KASAN) {
+ struct kasan_track *free_track;
+
print_track(&alloc_info->alloc_track, "Allocated");
pr_err("\n");
- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
- struct kasan_track *free_track;
- u8 tag = get_tag(tagged_addr);
-
- free_track = kasan_get_free_track(cache, object, tag);
- print_track(free_track, "Freed");
- } else {
- print_track(&alloc_info->free_track, "Freed");
- pr_err("\n");
- }
+ free_track = kasan_get_free_track(cache, object, tag);
+ print_track(free_track, "Freed");
+ pr_err("\n");
}
- describe_object_addr(cache, object, untagged_addr);
+ describe_object_addr(cache, object, addr);
}
static inline bool kernel_or_module_addr(const void *addr)
@@ -345,25 +367,23 @@ static void print_address_stack_frame(const void *addr)
print_decoded_frame_descr(frame_descr);
}
-static void print_address_description(void *tagged_addr)
+static void print_address_description(void *addr, u8 tag)
{
- void *untagged_addr = reset_tag(tagged_addr);
- struct page *page = addr_to_page(untagged_addr);
+ struct page *page = kasan_addr_to_page(addr);
dump_stack();
pr_err("\n");
if (page && PageSlab(page)) {
struct kmem_cache *cache = page->slab_cache;
- void *object = nearest_obj(cache, page, untagged_addr);
+ void *object = nearest_obj(cache, page, addr);
- describe_object(cache, object, tagged_addr);
+ describe_object(cache, object, addr, tag);
}
- if (kernel_or_module_addr(untagged_addr) &&
- !init_task_stack_addr(untagged_addr)) {
+ if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {
pr_err("The buggy address belongs to the variable:\n");
- pr_err(" %pS\n", tagged_addr);
+ pr_err(" %pS\n", addr);
}
if (page) {
@@ -371,7 +391,7 @@ static void print_address_description(void *tagged_addr)
dump_page(page, "kasan: bad access detected");
}
- print_address_stack_frame(untagged_addr);
+ print_address_stack_frame(addr);
}
static bool row_is_guilty(const void *row, const void *guilty)
@@ -435,25 +455,18 @@ static bool report_enabled(void)
return !test_and_set_bit(KASAN_BIT_REPORTED, &kasan_flags);
}
-struct page *addr_to_page(const void *addr)
-{
- if ((addr >= (void *)PAGE_OFFSET) &&
- (addr < high_memory))
- return virt_to_head_page(addr);
- return NULL;
-}
-
void kasan_report_invalid_free(void *object, unsigned long ip)
{
unsigned long flags;
+ u8 tag = get_tag(object);
+ object = reset_tag(object);
start_report(&flags);
pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip);
- print_tags(get_tag(object), reset_tag(object));
+ print_tags(tag, object);
pr_err("\n");
- print_address_description(object);
+ print_address_description(object, tag);
pr_err("\n");
- object = reset_tag(object);
print_shadow_for_address(object);
end_report(&flags);
}
@@ -490,7 +503,7 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon
pr_err("\n");
if (addr_has_shadow(untagged_addr)) {
- print_address_description(tagged_addr);
+ print_address_description(untagged_addr, get_tag(tagged_addr));
pr_err("\n");
print_shadow_for_address(info.first_bad_addr);
} else {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 05a11f1cfff7..0e987c9ca052 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -161,89 +161,3 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
kasan_poison_shadow((void *)addr, size, tag);
}
EXPORT_SYMBOL(__hwasan_tag_memory);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- u8 idx;
-
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- if (free_info->free_track_tail == 0)
- free_info->free_track_tail = KASAN_EXTRA_FREE_INFO_COUNT;
- else
- free_info->free_track_tail -= 1;
-
- idx = free_info->free_track_tail;
- free_info->free_pointer_tag[idx] = tag;
-
- if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
- kasan_set_track(&alloc_meta->free_track, GFP_NOWAIT);
- else
- kasan_set_track(&free_info->free_track[idx], GFP_NOWAIT);
-}
-
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- int idx, i;
-
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
- idx = free_info->free_track_tail + i;
- if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
- idx -= KASAN_TOTAL_FREE_INFO_COUNT;
-
- if (free_info->free_pointer_tag[idx] == tag) {
- if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
- return &alloc_meta->free_track;
- else
- return &free_info->free_track[idx];
- }
- }
- if (free_info->free_track_tail == KASAN_EXTRA_FREE_INFO_COUNT)
- return &alloc_meta->free_track;
- else
- return &free_info->free_track[free_info->free_track_tail];
-}
-
-char *kasan_get_corruption_type(void *addr)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- struct page *page;
- struct kmem_cache *cache;
- void *object;
- u8 tag;
- int idx, i;
-
- tag = get_tag(addr);
- addr = reset_tag(addr);
- page = addr_to_page(addr);
- if (page && PageSlab(page)) {
- cache = page->slab_cache;
- object = nearest_obj(cache, page, addr);
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
- idx = free_info->free_track_tail + i;
- if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
- idx -= KASAN_TOTAL_FREE_INFO_COUNT;
-
- if (free_info->free_pointer_tag[idx] == tag)
- return "use-after-free";
- }
- return "out-of-bounds";
- }
- return "invalid-access";
-}
-#endif
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 6d8cdb91c4b6..969ae08f59d7 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,10 +36,31 @@
const char *get_bug_type(struct kasan_access_info *info)
{
- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- return(kasan_get_corruption_type((void *)info->access_addr));
- else
- return "invalid-access";
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ struct kasan_alloc_meta *alloc_meta;
+ struct kmem_cache *cache;
+ struct page *page;
+ const void *addr;
+ void *object;
+ u8 tag;
+ int i;
+
+ tag = get_tag(info->access_addr);
+ addr = reset_tag(info->access_addr);
+ page = kasan_addr_to_page(addr);
+ if (page && PageSlab(page)) {
+ cache = page->slab_cache;
+ object = nearest_obj(cache, page, (void *)addr);
+ alloc_meta = get_alloc_info(cache, object);
+
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++)
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ return "use-after-free";
+ return "out-of-bounds";
+ }
+
+#endif
+ return "invalid-access";
}
void *find_first_bad_addr(void *addr, size_t size)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Nathan Huckleberry @ 2019-08-21 17:46 UTC (permalink / raw)
To: linux
Cc: Tri Vo, ndesaulniers, linux-kernel, Nathan Huckleberry,
clang-built-linux, miles.chen, linux-arm-kernel
In-Reply-To: <CAJkfWY4cHz+i8kYg2i1Krs-32nh7-WQU+psT=DRGYnTje6yj4Q@mail.gmail.com>
The stackframe setup when compiled with clang is different.
Since the stack unwinder expects the gcc stackframe setup it
fails to print backtraces. This patch adds support for the
clang stackframe setup.
Link: https://github.com/ClangBuiltLinux/linux/issues/35
Cc: clang-built-linux@googlegroups.com
Suggested-by: Tri Vo <trong@google.com>
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
Changes from v1->v2
* Fix indentation in various files
* Swap spaces for tabs
* Rename Ldsi to Lopcode
* Remove unused Ldsi entry
arch/arm/Kconfig.debug | 2 +-
arch/arm/Makefile | 5 +-
arch/arm/lib/Makefile | 8 +-
arch/arm/lib/backtrace-clang.S | 229 +++++++++++++++++++++++++++++++++
4 files changed, 241 insertions(+), 3 deletions(-)
create mode 100644 arch/arm/lib/backtrace-clang.S
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 85710e078afb..b9c674ec19e0 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -56,7 +56,7 @@ choice
config UNWINDER_FRAME_POINTER
bool "Frame pointer unwinder"
- depends on !THUMB2_KERNEL && !CC_IS_CLANG
+ depends on !THUMB2_KERNEL
select ARCH_WANT_FRAME_POINTERS
select FRAME_POINTER
help
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index c3624ca6c0bc..6f251c201db0 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -36,7 +36,10 @@ KBUILD_CFLAGS += $(call cc-option,-mno-unaligned-access)
endif
ifeq ($(CONFIG_FRAME_POINTER),y)
-KBUILD_CFLAGS +=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
+KBUILD_CFLAGS +=-fno-omit-frame-pointer
+ifeq ($(CONFIG_CC_IS_GCC),y)
+KBUILD_CFLAGS += -mapcs -mno-sched-prolog
+endif
endif
ifeq ($(CONFIG_CPU_BIG_ENDIAN),y)
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index b25c54585048..6d2ba454f25b 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -5,7 +5,7 @@
# Copyright (C) 1995-2000 Russell King
#
-lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
+lib-y := changebit.o csumipv6.o csumpartial.o \
csumpartialcopy.o csumpartialcopyuser.o clearbit.o \
delay.o delay-loop.o findbit.o memchr.o memcpy.o \
memmove.o memset.o setbit.o \
@@ -19,6 +19,12 @@ lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
mmu-y := clear_user.o copy_page.o getuser.o putuser.o \
copy_from_user.o copy_to_user.o
+ifdef CONFIG_CC_IS_CLANG
+ lib-y += backtrace-clang.o
+else
+ lib-y += backtrace.o
+endif
+
# using lib_ here won't override already available weak symbols
obj-$(CONFIG_UACCESS_WITH_MEMCPY) += uaccess_with_memcpy.o
diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
new file mode 100644
index 000000000000..6f2a8a57d0fb
--- /dev/null
+++ b/arch/arm/lib/backtrace-clang.S
@@ -0,0 +1,229 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * linux/arch/arm/lib/backtrace-clang.S
+ *
+ * Copyright (C) 2019 Nathan Huckleberry
+ *
+ */
+#include <linux/kern_levels.h>
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+ .text
+
+/* fp is 0 or stack frame */
+
+#define frame r4
+#define sv_fp r5
+#define sv_pc r6
+#define mask r7
+#define sv_lr r8
+
+ENTRY(c_backtrace)
+
+#if !defined(CONFIG_FRAME_POINTER) || !defined(CONFIG_PRINTK)
+ ret lr
+ENDPROC(c_backtrace)
+#else
+
+
+/*
+ * Clang does not store pc or sp in function prologues
+ * so we don't know exactly where the function
+ * starts.
+ *
+ * We can treat the current frame's lr as the saved pc and the
+ * preceding frame's lr as the current frame's lr,
+ * but we can't trace the most recent call.
+ * Inserting a false stack frame allows us to reference the
+ * function called last in the stacktrace.
+ *
+ * If the call instruction was a bl we can look at the callers
+ * branch instruction to calculate the saved pc.
+ * We can recover the pc in most cases, but in cases such as
+ * calling function pointers we cannot. In this
+ * case, default to using the lr. This will be
+ * some address in the function, but will not
+ * be the function start.
+ *
+ * Unfortunately due to the stack frame layout we can't dump
+ * r0 - r3, but these are less frequently saved.
+ *
+ * Stack frame layout:
+ * <larger addresses>
+ * saved lr
+ * frame=> saved fp
+ * optionally saved caller registers (r4 - r10)
+ * optionally saved arguments (r0 - r3)
+ * <top of stack frame>
+ * <smaller addresses>
+ *
+ * Functions start with the following code sequence:
+ * corrected pc => stmfd sp!, {..., fp, lr}
+ * add fp, sp, #x
+ * stmfd sp!, {r0 - r3} (optional)
+ *
+ *
+ *
+ *
+ *
+ *
+ * The diagram below shows an example stack setup
+ * for dump_stack.
+ *
+ * The frame for c_backtrace has pointers to the
+ * code of dump_stack. This is why the frame of
+ * c_backtrace is used to for the pc calculation
+ * of dump_stack. This is why we must move back
+ * a frame to print dump_stack.
+ *
+ * The stored locals for dump_stack are in dump_stack's
+ * frame. This means that to fully print dump_stack's frame
+ * we need both the frame for dump_stack (for locals) and the
+ * frame that was called by dump_stack (for pc).
+ *
+ * To print locals we must know where the function start is. If
+ * we read the function prologue opcodes we can determine
+ * which variables are stored in the stack frame.
+ *
+ * To find the function start of dump_stack we can look at the
+ * stored LR of show_stack. It points at the instruction
+ * directly after the bl dump_stack. We can then read the
+ * offset from the bl opcode to determine where the branch takes us.
+ * The address calculated must be the start of dump_stack.
+ *
+ * c_backtrace frame dump_stack:
+ * {[LR] } ============| ...
+ * {[FP] } =======| | bl c_backtrace
+ * | |=> ...
+ * {[R4-R10]} |
+ * {[R0-R3] } | show_stack:
+ * dump_stack frame | ...
+ * {[LR] } =============| bl dump_stack
+ * {[FP] } <=======| |=> ...
+ * {[R4-R10]}
+ * {[R0-R3] }
+ */
+
+stmfd sp!, {r4 - r9, fp, lr} @ Save an extra register
+ @ to ensure 8 byte alignment
+movs frame, r0 @ if frame pointer is zero
+beq no_frame @ we have no stack frames
+
+tst r1, #0x10 @ 26 or 32-bit mode?
+moveq mask, #0xfc000003
+movne mask, #0 @ mask for 32-bit
+
+/*
+ * Switches the current frame to be the frame for dump_stack.
+ */
+ add frame, sp, #24 @ switch to false frame
+for_each_frame: tst frame, mask @ Check for address exceptions
+ bne no_frame
+
+/*
+ * sv_fp is the stack frame with the locals for the current considered
+ * function.
+ *
+ * sv_pc is the saved lr frame the frame above. This is a pointer to a
+ * code address within the current considered function, but
+ * it is not the function start. This value gets updated to be
+ * the function start later if it is possible.
+ */
+1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
+1002: ldr sv_fp, [frame, #0] @ get saved fp
+
+ teq sv_fp, mask @ make sure next frame exists
+ beq no_frame
+
+/*
+ * sv_lr is the lr from the function that called the current function. This
+ * is a pointer to a code address in the current function's caller.
+ * sv_lr-4 is the instruction used to call the current function.
+ *
+ * This sv_lr can be used to calculate the function start if the function
+ * was called using a bl instruction. If the function start
+ * can be recovered sv_pc is overwritten with the function start.
+ *
+ * If the current function was called using a function pointer we cannot
+ * recover the function start and instead continue with sv_pc as
+ * an arbitrary value within the current function. If this is the case
+ * we cannot print registers for the current function, but the stacktrace
+ * is still printed properly.
+ */
+1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
+
+ ldr r0, [sv_lr, #-4] @ get call instruction
+ ldr r3, .Lopcode+4
+ and r2, r3, r0 @ is this a bl call
+ teq r2, r3
+ bne finished_setup @ give up if it's not
+ and r0, #0xffffff @ get call offset 24-bit int
+ lsl r0, r0, #8 @ sign extend offset
+ asr r0, r0, #8
+ ldr sv_pc, [sv_fp, #4] @ get lr address
+ add sv_pc, sv_pc, #-4 @ get call instruction address
+ add sv_pc, sv_pc, #8 @ take care of prefetch
+ add sv_pc, sv_pc, r0, lsl #2@ find function start
+
+finished_setup:
+
+ bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
+
+/*
+ * Print the function (sv_pc) and where it was called
+ * from (sv_lr).
+ */
+1004: mov r0, sv_pc
+
+ mov r1, sv_lr
+ mov r2, frame
+ bic r1, r1, mask @ mask PC/LR for the mode
+ bl dump_backtrace_entry
+
+/*
+ * Test if the function start is a stmfd instruction
+ * to determine which registers were stored in the function
+ * prologue.
+ *
+ * If we could not recover the sv_pc because we were called through
+ * a function pointer the comparison will fail and no registers
+ * will print.
+ */
+1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
+ ldr r3, .Lopcode @ instruction exists,
+ teq r3, r1, lsr #11
+ ldr r0, [frame] @ locals are stored in
+ @ the preceding frame
+ subeq r0, r0, #4
+ bleq dump_backtrace_stm @ dump saved registers
+
+/*
+ * If we are out of frames or if the next frame is invalid.
+ */
+ teq sv_fp, #0 @ zero saved fp means
+ beq no_frame @ no further frames
+
+ cmp sv_fp, frame @ next frame must be
+ mov frame, sv_fp @ above the current frame
+ bhi for_each_frame
+
+1006: adr r0, .Lbad
+ mov r1, frame
+ bl printk
+no_frame: ldmfd sp!, {r4 - r9, fp, pc}
+ENDPROC(c_backtrace)
+ .pushsection __ex_table,"a"
+ .align 3
+ .long 1001b, 1006b
+ .long 1002b, 1006b
+ .long 1003b, 1006b
+ .long 1004b, 1006b
+ .long 1005b, 1006b
+ .popsection
+
+.Lbad: .asciz "Backtrace aborted due to bad frame pointer <%p>\n"
+ .align
+.Lopcode: .word 0xe92d4800 >> 11 @ stmfd sp!, {... fp, lr}
+ .word 0x0b000000 @ bl if these bits are set
+
+#endif
--
2.23.0.rc1.153.gdeed80330f-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Nathan Huckleberry @ 2019-08-21 17:43 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Tri Vo, Russell King, LKML, clang-built-linux,
Miles Chen (陳民樺), Linux ARM
In-Reply-To: <CAKwvOdm+sGyKfAMNbL10ME=DrG5=4d5kvzdMxjNC22JLLr1h=g@mail.gmail.com>
On Tue, Aug 20, 2019 at 2:39 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Aug 20, 2019 at 12:44 PM Nathan Huckleberry <nhuck@google.com> wrote:
> >
> > The stackframe setup when compiled with clang is different.
> > Since the stack unwinder expects the gcc stackframe setup it
> > fails to print backtraces. This patch adds support for the
> > clang stackframe setup.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/35
> > Cc: clang-built-linux@googlegroups.com
> > Suggested-by: Tri Vo <trong@google.com>
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > ---
> > Changes from RFC
> > * Push extra register to satisfy 8 byte alignment requirement
> > * Add clarifying comments
> > * Separate code into its own file
>
> Thanks for the patch! The added comments and moving the implementation
> to its own file make it easier to review.
>
> >
> > arch/arm/Kconfig.debug | 4 +-
> > arch/arm/Makefile | 5 +-
> > arch/arm/lib/Makefile | 8 +-
> > arch/arm/lib/backtrace-clang.S | 224 +++++++++++++++++++++++++++++++++
> > 4 files changed, 237 insertions(+), 4 deletions(-)
> > create mode 100644 arch/arm/lib/backtrace-clang.S
> >
> > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> > index 85710e078afb..92fca7463e21 100644
> > --- a/arch/arm/Kconfig.debug
> > +++ b/arch/arm/Kconfig.debug
> > @@ -56,7 +56,7 @@ choice
> >
> > config UNWINDER_FRAME_POINTER
> > bool "Frame pointer unwinder"
> > - depends on !THUMB2_KERNEL && !CC_IS_CLANG
> > + depends on !THUMB2_KERNEL
> > select ARCH_WANT_FRAME_POINTERS
> > select FRAME_POINTER
> > help
> > @@ -1872,7 +1872,7 @@ config DEBUG_UNCOMPRESS
> > When this option is set, the selected DEBUG_LL output method
> > will be re-used for normal decompressor output on multiplatform
> > kernels.
> > -
> > +
>
> Probably can drop the added newline?
>
> >
> > config UNCOMPRESS_INCLUDE
> > string
> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> > index c3624ca6c0bc..729e223b83fe 100644
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -36,7 +36,10 @@ KBUILD_CFLAGS += $(call cc-option,-mno-unaligned-access)
> > endif
> >
> > ifeq ($(CONFIG_FRAME_POINTER),y)
> > -KBUILD_CFLAGS +=-fno-omit-frame-pointer -mapcs -mno-sched-prolog
> > +KBUILD_CFLAGS +=-fno-omit-frame-pointer
> > + ifeq ($(CONFIG_CC_IS_GCC),y)
> > + KBUILD_CFLAGS += -mapcs -mno-sched-prolog
> > + endif
>
> While I can appreciate the indentation, it's unusual to indent
> additional depths of kernel Makefiles. At least the rest of this file
> does not do so. Of course, the other Makefile you touch below does
> two spaces. At least try to keep the file internally consistent, even
> if the kernel itself is inconsistent.
>
> > endif
> >
> > ifeq ($(CONFIG_CPU_BIG_ENDIAN),y)
> > diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> > index b25c54585048..e10a769c72ec 100644
> > --- a/arch/arm/lib/Makefile
> > +++ b/arch/arm/lib/Makefile
> > @@ -5,7 +5,7 @@
> > # Copyright (C) 1995-2000 Russell King
> > #
> >
> > -lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
> > +lib-y := changebit.o csumipv6.o csumpartial.o \
> > csumpartialcopy.o csumpartialcopyuser.o clearbit.o \
> > delay.o delay-loop.o findbit.o memchr.o memcpy.o \
> > memmove.o memset.o setbit.o \
> > @@ -19,6 +19,12 @@ lib-y := backtrace.o changebit.o csumipv6.o csumpartial.o \
> > mmu-y := clear_user.o copy_page.o getuser.o putuser.o \
> > copy_from_user.o copy_to_user.o
> >
> > +ifdef CONFIG_CC_IS_CLANG
> > + lib-y += backtrace-clang.o
> > +else
> > + lib-y += backtrace.o
> > +endif
>
> The indentation should match the above (from this file). Looks like 1
> tab after lib-y. See L34(CONFIG_CPU_32v3) for what I would have
> expected.
>
> > +
> > # using lib_ here won't override already available weak symbols
> > obj-$(CONFIG_UACCESS_WITH_MEMCPY) += uaccess_with_memcpy.o
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > new file mode 100644
> > index 000000000000..2b02014dbdd1
> > --- /dev/null
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -0,0 +1,224 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * linux/arch/arm/lib/backtrace-clang.S
> > + *
> > + * Copyright (C) 2019 Nathan Huckleberry
> > + *
> > + */
> > +#include <linux/kern_levels.h>
> > +#include <linux/linkage.h>
> > +#include <asm/assembler.h>
> > + .text
> > +
> > +/* fp is 0 or stack frame */
>
> ah, I see that the reference implementation uses an assembly comment
> here. Ok (sorry for the noise).
>
> > +
> > +#define frame r4
> > +#define sv_fp r5
> > +#define sv_pc r6
> > +#define mask r7
> > +#define sv_lr r8
> > +
> > +ENTRY(c_backtrace)
> > +
> > +#if !defined(CONFIG_FRAME_POINTER) || !defined(CONFIG_PRINTK)
> > + ret lr
> > +ENDPROC(c_backtrace)
> > +#else
> > +
> > +
> > +/*
> > + * Clang does not store pc or sp in function prologues
> > + * so we don't know exactly where the function
> > + * starts.
> > + * We can treat the current frame's lr as the saved pc and the
> > + * preceding frame's lr as the lr, but we can't
>
> preceding frame's lr as the current frame's lr, ...
>
> > + * trace the most recent call.
> > + * Inserting a false stack frame allows us to reference the
> > + * function called last in the stacktrace.
> > + * If the call instruction was a bl we can look at the callers
> > + * branch instruction to calculate the saved pc.
> > + * We can recover the pc in most cases, but in cases such as
> > + * calling function pointers we cannot. In this
> > + * case, default to using the lr. This will be
> > + * some address in the function, but will not
> > + * be the function start.
> > + * Unfortunately due to the stack frame layout we can't dump
> > + * r0 - r3, but these are less frequently saved.
>
> The use of tabs vs spaces in these comments is inconsistent. Not that
> I can see whitespace, but:
> https://github.com/nickdesaulniers/dotfiles/blob/37359525f5a403b4ed2d3f9d1bbbee2da8ec8115/.vimrc#L35-L41
> Also, I don't think you need to tab indent every line after the first.
> Where did that format come from?
>
> > + *
> > + * Stack frame layout:
> > + * <larger addresses>
> > + * saved lr
> > + * frame => saved fp
> > + * optionally saved caller registers (r4 - r10)
> > + * optionally saved arguments (r0 - r3)
> > + * <top of stack frame>
> > + * <smaller addresses>
> > + *
> > + * Functions start with the following code sequence:
> > + * corrected pc => stmfd sp!, {..., fp, lr}
> > + * add fp, sp, #x
> > + * stmfd sp!, {r0 - r3} (optional)
> > + *
> > + *
> > + *
> > + *
> > + *
> > + *
> > + * The diagram below shows an example stack setup
> > + * for dump_stack.
> > + *
> > + * The frame for c_backtrace has pointers to the
> > + * code of dump_stack. This is why the frame of
> > + * c_backtrace is used to for the pc calculation
> > + * of dump_stack. This is why we must move back
> > + * a frame to print dump_stack.
> > + *
> > + * The stored locals for dump_stack are in dump_stack's
> > + * frame. This means that to fully print dump_stack's frame
> > + * we need the both the frame for dump_stack (for locals) and the
>
> we need both the ...
> (There's an extra `the` in the sentence).
>
> > + * frame that was called by dump_stack (for pc).
> > + *
> > + * To print locals we must know where the function start is. If
> > + * we read the function prologue opcodes we can determine
> > + * which variables are stored in the stack frame.
> > + *
> > + * To find the function start of dump_stack we can look at the
> > + * stored LR of show_stack. It points at the instruction
> > + * directly after the bl dump_stack. We can then read the
> > + * offset from the bl opcode to determine where the branch takes us.
> > + * The address calculated must be the start of dump_stack.
> > + *
> > + * c_backtrace frame dump_stack:
> > + * {[LR] } ============| ...
> > + * {[FP] } =======| | bl c_backtrace
> > + * | |=> ...
> > + * {[R4-R10]} |
> > + * {[R0-R3] } | show_stack:
> > + * dump_stack frame | ...
> > + * {[LR] } =============| bl dump_stack
> > + * {[FP] } <=======| |=> ...
> > + * {[R4-R10]}
> > + * {[R0-R3] }
> > + */
> > +
> > +stmfd sp!, {r4 - r9, fp, lr} @ Save an extra register
> > + @ to ensure 8 byte alignment
> > +movs frame, r0 @ if frame pointer is zero
> > +beq no_frame @ we have no stack frames
> > +
> > +tst r1, #0x10 @ 26 or 32-bit mode?
> > +moveq mask, #0xfc000003
>
> Should we be using different masks for ARM vs THUMB as per the
> reference implementation?
The change that introduces the arm/thumb code looked like a script
that was run over all arm in the kernel. Neither this code nor the
reference solution is compatible with arm, so there's no need for the
change.
>
> > +movne mask, #0 @ mask for 32-bit
> > +
> > +/*
> > + * Switches the current frame to be the frame for dump_stack.
> > + */
> > + add frame, sp, #24 @ switch to false frame
> > +for_each_frame: tst frame, mask @ Check for address exceptions
> > + bne no_frame
> > +
> > +/*
> > + * sv_fp is the stack frame with the locals for the current considered
> > + * function.
> > + * sv_pc is the saved lr frame the frame above. This is a pointer to a
> > + * code address within the current considered function, but
> > + * it is not the function start. This value gets updated to be
> > + * the function start later if it is possible.
> > + */
> > +1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +1002: ldr sv_fp, [frame, #0] @ get saved fp
>
> The reference implementation applies the mask to sv_pc and sv_fp. I
> assume we want to, too?
The mask is already applied to both. See for_each_frame:
>
> > +
> > + teq sv_fp, #0 @ make sure next frame exists
> > + beq no_frame
> > +
> > +/*
> > + * sv_lr is the lr from the function that called the current function. This
> > + * is a pointer to a code address in the current function's caller.
> > + * sv_lr-4 is the instruction used to call the current function.
> > + * This sv_lr can be used to calculate the function start if the function
> > + * was called using a bl instruction. If the function start
> > + * can be recovered sv_pc is overwritten with the function start.
> > + * If the current function was called using a function pointer we cannot
> > + * recover the function start and instead continue with sv_pc as
> > + * an arbitrary value within the current function. If this is the case
> > + * we cannot print registers for the current function, but the stacktrace
> > + * is still printed properly.
> > + */
> > +1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +
> > + ldr r0, [sv_lr, #-4] @ get call instruction
> > + ldr r3, .Ldsi+8
>
> I wonder what `dsi` stands for, it could use a better name. Maybe put
> that mask in a more descriptively named section and use that instead
> of `.Ldsi+8`?
>
> > + and r2, r3, r0 @ is this a bl call
> > + teq r2, r3
> > + bne finished_setup @ give up if it's not
> > + and r0, #0xffffff @ get call offset 24-bit int
> > + lsl r0, r0, #8 @ sign extend offset
> > + asr r0, r0, #8
>
> It's too bad this should work for older ARM versions, v6 added
> dedicated instructions for this:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/Cihfifdd.html
>
> > + ldr sv_pc, [sv_fp, #4] @ get lr address
> > + add sv_pc, sv_pc, #-4 @ get call instruction address
> > + add sv_pc, sv_pc, #8 @ take care of prefetch
> > + add sv_pc, sv_pc, r0, lsl #2 @ find function start
> > +
> > +finished_setup:
> > +
> > + bic sv_pc, sv_pc, mask @ mask PC/LR for the mode
> > +
> > +/*
> > + * Print the function (sv_pc) and where it was called
> > + * from (sv_lr).
> > + */
> > +1004: mov r0, sv_pc
> > +
> > + mov r1, sv_lr
> > + mov r2, frame
> > + bic r1, r1, mask @ mask PC/LR for the mode
> > + bl dump_backtrace_entry
> > +
> > +/*
> > + * Test if the function start is a stmfd instruction
> > + * to determine which registers were stored in the function
> > + * prologue.
> > + * If we could not recover the sv_pc because we were called through
> > + * a function pointer the comparison will fail and no registers
> > + * will print.
> > + */
> > +1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > + ldr r3, .Ldsi @ instruction exists,
> > + teq r3, r1, lsr #11
> > + ldr r0, [frame] @ locals are stored in
> > + @ the preceding frame
> > + subeq r0, r0, #4
> > + bleq dump_backtrace_stm @ dump saved registers
>
> Do we need to do anything to test .Ldsi+4? Otherwise looks like we
> define it but don't use it?
>
> > +
> > +/*
> > + * If we are out of frames or if the next frame
> > + * is invalid.
> > + */
> > + teq sv_fp, #0 @ zero saved fp means
> > + beq no_frame @ no further frames
> > +
> > + cmp sv_fp, frame @ next frame must be
> > + mov frame, sv_fp @ above the current frame
> > + bhi for_each_frame
> > +
> > +1006: adr r0, .Lbad
> > + mov r1, frame
> > + bl printk
> > +no_frame: ldmfd sp!, {r4 - r9, fp, pc}
> > +ENDPROC(c_backtrace)
> > + .pushsection __ex_table,"a"
> > + .align 3
> > + .long 1001b, 1006b
> > + .long 1002b, 1006b
> > + .long 1003b, 1006b
> > + .long 1004b, 1006b
> > + .long 1005b, 1006b
> > + .popsection
> > +
> > +.Lbad: .asciz "Backtrace aborted due to bad frame pointer <%p>\n"
> > + .align
> > +.Ldsi: .word 0xe92d4800 >> 11 @ stmfd sp!, {... fp, lr}
> > + .word 0xe92d0000 >> 11 @ stmfd sp!, {}
> > + .word 0x0b000000 @ bl if these bits are set
> > +
> > +#endif
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
Thanks for the review, will send a v2 with your suggestions.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot
From: Alexander Amelkin @ 2019-08-21 17:42 UTC (permalink / raw)
To: Guenter Roeck, Ivan Mikhaylov
Cc: linux-watchdog, linux-aspeed, Andrew Jeffery, linux-kernel,
Joel Stanley, Wim Van Sebroeck, linux-arm-kernel
In-Reply-To: <20190821163220.GA11547@roeck-us.net>
[-- Attachment #1.1.1: Type: text/plain, Size: 4787 bytes --]
21.08.2019 19:32, Guenter Roeck wrote:
> On Wed, Aug 21, 2019 at 06:57:43PM +0300, Ivan Mikhaylov wrote:
>> Set WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION into WDT_CLEAR_TIMEOUT_STATUS
>> to clear out boot code source and re-enable access to the primary SPI flash
>> chip while booted via wdt2 from the alternate chip.
>>
>> AST2400 datasheet says:
>> "In the 2nd flash booting mode, all the address mapping to CS0# would be
>> re-directed to CS1#. And CS0# is not accessable under this mode. To access
>> CS0#, firmware should clear the 2nd boot mode register in the WDT2 status
>> register WDT30.bit[1]."
> Is there reason to not do this automatically when loading the module
> in alt-boot mode ? What means does userspace have to determine if CS0
> or CS1 is active at any given time ? If there is reason to ever have CS1
> active instead of CS0, what means would userspace have to enable it ?
Yes, there is. The driver is loaded long before the filesystems are mounted. The filesystems, in the event of alternate/recovery boot, need to be mounted from the same chip that the kernel was booted. For one reason because the main chip at CS0 is most probably corrupt. If you clear that bit when driver is loaded, your software will not know that and will try to mount the wrong filesystems. The whole idea of ASPEED's switching chipselects is to have identical firmware in both chips, without the need to process the alternate boot state in any way except for indicating a successful boot and restoring access to CS0 when needed.
The userspace can read bootstatus sysfs node to determine if an alternate boot has occured.
With ASPEED, CS1 is activated automatically by wdt2 when system fails to boot from the primary flash chip (at CS0) and disable the watchdog to indicate a successful boot. When that happens, both CS0 and CS1 controls get routed in hardware to CS1 line, making the primary flash chip inaccessible. Depending on the architecture of the user-space software, it may choose to re-enable access to the primary chip via CS0 at different times. There must be a way to do so.
> If userspace can not really determine if CS1 or CS0 is active, all it could
> ever do was to enable CS0 to be in a deterministic state. If so, it doesn't
> make sense to ever have CS1 active, and re-enabling CS0 could be automatic.
>
> Similar, if CS1 can ever be enabled, there is no means for userspace to ensure
> that some other application did not re-enable CS0 while it believes that CS1
> is enabled. If there is no means for userspace to enable CS1, it can never be
> sure what is enabled (because some other entity may have enabled CS0 while
> userspace just thought that CS1 is still enabled). Again, the only means
> to guarantee a well defined state would be to explicitly enable CS0 and provive
> no means to enable CS1. Again, this could be done during boot, not requiring
> an explicit request from userspace.
Please understand that activation of CS1 in place of CS0 is NOT a software choice!
>> + if (unlikely(!wdt))
>> + return -ENODEV;
>> +
> How would this ever happen, and how / where is drvdata set to NULL ?
This is purely for robustness. Seeing a pointer obtained via a function accessed without first checking it for validity makes me nervous.
This code most probably adds nothing at the assembly level.
>
>> + writel(WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION,
>> + wdt->base + WDT_CLEAR_TIMEOUT_STATUS);
>> + wdt->wdd.bootstatus |= WDIOF_EXTERN1;
> The variable reflects the _boot status_. It should not change after booting.
Is there any documentation that dictates that? All I could find is
"bootstatus: status of the device after booting". That doesn't look to me like it absolutely can not change to reflect the updated status (that is, to reflect that the originally set up alternate CS routing has been reset to normal).
If you absolutely disallow that, I think we could make 'access_cs0' readable instead, so it could report the current state of the boot code selection bit. Reverted, I suppose. That way 'access_cs0' would report 1 after 1 has been written to it (it wouldn't be possible to write a zero).
> @@ -223,6 +248,9 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>
> wdt->ctrl = WDT_CTRL_1MHZ_CLK;
>
> + if (of_property_read_bool(np, "aspeed,alt-boot"))
> + wdt->wdd.groups = bswitch_groups;
> +
> Why does this have to be separate to the existing evaluation of
> aspeed,alt-boot, and why does the existing code not work ?
>
> Also, is it guaranteed that this does not interfer with existing
> support for alt-boot ?
I think Ivan will comment on this.
With best regards,
Alexander Amelkin,
BIOS/BMC Team Lead, YADRO
https://yadro.com
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/3] arm64: imx8mq: add imx8mq iomux-gpr field defines
From: Guido Günther @ 2019-08-21 17:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, DTML, Jernej Skrabec, Daniel Vetter, Neil Armstrong,
David Airlie, Fabio Estevam, Sascha Hauer, Jonas Karlman,
Linux Kernel Mailing List, dri-devel, Andrzej Hajda, Rob Herring,
NXP Linux Team, Pengutronix Kernel Team, Robert Chiras, Shawn Guo,
Lee Jones, Sam Ravnborg, Linux ARM, Laurent Pinchart
In-Reply-To: <CAK8P3a1q9G8VKgNKh+6khzoW3bFTVR_Zorygy=Qqsq-PYzM4=g@mail.gmail.com>
Hi,
On Tue, Aug 13, 2019 at 01:07:52PM +0200, Arnd Bergmann wrote:
> On Tue, Aug 13, 2019 at 12:10 PM Guido Günther <agx@sigxcpu.org> wrote:
> > On Tue, Aug 13, 2019 at 10:08:44AM +0200, Arnd Bergmann wrote:
> > > On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
> > > >
> > > > This adds all the gpr registers and the define needed for selecting
> > > > the input source in the imx-nwl drm bridge.
> > > >
> > > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > > +
> > > > +#define IOMUXC_GPR0 0x00
> > > > +#define IOMUXC_GPR1 0x04
> > > > +#define IOMUXC_GPR2 0x08
> > > > +#define IOMUXC_GPR3 0x0c
> > > > +#define IOMUXC_GPR4 0x10
> > > > +#define IOMUXC_GPR5 0x14
> > > > +#define IOMUXC_GPR6 0x18
> > > > +#define IOMUXC_GPR7 0x1c
> > > (more of the same)
> > >
> > > huh?
> >
> > These are the names from the imx8MQ reference manual (general purpose
> > registers, they lump together all sorts of things), it's the same on
> > imx6/imx7):
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx7-iomuxc-gpr.h
> >
> > > > +/* i.MX8Mq iomux gpr register field defines */
> > > > +#define IMX8MQ_GPR13_MIPI_MUX_SEL BIT(2)
> > >
> > > I think this define should probably be local to the pinctrl driver, to
> > > ensure that no other drivers fiddle with the registers manually.
> >
> > The purpose of these bits is for a driver to fiddle with them to select
> > the input source. Similar on imx7 it's already used for e.g. the phy
> > refclk in the pci controller:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c#n638
>
> That one should likely use either the clk interface or the phy
> interface instead.
>
> > The GPRs are not about pad configuration but gather all sorts of things
> > (section 8.2.4 of the imx8mq reference manual): pcie setup, dsi related
> > bits so I don't think this should be done via a pinctrl
> > driver. Should we handle that differently than on imx6/7?
>
> It would be nice to fix the existing code as well, but for the moment,
> I only think we should not add more of that.
>
> Generally speaking, we can use syscon to do random things that don't
> have a subsystem of their own, but we should not use it to do things
> that have an existing driver framework like pinctrl, clock, reset, phy
> etc.
Since it's not an external pin i opted to use MUX_MMIO instead which
seems like a good fit here. Does that make sense?
Cheers,
-- Guido
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 2/3] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Will Deacon @ 2019-08-21 17:35 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arch, Dave Hansen, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, linux-doc, Will Deacon, linux-mm, Andrew Morton,
Vincenzo Frascino, Dave P Martin, linux-arm-kernel
In-Reply-To: <20190821164730.47450-3-catalin.marinas@arm.com>
On Wed, Aug 21, 2019 at 05:47:29PM +0100, Catalin Marinas wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
> (EL0) to perform memory accesses through 64-bit pointers with a non-zero
> top byte. Introduce the document describing the relaxation of the
> syscall ABI that allows userspace to pass certain tagged pointers to
> kernel syscalls.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Cc: Kevin Brodsky <kevin.brodsky@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> Documentation/arm64/tagged-address-abi.rst | 156 +++++++++++++++++++++
> 1 file changed, 156 insertions(+)
> create mode 100644 Documentation/arm64/tagged-address-abi.rst
Thanks, I'll pick this on up.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 3/3] arm64: Relax Documentation/arm64/tagged-pointers.rst
From: Will Deacon @ 2019-08-21 17:33 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arch, Dave Hansen, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, linux-doc, Will Deacon, linux-mm, Andrew Morton,
Vincenzo Frascino, Dave P Martin, linux-arm-kernel
In-Reply-To: <20190821164730.47450-4-catalin.marinas@arm.com>
On Wed, Aug 21, 2019 at 05:47:30PM +0100, Catalin Marinas wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
> (EL0) to perform memory accesses through 64-bit pointers with a non-zero
> top byte. However, such pointers were not allowed at the user-kernel
> syscall ABI boundary.
>
> With the Tagged Address ABI patchset, it is now possible to pass tagged
> pointers to the syscalls. Relax the requirements described in
> tagged-pointers.rst to be compliant with the behaviours guaranteed by
> the AArch64 Tagged Address ABI.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Cc: Kevin Brodsky <kevin.brodsky@arm.com>
> Acked-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> Documentation/arm64/tagged-pointers.rst | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/arm64/tagged-pointers.rst b/Documentation/arm64/tagged-pointers.rst
> index 2acdec3ebbeb..04f2ba9b779e 100644
> --- a/Documentation/arm64/tagged-pointers.rst
> +++ b/Documentation/arm64/tagged-pointers.rst
> @@ -20,7 +20,9 @@ Passing tagged addresses to the kernel
> --------------------------------------
>
> All interpretation of userspace memory addresses by the kernel assumes
> -an address tag of 0x00.
> +an address tag of 0x00, unless the application enables the AArch64
> +Tagged Address ABI explicitly
> +(Documentation/arm64/tagged-address-abi.rst).
>
> This includes, but is not limited to, addresses found in:
>
> @@ -33,13 +35,15 @@ This includes, but is not limited to, addresses found in:
> - the frame pointer (x29) and frame records, e.g. when interpreting
> them to generate a backtrace or call graph.
>
> -Using non-zero address tags in any of these locations may result in an
> -error code being returned, a (fatal) signal being raised, or other modes
> -of failure.
> +Using non-zero address tags in any of these locations when the
> +userspace application did not enable the AArch64 Tagged Address ABI may
> +result in an error code being returned, a (fatal) signal being raised,
> +or other modes of failure.
>
> -For these reasons, passing non-zero address tags to the kernel via
> -system calls is forbidden, and using a non-zero address tag for sp is
> -strongly discouraged.
> +For these reasons, when the AArch64 Tagged Address ABI is disabled,
> +passing non-zero address tags to the kernel via system calls is
> +forbidden, and using a non-zero address tag for sp is strongly
> +discouraged.
>
> Programs maintaining a frame pointer and frame records that use non-zero
> address tags may suffer impaired or inaccurate debug and profiling
> @@ -59,6 +63,11 @@ be preserved.
> The architecture prevents the use of a tagged PC, so the upper byte will
> be set to a sign-extension of bit 55 on exception return.
>
> +This behaviour is maintained when the AArch64 Tagged Address ABI is
> +enabled. In addition, with the exceptions above, the kernel will
> +preserve any non-zero tags passed by the user via syscalls and stored in
> +kernel data structures (e.g. ``set_robust_list()``, ``sigaltstack()``).
Hmm. I can see the need to provide this guarantee for things like
set_robust_list(), but the problem is that the statement above is too broad
and isn't strictly true: for example, mmap() doesn't propagate the tag of
its address parameter into the VMA.
So I think we need to nail this down a bit more, but I'm having a really
hard time coming up with some wording :(
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity
From: Zenghui Yu @ 2019-08-21 17:13 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier
Cc: Dave Martin, Julien Grall, kvmarm, linux-arm-kernel,
Christoffer Dall
In-Reply-To: <20190821170052.169065-1-andre.przywara@arm.com>
On 2019/8/22 1:00, Andre Przywara wrote:
> At the moment we initialise the target *mask* of a virtual IRQ to the
> VCPU it belongs to, even though this mask is only defined for GICv2 and
> quickly runs out of bits for many GICv3 guests.
> This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
> ------
> [ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
> [ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
> ------
> Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
> dump is wrong, due to this very same problem.
>
> Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
> and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
> use the actual MPIDR for that, as the VCPU's system register is not
> initialised at this point yet. This is not really an issue, as ->mpidr
> is just used for the debugfs output and the IROUTER MMIO register, which
> does not exist in redistributors (dealing with SGIs and PPIs).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Dave Martin <dave.martin@arm.com>
> ---
> Hi,
>
> this came up here again, I think it fell through the cracks back in
> March:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/637209.html
>
> Cheers,
> Andre.
>
> virt/kvm/arm/vgic/vgic-init.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 80127ca9269f..8bce2f75e0c1 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -210,7 +210,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> irq->intid = i;
> irq->vcpu = NULL;
> irq->target_vcpu = vcpu;
> - irq->targets = 1U << vcpu->vcpu_id;
> kref_init(&irq->refcount);
> if (vgic_irq_is_sgi(i)) {
> /* SGIs */
> @@ -221,10 +220,14 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> irq->config = VGIC_CONFIG_LEVEL;
> }
>
> - if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> + if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
I still think that if user-space create VCPUs before vGIC (like what
Qemu does), the actual vGIC model will be unknown here. The UBSAN
warning will still show up when booting a vGIC-v3 guest (with Qemu).
Thanks,
zenghui
> irq->group = 1;
> - else
> + /* The actual MPIDR is not initialised at this point. */
> + irq->mpidr = 0;
> + } else {
> irq->group = 0;
> + irq->targets = 1U << vcpu->vcpu_id;
> + }
> }
>
> if (!irqchip_in_kernel(vcpu->kvm))
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity
From: Marc Zyngier @ 2019-08-21 17:12 UTC (permalink / raw)
To: Julien Grall, Andre Przywara; +Cc: kvmarm, linux-arm-kernel, Dave Martin
In-Reply-To: <ebd572f8-0fa7-83dc-d074-891a77787b0d@arm.com>
On 21/08/2019 18:01, Julien Grall wrote:
> Hi Andre,
>
> On 21/08/2019 18:00, Andre Przywara wrote:
>> At the moment we initialise the target *mask* of a virtual IRQ to the
>> VCPU it belongs to, even though this mask is only defined for GICv2 and
>> quickly runs out of bits for many GICv3 guests.
>> This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
>> ------
>> [ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
>> [ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
>> ------
>> Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
>> dump is wrong, due to this very same problem.
>>
>> Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
>> and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
>> use the actual MPIDR for that, as the VCPU's system register is not
>> initialised at this point yet. This is not really an issue, as ->mpidr
>> is just used for the debugfs output and the IROUTER MMIO register, which
>> does not exist in redistributors (dealing with SGIs and PPIs).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> Reported-by: Dave Martin <dave.martin@arm.com>
>
> Tested-by: Julien Grall <julien.grall@arm.com>
Sorry for having dropped the ball on that one. Now applied to
kvmarm/next, with Julien's TB and a Cc: stable.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity
From: Julien Grall @ 2019-08-21 17:01 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier; +Cc: kvmarm, linux-arm-kernel, Dave Martin
In-Reply-To: <20190821170052.169065-1-andre.przywara@arm.com>
Hi Andre,
On 21/08/2019 18:00, Andre Przywara wrote:
> At the moment we initialise the target *mask* of a virtual IRQ to the
> VCPU it belongs to, even though this mask is only defined for GICv2 and
> quickly runs out of bits for many GICv3 guests.
> This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
> ------
> [ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
> [ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
> ------
> Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
> dump is wrong, due to this very same problem.
>
> Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
> and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
> use the actual MPIDR for that, as the VCPU's system register is not
> initialised at this point yet. This is not really an issue, as ->mpidr
> is just used for the debugfs output and the IROUTER MMIO register, which
> does not exist in redistributors (dealing with SGIs and PPIs).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Dave Martin <dave.martin@arm.com>
Tested-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> Hi,
>
> this came up here again, I think it fell through the cracks back in
> March:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/637209.html
>
> Cheers,
> Andre.
>
> virt/kvm/arm/vgic/vgic-init.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 80127ca9269f..8bce2f75e0c1 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -210,7 +210,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> irq->intid = i;
> irq->vcpu = NULL;
> irq->target_vcpu = vcpu;
> - irq->targets = 1U << vcpu->vcpu_id;
> kref_init(&irq->refcount);
> if (vgic_irq_is_sgi(i)) {
> /* SGIs */
> @@ -221,10 +220,14 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> irq->config = VGIC_CONFIG_LEVEL;
> }
>
> - if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> + if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> irq->group = 1;
> - else
> + /* The actual MPIDR is not initialised at this point. */
> + irq->mpidr = 0;
> + } else {
> irq->group = 0;
> + irq->targets = 1U << vcpu->vcpu_id;
> + }
> }
>
> if (!irqchip_in_kernel(vcpu->kvm))
>
--
Julien Grall
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RESEND PATCH] KVM: arm: VGIC: properly initialise private IRQ affinity
From: Andre Przywara @ 2019-08-21 17:00 UTC (permalink / raw)
To: Marc Zyngier
Cc: Julien Grall, kvmarm, Christoffer Dall, linux-arm-kernel,
Dave Martin
At the moment we initialise the target *mask* of a virtual IRQ to the
VCPU it belongs to, even though this mask is only defined for GICv2 and
quickly runs out of bits for many GICv3 guests.
This behaviour triggers an UBSAN complaint for more than 32 VCPUs:
------
[ 5659.462377] UBSAN: Undefined behaviour in virt/kvm/arm/vgic/vgic-init.c:223:21
[ 5659.471689] shift exponent 32 is too large for 32-bit type 'unsigned int'
------
Also for GICv3 guests the reporting of TARGET in the "vgic-state" debugfs
dump is wrong, due to this very same problem.
Fix both issues by only initialising vgic_irq->targets for a vGICv2 guest,
and by initialising vgic_irq->mpdir for vGICv3 guests instead. We can't
use the actual MPIDR for that, as the VCPU's system register is not
initialised at this point yet. This is not really an issue, as ->mpidr
is just used for the debugfs output and the IROUTER MMIO register, which
does not exist in redistributors (dealing with SGIs and PPIs).
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reported-by: Dave Martin <dave.martin@arm.com>
---
Hi,
this came up here again, I think it fell through the cracks back in
March:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-March/637209.html
Cheers,
Andre.
virt/kvm/arm/vgic/vgic-init.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 80127ca9269f..8bce2f75e0c1 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -210,7 +210,6 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
irq->intid = i;
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
- irq->targets = 1U << vcpu->vcpu_id;
kref_init(&irq->refcount);
if (vgic_irq_is_sgi(i)) {
/* SGIs */
@@ -221,10 +220,14 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
irq->config = VGIC_CONFIG_LEVEL;
}
- if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
+ if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
irq->group = 1;
- else
+ /* The actual MPIDR is not initialised at this point. */
+ irq->mpidr = 0;
+ } else {
irq->group = 0;
+ irq->targets = 1U << vcpu->vcpu_id;
+ }
}
if (!irqchip_in_kernel(vcpu->kvm))
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v9 2/3] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Andrey Konovalov @ 2019-08-21 16:57 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arch, Dave Hansen, Szabolcs Nagy, Will Deacon,
Kevin Brodsky, open list:DOCUMENTATION,
Linux Memory Management List, Andrew Morton, Vincenzo Frascino,
Will Deacon, Dave P Martin, Linux ARM
In-Reply-To: <20190821164730.47450-3-catalin.marinas@arm.com>
On Wed, Aug 21, 2019 at 6:47 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
> (EL0) to perform memory accesses through 64-bit pointers with a non-zero
> top byte. Introduce the document describing the relaxation of the
> syscall ABI that allows userspace to pass certain tagged pointers to
> kernel syscalls.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Cc: Kevin Brodsky <kevin.brodsky@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> Documentation/arm64/tagged-address-abi.rst | 156 +++++++++++++++++++++
> 1 file changed, 156 insertions(+)
> create mode 100644 Documentation/arm64/tagged-address-abi.rst
>
> diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
> new file mode 100644
> index 000000000000..d4a85d535bf9
> --- /dev/null
> +++ b/Documentation/arm64/tagged-address-abi.rst
> @@ -0,0 +1,156 @@
> +==========================
> +AArch64 TAGGED ADDRESS ABI
> +==========================
> +
> +Authors: Vincenzo Frascino <vincenzo.frascino@arm.com>
> + Catalin Marinas <catalin.marinas@arm.com>
> +
> +Date: 21 August 2019
> +
> +This document describes the usage and semantics of the Tagged Address
> +ABI on AArch64 Linux.
> +
> +1. Introduction
> +---------------
> +
> +On AArch64 the ``TCR_EL1.TBI0`` bit is set by default, allowing
> +userspace (EL0) to perform memory accesses through 64-bit pointers with
> +a non-zero top byte. This document describes the relaxation of the
> +syscall ABI that allows userspace to pass certain tagged pointers to
> +kernel syscalls.
> +
> +2. AArch64 Tagged Address ABI
> +-----------------------------
> +
> +From the kernel syscall interface perspective and for the purposes of
> +this document, a "valid tagged pointer" is a pointer with a potentially
> +non-zero top-byte that references an address in the user process address
> +space obtained in one of the following ways:
> +
> +- ``mmap()`` syscall where either:
> +
> + - flags have the ``MAP_ANONYMOUS`` bit set or
> + - the file descriptor refers to a regular file (including those
> + returned by ``memfd_create()``) or ``/dev/zero``
> +
> +- ``brk()`` syscall (i.e. the heap area between the initial location of
> + the program break at process creation and its current location).
> +
> +- any memory mapped by the kernel in the address space of the process
> + during creation and with the same restrictions as for ``mmap()`` above
> + (e.g. data, bss, stack).
> +
> +The AArch64 Tagged Address ABI has two stages of relaxation depending
> +how the user addresses are used by the kernel:
> +
> +1. User addresses not accessed by the kernel but used for address space
> + management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
> + of valid tagged pointers in this context is always allowed.
> +
> +2. User addresses accessed by the kernel (e.g. ``write()``). This ABI
> + relaxation is disabled by default and the application thread needs to
> + explicitly enable it via ``prctl()`` as follows:
> +
> + - ``PR_SET_TAGGED_ADDR_CTRL``: enable or disable the AArch64 Tagged
> + Address ABI for the calling thread.
> +
> + The ``(unsigned int) arg2`` argument is a bit mask describing the
> + control mode used:
> +
> + - ``PR_TAGGED_ADDR_ENABLE``: enable AArch64 Tagged Address ABI.
> + Default status is disabled.
> +
> + Arguments ``arg3``, ``arg4``, and ``arg5`` must be 0.
> +
> + - ``PR_GET_TAGGED_ADDR_CTRL``: get the status of the AArch64 Tagged
> + Address ABI for the calling thread.
> +
> + Arguments ``arg2``, ``arg3``, ``arg4``, and ``arg5`` must be 0.
> +
> + The ABI properties described above are thread-scoped, inherited on
> + clone() and fork() and cleared on exec().
> +
> + Calling ``prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0)``
> + returns ``-EINVAL`` if the AArch64 Tagged Address ABI is globally
> + disabled by ``sysctl abi.tagged_addr_disabled=1``. The default
> + ``sysctl abi.tagged_addr_disabled`` configuration is 0.
> +
> +When the AArch64 Tagged Address ABI is enabled for a thread, the
> +following behaviours are guaranteed:
> +
> +- All syscalls except the cases mentioned in section 3 can accept any
> + valid tagged pointer.
> +
> +- The syscall behaviour is undefined for invalid tagged pointers: it may
> + result in an error code being returned, a (fatal) signal being raised,
> + or other modes of failure.
> +
> +- The syscall behaviour for a valid tagged pointer is the same as for
> + the corresponding untagged pointer.
> +
> +
> +A definition of the meaning of tagged pointers on AArch64 can be found
> +in Documentation/arm64/tagged-pointers.rst.
> +
> +3. AArch64 Tagged Address ABI Exceptions
> +-----------------------------------------
> +
> +The following system call parameters must be untagged regardless of the
> +ABI relaxation:
> +
> +- ``prctl()`` other than pointers to user data either passed directly or
> + indirectly as arguments to be accessed by the kernel.
> +
> +- ``ioctl()`` other than pointers to user data either passed directly or
> + indirectly as arguments to be accessed by the kernel.
> +
> +- ``shmat()`` and ``shmdt()``.
> +
> +Any attempt to use non-zero tagged pointers may result in an error code
> +being returned, a (fatal) signal being raised, or other modes of
> +failure.
> +
> +4. Example of correct usage
> +---------------------------
> +.. code-block:: c
> +
> + #include <stdlib.h>
> + #include <string.h>
> + #include <unistd.h>
> + #include <sys/mman.h>
> + #include <sys/prctl.h>
> +
> + #define PR_SET_TAGGED_ADDR_CTRL 55
> + #define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> +
> + #define TAG_SHIFT 56
> +
> + int main(void)
> + {
> + int tbi_enabled = 0;
> + unsigned long tag = 0;
> + char *ptr;
> +
> + /* check/enable the tagged address ABI */
> + if (!prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0))
> + tbi_enabled = 1;
> +
> + /* memory allocation */
> + ptr = mmap(NULL, sysconf(_SC_PAGE_SIZE), PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + if (ptr == MAP_FAILED)
> + return 1;
> +
> + /* set a non-zero tag if the ABI is available */
> + if (tbi_enabled)
> + tag = rand() & 0xff;
> + ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +
> + /* memory access to a tagged address */
> + strcpy(ptr, "tagged pointer\n");
> +
> + /* syscall with a tagged pointer */
> + write(1, ptr, strlen(ptr));
> +
> + return 0;
> + }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
From: Will Deacon @ 2019-08-21 16:53 UTC (permalink / raw)
To: Mark Rutland
Cc: Ganapatrao Kulkarni, corbet@lwn.net, Jan Glauber,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Robert Richter, Ganapatrao Kulkarni,
Jayachandran Chandrasekharan Nair,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190813110345.GD866@lakrids.cambridge.arm.com>
On Tue, Aug 13, 2019 at 12:03:45PM +0100, Mark Rutland wrote:
> On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> > On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> > >
> > > It would be worth pointing out in the commit message how the CCPI2
> > > counters differ from the others. I realise you have that in the body of
> > > patch 1, but it's critical information when reviewing this patch...
> >
> > Ok, I will add in next version.
> > >
> > > >
> > > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > > > ---
> > > > drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > > > 1 file changed, 214 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > > index 43d76c85da56..a4e1273eafa3 100644
> > > > --- a/drivers/perf/thunderx2_pmu.c
> > > > +++ b/drivers/perf/thunderx2_pmu.c
> > > > @@ -17,22 +17,31 @@
> > > > */
> > > >
> > > > #define TX2_PMU_MAX_COUNTERS 4
> > >
> > > Shouldn't this be 8 now?
> >
> > It is kept unchanged to 4(as suggested by Will), which is same for
> > both L3 and DMC.
> > For CCPI2 this macro is not used.
>
> Hmmm....
>
> I disagree with that suggestion given that this also affects the
> active_counters bitmap size (and thus it is not correctly sized as of
> this patch), and it doesn't really save us much.
>
> I think it would be better to bump this to 8 and always update the
> events array, even though it will be unused for CCPI2. That's less
> surprising, needs fewer special-cases, and we can use the hrtimer
> function pointer alone to determine if we need to do any hrtimer work.
tbf, my complaint was actually about some macros applying to the whole
PMU whilst others refer only to DMC/L3C and this not being apparent from
the naming:
https://lkml.org/lkml/2019/6/27/250
so I'm fine having TX2_PMU_DMC_L3C_MAX_COUNTERS and
TX2_PMU_CCPI2_MAX_COUNTERS, but that sort of naming needs to be consistent
unless the macro/definition really applies to both. That fed the suggestion
that GET_EVENTID could be generic and switch on the event type internally
instead of at the caller.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 9/9] ARM: dts: at91: at91sam9x5dm.dtsi: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:48 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-9-uwe@kleine-koenig.org>
On 12/08/2019 23:27:57+0200, Uwe Kleine-König wrote:
> - use tags from included dtsi instead of duplicating the hierarchy
>
> There are no differences in the generated .dtbs
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91sam9x5dm.dtsi | 86 ++++++++++++++---------------
> 1 file changed, 41 insertions(+), 45 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v9 3/3] arm64: Relax Documentation/arm64/tagged-pointers.rst
From: Catalin Marinas @ 2019-08-21 16:47 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: linux-arch, linux-doc, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, Will Deacon, Dave Hansen, Andrew Morton,
Vincenzo Frascino, Will Deacon, Dave P Martin
In-Reply-To: <20190821164730.47450-1-catalin.marinas@arm.com>
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
(EL0) to perform memory accesses through 64-bit pointers with a non-zero
top byte. However, such pointers were not allowed at the user-kernel
syscall ABI boundary.
With the Tagged Address ABI patchset, it is now possible to pass tagged
pointers to the syscalls. Relax the requirements described in
tagged-pointers.rst to be compliant with the behaviours guaranteed by
the AArch64 Tagged Address ABI.
Cc: Will Deacon <will.deacon@arm.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: Kevin Brodsky <kevin.brodsky@arm.com>
Acked-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
Documentation/arm64/tagged-pointers.rst | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/Documentation/arm64/tagged-pointers.rst b/Documentation/arm64/tagged-pointers.rst
index 2acdec3ebbeb..04f2ba9b779e 100644
--- a/Documentation/arm64/tagged-pointers.rst
+++ b/Documentation/arm64/tagged-pointers.rst
@@ -20,7 +20,9 @@ Passing tagged addresses to the kernel
--------------------------------------
All interpretation of userspace memory addresses by the kernel assumes
-an address tag of 0x00.
+an address tag of 0x00, unless the application enables the AArch64
+Tagged Address ABI explicitly
+(Documentation/arm64/tagged-address-abi.rst).
This includes, but is not limited to, addresses found in:
@@ -33,13 +35,15 @@ This includes, but is not limited to, addresses found in:
- the frame pointer (x29) and frame records, e.g. when interpreting
them to generate a backtrace or call graph.
-Using non-zero address tags in any of these locations may result in an
-error code being returned, a (fatal) signal being raised, or other modes
-of failure.
+Using non-zero address tags in any of these locations when the
+userspace application did not enable the AArch64 Tagged Address ABI may
+result in an error code being returned, a (fatal) signal being raised,
+or other modes of failure.
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
-strongly discouraged.
+For these reasons, when the AArch64 Tagged Address ABI is disabled,
+passing non-zero address tags to the kernel via system calls is
+forbidden, and using a non-zero address tag for sp is strongly
+discouraged.
Programs maintaining a frame pointer and frame records that use non-zero
address tags may suffer impaired or inaccurate debug and profiling
@@ -59,6 +63,11 @@ be preserved.
The architecture prevents the use of a tagged PC, so the upper byte will
be set to a sign-extension of bit 55 on exception return.
+This behaviour is maintained when the AArch64 Tagged Address ABI is
+enabled. In addition, with the exceptions above, the kernel will
+preserve any non-zero tags passed by the user via syscalls and stored in
+kernel data structures (e.g. ``set_robust_list()``, ``sigaltstack()``).
+
Other considerations
--------------------
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v9 2/3] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Catalin Marinas @ 2019-08-21 16:47 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: linux-arch, linux-doc, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, Will Deacon, Dave Hansen, Andrew Morton,
Vincenzo Frascino, Will Deacon, Dave P Martin
In-Reply-To: <20190821164730.47450-1-catalin.marinas@arm.com>
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
(EL0) to perform memory accesses through 64-bit pointers with a non-zero
top byte. Introduce the document describing the relaxation of the
syscall ABI that allows userspace to pass certain tagged pointers to
kernel syscalls.
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: Kevin Brodsky <kevin.brodsky@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
Documentation/arm64/tagged-address-abi.rst | 156 +++++++++++++++++++++
1 file changed, 156 insertions(+)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
new file mode 100644
index 000000000000..d4a85d535bf9
--- /dev/null
+++ b/Documentation/arm64/tagged-address-abi.rst
@@ -0,0 +1,156 @@
+==========================
+AArch64 TAGGED ADDRESS ABI
+==========================
+
+Authors: Vincenzo Frascino <vincenzo.frascino@arm.com>
+ Catalin Marinas <catalin.marinas@arm.com>
+
+Date: 21 August 2019
+
+This document describes the usage and semantics of the Tagged Address
+ABI on AArch64 Linux.
+
+1. Introduction
+---------------
+
+On AArch64 the ``TCR_EL1.TBI0`` bit is set by default, allowing
+userspace (EL0) to perform memory accesses through 64-bit pointers with
+a non-zero top byte. This document describes the relaxation of the
+syscall ABI that allows userspace to pass certain tagged pointers to
+kernel syscalls.
+
+2. AArch64 Tagged Address ABI
+-----------------------------
+
+From the kernel syscall interface perspective and for the purposes of
+this document, a "valid tagged pointer" is a pointer with a potentially
+non-zero top-byte that references an address in the user process address
+space obtained in one of the following ways:
+
+- ``mmap()`` syscall where either:
+
+ - flags have the ``MAP_ANONYMOUS`` bit set or
+ - the file descriptor refers to a regular file (including those
+ returned by ``memfd_create()``) or ``/dev/zero``
+
+- ``brk()`` syscall (i.e. the heap area between the initial location of
+ the program break at process creation and its current location).
+
+- any memory mapped by the kernel in the address space of the process
+ during creation and with the same restrictions as for ``mmap()`` above
+ (e.g. data, bss, stack).
+
+The AArch64 Tagged Address ABI has two stages of relaxation depending
+how the user addresses are used by the kernel:
+
+1. User addresses not accessed by the kernel but used for address space
+ management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
+ of valid tagged pointers in this context is always allowed.
+
+2. User addresses accessed by the kernel (e.g. ``write()``). This ABI
+ relaxation is disabled by default and the application thread needs to
+ explicitly enable it via ``prctl()`` as follows:
+
+ - ``PR_SET_TAGGED_ADDR_CTRL``: enable or disable the AArch64 Tagged
+ Address ABI for the calling thread.
+
+ The ``(unsigned int) arg2`` argument is a bit mask describing the
+ control mode used:
+
+ - ``PR_TAGGED_ADDR_ENABLE``: enable AArch64 Tagged Address ABI.
+ Default status is disabled.
+
+ Arguments ``arg3``, ``arg4``, and ``arg5`` must be 0.
+
+ - ``PR_GET_TAGGED_ADDR_CTRL``: get the status of the AArch64 Tagged
+ Address ABI for the calling thread.
+
+ Arguments ``arg2``, ``arg3``, ``arg4``, and ``arg5`` must be 0.
+
+ The ABI properties described above are thread-scoped, inherited on
+ clone() and fork() and cleared on exec().
+
+ Calling ``prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0)``
+ returns ``-EINVAL`` if the AArch64 Tagged Address ABI is globally
+ disabled by ``sysctl abi.tagged_addr_disabled=1``. The default
+ ``sysctl abi.tagged_addr_disabled`` configuration is 0.
+
+When the AArch64 Tagged Address ABI is enabled for a thread, the
+following behaviours are guaranteed:
+
+- All syscalls except the cases mentioned in section 3 can accept any
+ valid tagged pointer.
+
+- The syscall behaviour is undefined for invalid tagged pointers: it may
+ result in an error code being returned, a (fatal) signal being raised,
+ or other modes of failure.
+
+- The syscall behaviour for a valid tagged pointer is the same as for
+ the corresponding untagged pointer.
+
+
+A definition of the meaning of tagged pointers on AArch64 can be found
+in Documentation/arm64/tagged-pointers.rst.
+
+3. AArch64 Tagged Address ABI Exceptions
+-----------------------------------------
+
+The following system call parameters must be untagged regardless of the
+ABI relaxation:
+
+- ``prctl()`` other than pointers to user data either passed directly or
+ indirectly as arguments to be accessed by the kernel.
+
+- ``ioctl()`` other than pointers to user data either passed directly or
+ indirectly as arguments to be accessed by the kernel.
+
+- ``shmat()`` and ``shmdt()``.
+
+Any attempt to use non-zero tagged pointers may result in an error code
+being returned, a (fatal) signal being raised, or other modes of
+failure.
+
+4. Example of correct usage
+---------------------------
+.. code-block:: c
+
+ #include <stdlib.h>
+ #include <string.h>
+ #include <unistd.h>
+ #include <sys/mman.h>
+ #include <sys/prctl.h>
+
+ #define PR_SET_TAGGED_ADDR_CTRL 55
+ #define PR_TAGGED_ADDR_ENABLE (1UL << 0)
+
+ #define TAG_SHIFT 56
+
+ int main(void)
+ {
+ int tbi_enabled = 0;
+ unsigned long tag = 0;
+ char *ptr;
+
+ /* check/enable the tagged address ABI */
+ if (!prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0))
+ tbi_enabled = 1;
+
+ /* memory allocation */
+ ptr = mmap(NULL, sysconf(_SC_PAGE_SIZE), PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (ptr == MAP_FAILED)
+ return 1;
+
+ /* set a non-zero tag if the ABI is available */
+ if (tbi_enabled)
+ tag = rand() & 0xff;
+ ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
+
+ /* memory access to a tagged address */
+ strcpy(ptr, "tagged pointer\n");
+
+ /* syscall with a tagged pointer */
+ write(1, ptr, strlen(ptr));
+
+ return 0;
+ }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v9 1/3] mm: untag user pointers in mmap/munmap/mremap/brk
From: Catalin Marinas @ 2019-08-21 16:47 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: linux-arch, linux-doc, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, Dave Hansen, Andrew Morton, Vincenzo Frascino,
Will Deacon, Dave P Martin
In-Reply-To: <20190821164730.47450-1-catalin.marinas@arm.com>
There isn't a good reason to differentiate between the user address
space layout modification syscalls and the other memory
permission/attributes ones (e.g. mprotect, madvise) w.r.t. the tagged
address ABI. Untag the user addresses on entry to these functions.
Acked-by: Will Deacon <will@kernel.org>
Acked-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
mm/mmap.c | 5 +++++
mm/mremap.c | 6 +-----
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index 7e8c3e8ae75f..b766b633b7ae 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -201,6 +201,8 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
bool downgraded = false;
LIST_HEAD(uf);
+ brk = untagged_addr(brk);
+
if (down_write_killable(&mm->mmap_sem))
return -EINTR;
@@ -1573,6 +1575,8 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
struct file *file = NULL;
unsigned long retval;
+ addr = untagged_addr(addr);
+
if (!(flags & MAP_ANONYMOUS)) {
audit_mmap_fd(fd, flags);
file = fget(fd);
@@ -2874,6 +2878,7 @@ EXPORT_SYMBOL(vm_munmap);
SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
{
+ addr = untagged_addr(addr);
profile_munmap(addr);
return __vm_munmap(addr, len, true);
}
diff --git a/mm/mremap.c b/mm/mremap.c
index 64c9a3b8be0a..1fc8a29fbe3f 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -606,12 +606,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
LIST_HEAD(uf_unmap_early);
LIST_HEAD(uf_unmap);
- /*
- * Architectures may interpret the tag passed to mmap as a background
- * colour for the corresponding vma. For mremap we don't allow tagged
- * new_addr to preserve similar behaviour to mmap.
- */
addr = untagged_addr(addr);
+ new_addr = untagged_addr(new_addr);
if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
return ret;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v9 0/3] arm64 tagged address ABI
From: Catalin Marinas @ 2019-08-21 16:47 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: linux-arch, linux-doc, Szabolcs Nagy, Andrey Konovalov,
Kevin Brodsky, Dave Hansen, Andrew Morton, Vincenzo Frascino,
Will Deacon, Dave P Martin
Hi,
This series is an update to the arm64 tagged address ABI documentation
patches v8, posted here:
http://lkml.kernel.org/r/20190815154403.16473-1-catalin.marinas@arm.com
From v8, I dropped patches 2 and 3 as they've been queued by Will via
the arm64 tree. Reposting patch 1 (unmodified) as it should be merged
via the mm tree.
Changes in v9:
- Replaced the emphasized/bold font with a typewriter one for
function/constant names
- Simplified the mmap/brk bullet points when describing the tagged
pointer origin
- Reworded expected syscall behaviour with valid tagged pointers
- Reworded the prctl/ioctl restrictions to clarify the allowed tagged
pointers w.r.t. user data access by the kernel
Catalin Marinas (1):
mm: untag user pointers in mmap/munmap/mremap/brk
Vincenzo Frascino (2):
arm64: Define Documentation/arm64/tagged-address-abi.rst
arm64: Relax Documentation/arm64/tagged-pointers.rst
Documentation/arm64/tagged-address-abi.rst | 156 +++++++++++++++++++++
Documentation/arm64/tagged-pointers.rst | 23 ++-
mm/mmap.c | 5 +
mm/mremap.c | 6 +-
4 files changed, 178 insertions(+), 12 deletions(-)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 8/9] ARM: dts: at91: at91sam9x5_lcd.dtsi: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:47 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-8-uwe@kleine-koenig.org>
On 12/08/2019 23:27:56+0200, Uwe Kleine-König wrote:
> - use tags from included dtsi instead of duplicating the hierarchy
>
> There are no differences in the generated .dtbs
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91sam9x5_lcd.dtsi | 194 +++++++++++++-------------
> 1 file changed, 97 insertions(+), 97 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 7/9] ARM: dts: at91: at91sam9xx5ek: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:46 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-7-uwe@kleine-koenig.org>
On 12/08/2019 23:27:55+0200, Uwe Kleine-König wrote:
> - newline between properties and sub-nodes
> - use tags from included dtsi instead of duplicating the hierarchy
>
> There are no differences in the generated .dtbs
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91sam9g15ek.dts | 12 +-
> arch/arm/boot/dts/at91sam9g25ek.dts | 89 +++++-----
> arch/arm/boot/dts/at91sam9g35ek.dts | 22 +--
> arch/arm/boot/dts/at91sam9x25ek.dts | 36 ++--
> arch/arm/boot/dts/at91sam9x35ek.dts | 43 +++--
> arch/arm/boot/dts/at91sam9x5ek.dtsi | 265 ++++++++++++++--------------
> 6 files changed, 224 insertions(+), 243 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/9] ARM: dts: at91: at91sam9g15: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:46 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-6-uwe@kleine-koenig.org>
On 12/08/2019 23:27:54+0200, Uwe Kleine-König wrote:
> - use tags from included dtsi instead of duplicating the hierarchy
>
> There are no differences in the generated .dtbs
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91sam9g15.dtsi | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 5/9] ARM: dts: at91: kizboxmini: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:45 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-5-uwe@kleine-koenig.org>
On 12/08/2019 23:27:53+0200, Uwe Kleine-König wrote:
> - use tags from included dtsi instead of duplicating the hierarchy
>
> There are no differences in the generated .dtb
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91-kizboxmini.dts | 179 +++++++++++++-------------
> 1 file changed, 88 insertions(+), 91 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/9] ARM: dts: at91: cosino: Style cleanup
From: Alexandre Belloni @ 2019-08-21 16:44 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Ludovic Desroches, linux-arm-kernel
In-Reply-To: <20190812212757.23432-4-uwe@kleine-koenig.org>
On 12/08/2019 23:27:52+0200, Uwe Kleine-König wrote:
> - newline between properties and sub-nodes
> - use tags from included dtsi instead of duplicating the hierarchy
> - status should be the last property
>
> There are no differences in the generated .dtb
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> arch/arm/boot/dts/at91-cosino.dtsi | 203 ++++++++++-----------
> arch/arm/boot/dts/at91-cosino_mega2560.dts | 93 +++++-----
> 2 files changed, 145 insertions(+), 151 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox