LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 33/36] dt-bindings: arm: Convert Tegra board/soc bindings to json-schema
From: Marcel Ziswiler @ 2018-10-05 22:19 UTC (permalink / raw)
  To: robh@kernel.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org
  Cc: mark.rutland@arm.com, trini@konsulko.com, kumar.gala@linaro.org,
	glikely@secretlab.ca, arnd@arndb.de, linus.walleij@linaro.org,
	pantelis.antoniou@konsulko.com, jonathanh@nvidia.com,
	linux-tegra@vger.kernel.org, broonie@kernel.org,
	thierry.reding@gmail.com, olof@lixom.net, geert@linux-m68k.org,
	bjorn.andersson@linaro.org, frowand.list@gmail.com,
	jic23@kernel.org
In-Reply-To: <20181005165848.3474-34-robh@kernel.org>

Hi Rob

On Fri, 2018-10-05 at 11:58 -0500, Rob Herring wrote:
> Convert Tegra SoC bindings to DT schema format using json-schema.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-tegra@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/arm/tegra.txt         | 60 -------------
>  .../devicetree/bindings/arm/tegra.yaml        | 88
> +++++++++++++++++++
>  2 files changed, 88 insertions(+), 60 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/tegra.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
> b/Documentation/devicetree/bindings/arm/tegra.txt
> deleted file mode 100644
> index 32f62bb7006d..000000000000
> --- a/Documentation/devicetree/bindings/arm/tegra.txt
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -NVIDIA Tegra device tree bindings
> --------------------------------------------
> -
> -SoCs
> --------------------------------------------
> -
> -Each device tree must specify which Tegra SoC it uses, using one of
> the
> -following compatible values:
> -
> -  nvidia,tegra20
> -  nvidia,tegra30
> -  nvidia,tegra114
> -  nvidia,tegra124
> -  nvidia,tegra132
> -  nvidia,tegra210
> -  nvidia,tegra186
> -  nvidia,tegra194
> -
> -Boards
> --------------------------------------------
> -
> -Each device tree must specify which one or more of the following
> -board-specific compatible values:
> -
> -  ad,medcom-wide
> -  ad,plutux
> -  ad,tamonten
> -  ad,tec
> -  compal,paz00
> -  compulab,trimslice
> -  nvidia,beaver
> -  nvidia,cardhu
> -  nvidia,cardhu-a02
> -  nvidia,cardhu-a04
> -  nvidia,dalmore
> -  nvidia,harmony
> -  nvidia,jetson-tk1
> -  nvidia,norrin
> -  nvidia,p2371-0000
> -  nvidia,p2371-2180
> -  nvidia,p2571
> -  nvidia,p2771-0000
> -  nvidia,p2972-0000
> -  nvidia,roth
> -  nvidia,seaboard
> -  nvidia,tn7
> -  nvidia,ventana
> -  toradex,apalis_t30
> -  toradex,apalis_t30-eval
> -  toradex,apalis-tk1
> -  toradex,apalis-tk1-eval
> -  toradex,colibri_t20-512
> -  toradex,colibri_t30
> -  toradex,colibri_t30-eval-v3
> -  toradex,iris

Are you aware that -next already features a few updating commits
thereof from around the beginning of September one of which even bears
your reviewed-by.

> -
> -Trusted Foundations
> --------------------------------------------
> -Tegra supports the Trusted Foundation secure monitor. See the
> -"tlm,trusted-foundations" binding's documentation for more details.
> diff --git a/Documentation/devicetree/bindings/arm/tegra.yaml
> b/Documentation/devicetree/bindings/arm/tegra.yaml
> new file mode 100644
> index 000000000000..9cebcfaaad1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra.yaml
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: None
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/arm/tegra.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra device tree bindings
> +
> +maintainers:
> +  - Marcel Ziswiler <marcel.ziswiler@toradex.com>

Wow, seems I got promoted to maintainer now. I guess that may make
sense at least for the Toradex based boards.

> +  - Peter De Schrijver <pdeschrijver@nvidia.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - compal,paz00
> +              - compulab,trimslice
> +              - nvidia,harmony
> +              - nvidia,seaboard
> +              - nvidia,ventana
> +          - const: nvidia,tegra20
> +      - items:
> +          - enum:
> +              - ad,medcom-wide
> +              - ad,plutux
> +              - ad,tec
> +          - const: ad,tamonten
> +          - const: nvidia,tegra20
> +      - items:
> +          - const: toradex,iris
> +          - const: toradex,colibri_t20-512
> +          - const: nvidia,tegra20
> +      - items:
> +          - enum:
> +              - nvidia,beaver
> +          - const: nvidia,tegra30
> +      - items:
> +          - enum:
> +              - nvidia,cardhu-a02
> +              - nvidia,cardhu-a04
> +          - const: nvidia,cardhu
> +          - const: nvidia,tegra30
> +      - items:
> +          - enum:
> +              - toradex,apalis_t30-eval
> +          - const: toradex,apalis_t30
> +          - const: nvidia,tegra30
> +      - items:
> +          - enum:
> +              - toradex,colibri_t30-eval-v3
> +          - const: toradex,colibri_t30
> +          - const: nvidia,tegra30
> +      - items:
> +          - enum:
> +              - nvidia,dalmore
> +              - nvidia,roth
> +              - nvidia,tn7
> +          - const: nvidia,tegra114
> +      - items:
> +          - enum:
> +              - nvidia,jetson-tk1
> +              - nvidia,venice2
> +          - const: nvidia,tegra124
> +      - items:
> +          - const: toradex,apalis-tk1-eval
> +          - const: toradex,apalis-tk1
> +          - const: nvidia,tegra124
> +      - items:
> +          - enum:
> +              - nvidia,norrin
> +          - const: nvidia,tegra132
> +          - const: nvidia,tegra124
> +      - items:
> +          - enum:
> +              - nvidia,p2371-0000
> +              - nvidia,p2371-2180
> +              - nvidia,p2571
> +          - const: nvidia,tegra210
> +      - items:
> +          - enum:
> +              - nvidia,p2771-0000
> +          - const: nvidia,tegra186
> +      - items:
> +          - enum:
> +              - nvidia,p2972-0000
> +          - const: nvidia,tegra194

Other than that I'm all in to move towards more structured bindings documentation.

Cheers

Marcel

^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Jason A. Donenfeld @ 2018-10-05 18:28 UTC (permalink / raw)
  To: Andrew Lutomirski
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	paulus, Herbert Xu, richard, Eric Biggers, mingo, Kees Cook,
	Arnd Bergmann, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Ard Biesheuvel, Greg Kroah-Hartman, LKML,
	Linux Crypto Mailing List, Andrew Morton, linuxppc-dev,
	David Miller
In-Reply-To: <CALCETrWjCY2Wceiqu99C+g5RdnrZuRujYO_+jBvuJFRB1LTG4w@mail.gmail.com>

Hey Andy,

On Fri, Oct 5, 2018 at 7:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> I *think* the only change to Zinc per se would be that the calls like
> chacha20_simd() would be static calls or patchable functions or
> whatever we want to call them.  And there could be a debugfs to
> override the default selection.

Yea, right, exactly. It turns out this is really easy to do with the
way it's structured now. I'd actually experimented considerably with
using the static keys a while back, but couldn't find any performance
difference on any platform at all (four ARM microarchitectures, three
MIPS, various random intel, an old powerpc), so went with the simplest
solution. But we can certainly play with more elaborate patching
mechanisms later on and see how those turn out. Also, even with the
simple bools as we have now, it's quite easy to make all the
parameters toggle-able.

> Ard, I don't think that sticking this in udev rules makes sense.  The
> kernel has bascially complete information as to what the right choice
> is, and that will change over time as the implementation gets tuned,
> and the udev rules will never get updated in sync.

Yes, I agree with this.

Jason

^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Jason A. Donenfeld @ 2018-10-05 17:37 UTC (permalink / raw)
  To: Andrew Lutomirski
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	paulus, Herbert Xu, richard, Eric Biggers, mingo, Kees Cook,
	Arnd Bergmann, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Ard Biesheuvel, Greg Kroah-Hartman, LKML,
	Linux Crypto Mailing List, Andrew Morton, linuxppc-dev,
	David Miller
In-Reply-To: <CALCETrVMBULVP9qEQDVuisxAMzdheM1qQ97AZFs-xcO0eaXiiA@mail.gmail.com>

On Fri, Oct 5, 2018 at 7:29 PM Andy Lutomirski <luto@kernel.org> wrote:
> (None of this is to say that I disagree with Jason, though -- I'm not
> entirely convinced that this makes sense for Zinc.  But maybe it can
> be done in a way that makes everyone happy.)

Zinc indeed will continue to push in the simpler and more minimal
direction. Down the line I'm open to trying and benching a few
different ways of going about it with dynamic patching -- something
that will be pretty easy to experiment with given the lean structure
of Zinc -- but for the initial merge I intend to do it the way it is,
which is super fast and pretty straightforward to follow.

Jason

^ permalink raw reply

* [PATCH AUTOSEL 4.18 24/48] KVM: PPC: Book3S HV: Don't use compound_order to determine host mapping size
From: Sasha Levin @ 2018-10-05 16:14 UTC (permalink / raw)
  To: stable, linux-kernel
  Cc: Sasha Levin, Aneesh Kumar K.V, kvm-ppc, Nicholas Piggin,
	linuxppc-dev, David Gibson
In-Reply-To: <20181005161424.20521-1-sashal@kernel.org>

From: Nicholas Piggin <npiggin@gmail.com>

[ Upstream commit 71d29f43b6332badc5598c656616a62575e83342 ]

THP paths can defer splitting compound pages until after the actual
remap and TLB flushes to split a huge PMD/PUD. This causes radix
partition scope page table mappings to get out of synch with the host
qemu page table mappings.

