* [PATCH v2 1/2] powerpc/eeh: fix pseries_eeh_configure_bridge()
From: Sam Bobroff @ 2020-04-20 5:47 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <cover.1587361657.git.sbobroff@linux.ibm.com>
If a device is hot unplgged during EEH recovery, it's possible for the
RTAS call to ibm,configure-pe in pseries_eeh_configure() to return
parameter error (-3), however negative return values are not checked
for and this leads to an infinite loop.
Fix this by correctly bailing out on negative values.
Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
---
arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 893ba3f562c4..c4ef03bec0de 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -605,7 +605,7 @@ static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
config_addr, BUID_HI(pe->phb->buid),
BUID_LO(pe->phb->buid));
- if (!ret)
+ if (ret <= 0)
return ret;
/*
--
2.22.0.216.g00a2a96fc9
^ permalink raw reply related
* [PATCH v2 0/2] powerpc/eeh: Release EEH device state synchronously
From: Sam Bobroff @ 2020-04-20 5:47 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
Hi everyone,
Here are some fixes and cleanups that have come from other work but that I
think stand on their own.
Only one patch ("Release EEH device state synchronously", suggested by Oliver
O'Halloran) is a significant change: it moves the cleanup of some EEH device
data out of the (possibly asynchronous) device release handler and into the
(synchronously called) bus notifier. This is useful for future work as it makes
it easier to reason about the lifetimes of EEH structures.
Note that I've left a few WARN_ON_ONCEs in the code because I'm paranoid, but I
have not been able to hit them during testing.
Cheers,
Sam.
Notes for v2:
I've dropped both cleanup patches (3/4, 4/4) because that type of cleanup
(replacing a call to eeh_rmv_from_parent_pe() with one to eeh_remove_device())
is incorrect: if called during recovery, it will cause edev->pe to remain set
when it would have been cleared previously. This would lead to stale
information in the edev. I think there should be a way to simplify the code
around EEH_PE_KEEP but I'll look at that separately.
Patch set changelog follows:
Patch set v2:
Patch 1/2: powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 2/2: powerpc/eeh: Release EEH device state synchronously
- Added comment explaining why the add case can't be handled similarly to the remove case.
Dropped (was 3/4) powerpc/eeh: Remove workaround from eeh_add_device_late()
Dropped (was 4/4) powerpc/eeh: Clean up edev cleanup for VFs
Patch set v1:
Patch 1/4: powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 2/4: powerpc/eeh: Release EEH device state synchronously
Patch 3/4: powerpc/eeh: Remove workaround from eeh_add_device_late()
Patch 4/4: powerpc/eeh: Clean up edev cleanup for VFs
Sam Bobroff (2):
powerpc/eeh: fix pseries_eeh_configure_bridge()
powerpc/eeh: Release EEH device state synchronously
arch/powerpc/kernel/eeh.c | 31 ++++++++++++++++++++
arch/powerpc/kernel/pci-hotplug.c | 2 --
arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
3 files changed, 32 insertions(+), 3 deletions(-)
--
2.22.0.216.g00a2a96fc9
^ permalink raw reply
* [PATCH v2] powerpc/8xx: Fix STRICT_KERNEL_RWX startup test failure
From: Christophe Leroy @ 2020-04-20 5:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
WRITE_RO lkdtm test works.
But when selecting CONFIG_DEBUG_RODATA_TEST, the kernel reports
rodata_test: test data was not read only
This is because when rodata test runs, there are still old entries
in TLB.
Flush TLB after setting kernel pages RO or NX.
Fixes: d5f17ee96447 ("powerpc/8xx: don't disable large TLBs with CONFIG_STRICT_KERNEL_RWX")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/nohash/8xx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c
index 3189308dece4..d83a12c5bc7f 100644
--- a/arch/powerpc/mm/nohash/8xx.c
+++ b/arch/powerpc/mm/nohash/8xx.c
@@ -185,6 +185,7 @@ void mmu_mark_initmem_nx(void)
mmu_mapin_ram_chunk(etext8, einittext8, PAGE_KERNEL);
}
}
+ _tlbil_all();
}
#ifdef CONFIG_STRICT_KERNEL_RWX
@@ -199,6 +200,8 @@ void mmu_mark_rodata_ro(void)
~(LARGE_PAGE_SIZE_8M - 1)));
mmu_patch_addis(&patch__dtlbmiss_romem_top, -__pa(_sinittext));
+ _tlbil_all();
+
/* Update page tables for PTDUMP and BDI */
mmu_mapin_ram_chunk(0, sinittext, __pgprot(0));
mmu_mapin_ram_chunk(0, etext, PAGE_KERNEL_ROX);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/8xx: Fix STRICT_KERNEL_RWX startup test failure
From: Christophe Leroy @ 2020-04-20 5:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <0231963e81d6e72ff725212c14f4011d2ee36a9e.1587360530.git.christophe.leroy@c-s.fr>
Le 20/04/2020 à 07:29, Christophe Leroy a écrit :
> WRITE_RO lkdtm test works.
>
> But when selecting CONFIG_DEBUG_RODATA_TEST, the kernel reports
> rodata_test: test data was not read only
>
> This is because when rodata test runs, there are still old entries
> in TLB.
>
> Flush TLB after setting kernel pages RO or NX.
>
> Fixes: d5f17ee96447 ("powerpc/8xx: don't disable large TLBs with CONFIG_STRICT_KERNEL_RWX")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kvm/Makefile | 2 +-
Oops, this change shouldn't be there. Will send v2.
> arch/powerpc/mm/nohash/8xx.c | 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
Christophe
^ permalink raw reply
* [PATCH] powerpc/8xx: Fix STRICT_KERNEL_RWX startup test failure
From: Christophe Leroy @ 2020-04-20 5:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
WRITE_RO lkdtm test works.
But when selecting CONFIG_DEBUG_RODATA_TEST, the kernel reports
rodata_test: test data was not read only
This is because when rodata test runs, there are still old entries
in TLB.
Flush TLB after setting kernel pages RO or NX.
Fixes: d5f17ee96447 ("powerpc/8xx: don't disable large TLBs with CONFIG_STRICT_KERNEL_RWX")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kvm/Makefile | 2 +-
arch/powerpc/mm/nohash/8xx.c | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
index 2bfeaa13befb..906707d15810 100644
--- a/arch/powerpc/kvm/Makefile
+++ b/arch/powerpc/kvm/Makefile
@@ -135,4 +135,4 @@ obj-$(CONFIG_KVM_BOOK3S_32) += kvm.o
obj-$(CONFIG_KVM_BOOK3S_64_PR) += kvm-pr.o
obj-$(CONFIG_KVM_BOOK3S_64_HV) += kvm-hv.o
-obj-y += $(kvm-book3s_64-builtin-objs-y)
+obj-$(CONFIG_KVM_BOOK3S_64) += $(kvm-book3s_64-builtin-objs-y)
diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c
index 3189308dece4..d83a12c5bc7f 100644
--- a/arch/powerpc/mm/nohash/8xx.c
+++ b/arch/powerpc/mm/nohash/8xx.c
@@ -185,6 +185,7 @@ void mmu_mark_initmem_nx(void)
mmu_mapin_ram_chunk(etext8, einittext8, PAGE_KERNEL);
}
}
+ _tlbil_all();
}
#ifdef CONFIG_STRICT_KERNEL_RWX
@@ -199,6 +200,8 @@ void mmu_mark_rodata_ro(void)
~(LARGE_PAGE_SIZE_8M - 1)));
mmu_patch_addis(&patch__dtlbmiss_romem_top, -__pa(_sinittext));
+ _tlbil_all();
+
/* Update page tables for PTDUMP and BDI */
mmu_mapin_ram_chunk(0, sinittext, __pgprot(0));
mmu_mapin_ram_chunk(0, etext, PAGE_KERNEL_ROX);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 2/2] powerpc/fadump: consider reserved ranges while reserving memory
From: Mahesh J Salgaonkar @ 2020-04-20 5:20 UTC (permalink / raw)
To: Hari Bathini; +Cc: Vasant Hegde, Sourabh Jain, linuxppc-dev
In-Reply-To: <158387202999.17176.116917127748245682.stgit@hbathini.in.ibm.com>
On 2020-03-11 01:57:10 Wed, Hari Bathini wrote:
> Commit 0962e8004e97 ("powerpc/prom: Scan reserved-ranges node for
> memory reservations") enabled support to parse reserved-ranges DT
> node and reserve kernel memory falling in these ranges for F/W
> purposes. Memory reserved for FADump should not overlap with these
> ranges as it could corrupt memory meant for F/W or crash'ed kernel
> memory to be exported as vmcore.
>
> But since commit 579ca1a27675 ("powerpc/fadump: make use of memblock's
> bottom up allocation mode"), memblock_find_in_range() is being used to
> find the appropriate area to reserve memory for FADump, which can't
> account for reserved-ranges as these ranges are reserved only after
> FADump memory reservation.
>
> With reserved-ranges now being populated during early boot, look out
> for these memory ranges while reserving memory for FADump. Without
> this change, MPIPL on PowerNV systems aborts with hostboot failure,
> when memory reserved for FADump is less than 4096MB.
>
> Fixes: 579ca1a27675 ("powerpc/fadump: make use of memblock's bottom up allocation mode")
> Cc: stable@vger.kernel.org # v5.4+
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
> arch/powerpc/kernel/fadump.c | 76 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 7fcf4a8f..ab83be9 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -443,10 +443,70 @@ static int __init fadump_get_boot_mem_regions(void)
> return ret;
> }
>
> +/*
> + * Returns true, if the given range overlaps with reserved memory ranges
> + * starting at idx. Also, updates idx to index of overlapping memory range
> + * with the given memory range.
> + * False, otherwise.
> + */
> +static bool overlaps_reserved_ranges(u64 base, u64 end, int *idx)
> +{
> + bool ret = false;
> + int i;
> +
> + for (i = *idx; i < reserved_mrange_info.mem_range_cnt; i++) {
> + u64 rbase = reserved_mrange_info.mem_ranges[i].base;
> + u64 rend = rbase + reserved_mrange_info.mem_ranges[i].size;
> +
> + if (end <= rbase)
> + break;
> +
> + if ((end > rbase) && (base < rend)) {
> + *idx = i;
> + ret = true;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Locate a suitable memory area to reserve memory for FADump. While at it,
> + * lookup reserved-ranges & avoid overlap with them, as they are used by F/W.
> + */
> +static u64 __init fadump_locate_reserve_mem(u64 base, u64 size)
> +{
> + struct fadump_memory_range *mrngs;
> + phys_addr_t mstart, mend;
> + int idx = 0;
> + u64 i;
> +
> + mrngs = reserved_mrange_info.mem_ranges;
> + for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
> + &mstart, &mend, NULL) {
> + pr_debug("%llu) mstart: %llx, mend: %llx, base: %llx\n",
> + i, mstart, mend, base);
> +
> + if (mstart > base)
> + base = PAGE_ALIGN(mstart);
> +
> + while ((mend > base) && ((mend - base) >= size)) {
> + if (!overlaps_reserved_ranges(base, base + size, &idx))
> + goto out;
> +
> + base = mrngs[idx].base + mrngs[idx].size;
> + base = PAGE_ALIGN(base);
What happens when all the memory ranges found to be overlaped with
reserved ranges ? Shoudn't this function return NULL ? Looks like in
that case this function returns the last set base address which is
either still overlaped or not big enough in size.
Rest looks good to me.
Thanks,
-Mahesh.
> + }
> + }
> +
> +out:
> + return base;
> +}
> +
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/fadump: use static allocation for reserved memory ranges
From: Mahesh J Salgaonkar @ 2020-04-20 5:10 UTC (permalink / raw)
To: Hari Bathini; +Cc: Vasant Hegde, Sourabh Jain, linuxppc-dev
In-Reply-To: <158387202020.17176.15258122288090851051.stgit@hbathini.in.ibm.com>
On 2020-03-11 01:57:00 Wed, Hari Bathini wrote:
> At times, memory ranges have to be looked up during early boot, when
> kernel couldn't be initialized for dynamic memory allocation. In fact,
> reserved-ranges look up is needed during FADump memory reservation.
> Without accounting for reserved-ranges in reserving memory for FADump,
> MPIPL boot fails with memory corruption issues. So, extend memory
> ranges handling to support static allocation and populate reserved
> memory ranges during early boot.
>
> Fixes: dda9dbfeeb7a ("powerpc/fadump: consider reserved ranges while releasing memory")
> Cc: stable@vger.kernel.org # v5.4+
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Thanks,
-Mahesh.
> ---
> arch/powerpc/include/asm/fadump-internal.h | 4 +
> arch/powerpc/kernel/fadump.c | 77 ++++++++++++++++------------
> 2 files changed, 48 insertions(+), 33 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/fadump-internal.h b/arch/powerpc/include/asm/fadump-internal.h
> index c814a2b..8d61c8f 100644
> --- a/arch/powerpc/include/asm/fadump-internal.h
> +++ b/arch/powerpc/include/asm/fadump-internal.h
> @@ -64,12 +64,14 @@ struct fadump_memory_range {
> };
>
> /* fadump memory ranges info */
> +#define RNG_NAME_SZ 16
> struct fadump_mrange_info {
> - char name[16];
> + char name[RNG_NAME_SZ];
> struct fadump_memory_range *mem_ranges;
> u32 mem_ranges_sz;
> u32 mem_range_cnt;
> u32 max_mem_ranges;
> + bool is_static;
> };
>
> /* Platform specific callback functions */
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index ff0114a..7fcf4a8f 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -38,8 +38,17 @@ static void __init fadump_reserve_crash_area(u64 base);
>
> #ifndef CONFIG_PRESERVE_FA_DUMP
> static DEFINE_MUTEX(fadump_mutex);
> -struct fadump_mrange_info crash_mrange_info = { "crash", NULL, 0, 0, 0 };
> -struct fadump_mrange_info reserved_mrange_info = { "reserved", NULL, 0, 0, 0 };
> +struct fadump_mrange_info crash_mrange_info = { "crash", NULL, 0, 0, 0, false };
> +
> +#define RESERVED_RNGS_SZ 16384 /* 16K - 128 entries */
> +#define RESERVED_RNGS_CNT (RESERVED_RNGS_SZ / \
> + sizeof(struct fadump_memory_range))
> +static struct fadump_memory_range rngs[RESERVED_RNGS_CNT];
> +struct fadump_mrange_info reserved_mrange_info = { "reserved", rngs,
> + RESERVED_RNGS_SZ, 0,
> + RESERVED_RNGS_CNT, true };
> +
> +static void __init early_init_dt_scan_reserved_ranges(unsigned long node);
>
> #ifdef CONFIG_CMA
> static struct cma *fadump_cma;
> @@ -108,6 +117,11 @@ static int __init fadump_cma_init(void) { return 1; }
> int __init early_init_dt_scan_fw_dump(unsigned long node, const char *uname,
> int depth, void *data)
> {
> + if (depth == 0) {
> + early_init_dt_scan_reserved_ranges(node);
> + return 0;
> + }
> +
> if (depth != 1)
> return 0;
>
> @@ -726,10 +740,14 @@ void fadump_free_cpu_notes_buf(void)
>
> static void fadump_free_mem_ranges(struct fadump_mrange_info *mrange_info)
> {
> + if (mrange_info->is_static) {
> + mrange_info->mem_range_cnt = 0;
> + return;
> + }
> +
> kfree(mrange_info->mem_ranges);
> - mrange_info->mem_ranges = NULL;
> - mrange_info->mem_ranges_sz = 0;
> - mrange_info->max_mem_ranges = 0;
> + memset((void *)((u64)mrange_info + RNG_NAME_SZ), 0,
> + (sizeof(struct fadump_mrange_info) - RNG_NAME_SZ));
> }
>
> /*
> @@ -786,6 +804,12 @@ static inline int fadump_add_mem_range(struct fadump_mrange_info *mrange_info,
> if (mrange_info->mem_range_cnt == mrange_info->max_mem_ranges) {
> int ret;
>
> + if (mrange_info->is_static) {
> + pr_err("Reached array size limit for %s memory ranges\n",
> + mrange_info->name);
> + return -ENOSPC;
> + }
> +
> ret = fadump_alloc_mem_ranges(mrange_info);
> if (ret)
> return ret;
> @@ -1202,20 +1226,19 @@ static void sort_and_merge_mem_ranges(struct fadump_mrange_info *mrange_info)
> * Scan reserved-ranges to consider them while reserving/releasing
> * memory for FADump.
> */
> -static inline int fadump_scan_reserved_mem_ranges(void)
> +static void __init early_init_dt_scan_reserved_ranges(unsigned long node)
> {
> - struct device_node *root;
> const __be32 *prop;
> int len, ret = -1;
> unsigned long i;
>
> - root = of_find_node_by_path("/");
> - if (!root)
> - return ret;
> + /* reserved-ranges already scanned */
> + if (reserved_mrange_info.mem_range_cnt != 0)
> + return;
>
> - prop = of_get_property(root, "reserved-ranges", &len);
> + prop = of_get_flat_dt_prop(node, "reserved-ranges", &len);
> if (!prop)
> - return ret;
> + return;
>
> /*
> * Each reserved range is an (address,size) pair, 2 cells each,
> @@ -1237,7 +1260,8 @@ static inline int fadump_scan_reserved_mem_ranges(void)
> }
> }
>
> - return ret;
> + /* Compact reserved ranges */
> + sort_and_merge_mem_ranges(&reserved_mrange_info);
> }
>
> /*
> @@ -1251,32 +1275,21 @@ static void fadump_release_memory(u64 begin, u64 end)
> u64 ra_start, ra_end, tstart;
> int i, ret;
>
> - fadump_scan_reserved_mem_ranges();
> -
> ra_start = fw_dump.reserve_dump_area_start;
> ra_end = ra_start + fw_dump.reserve_dump_area_size;
>
> /*
> - * Add reserved dump area to reserved ranges list
> - * and exclude all these ranges while releasing memory.
> + * If reserved ranges array limit is hit, overwrite the last reserved
> + * memory range with reserved dump area to ensure it is excluded from
> + * the memory being released (reused for next FADump registration).
> */
> - ret = fadump_add_mem_range(&reserved_mrange_info, ra_start, ra_end);
> - if (ret != 0) {
> - /*
> - * Not enough memory to setup reserved ranges but the system is
> - * running shortage of memory. So, release all the memory except
> - * Reserved dump area (reused for next fadump registration).
> - */
> - if (begin < ra_end && end > ra_start) {
> - if (begin < ra_start)
> - fadump_release_reserved_area(begin, ra_start);
> - if (end > ra_end)
> - fadump_release_reserved_area(ra_end, end);
> - } else
> - fadump_release_reserved_area(begin, end);
> + if (reserved_mrange_info.mem_range_cnt ==
> + reserved_mrange_info.max_mem_ranges)
> + reserved_mrange_info.mem_range_cnt--;
>
> + ret = fadump_add_mem_range(&reserved_mrange_info, ra_start, ra_end);
> + if (ret != 0)
> return;
> - }
>
> /* Get the reserved ranges list in order first. */
> sort_and_merge_mem_ranges(&reserved_mrange_info);
>
--
Mahesh J Salgaonkar
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 4:31 UTC (permalink / raw)
To: Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, Adhemerval Zanella, musl
In-Reply-To: <20200420040926.GA11469@brightrain.aerifal.cx>
Excerpts from Rich Felker's message of April 20, 2020 2:09 pm:
> On Mon, Apr 20, 2020 at 12:32:21PM +1000, Nicholas Piggin wrote:
>> Excerpts from Rich Felker's message of April 20, 2020 11:34 am:
>> > On Mon, Apr 20, 2020 at 11:10:25AM +1000, Nicholas Piggin wrote:
>> >> Excerpts from Rich Felker's message of April 17, 2020 4:31 am:
>> >> > Note that because lr is clobbered we need at least once normally
>> >> > call-clobbered register that's not syscall clobbered to save lr in.
>> >> > Otherwise stack frame setup is required to spill it.
>> >>
>> >> The kernel would like to use r9-r12 for itself. We could do with fewer
>> >> registers, but we have some delay establishing the stack (depends on a
>> >> load which depends on a mfspr), and entry code tends to be quite store
>> >> heavy whereas on the caller side you have r1 set up (modulo stack
>> >> updates), and the system call is a long delay during which time the
>> >> store queue has significant time to drain.
>> >>
>> >> My feeling is it would be better for kernel to have these scratch
>> >> registers.
>> >
>> > If your new kernel syscall mechanism requires the caller to make a
>> > whole stack frame it otherwise doesn't need and spill registers to it,
>> > it becomes a lot less attractive. Some of those 90 cycles saved are
>> > immediately lost on the userspace side, plus you either waste icache
>> > at the call point or require the syscall to go through a
>> > userspace-side helper function that performs the spill and restore.
>>
>> You would be surprised how few cycles that takes on a high end CPU. Some
>> might be a couple of %. I am one for counting cycles mind you, I'm not
>> being flippant about it. If we can come up with something faster I'd be
>> up for it.
>
> If the cycle count is trivial then just do it on the kernel side.
The cycle count for user is, because you have r1 ready. Kernel does not
have its stack ready, it has to mfspr rX ; ld rY,N(rX); to get stack to
save into.
Which is also wasted work for a userspace.
Now that I think about it, no stack frame is even required! lr is saved
into the caller's stack when its clobbered with an asm, just as when
it's used for a function call.
>> > The right way to do this is to have the kernel preserve enough
>> > registers that userspace can avoid having any spills. It doesn't have
>> > to preserve everything, probably just enough to save lr. (BTW are
>>
>> Again, the problem is the kernel doesn't have its dependencies
>> immediately ready to spill, and spilling (may be) more costly
>> immediately after the call because we're doing a lot of stores.
>>
>> I could try measure this. Unfortunately our pipeline simulator tool
>> doesn't model system calls properly so it's hard to see what's happening
>> across the user/kernel horizon, I might check if that can be improved
>> or I can hack it by putting some isync in there or something.
>
> I think it's unlikely to make any real difference to the total number
> of cycles spent which side it happens on, but putting it on the kernel
> side makes it easier to avoid wasting size/icache at each syscall
> site.
>
>> > syscall arg registers still preserved? If not, this is a major cost on
>> > the userspace side, since any call point that has to loop-and-retry
>> > (e.g. futex) now needs to make its own place to store the original
>> > values.)
>>
>> Powerpc system calls never did. We could have scv preserve them, but
>> you'd still need to restore r3. We could make an ABI which does not
>> clobber r3 but puts the return value in r9, say. I'd like to see what
>> the user side code looks like to take advantage of such a thing though.
>
> Oh wow, I hadn't realized that, but indeed the code we have now is
> allowing for the kernel to clobber them all. So at least this isn't
> getting any worse I guess. I think it was a very poor choice of
> behavior though and a disadvantage vs what other archs do (some of
> them preserve all registers; others preserve only normally call-saved
> ones plus the syscall arg ones and possibly a few other specials).
Well, we could change it. Does the generated code improve significantly
we take those clobbers away?
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-20 4:09 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-dev, libc-alpha, linuxppc-dev, Adhemerval Zanella, musl
In-Reply-To: <1587348538.l1ioqml73m.astroid@bobo.none>
On Mon, Apr 20, 2020 at 12:32:21PM +1000, Nicholas Piggin wrote:
> Excerpts from Rich Felker's message of April 20, 2020 11:34 am:
> > On Mon, Apr 20, 2020 at 11:10:25AM +1000, Nicholas Piggin wrote:
> >> Excerpts from Rich Felker's message of April 17, 2020 4:31 am:
> >> > Note that because lr is clobbered we need at least once normally
> >> > call-clobbered register that's not syscall clobbered to save lr in.
> >> > Otherwise stack frame setup is required to spill it.
> >>
> >> The kernel would like to use r9-r12 for itself. We could do with fewer
> >> registers, but we have some delay establishing the stack (depends on a
> >> load which depends on a mfspr), and entry code tends to be quite store
> >> heavy whereas on the caller side you have r1 set up (modulo stack
> >> updates), and the system call is a long delay during which time the
> >> store queue has significant time to drain.
> >>
> >> My feeling is it would be better for kernel to have these scratch
> >> registers.
> >
> > If your new kernel syscall mechanism requires the caller to make a
> > whole stack frame it otherwise doesn't need and spill registers to it,
> > it becomes a lot less attractive. Some of those 90 cycles saved are
> > immediately lost on the userspace side, plus you either waste icache
> > at the call point or require the syscall to go through a
> > userspace-side helper function that performs the spill and restore.
>
> You would be surprised how few cycles that takes on a high end CPU. Some
> might be a couple of %. I am one for counting cycles mind you, I'm not
> being flippant about it. If we can come up with something faster I'd be
> up for it.
If the cycle count is trivial then just do it on the kernel side.
> > The right way to do this is to have the kernel preserve enough
> > registers that userspace can avoid having any spills. It doesn't have
> > to preserve everything, probably just enough to save lr. (BTW are
>
> Again, the problem is the kernel doesn't have its dependencies
> immediately ready to spill, and spilling (may be) more costly
> immediately after the call because we're doing a lot of stores.
>
> I could try measure this. Unfortunately our pipeline simulator tool
> doesn't model system calls properly so it's hard to see what's happening
> across the user/kernel horizon, I might check if that can be improved
> or I can hack it by putting some isync in there or something.
I think it's unlikely to make any real difference to the total number
of cycles spent which side it happens on, but putting it on the kernel
side makes it easier to avoid wasting size/icache at each syscall
site.
> > syscall arg registers still preserved? If not, this is a major cost on
> > the userspace side, since any call point that has to loop-and-retry
> > (e.g. futex) now needs to make its own place to store the original
> > values.)
>
> Powerpc system calls never did. We could have scv preserve them, but
> you'd still need to restore r3. We could make an ABI which does not
> clobber r3 but puts the return value in r9, say. I'd like to see what
> the user side code looks like to take advantage of such a thing though.
Oh wow, I hadn't realized that, but indeed the code we have now is
allowing for the kernel to clobber them all. So at least this isn't
getting any worse I guess. I think it was a very poor choice of
behavior though and a disadvantage vs what other archs do (some of
them preserve all registers; others preserve only normally call-saved
ones plus the syscall arg ones and possibly a few other specials).
Rich
^ permalink raw reply
* [PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
From: Wang Wenhu @ 2020-04-20 3:05 UTC (permalink / raw)
To: gregkh, arnd, linux-kernel, linuxppc-dev
Cc: robh, Randy Dunlap, Scott Wood, kernel, Wang Wenhu
A generic User-Kernel interface that allows a misc device created
by it to support file-operations of ioctl and mmap to access SRAM
memory from user level. Different kinds of SRAM alloction and free
APIs could be registered by specific SRAM hardware level driver to
the available list and then be chosen by users to allocate and map
SRAM memory from user level.
It is extremely helpful for the user space applications that require
high performance memory accesses, such as embedded networking devices
that would process data in user space, and PowerPC e500 is a case.
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
Changes since v1: addressed comments from Arnd
* Changed the ioctl cmd definitions using _IO micros
* Export interfaces for HW-SRAM drivers to register apis to available list
* Modified allocation alignment to PAGE_SIZE
* Use phys_addr_t as type of SRAM resource size and offset
* Support compat_ioctl
* Misc device name:sram
Note: From this on, the SRAM_UAPI driver is independent to any hardware
drivers, so I would only commit the patch itself as v2, while the v1 of
it was wrapped together with patches for Freescale L2-Cache-SRAM device.
Then after, I'd create patches for Freescale L2-Cache-SRAM device as
another series.
Link for v1:
* https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@vivo.com/
---
drivers/misc/Kconfig | 11 ++
drivers/misc/Makefile | 1 +
drivers/misc/sram_uapi.c | 352 ++++++++++++++++++++++++++++++++++++++
include/linux/sram_uapi.h | 28 +++
4 files changed, 392 insertions(+)
create mode 100644 drivers/misc/sram_uapi.c
create mode 100644 include/linux/sram_uapi.h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 99e151475d8f..b19c8b24f18e 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -465,6 +465,17 @@ config PVPANIC
a paravirtualized device provided by QEMU; it lets a virtual machine
(guest) communicate panic events to the host.
+config SRAM_UAPI
+ bool "Generic SRAM User Level API driver"
+ help
+ This driver allows you to create a misc device which could be used
+ as an interface to allocate SRAM memory from user level.
+
+ It is extremely helpful for some user space applications that require
+ high performance memory accesses.
+
+ If unsure, say N.
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 9abf2923d831..794447ca07ca 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
obj-$(CONFIG_SRAM) += sram.o
obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
+obj-$(CONFIG_SRAM_UAPI) += sram_uapi.o
obj-y += mic/
obj-$(CONFIG_GENWQE) += genwqe/
obj-$(CONFIG_ECHO) += echo/
diff --git a/drivers/misc/sram_uapi.c b/drivers/misc/sram_uapi.c
new file mode 100644
index 000000000000..66c7b56b635f
--- /dev/null
+++ b/drivers/misc/sram_uapi.c
@@ -0,0 +1,352 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
+ * All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/sram_uapi.h>
+
+#define DRIVER_NAME "sram_uapi"
+
+struct res_info {
+ phys_addr_t offset;
+ phys_addr_t size;
+};
+
+struct sram_resource {
+ struct list_head list;
+ struct res_info info;
+ phys_addr_t phys;
+ void *virt;
+ struct vm_area_struct *vma;
+ struct sram_uapi *parent;
+};
+
+struct sram_uapi {
+ struct list_head res_list;
+ struct sram_api *sa;
+};
+
+static DEFINE_MUTEX(sram_api_list_lock);
+static LIST_HEAD(sram_api_list);
+
+long sram_api_register(struct sram_api *sa)
+{
+ struct sram_api *cur;
+
+ if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
+ return -EINVAL;
+
+ mutex_lock(&sram_api_list_lock);
+ list_for_each_entry(cur, &sram_api_list, list) {
+ if (cur->type == sa->type) {
+ pr_err("error sram %s type %d exists\n", sa->name,
+ sa->type);
+ mutex_unlock(&sram_api_list_lock);
+ return -EEXIST;
+ }
+ }
+
+ kref_init(&sa->kref);
+ list_add_tail(&sa->list, &sram_api_list);
+ pr_info("sram %s type %d registered\n", sa->name, sa->type);
+
+ mutex_unlock(&sram_api_list_lock);
+
+ return 0;
+};
+EXPORT_SYMBOL(sram_api_register);
+
+long sram_api_unregister(struct sram_api *sa)
+{
+ struct sram_api *cur, *tmp;
+ long ret = -ENODEV;
+
+ if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
+ return -EINVAL;
+
+ mutex_lock(&sram_api_list_lock);
+ list_for_each_entry_safe(cur, tmp, &sram_api_list, list) {
+ if (cur->type == sa->type && !strcmp(cur->name, sa->name)) {
+ if (kref_read(&cur->kref)) {
+ pr_err("error sram %s type %d is busy\n",
+ sa->name, sa->type);
+ ret = -EBUSY;
+ } else {
+ list_del(&cur->list);
+ pr_info("sram %s type %d unregistered\n",
+ sa->name, sa->type);
+ ret = 0;
+ }
+ break;
+ }
+ }
+ mutex_unlock(&sram_api_list_lock);
+
+ return ret;
+};
+EXPORT_SYMBOL(sram_api_unregister);
+
+static struct sram_api *get_sram_api_from_type(__u32 type)
+{
+ struct sram_api *cur;
+
+ mutex_lock(&sram_api_list_lock);
+ list_for_each_entry(cur, &sram_api_list, list) {
+ if (cur->type == type) {
+ kref_get(&cur->kref);
+ mutex_unlock(&sram_api_list_lock);
+ return cur;
+ }
+ }
+ mutex_unlock(&sram_api_list_lock);
+
+ return NULL;
+}
+
+static void sram_uapi_res_insert(struct sram_uapi *uapi,
+ struct sram_resource *res)
+{
+ struct sram_resource *cur, *tmp;
+ struct list_head *head = &uapi->res_list;
+
+ list_for_each_entry_safe(cur, tmp, head, list) {
+ if (&tmp->list != head &&
+ (cur->info.offset + cur->info.size + res->info.size <=
+ tmp->info.offset)) {
+ res->info.offset = cur->info.offset + cur->info.size;
+ res->parent = uapi;
+ list_add(&res->list, &cur->list);
+ return;
+ }
+ }
+
+ if (list_empty(head))
+ res->info.offset = 0;
+ else {
+ tmp = list_last_entry(head, struct sram_resource, list);
+ res->info.offset = tmp->info.offset + tmp->info.size;
+ }
+ list_add_tail(&res->list, head);
+}
+
+static struct sram_resource *sram_uapi_res_delete(struct sram_uapi *uapi,
+ struct res_info *info)
+{
+ struct sram_resource *res, *tmp;
+
+ list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
+ if (res->info.offset == info->offset) {
+ list_del(&res->list);
+ res->parent = NULL;
+ return res;
+ }
+ }
+
+ return NULL;
+}
+
+static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
+ __u32 offset)
+{
+ struct sram_resource *res;
+
+ list_for_each_entry(res, &uapi->res_list, list) {
+ if (res->info.offset == offset)
+ return res;
+ }
+
+ return NULL;
+}
+
+static int sram_uapi_open(struct inode *inode, struct file *filp)
+{
+ struct sram_uapi *uapi;
+
+ uapi = kzalloc(sizeof(*uapi), GFP_KERNEL);
+ if (!uapi)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&uapi->res_list);
+ filp->private_data = uapi;
+
+ return 0;
+}
+
+static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct sram_uapi *uapi = filp->private_data;
+ struct sram_resource *res;
+ struct res_info info;
+ long ret = -ENOIOCTLCMD;
+ int size;
+ __u32 type;
+
+ if (!uapi)
+ return ret;
+
+ switch (cmd) {
+ case SRAM_UAPI_IOC_SET_SRAM_TYPE:
+ if (uapi->sa)
+ return -EEXIST;
+
+ get_user(type, (const __u32 __user *)arg);
+ uapi->sa = get_sram_api_from_type(type);
+ if (uapi->sa)
+ ret = 0;
+ else
+ ret = -ENODEV;
+
+ break;
+
+ case SRAM_UAPI_IOC_ALLOC:
+ if (!uapi->sa)
+ return -EINVAL;
+
+ res = kzalloc(sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ size = copy_from_user((void *)&res->info,
+ (const void __user *)arg,
+ sizeof(res->info));
+ if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
+ return -EINVAL;
+
+ res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
+ &res->phys,
+ PAGE_SIZE);
+ if (!res->virt) {
+ kfree(res);
+ return -ENOMEM;
+ }
+
+ sram_uapi_res_insert(uapi, res);
+ size = copy_to_user((void __user *)arg,
+ (const void *)&res->info,
+ sizeof(res->info));
+
+ ret = 0;
+ break;
+
+ case SRAM_UAPI_IOC_FREE:
+ if (!uapi->sa)
+ return -EINVAL;
+
+ size = copy_from_user((void *)&info, (const void __user *)arg,
+ sizeof(info));
+
+ res = sram_uapi_res_delete(uapi, &info);
+ if (!res) {
+ pr_err("error no sram resource found\n");
+ return -EINVAL;
+ }
+
+ uapi->sa->sram_free(res->virt);
+ kfree(res);
+
+ ret = 0;
+ break;
+
+ default:
+ pr_err("error no cmd not supported\n");
+ break;
+ }
+
+ return ret;
+}
+
+static int sram_uapi_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ struct sram_uapi *uapi = filp->private_data;
+ struct sram_resource *res;
+
+ res = sram_uapi_find_res(uapi, vma->vm_pgoff);
+ if (!res)
+ return -EINVAL;
+
+ if (vma->vm_end - vma->vm_start > res->info.size)
+ return -EINVAL;
+
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+ return remap_pfn_range(vma, vma->vm_start,
+ res->phys >> PAGE_SHIFT,
+ vma->vm_end - vma->vm_start,
+ vma->vm_page_prot);
+}
+
+static void sram_uapi_res_release(struct sram_uapi *uapi)
+{
+ struct sram_resource *res, *tmp;
+
+ list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
+ list_del(&res->list);
+ uapi->sa->sram_free(res->virt);
+ kfree(res);
+ }
+}
+
+static int sram_uapi_release(struct inode *inodp, struct file *filp)
+{
+ struct sram_uapi *uapi = filp->private_data;
+
+ sram_uapi_res_release(uapi);
+ if (uapi->sa)
+ kref_put(&uapi->sa->kref, NULL);
+
+ kfree(uapi);
+
+ return 0;
+}
+
+static const struct file_operations sram_uapi_ops = {
+ .owner = THIS_MODULE,
+ .open = sram_uapi_open,
+ .unlocked_ioctl = sram_uapi_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+ .mmap = sram_uapi_mmap,
+ .release = sram_uapi_release,
+};
+
+static struct miscdevice sram_uapi_miscdev = {
+ MISC_DYNAMIC_MINOR,
+ "sram",
+ &sram_uapi_ops,
+};
+
+static int __init sram_uapi_init(void)
+{
+ int ret;
+
+ INIT_LIST_HEAD(&sram_api_list);
+ mutex_init(&sram_api_list_lock);
+
+ ret = misc_register(&sram_uapi_miscdev);
+ if (ret)
+ pr_err("failed to register sram uapi misc device\n");
+
+ return ret;
+}
+
+static void __exit sram_uapi_exit(void)
+{
+ misc_deregister(&sram_uapi_miscdev);
+}
+
+module_init(sram_uapi_init);
+module_exit(sram_uapi_exit);
+
+MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
+MODULE_DESCRIPTION("SRAM User API Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/sram_uapi.h b/include/linux/sram_uapi.h
new file mode 100644
index 000000000000..50fbf9dc308f
--- /dev/null
+++ b/include/linux/sram_uapi.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SRAM_UAPI_H
+#define __SRAM_UAPI_H
+
+/* Set SRAM type to be accessed */
+#define SRAM_UAPI_IOC_SET_SRAM_TYPE _IOW('S', 0, __u32)
+
+/* Allocate resource from SRAM */
+#define SRAM_UAPI_IOC_ALLOC _IOWR('S', 1, struct res_info)
+
+/* Free allocated resource of SRAM */
+#define SRAM_UAPI_IOC_FREE _IOW('S', 2, struct res_info)
+
+struct sram_api {
+ struct list_head list;
+ struct kref kref;
+ __u32 type;
+ const char *name;
+
+ long (*sram_alloc)(__u32 size, phys_addr_t *phys, __u32 align);
+ void (*sram_free)(void *ptr);
+};
+
+extern long sram_api_register(struct sram_api *sa);
+
+extern long sram_api_unregister(struct sram_api *sa);
+
+#endif /* __SRAM_UAPI_H */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] iommu: spapr_tce: Disable compile testing to fix build on book3s_32 config
From: Michael Ellerman @ 2020-04-20 3:04 UTC (permalink / raw)
To: Christophe Leroy, Krzysztof Kozlowski, Joerg Roedel, iommu,
linux-kernel
Cc: linuxppc-dev, netdev, Geert Uytterhoeven, virtualization
In-Reply-To: <a99ee461-664c-51ae-cb3a-cf5d87048d86@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> On 04/14/2020 02:26 PM, Krzysztof Kozlowski wrote:
>> Although SPAPR_TCE_IOMMU itself can be compile tested on certain PowerPC
>> configurations, its presence makes arch/powerpc/kvm/Makefile to select
>> modules which do not build in such configuration.
>>
>> The arch/powerpc/kvm/ modules use kvm_arch.spapr_tce_tables which exists
>> only with CONFIG_PPC_BOOK3S_64. However these modules are selected when
>> COMPILE_TEST and SPAPR_TCE_IOMMU are chosen leading to build failures:
>>
>> In file included from arch/powerpc/include/asm/book3s/64/mmu-hash.h:20:0,
>> from arch/powerpc/kvm/book3s_64_vio_hv.c:22:
>> arch/powerpc/include/asm/book3s/64/pgtable.h:17:0: error: "_PAGE_EXEC" redefined [-Werror]
>> #define _PAGE_EXEC 0x00001 /* execute permission */
>>
>> In file included from arch/powerpc/include/asm/book3s/32/pgtable.h:8:0,
>> from arch/powerpc/include/asm/book3s/pgtable.h:8,
>> from arch/powerpc/include/asm/pgtable.h:18,
>> from include/linux/mm.h:95,
>> from arch/powerpc/include/asm/io.h:29,
>> from include/linux/io.h:13,
>> from include/linux/irq.h:20,
>> from arch/powerpc/include/asm/hardirq.h:6,
>> from include/linux/hardirq.h:9,
>> from include/linux/kvm_host.h:7,
>> from arch/powerpc/kvm/book3s_64_vio_hv.c:12:
>> arch/powerpc/include/asm/book3s/32/hash.h:29:0: note: this is the location of the previous definition
>> #define _PAGE_EXEC 0x200 /* software: exec allowed */
>>
>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Fixes: e93a1695d7fb ("iommu: Enable compile testing for some of drivers")
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>> drivers/iommu/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 58b4a4dbfc78..3532b1ead19d 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -362,7 +362,7 @@ config IPMMU_VMSA
>>
>> config SPAPR_TCE_IOMMU
>> bool "sPAPR TCE IOMMU Support"
>> - depends on PPC_POWERNV || PPC_PSERIES || (PPC && COMPILE_TEST)
>> + depends on PPC_POWERNV || PPC_PSERIES
>> select IOMMU_API
>> help
>> Enables bits of IOMMU API required by VFIO. The iommu_ops
>>
>
> Should it be fixed the other way round, something like:
That doesn't actually fix this specific issue, the code will build but
then not link:
ld: arch/powerpc/../../virt/kvm/vfio.o: in function `.kvm_spapr_tce_release_vfio_group':
vfio.c:(.text.kvm_spapr_tce_release_vfio_group+0xb0): undefined reference to `.kvm_spapr_tce_release_iommu_group'
ld: arch/powerpc/../../virt/kvm/vfio.o: in function `.kvm_vfio_set_group':
vfio.c:(.text.kvm_vfio_set_group+0x7f4): undefined reference to `.kvm_spapr_tce_attach_iommu_group'
ld: arch/powerpc/kvm/powerpc.o: in function `.kvm_arch_vm_ioctl':
(.text.kvm_arch_vm_ioctl+0x1a4): undefined reference to `.kvm_vm_ioctl_create_spapr_tce'
ld: (.text.kvm_arch_vm_ioctl+0x230): undefined reference to `.kvm_vm_ioctl_create_spapr_tce'
make[1]: *** [/home/michael/linux/Makefile:1106: vmlinux] Error 1
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index 2bfeaa13befb..906707d15810 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -135,4 +135,4 @@ obj-$(CONFIG_KVM_BOOK3S_32) += kvm.o
> obj-$(CONFIG_KVM_BOOK3S_64_PR) += kvm-pr.o
> obj-$(CONFIG_KVM_BOOK3S_64_HV) += kvm-hv.o
>
> -obj-y += $(kvm-book3s_64-builtin-objs-y)
> +obj-$(CONFIG_KVM_BOOK3S_64) += $(kvm-book3s_64-builtin-objs-y)
But this is probably still a good thing to do, as it would have made the
error messages clearer in this case I think.
cheers
^ permalink raw reply
* Re: [PATCH] papr/scm: Add bad memory ranges to nvdimm bad ranges
From: Philip Li @ 2020-04-20 3:02 UTC (permalink / raw)
To: Santosh Sivaraj
Cc: kbuild-all, kbuild test robot, Aneesh Kumar K.V,
Mahesh Salgaonkar, Oliver, Ganesh Goudar, linuxppc-dev
In-Reply-To: <87a73fiqd5.fsf@santosiv.in.ibm.com>
On Mon, Apr 13, 2020 at 04:50:38PM +0530, Santosh Sivaraj wrote:
> kbuild test robot <lkp@intel.com> writes:
>
> > Hi Santosh,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on powerpc/next]
> > [also build test ERROR on v5.7-rc1 next-20200412]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see
> > https://stackoverflow.com/a/37406982]
>
> This patch depends on "powerpc/mce: Add MCE notification chain" [1].
got it, thanks for input, though currently the bot is not able to figure
out this yet for two separated patch sets, here the --base may help.
>
> [1]: https://lore.kernel.org/linuxppc-dev/20200330071219.12284-1-ganeshgr@linux.ibm.com/
>
> Thanks,
> Santosh
>
> >
> > url: https://github.com/0day-ci/linux/commits/Santosh-Sivaraj/papr-scm-Add-bad-memory-ranges-to-nvdimm-bad-ranges/20200401-171233
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > config: powerpc-allyesconfig (attached as .config)
> > compiler: powerpc64-linux-gcc (GCC) 9.3.0
> > reproduce:
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # save the attached .config to linux build tree
> > GCC_VERSION=9.3.0 make.cross ARCH=powerpc
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kbuild test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> > arch/powerpc/platforms/pseries/papr_scm.c: In function 'papr_scm_init':
> >>> arch/powerpc/platforms/pseries/papr_scm.c:584:3: error: implicit declaration of function 'mce_register_notifier'; did you mean 'bus_register_notifier'? [-Werror=implicit-function-declaration]
> > 584 | mce_register_notifier(&mce_ue_nb);
> > | ^~~~~~~~~~~~~~~~~~~~~
> > | bus_register_notifier
> > arch/powerpc/platforms/pseries/papr_scm.c: In function 'papr_scm_exit':
> >>> arch/powerpc/platforms/pseries/papr_scm.c:592:2: error: implicit declaration of function 'mce_unregister_notifier'; did you mean 'bus_unregister_notifier'? [-Werror=implicit-function-declaration]
> > 592 | mce_unregister_notifier(&mce_ue_nb);
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > | bus_unregister_notifier
> > cc1: some warnings being treated as errors
> >
> > vim +584 arch/powerpc/platforms/pseries/papr_scm.c
> >
> > 577
> > 578 static int __init papr_scm_init(void)
> > 579 {
> > 580 int ret;
> > 581
> > 582 ret = platform_driver_register(&papr_scm_driver);
> > 583 if (!ret)
> > > 584 mce_register_notifier(&mce_ue_nb);
> > 585
> > 586 return ret;
> > 587 }
> > 588 module_init(papr_scm_init);
> > 589
> > 590 static void __exit papr_scm_exit(void)
> > 591 {
> > > 592 mce_unregister_notifier(&mce_ue_nb);
> > 593 platform_driver_unregister(&papr_scm_driver);
> > 594 }
> > 595 module_exit(papr_scm_exit);
> > 596
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/setup_64: Set cache-line-size based on cache-block-size
From: Michael Ellerman @ 2020-04-20 2:53 UTC (permalink / raw)
To: Chris Packham, christophe.leroy@c-s.fr, paulus@samba.org,
benh@kernel.crashing.org, oss@buserror.net, tglx@linutronix.de
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <4d84f89aa682dc78bc0d3a8df2f14b0452465da4.camel@alliedtelesis.co.nz>
Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
> On Thu, 2020-04-16 at 21:43 +1000, Michael Ellerman wrote:
>> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
>> > On Wed, 2020-03-25 at 16:18 +1300, Chris Packham wrote:
>> > > If {i,d}-cache-block-size is set and {i,d}-cache-line-size is
>> > > not,
>> > > use
>> > > the block-size value for both. Per the devicetree spec cache-
>> > > line-
>> > > size
>> > > is only needed if it differs from the block size.
>> > >
>> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> > > ---
>> > > It looks as though the bsizep = lsizep is not required per the
>> > > spec
>> > > but it's
>> > > probably safer to retain it.
>> > >
>> > > Changes in v2:
>> > > - Scott pointed out that u-boot should be filling in the cache
>> > > properties
>> > > (which it does). But it does not specify a cache-line-size
>> > > because
>> > > it
>> > > provides a cache-block-size and the spec says you don't have to
>> > > if
>> > > they are
>> > > the same. So the error is in the parsing not in the devicetree
>> > > itself.
>> > >
>> >
>> > Ping? This thread went kind of quiet.
>>
>> I replied in the other thread:
>>
>>
>> https://lore.kernel.org/linuxppc-dev/87369xx99u.fsf@mpe.ellerman.id.au/
>>
>> But then the merge window happened which is a busy time.
>>
>
> Yeah I figured that was the case.
>
>> What I'd really like is a v3 that incorporates the info I wrote in
>> the
>> other thread and a Fixes tag.
>>
>> If you feel like doing that, that would be great. Otherwise I'll do
>> it
>> tomorrow.
>
> I'll rebase against Linus's tree and have a go a adding some more words
> to the commit message.
Thanks.
cheers
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 2:32 UTC (permalink / raw)
To: Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, Adhemerval Zanella, musl
In-Reply-To: <20200420013412.GZ11469@brightrain.aerifal.cx>
Excerpts from Rich Felker's message of April 20, 2020 11:34 am:
> On Mon, Apr 20, 2020 at 11:10:25AM +1000, Nicholas Piggin wrote:
>> Excerpts from Rich Felker's message of April 17, 2020 4:31 am:
>> > Note that because lr is clobbered we need at least once normally
>> > call-clobbered register that's not syscall clobbered to save lr in.
>> > Otherwise stack frame setup is required to spill it.
>>
>> The kernel would like to use r9-r12 for itself. We could do with fewer
>> registers, but we have some delay establishing the stack (depends on a
>> load which depends on a mfspr), and entry code tends to be quite store
>> heavy whereas on the caller side you have r1 set up (modulo stack
>> updates), and the system call is a long delay during which time the
>> store queue has significant time to drain.
>>
>> My feeling is it would be better for kernel to have these scratch
>> registers.
>
> If your new kernel syscall mechanism requires the caller to make a
> whole stack frame it otherwise doesn't need and spill registers to it,
> it becomes a lot less attractive. Some of those 90 cycles saved are
> immediately lost on the userspace side, plus you either waste icache
> at the call point or require the syscall to go through a
> userspace-side helper function that performs the spill and restore.
You would be surprised how few cycles that takes on a high end CPU. Some
might be a couple of %. I am one for counting cycles mind you, I'm not
being flippant about it. If we can come up with something faster I'd be
up for it.
>
> The right way to do this is to have the kernel preserve enough
> registers that userspace can avoid having any spills. It doesn't have
> to preserve everything, probably just enough to save lr. (BTW are
Again, the problem is the kernel doesn't have its dependencies
immediately ready to spill, and spilling (may be) more costly
immediately after the call because we're doing a lot of stores.
I could try measure this. Unfortunately our pipeline simulator tool
doesn't model system calls properly so it's hard to see what's happening
across the user/kernel horizon, I might check if that can be improved
or I can hack it by putting some isync in there or something.
> syscall arg registers still preserved? If not, this is a major cost on
> the userspace side, since any call point that has to loop-and-retry
> (e.g. futex) now needs to make its own place to store the original
> values.)
Powerpc system calls never did. We could have scv preserve them, but
you'd still need to restore r3. We could make an ABI which does not
clobber r3 but puts the return value in r9, say. I'd like to see what
the user side code looks like to take advantage of such a thing though.
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 2:08 UTC (permalink / raw)
To: Rich Felker
Cc: libc-dev, Szabolcs Nagy, Nicholas Piggin via Libc-alpha,
linuxppc-dev, musl
In-Reply-To: <20200420012904.GY11469@brightrain.aerifal.cx>
Excerpts from Rich Felker's message of April 20, 2020 11:29 am:
> On Mon, Apr 20, 2020 at 10:27:58AM +1000, Nicholas Piggin wrote:
>> Excerpts from Szabolcs Nagy's message of April 16, 2020 7:58 pm:
>> > * Nicholas Piggin via Libc-alpha <libc-alpha@sourceware.org> [2020-04-16 10:16:54 +1000]:
>> >> Well it would have to test HWCAP and patch in or branch to two
>> >> completely different sequences including register save/restores yes.
>> >> You could have the same asm and matching clobbers to put the sequence
>> >> inline and then you could patch the one sc/scv instruction I suppose.
>> >
>> > how would that 'patch' work?
>> >
>> > there are many reasons why you don't
>> > want libc to write its .text
>>
>> I guess I don't know what I'm talking about when it comes to libraries.
>> Shame if there is no good way to load-time patch libc. It's orthogonal
>> to the scv selection though -- if you don't patch you have to
>> conditional or indirect branch however you implement it.
>
> Patched pages cannot be shared. The whole design of PIC and shared
> libraries is that the code("text")/rodata is immutable and shared and
> that only a minimal amount of data, packed tightly together (the GOT)
> has to exist per-instance.
Yeah the pages which were patched couldn't be shared across exec, which
is a significant downside, unless you could group all patch sites into
their own section and similarly pack it together (which has issues of
being out of line).
>
> Also, allowing patching of executable pages is generally frowned upon
> these days because W^X is a desirable hardening property.
Right, it would want be write-protected after being patched.
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-20 1:34 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-dev, libc-alpha, linuxppc-dev, Adhemerval Zanella, musl
In-Reply-To: <1587344003.daumxvs1kh.astroid@bobo.none>
On Mon, Apr 20, 2020 at 11:10:25AM +1000, Nicholas Piggin wrote:
> Excerpts from Rich Felker's message of April 17, 2020 4:31 am:
> > Note that because lr is clobbered we need at least once normally
> > call-clobbered register that's not syscall clobbered to save lr in.
> > Otherwise stack frame setup is required to spill it.
>
> The kernel would like to use r9-r12 for itself. We could do with fewer
> registers, but we have some delay establishing the stack (depends on a
> load which depends on a mfspr), and entry code tends to be quite store
> heavy whereas on the caller side you have r1 set up (modulo stack
> updates), and the system call is a long delay during which time the
> store queue has significant time to drain.
>
> My feeling is it would be better for kernel to have these scratch
> registers.
If your new kernel syscall mechanism requires the caller to make a
whole stack frame it otherwise doesn't need and spill registers to it,
it becomes a lot less attractive. Some of those 90 cycles saved are
immediately lost on the userspace side, plus you either waste icache
at the call point or require the syscall to go through a
userspace-side helper function that performs the spill and restore.
The right way to do this is to have the kernel preserve enough
registers that userspace can avoid having any spills. It doesn't have
to preserve everything, probably just enough to save lr. (BTW are
syscall arg registers still preserved? If not, this is a major cost on
the userspace side, since any call point that has to loop-and-retry
(e.g. futex) now needs to make its own place to store the original
values.)
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-20 1:29 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-dev, Szabolcs Nagy, Nicholas Piggin via Libc-alpha,
linuxppc-dev, musl
In-Reply-To: <1587341904.1r83vbudyf.astroid@bobo.none>
On Mon, Apr 20, 2020 at 10:27:58AM +1000, Nicholas Piggin wrote:
> Excerpts from Szabolcs Nagy's message of April 16, 2020 7:58 pm:
> > * Nicholas Piggin via Libc-alpha <libc-alpha@sourceware.org> [2020-04-16 10:16:54 +1000]:
> >> Well it would have to test HWCAP and patch in or branch to two
> >> completely different sequences including register save/restores yes.
> >> You could have the same asm and matching clobbers to put the sequence
> >> inline and then you could patch the one sc/scv instruction I suppose.
> >
> > how would that 'patch' work?
> >
> > there are many reasons why you don't
> > want libc to write its .text
>
> I guess I don't know what I'm talking about when it comes to libraries.
> Shame if there is no good way to load-time patch libc. It's orthogonal
> to the scv selection though -- if you don't patch you have to
> conditional or indirect branch however you implement it.
Patched pages cannot be shared. The whole design of PIC and shared
libraries is that the code("text")/rodata is immutable and shared and
that only a minimal amount of data, packed tightly together (the GOT)
has to exist per-instance.
Also, allowing patching of executable pages is generally frowned upon
these days because W^X is a desirable hardening property.
Rich
^ permalink raw reply
* Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
From: Nicholas Piggin @ 2020-04-20 1:12 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
In-Reply-To: <1587341611.capj46kr99.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of April 20, 2020 10:17 am:
> Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm:
>> As per the ISA, context synchronizing instructions is needed before and after
>> SPRN_AMR update. Use isync before and the CSI after is implied by the rfid
>> that we will use to switch to a new context.
>
> Not entirely sure if we need this. This will restore AMR to more
> permissive, so if it executes ahead of a stray load from this
> context, it won't make it fault.
>
> That said, leaving this end open makes it harder to reason about
> user access protection I guess, so let's add it.
We probably should test whether it needs updating, like the entry
code does.
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 1:10 UTC (permalink / raw)
To: Adhemerval Zanella, Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, musl
In-Reply-To: <20200416183151.GA11469@brightrain.aerifal.cx>
Excerpts from Rich Felker's message of April 17, 2020 4:31 am:
> On Thu, Apr 16, 2020 at 03:18:42PM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 16/04/2020 14:59, Rich Felker wrote:
>> > On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
>> >>
>> >>
>> >> On 16/04/2020 12:37, Rich Felker wrote:
>> >>> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
>> >>>>> My preference would be that it work just like the i386 AT_SYSINFO
>> >>>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
>> >>>>> provides a stub in the vdso that performs either scv or the old
>> >>>>> mechanism with the same calling convention. Then if the kernel doesn't
>> >>>>> provide it (because the kernel is too old) libc would have to provide
>> >>>>> its own stub that uses the legacy method and matches the calling
>> >>>>> convention of the one the kernel is expected to provide.
>> >>>>
>> >>>> What about pthread cancellation and the requirement of checking the
>> >>>> cancellable syscall anchors in asynchronous cancellation? My plan is
>> >>>> still to use musl strategy on glibc (BZ#12683) and for i686 it
>> >>>> requires to always use old int$128 for program that uses cancellation
>> >>>> (static case) or just threads (dynamic mode, which should be more
>> >>>> common on glibc).
>> >>>>
>> >>>> Using the i686 strategy of a vDSO bridge symbol would require to always
>> >>>> fallback to 'sc' to still use the same cancellation strategy (and
>> >>>> thus defeating this optimization in such cases).
>> >>>
>> >>> Yes, I assumed it would be the same, ignoring the new syscall
>> >>> mechanism for cancellable syscalls. While there are some exceptions,
>> >>> cancellable syscalls are generally not hot paths but things that are
>> >>> expected to block and to have significant amounts of work to do in
>> >>> kernelspace, so saving a few tens of cycles is rather pointless.
>> >>>
>> >>> It's possible to do a branch/multiple versions of the syscall asm for
>> >>> cancellation but would require extending the cancellation handler to
>> >>> support checking against multiple independent address ranges or using
>> >>> some alternate markup of them.
>> >>
>> >> The main issue is at least for glibc dynamic linking is way more common
>> >> than static linking and once the program become multithread the fallback
>> >> will be always used.
>> >
>> > I'm not relying on static linking optimizing out the cancellable
>> > version. I'm talking about how cancellable syscalls are pretty much
>> > all "heavy" operations to begin with where a few tens of cycles are in
>> > the realm of "measurement noise" relative to the dominating time
>> > costs.
>>
>> Yes I am aware, but at same time I am not sure how it plays on real world.
>> For instance, some workloads might issue kernel query syscalls, such as
>> recv, where buffer copying might not be dominant factor. So I see that if
>> the idea is optimizing syscall mechanism, we should try to leverage it
>> as whole in libc.
>
> Have you timed a minimal recv? I'm not assuming buffer copying is the
> dominant factor. I'm assuming the overhead of all the kernel layers
> involved is dominant.
>
>> >> And besides the cancellation performance issue, a new bridge vDSO mechanism
>> >> will still require to setup some extra bridge for the case of the older
>> >> kernel. In the scheme you suggested:
>> >>
>> >> __asm__("indirect call" ... with common clobbers);
>> >>
>> >> The indirect call will be either the vDSO bridge or an libc provided that
>> >> fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
>> >> against:
>> >>
>> >> if (hwcap & PPC_FEATURE2_SCV) {
>> >> __asm__(... with some clobbers);
>> >> } else {
>> >> __asm__(... with different clobbers);
>> >> }
>> >
>> > If the indirect call can be made roughly as efficiently as the sc
>> > sequence now (which already have some cost due to handling the nasty
>> > error return convention, making the indirect call likely just as small
>> > or smaller), it's O(1) additional code size (and thus icache usage)
>> > rather than O(n) where n is number of syscall points.
>> >
>> > Of course it would work just as well (for avoiding O(n) growth) to
>> > have a direct call to out-of-line branch like you suggested.
>>
>> Yes, but does it really matter to optimize this specific usage case
>> for size? glibc, for instance, tries to leverage the syscall mechanism
>> by adding some complex pre-processor asm directives. It optimizes
>> the syscall code size in most cases. For instance, kill in static case
>> generates on x86_64:
>>
>> 0000000000000000 <__kill>:
>> 0: b8 3e 00 00 00 mov $0x3e,%eax
>> 5: 0f 05 syscall
>> 7: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
>> d: 0f 83 00 00 00 00 jae 13 <__kill+0x13>
>> 13: c3 retq
>>
>> While on musl:
>>
>> 0000000000000000 <kill>:
>> 0: 48 83 ec 08 sub $0x8,%rsp
>> 4: 48 63 ff movslq %edi,%rdi
>> 7: 48 63 f6 movslq %esi,%rsi
>> a: b8 3e 00 00 00 mov $0x3e,%eax
>> f: 0f 05 syscall
>> 11: 48 89 c7 mov %rax,%rdi
>> 14: e8 00 00 00 00 callq 19 <kill+0x19>
>> 19: 5a pop %rdx
>> 1a: c3 retq
>
> Wow that's some extraordinarily bad codegen going on by gcc... The
> sign-extension is semantically needed and I don't see a good way
> around it (glibc's asm is kinda a hack taking advantage of kernel not
> looking at high bits, I think), but the gratuitous stack adjustment
> and refusal to generate a tail call isn't. I'll see if we can track
> down what's going on and get it fixed.
>
>> But I hardly think it pays off the required code complexity. Some
>> for providing a O(1) bridge: this will require additional complexity
>> to write it and setup correctly.
>
> In some sense I agree, but inline instructions are a lot more
> expensive on ppc (being 32-bit each), and it might take out-of-lining
> anyway to get rid of stack frame setups if that ends up being a
> problem.
>
>> >> Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
>> >> TCB member (as we do on glibc) and if we could make the asm clever
>> >> enough to not require different clobbers (although not sure if
>> >> it would be possible).
>> >
>> > The easy way not to require different clobbers is just using the union
>> > of the clobbers, no? Does the proposed new method clobber any
>> > call-saved registers that would make it painful (requiring new call
>> > frames to save them in)?
>>
>> As far I can tell, it should be ok.
>
> Note that because lr is clobbered we need at least once normally
> call-clobbered register that's not syscall clobbered to save lr in.
> Otherwise stack frame setup is required to spill it.
The kernel would like to use r9-r12 for itself. We could do with fewer
registers, but we have some delay establishing the stack (depends on a
load which depends on a mfspr), and entry code tends to be quite store
heavy whereas on the caller side you have r1 set up (modulo stack
updates), and the system call is a long delay during which time the
store queue has significant time to drain.
My feeling is it would be better for kernel to have these scratch
registers.
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 0:46 UTC (permalink / raw)
To: Adhemerval Zanella, Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, musl
In-Reply-To: <65f70b10-bfc1-e9f6-d48a-4b063ad6b669@linaro.org>
Excerpts from Adhemerval Zanella's message of April 17, 2020 4:52 am:
>
>
> On 16/04/2020 15:31, Rich Felker wrote:
>> On Thu, Apr 16, 2020 at 03:18:42PM -0300, Adhemerval Zanella wrote:
>>>
>>>
>>> On 16/04/2020 14:59, Rich Felker wrote:
>>>> On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
>>>>>
>>>>>
>>>>> On 16/04/2020 12:37, Rich Felker wrote:
>>>>>> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
>>>>>>>> My preference would be that it work just like the i386 AT_SYSINFO
>>>>>>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
>>>>>>>> provides a stub in the vdso that performs either scv or the old
>>>>>>>> mechanism with the same calling convention. Then if the kernel doesn't
>>>>>>>> provide it (because the kernel is too old) libc would have to provide
>>>>>>>> its own stub that uses the legacy method and matches the calling
>>>>>>>> convention of the one the kernel is expected to provide.
>>>>>>>
>>>>>>> What about pthread cancellation and the requirement of checking the
>>>>>>> cancellable syscall anchors in asynchronous cancellation? My plan is
>>>>>>> still to use musl strategy on glibc (BZ#12683) and for i686 it
>>>>>>> requires to always use old int$128 for program that uses cancellation
>>>>>>> (static case) or just threads (dynamic mode, which should be more
>>>>>>> common on glibc).
>>>>>>>
>>>>>>> Using the i686 strategy of a vDSO bridge symbol would require to always
>>>>>>> fallback to 'sc' to still use the same cancellation strategy (and
>>>>>>> thus defeating this optimization in such cases).
>>>>>>
>>>>>> Yes, I assumed it would be the same, ignoring the new syscall
>>>>>> mechanism for cancellable syscalls. While there are some exceptions,
>>>>>> cancellable syscalls are generally not hot paths but things that are
>>>>>> expected to block and to have significant amounts of work to do in
>>>>>> kernelspace, so saving a few tens of cycles is rather pointless.
>>>>>>
>>>>>> It's possible to do a branch/multiple versions of the syscall asm for
>>>>>> cancellation but would require extending the cancellation handler to
>>>>>> support checking against multiple independent address ranges or using
>>>>>> some alternate markup of them.
>>>>>
>>>>> The main issue is at least for glibc dynamic linking is way more common
>>>>> than static linking and once the program become multithread the fallback
>>>>> will be always used.
>>>>
>>>> I'm not relying on static linking optimizing out the cancellable
>>>> version. I'm talking about how cancellable syscalls are pretty much
>>>> all "heavy" operations to begin with where a few tens of cycles are in
>>>> the realm of "measurement noise" relative to the dominating time
>>>> costs.
>>>
>>> Yes I am aware, but at same time I am not sure how it plays on real world.
>>> For instance, some workloads might issue kernel query syscalls, such as
>>> recv, where buffer copying might not be dominant factor. So I see that if
>>> the idea is optimizing syscall mechanism, we should try to leverage it
>>> as whole in libc.
>>
>> Have you timed a minimal recv? I'm not assuming buffer copying is the
>> dominant factor. I'm assuming the overhead of all the kernel layers
>> involved is dominant.
>
> Not really, but reading the advantages of using 'scv' over 'sc' also does
> not outline the real expect gain. Taking in consideration this should
> be a micro-optimization (focused on entry syscall patch), I think we should
> use where it possible.
It's around 90 cycles improvement, depending on config options and
speculative mitigations in place, this may be roughly 5-20% of a gettid
syscall, which itself probably bears little relationship to what a recv
syscall doing real work would do, it's easy to swamp it with other work.
But it's a pretty big win in terms of how much we try to optimise this
path.
Thanks,
Nick
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Nicholas Piggin @ 2020-04-20 0:27 UTC (permalink / raw)
To: Nicholas Piggin via Libc-alpha, Szabolcs Nagy
Cc: libc-dev, Rich Felker, linuxppc-dev, musl
In-Reply-To: <20200416095800.GC23945@port70.net>
Excerpts from Szabolcs Nagy's message of April 16, 2020 7:58 pm:
> * Nicholas Piggin via Libc-alpha <libc-alpha@sourceware.org> [2020-04-16 10:16:54 +1000]:
>> Well it would have to test HWCAP and patch in or branch to two
>> completely different sequences including register save/restores yes.
>> You could have the same asm and matching clobbers to put the sequence
>> inline and then you could patch the one sc/scv instruction I suppose.
>
> how would that 'patch' work?
>
> there are many reasons why you don't
> want libc to write its .text
I guess I don't know what I'm talking about when it comes to libraries.
Shame if there is no good way to load-time patch libc. It's orthogonal
to the scv selection though -- if you don't patch you have to
conditional or indirect branch however you implement it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
From: Nicholas Piggin @ 2020-04-20 0:17 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
In-Reply-To: <20200419135359.731325-1-aneesh.kumar@linux.ibm.com>
Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm:
> As per the ISA, context synchronizing instructions is needed before and after
> SPRN_AMR update. Use isync before and the CSI after is implied by the rfid
> that we will use to switch to a new context.
Not entirely sure if we need this. This will restore AMR to more
permissive, so if it executes ahead of a stray load from this
context, it won't make it fault.
That said, leaving this end open makes it harder to reason about
user access protection I guess, so let's add it.
Thanks,
Nick
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/kup-radix.h | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> index 3bcef989a35d..224658efe2fd 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -16,6 +16,7 @@
> #ifdef CONFIG_PPC_KUAP
> BEGIN_MMU_FTR_SECTION_NESTED(67)
> ld \gpr, STACK_REGS_KUAP(r1)
> + isync
> mtspr SPRN_AMR, \gpr
> END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
> #endif
> @@ -62,8 +63,14 @@
>
> static inline void kuap_restore_amr(struct pt_regs *regs)
> {
> - if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
> + isync();
> mtspr(SPRN_AMR, regs->kuap);
> + /*
> + * No following isync/CSI required because we will be
> + * returning to a different context using rfid
> + */
> + }
> }
>
> static inline void kuap_check_amr(void)
> --
> 2.25.2
>
>
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.5 73/75] ocxl: Add PCI hotplug dependency to Kconfig
From: Sasha Levin @ 2020-04-19 23:40 UTC (permalink / raw)
To: Andrew Donnellan
Cc: linux-kernel, stable, Alastair D'Silva, Frederic Barrat,
linuxppc-dev
In-Reply-To: <c2bceeb6-07bb-1cc4-0d67-48b9fe0f6ba9@linux.ibm.com>
On Mon, Apr 20, 2020 at 02:32:19AM +1000, Andrew Donnellan wrote:
>On 19/4/20 12:09 am, Sasha Levin wrote:
>>From: Frederic Barrat <fbarrat@linux.ibm.com>
>>
>>[ Upstream commit 49ce94b8677c7d7a15c4d7cbbb9ff1cd8387827b ]
>>
>>The PCI hotplug framework is used to update the devices when a new
>>image is written to the FPGA.
>>
>>Reviewed-by: Alastair D'Silva <alastair@d-silva.org>
>>Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
>>Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>Link: https://lore.kernel.org/r/20191121134918.7155-12-fbarrat@linux.ibm.com
>>Signed-off-by: Sasha Levin <sashal@kernel.org>
>
>This shouldn't be backported to any of the stable trees.
I'll drop it, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* [Bug 207359] MegaRAID SAS 9361 controller hang/reset
From: bugzilla-daemon @ 2020-04-19 20:55 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-207359-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=207359
--- Comment #2 from Cameron (cam@neo-zeon.de) ---
Looking at bug 206123 above, it's worth noting that the amd64 box I'm using for
comparison has SATA disks, though this is probably still a PPC specific issue.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 207359] MegaRAID SAS 9361 controller hang/reset
From: bugzilla-daemon @ 2020-04-19 20:24 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-207359-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=207359
gyakovlev@gentoo.org changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |gyakovlev@gentoo.org
--- Comment #1 from gyakovlev@gentoo.org ---
In my case I see similar problem on same motherboard but with aacraid driver
(microsemi one)
https://bugzilla.kernel.org/show_bug.cgi?id=206123
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ 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