* Re: [PATCH v6 1/7] kvmppc: Driver to manage pages of secure guest
From: Christoph Hellwig @ 2019-08-10 10:58 UTC (permalink / raw)
To: Bharata B Rao
Cc: linuxram, cclaudio, kvm-ppc, linux-mm, jglisse, aneesh.kumar,
paulus, sukadev, linuxppc-dev, hch
In-Reply-To: <20190809084108.30343-2-bharata@linux.ibm.com>
> +#ifdef CONFIG_PPC_UV
> +extern unsigned long kvmppc_h_svm_page_in(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
> +extern unsigned long kvmppc_h_svm_page_out(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
No need for externs on function declarations.
> +struct kvmppc_devm_device {
> + struct device dev;
> + dev_t devt;
> + struct dev_pagemap pagemap;
> + unsigned long pfn_first, pfn_last;
> + unsigned long *pfn_bitmap;
> +};
We shouldn't really need this conaining structucture given that there
is only a single global instance of it anyway.
> +struct kvmppc_devm_copy_args {
> + unsigned long *rmap;
> + unsigned int lpid;
> + unsigned long gpa;
> + unsigned long page_shift;
> +};
Do we really need this args structure? It is just used in a single
function call where passing the arguments might be cleaner.
> +static void kvmppc_devm_put_page(struct page *page)
> +{
> + unsigned long pfn = page_to_pfn(page);
> + unsigned long flags;
> + struct kvmppc_devm_page_pvt *pvt;
> +
> + spin_lock_irqsave(&kvmppc_devm_lock, flags);
> + pvt = (struct kvmppc_devm_page_pvt *)page->zone_device_data;
No need for the cast.
> + page->zone_device_data = 0;
This should be NULL.
> +
> + bitmap_clear(kvmppc_devm.pfn_bitmap,
> + pfn - kvmppc_devm.pfn_first, 1);
> + *(pvt->rmap) = 0;
No need for the braces.
> + dpage = alloc_page_vma(GFP_HIGHUSER, mig->vma, mig->start);
> + if (!dpage)
> + return -EINVAL;
> + lock_page(dpage);
> + pvt = (struct kvmppc_devm_page_pvt *)spage->zone_device_data;
No need for the cast here.
> +static void kvmppc_devm_page_free(struct page *page)
> +{
> + kvmppc_devm_put_page(page);
> +}
This seems to be the only caller of kvmppc_devm_put_page, any reason
not to just merge the two functions?
> +static int kvmppc_devm_pages_init(void)
> +{
> + unsigned long nr_pfns = kvmppc_devm.pfn_last -
> + kvmppc_devm.pfn_first;
> +
> + kvmppc_devm.pfn_bitmap = kcalloc(BITS_TO_LONGS(nr_pfns),
> + sizeof(unsigned long), GFP_KERNEL);
> + if (!kvmppc_devm.pfn_bitmap)
> + return -ENOMEM;
> +
> + spin_lock_init(&kvmppc_devm_lock);
Just initialize the spinlock using DEFINE_SPINLOCK() at compile time.
The rest of the function is so trivial that it can be inlined into the
caller.
Also is kvmppc_devm_lock such a good name? This mostly just protects
the allocation bitmap, so reflecting that in the name might be a good
idea.
> +int kvmppc_devm_init(void)
> +{
> + int ret = 0;
> + unsigned long size;
> + struct resource *res;
> + void *addr;
> +
> + size = kvmppc_get_secmem_size();
> + if (!size) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + ret = alloc_chrdev_region(&kvmppc_devm.devt, 0, 1,
> + "kvmppc-devm");
> + if (ret)
> + goto out;
> +
> + dev_set_name(&kvmppc_devm.dev, "kvmppc_devm_device%d", 0);
> + kvmppc_devm.dev.release = kvmppc_devm_release;
> + device_initialize(&kvmppc_devm.dev);
> + res = devm_request_free_mem_region(&kvmppc_devm.dev,
> + &iomem_resource, size);
> + if (IS_ERR(res)) {
> + ret = PTR_ERR(res);
> + goto out_unregister;
> + }
> +
> + kvmppc_devm.pagemap.type = MEMORY_DEVICE_PRIVATE;
> + kvmppc_devm.pagemap.res = *res;
> + kvmppc_devm.pagemap.ops = &kvmppc_devm_ops;
> + addr = devm_memremap_pages(&kvmppc_devm.dev, &kvmppc_devm.pagemap);
> + if (IS_ERR(addr)) {
> + ret = PTR_ERR(addr);
> + goto out_unregister;
> + }
It seems a little silly to allocate a struct device just so that we can
pass it to devm_request_free_mem_region and devm_memremap_pages. I think
we should just create non-dev_ versions of those as well.
> +
> + kvmppc_devm.pfn_first = res->start >> PAGE_SHIFT;
> + kvmppc_devm.pfn_last = kvmppc_devm.pfn_first +
> + (resource_size(res) >> PAGE_SHIFT);
pfn_last is only used to calculat a size. Also I think we could
just use kvmppc_devm.pagemap.res directly instead of copying these
values out. the ">> PAGE_SHIFT" is cheap enough.
^ permalink raw reply
* Re: [PATCH] powerpc/rtas: unexport rtas_online_cpus_mask, rtas_offline_cpus_mask
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Nathan Lynch, linuxppc-dev
In-Reply-To: <20190718162214.5694-1-nathanl@linux.ibm.com>
On Thu, 2019-07-18 at 16:22:14 UTC, Nathan Lynch wrote:
> These aren't used by modular code, nor should they be.
>
> Fixes: 120496ac2d2d ("powerpc: Bring all threads online prior to migration/hibernation")
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/ae2e953fdca791270e80c08d6a830d9aa472a111
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc/powernv: Restrict OPAL symbol map to only be readable by root
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev; +Cc: Jordan Niethe, Stewart Smith, stable
In-Reply-To: <20190503075253.22798-1-ajd@linux.ibm.com>
On Fri, 2019-05-03 at 07:52:53 UTC, Andrew Donnellan wrote:
> Currently the OPAL symbol map is globally readable, which seems bad as it
> contains physical addresses.
>
> Restrict it to root.
>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Jordan Niethe <jniethe5@gmail.com>
> Cc: Stewart Smith <stewart@linux.ibm.com>
> Fixes: c8742f85125d ("powerpc/powernv: Expose OPAL firmware symbol map")
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/e7de4f7b64c23e503a8c42af98d56f2a7462bd6d
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/64s: Make boot look nice(r)
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20190516020437.11783-1-npiggin@gmail.com>
On Thu, 2019-05-16 at 02:04:37 UTC, Nicholas Piggin wrote:
> Radix boot looks like this:
>
> -----------------------------------------------------
> phys_mem_size = 0x200000000
> dcache_bsize = 0x80
> icache_bsize = 0x80
> cpu_features = 0x0000c06f8f5fb1a7
> possible = 0x0000fbffcf5fb1a7
> always = 0x00000003800081a1
> cpu_user_features = 0xdc0065c2 0xaee00000
> mmu_features = 0xbc006041
> firmware_features = 0x0000000010000000
> hash-mmu: ppc64_pft_size = 0x0
> hash-mmu: kernel vmalloc start = 0xc008000000000000
> hash-mmu: kernel IO start = 0xc00a000000000000
> hash-mmu: kernel vmemmap start = 0xc00c000000000000
> -----------------------------------------------------
>
> Fix:
>
> -----------------------------------------------------
> phys_mem_size = 0x200000000
> dcache_bsize = 0x80
> icache_bsize = 0x80
> cpu_features = 0x0000c06f8f5fb1a7
> possible = 0x0000fbffcf5fb1a7
> always = 0x00000003800081a1
> cpu_user_features = 0xdc0065c2 0xaee00000
> mmu_features = 0xbc006041
> firmware_features = 0x0000000010000000
> vmalloc start = 0xc008000000000000
> IO start = 0xc00a000000000000
> vmemmap start = 0xc00c000000000000
> -----------------------------------------------------
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/a26c29a00f20015a30e618caf7c0720a351e1d03
cheers
^ permalink raw reply
* Re: [PATCH v2 1/5] powerpc/powernv: Move SCOM access code into powernv platform
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev
In-Reply-To: <20190509051119.7694-1-ajd@linux.ibm.com>
On Thu, 2019-05-09 at 05:11:15 UTC, Andrew Donnellan wrote:
> The powernv platform is the only one that directly accesses SCOMs. Move the
> support code to platforms/powernv, and get rid of the PPC_SCOM Kconfig
> option, as SCOM support is always selected when compiling for powernv.
>
> This also means that the Kconfig item for CONFIG_SCOM_DEBUGFS will actually
> show up in menuconfig, as previously it was the only labelled option in
> sysdev/Kconfig and wasn't actually in a menu.
>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/08a456aa643776757e07adfdebe7f7681117d144
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc/32: activate ARCH_HAS_PMEM_API and ARCH_HAS_UACCESS_FLUSHCACHE
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <a682a2f9db308c5cfe77e45aa3352e41bc9f4e33.1564554634.git.christophe.leroy@c-s.fr>
On Wed, 2019-07-31 at 06:31:41 UTC, Christophe Leroy wrote:
> PPC32 also have flush_dcache_range() so it can also support
> ARCH_HAS_PMEM_API and ARCH_HAS_UACCESS_FLUSHCACHE without changes.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/461cef2a676e7c578d0ef62969dbcb8237c8631d
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/xive: Update comment referencing magic loads from an ESB
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: Jordan Niethe
In-Reply-To: <20190802000835.26191-1-jniethe5@gmail.com>
On Fri, 2019-08-02 at 00:08:35 UTC, Jordan Niethe wrote:
> The comment above xive_esb_read() references magic loads from an ESB as
> described xive.h. This has been inaccurate since commit 12c1f339cd49
> ("powerpc/xive: Move definition of ESB bits") which moved the
> description. Update the comment to reference the new location of the
> description in xive-regs.h
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> Acked-by: Stewart Smith <stewart@linux.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/1ebe0dcce1750109181d666394b7dfd9af9ff645
cheers
^ permalink raw reply
* Re: [PATCH 1/1] pseries/hotplug-memory.c: Replace nested ifs by switch-case
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Leonardo Bras, linuxppc-dev, linux-kernel
Cc: Nathan Lynch, Pavel Tatashin, David Hildenbrand,
Mahesh Salgaonkar, YueHaibing, Paul Mackerras, Greg Kroah-Hartman,
Nathan Fontenot, Andrew Morton, Rob Herring, Leonardo Bras
In-Reply-To: <20190801225251.17864-1-leonardo@linux.ibm.com>
On Thu, 2019-08-01 at 22:52:51 UTC, Leonardo Bras wrote:
> I noticed these nested ifs can be easily replaced by switch-cases,
> which can improve readability.
>
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/9616dda1aaddb2080122aaeab96ad7fc064e36b4
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/xive: Use GFP_KERNEL instead of GFP_ATOMIC in 'xive_irq_bitmap_add()'
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Christophe JAILLET, benh, paulus, allison, tglx, clg, groug
Cc: Christophe JAILLET, kernel-janitors, linuxppc-dev, linux-kernel
In-Reply-To: <85d5d247ce753befd6aa63c473f7823de6520ccd.1564647619.git.christophe.jaillet@wanadoo.fr>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 502 bytes --]
On Thu, 2019-08-01 at 08:32:31 UTC, Christophe JAILLET wrote:
> There is no need to use GFP_ATOMIC here. GFP_KERNEL should be enough.
> GFP_KERNEL is also already used for another allocation just a few lines
> below.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/2705ec100b46390851542fa97e920cc21ffaac4f
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Print helpful message when cores guarded
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Joel Stanley, linuxppc-dev
In-Reply-To: <20190801051630.5804-1-joel@jms.id.au>
On Thu, 2019-08-01 at 05:16:30 UTC, Joel Stanley wrote:
> Often the firmware will guard out cores after a crash. This often
> undesirable, and is not immediately noticeable.
>
> This adds an informative message when a CPU device tree nodes are marked
> bad in the device tree.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/680d00741838b1c1bd59d5d7ac96791f5558e05d
cheers
^ permalink raw reply
* Re: [PATCH v2] PCI: rpaphp: Avoid a sometimes-uninitialized warning
From: Michael Ellerman @ 2019-08-10 10:20 UTC (permalink / raw)
To: Nathan Chancellor, Tyrel Datwyler, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linux-pci, Nick Desaulniers, linux-kernel, clang-built-linux,
Bjorn Helgaas, Nathan Chancellor, linuxppc-dev
In-Reply-To: <20190603221157.58502-1-natechancellor@gmail.com>
On Mon, 2019-06-03 at 22:11:58 UTC, Nathan Chancellor wrote:
> When building with -Wsometimes-uninitialized, clang warns:
>
> drivers/pci/hotplug/rpaphp_core.c:243:14: warning: variable 'fndit' is
> used uninitialized whenever 'for' loop exits because its condition is
> false [-Wsometimes-uninitialized]
> for (j = 0; j < entries; j++) {
> ^~~~~~~~~~~
> drivers/pci/hotplug/rpaphp_core.c:256:6: note: uninitialized use occurs
> here
> if (fndit)
> ^~~~~
> drivers/pci/hotplug/rpaphp_core.c:243:14: note: remove the condition if
> it is always true
> for (j = 0; j < entries; j++) {
> ^~~~~~~~~~~
> drivers/pci/hotplug/rpaphp_core.c:233:14: note: initialize the variable
> 'fndit' to silence this warning
> int j, fndit;
> ^
> = 0
>
> fndit is only used to gate a sprintf call, which can be moved into the
> loop to simplify the code and eliminate the local variable, which will
> fix this warning.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/504
> Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info property")
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> Acked-by: Joel Savitz <jsavitz@redhat.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/0df3e42167caaf9f8c7b64de3da40a459979afe8
cheers
^ permalink raw reply
* [GIT PULL] Please pull powerpc/linux.git powerpc-5.3-4 tag
From: Michael Ellerman @ 2019-08-10 10:11 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linuxppc-dev, linux-kernel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hi Linus,
Please pull another powerpc fix for 5.3:
The following changes since commit d7e23b887f67178c4f840781be7a6aa6aeb52ab1:
powerpc/kasan: fix early boot failure on PPC32 (2019-07-31 22:02:52 +1000)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.3-4
for you to fetch changes up to ed4289e8b48845888ee46377bd2b55884a55e60b:
Revert "powerpc: slightly improve cache helpers" (2019-07-31 22:56:27 +1000)
- ------------------------------------------------------------------
powerpc fixes for 5.3 #4
Just one fix, a revert of a commit that was meant to be a minor improvement to
some inline asm, but ended up having no real benefit with GCC and broke booting
32-bit machines when using Clang.
Thanks to:
Arnd Bergmann, Christophe Leroy, Nathan Chancellor, Nick Desaulniers, Segher
Boessenkool.
- ------------------------------------------------------------------
Michael Ellerman (1):
Revert "powerpc: slightly improve cache helpers"
arch/powerpc/include/asm/cache.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAl1OmC4ACgkQUevqPMjh
pYBuKw/9H4ZMZpetAqOfJgfpByyhXlNRFriW1QS+KkMjjI60ZY1QYwZNFhZsUs8P
chpAvGnLv6zuJafOQ1hlxFa7wc70LTc/RRDRwWPFdtWdDl54OB3YvwFP2do2b/7D
gDtqCRMWTgtWdLx7ZpDk3qSN2mDxPRss//FO/UqtkTzNWh2LsQr9sbTZHc0BsIa/
VAyEZSGeFMkUBHjuCKoqyMZAc0aBk7HmJ4LN8zFGH6CtY05c1TBA4rN0qBnGBQI9
upNUgwbG2/vVS3t5YgHzHbrC8KfX+/8Oqo6ePLVlH39g0+wvp6avTeZDwrA7ZHtR
wNzHBxlLIpphOSRDr/fQk6yd69rVVbK14DZk3UHaz2qqyBkj3LxlRHKFIYWmPwoi
6E01ovThjSiHyxN1oPJfI1TXmhualhOlER9RTHPYsA20SnFYqAwH+TCydzuhvwAO
ueWSqKjK7O+KNhR1QJ3PGD6LIUWiPLMBG0a860Jg9oJ28/X5ZU/UqxBunoYUyQg+
u5bBViUtMkD8d7LEfQjvHSW9IpIRA/N2Y5cHTtmQxPEgspI/t/jXAxLEgkOBVkS2
kJoljNsTNFfKnyTD4v7zh5AABZquy5kyGLFBywQuloJ/7Jc4U3rBOqvRQ+f55E3s
naAU0Pf27Bob6VKKNPVeWP5OhPuYZ8YXHnvZ/fpcrYpA/k+xYsg=
=Dzq1
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v8 7/7] powerpc: add machine check safe copy_to_user
From: Michael Ellerman @ 2019-08-10 9:33 UTC (permalink / raw)
To: Santosh Sivaraj, linuxppc-dev, Linux Kernel
Cc: Aneesh Kumar K.V, Mahesh Salgaonkar, Nicholas Piggin,
Chandan Rajendra, Reza Arbab
In-Reply-To: <20190807145700.25599-8-santosh@fossix.org>
Santosh Sivaraj <santosh@fossix.org> writes:
> Use memcpy_mcsafe() implementation to define copy_to_user_mcsafe()
>
> Signed-off-by: Santosh Sivaraj <santosh@fossix.org>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/uaccess.h | 14 ++++++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 77f6ebf97113..4316e36095a2 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -137,6 +137,7 @@ config PPC
> select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !RELOCATABLE && !HIBERNATION)
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UACCESS_FLUSHCACHE if PPC64
> + select ARCH_HAS_UACCESS_MCSAFE if PPC64
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> select ARCH_KEEP_MEMBLOCK
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 8b03eb44e876..15002b51ff18 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -387,6 +387,20 @@ static inline unsigned long raw_copy_to_user(void __user *to,
> return ret;
> }
>
> +static __always_inline unsigned long __must_check
> +copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
> +{
> + if (likely(check_copy_size(from, n, true))) {
> + if (access_ok(to, n)) {
> + allow_write_to_user(to, n);
> + n = memcpy_mcsafe((void *)to, from, n);
> + prevent_write_to_user(to, n);
> + }
> + }
> +
> + return n;
> +}
This looks OK to me.
It would be nice though if copy_to_user_mcsafe() followed the pattern of
the other copy_to_user() etc. routines where the arch code is only
responsible for the actual arch details, and all the checks are done in
the generic code. That would be a good cleanup to do after this has gone
in, as the 2nd implementation of the API.
cheers
^ permalink raw reply
* Re: [PATCH v3] Revert "powerpc: slightly improve cache helpers"
From: Michael Ellerman @ 2019-08-10 9:09 UTC (permalink / raw)
To: Nick Desaulniers
Cc: arnd, Nick Desaulniers, linux-kernel, clang-built-linux,
Paul Mackerras, Nathan Chancellor, linuxppc-dev,
kbuild test robot
In-Reply-To: <20190809220348.127314-1-ndesaulniers@google.com>
Nick Desaulniers <ndesaulniers@google.com> writes:
> This reverts commit 6c5875843b87c3adea2beade9d1b8b3d4523900a.
>
> Work around Clang bug preventing ppc32 from booting.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=42762
> Link: https://github.com/ClangBuiltLinux/linux/issues/593
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: kbuild test robot <lkp@intel.com>
> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V2 -> V3:
> * Just revert, as per Christophe.
> Changes V1 -> V2:
> * Change to ouput paremeter.
Thanks. I actually already had this revert in my tree since ~10 days
ago, but hadn't pushed it yet because the discussion was ongoing.
So I'll just use that version, and ask Linus to pull it.
cheers
^ permalink raw reply
* Re: [PATCH v4 0/6] Remove x86-specific code from generic headers
From: Christoph Hellwig @ 2019-08-10 7:46 UTC (permalink / raw)
To: mpe
Cc: linux-s390, Mike Anderson, Lianbo Jiang, Konrad Rzeszutek Wilk,
Robin Murphy, x86, Ram Pai, linux-kernel, Alexey Dobriyan,
Halil Pasic, iommu, Ingo Molnar, Borislav Petkov, Thomas Lendacky,
H. Peter Anvin, linux-fsdevel, Thomas Gleixner, linuxppc-dev,
Christoph Hellwig, Thiago Jung Bauermann, Marek Szyprowski
In-Reply-To: <87sgqasdr6.fsf@concordia.ellerman.id.au>
On Fri, Aug 09, 2019 at 10:51:41PM +1000, mpe@ellerman.id.au wrote:
> I need to take this series via the powerpc tree because there is another
> fairly large powerpc specific series dependent on it.
>
> I think this series already has pretty much all the acks it needs, which
> almost never happens, amazing work!
>
> I'll put the series in a topic branch, just in case there's any bad
> conflicts and other folks want to merge it later on. I'll then merge the
> topic branch into my next, and so this series will be tested in
> linux-next that way.
Sounds good to me, I don't expect conflicts from the dma-mapping tree.
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Use refcount_t for refcount
From: Chuhong Yuan @ 2019-08-10 1:25 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Paul Mackerras, linuxppc-dev, LKML
In-Reply-To: <87y302segw.fsf@concordia.ellerman.id.au>
On Fri, Aug 9, 2019 at 8:36 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Chuhong Yuan <hslester96@gmail.com> writes:
> > Reference counters are preferred to use refcount_t instead of
> > atomic_t.
> > This is because the implementation of refcount_t can prevent
> > overflows and detect possible use-after-free.
> > So convert atomic_t ref counters to refcount_t.
> >
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
>
> Thanks.
>
> We don't have a fast implementation of refcount_t, so I'm worried this
> could cause a measurable performance regression.
>
> Did you benchmark it at all?
>
I did not benchmark it and I don't have the testing environment...
> cheers
>
> > diff --git a/arch/powerpc/mm/book3s64/mmu_context.c b/arch/powerpc/mm/book3s64/mmu_context.c
> > index 2d0cb5ba9a47..f836fd5a6abc 100644
> > --- a/arch/powerpc/mm/book3s64/mmu_context.c
> > +++ b/arch/powerpc/mm/book3s64/mmu_context.c
> > @@ -231,7 +231,7 @@ static void pmd_frag_destroy(void *pmd_frag)
> > /* drop all the pending references */
> > count = ((unsigned long)pmd_frag & ~PAGE_MASK) >> PMD_FRAG_SIZE_SHIFT;
> > /* We allow PTE_FRAG_NR fragments from a PTE page */
> > - if (atomic_sub_and_test(PMD_FRAG_NR - count, &page->pt_frag_refcount)) {
> > + if (refcount_sub_and_test(PMD_FRAG_NR - count, &page->pt_frag_refcount)) {
> > pgtable_pmd_page_dtor(page);
> > __free_page(page);
> > }
> > diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
> > index 7d0e0d0d22c4..40056896ce4e 100644
> > --- a/arch/powerpc/mm/book3s64/pgtable.c
> > +++ b/arch/powerpc/mm/book3s64/pgtable.c
> > @@ -277,7 +277,7 @@ static pmd_t *__alloc_for_pmdcache(struct mm_struct *mm)
> > return NULL;
> > }
> >
> > - atomic_set(&page->pt_frag_refcount, 1);
> > + refcount_set(&page->pt_frag_refcount, 1);
> >
> > ret = page_address(page);
> > /*
> > @@ -294,7 +294,7 @@ static pmd_t *__alloc_for_pmdcache(struct mm_struct *mm)
> > * count.
> > */
> > if (likely(!mm->context.pmd_frag)) {
> > - atomic_set(&page->pt_frag_refcount, PMD_FRAG_NR);
> > + refcount_set(&page->pt_frag_refcount, PMD_FRAG_NR);
> > mm->context.pmd_frag = ret + PMD_FRAG_SIZE;
> > }
> > spin_unlock(&mm->page_table_lock);
> > @@ -317,8 +317,7 @@ void pmd_fragment_free(unsigned long *pmd)
> > {
> > struct page *page = virt_to_page(pmd);
> >
> > - BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
> > - if (atomic_dec_and_test(&page->pt_frag_refcount)) {
> > + if (refcount_dec_and_test(&page->pt_frag_refcount)) {
> > pgtable_pmd_page_dtor(page);
> > __free_page(page);
> > }
> > diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
> > index a7b05214760c..4ef8231b677f 100644
> > --- a/arch/powerpc/mm/pgtable-frag.c
> > +++ b/arch/powerpc/mm/pgtable-frag.c
> > @@ -24,7 +24,7 @@ void pte_frag_destroy(void *pte_frag)
> > /* drop all the pending references */
> > count = ((unsigned long)pte_frag & ~PAGE_MASK) >> PTE_FRAG_SIZE_SHIFT;
> > /* We allow PTE_FRAG_NR fragments from a PTE page */
> > - if (atomic_sub_and_test(PTE_FRAG_NR - count, &page->pt_frag_refcount)) {
> > + if (refcount_sub_and_test(PTE_FRAG_NR - count, &page->pt_frag_refcount)) {
> > pgtable_page_dtor(page);
> > __free_page(page);
> > }
> > @@ -71,7 +71,7 @@ static pte_t *__alloc_for_ptecache(struct mm_struct *mm, int kernel)
> > return NULL;
> > }
> >
> > - atomic_set(&page->pt_frag_refcount, 1);
> > + refcount_set(&page->pt_frag_refcount, 1);
> >
> > ret = page_address(page);
> > /*
> > @@ -87,7 +87,7 @@ static pte_t *__alloc_for_ptecache(struct mm_struct *mm, int kernel)
> > * count.
> > */
> > if (likely(!pte_frag_get(&mm->context))) {
> > - atomic_set(&page->pt_frag_refcount, PTE_FRAG_NR);
> > + refcount_set(&page->pt_frag_refcount, PTE_FRAG_NR);
> > pte_frag_set(&mm->context, ret + PTE_FRAG_SIZE);
> > }
> > spin_unlock(&mm->page_table_lock);
> > @@ -110,8 +110,7 @@ void pte_fragment_free(unsigned long *table, int kernel)
> > {
> > struct page *page = virt_to_page(table);
> >
> > - BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
> > - if (atomic_dec_and_test(&page->pt_frag_refcount)) {
> > + if (refcount_dec_and_test(&page->pt_frag_refcount)) {
> > if (!kernel)
> > pgtable_page_dtor(page);
> > __free_page(page);
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 3a37a89eb7a7..7fe23a3faf95 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -14,6 +14,7 @@
> > #include <linux/uprobes.h>
> > #include <linux/page-flags-layout.h>
> > #include <linux/workqueue.h>
> > +#include <linux/refcount.h>
> >
> > #include <asm/mmu.h>
> >
> > @@ -147,7 +148,7 @@ struct page {
> > unsigned long _pt_pad_2; /* mapping */
> > union {
> > struct mm_struct *pt_mm; /* x86 pgds only */
> > - atomic_t pt_frag_refcount; /* powerpc */
> > + refcount_t pt_frag_refcount; /* powerpc */
> > };
> > #if ALLOC_SPLIT_PTLOCKS
> > spinlock_t *ptl;
> > --
> > 2.20.1
^ permalink raw reply
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Segher Boessenkool @ 2019-08-09 22:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a1AwmAe+PpHTRmN153fhG4ZkF_pb+240rj1ZAg-S6SKeg@mail.gmail.com>
On Fri, Aug 09, 2019 at 10:12:56PM +0200, Arnd Bergmann wrote:
> @@ -106,7 +106,7 @@ static inline u##size name(const volatile u##size
> __iomem *addr) \
> { \
> u##size ret; \
> __asm__ __volatile__("sync;"#insn" %0,%y1;twi 0,%0,0;isync" \
> - : "=r" (ret) : "Z" (*addr) : "memory"); \
> + : "=r" (ret) : "m" (*addr) : "memory"); \
> return ret; \
> }
That will no longer compile something like
u8 *p;
u16 x = in_le16(p + 12);
(you'll get something like "invalid %y value, try using the 'Z' constraint").
So then you remove the %y, but that makes you get something like
sync;lhbrx 3,12(3);twi 0,3,0;isync
which is completely wrong.
Segher
^ permalink raw reply
* [PATCH v3] Revert "powerpc: slightly improve cache helpers"
From: Nick Desaulniers @ 2019-08-09 22:03 UTC (permalink / raw)
To: mpe
Cc: arnd, Nick Desaulniers, linux-kernel, clang-built-linux,
Paul Mackerras, Nathan Chancellor, linuxppc-dev,
kbuild test robot
In-Reply-To: <20190809220301.Horde.AR6y4Bx4WGIq58V9K0En9g4@messagerie.si.c-s.fr>
This reverts commit 6c5875843b87c3adea2beade9d1b8b3d4523900a.
Work around Clang bug preventing ppc32 from booting.
Link: https://bugs.llvm.org/show_bug.cgi?id=42762
Link: https://github.com/ClangBuiltLinux/linux/issues/593
Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: Nathan Chancellor <natechancellor@gmail.com>
Reported-by: kbuild test robot <lkp@intel.com>
Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V2 -> V3:
* Just revert, as per Christophe.
Changes V1 -> V2:
* Change to ouput paremeter.
arch/powerpc/include/asm/cache.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/cache.h b/arch/powerpc/include/asm/cache.h
index b3388d95f451..45e3137ccd71 100644
--- a/arch/powerpc/include/asm/cache.h
+++ b/arch/powerpc/include/asm/cache.h
@@ -107,22 +107,22 @@ extern void _set_L3CR(unsigned long);
static inline void dcbz(void *addr)
{
- __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
+ __asm__ __volatile__ ("dcbz 0, %0" : : "r"(addr) : "memory");
}
static inline void dcbi(void *addr)
{
- __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
+ __asm__ __volatile__ ("dcbi 0, %0" : : "r"(addr) : "memory");
}
static inline void dcbf(void *addr)
{
- __asm__ __volatile__ ("dcbf %y0" : : "Z"(*(u8 *)addr) : "memory");
+ __asm__ __volatile__ ("dcbf 0, %0" : : "r"(addr) : "memory");
}
static inline void dcbst(void *addr)
{
- __asm__ __volatile__ ("dcbst %y0" : : "Z"(*(u8 *)addr) : "memory");
+ __asm__ __volatile__ ("dcbst 0, %0" : : "r"(addr) : "memory");
}
#endif /* !__ASSEMBLY__ */
#endif /* __KERNEL__ */
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Nick Desaulniers @ 2019-08-09 22:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Linux Kernel Mailing List, clang-built-linux,
Paul Mackerras, Nathan Chancellor, linuxppc-dev
In-Reply-To: <CAK8P3a1AwmAe+PpHTRmN153fhG4ZkF_pb+240rj1ZAg-S6SKeg@mail.gmail.com>
On Fri, Aug 9, 2019 at 1:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Aug 9, 2019 at 10:02 PM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
> >
> > Arnd Bergmann <arnd@arndb.de> a écrit :
> > > On Fri, Aug 9, 2019 at 8:21 PM 'Nick Desaulniers' via Clang Built
> > > Linux <clang-built-linux@googlegroups.com> wrote:
> > >
> > >> static inline void dcbz(void *addr)
> > >> {
> > >> - __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
> > >> + __asm__ __volatile__ ("dcbz %y0" : "=Z"(*(u8 *)addr) :: "memory");
> > >> }
> > >>
> > >> static inline void dcbi(void *addr)
> > >> {
> > >> - __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
> > >> + __asm__ __volatile__ ("dcbi %y0" : "=Z"(*(u8 *)addr) :: "memory");
> > >> }
> > >
> > > I think the result of the discussion was that an output argument only kind-of
> > > makes sense for dcbz, but for the others it's really an input, and clang is
> > > wrong in the way it handles the "Z" constraint by making a copy, which it
> > > doesn't do for "m".
> > >
> > > I'm not sure whether it's correct to use "m" instead of "Z" here, which
> > > would be a better workaround if that works. More importantly though,
> > > clang really needs to be fixed to handle "Z" correctly.
> >
> > As the benefit is null, I think the best is probably to reverse my
> > original commit until at least CLang is fixed, as initialy suggested
> > by mpe
>
> Yes, makes sense.
>
> There is one other use of the "Z" constraint, so on top of the revert, I
> think it might be helpful if Nick could check if the patch below makes
> any difference with clang and, if it does, whether the current version
> is broken.
>
> Arnd
>
> diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
> index 23e5d5d16c7e..28b467779328 100644
> --- a/arch/powerpc/include/asm/io.h
> +++ b/arch/powerpc/include/asm/io.h
> @@ -106,7 +106,7 @@ static inline u##size name(const volatile u##size
> __iomem *addr) \
> { \
> u##size ret; \
> __asm__ __volatile__("sync;"#insn" %0,%y1;twi 0,%0,0;isync" \
> - : "=r" (ret) : "Z" (*addr) : "memory"); \
> + : "=r" (ret) : "m" (*addr) : "memory"); \
> return ret; \
> }
>
> @@ -114,7 +114,7 @@ static inline u##size name(const volatile u##size
> __iomem *addr) \
> static inline void name(volatile u##size __iomem *addr, u##size val) \
> { \
> __asm__ __volatile__("sync;"#insn" %1,%y0" \
> - : "=Z" (*addr) : "r" (val) : "memory"); \
> + : "=m" (*addr) : "r" (val) : "memory"); \
> mmiowb_set_pending(); \
> }
Does not work:
https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/122654899
https://github.com/ClangBuiltLinux/continuous-integration/pull/197/files#diff-40bd16e3188587e4d648c30e0c2d6d37
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Segher Boessenkool @ 2019-08-09 22:00 UTC (permalink / raw)
To: Christophe Leroy
Cc: kbuild test robot, Arnd Bergmann, Nick Desaulniers,
Linux Kernel Mailing List, clang-built-linux, Paul Mackerras,
Nathan Chancellor, linuxppc-dev
In-Reply-To: <20190809220301.Horde.AR6y4Bx4WGIq58V9K0En9g4@messagerie.si.c-s.fr>
On Fri, Aug 09, 2019 at 10:03:01PM +0200, Christophe Leroy wrote:
> Arnd Bergmann <arnd@arndb.de> a écrit :
>
> >On Fri, Aug 9, 2019 at 8:21 PM 'Nick Desaulniers' via Clang Built
> >Linux <clang-built-linux@googlegroups.com> wrote:
> >
> >> static inline void dcbz(void *addr)
> >> {
> >>- __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
> >>+ __asm__ __volatile__ ("dcbz %y0" : "=Z"(*(u8 *)addr) :: "memory");
> >> }
> >>
> >> static inline void dcbi(void *addr)
> >> {
> >>- __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
> >>+ __asm__ __volatile__ ("dcbi %y0" : "=Z"(*(u8 *)addr) :: "memory");
> >> }
> >
> >I think the result of the discussion was that an output argument only
> >kind-of
> >makes sense for dcbz, but for the others it's really an input, and clang is
> >wrong in the way it handles the "Z" constraint by making a copy, which it
> >doesn't do for "m".
> >
> >I'm not sure whether it's correct to use "m" instead of "Z" here, which
> >would be a better workaround if that works. More importantly though,
> >clang really needs to be fixed to handle "Z" correctly.
>
> As the benefit is null, I think the best is probably to reverse my
> original commit until at least CLang is fixed, as initialy suggested
> by mpe
And what about the other uses of "Z"?
Also, if you use C routines (instead of assembler code) for the basic
"clear a block" and the like routines, as there have been patches for
recently, the benefit is not zero.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Segher Boessenkool @ 2019-08-09 21:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a3LynWTbpV8=VPm2TqgNM2MnoEyCPJd0PL2D+tcZqJgHg@mail.gmail.com>
On Fri, Aug 09, 2019 at 08:28:19PM +0200, Arnd Bergmann wrote:
> On Fri, Aug 9, 2019 at 8:21 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
>
> > static inline void dcbz(void *addr)
> > {
> > - __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
> > + __asm__ __volatile__ ("dcbz %y0" : "=Z"(*(u8 *)addr) :: "memory");
> > }
> >
> > static inline void dcbi(void *addr)
> > {
> > - __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
> > + __asm__ __volatile__ ("dcbi %y0" : "=Z"(*(u8 *)addr) :: "memory");
> > }
>
> I think the result of the discussion was that an output argument only kind-of
> makes sense for dcbz, but for the others it's really an input, and clang is
> wrong in the way it handles the "Z" constraint by making a copy, which it
> doesn't do for "m".
Yes. And clang has probably miscompiled this in all kernels since we
have used "Z" for the first time, in 2008 (0f3d6bcd391b).
It is not necessarily fatal or at least not easily visible for the I/O
accessors: it "just" gets memory ordering wrong slightly (it looks like
it does the sync;tw;isync thing around an extra stack access, after it
has performed the actual I/O as any other memory load, without any
synchronisation).
> I'm not sure whether it's correct to use "m" instead of "Z" here, which
> would be a better workaround if that works. More importantly though,
> clang really needs to be fixed to handle "Z" correctly.
"m" allows offset addressing, which these insns do not. That is the
same reason you need the "y" output modifier. "m" is wrong here.
We have other memory constraints, but do those work with LLVM?
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Nathan Chancellor @ 2019-08-09 20:36 UTC (permalink / raw)
To: Nick Desaulniers
Cc: arnd, linux-kernel, clang-built-linux, Paul Mackerras,
linuxppc-dev, kbuild test robot
In-Reply-To: <20190809182106.62130-1-ndesaulniers@google.com>
[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]
On Fri, Aug 09, 2019 at 11:21:05AM -0700, Nick Desaulniers wrote:
> The input parameter is modified, so it should be an output parameter
> with "=" to make it so that a copy of the input is not made by Clang.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=42762
> Link: https://gcc.gnu.org/onlinedocs/gcc/Modifiers.html#Modifiers
> Link: https://github.com/ClangBuiltLinux/linux/issues/593
> Link: https://godbolt.org/z/QwhZXi
> Link: https://lore.kernel.org/lkml/20190721075846.GA97701@archlinux-threadripper/
> Fixes: 6c5875843b87 ("powerpc: slightly improve cache helpers")
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Reported-by: kbuild test robot <lkp@intel.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
I applied this patch as well as a revert of the original patch and both
clang and GCC appear to generate the same code; I think a straight
revert would be better.
Crude testing script and the generated files attached.
Cheers,
Nathan
[-- Attachment #2: tmp.bRmcRT0jd0.sh --]
[-- Type: application/x-sh, Size: 2707 bytes --]
[-- Attachment #3: testing-output.tar.gz --]
[-- Type: application/gzip, Size: 16412 bytes --]
^ permalink raw reply
* [Bug 204479] KASAN hit at modprobe zram
From: bugzilla-daemon @ 2019-08-09 20:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-204479-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=204479
--- Comment #15 from Christophe Leroy (christophe.leroy@c-s.fr) ---
As far as I can see in the latest dmesg, the Oops occurs in raid6 pq module.
An this time it is not anymore in kasan register global.
--
You are receiving this mail because:
You are on the CC list for the bug.
^ permalink raw reply
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Arnd Bergmann @ 2019-08-09 20:12 UTC (permalink / raw)
To: Christophe Leroy
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <20190809220301.Horde.AR6y4Bx4WGIq58V9K0En9g4@messagerie.si.c-s.fr>
On Fri, Aug 9, 2019 at 10:02 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
> Arnd Bergmann <arnd@arndb.de> a écrit :
> > On Fri, Aug 9, 2019 at 8:21 PM 'Nick Desaulniers' via Clang Built
> > Linux <clang-built-linux@googlegroups.com> wrote:
> >
> >> static inline void dcbz(void *addr)
> >> {
> >> - __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
> >> + __asm__ __volatile__ ("dcbz %y0" : "=Z"(*(u8 *)addr) :: "memory");
> >> }
> >>
> >> static inline void dcbi(void *addr)
> >> {
> >> - __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
> >> + __asm__ __volatile__ ("dcbi %y0" : "=Z"(*(u8 *)addr) :: "memory");
> >> }
> >
> > I think the result of the discussion was that an output argument only kind-of
> > makes sense for dcbz, but for the others it's really an input, and clang is
> > wrong in the way it handles the "Z" constraint by making a copy, which it
> > doesn't do for "m".
> >
> > I'm not sure whether it's correct to use "m" instead of "Z" here, which
> > would be a better workaround if that works. More importantly though,
> > clang really needs to be fixed to handle "Z" correctly.
>
> As the benefit is null, I think the best is probably to reverse my
> original commit until at least CLang is fixed, as initialy suggested
> by mpe
Yes, makes sense.
There is one other use of the "Z" constraint, so on top of the revert, I
think it might be helpful if Nick could check if the patch below makes
any difference with clang and, if it does, whether the current version
is broken.
Arnd
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 23e5d5d16c7e..28b467779328 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -106,7 +106,7 @@ static inline u##size name(const volatile u##size
__iomem *addr) \
{ \
u##size ret; \
__asm__ __volatile__("sync;"#insn" %0,%y1;twi 0,%0,0;isync" \
- : "=r" (ret) : "Z" (*addr) : "memory"); \
+ : "=r" (ret) : "m" (*addr) : "memory"); \
return ret; \
}
@@ -114,7 +114,7 @@ static inline u##size name(const volatile u##size
__iomem *addr) \
static inline void name(volatile u##size __iomem *addr, u##size val) \
{ \
__asm__ __volatile__("sync;"#insn" %1,%y0" \
- : "=Z" (*addr) : "r" (val) : "memory"); \
+ : "=m" (*addr) : "r" (val) : "memory"); \
mmiowb_set_pending(); \
}
^ permalink raw reply related
* Re: [PATCH] powerpc: fix inline asm constraints for dcbz
From: Christophe Leroy @ 2019-08-09 20:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a3LynWTbpV8=VPm2TqgNM2MnoEyCPJd0PL2D+tcZqJgHg@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> a écrit :
> On Fri, Aug 9, 2019 at 8:21 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
>
>> static inline void dcbz(void *addr)
>> {
>> - __asm__ __volatile__ ("dcbz %y0" : : "Z"(*(u8 *)addr) : "memory");
>> + __asm__ __volatile__ ("dcbz %y0" : "=Z"(*(u8 *)addr) :: "memory");
>> }
>>
>> static inline void dcbi(void *addr)
>> {
>> - __asm__ __volatile__ ("dcbi %y0" : : "Z"(*(u8 *)addr) : "memory");
>> + __asm__ __volatile__ ("dcbi %y0" : "=Z"(*(u8 *)addr) :: "memory");
>> }
>
> I think the result of the discussion was that an output argument only kind-of
> makes sense for dcbz, but for the others it's really an input, and clang is
> wrong in the way it handles the "Z" constraint by making a copy, which it
> doesn't do for "m".
>
> I'm not sure whether it's correct to use "m" instead of "Z" here, which
> would be a better workaround if that works. More importantly though,
> clang really needs to be fixed to handle "Z" correctly.
As the benefit is null, I think the best is probably to reverse my
original commit until at least CLang is fixed, as initialy suggested
by mpe
Christophe
^ 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