This results in random memory corruption in the guest when running
with THP. The easiest way to reproduce is use KVM balloon to free up
a lot of memory in the guest and then shrink the balloon to give the
memory back, while some work is being done in the guest.

Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
---
 arch/powerpc/kvm/book3s_64_mmu_radix.c | 91 +++++++++++---------------
 1 file changed, 37 insertions(+), 54 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index 7efc42538ccf..ae023d2256ef 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -538,8 +538,8 @@ int kvmppc_book3s_radix_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 				   unsigned long ea, unsigned long dsisr)
 {
 	struct kvm *kvm = vcpu->kvm;
-	unsigned long mmu_seq, pte_size;
-	unsigned long gpa, gfn, hva, pfn;
+	unsigned long mmu_seq;
+	unsigned long gpa, gfn, hva;
 	struct kvm_memory_slot *memslot;
 	struct page *page = NULL;
 	long ret;
@@ -636,9 +636,10 @@ int kvmppc_book3s_radix_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	 */
 	hva = gfn_to_hva_memslot(memslot, gfn);
 	if (upgrade_p && __get_user_pages_fast(hva, 1, 1, &page) == 1) {
-		pfn = page_to_pfn(page);
 		upgrade_write = true;
 	} else {
+		unsigned long pfn;
+
 		/* Call KVM generic code to do the slow-path check */
 		pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
 					   writing, upgrade_p);
@@ -652,63 +653,45 @@ int kvmppc_book3s_radix_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		}
 	}
 
-	/* See if we can insert a 1GB or 2MB large PTE here */
-	level = 0;
-	if (page && PageCompound(page)) {
-		pte_size = PAGE_SIZE << compound_order(compound_head(page));
-		if (pte_size >= PUD_SIZE &&
-		    (gpa & (PUD_SIZE - PAGE_SIZE)) ==
-		    (hva & (PUD_SIZE - PAGE_SIZE))) {
-			level = 2;
-			pfn &= ~((PUD_SIZE >> PAGE_SHIFT) - 1);
-		} else if (pte_size >= PMD_SIZE &&
-			   (gpa & (PMD_SIZE - PAGE_SIZE)) ==
-			   (hva & (PMD_SIZE - PAGE_SIZE))) {
-			level = 1;
-			pfn &= ~((PMD_SIZE >> PAGE_SHIFT) - 1);
-		}
-	}
-
 	/*
-	 * Compute the PTE value that we need to insert.
+	 * Read the PTE from the process' radix tree and use that
+	 * so we get the shift and attribute bits.
 	 */
-	if (page) {
-		pgflags = _PAGE_READ | _PAGE_EXEC | _PAGE_PRESENT | _PAGE_PTE |
-			_PAGE_ACCESSED;
-		if (writing || upgrade_write)
-			pgflags |= _PAGE_WRITE | _PAGE_DIRTY;
-		pte = pfn_pte(pfn, __pgprot(pgflags));
+	local_irq_disable();
+	ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
+	pte = *ptep;
+	local_irq_enable();
+
+	/* Get pte level from shift/size */
+	if (shift == PUD_SHIFT &&
+	    (gpa & (PUD_SIZE - PAGE_SIZE)) ==
+	    (hva & (PUD_SIZE - PAGE_SIZE))) {
+		level = 2;
+	} else if (shift == PMD_SHIFT &&
+		   (gpa & (PMD_SIZE - PAGE_SIZE)) ==
+		   (hva & (PMD_SIZE - PAGE_SIZE))) {
+		level = 1;
 	} else {
-		/*
-		 * Read the PTE from the process' radix tree and use that
-		 * so we get the attribute bits.
-		 */
-		local_irq_disable();
-		ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
-		pte = *ptep;
-		local_irq_enable();
-		if (shift == PUD_SHIFT &&
-		    (gpa & (PUD_SIZE - PAGE_SIZE)) ==
-		    (hva & (PUD_SIZE - PAGE_SIZE))) {
-			level = 2;
-		} else if (shift == PMD_SHIFT &&
-			   (gpa & (PMD_SIZE - PAGE_SIZE)) ==
-			   (hva & (PMD_SIZE - PAGE_SIZE))) {
-			level = 1;
-		} else if (shift && shift != PAGE_SHIFT) {
-			/* Adjust PFN */
-			unsigned long mask = (1ul << shift) - PAGE_SIZE;
-			pte = __pte(pte_val(pte) | (hva & mask));
-		}
-		pte = __pte(pte_val(pte) | _PAGE_EXEC | _PAGE_ACCESSED);
-		if (writing || upgrade_write) {
-			if (pte_val(pte) & _PAGE_WRITE)
-				pte = __pte(pte_val(pte) | _PAGE_DIRTY);
-		} else {
-			pte = __pte(pte_val(pte) & ~(_PAGE_WRITE | _PAGE_DIRTY));
+		level = 0;
+		if (shift > PAGE_SHIFT) {
+			/*
+			 * If the pte maps more than one page, bring over
+			 * bits from the virtual address to get the real
+			 * address of the specific single page we want.
+			 */
+			unsigned long rpnmask = (1ul << shift) - PAGE_SIZE;
+			pte = __pte(pte_val(pte) | (hva & rpnmask));
 		}
 	}
 
+	pte = __pte(pte_val(pte) | _PAGE_EXEC | _PAGE_ACCESSED);
+	if (writing || upgrade_write) {
+		if (pte_val(pte) & _PAGE_WRITE)
+			pte = __pte(pte_val(pte) | _PAGE_DIRTY);
+	} else {
+		pte = __pte(pte_val(pte) & ~(_PAGE_WRITE | _PAGE_DIRTY));
+	}
+
 	/* Allocate space in the tree and write the PTE */
 	ret = kvmppc_create_pte(kvm, pte, gpa, level, mmu_seq);
 
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Jann Horn @ 2018-10-05 14:02 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, muriloo
In-Reply-To: <20181005132123.6038-1-mpe@ellerman.id.au>

On Fri, Oct 5, 2018 at 3:21 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
>
>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:

This fix looks good to me.

In the long term, I think it is somewhat awkward to use
probe_kernel_address(), which uses set_fs(KERNEL_DS), when you
actually just want to access userspace memory. It might make sense to
provide a better helper for explicitly accessing memory with USER_DS.

^ permalink raw reply

* Re: [RFC PATCH 0/9] patchable function pointers for pluggable crypto routines
From: Jason A. Donenfeld @ 2018-10-05 13:37 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	Paul Mackerras, Herbert Xu, Richard Weinberger, Eric Biggers,
	Ingo Molnar, Kees Cook, Arnd Bergmann, Andy Lutomirski,
	Thomas Gleixner, linux-arm-kernel, Martin K. Petersen,
	Greg Kroah-Hartman, linux-kernel, linux-crypto, Andrew Morton,
	linuxppc-dev, David S. Miller
In-Reply-To: <20181005081333.15018-1-ard.biesheuvel@linaro.org>

Hi Ard,

On Fri, Oct 5, 2018 at 10:13 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> At the moment, the Zinc library [1] is being proposed as a solution for that,
> and while it does address the usability problems, it does a lot more than
> that, and what we end up with is a lot less flexible than what we have now.
>
> Currently, arch specific implementations (based on SIMD or optimized
> assembler) live in arch/*/crypto, where each sub-community is in charge
> of its own specialized versions of the various primitives (although still
> under the purview of the crypto maintainer). Any proposal to change this
> model should be judged on its own merit, and not blindly accepted as the
> fallout of cleaning up some library code.
>
> Also, Zinc removes the possibility to plug different versions of a routine,
> and instead, keeps all versions in the same module. Currently, the kernel's
> module support permits user land to take charge of the policies that decide
> which version to use in which context (by loading or blacklisting modules).

I think this explanation misunderstands many of the design goals of Zinc and
also points to why going your direction instead is a bad idea that will cause
even more problems down the road.

Zinc does several important things:

- Introduces direct C function calls throughout, as a central way of being
  implemented and as a central way of being used by consumers of the
  API. This has various obvious benefits for the consumers of the API,
  but it also has big benefits for the developers of the library as
  well. Namely, it keeps the relationship between different parts
  extremely clear and direct. It's an explicit choice for simplicity.
  And by being the simpler and more direct solution, it also gives gcc
  an important opportunity to optimize and inline.

- Reorganizes crypto routines so that they're grouped together by
  primitive. This again leads to a much simpler design and layout,
  making it more obvious what's happening, and making things generally
  cleaner. It is not only useful and clearer for developers, but it
  also makes contributors and auditors more easily aware of what
  implementations are available.

- Has higher standards for code and implementations that are introduced.
  Zinc prefers code that has been formally verified, that has been in
  widespread usage and has received many eyeballs and fuzzing hours,
  that has been fuzzed extensively, that is simple in design, that comes
  from well-known high-quality authors -- in roughly but not precisely
  that order of preference. That's a bit different from the current
  practices of the existing crypto API.

- Has a simpler mechanism that is just as effective for choosing
  available implementations. This is, again, more obvious and direct
  than the present crypto API's module approach, leads to smaller code
  size, and has the potential of being just as flexible with the
  inevitable desire for nobs, adjustable from userspace, from
  kernelspace, or from elsewhere.

- Is designed to promote collaboration with the larger cryptography
  community and with academia, which will yield better implementations
  and for assurance.

- Can easily be extracted to userspace libraries (perhaps a future
  libzinc could be easily based on it), which makes testing and fuzzing
  using tools like libfuzzer and afl more accessible.

- Has faster implementations than the current crypto API.

- Has, again, a very strong focus on being simple and minimal, as
  opposed to bloated and complicated, so that it's actually possible to
  understand and audit the library.

Therefore, I think this patch goes in exactly the wrong direction. I
mean, if you want to introduce dynamic patching as a means for making
the crypto API's dynamic dispatch stuff not as slow in a post-spectre
world, sure, go for it; that may very well be a good idea. But
presenting it as an alternative to Zinc very widely misses the point and
serves to prolong a series of bad design choices, which are now able to
be rectified by putting energy into Zinc instead.

Jason

^ permalink raw reply

* Re: [PATCH 33/36] dt-bindings: arm: Convert Tegra board/soc bindings to json-schema
From: Rob Herring @ 2018-10-05 23:36 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: Mark Rutland, devicetree, Kumar Gala, Grant Likely, Arnd Bergmann,
	Tom Rini, Frank Rowand, Linus Walleij, Pantelis Antoniou,
	linux-kernel@vger.kernel.org, Jon Hunter, linux-tegra, Mark Brown,
	Thierry Reding, Jonathan Cameron, Olof Johansson,
	Geert Uytterhoeven, Bjorn Andersson, linuxppc-dev,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1538777991.28477.6.camel@toradex.com>

On Fri, Oct 5, 2018 at 5:20 PM Marcel Ziswiler
<marcel.ziswiler@toradex.com> wrote:
>
> Hi Rob
>
> On Fri, 2018-10-05 at 11:58 -0500, Rob Herring wrote:
> > Convert Tegra SoC bindings to DT schema format using json-schema.
> >

[...]

> Are you aware that -next already features a few updating commits
> thereof from around the beginning of September one of which even bears
> your reviewed-by.

I'm not targeting 4.20, but rather early in the 4.21 cycle. So I'll be
rebasing on rc1 and will update with any changes. I've been doing that
a couple of cycles already.

Rob

^ permalink raw reply

* Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
From: Andrew Morton @ 2018-10-05 22:19 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: linux-mips, linux-m68k, Michal Hocko, linux-ia64, Catalin Marinas,
	linux-mm, Richard Weinberger, Russell King, Ingo Molnar,
	Geert Uytterhoeven, Matt Turner, linux-um, Thomas Gleixner,
	Guan Xuetao, linux-arm-kernel, Chris Zankel, Michal Simek,
	Tony Luck, linux-kernel, Paul Burton, linux-alpha, linuxppc-dev
In-Reply-To: <1538687224-17535-1-git-send-email-rppt@linux.vnet.ibm.com>

On Fri,  5 Oct 2018 00:07:04 +0300 Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:

> When a memblock allocation APIs are called with align = 0, the alignment is
> implicitly set to SMP_CACHE_BYTES.
> 
> Replace all such uses of memblock APIs with the 'align' parameter explicitly
> set to SMP_CACHE_BYTES and stop implicit alignment assignment in the
> memblock internal allocation functions.
> 
> For the case when memblock APIs are used via helper functions, e.g. like
> iommu_arena_new_node() in Alpha, the helper functions were detected with
> Coccinelle's help and then manually examined and updated where appropriate.
> 
> ...
>
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1298,9 +1298,6 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
>  {
>  	phys_addr_t found;
>  
> -	if (!align)
> -		align = SMP_CACHE_BYTES;
> -

Can we add a WARN_ON_ONCE(!align) here?  To catch unconverted code
which sneaks in later on.


^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Ard Biesheuvel @ 2018-10-05 19:13 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	paulus, Herbert Xu, richard, Eric Biggers, mingo, Kees Cook,
	Arnd Bergmann, Andrew Lutomirski, Thomas Gleixner,
	linux-arm-kernel, Martin K. Petersen, Greg Kroah-Hartman, LKML,
	Linux Crypto Mailing List, Andrew Morton, linuxppc-dev,
	David Miller
In-Reply-To: <CAHmME9pcQF=FvKRuxnqrJ5R1pvtUVbx3DXvXiUX5HywS2Xgj3A@mail.gmail.com>



> On 5 Oct 2018, at 20:28, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> Hey Andy,
> 
>> On Fri, Oct 5, 2018 at 7:44 PM Andy Lutomirski <luto@kernel.org> wrote:
>> I *think* the only change to Zinc per se would be that the calls like
>> chacha20_simd() would be static calls or patchable functions or
>> whatever we want to call them.  And there could be a debugfs to
>> override the default selection.
> 
> Yea, right, exactly. It turns out this is really easy to do with the
> way it's structured now. I'd actually experimented considerably with
> using the static keys a while back, but couldn't find any performance
> difference on any platform at all (four ARM microarchitectures, three
> MIPS, various random intel, an old powerpc), so went with the simplest
> solution. But we can certainly play with more elaborate patching
> mechanisms later on and see how those turn out. Also, even with the
> simple bools as we have now, it's quite easy to make all the
> parameters toggle-able.
> 
>> Ard, I don't think that sticking this in udev rules makes sense.  The
>> kernel has bascially complete information as to what the right choice
>> is, and that will change over time as the implementation gets tuned,
>> and the udev rules will never get updated in sync.
> 
> Yes, I agree with this.
> 
> 

I am not referring to udev rules. I just mean the current way that udev autoloads modules based on CPU features.

^ permalink raw reply

* Re: [PATCH 09/16] of: overlay: validate overlay properties #address-cells and #size-cells
From: Frank Rowand @ 2018-10-05 19:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
	linux-kernel@vger.kernel.org, Moritz Fischer, Paul Mackerras,
	linuxppc-dev
In-Reply-To: <CAL_Jsq+53znm=LHxH8bZZJ2wN4uUez8D7E2V03+Zc9nvYBNjrg@mail.gmail.com>

On 10/05/18 12:04, Rob Herring wrote:
> On Fri, Oct 5, 2018 at 1:53 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 10/05/18 08:07, Rob Herring wrote:
>>> On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
>>>>
>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>
>>>> If overlay properties #address-cells or #size-cells are already in
>>>> the live devicetree for any given node, then the values in the
>>>> overlay must match the values in the live tree.
>>>>
>>>> If the properties are already in the live tree then there is no
>>>> need to create a changeset entry to add them since they must
>>>> have the same value.  This reduces the memory used by the
>>>> changeset and eliminates a possible memory leak.  This is
>>>> verified by 12 fewer warnings during the devicetree unittest,
>>>> as the possible memory leak warnings about #address-cells and
>>>
>>> and...?
>>
>> #size-cells no longer occur.
>>
>> (Thanks for catching that.)
>>
>>
>>>>
>>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>>> ---
>>>>  drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 35 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 29c33a5c533f..e6fb3ffe9d93 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
>>>>   * @target may be either in the live devicetree or in a new subtree that
>>>>   * is contained in the changeset.
>>>>   *
>>>> - * Some special properties are not updated (no error returned).
>>>> + * Some special properties are not added or updated (no error returned):
>>>> + * "name", "phandle", "linux,phandle".
>>>> + *
>>>> + * Properties "#address-cells" and "#size-cells" are not updated if they
>>>> + * are already in the live tree, but if present in the live tree, the values
>>>> + * in the overlay must match the values in the live tree.
>>>
>>> Perhaps this should be generalized to apply to any property? We can't
>>> really deal with property values changing on the fly anyways.
>>
>> That is a bigger discussion.  I'd prefer to not hold up this series for that
>> question to be resolved.  It will be easy enough to generalize in an add-on
>> patch later.
> 
> Fair enough.
> 
>>>> +               if (prop->length != 4 || new_prop->length != 4 ||
>>>> +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
>>>
>>> Technically these are __be32 types. This could use a helper (of_prop_val_eq).
>>
>> These are in a unpacked form, so cpu byte order, not FDT byte order.
> 
> You sure about that? Unpacking doesn't change the order. It can't
> because the type is unknown. The value of 'value' is the address of
> the data in the FDT.

Aargh.  You are totally right.


>>> I'm not sure we really need to validate the length here as dtc does
>>> that (but yes, not everything is from dtc).
>>
>> Since I'm accessing 4 bytes of the values, I need to be sure the lengths
>> are at least 4.  For #address-cells and #size-cells the property is
>> specified as four bytes, so I could simplify the code for the specific case.
>>
>> If this gets extended to any arbitrary property then a new of_prop_val_eq()
>> would check that the lengths are equal and the values (of size length) are
>> also equal.
> 
> Right, that's what I was thinking. Check lengths are equal and then
> you can just do a memcmp().

Based on all of this it seems better that I create of_prop_val_eq(), as you
suggested, and change to use that.


> 
> Rob
> 


^ permalink raw reply

* Re: [PATCH 15/16] of: unittest: initialize args before calling of_irq_parse_one()
From: Frank Rowand @ 2018-10-05 19:05 UTC (permalink / raw)
  To: Guenter Roeck, Rob Herring, Pantelis Antoniou, Michael Ellerman,
	Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
  Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel, Frank Rowand
In-Reply-To: <7fc4b9da-5fdf-11c9-e168-ccb2d960adea@roeck-us.net>

On 10/05/18 06:26, Guenter Roeck wrote:
> On 10/04/2018 09:12 PM, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np.  Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> The same problem exists when of_parse_phandle_with_args() reports an error.

Thanks, I'll add a fix for that.

-Frank

> 
> Guenter
> 
>> ---
>>   drivers/of/unittest.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 6d80f474c8f2..b61a33f30a56 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
>>       for (i = 0; i < 4; i++) {
>>           bool passed = true;
>>   -        args.args_count = 0;
>> +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             passed &= !rc;
>> @@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
>>       for (i = 0; i < 4; i++) {
>>           bool passed = true;
>>   -        args.args_count = 0;
>> +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             /* Test the values from tests-phandle.dtsi */
>> @@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
>>       for (i = 0; i < 7; i++) {
>>           bool passed = true;
>>   +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             /* Test the values from tests-phandle.dtsi */
>>
> 
> 


^ permalink raw reply

* Re: [PATCH 09/16] of: overlay: validate overlay properties #address-cells and #size-cells
From: Rob Herring @ 2018-10-05 19:04 UTC (permalink / raw)
  To: Frank Rowand
  Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
	linux-kernel@vger.kernel.org, Moritz Fischer, Paul Mackerras,
	linuxppc-dev
In-Reply-To: <f39a84d3-f706-d31e-db22-4e3ebc72c447@gmail.com>

On Fri, Oct 5, 2018 at 1:53 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 10/05/18 08:07, Rob Herring wrote:
> > On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
> >>
> >> From: Frank Rowand <frank.rowand@sony.com>
> >>
> >> If overlay properties #address-cells or #size-cells are already in
> >> the live devicetree for any given node, then the values in the
> >> overlay must match the values in the live tree.
> >>
> >> If the properties are already in the live tree then there is no
> >> need to create a changeset entry to add them since they must
> >> have the same value.  This reduces the memory used by the
> >> changeset and eliminates a possible memory leak.  This is
> >> verified by 12 fewer warnings during the devicetree unittest,
> >> as the possible memory leak warnings about #address-cells and
> >
> > and...?
>
> #size-cells no longer occur.
>
> (Thanks for catching that.)
>
>
> >>
> >> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> >> ---
> >>  drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index 29c33a5c533f..e6fb3ffe9d93 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> >>   * @target may be either in the live devicetree or in a new subtree that
> >>   * is contained in the changeset.
> >>   *
> >> - * Some special properties are not updated (no error returned).
> >> + * Some special properties are not added or updated (no error returned):
> >> + * "name", "phandle", "linux,phandle".
> >> + *
> >> + * Properties "#address-cells" and "#size-cells" are not updated if they
> >> + * are already in the live tree, but if present in the live tree, the values
> >> + * in the overlay must match the values in the live tree.
> >
> > Perhaps this should be generalized to apply to any property? We can't
> > really deal with property values changing on the fly anyways.
>
> That is a bigger discussion.  I'd prefer to not hold up this series for that
> question to be resolved.  It will be easy enough to generalize in an add-on
> patch later.

Fair enough.

> >> +               if (prop->length != 4 || new_prop->length != 4 ||
> >> +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
> >
> > Technically these are __be32 types. This could use a helper (of_prop_val_eq).
>
> These are in a unpacked form, so cpu byte order, not FDT byte order.

You sure about that? Unpacking doesn't change the order. It can't
because the type is unknown. The value of 'value' is the address of
the data in the FDT.

> > I'm not sure we really need to validate the length here as dtc does
> > that (but yes, not everything is from dtc).
>
> Since I'm accessing 4 bytes of the values, I need to be sure the lengths
> are at least 4.  For #address-cells and #size-cells the property is
> specified as four bytes, so I could simplify the code for the specific case.
>
> If this gets extended to any arbitrary property then a new of_prop_val_eq()
> would check that the lengths are equal and the values (of size length) are
> also equal.

Right, that's what I was thinking. Check lengths are equal and then
you can just do a memcmp().

Rob

^ permalink raw reply

* Re: [PATCH 15/16] of: unittest: initialize args before calling of_irq_parse_one()
From: Frank Rowand @ 2018-10-05 19:04 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Alan Tull, linux-fpga,
	Pantelis Antoniou, linux-kernel@vger.kernel.org, Moritz Fischer,
	Paul Mackerras, linuxppc-dev, Guenter Roeck
In-Reply-To: <CAL_Jsq+dqRr54_WAdBQ88sJytAuLL=U9kF_Xbj5rJv4PVn9gQg@mail.gmail.com>

On 10/05/18 07:53, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np.  Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  drivers/of/unittest.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Does this need to be part of this series?

I do not know if it could be independently applied before the
rest of the series (I have not tested that).  I included it
in the series because the series has so many other changes to
unittest.c.

If you want me to break this out, I will remove it from this
series and resend it after the rest of the series has been
pulled to mainline (and rebase on the new mainline).

This patch is not fixing a known failure case - the current
test data does not trigger the problem.  The recent patch
from  Guenter that you already accepted fixes the known failure
case, so this patch is not urgent.  The same is true about the
other cases Guenter pointed out that this patch should fix.

-Frank

^ permalink raw reply

* Re: [PATCH 09/16] of: overlay: validate overlay properties #address-cells and #size-cells
From: Frank Rowand @ 2018-10-05 18:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
	linux-kernel@vger.kernel.org, Moritz Fischer, Paul Mackerras,
	linuxppc-dev
In-Reply-To: <CAL_JsqKK3f4W1DdPkzNed0=tx5Oxu9w=RKs8PnaXMfuShBpV1A@mail.gmail.com>

On 10/05/18 08:07, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> If overlay properties #address-cells or #size-cells are already in
>> the live devicetree for any given node, then the values in the
>> overlay must match the values in the live tree.
>>
>> If the properties are already in the live tree then there is no
>> need to create a changeset entry to add them since they must
>> have the same value.  This reduces the memory used by the
>> changeset and eliminates a possible memory leak.  This is
>> verified by 12 fewer warnings during the devicetree unittest,
>> as the possible memory leak warnings about #address-cells and
> 
> and...?

#size-cells no longer occur.

(Thanks for catching that.)


>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 29c33a5c533f..e6fb3ffe9d93 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
>>   * @target may be either in the live devicetree or in a new subtree that
>>   * is contained in the changeset.
>>   *
>> - * Some special properties are not updated (no error returned).
>> + * Some special properties are not added or updated (no error returned):
>> + * "name", "phandle", "linux,phandle".
>> + *
>> + * Properties "#address-cells" and "#size-cells" are not updated if they
>> + * are already in the live tree, but if present in the live tree, the values
>> + * in the overlay must match the values in the live tree.
> 
> Perhaps this should be generalized to apply to any property? We can't
> really deal with property values changing on the fly anyways.

That is a bigger discussion.  I'd prefer to not hold up this series for that
question to be resolved.  It will be easy enough to generalize in an add-on
patch later.


>>   *
>>   * Update of property in symbols node is not allowed.
>>   *
>> @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>  {
>>         struct property *new_prop = NULL, *prop;
>>         int ret = 0;
>> +       bool check_for_non_overlay_node = false;
>>
>>         if (!of_prop_cmp(overlay_prop->name, "name") ||
>>             !of_prop_cmp(overlay_prop->name, "phandle") ||
>> @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>>         if (!new_prop)
>>                 return -ENOMEM;
>>
>> -       if (!prop)
>> +       if (!prop) {
>> +
> 
> Remove the extra blank lines.

Will do.

> 
>> +               check_for_non_overlay_node = true;
>>                 ret = of_changeset_add_property(&ovcs->cset, target->np,
>>                                                 new_prop);
>> -       else
>> +
>> +       } else if (!of_prop_cmp(prop->name, "#address-cells")) {
>> +
>> +               if (prop->length != 4 || new_prop->length != 4 ||
>> +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
> 
> Technically these are __be32 types. This could use a helper (of_prop_val_eq).

These are in a unpacked form, so cpu byte order, not FDT byte order.

> 
> I'm not sure we really need to validate the length here as dtc does
> that (but yes, not everything is from dtc).

Since I'm accessing 4 bytes of the values, I need to be sure the lengths
are at least 4.  For #address-cells and #size-cells the property is
specified as four bytes, so I could simplify the code for the specific case.

If this gets extended to any arbitrary property then a new of_prop_val_eq()
would check that the lengths are equal and the values (of size length) are
also equal.


> 
>> +                       pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
>> +                              target->np);
>> +
>> +       } else if (!of_prop_cmp(prop->name, "#size-cells")) {
>> +
>> +               if (prop->length != 4 || new_prop->length != 4 ||
>> +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
>> +                       pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
>> +                              target->np);
>> +
>> +       } else {
>> +
>> +               check_for_non_overlay_node = true;
>>                 ret = of_changeset_update_property(&ovcs->cset, target->np,
>>                                                    new_prop);
>>
>> +       }
>> +
>> +       if (check_for_non_overlay_node &&
>> +           !of_node_check_flag(target->np, OF_OVERLAY))
>> +               pr_err("WARNING: %s(), memory leak will occur if overlay removed.  Property: %pOF/%s\n",
>> +                      __func__, target->np, new_prop->name);
>> +
>>         if (ret) {
>>                 kfree(new_prop->name);
>>                 kfree(new_prop->value);
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>
> 


^ permalink raw reply

* Re: [PATCH 19/36] dt-bindings: arm: Convert Atmel board/soc bindings to json-schema
From: Rob Herring @ 2018-10-05 18:32 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Mark Rutland, devicetree, Kumar Gala, Grant Likely, Arnd Bergmann,
	Tom Rini, Frank Rowand, Linus Walleij, Nicolas Ferre,
	Pantelis Antoniou, linux-kernel@vger.kernel.org, Bjorn Andersson,
	Mark Brown, Geert Uytterhoeven, Jonathan Cameron, Olof Johansson,
	linuxppc-dev,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20181005180725.GH5626@piout.net>

On Fri, Oct 5, 2018 at 1:07 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Hello,
>
> On 05/10/2018 11:58:31-0500, Rob Herring wrote:
> > diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.yaml b/Documentation/devicetree/bindings/arm/atmel-at91.yaml
> > new file mode 100644
> > index 000000000000..f788315b94fa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/atmel-at91.yaml
> > @@ -0,0 +1,132 @@
> > +# SPDX-License-Identifier: None
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/bindings/arm/atmel-at91.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Atmel AT91 device tree bindings.
> > +
> > +maintainers:
> > +  - Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > +  - Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>
> Jean-Christophe has not been active for years, I'd mention Ludovic
> instead.

Will update. I generated these out of git log. I didn't use
get_maintainers.pl because it seems lots of files don't have
maintainers listed (other than Mark and me) and I didn't want to be
it.

>
> > +description: |
> > +  Boards with a SoC of the Atmel AT91 or SMART family shall have the following
> > +
> > +properties:
> > +  $nodename:
> > +    const: '/'
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - const: atmel,at91rm9200
> > +      - items:
> > +          - enum:
> > +              - olimex,sam9-l9260
> > +          - enum:
> > +              - atmel,at91sam9260
> > +              - atmel,at91sam9261
> > +              - atmel,at91sam9263
> > +              - atmel,at91sam9g20
> > +              - atmel,at91sam9g45
> > +              - atmel,at91sam9n12
> > +              - atmel,at91sam9rl
> > +              - atmel,at91sam9xe
> > +          - const: atmel,at91sam9
> > +
> > +      - items:
> > +          - enum:
> > +              - atmel,at91sam9g15
> > +              - atmel,at91sam9g25
> > +              - atmel,at91sam9g35
> > +              - atmel,at91sam9x25
> > +              - atmel,at91sam9x35
> > +          - const: atmel,at91sam9x5
> > +          - const: atmel,at91sam9
> > +
> > +      - items:
> > +          - const: atmel,sama5d27
> > +          - const: atmel,sama5d2
> > +          - const: atmel,sama5
> > +
> > +      - description: Nattis v2 board with Natte v2 power board
> > +        items:
> > +          - const: axentia,nattis-2
> > +          - const: axentia,natte-2
> > +          - const: axentia,linea
>
> Shouldn't we have the board specific compatibles in a separate file to
> avoid mixing everything with the SoC compatibles?

You can't validate it that way. I have to say "must be compatible A,
B, C and in that order" and you can't if A, B, and C are in different
files. We could do board vendor files, but then we have to duplicate
the SoC compatibles. I don't think there's any board vendor with
enough boards to justify that. The only place I've found that the
compatible lists get kind of messy is when platforms have a variable
number of compatible strings.

We generally have not split things this way for most platforms except
i.MX which this series changes. Looks like I forgot to remove the
axentia.txt for Atmel.

Rob

^ permalink raw reply

* Re: [PATCH 19/36] dt-bindings: arm: Convert Atmel board/soc bindings to json-schema
From: Alexandre Belloni @ 2018-10-05 18:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, Kumar Gala, Grant Likely, Arnd Bergmann,
	Tom Rini, Frank Rowand, Linus Walleij, Nicolas Ferre,
	Pantelis Antoniou, linux-kernel, Bjorn Andersson, Mark Brown,
	Geert Uytterhoeven, Jonathan Cameron, Olof Johansson,
	linuxppc-dev, linux-arm-kernel
In-Reply-To: <20181005165848.3474-20-robh@kernel.org>

Hello,

On 05/10/2018 11:58:31-0500, Rob Herring wrote:
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.yaml b/Documentation/devicetree/bindings/arm/atmel-at91.yaml
> new file mode 100644
> index 000000000000..f788315b94fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.yaml
> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: None
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/arm/atmel-at91.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel AT91 device tree bindings.
> +
> +maintainers:
> +  - Alexandre Belloni <alexandre.belloni@free-electrons.com>
> +  - Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Jean-Christophe has not been active for years, I'd mention Ludovic
instead.

> +description: |
> +  Boards with a SoC of the Atmel AT91 or SMART family shall have the following
> +
> +properties:
> +  $nodename:
> +    const: '/'
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: atmel,at91rm9200
> +      - items:
> +          - enum:
> +              - olimex,sam9-l9260
> +          - enum:
> +              - atmel,at91sam9260
> +              - atmel,at91sam9261
> +              - atmel,at91sam9263
> +              - atmel,at91sam9g20
> +              - atmel,at91sam9g45
> +              - atmel,at91sam9n12
> +              - atmel,at91sam9rl
> +              - atmel,at91sam9xe
> +          - const: atmel,at91sam9
> +
> +      - items:
> +          - enum:
> +              - atmel,at91sam9g15
> +              - atmel,at91sam9g25
> +              - atmel,at91sam9g35
> +              - atmel,at91sam9x25
> +              - atmel,at91sam9x35
> +          - const: atmel,at91sam9x5
> +          - const: atmel,at91sam9
> +
> +      - items:
> +          - const: atmel,sama5d27
> +          - const: atmel,sama5d2
> +          - const: atmel,sama5
> +
> +      - description: Nattis v2 board with Natte v2 power board
> +        items:
> +          - const: axentia,nattis-2
> +          - const: axentia,natte-2
> +          - const: axentia,linea

Shouldn't we have the board specific compatibles in a separate file to
avoid mixing everything with the SoC compatibles?


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply

* Re: [RFC PATCH 0/9] patchable function pointers for pluggable crypto routines
From: Andy Lutomirski @ 2018-10-05 18:00 UTC (permalink / raw)
  To: Ard Biesheuvel, Josh Poimboeuf
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Andrew Lutomirski, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Greg KH, LKML, Linux Crypto Mailing List,
	Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAKv+Gu_fFvoXmWscnx=dF7ovc2qXJgBnDte3Z8Co9Y4TzDfwbQ@mail.gmail.com>

On Fri, Oct 5, 2018 at 10:28 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On 5 October 2018 at 19:26, Andy Lutomirski <luto@kernel.org> wrote:
> > On Fri, Oct 5, 2018 at 10:15 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On 5 October 2018 at 15:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> ...
> >> > Therefore, I think this patch goes in exactly the wrong direction. I
> >> > mean, if you want to introduce dynamic patching as a means for making
> >> > the crypto API's dynamic dispatch stuff not as slow in a post-spectre
> >> > world, sure, go for it; that may very well be a good idea. But
> >> > presenting it as an alternative to Zinc very widely misses the point and
> >> > serves to prolong a series of bad design choices, which are now able to
> >> > be rectified by putting energy into Zinc instead.
> >> >
> >>
> >> This series has nothing to do with dynamic dispatch: the call sites
> >> call crypto functions using ordinary function calls (although my
> >> example uses CRC-T10DIF), and these calls are redirected via what is
> >> essentially a PLT entry, so that we can supsersede those routines at
> >> runtime.
> >
> > If you really want to do it PLT-style, then just do:
> >
> > extern void whatever_func(args);
> >
> > Call it like:
> > whatever_func(args here);
> >
> > And rig up something to emit asm like:
> >
> > GLOBAL(whatever_func)
> >   jmpq default_whatever_func
> > ENDPROC(whatever_func)
> >
> > Architectures without support can instead do:
> >
> > void whatever_func(args)
> > {
> >   READ_ONCE(patchable_function_struct_for_whatever_func->ptr)(args);
> > }
> >
> > and patch the asm function for basic support.  It will be slower than
> > necessary, but maybe the relocation trick could be used on top of this
> > to redirect the call to whatever_func directly to the target for
> > architectures that want to squeeze out the last bit of performance.
> > This might actually be the best of all worlds: easy implementation on
> > all architectures, no inline asm, and the totally non-magical version
> > works with okay performance.
> >
> > (Is this what your code is doing?  I admit I didn't follow all the way
> > through all the macros.)
>
> Basically

Adding Josh Poimboeuf.

Here's a sketch of how this could work for better performance.  For a
static call "foo" that returns void and takes no arguments, the
generic implementation does something like this:

extern void foo(void);

struct static_call {
  void (*target)(void);

  /* arch-specific part containing an array of struct static_call_site */
};

void foo(void)
{
  READ_ONCE(__static_call_foo->target)();
}

Arch code overrides it to:

GLOBAL(foo)
  jmpq *__static_call_foo(%rip)
ENDPROC(foo)

and some extra asm to emit a static_call_site object saying that the
address "foo" is a jmp/call instruction where the operand is at offset
1 into the instruction.  (Or whatever the offset is.)

The patch code is like:

void set_static_call(struct static_call *call, void *target)
{
  /* take a spinlock? */
  WRITE_ONCE(call->target, target);
  arch_set_static_call(call, target);
}

and the arch code patches the call site if needed.

On x86, an even better implementation would have objtool make a bunch
of additional static_call_site objects for each call to foo, and
arch_set_static_call() would update all of them, too.  Using
text_poke_bp() if needed, and "if needed" can maybe be clever and
check the alignment of the instruction.  I admit that I never actually
remember the full rules for atomically patching an instruction on x86
SMP.  (Hmm.  This will be really epically slow.  Maybe we don't care.
Or we could finally optimize text_poke, etc to take a list of pokes to
do and do them as a batch.  But that's not a prerequisite for the rest
of this.)

What do you all think?

^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Andy Lutomirski @ 2018-10-05 17:44 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	Paul Mackerras, Herbert Xu, Richard Weinberger, Eric Biggers,
	Ingo Molnar, Kees Cook, Arnd Bergmann, Andrew Lutomirski,
	Thomas Gleixner, linux-arm-kernel, Martin K. Petersen,
	Ard Biesheuvel, Greg KH, LKML, Linux Crypto Mailing List,
	Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAHmME9pgJ_2GGDUVsbQXcNRfh3yACqFG+5uemFRMZkb8LdH95g@mail.gmail.com>

On Fri, Oct 5, 2018 at 10:38 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Fri, Oct 5, 2018 at 7:29 PM Andy Lutomirski <luto@kernel.org> wrote:
> > (None of this is to say that I disagree with Jason, though -- I'm not
> > entirely convinced that this makes sense for Zinc.  But maybe it can
> > be done in a way that makes everyone happy.)
>
> Zinc indeed will continue to push in the simpler and more minimal
> direction. Down the line I'm open to trying and benching a few
> different ways of going about it with dynamic patching -- something
> that will be pretty easy to experiment with given the lean structure
> of Zinc -- but for the initial merge I intend to do it the way it is,
> which is super fast and pretty straightforward to follow.
>

I *think* the only change to Zinc per se would be that the calls like
chacha20_simd() would be static calls or patchable functions or
whatever we want to call them.  And there could be a debugfs to
override the default selection.

Ard, I don't think that sticking this in udev rules makes sense.  The
kernel has bascially complete information as to what the right choice
is, and that will change over time as the implementation gets tuned,
and the udev rules will never get updated in sync.

^ permalink raw reply

* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Murilo Opsfelder Araujo @ 2018-10-05 17:29 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, jannh
In-Reply-To: <20181005132123.6038-1-mpe@ellerman.id.au>

On Fri, Oct 05, 2018 at 11:21:23PM +1000, Michael Ellerman wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
>
>   bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
>
>   bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>   bad-bctr[2969]: Bad NIP, not dumping instructions.
>
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>

Thank you all!

> ---
>  arch/powerpc/kernel/process.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..bb6ac471a784 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>
>  	pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>
> +	/*
> +	 * Make sure the NIP points at userspace, not kernel text/data or
> +	 * elsewhere.
> +	 */
> +	if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> +		pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> +			current->comm, current->pid);
> +		return;
> +	}
> +
>  	pr_info("%s[%d]: code: ", current->comm, current->pid);
>
>  	for (i = 0; i < instructions_to_print; i++) {
> --
> 2.17.1
>

--
Murilo


^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Andy Lutomirski @ 2018-10-05 17:28 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Andrew Lutomirski, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Greg KH, LKML, Linux Crypto Mailing List,
	Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAKv+Gu8eHPEsUbkhN=61z9FvhdCo2e3VOdemp4g0bzdO1EJFYQ@mail.gmail.com>

On Fri, Oct 5, 2018 at 10:23 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On 5 October 2018 at 19:20, Andy Lutomirski <luto@kernel.org> wrote:
> > On Fri, Oct 5, 2018 at 10:11 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On 5 October 2018 at 18:58, Andy Lutomirski <luto@kernel.org> wrote:
> >> > On Fri, Oct 5, 2018 at 8:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> >>
> >> >> On 5 October 2018 at 17:08, Andy Lutomirski <luto@amacapital.net> wrote:
> >> >> >
> >> >> >
> >> >> >> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> >> >>
> >> >> >>> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote:
> >> >> >>> diff --git a/include/linux/ffp.h b/include/linux/ffp.h
> >> >> >>> new file mode 100644
> >> >> >>> index 000000000000..8fc3b4c9b38f
> >> >> >>> --- /dev/null
> >> >> >>> +++ b/include/linux/ffp.h
> >> >> >>> @@ -0,0 +1,43 @@
> >> >> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >> >> >>> +
> >> >> >>> +#ifndef __LINUX_FFP_H
> >> >> >>> +#define __LINUX_FFP_H
> >> >> >>> +
> >> >> >>> +#include <linux/types.h>
> >> >> >>> +#include <linux/compiler.h>
> >> >> >>> +
> >> >> >>> +#ifdef CONFIG_HAVE_ARCH_FFP
> >> >> >>> +#include <asm/ffp.h>
> >> >> >>> +#else
> >> >> >>> +
> >> >> >>> +struct ffp {
> >> >> >>> +    void    (**fn)(void);
> >> >> >>> +    void    (*default_fn)(void);
> >> >> >>> +};
> >> >> >>> +
> >> >> >>> +#define DECLARE_FFP(_fn, _def)                        \
> >> >> >>> +    extern typeof(_def) *_fn;                    \
> >> >> >>> +    extern struct ffp const __ffp_ ## _fn
> >> >> >>> +
> >> >> >>> +#define DEFINE_FFP(_fn, _def)                        \
> >> >> >>> +    typeof(_def) *_fn = &_def;                    \
> >> >> >>> +    struct ffp const __ffp_ ## _fn                    \
> >> >> >>> +        = { (void(**)(void))&_fn, (void(*)(void))&_def };    \
> >> >> >>> +    EXPORT_SYMBOL(__ffp_ ## _fn)
> >> >> >>> +
> >> >> >>> +static inline void ffp_set_target(const struct ffp *m, void *new_fn)
> >> >> >>> +{
> >> >> >>> +    WRITE_ONCE(*m->fn, new_fn);
> >> >> >>> +}
> >> >> >>> +
> >> >> >>> +static inline void ffp_reset_target(const struct ffp *m)
> >> >> >>> +{
> >> >> >>> +    WRITE_ONCE(*m->fn, m->default_fn);
> >> >> >>> +}
> >> >> >>> +
> >> >> >>> +#endif
> >> >> >>> +
> >> >> >>> +#define SET_FFP(_fn, _new)    ffp_set_target(&__ffp_ ## _fn, _new)
> >> >> >>> +#define RESET_FFP(_fn)        ffp_reset_target(&__ffp_ ## _fn)
> >> >> >>> +
> >> >> >>> +#endif
> >> >> >>
> >> >> >> I don't understand this interface. There is no wrapper for the call
> >> >> >> site, so how are we going to patch all call-sites when you update the
> >> >> >> target?
> >> >> >
> >> >> > I’m also confused.
> >> >> >
> >> >> > Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated.
> >> >> >
> >> >> > I’ve proposed a better way that should generate better code, be more portable, and be more maintainable.  It goes like this.
> >> >> >
> >> >> > To call the function, you literally just call  the default implementation.  It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation.
> >> >> >
> >> >> > To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment.  Or we ask gcc for an extension to align calls to a function.)
> >> >> >
> >> >> > Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module.
> >> >>
> >> >> Yeah nothing is ever simple on x86 :-(
> >> >>
> >> >> So are you saying the approach i use in patch #2 (which would
> >> >> translate to emitting a jmpq instruction pointing to the default
> >> >> implementation, and patching it at runtime to point elsewhere) would
> >> >> not fly on x86?
> >> >
> >> > After getting some more sleep, I'm obviously wrong.  The
> >> > text_poke_bp() mechanism will work.  It's just really slow.
> >> >
> >>
> >> OK
> >>
> >> > Let me try to summarize some of the issues.  First, when emitting
> >> > jumps and calls from inline asm on x86, there are a few considerations
> >> > that are annoying:
> >> >
> >> > 1. Following the x86_64 ABI calling conventions is basically
> >> > impossible.  x86_64 requires a 128-byte redzone and 16-byte stack
> >> > alignment.  After much discussion a while back, we decided that it was
> >> > flat-out impossible on current gcc to get the stack pointer aligned in
> >> > a known manner in an inline asm statement.  Instead, if we actually
> >> > need alignment, we need to align manually.  Fortunately, the kernel is
> >> > built with an override that forces only 8-byte alignment (on *most*
> >> > GCC versions).  But for crypto in particular, it sucks extra, since
> >> > the crypto code is basically the only thing in the kernel that
> >> > actually wants 16-byte alignment.  I don't think this is a huge
> >> > problem in practice, but it's annoying.  And the kernel is built
> >> > without a redzone.
> >> >
> >> > 2. On x86_64, depending on config, we either need frame pointers or
> >> > ORC.  ORC is no big deal -- it Just Works (tm).  Frame pointers need
> >> > extra asm hackery.  It's doable, but it's still annoying.
> >> >
> >> > 3. Actually getting the asm constraints right to do what a C
> >> > programmer expects is distinctly nontrivial.  I just fixed an
> >> > extremely longstanding bug in the vDSO code in which the asm
> >> > constraints for the syscall fallback were wrong in such a way that GCC
> >> > didn't notice that the fallback wrote to its output parameter.
> >> > Whoops.
> >> >
> >>
> >> OK, so the thing I am missing is why this all matters.
> >>
> >> Note that the compiler should take care of all of this. It emits a
> >> call a function with external linkage having prototype X, and all the
> >> inline asm does is emit a jmp to some function having that same
> >> prototype, either the default one or the one we patched in.
> >>
> >> Apologies if I am missing something obvious here: as you know, x86 is
> >> not my focus in general.
> >
> > The big issue that bothers me isn't the x86-ism so much as the nasty
> > interactions with the optimizer.  On x86, we have all of this working.
> > It's in arch/x86/include/asm/paravirt_types.h, and it looks roughly
> > like:
> >
> >                         asm volatile(pre                                \
> >                                      paravirt_alt(PARAVIRT_CALL)        \
> >                                      post                               \
> >                                      : call_clbr, ASM_CALL_CONSTRAINT   \
> >                                      : paravirt_type(op),               \
> >                                        paravirt_clobber(clbr),          \
> >                                        ##__VA_ARGS__                    \
> >                                      : "memory", "cc" extra_clbr);      \
> >
> > With some extra magic for the constraints.  And I don't even think
> > this is strictly correct -- from very recent experience, telling the
> > compiler that "memory" is clobbered and that a bunch of arguments are
> > used as numeric inputs may not actually imply that the asm modifies
> > the target of pointer arguments.  Checks this lovely bug out:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b
> >
> > As far as I can tell, the whole PVOP infrastructure has the same bug.
> > And I don't see how to avoid it generically on x86 or any other
> > architecture.  (PeterZ, am I wrong?  Are we really just getting lucky
> > that x86 pvop calls actually work?  Or do we not have enough of them
> > that take pointers as arguments for this to matter?)
> >
> > Plus, asm volatile ( ..., "memory" ) is a barrier and makes code
> > generation suck.
> >
> > Whereas, if we use my suggestion the semantics are precisely those of
> > any other C function call because, as far as GCC is concerned, it *is*
> > a C function call.  So the generated code *is* a function call.
> >
>
> But it is the *compiler* that actually emits the call.
>
> Only, the target of that call is a jmpq to another location where some
> version of the routine lives, and all have the same prototype.
>
> How is that any different from PLTs in shared libraries?

Ah, I see, I misunderstood your code.

See other email.  I think that, if you rework your series a bit to
have a generic version that works on all architectures, then do it
like you did on ARM, and make sure you leave the door open for the
inline patching approach, then it looks pretty good.

(None of this is to say that I disagree with Jason, though -- I'm not
entirely convinced that this makes sense for Zinc.  But maybe it can
be done in a way that makes everyone happy.)

^ permalink raw reply

* Re: [RFC PATCH 0/9] patchable function pointers for pluggable crypto routines
From: Ard Biesheuvel @ 2018-10-05 17:28 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Thomas Gleixner, linux-arm-kernel, Martin K. Petersen, Greg KH,
	LKML, Linux Crypto Mailing List, Andrew Morton, linuxppc-dev,
	David S. Miller
In-Reply-To: <CALCETrVxv1fHt6WE4ZpvVEwtfsKQ19gRrhhA40zwCXGMk+DTLQ@mail.gmail.com>

On 5 October 2018 at 19:26, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Oct 5, 2018 at 10:15 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> On 5 October 2018 at 15:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> ...
>> > Therefore, I think this patch goes in exactly the wrong direction. I
>> > mean, if you want to introduce dynamic patching as a means for making
>> > the crypto API's dynamic dispatch stuff not as slow in a post-spectre
>> > world, sure, go for it; that may very well be a good idea. But
>> > presenting it as an alternative to Zinc very widely misses the point and
>> > serves to prolong a series of bad design choices, which are now able to
>> > be rectified by putting energy into Zinc instead.
>> >
>>
>> This series has nothing to do with dynamic dispatch: the call sites
>> call crypto functions using ordinary function calls (although my
>> example uses CRC-T10DIF), and these calls are redirected via what is
>> essentially a PLT entry, so that we can supsersede those routines at
>> runtime.
>
> If you really want to do it PLT-style, then just do:
>
> extern void whatever_func(args);
>
> Call it like:
> whatever_func(args here);
>
> And rig up something to emit asm like:
>
> GLOBAL(whatever_func)
>   jmpq default_whatever_func
> ENDPROC(whatever_func)
>
> Architectures without support can instead do:
>
> void whatever_func(args)
> {
>   READ_ONCE(patchable_function_struct_for_whatever_func->ptr)(args);
> }
>
> and patch the asm function for basic support.  It will be slower than
> necessary, but maybe the relocation trick could be used on top of this
> to redirect the call to whatever_func directly to the target for
> architectures that want to squeeze out the last bit of performance.
> This might actually be the best of all worlds: easy implementation on
> all architectures, no inline asm, and the totally non-magical version
> works with okay performance.
>
> (Is this what your code is doing?  I admit I didn't follow all the way
> through all the macros.)

Basically

^ permalink raw reply

* Re: [RFC PATCH 0/9] patchable function pointers for pluggable crypto routines
From: Andy Lutomirski @ 2018-10-05 17:26 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Andrew Lutomirski, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Greg KH, LKML, Linux Crypto Mailing List,
	Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAKv+Gu-f4uKBF=gtFcNJEeoC2L4qShX0+3p7kVhjTn0tR+3Gng@mail.gmail.com>

On Fri, Oct 5, 2018 at 10:15 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On 5 October 2018 at 15:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> ...
> > Therefore, I think this patch goes in exactly the wrong direction. I
> > mean, if you want to introduce dynamic patching as a means for making
> > the crypto API's dynamic dispatch stuff not as slow in a post-spectre
> > world, sure, go for it; that may very well be a good idea. But
> > presenting it as an alternative to Zinc very widely misses the point and
> > serves to prolong a series of bad design choices, which are now able to
> > be rectified by putting energy into Zinc instead.
> >
>
> This series has nothing to do with dynamic dispatch: the call sites
> call crypto functions using ordinary function calls (although my
> example uses CRC-T10DIF), and these calls are redirected via what is
> essentially a PLT entry, so that we can supsersede those routines at
> runtime.

If you really want to do it PLT-style, then just do:

extern void whatever_func(args);

Call it like:
whatever_func(args here);

And rig up something to emit asm like:

GLOBAL(whatever_func)
  jmpq default_whatever_func
ENDPROC(whatever_func)

Architectures without support can instead do:

void whatever_func(args)
{
  READ_ONCE(patchable_function_struct_for_whatever_func->ptr)(args);
}

and patch the asm function for basic support.  It will be slower than
necessary, but maybe the relocation trick could be used on top of this
to redirect the call to whatever_func directly to the target for
architectures that want to squeeze out the last bit of performance.
This might actually be the best of all worlds: easy implementation on
all architectures, no inline asm, and the totally non-magical version
works with okay performance.

(Is this what your code is doing?  I admit I didn't follow all the way
through all the macros.)

^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Ard Biesheuvel @ 2018-10-05 17:23 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Thomas Gleixner, linux-arm-kernel, Martin K. Petersen, Greg KH,
	LKML, Linux Crypto Mailing List, Andrew Morton, linuxppc-dev,
	David S. Miller
In-Reply-To: <CALCETrU=fm+ZG4WZJfG-5GwL7kv5_GS1aZYu=L42d5BuSbhJDQ@mail.gmail.com>

On 5 October 2018 at 19:20, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Oct 5, 2018 at 10:11 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> On 5 October 2018 at 18:58, Andy Lutomirski <luto@kernel.org> wrote:
>> > On Fri, Oct 5, 2018 at 8:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> >>
>> >> On 5 October 2018 at 17:08, Andy Lutomirski <luto@amacapital.net> wrote:
>> >> >
>> >> >
>> >> >> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> >> >>
>> >> >>> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote:
>> >> >>> diff --git a/include/linux/ffp.h b/include/linux/ffp.h
>> >> >>> new file mode 100644
>> >> >>> index 000000000000..8fc3b4c9b38f
>> >> >>> --- /dev/null
>> >> >>> +++ b/include/linux/ffp.h
>> >> >>> @@ -0,0 +1,43 @@
>> >> >>> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> >>> +
>> >> >>> +#ifndef __LINUX_FFP_H
>> >> >>> +#define __LINUX_FFP_H
>> >> >>> +
>> >> >>> +#include <linux/types.h>
>> >> >>> +#include <linux/compiler.h>
>> >> >>> +
>> >> >>> +#ifdef CONFIG_HAVE_ARCH_FFP
>> >> >>> +#include <asm/ffp.h>
>> >> >>> +#else
>> >> >>> +
>> >> >>> +struct ffp {
>> >> >>> +    void    (**fn)(void);
>> >> >>> +    void    (*default_fn)(void);
>> >> >>> +};
>> >> >>> +
>> >> >>> +#define DECLARE_FFP(_fn, _def)                        \
>> >> >>> +    extern typeof(_def) *_fn;                    \
>> >> >>> +    extern struct ffp const __ffp_ ## _fn
>> >> >>> +
>> >> >>> +#define DEFINE_FFP(_fn, _def)                        \
>> >> >>> +    typeof(_def) *_fn = &_def;                    \
>> >> >>> +    struct ffp const __ffp_ ## _fn                    \
>> >> >>> +        = { (void(**)(void))&_fn, (void(*)(void))&_def };    \
>> >> >>> +    EXPORT_SYMBOL(__ffp_ ## _fn)
>> >> >>> +
>> >> >>> +static inline void ffp_set_target(const struct ffp *m, void *new_fn)
>> >> >>> +{
>> >> >>> +    WRITE_ONCE(*m->fn, new_fn);
>> >> >>> +}
>> >> >>> +
>> >> >>> +static inline void ffp_reset_target(const struct ffp *m)
>> >> >>> +{
>> >> >>> +    WRITE_ONCE(*m->fn, m->default_fn);
>> >> >>> +}
>> >> >>> +
>> >> >>> +#endif
>> >> >>> +
>> >> >>> +#define SET_FFP(_fn, _new)    ffp_set_target(&__ffp_ ## _fn, _new)
>> >> >>> +#define RESET_FFP(_fn)        ffp_reset_target(&__ffp_ ## _fn)
>> >> >>> +
>> >> >>> +#endif
>> >> >>
>> >> >> I don't understand this interface. There is no wrapper for the call
>> >> >> site, so how are we going to patch all call-sites when you update the
>> >> >> target?
>> >> >
>> >> > I’m also confused.
>> >> >
>> >> > Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated.
>> >> >
>> >> > I’ve proposed a better way that should generate better code, be more portable, and be more maintainable.  It goes like this.
>> >> >
>> >> > To call the function, you literally just call  the default implementation.  It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation.
>> >> >
>> >> > To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment.  Or we ask gcc for an extension to align calls to a function.)
>> >> >
>> >> > Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module.
>> >>
>> >> Yeah nothing is ever simple on x86 :-(
>> >>
>> >> So are you saying the approach i use in patch #2 (which would
>> >> translate to emitting a jmpq instruction pointing to the default
>> >> implementation, and patching it at runtime to point elsewhere) would
>> >> not fly on x86?
>> >
>> > After getting some more sleep, I'm obviously wrong.  The
>> > text_poke_bp() mechanism will work.  It's just really slow.
>> >
>>
>> OK
>>
>> > Let me try to summarize some of the issues.  First, when emitting
>> > jumps and calls from inline asm on x86, there are a few considerations
>> > that are annoying:
>> >
>> > 1. Following the x86_64 ABI calling conventions is basically
>> > impossible.  x86_64 requires a 128-byte redzone and 16-byte stack
>> > alignment.  After much discussion a while back, we decided that it was
>> > flat-out impossible on current gcc to get the stack pointer aligned in
>> > a known manner in an inline asm statement.  Instead, if we actually
>> > need alignment, we need to align manually.  Fortunately, the kernel is
>> > built with an override that forces only 8-byte alignment (on *most*
>> > GCC versions).  But for crypto in particular, it sucks extra, since
>> > the crypto code is basically the only thing in the kernel that
>> > actually wants 16-byte alignment.  I don't think this is a huge
>> > problem in practice, but it's annoying.  And the kernel is built
>> > without a redzone.
>> >
>> > 2. On x86_64, depending on config, we either need frame pointers or
>> > ORC.  ORC is no big deal -- it Just Works (tm).  Frame pointers need
>> > extra asm hackery.  It's doable, but it's still annoying.
>> >
>> > 3. Actually getting the asm constraints right to do what a C
>> > programmer expects is distinctly nontrivial.  I just fixed an
>> > extremely longstanding bug in the vDSO code in which the asm
>> > constraints for the syscall fallback were wrong in such a way that GCC
>> > didn't notice that the fallback wrote to its output parameter.
>> > Whoops.
>> >
>>
>> OK, so the thing I am missing is why this all matters.
>>
>> Note that the compiler should take care of all of this. It emits a
>> call a function with external linkage having prototype X, and all the
>> inline asm does is emit a jmp to some function having that same
>> prototype, either the default one or the one we patched in.
>>
>> Apologies if I am missing something obvious here: as you know, x86 is
>> not my focus in general.
>
> The big issue that bothers me isn't the x86-ism so much as the nasty
> interactions with the optimizer.  On x86, we have all of this working.
> It's in arch/x86/include/asm/paravirt_types.h, and it looks roughly
> like:
>
>                         asm volatile(pre                                \
>                                      paravirt_alt(PARAVIRT_CALL)        \
>                                      post                               \
>                                      : call_clbr, ASM_CALL_CONSTRAINT   \
>                                      : paravirt_type(op),               \
>                                        paravirt_clobber(clbr),          \
>                                        ##__VA_ARGS__                    \
>                                      : "memory", "cc" extra_clbr);      \
>
> With some extra magic for the constraints.  And I don't even think
> this is strictly correct -- from very recent experience, telling the
> compiler that "memory" is clobbered and that a bunch of arguments are
> used as numeric inputs may not actually imply that the asm modifies
> the target of pointer arguments.  Checks this lovely bug out:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b
>
> As far as I can tell, the whole PVOP infrastructure has the same bug.
> And I don't see how to avoid it generically on x86 or any other
> architecture.  (PeterZ, am I wrong?  Are we really just getting lucky
> that x86 pvop calls actually work?  Or do we not have enough of them
> that take pointers as arguments for this to matter?)
>
> Plus, asm volatile ( ..., "memory" ) is a barrier and makes code
> generation suck.
>
> Whereas, if we use my suggestion the semantics are precisely those of
> any other C function call because, as far as GCC is concerned, it *is*
> a C function call.  So the generated code *is* a function call.
>

But it is the *compiler* that actually emits the call.

Only, the target of that call is a jmpq to another location where some
version of the routine lives, and all have the same prototype.

How is that any different from PLTs in shared libraries?

^ permalink raw reply

* Re: [RFC PATCH 1/9] kernel: add support for patchable function pointers
From: Andy Lutomirski @ 2018-10-05 17:20 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jason A. Donenfeld, Peter Zijlstra, Catalin Marinas, Will Deacon,
	Samuel Neves, Paul Mackerras, Herbert Xu, Richard Weinberger,
	Eric Biggers, Ingo Molnar, Kees Cook, Arnd Bergmann,
	Andrew Lutomirski, Thomas Gleixner, linux-arm-kernel,
	Martin K. Petersen, Greg KH, LKML, Linux Crypto Mailing List,
	Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAKv+Gu-oHBtgGbWXJu6+z4+BNhBZyY5VvF_v6cJJUGk_5YX1eQ@mail.gmail.com>

On Fri, Oct 5, 2018 at 10:11 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On 5 October 2018 at 18:58, Andy Lutomirski <luto@kernel.org> wrote:
> > On Fri, Oct 5, 2018 at 8:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On 5 October 2018 at 17:08, Andy Lutomirski <luto@amacapital.net> wrote:
> >> >
> >> >
> >> >> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> >>
> >> >>> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote:
> >> >>> diff --git a/include/linux/ffp.h b/include/linux/ffp.h
> >> >>> new file mode 100644
> >> >>> index 000000000000..8fc3b4c9b38f
> >> >>> --- /dev/null
> >> >>> +++ b/include/linux/ffp.h
> >> >>> @@ -0,0 +1,43 @@
> >> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >> >>> +
> >> >>> +#ifndef __LINUX_FFP_H
> >> >>> +#define __LINUX_FFP_H
> >> >>> +
> >> >>> +#include <linux/types.h>
> >> >>> +#include <linux/compiler.h>
> >> >>> +
> >> >>> +#ifdef CONFIG_HAVE_ARCH_FFP
> >> >>> +#include <asm/ffp.h>
> >> >>> +#else
> >> >>> +
> >> >>> +struct ffp {
> >> >>> +    void    (**fn)(void);
> >> >>> +    void    (*default_fn)(void);
> >> >>> +};
> >> >>> +
> >> >>> +#define DECLARE_FFP(_fn, _def)                        \
> >> >>> +    extern typeof(_def) *_fn;                    \
> >> >>> +    extern struct ffp const __ffp_ ## _fn
> >> >>> +
> >> >>> +#define DEFINE_FFP(_fn, _def)                        \
> >> >>> +    typeof(_def) *_fn = &_def;                    \
> >> >>> +    struct ffp const __ffp_ ## _fn                    \
> >> >>> +        = { (void(**)(void))&_fn, (void(*)(void))&_def };    \
> >> >>> +    EXPORT_SYMBOL(__ffp_ ## _fn)
> >> >>> +
> >> >>> +static inline void ffp_set_target(const struct ffp *m, void *new_fn)
> >> >>> +{
> >> >>> +    WRITE_ONCE(*m->fn, new_fn);
> >> >>> +}
> >> >>> +
> >> >>> +static inline void ffp_reset_target(const struct ffp *m)
> >> >>> +{
> >> >>> +    WRITE_ONCE(*m->fn, m->default_fn);
> >> >>> +}
> >> >>> +
> >> >>> +#endif
> >> >>> +
> >> >>> +#define SET_FFP(_fn, _new)    ffp_set_target(&__ffp_ ## _fn, _new)
> >> >>> +#define RESET_FFP(_fn)        ffp_reset_target(&__ffp_ ## _fn)
> >> >>> +
> >> >>> +#endif
> >> >>
> >> >> I don't understand this interface. There is no wrapper for the call
> >> >> site, so how are we going to patch all call-sites when you update the
> >> >> target?
> >> >
> >> > I’m also confused.
> >> >
> >> > Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated.
> >> >
> >> > I’ve proposed a better way that should generate better code, be more portable, and be more maintainable.  It goes like this.
> >> >
> >> > To call the function, you literally just call  the default implementation.  It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation.
> >> >
> >> > To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment.  Or we ask gcc for an extension to align calls to a function.)
> >> >
> >> > Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module.
> >>
> >> Yeah nothing is ever simple on x86 :-(
> >>
> >> So are you saying the approach i use in patch #2 (which would
> >> translate to emitting a jmpq instruction pointing to the default
> >> implementation, and patching it at runtime to point elsewhere) would
> >> not fly on x86?
> >
> > After getting some more sleep, I'm obviously wrong.  The
> > text_poke_bp() mechanism will work.  It's just really slow.
> >
>
> OK
>
> > Let me try to summarize some of the issues.  First, when emitting
> > jumps and calls from inline asm on x86, there are a few considerations
> > that are annoying:
> >
> > 1. Following the x86_64 ABI calling conventions is basically
> > impossible.  x86_64 requires a 128-byte redzone and 16-byte stack
> > alignment.  After much discussion a while back, we decided that it was
> > flat-out impossible on current gcc to get the stack pointer aligned in
> > a known manner in an inline asm statement.  Instead, if we actually
> > need alignment, we need to align manually.  Fortunately, the kernel is
> > built with an override that forces only 8-byte alignment (on *most*
> > GCC versions).  But for crypto in particular, it sucks extra, since
> > the crypto code is basically the only thing in the kernel that
> > actually wants 16-byte alignment.  I don't think this is a huge
> > problem in practice, but it's annoying.  And the kernel is built
> > without a redzone.
> >
> > 2. On x86_64, depending on config, we either need frame pointers or
> > ORC.  ORC is no big deal -- it Just Works (tm).  Frame pointers need
> > extra asm hackery.  It's doable, but it's still annoying.
> >
> > 3. Actually getting the asm constraints right to do what a C
> > programmer expects is distinctly nontrivial.  I just fixed an
> > extremely longstanding bug in the vDSO code in which the asm
> > constraints for the syscall fallback were wrong in such a way that GCC
> > didn't notice that the fallback wrote to its output parameter.
> > Whoops.
> >
>
> OK, so the thing I am missing is why this all matters.
>
> Note that the compiler should take care of all of this. It emits a
> call a function with external linkage having prototype X, and all the
> inline asm does is emit a jmp to some function having that same
> prototype, either the default one or the one we patched in.
>
> Apologies if I am missing something obvious here: as you know, x86 is
> not my focus in general.

The big issue that bothers me isn't the x86-ism so much as the nasty
interactions with the optimizer.  On x86, we have all of this working.
It's in arch/x86/include/asm/paravirt_types.h, and it looks roughly
like:

                        asm volatile(pre                                \
                                     paravirt_alt(PARAVIRT_CALL)        \
                                     post                               \
                                     : call_clbr, ASM_CALL_CONSTRAINT   \
                                     : paravirt_type(op),               \
                                       paravirt_clobber(clbr),          \
                                       ##__VA_ARGS__                    \
                                     : "memory", "cc" extra_clbr);      \

With some extra magic for the constraints.  And I don't even think
this is strictly correct -- from very recent experience, telling the
compiler that "memory" is clobbered and that a bunch of arguments are
used as numeric inputs may not actually imply that the asm modifies
the target of pointer arguments.  Checks this lovely bug out:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/vdso-tglx&id=715bd9d12f84d8f5cc8ad21d888f9bc304a8eb0b

As far as I can tell, the whole PVOP infrastructure has the same bug.
And I don't see how to avoid it generically on x86 or any other
architecture.  (PeterZ, am I wrong?  Are we really just getting lucky
that x86 pvop calls actually work?  Or do we not have enough of them
that take pointers as arguments for this to matter?)

Plus, asm volatile ( ..., "memory" ) is a barrier and makes code
generation suck.

Whereas, if we use my suggestion the semantics are precisely those of
any other C function call because, as far as GCC is concerned, it *is*
a C function call.  So the generated code *is* a function call.

--Andy

^ permalink raw reply

* Re: [RFC PATCH 0/9] patchable function pointers for pluggable crypto routines
From: Ard Biesheuvel @ 2018-10-05 17:15 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Peter Zijlstra, Catalin Marinas, Will Deacon, Samuel Neves,
	Paul Mackerras, Herbert Xu, Richard Weinberger, Eric Biggers,
	Ingo Molnar, Kees Cook, Arnd Bergmann, Andy Lutomirski,
	Thomas Gleixner, linux-arm-kernel, Martin K. Petersen,
	Greg Kroah-Hartman, Linux Kernel Mailing List,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Andrew Morton,
	linuxppc-dev, David S. Miller
In-Reply-To: <20181005133705.GA4588@zx2c4.com>

On 5 October 2018 at 15:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
...
> Therefore, I think this patch goes in exactly the wrong direction. I
> mean, if you want to introduce dynamic patching as a means for making
> the crypto API's dynamic dispatch stuff not as slow in a post-spectre
> world, sure, go for it; that may very well be a good idea. But
> presenting it as an alternative to Zinc very widely misses the point and
> serves to prolong a series of bad design choices, which are now able to
> be rectified by putting energy into Zinc instead.
>

This series has nothing to do with dynamic dispatch: the call sites
call crypto functions using ordinary function calls (although my
example uses CRC-T10DIF), and these calls are redirected via what is
essentially a PLT entry, so that we can supsersede those routines at
runtime.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox