* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format
From: Nicolin Chen @ 2020-03-23 21:20 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, Shengjiu Wang, tiwai, lgirdwood, perex, broonie,
festevam, linux-kernel
In-Reply-To: <20200320173213.GA9093@bogus>
On Fri, Mar 20, 2020 at 11:32:13AM -0600, Rob Herring wrote:
> On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > In order to support new EASRC and simplify the code structure,
> > > We decide to share the common structure between them. This bring
> > > a problem that EASRC accept format directly from devicetree, but
> > > ASRC accept width from devicetree.
> > >
> > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > can accept format from devicetree, don't need to convert it to
> > > format through width.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > index cb9a25165503..780455cf7f71 100644
> > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > @@ -51,6 +51,11 @@ Optional properties:
> > > will be in use as default. Otherwise, the big endian
> > > mode will be in use for all the device registers.
> > >
> > > + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > + Ends, which can replace the fsl,asrc-width.
> > > + The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > + SNDRV_PCM_FORMAT_S24_LE
> >
> > I am still holding the concern at the DT binding of this format,
> > as it uses values from ASoC header file instead of a dt-binding
> > header file -- not sure if we can do this. Let's wait for Rob's
> > comments.
>
> I assume those are an ABI as well, so it's okay to copy them unless we
They are defined under include/uapi. So I think we can use them?
> already have some format definitions for DT. But it does need to be copy
> in a header under include/dt-bindings/.
Shengjiu is actually quoting those integral values, rather than
those macros, so actually no need copy to include/dt-bindings,
yet whoever adds this format property to a new DT would need to
look up the value in a header file under include/uapi. I's just
wondering if that's okay.
Thanks
^ permalink raw reply
* Re: [PATCH] powerpc/64: allow rtas to be called in real-mode, use this in machine check
From: Ganesh @ 2020-03-23 10:19 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Mahesh Salgaonkar
In-Reply-To: <20200320152816.1469657-1-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5449 bytes --]
On 3/20/20 8:58 PM, Nicholas Piggin wrote:
> rtas_call allocates and uses memory in failure paths, which is
> not safe for RMA. It also calls local_irq_save() which may not be safe
> in all real mode contexts.
>
> Particularly machine check may run with interrupts not "reconciled",
> and it may have hit while it was in tracing code that should not be
> rentered.
>
> Create minimal rtas call that should be usable by guest machine check
> code, use it there to call "ibm,nmi-interlock".
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/rtas.h | 1 +
> arch/powerpc/kernel/entry_64.S | 12 ++++++--
> arch/powerpc/kernel/rtas.c | 43 ++++++++++++++++++++++++++++
> arch/powerpc/platforms/pseries/ras.c | 2 +-
> 4 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 3c1887351c71..4ffc499ce1ac 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -352,6 +352,7 @@ extern struct rtas_t rtas;
> extern int rtas_token(const char *service);
> extern int rtas_service_present(const char *service);
> extern int rtas_call(int token, int, int, int *, ...);
> +extern int raw_rtas_call(int token, int, int, int *, ...);
> void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
> int nret, ...);
> extern void __noreturn rtas_restart(char *cmd);
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 51c5b681f70c..309abb677788 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -759,6 +759,13 @@ _GLOBAL(enter_rtas)
> li r0,0
> mtcr r0
>
> + /* enter_rtas called from real-mode may not have irqs reconciled
> + * but will always have interrupts disabled.
> + */
> + mfmsr r6
> + andi. r7,r6,(MSR_IR|MSR_DR)
> + beq 2f
> +
> #ifdef CONFIG_BUG
> /* There is no way it is acceptable to get here with interrupts enabled,
> * check it with the asm equivalent of WARN_ON
> @@ -769,10 +776,10 @@ _GLOBAL(enter_rtas)
> #endif
>
> /* Hard-disable interrupts */
> - mfmsr r6
> rldicl r7,r6,48,1
> rotldi r7,r7,16
> mtmsrd r7,1
> +2:
>
> /* Unfortunately, the stack pointer and the MSR are also clobbered,
> * so they are saved in the PACA which allows us to restore
> @@ -795,7 +802,6 @@ _GLOBAL(enter_rtas)
> ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
> andc r6,r0,r9
>
> -__enter_rtas:
> sync /* disable interrupts so SRR0/1 */
> mtmsrd r0 /* don't get trashed */
>
> @@ -837,7 +843,7 @@ rtas_return_loc:
> mtspr SPRN_SRR1,r4
> RFI_TO_KERNEL
> b . /* prevent speculative execution */
> -_ASM_NOKPROBE_SYMBOL(__enter_rtas)
> +_ASM_NOKPROBE_SYMBOL(enter_rtas)
> _ASM_NOKPROBE_SYMBOL(rtas_return_loc)
>
> .align 3
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index c5fa251b8950..a058dcfb6726 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -450,6 +450,8 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
> char *buff_copy = NULL;
> int ret;
>
> + WARN_ON_ONCE((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR));
> +
> if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> return -1;
>
> @@ -483,6 +485,47 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
> }
> EXPORT_SYMBOL(rtas_call);
>
> +/*
> + * Like rtas_call but no kmalloc or printk etc in error handling, so
> + * error won't go through log_error. No tracing, may be called in real mode.
> + */
> +int notrace raw_rtas_call(int token, int nargs, int nret, int *outputs, ...)
> +{
> + va_list list;
> + int i;
> + struct rtas_args *rtas_args;
> + int ret;
> +
> + WARN_ON_ONCE((mfmsr() & MSR_EE));
> +
> + if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> + return -1;
> +
> + /*
> + * Real mode must have MSR[EE]=0 and we prefer not to touch any
> + * irq or preempt state (this may be called in machine check).
> + */
> + preempt_disable_notrace();
> + arch_spin_lock(&rtas.lock);
I wonder, if its ok to attempt for this lock in nested MCE.
> +
> + /* We use the global rtas args buffer */
> + rtas_args = &rtas.args;
> +
> + va_start(list, outputs);
> + va_rtas_call_unlocked(rtas_args, token, nargs, nret, list);
> + va_end(list);
> +
> + if (nret > 1 && outputs != NULL)
> + for (i = 0; i < nret-1; ++i)
> + outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
> + ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
> +
> + arch_spin_unlock(&rtas.lock);
> + preempt_enable_notrace();
> +
> + return ret;
> +}
> +
> /* For RTAS_BUSY (-2), delay for 1 millisecond. For an extended busy status
> * code of 990n, perform the hinted delay of 10^n (last digit) milliseconds.
> */
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index c74d5e740922..e87f86f02569 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -458,7 +458,7 @@ static struct rtas_error_log *fwnmi_get_errinfo(struct pt_regs *regs)
> */
> static void fwnmi_release_errinfo(void)
> {
> - int ret = rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
> + int ret = raw_rtas_call(ibm_nmi_interlock_token, 0, 1, NULL);
> if (ret != 0)
> printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
> }
[-- Attachment #2: Type: text/html, Size: 5772 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/legacy_serial: Use early_ioremap()
From: Chris Packham @ 2020-03-23 21:54 UTC (permalink / raw)
To: christophe.leroy@c-s.fr, paulus@samba.org, mpe@ellerman.id.au,
benh@kernel.crashing.org
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <3df15946ed0c29663dc7928b31ca07576e1444f6.1580904214.git.christophe.leroy@c-s.fr>
Hi Christophe,
On Wed, 2020-02-05 at 12:03 +0000, Christophe Leroy wrote:
> [ 0.000000] ioremap() called early from
> find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>
I was just about to dig into this error message and found you patch. I
applied it to a v5.5 base.
> find_legacy_serial_ports() is called early from setup_arch(), before
> paging_init(). vmalloc is not available yet, ioremap shouldn't be
> used that early.
>
> Use early_ioremap() and switch to a regular ioremap() later.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
On my system (Freescale T2080 SOC) this seems to cause a crash/hang in
early boot. Unfortunately because this is affecting the boot console I
don't get any earlyprintk output.
> ---
> arch/powerpc/kernel/legacy_serial.c | 33 +++++++++++++++++++++++++
> ----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c
> b/arch/powerpc/kernel/legacy_serial.c
> index f061e06e9f51..8b2c1a8553a0 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -15,6 +15,7 @@
> #include <asm/udbg.h>
> #include <asm/pci-bridge.h>
> #include <asm/ppc-pci.h>
> +#include <asm/early_ioremap.h>
>
> #undef DEBUG
>
> @@ -34,6 +35,7 @@ static struct legacy_serial_info {
> unsigned int clock;
> int irq_check_parent;
> phys_addr_t taddr;
> + void __iomem *early_addr;
> } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
> static const struct of_device_id legacy_serial_parents[] __initconst
> = {
> @@ -325,17 +327,16 @@ static void __init
> setup_legacy_serial_console(int console)
> {
> struct legacy_serial_info *info =
> &legacy_serial_infos[console];
> struct plat_serial8250_port *port =
> &legacy_serial_ports[console];
> - void __iomem *addr;
> unsigned int stride;
>
> stride = 1 << port->regshift;
>
> /* Check if a translated MMIO address has been found */
> if (info->taddr) {
> - addr = ioremap(info->taddr, 0x1000);
> - if (addr == NULL)
> + info->early_addr = early_ioremap(info->taddr, 0x1000);
> + if (info->early_addr == NULL)
> return;
> - udbg_uart_init_mmio(addr, stride);
> + udbg_uart_init_mmio(info->early_addr, stride);
> } else {
> /* Check if it's PIO and we support untranslated PIO */
> if (port->iotype == UPIO_PORT && isa_io_special)
> @@ -353,6 +354,30 @@ static void __init
> setup_legacy_serial_console(int console)
> udbg_uart_setup(info->speed, info->clock);
> }
>
> +static int __init ioremap_legacy_serial_console(void)
> +{
> + struct legacy_serial_info *info =
> &legacy_serial_infos[legacy_serial_console];
> + struct plat_serial8250_port *port =
> &legacy_serial_ports[legacy_serial_console];
> + void __iomem *vaddr;
> +
> + if (legacy_serial_console < 0)
> + return 0;
> +
> + if (!info->early_addr)
> + return 0;
> +
> + vaddr = ioremap(info->taddr, 0x1000);
> + if (WARN_ON(!vaddr))
> + return -ENOMEM;
> +
> + udbg_uart_init_mmio(vaddr, 1 << port->regshift);
> + early_iounmap(info->early_addr, 0x1000);
> + info->early_addr = NULL;
> +
> + return 0;
> +}
> +early_initcall(ioremap_legacy_serial_console);
> +
> /*
> * This is called very early, as part of setup_system() or
> eventually
> * setup_arch(), basically before anything else in this file. This
> function
^ permalink raw reply
* RE: [patch V3 08/20] hexagon: Remove mm.h from asm/uaccess.h
From: Brian Cain @ 2020-03-23 21:46 UTC (permalink / raw)
To: 'Thomas Gleixner', 'LKML'
Cc: 'Randy Dunlap', linux-ia64, 'Peter Zijlstra',
linux-pci, 'Sebastian Siewior', platform-driver-x86,
'Guo Ren', 'Joel Fernandes',
'Vincent Chen', 'Ingo Molnar',
'Jonathan Corbet', 'Davidlohr Bueso',
'kbuild test robot', linux-acpi,
'Paul E . McKenney', linux-hexagon,
'Rafael J. Wysocki', linux-csky, 'Linus Torvalds',
'Darren Hart', 'Zhang Rui', 'Len Brown',
'Fenghua Yu', 'Arnd Bergmann', linux-pm,
linuxppc-dev, 'Greentime Hu', 'Bjorn Helgaas',
'Kurt Schwemmer', 'Kalle Valo',
'Felipe Balbi', 'Michal Simek',
'Tony Luck', 'Nick Hu', 'Geoff Levand',
'Greg Kroah-Hartman', linux-usb, linux-wireless,
'Oleg Nesterov', 'Davidlohr Bueso', netdev,
'Logan Gunthorpe', 'David S. Miller',
'Andy Shevchenko'
In-Reply-To: <20200321113241.531525286@linutronix.de>
> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
...
> Subject: [patch V3 08/20] hexagon: Remove mm.h from asm/uaccess.h
>
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> The defconfig compiles without linux/mm.h. With mm.h included the include
> chain leands to:
> | CC kernel/locking/percpu-rwsem.o
> | In file included from include/linux/huge_mm.h:8,
> | from include/linux/mm.h:567,
> | from arch/hexagon/include/asm/uaccess.h:,
> | from include/linux/uaccess.h:11,
> | from include/linux/sched/task.h:11,
> | from include/linux/sched/signal.h:9,
> | from include/linux/rcuwait.h:6,
> | from include/linux/percpu-rwsem.h:8,
> | from kernel/locking/percpu-rwsem.c:6:
> | include/linux/fs.h:1422:29: error: array type has incomplete element type
> 'struct percpu_rw_semaphore'
> | 1422 | struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
>
> once rcuwait.h includes linux/sched/signal.h.
>
> Remove the linux/mm.h include.
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Brian Cain <bcain@codeaurora.org>
> Cc: linux-hexagon@vger.kernel.org
> ---
> V3: New patch
> ---
> arch/hexagon/include/asm/uaccess.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/hexagon/include/asm/uaccess.h
> b/arch/hexagon/include/asm/uaccess.h
> index 00cb38faad0c4..c1019a736ff13 100644
> --- a/arch/hexagon/include/asm/uaccess.h
> +++ b/arch/hexagon/include/asm/uaccess.h
> @@ -10,7 +10,6 @@
> /*
> * User space memory access functions
> */
> -#include <linux/mm.h>
> #include <asm/sections.h>
>
> /*
> --
> 2.26.0.rc2
>
Acked-by: Brian Cain <bcain@codeaurora.org>
^ permalink raw reply
* [PATCH] powerpc/maple: Fix declaration made after definition
From: Nathan Chancellor @ 2020-03-23 22:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: Ilie Halip, Nick Desaulniers, linux-kernel, clang-built-linux,
Nathan Chancellor, linuxppc-dev
When building ppc64 defconfig, Clang errors (trimmed for brevity):
arch/powerpc/platforms/maple/setup.c:365:1: error: attribute declaration
must precede definition [-Werror,-Wignored-attributes]
machine_device_initcall(maple, maple_cpc925_edac_setup);
^
machine_device_initcall expands to __define_machine_initcall, which in
turn has the macro machine_is used in it, which declares mach_##name
with an __attribute__((weak)). define_machine actually defines
mach_##name, which in this file happens before the declaration, hence
the warning.
To fix this, move define_machine after machine_device_initcall so that
the declaration occurs before the definition, which matches how
machine_device_initcall and define_machine work throughout arch/powerpc.
While we're here, remove some spaces before tabs.
Fixes: 8f101a051ef0 ("edac: cpc925 MC platform device setup")
Link: https://godbolt.org/z/kDoYSA
Link: https://github.com/ClangBuiltLinux/linux/issues/662
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Suggested-by: Ilie Halip <ilie.halip@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
arch/powerpc/platforms/maple/setup.c | 34 ++++++++++++++--------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index 6f019df37916..15b2c6eb506d 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -291,23 +291,6 @@ static int __init maple_probe(void)
return 1;
}
-define_machine(maple) {
- .name = "Maple",
- .probe = maple_probe,
- .setup_arch = maple_setup_arch,
- .init_IRQ = maple_init_IRQ,
- .pci_irq_fixup = maple_pci_irq_fixup,
- .pci_get_legacy_ide_irq = maple_pci_get_legacy_ide_irq,
- .restart = maple_restart,
- .halt = maple_halt,
- .get_boot_time = maple_get_boot_time,
- .set_rtc_time = maple_set_rtc_time,
- .get_rtc_time = maple_get_rtc_time,
- .calibrate_decr = generic_calibrate_decr,
- .progress = maple_progress,
- .power_save = power4_idle,
-};
-
#ifdef CONFIG_EDAC
/*
* Register a platform device for CPC925 memory controller on
@@ -364,3 +347,20 @@ static int __init maple_cpc925_edac_setup(void)
}
machine_device_initcall(maple, maple_cpc925_edac_setup);
#endif
+
+define_machine(maple) {
+ .name = "Maple",
+ .probe = maple_probe,
+ .setup_arch = maple_setup_arch,
+ .init_IRQ = maple_init_IRQ,
+ .pci_irq_fixup = maple_pci_irq_fixup,
+ .pci_get_legacy_ide_irq = maple_pci_get_legacy_ide_irq,
+ .restart = maple_restart,
+ .halt = maple_halt,
+ .get_boot_time = maple_get_boot_time,
+ .set_rtc_time = maple_set_rtc_time,
+ .get_rtc_time = maple_get_rtc_time,
+ .calibrate_decr = generic_calibrate_decr,
+ .progress = maple_progress,
+ .power_save = power4_idle,
+};
--
2.26.0
^ permalink raw reply related
* Re: [PATCH v4 14/16] powerpc64: Add prefixed instructions to instruction data type
From: Jordan Niethe @ 2020-03-23 23:45 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584947189.oay6araq0n.astroid@bobo.none>
On Mon, Mar 23, 2020 at 6:37 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:18 pm:
> > For powerpc64, redefine the ppc_inst type so both word and prefixed
> > instructions can be represented. On powerpc32 the type will remain the
> > same. Update places which had assumed instructions to be 4 bytes long.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/include/asm/code-patching.h | 10 +--
> > arch/powerpc/include/asm/inst.h | 90 ++++++++++++++++++++++++
> > arch/powerpc/include/asm/kprobes.h | 2 +-
> > arch/powerpc/include/asm/sstep.h | 4 ++
> > arch/powerpc/include/asm/uaccess.h | 22 ++++++
> > arch/powerpc/include/asm/uprobes.h | 2 +-
> > arch/powerpc/kernel/align.c | 5 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 2 +-
> > arch/powerpc/kernel/kprobes.c | 7 +-
> > arch/powerpc/kernel/optprobes.c | 42 ++++++-----
> > arch/powerpc/kernel/optprobes_head.S | 3 +
> > arch/powerpc/kernel/trace/ftrace.c | 19 ++++-
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/lib/code-patching.c | 22 ++++--
> > arch/powerpc/lib/sstep.c | 4 +-
> > arch/powerpc/xmon/xmon.c | 38 +++++++---
> > 16 files changed, 221 insertions(+), 53 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> > index 68bd9db334bd..bd41e1558707 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -25,11 +25,11 @@
> > bool is_offset_in_branch_range(long offset);
> > ppc_inst create_branch(const ppc_inst *addr,
> > unsigned long target, int flags);
> > -unsigned int create_cond_branch(const ppc_inst *addr,
> > +ppc_inst create_cond_branch(const void *addr,
> > unsigned long target, int flags);
> > -int patch_branch(ppc_inst *addr, unsigned long target, int flags);
> > -int patch_instruction(ppc_inst *addr, ppc_inst instr);
> > -int raw_patch_instruction(ppc_inst *addr, ppc_inst instr);
> > +int patch_branch(void *addr, unsigned long target, int flags);
> > +int patch_instruction(void *addr, ppc_inst instr);
> > +int raw_patch_instruction(void *addr, ppc_inst instr);
> >
> > static inline unsigned long patch_site_addr(s32 *site)
> > {
> > @@ -60,7 +60,7 @@ static inline int modify_instruction_site(s32 *site, unsigned int clr, unsigned
> > int instr_is_relative_branch(ppc_inst instr);
> > int instr_is_relative_link_branch(ppc_inst instr);
> > int instr_is_branch_to_addr(const ppc_inst *instr, unsigned long addr);
> > -unsigned long branch_target(const ppc_inst *instr);
> > +unsigned long branch_target(const void *instr);
> > ppc_inst translate_branch(const ppc_inst *dest,
> > const ppc_inst *src);
> > extern bool is_conditional_branch(ppc_inst instr);
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > index 7c8596ee411e..1a40b0a71128 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -6,6 +6,95 @@
> > * Instruction data type for POWER
> > */
> >
> > +#ifdef __powerpc64__
> > +
> > +typedef struct ppc_inst {
> > + union {
> > + struct {
> > + u32 word;
> > + u32 pad;
> > + } __packed;
> > + struct {
> > + u32 prefix;
> > + u32 suffix;
> > + } __packed;
> > + };
> > +} ppc_inst;
> > +
> > +#define PPC_INST(x) ((ppc_inst) { .word = (x), .pad = 0 })
> > +#define PPC_INST_PREFIXED(x, y) ((ppc_inst) { .prefix = (x), .suffix = (y) })
> > +
> > +static inline int ppc_inst_opcode(ppc_inst x)
> > +{
> > + return x.word >> 26;
> > +}
> > +
> > +static inline bool ppc_inst_prefixed(ppc_inst x) {
> > + return ppc_inst_opcode(x) == 1;
> > +}
> > +
> > +static inline int ppc_inst_len(ppc_inst x)
> > +{
> > + if (ppc_inst_prefixed(x))
> > + return 8;
> > + else
> > + return 4;
> > +}
> > +
> > +static inline u32 ppc_inst_word(ppc_inst x)
> > +{
> > + return x.word;
> > +}
>
> I guess a concern could be that code using ppc_inst_word could now get a
> prefix unexpectedly and not handle it properly. The reason it should
> generally be okay is that prefix won't match any existing valid
> instruction words, so callers won't match or think it's an unknown
> instruction. Am I right? Possibly a small comment?
Yes all prefixes are illegal as word instructions so I thought that
should be okay.
>
> > +
> > +static inline u32 ppc_inst_prefix(ppc_inst x)
> > +{
> > + return x.prefix;
> > +}
> > +
> > +static inline u32 ppc_inst_suffix(ppc_inst x)
> > +{
> > + return x.suffix;
> > +}
> > +
> > +
> > +static inline ppc_inst ppc_inst_read(const void *ptr)
> > +{
> > + ppc_inst inst;
> > + inst.word = *(u32 *)ptr;
> > + if (ppc_inst_prefixed(inst))
> > + inst.suffix = *((u32 *)ptr + 1);
> > + else
> > + inst.pad = 0;
>
> I'm a bit against using partially constructed opaque type for things
> like this, even if it is in the code that knows about the type. We
> could modify ppc_inst_prefixed() to assert that pad is equal to zero
> (or some poisoned value) if it's not prefixed. Or do some validation
> on the suffix if it is.
Okay what about something like:
+static inline ppc_inst ppc_inst_read(const void *ptr)
+{
+ u32 prefix, suffix;
+ prefix = *(u32 *)ptr;
+ if (prefix >> 26 == 1)
+ suffix = *((u32 *)ptr + 1);
+ else
+ suffix = 0;
+ return PPC_INST_PREFIX(prefix, suffix);
+}
>
>
>
> > +static inline bool ppc_inst_equal(ppc_inst x, ppc_inst y)
> > +{
> > + return !memcmp(&x, &y, sizeof(struct ppc_inst));
> > +}
>
> I guess a variable length memcmp will make terrible code, so you're
> requiring pad to equal 0 to match non-prefixed. Fine.
Yes this assumes pad always gets set to 0.
>
> > +
> > +static inline bool ppc_inst_null(ppc_inst x)
> > +{
> > + return x.word == 0 && x.pad == 0;
> > +}
>
> In this case you shouldn't need x.pad == 0. If x.word == 0, then
> WARN_ON_ONCE(x.pad != 0) ?
True.
>
> > if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
> > /* We don't handle PPC little-endian any more... */
> > if (cpu_has_feature(CPU_FTR_PPC_LE))
> > return -EIO;
> > - instr = PPC_INST(swab32(ppc_inst_word(instr)));
> > + instr = PPC_INST_PREFIXED(swab32(ppc_inst_word(instr)),
> > + swab32(ppc_inst_suffix(instr)));
>
> Ugly, don't suppose you'd bother to do a ppc_inst_bswap function for
> this one case?
I can do that.
>
> [snip probes stuff]
>
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index fa7f32adf029..3b8277a64b8f 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -24,17 +24,27 @@ static int __patch_instruction(ppc_inst *exec_addr, ppc_inst instr,
> > {
> > int err = 0;
> >
> > - __put_user_asm(instr, patch_addr, err, "stw");
> > + __put_user_asm(ppc_inst_word(instr), patch_addr, err, "stw");
> > if (err)
> > return err;
> >
> > asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> > "r" (exec_addr));
> >
> > + if (!ppc_inst_prefixed(instr))
> > + return 0;
> > +
> > + __put_user_asm(ppc_inst_suffix(instr), patch_addr + 4, err, "stw");
> > + if (err)
> > + return err;
> > +
> > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr + 4),
> > + "r" (exec_addr + 4));
>
> Although there's proably no real performance or atomicity issues here,
> I'd be pleased if we could do a case for prefixed and a case for non
> prefixed, and store the non-prefixed with "std". Just for the principle
> of not having half-written instructions in the image.
Do you mean store the prefixed with std?
>
> You could skip the dcbst and icbi for the second address if you happen
> to know this future CPU does not store prefix insns across a CL
> boundary. But probably not necessary to make that assumption in non
> perf critical code here, so I'd leave it as you have.
>
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index ee084411f2f5..c5536e1a3356 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -110,7 +110,7 @@ struct bpt {
> > #define BP_DABR 4
> >
> > #define NBPTS 256
> > -#define BPT_WORDS 2
> > +#define BPT_WORDS 4
>
> (2 * sizeof(ppc_inst) / sizeof(u32)) ?
>
> > static struct bpt bpts[NBPTS];
> > static struct bpt dabr;
> > static struct bpt *iabr;
> > @@ -118,12 +118,13 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
> >
> > #define BP_NUM(bp) ((bp) - bpts + 1)
> >
> > -static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS * BPT_WORDS];
> > +static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS * BPT_WORDS] __aligned(64);
>
> Should have a define somewhere for this magical 64.
Sure.
>
> > /* Prototypes */
> > static int cmds(struct pt_regs *);
> > static int mread(unsigned long, void *, int);
> > static int mwrite(unsigned long, void *, int);
> > +static int mread_instr(unsigned long, ppc_inst *);
>
> In some cases you've addd helpers like this as separate patches,
> others you've bundled them together. NBD but I liked the prep patches
> which then made the more important changes easier to see.
I will make some more prep patches.
>
> > @@ -759,8 +760,8 @@ static int xmon_bpt(struct pt_regs *regs)
> >
> > /* Are we at the trap at bp->instr[1] for some bp? */
> > bp = in_breakpoint_table(regs->nip, &offset);
> > - if (bp != NULL && offset == 4) {
> > - regs->nip = bp->address + 4;
> > + if (bp != NULL && (offset == 4 || offset == 8)) {
> > + regs->nip = bp->address + offset;
> > atomic_dec(&bp->ref_count);
> > return 1;
> > }
> > @@ -862,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> > if (off >= sizeof(bpt_table))
> > return NULL;
> > bp_off = off % (sizeof(unsigned int) * BPT_WORDS);
> > - if (bp_off != 0 && bp_off != 4)
> > + if (bp_off != 0 && bp_off != 4 && bp_off != 8)
> > return NULL;
> > *offp = bp_off;
> > return bpts + ((off - bp_off) / (sizeof(unsigned int) * BPT_WORDS));
> > @@ -881,7 +882,6 @@ static struct bpt *new_breakpoint(unsigned long a)
> > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> > bp->address = a;
> > bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS);
> > - patch_instruction(bp->instr + 1, PPC_INST(bpinstr));
> > return bp;
> > }
> > }
>
> Why is this okay to remove?
When we only had word instructions the bpt was just patched in here
once and that was that.
With prefixed instructions bp->instr + 1 might be the suffix. So I
moved putting the breakpoint to insert_bpts():
patch_instruction(bp->instr + ppc_inst_len(instr), PPC_INST(bpinstr));
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH 3/4] hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
From: Mike Kravetz @ 2020-03-24 0:16 UTC (permalink / raw)
To: Mina Almasry
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S.Miller
In-Reply-To: <CAHS8izN1Q+yiZq8zcU8hoojafcQ19W7XQx-HyFm3Ud8hSvN1Nw@mail.gmail.com>
On 3/23/20 5:01 PM, Mina Almasry wrote:
> On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>
>> The routine hugetlb_add_hstate prints a warning if the hstate already
>> exists. This was originally done as part of kernel command line
>> parsing. If 'hugepagesz=' was specified more than once, the warning
>> pr_warn("hugepagesz= specified twice, ignoring\n");
>> would be printed.
>>
>> Some architectures want to enable all huge page sizes. They would
>> call hugetlb_add_hstate for all supported sizes. However, this was
>> done after command line processing and as a result hstates could have
>> already been created for some sizes. To make sure no warning were
>> printed, there would often be code like:
>> if (!size_to_hstate(size)
>> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT)
>>
>> The only time we want to print the warning is as the result of command
>> line processing. So, remove the warning from hugetlb_add_hstate and
>> add it to the single arch independent routine processing "hugepagesz=".
>> After this, calls to size_to_hstate() in arch specific code can be
>> removed and hugetlb_add_hstate can be called without worrying about
>> warning messages.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>> arch/arm64/mm/hugetlbpage.c | 16 ++++------------
>> arch/powerpc/mm/hugetlbpage.c | 3 +--
>> arch/riscv/mm/hugetlbpage.c | 2 +-
>> arch/sparc/mm/init_64.c | 19 ++++---------------
>> arch/x86/mm/hugetlbpage.c | 2 +-
>> mm/hugetlb.c | 10 +++++++---
>> 6 files changed, 18 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 4aa9534a45d7..050809e6f0a9 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -441,22 +441,14 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
>> clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
>> }
>>
>> -static void __init add_huge_page_size(unsigned long size)
>> -{
>> - if (size_to_hstate(size))
>> - return;
>> -
>> - hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
>> -}
>> -
>> static int __init hugetlbpage_init(void)
>> {
>> #ifdef CONFIG_ARM64_4K_PAGES
>> - add_huge_page_size(PUD_SIZE);
>> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
>> #endif
>> - add_huge_page_size(CONT_PMD_SIZE);
>> - add_huge_page_size(PMD_SIZE);
>> - add_huge_page_size(CONT_PTE_SIZE);
>> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
>> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
>>
>> return 0;
>> }
>> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
>> index 166960ba1236..f46464ba6fb4 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -584,8 +584,7 @@ static int __init add_huge_page_size(unsigned long long size)
>> if (!arch_hugetlb_valid_size(size))
>> return -EINVAL;
>>
>> - if (!size_to_hstate(size))
>> - hugetlb_add_hstate(shift - PAGE_SHIFT);
>> + hugetlb_add_hstate(shift - PAGE_SHIFT);
>> return 0;
>> }
>>
>> diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c
>> index bdf89d7eb714..beaa91941db8 100644
>> --- a/arch/riscv/mm/hugetlbpage.c
>> +++ b/arch/riscv/mm/hugetlbpage.c
>> @@ -26,7 +26,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
>> static __init int gigantic_pages_init(void)
>> {
>> /* With CONTIG_ALLOC, we can allocate gigantic pages at runtime */
>> - if (IS_ENABLED(CONFIG_64BIT) && !size_to_hstate(1UL << PUD_SHIFT))
>> + if (IS_ENABLED(CONFIG_64BIT))
>> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
>> return 0;
>> }
>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>> index 5c29203fd460..8f619edc8f8c 100644
>> --- a/arch/sparc/mm/init_64.c
>> +++ b/arch/sparc/mm/init_64.c
>> @@ -325,23 +325,12 @@ static void __update_mmu_tsb_insert(struct mm_struct *mm, unsigned long tsb_inde
>> }
>>
>> #ifdef CONFIG_HUGETLB_PAGE
>> -static void __init add_huge_page_size(unsigned long size)
>> -{
>> - unsigned int order;
>> -
>> - if (size_to_hstate(size))
>> - return;
>> -
>> - order = ilog2(size) - PAGE_SHIFT;
>> - hugetlb_add_hstate(order);
>> -}
>> -
>> static int __init hugetlbpage_init(void)
>> {
>> - add_huge_page_size(1UL << HPAGE_64K_SHIFT);
>> - add_huge_page_size(1UL << HPAGE_SHIFT);
>> - add_huge_page_size(1UL << HPAGE_256MB_SHIFT);
>> - add_huge_page_size(1UL << HPAGE_2GB_SHIFT);
>> + hugetlb_add_hstate(HPAGE_64K_SHIFT - PAGE_SHIFT);
>> + hugetlb_add_hstate(HPAGE_SHIFT - PAGE_SHIFT);
>> + hugetlb_add_hstate(HPAGE_256MB_SHIFT - PAGE_SHIFT);
>> + hugetlb_add_hstate(HPAGE_2GB_SHIFT - PAGE_SHIFT);
>>
>> return 0;
>> }
>> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
>> index dd3ed09f6c23..8a3f586e1217 100644
>> --- a/arch/x86/mm/hugetlbpage.c
>> +++ b/arch/x86/mm/hugetlbpage.c
>> @@ -195,7 +195,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
>> static __init int gigantic_pages_init(void)
>> {
>> /* With compaction or CMA we can allocate gigantic pages at runtime */
>> - if (boot_cpu_has(X86_FEATURE_GBPAGES) && !size_to_hstate(1UL << PUD_SHIFT))
>> + if (boot_cpu_has(X86_FEATURE_GBPAGES))
>> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
>> return 0;
>> }
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index cd4ec07080fb..cc85b4f156ca 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3116,8 +3116,7 @@ static int __init hugetlb_init(void)
>> }
>>
>> default_hstate_size = HPAGE_SIZE;
>> - if (!size_to_hstate(default_hstate_size))
>> - hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
>> + hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
>> }
>> default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size));
>> if (default_hstate_max_huge_pages) {
>> @@ -3155,7 +3154,6 @@ void __init hugetlb_add_hstate(unsigned int order)
>> unsigned long i;
>>
>> if (size_to_hstate(PAGE_SIZE << order)) {
>> - pr_warn("hugepagesz= specified twice, ignoring\n");
>> return;
>> }
>> BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
>> @@ -3231,6 +3229,12 @@ static int __init hugepagesz_setup(char *s)
>> return 0;
>> }
>>
>> + if (size_to_hstate(size)) {
>> + pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n",
>> + saved_s);
>> + return 0;
>> + }
>> +
>
> Not too familiar with the code but I'm a bit confused by this print.
> AFAICT this prints the warning when hugepagesz= refers to a hugepage
> that is already added via hugetlb_add_hstate, but there is a default
> hstate added here without the user specifying hugepagesz, no?
Correct.
> Does
> that mean the warning prints if you specify this size?
The code which adds the default hstate (in hugetlb_init) runs after this
code which is reading/processing command line options. So, the case you
are concerned with will not happen.
Thanks for taking a look,
--
Mike Kravetz
> 'HugeTLB: hugepage size already supported: xxxxx' or
> 'HugeTLB: hugepage size xxxx specified twice or is default size, ignoring.'
>
> Or don't print anything if it's the default size.
>
>> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
>> return 1;
>> }
>> --
>> 2.24.1
^ permalink raw reply
* Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size
From: Mina Almasry @ 2020-03-23 23:43 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S . Miller
In-Reply-To: <20200318220634.32100-2-mike.kravetz@oracle.com>
On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> The architecture independent routine hugetlb_default_setup sets up
> the default huge pages size. It has no way to verify if the passed
> value is valid, so it accepts it and attempts to validate at a later
> time. This requires undocumented cooperation between the arch specific
> and arch independent code.
>
> For architectures that support more than one huge page size, provide
> a routine arch_hugetlb_valid_size to validate a huge page size.
> hugetlb_default_setup can use this to validate passed values.
>
> arch_hugetlb_valid_size will also be used in a subsequent patch to
> move processing of the "hugepagesz=" in arch specific code to a common
> routine in arch independent code.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
> arch/arm64/include/asm/hugetlb.h | 2 ++
> arch/arm64/mm/hugetlbpage.c | 19 ++++++++++++++-----
> arch/powerpc/include/asm/hugetlb.h | 3 +++
> arch/powerpc/mm/hugetlbpage.c | 20 +++++++++++++-------
> arch/riscv/include/asm/hugetlb.h | 3 +++
> arch/riscv/mm/hugetlbpage.c | 28 ++++++++++++++++++----------
> arch/s390/include/asm/hugetlb.h | 3 +++
> arch/s390/mm/hugetlbpage.c | 18 +++++++++++++-----
> arch/sparc/include/asm/hugetlb.h | 3 +++
> arch/sparc/mm/init_64.c | 23 ++++++++++++++++-------
> arch/x86/include/asm/hugetlb.h | 3 +++
> arch/x86/mm/hugetlbpage.c | 21 +++++++++++++++------
> include/linux/hugetlb.h | 7 +++++++
> mm/hugetlb.c | 16 +++++++++++++---
> 14 files changed, 126 insertions(+), 43 deletions(-)
>
With build fixes:
Acked-by: Mina Almasry <almasrymina@google.com>
^ permalink raw reply
* Re: [PATCH 2/4] hugetlbfs: move hugepagesz= parsing to arch independent code
From: Mina Almasry @ 2020-03-23 23:56 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S . Miller, Mike Kravetz
In-Reply-To: <2ca058dc-47e6-1d08-154b-77d2cbe98e34@c-s.fr>
On Thu, Mar 19, 2020 at 12:04 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 18/03/2020 à 23:06, Mike Kravetz a écrit :
> > Now that architectures provide arch_hugetlb_valid_size(), parsing
> > of "hugepagesz=" can be done in architecture independent code.
> > Create a single routine to handle hugepagesz= parsing and remove
> > all arch specific routines. We can also remove the interface
> > hugetlb_bad_size() as this is no longer used outside arch independent
> > code.
> >
> > This also provides consistent behavior of hugetlbfs command line
> > options. The hugepagesz= option should only be specified once for
> > a specific size, but some architectures allow multiple instances.
> > This appears to be more of an oversight when code was added by some
> > architectures to set up ALL huge pages sizes.
> >
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > ---
> > arch/arm64/mm/hugetlbpage.c | 15 ---------------
> > arch/powerpc/mm/hugetlbpage.c | 15 ---------------
> > arch/riscv/mm/hugetlbpage.c | 16 ----------------
> > arch/s390/mm/hugetlbpage.c | 18 ------------------
> > arch/sparc/mm/init_64.c | 22 ----------------------
> > arch/x86/mm/hugetlbpage.c | 16 ----------------
> > include/linux/hugetlb.h | 1 -
> > mm/hugetlb.c | 24 ++++++++++++++++++------
> > 8 files changed, 18 insertions(+), 109 deletions(-)
> >
>
> [snip]
>
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 2f99359b93af..cd4ec07080fb 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -3149,12 +3149,6 @@ static int __init hugetlb_init(void)
> > }
> > subsys_initcall(hugetlb_init);
> >
> > -/* Should be called on processing a hugepagesz=... option */
> > -void __init hugetlb_bad_size(void)
> > -{
> > - parsed_valid_hugepagesz = false;
> > -}
> > -
> > void __init hugetlb_add_hstate(unsigned int order)
> > {
> > struct hstate *h;
> > @@ -3224,6 +3218,24 @@ static int __init hugetlb_nrpages_setup(char *s)
> > }
> > __setup("hugepages=", hugetlb_nrpages_setup);
> >
> > +static int __init hugepagesz_setup(char *s)
> > +{
> > + unsigned long long size;
> > + char *saved_s = s;
> > +
> > + size = memparse(s, &s);
>
> You don't use s after that, so you can pass NULL instead of &s and avoid
> the saved_s
>
+1
Acked-by: Mina Almasry <almasrymina@google.com>
^ permalink raw reply
* Re: [PATCH 3/4] hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
From: Mina Almasry @ 2020-03-24 0:01 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S . Miller
In-Reply-To: <20200318220634.32100-4-mike.kravetz@oracle.com>
On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> The routine hugetlb_add_hstate prints a warning if the hstate already
> exists. This was originally done as part of kernel command line
> parsing. If 'hugepagesz=' was specified more than once, the warning
> pr_warn("hugepagesz= specified twice, ignoring\n");
> would be printed.
>
> Some architectures want to enable all huge page sizes. They would
> call hugetlb_add_hstate for all supported sizes. However, this was
> done after command line processing and as a result hstates could have
> already been created for some sizes. To make sure no warning were
> printed, there would often be code like:
> if (!size_to_hstate(size)
> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT)
>
> The only time we want to print the warning is as the result of command
> line processing. So, remove the warning from hugetlb_add_hstate and
> add it to the single arch independent routine processing "hugepagesz=".
> After this, calls to size_to_hstate() in arch specific code can be
> removed and hugetlb_add_hstate can be called without worrying about
> warning messages.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
> arch/arm64/mm/hugetlbpage.c | 16 ++++------------
> arch/powerpc/mm/hugetlbpage.c | 3 +--
> arch/riscv/mm/hugetlbpage.c | 2 +-
> arch/sparc/mm/init_64.c | 19 ++++---------------
> arch/x86/mm/hugetlbpage.c | 2 +-
> mm/hugetlb.c | 10 +++++++---
> 6 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 4aa9534a45d7..050809e6f0a9 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -441,22 +441,14 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
> clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
> }
>
> -static void __init add_huge_page_size(unsigned long size)
> -{
> - if (size_to_hstate(size))
> - return;
> -
> - hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
> -}
> -
> static int __init hugetlbpage_init(void)
> {
> #ifdef CONFIG_ARM64_4K_PAGES
> - add_huge_page_size(PUD_SIZE);
> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> #endif
> - add_huge_page_size(CONT_PMD_SIZE);
> - add_huge_page_size(PMD_SIZE);
> - add_huge_page_size(CONT_PTE_SIZE);
> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
>
> return 0;
> }
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index 166960ba1236..f46464ba6fb4 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -584,8 +584,7 @@ static int __init add_huge_page_size(unsigned long long size)
> if (!arch_hugetlb_valid_size(size))
> return -EINVAL;
>
> - if (!size_to_hstate(size))
> - hugetlb_add_hstate(shift - PAGE_SHIFT);
> + hugetlb_add_hstate(shift - PAGE_SHIFT);
> return 0;
> }
>
> diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c
> index bdf89d7eb714..beaa91941db8 100644
> --- a/arch/riscv/mm/hugetlbpage.c
> +++ b/arch/riscv/mm/hugetlbpage.c
> @@ -26,7 +26,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
> static __init int gigantic_pages_init(void)
> {
> /* With CONTIG_ALLOC, we can allocate gigantic pages at runtime */
> - if (IS_ENABLED(CONFIG_64BIT) && !size_to_hstate(1UL << PUD_SHIFT))
> + if (IS_ENABLED(CONFIG_64BIT))
> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> return 0;
> }
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 5c29203fd460..8f619edc8f8c 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -325,23 +325,12 @@ static void __update_mmu_tsb_insert(struct mm_struct *mm, unsigned long tsb_inde
> }
>
> #ifdef CONFIG_HUGETLB_PAGE
> -static void __init add_huge_page_size(unsigned long size)
> -{
> - unsigned int order;
> -
> - if (size_to_hstate(size))
> - return;
> -
> - order = ilog2(size) - PAGE_SHIFT;
> - hugetlb_add_hstate(order);
> -}
> -
> static int __init hugetlbpage_init(void)
> {
> - add_huge_page_size(1UL << HPAGE_64K_SHIFT);
> - add_huge_page_size(1UL << HPAGE_SHIFT);
> - add_huge_page_size(1UL << HPAGE_256MB_SHIFT);
> - add_huge_page_size(1UL << HPAGE_2GB_SHIFT);
> + hugetlb_add_hstate(HPAGE_64K_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(HPAGE_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(HPAGE_256MB_SHIFT - PAGE_SHIFT);
> + hugetlb_add_hstate(HPAGE_2GB_SHIFT - PAGE_SHIFT);
>
> return 0;
> }
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index dd3ed09f6c23..8a3f586e1217 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -195,7 +195,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
> static __init int gigantic_pages_init(void)
> {
> /* With compaction or CMA we can allocate gigantic pages at runtime */
> - if (boot_cpu_has(X86_FEATURE_GBPAGES) && !size_to_hstate(1UL << PUD_SHIFT))
> + if (boot_cpu_has(X86_FEATURE_GBPAGES))
> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> return 0;
> }
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cd4ec07080fb..cc85b4f156ca 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3116,8 +3116,7 @@ static int __init hugetlb_init(void)
> }
>
> default_hstate_size = HPAGE_SIZE;
> - if (!size_to_hstate(default_hstate_size))
> - hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> + hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> }
> default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size));
> if (default_hstate_max_huge_pages) {
> @@ -3155,7 +3154,6 @@ void __init hugetlb_add_hstate(unsigned int order)
> unsigned long i;
>
> if (size_to_hstate(PAGE_SIZE << order)) {
> - pr_warn("hugepagesz= specified twice, ignoring\n");
> return;
> }
> BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
> @@ -3231,6 +3229,12 @@ static int __init hugepagesz_setup(char *s)
> return 0;
> }
>
> + if (size_to_hstate(size)) {
> + pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n",
> + saved_s);
> + return 0;
> + }
> +
Not too familiar with the code but I'm a bit confused by this print.
AFAICT this prints the warning when hugepagesz= refers to a hugepage
that is already added via hugetlb_add_hstate, but there is a default
hstate added here without the user specifying hugepagesz, no? Does
that mean the warning prints if you specify this size? Maybe then
clarify the message like:
'HugeTLB: hugepage size already supported: xxxxx' or
'HugeTLB: hugepage size xxxx specified twice or is default size, ignoring.'
Or don't print anything if it's the default size.
> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
> return 1;
> }
> --
> 2.24.1
>
>
^ permalink raw reply
* Re: [PATCH 3/4] hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
From: Mina Almasry @ 2020-03-24 0:23 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S.Miller
In-Reply-To: <54fd7234-bdd9-de78-9351-16706bae2c08@oracle.com>
On Mon, Mar 23, 2020 at 5:17 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 3/23/20 5:01 PM, Mina Almasry wrote:
> > On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
> >>
> >> The routine hugetlb_add_hstate prints a warning if the hstate already
> >> exists. This was originally done as part of kernel command line
> >> parsing. If 'hugepagesz=' was specified more than once, the warning
> >> pr_warn("hugepagesz= specified twice, ignoring\n");
> >> would be printed.
> >>
> >> Some architectures want to enable all huge page sizes. They would
> >> call hugetlb_add_hstate for all supported sizes. However, this was
> >> done after command line processing and as a result hstates could have
> >> already been created for some sizes. To make sure no warning were
> >> printed, there would often be code like:
> >> if (!size_to_hstate(size)
> >> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT)
> >>
> >> The only time we want to print the warning is as the result of command
> >> line processing. So, remove the warning from hugetlb_add_hstate and
> >> add it to the single arch independent routine processing "hugepagesz=".
> >> After this, calls to size_to_hstate() in arch specific code can be
> >> removed and hugetlb_add_hstate can be called without worrying about
> >> warning messages.
> >>
> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> >> ---
> >> arch/arm64/mm/hugetlbpage.c | 16 ++++------------
> >> arch/powerpc/mm/hugetlbpage.c | 3 +--
> >> arch/riscv/mm/hugetlbpage.c | 2 +-
> >> arch/sparc/mm/init_64.c | 19 ++++---------------
> >> arch/x86/mm/hugetlbpage.c | 2 +-
> >> mm/hugetlb.c | 10 +++++++---
> >> 6 files changed, 18 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >> index 4aa9534a45d7..050809e6f0a9 100644
> >> --- a/arch/arm64/mm/hugetlbpage.c
> >> +++ b/arch/arm64/mm/hugetlbpage.c
> >> @@ -441,22 +441,14 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
> >> clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
> >> }
> >>
> >> -static void __init add_huge_page_size(unsigned long size)
> >> -{
> >> - if (size_to_hstate(size))
> >> - return;
> >> -
> >> - hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
> >> -}
> >> -
> >> static int __init hugetlbpage_init(void)
> >> {
> >> #ifdef CONFIG_ARM64_4K_PAGES
> >> - add_huge_page_size(PUD_SIZE);
> >> + hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
> >> #endif
> >> - add_huge_page_size(CONT_PMD_SIZE);
> >> - add_huge_page_size(PMD_SIZE);
> >> - add_huge_page_size(CONT_PTE_SIZE);
> >> + hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
> >> + hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);
> >>
> >> return 0;
> >> }
> >> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> >> index 166960ba1236..f46464ba6fb4 100644
> >> --- a/arch/powerpc/mm/hugetlbpage.c
> >> +++ b/arch/powerpc/mm/hugetlbpage.c
> >> @@ -584,8 +584,7 @@ static int __init add_huge_page_size(unsigned long long size)
> >> if (!arch_hugetlb_valid_size(size))
> >> return -EINVAL;
> >>
> >> - if (!size_to_hstate(size))
> >> - hugetlb_add_hstate(shift - PAGE_SHIFT);
> >> + hugetlb_add_hstate(shift - PAGE_SHIFT);
> >> return 0;
> >> }
> >>
> >> diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c
> >> index bdf89d7eb714..beaa91941db8 100644
> >> --- a/arch/riscv/mm/hugetlbpage.c
> >> +++ b/arch/riscv/mm/hugetlbpage.c
> >> @@ -26,7 +26,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
> >> static __init int gigantic_pages_init(void)
> >> {
> >> /* With CONTIG_ALLOC, we can allocate gigantic pages at runtime */
> >> - if (IS_ENABLED(CONFIG_64BIT) && !size_to_hstate(1UL << PUD_SHIFT))
> >> + if (IS_ENABLED(CONFIG_64BIT))
> >> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> >> return 0;
> >> }
> >> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> >> index 5c29203fd460..8f619edc8f8c 100644
> >> --- a/arch/sparc/mm/init_64.c
> >> +++ b/arch/sparc/mm/init_64.c
> >> @@ -325,23 +325,12 @@ static void __update_mmu_tsb_insert(struct mm_struct *mm, unsigned long tsb_inde
> >> }
> >>
> >> #ifdef CONFIG_HUGETLB_PAGE
> >> -static void __init add_huge_page_size(unsigned long size)
> >> -{
> >> - unsigned int order;
> >> -
> >> - if (size_to_hstate(size))
> >> - return;
> >> -
> >> - order = ilog2(size) - PAGE_SHIFT;
> >> - hugetlb_add_hstate(order);
> >> -}
> >> -
> >> static int __init hugetlbpage_init(void)
> >> {
> >> - add_huge_page_size(1UL << HPAGE_64K_SHIFT);
> >> - add_huge_page_size(1UL << HPAGE_SHIFT);
> >> - add_huge_page_size(1UL << HPAGE_256MB_SHIFT);
> >> - add_huge_page_size(1UL << HPAGE_2GB_SHIFT);
> >> + hugetlb_add_hstate(HPAGE_64K_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(HPAGE_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(HPAGE_256MB_SHIFT - PAGE_SHIFT);
> >> + hugetlb_add_hstate(HPAGE_2GB_SHIFT - PAGE_SHIFT);
> >>
> >> return 0;
> >> }
> >> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> >> index dd3ed09f6c23..8a3f586e1217 100644
> >> --- a/arch/x86/mm/hugetlbpage.c
> >> +++ b/arch/x86/mm/hugetlbpage.c
> >> @@ -195,7 +195,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
> >> static __init int gigantic_pages_init(void)
> >> {
> >> /* With compaction or CMA we can allocate gigantic pages at runtime */
> >> - if (boot_cpu_has(X86_FEATURE_GBPAGES) && !size_to_hstate(1UL << PUD_SHIFT))
> >> + if (boot_cpu_has(X86_FEATURE_GBPAGES))
> >> hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
> >> return 0;
> >> }
> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> >> index cd4ec07080fb..cc85b4f156ca 100644
> >> --- a/mm/hugetlb.c
> >> +++ b/mm/hugetlb.c
> >> @@ -3116,8 +3116,7 @@ static int __init hugetlb_init(void)
> >> }
> >>
> >> default_hstate_size = HPAGE_SIZE;
> >> - if (!size_to_hstate(default_hstate_size))
> >> - hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> >> + hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> >> }
> >> default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size));
> >> if (default_hstate_max_huge_pages) {
> >> @@ -3155,7 +3154,6 @@ void __init hugetlb_add_hstate(unsigned int order)
> >> unsigned long i;
> >>
> >> if (size_to_hstate(PAGE_SIZE << order)) {
> >> - pr_warn("hugepagesz= specified twice, ignoring\n");
> >> return;
> >> }
> >> BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
> >> @@ -3231,6 +3229,12 @@ static int __init hugepagesz_setup(char *s)
> >> return 0;
> >> }
> >>
> >> + if (size_to_hstate(size)) {
> >> + pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n",
> >> + saved_s);
> >> + return 0;
> >> + }
> >> +
> >
> > Not too familiar with the code but I'm a bit confused by this print.
> > AFAICT this prints the warning when hugepagesz= refers to a hugepage
> > that is already added via hugetlb_add_hstate, but there is a default
> > hstate added here without the user specifying hugepagesz, no?
>
> Correct.
>
> > Does
> > that mean the warning prints if you specify this size?
>
> The code which adds the default hstate (in hugetlb_init) runs after this
> code which is reading/processing command line options. So, the case you
> are concerned with will not happen.
>
> Thanks for taking a look,
> --
> Mike Kravetz
>
> > 'HugeTLB: hugepage size already supported: xxxxx' or
> > 'HugeTLB: hugepage size xxxx specified twice or is default size, ignoring.'
> >
> > Or don't print anything if it's the default size.
> >
> >> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
> >> return 1;
> >> }
> >> --
> >> 2.24.1
Acked-By: Mina Almasry <almasrymina@google.com>
^ permalink raw reply
* Re: [PATCH 4/4] hugetlbfs: clean up command line processing
From: Mina Almasry @ 2020-03-24 0:43 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Longpeng,
Albert Ou, Vasily Gorbik, Paul Walmsley, Thomas Gleixner,
linux-arm-kernel, open list, Palmer Dabbelt, Andrew Morton,
linuxppc-dev, David S . Miller
In-Reply-To: <20200318220634.32100-5-mike.kravetz@oracle.com>
On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> With all hugetlb page processing done in a single file clean up code.
Now that all hugepage page processing is done in a single file, clean
up the code.
> - Make code match desired semantics
> - Update documentation with semantics
> - Make all warnings and errors messages start with 'HugeTLB:'.
> - Consistently name command line parsing routines.
> - Add comments to code
> - Describe some of the subtle interactions
> - Describe semantics of command line arguments
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
> Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++
> mm/hugetlb.c | 78 +++++++++++++++-----
> 2 files changed, 87 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/hugetlbpage.rst b/Documentation/admin-guide/mm/hugetlbpage.rst
> index 1cc0bc78d10e..afc8888f33c3 100644
> --- a/Documentation/admin-guide/mm/hugetlbpage.rst
> +++ b/Documentation/admin-guide/mm/hugetlbpage.rst
> @@ -100,6 +100,32 @@ with a huge page size selection parameter "hugepagesz=<size>". <size> must
> be specified in bytes with optional scale suffix [kKmMgG]. The default huge
> page size may be selected with the "default_hugepagesz=<size>" boot parameter.
>
> +Hugetlb boot command line parameter semantics
> +hugepagesz - Specify a huge page size. Used in conjunction with hugepages
> + parameter to preallocate a number of huge pages of the specified
> + size. Hence, hugepagesz and hugepages are typically specified in
> + pairs such as:
> + hugepagesz=2M hugepages=512
> + hugepagesz can only be specified once on the command line for a
> + specific huge page size. Valid huge page sizes are architecture
> + dependent.
> +hugepages - Specify the number of huge pages to preallocate. This typically
> + follows a valid hugepagesz parameter. However, if hugepages is the
> + first or only hugetlb command line parameter it specifies the number
> + of huge pages of default size to allocate. The number of huge pages
> + of default size specified in this manner can be overwritten by a
> + hugepagesz,hugepages parameter pair for the default size.
> + For example, on an architecture with 2M default huge page size:
> + hugepages=256 hugepagesz=2M hugepages=512
> + will result in 512 2M huge pages being allocated. If a hugepages
> + parameter is preceded by an invalid hugepagesz parameter, it will
> + be ignored.
> +default_hugepagesz - Specify the default huge page size. This parameter can
> + only be specified on the command line. No other hugetlb command line
> + parameter is associated with default_hugepagesz. Therefore, it can
> + appear anywhere on the command line. Valid default huge page size is
> + architecture dependent.
Maybe specify what happens/should happen in a case like:
hugepages=100 default_hugepagesz=1G
Does that allocate 100 2MB pages or 100 1G pages? Assuming the default
size is 2MB.
Also, regarding Randy's comment. It may be nice to keep these docs in
one place only, so we don't have to maintain 2 docs in sync.
> +
> When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages``
> indicates the current number of pre-allocated huge pages of the default size.
> Thus, one can use the following command to dynamically allocate/deallocate
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cc85b4f156ca..2b9bf01db2b6 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2954,7 +2954,7 @@ static void __init hugetlb_sysfs_init(void)
> err = hugetlb_sysfs_add_hstate(h, hugepages_kobj,
> hstate_kobjs, &hstate_attr_group);
> if (err)
> - pr_err("Hugetlb: Unable to add hstate %s", h->name);
> + pr_err("HugeTLB: Unable to add hstate %s", h->name);
> }
> }
>
> @@ -3058,7 +3058,7 @@ static void hugetlb_register_node(struct node *node)
> nhs->hstate_kobjs,
> &per_node_hstate_attr_group);
> if (err) {
> - pr_err("Hugetlb: Unable to add hstate %s for node %d\n",
> + pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
> h->name, node->dev.id);
> hugetlb_unregister_node(node);
> break;
> @@ -3109,19 +3109,35 @@ static int __init hugetlb_init(void)
> if (!hugepages_supported())
> return 0;
>
> - if (!size_to_hstate(default_hstate_size)) {
> - if (default_hstate_size != 0) {
> - pr_err("HugeTLB: unsupported default_hugepagesz %lu. Reverting to %lu\n",
> - default_hstate_size, HPAGE_SIZE);
> - }
> -
> + /*
> + * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some
> + * architectures depend on setup being done here.
> + *
> + * If a valid default huge page size was specified on the command line,
> + * add associated hstate if necessary. If not, set default_hstate_size
> + * to default size. default_hstate_idx is used at runtime to identify
> + * the default huge page size/hstate.
> + */
> + hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> + if (default_hstate_size)
> + hugetlb_add_hstate(ilog2(default_hstate_size) - PAGE_SHIFT);
> + else
> default_hstate_size = HPAGE_SIZE;
> - hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
> - }
> default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size));
> +
> + /*
> + * default_hstate_max_huge_pages != 0 indicates a count (hugepages=)
> + * specified before a size (hugepagesz=). Use this count for the
> + * default huge page size, unless a specific value was specified for
> + * this size in a hugepagesz/hugepages pair.
> + */
> if (default_hstate_max_huge_pages) {
> if (!default_hstate.max_huge_pages)
> - default_hstate.max_huge_pages = default_hstate_max_huge_pages;
> + default_hstate.max_huge_pages =
> + default_hstate_max_huge_pages;
> + else
> + pr_warn("HugeTLB: First hugepages=%lu kB ignored\n",
> + default_hstate_max_huge_pages);
> }
>
> hugetlb_init_hstates();
> @@ -3174,20 +3190,27 @@ void __init hugetlb_add_hstate(unsigned int order)
> parsed_hstate = h;
> }
>
> -static int __init hugetlb_nrpages_setup(char *s)
> +/*
> + * hugepages command line processing
> + * hugepages must normally follows a valid hugepagsz specification. If not,
'hugepages must' or 'hugepages normally follows'
> + * ignore the hugepages value. hugepages can also be the first huge page
> + * command line option in which case it specifies the number of huge pages
> + * for the default size.
> + */
> +static int __init hugepages_setup(char *s)
> {
> unsigned long *mhp;
> static unsigned long *last_mhp;
>
> if (!parsed_valid_hugepagesz) {
> - pr_warn("hugepages = %s preceded by "
> + pr_warn("HugeTLB: hugepages = %s preceded by "
> "an unsupported hugepagesz, ignoring\n", s);
> parsed_valid_hugepagesz = true;
> return 1;
> }
> /*
> - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
> - * so this hugepages= parameter goes to the "default hstate".
> + * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter
> + * yet, so this hugepages= parameter goes to the "default hstate".
> */
> else if (!hugetlb_max_hstate)
> mhp = &default_hstate_max_huge_pages;
We don't set parsed_valid_hugepagesz to false at the end of this
function, shouldn't we? Parsing a hugepages= value should 'consume' a
previously defined hugepagesz= value, so that this is invalid IIUC:
hugepagesz=x hugepages=z hugepages=y
> @@ -3195,7 +3218,8 @@ static int __init hugetlb_nrpages_setup(char *s)
> mhp = &parsed_hstate->max_huge_pages;
>
> if (mhp == last_mhp) {
> - pr_warn("hugepages= specified twice without interleaving hugepagesz=, ignoring\n");
> + pr_warn("HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=%s\n",
> + s);
> return 1;
> }
>
> @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s)
>
> return 1;
> }
> -__setup("hugepages=", hugetlb_nrpages_setup);
> +__setup("hugepages=", hugepages_setup);
>
> +/*
> + * hugepagesz command line processing
> + * A specific huge page size can only be specified once with hugepagesz.
> + * hugepagesz is followed by hugepages on the commnad line. The global
> + * variable 'parsed_valid_hugepagesz' is used to determine if prior
> + * hugepagesz argument was valid.
> + */
> static int __init hugepagesz_setup(char *s)
> {
> unsigned long long size;
> @@ -3230,16 +3261,23 @@ static int __init hugepagesz_setup(char *s)
> }
>
> if (size_to_hstate(size)) {
> + parsed_valid_hugepagesz = false;
> pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n",
> saved_s);
> return 0;
> }
>
> + parsed_valid_hugepagesz = true;
> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
> return 1;
> }
> __setup("hugepagesz=", hugepagesz_setup);
>
> +/*
> + * default_hugepagesz command line input
> + * Only one instance of default_hugepagesz allowed on command line. Do not
> + * add hstate here as that will confuse hugepagesz/hugepages processing.
> + */
> static int __init default_hugepagesz_setup(char *s)
> {
> unsigned long long size;
> @@ -3252,6 +3290,12 @@ static int __init default_hugepagesz_setup(char *s)
> return 0;
> }
>
> + if (default_hstate_size) {
> + pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n",
> + saved_s);
> + return 0;
> + }
> +
> default_hstate_size = size;
> return 1;
> }
> --
> 2.24.1
>
>
^ permalink raw reply
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls
From: Paul Mackerras @ 2020-03-23 23:43 UTC (permalink / raw)
To: Greg Kurz
Cc: linux-kernel, kvm-ppc, Bharata B Rao, Laurent Dufour,
linuxppc-dev
In-Reply-To: <20200320132248.44b81b3b@bahia.lan>
On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote:
> On Fri, 20 Mar 2020 11:26:42 +0100
> Laurent Dufour <ldufour@linux.ibm.com> wrote:
>
> > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing
> > prevent a malicious VM or SVM to call them. This could lead to weird result
> > and should be filtered out.
> >
> > Checking the Secure bit of the calling MSR ensure that the call is coming
> > from either the Ultravisor or a SVM. But any system call made from a SVM
> > are going through the Ultravisor, and the Ultravisor should filter out
> > these malicious call. This way, only the Ultravisor is able to make such a
> > Hcall.
>
> "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ?
>
> Shouldn't we also check the HV bit of the calling MSR as well to
> disambiguate SVM and UV ?
The trouble with doing that (checking the HV bit) is that KVM does not
expect to see the HV bit set on an interrupt that occurred while we
were in the guest, and if it is set, it indicates a serious problem,
i.e. that an interrupt occurred while we were in the code that
transitions from host context to guest context, or from guest context
to host context. In those cases we don't know how much of the
transition has been completed and therefore whether we have guest
values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs).
If we do see HV set then KVM reports a severe error to userspace which
should cause userspace to terminate the guest.
Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1
when transitioning to KVM.
Paul.
^ permalink raw reply
* Re: [PATCH v3] ocxl: control via sysfs whether the FPGA is reloaded on a link reset
From: Andrew Donnellan @ 2020-03-24 1:09 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev, clombard, felix, alastair
In-Reply-To: <20200318100210.80035-1-fbarrat@linux.ibm.com>
On 18/3/20 9:02 pm, Frederic Barrat wrote:
> From: Philippe Bergheaud <felix@linux.ibm.com>
>
> Some opencapi FPGA images allow to control if the FPGA should be reloaded
> on the next adapter reset. If it is supported, the image specifies it
> through a Vendor Specific DVSEC in the config space of function 0.
>
> Signed-off-by: Philippe Bergheaud <felix@linux.ibm.com>
As I've raised privately - I think we need an additional identifier
within the Vendor-Specific DVSEC to distinguish between the IBM
reference implementation of the CFG subsystem and alternative
implementations that may use the Vendor-Specific DVSEC for other purposes.
Further comments below.
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index c8e19bfb5ef9..b364b6ceb996 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -71,6 +71,20 @@ static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
> return 0;
> }
>
> +/**
> + * get_function_0() - Find a related PCI device (function 0)
> + * @device: PCI device to match
> + *
> + * Returns a pointer to the related device, or null if not found
> + */
> +static struct pci_dev *get_function_0(struct pci_dev *dev)
> +{
> + unsigned int devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
> +
> + return pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
> + dev->bus->number, devfn);
> +}
> +
> static void read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
> {
> u16 val;
> @@ -159,7 +173,7 @@ static int read_dvsec_afu_info(struct pci_dev *dev, struct ocxl_fn_config *fn)
> static int read_dvsec_vendor(struct pci_dev *dev)
> {
> int pos;
> - u32 cfg, tlx, dlx;
> + u32 cfg, tlx, dlx, reset_reload;
>
> /*
> * vendor specific DVSEC is optional
> @@ -183,6 +197,58 @@ static int read_dvsec_vendor(struct pci_dev *dev)
> dev_dbg(&dev->dev, " CFG version = 0x%x\n", cfg);
> dev_dbg(&dev->dev, " TLX version = 0x%x\n", tlx);
> dev_dbg(&dev->dev, " DLX version = 0x%x\n", dlx);
> +
> + if (ocxl_config_get_reset_reload(dev, &reset_reload) != 0)
> + dev_dbg(&dev->dev, " ResetReload is not available\n");
> + else
> + dev_dbg(&dev->dev, " ResetReload = 0x%x\n", reset_reload);
> + return 0;
> +}
> +
> +int ocxl_config_get_reset_reload(struct pci_dev *dev, int *val)
> +{
> + int reset_reload = -1;
Would prefer if this was a u32, to match the type in the
pci_read_config_dword() signature
> + int pos = 0;
> + struct pci_dev *dev0 = dev;
> +
> + if (PCI_FUNC(dev->devfn) != 0)
> + dev0 = get_function_0(dev);
> +
> + if (dev0)
> + pos = find_dvsec(dev0, OCXL_DVSEC_VENDOR_ID);
> +
> + if (pos)
> + pci_read_config_dword(dev0,
> + pos + OCXL_DVSEC_VENDOR_RESET_RELOAD,
> + &reset_reload);
> + if (reset_reload == -1)
> + return reset_reload;
Can we safely assume that 0xFFFFFFFF is never going to be a valid value
for this dword? The document I'm looking at only states that bits 31:1
are reserved, not that any of them are guaranteed to be a 0 in a future
revision.
> +
> + *val = reset_reload & BIT(0);
> + return 0;
> +}
> +
> +int ocxl_config_set_reset_reload(struct pci_dev *dev, int val)
> +{
> + int reset_reload = -1;
> + int pos = 0;
> + struct pci_dev *dev0 = dev;
> +
> + if (PCI_FUNC(dev->devfn) != 0)
> + dev0 = get_function_0(dev);
> +
> + if (dev0)
> + pos = find_dvsec(dev0, OCXL_DVSEC_VENDOR_ID);
> +
> + if (pos)
> + pci_read_config_dword(dev0,
> + pos + OCXL_DVSEC_VENDOR_RESET_RELOAD,
> + &reset_reload);
> + if (reset_reload == -1)
> + return reset_reload;
> +
> + val &= BIT(0);
I think we want to keep the existing value of reserved bits.
Something like
val = (val & BIT(0)) | (reset_reload & ~BIT(0));
?
> + pci_write_config_dword(dev0, pos + OCXL_DVSEC_VENDOR_RESET_RELOAD, val);
> return 0;
> }
>
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 345bf843a38e..af9a84aeee6f 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -112,6 +112,12 @@ void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> */
> int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
>
> +/*
> + * Control whether the FPGA is reloaded on a link reset
> + */
> +int ocxl_config_get_reset_reload(struct pci_dev *dev, int *val);
> +int ocxl_config_set_reset_reload(struct pci_dev *dev, int val);
> +
> /*
> * Check if an AFU index is valid for the given function.
> *
> diff --git a/drivers/misc/ocxl/sysfs.c b/drivers/misc/ocxl/sysfs.c
> index 58f1ba264206..8f69f7311343 100644
> --- a/drivers/misc/ocxl/sysfs.c
> +++ b/drivers/misc/ocxl/sysfs.c
> @@ -51,11 +51,46 @@ static ssize_t contexts_show(struct device *device,
> afu->pasid_count, afu->pasid_max);
> }
>
> +static ssize_t reload_on_reset_show(struct device *device,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ocxl_afu *afu = to_afu(device);
> + struct ocxl_fn *fn = afu->fn;
> + struct pci_dev *pci_dev = to_pci_dev(fn->dev.parent);
> + int val;
> +
> + if (ocxl_config_get_reset_reload(pci_dev, &val))
> + return scnprintf(buf, PAGE_SIZE, "unavailable\n");
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t reload_on_reset_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ocxl_afu *afu = to_afu(device);
> + struct ocxl_fn *fn = afu->fn;
> + struct pci_dev *pci_dev = to_pci_dev(fn->dev.parent);
> + int rc, val;
> +
> + rc = sscanf(buf, "%i", &val);
Checkpatch suggests turning this into kstrtoint()
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/12312//artifact/linux/checkpatch.log
> + if ((rc != 1) || !(val == 1 || val == 0))
> + return -EINVAL;
> +
> + if (ocxl_config_set_reset_reload(pci_dev, val))
> + return -ENODEV;
> +
> + return count;
> +}
> +
> static struct device_attribute afu_attrs[] = {
> __ATTR_RO(global_mmio_size),
> __ATTR_RO(pp_mmio_size),
> __ATTR_RO(afu_version),
> __ATTR_RO(contexts),
> + __ATTR_RW(reload_on_reset),
> };
>
> static ssize_t global_mmio_read(struct file *filp, struct kobject *kobj,
> diff --git a/include/misc/ocxl-config.h b/include/misc/ocxl-config.h
> index 3526fa996a22..ccfd3b463517 100644
> --- a/include/misc/ocxl-config.h
> +++ b/include/misc/ocxl-config.h
> @@ -41,5 +41,6 @@
> #define OCXL_DVSEC_VENDOR_CFG_VERS 0x0C
> #define OCXL_DVSEC_VENDOR_TLX_VERS 0x10
> #define OCXL_DVSEC_VENDOR_DLX_VERS 0x20
> +#define OCXL_DVSEC_VENDOR_RESET_RELOAD 0x38
>
> #endif /* _OCXL_CONFIG_H_ */
>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH v2] libnvdimm: Update persistence domain value for of_pmem and papr_scm device
From: Dan Williams @ 2020-03-24 1:21 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, linux-nvdimm
In-Reply-To: <87k13fnzt8.fsf@linux.ibm.com>
On Fri, Mar 20, 2020 at 2:25 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
>
> Hi Dan,
>
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> ...
>
>
> >
> >>
> >> Or are you suggesting that application should not infer any of those
> >> details looking at persistence_domain value? If so what is the purpose
> >> of exporting that attribute?
> >
> > The way the patch was worded I thought it was referring to an explicit
> > mechanism outside cpu cache flushes, i.e. a mechanism that required a
> > driver call.
> >
>
> This patch is blocked because I am not expressing the details correctly.
> I updates this as below. Can you suggest if this is ok? If not what
> alternate wording do you suggest to document "memory controller"
>
>
> commit 329b46e88f8cd30eee4776b0de7913ab4d496bd8
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Date: Wed Dec 18 13:53:16 2019 +0530
>
> libnvdimm: Update persistence domain value for of_pmem and papr_scm device
>
> Currently, kernel shows the below values
> "persistence_domain":"cpu_cache"
> "persistence_domain":"memory_controller"
> "persistence_domain":"unknown"
>
> "cpu_cache" indicates no extra instructions is needed to ensure the persistence
> of data in the pmem media on power failure.
>
> "memory_controller" indicates cpu cache flush instructions is required to flush
> the data. Platform provides mechanisms to automatically flush outstanding
> write data from memory controler to pmem on system power loss.
>
> Based on the above use memory_controller for non volatile regions on ppc64.
Looks good to me, want to resend via git-format-patch?
^ permalink raw reply
* Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address
From: Oliver O'Halloran @ 2020-03-24 2:26 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Michael Neuling, Herbert Xu, Frederic Barrat, Haren Myneni,
Nicholas Piggin, Christoph Hellwig, Sukadev Bhattiprolu,
linuxppc-dev, Andrew Donnellan
In-Reply-To: <bd846a9c-0e21-1d97-0b03-e01c35ff01ae@kaod.org>
On Mon, Mar 23, 2020 at 8:28 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 3/23/20 10:06 AM, Cédric Le Goater wrote:
> > On 3/19/20 7:14 AM, Haren Myneni wrote:
> >>
> >> Alloc IRQ and get trigger port address for each VAS instance. Kernel
> >> register this IRQ per VAS instance and sets this port for each send
> >> window. NX interrupts the kernel when it sees page fault.
> >
> > I don't understand why this is not done by the OPAL driver for each VAS
> > of the system. Is the VAS unit very different from OpenCAPI regarding
> > the fault ?
>
> I checked the previous patchsets and I see that v3 was more like I expected
> it: one interrupt for faults allocated by the skiboot driver and exposed
> in the DT.
>
> What made you change your mind ?
From init_vas_inst() in arch/powerpc/platforms/powernv/vas.c:
if (pdev->num_resources != 4) {
pr_err("Unexpected DT configuration for [%s, %d]\n",
pdev->name, vasid);
return -ENODEV;
}
This code should never have been written, but here we are. Due to the
above adding an interrupt in the DT makes the driver unable to bind on
older kernels. In an older version of the patches (don't think it was
posted) Haren was using a non-standard interrupt property and we could
work around the problem by going back to that.
However, we already have the OPAL calls for allocating / freeing
hardware interrupt numbers so why not do that? If we ever want to take
advantage of the job completion interrupts we'd want to have the
ability to allocate them since the completion interrupts are
per-window rather than per-VAS.
> This version is hijacking the lowlevel routines of the XIVE irqchip which
> is not the best approach. OCXL is doing that because it needs to allocate
> interrupts for the user space processes using the AFU and we should rework
> that part.
What'd you have in mind for the reworking the oxcl interrupt
allocation? I didn't find it that objectionable since it's more or
less the same as what happens when allocating IPIs.
> However, the translation fault interrupt is allocated by skiboot.
>
> Sorry for the noise, I would like to understand more how this works. I also
> have passthrough in mind.
>
> C.
^ permalink raw reply
* Re: [PATCH v4 00/16] Initial Prefixed Instruction support
From: Jordan Niethe @ 2020-03-24 2:54 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584957138.lj5a68bk6x.astroid@bobo.none>
On Mon, Mar 23, 2020 at 9:21 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 23, 2020 7:25 pm:
> > On Mon, Mar 23, 2020 at 5:22 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Jordan Niethe's on March 20, 2020 3:17 pm:
> >> > A future revision of the ISA will introduce prefixed instructions. A
> >> > prefixed instruction is composed of a 4-byte prefix followed by a
> >> > 4-byte suffix.
> >> >
> >> > All prefixes have the major opcode 1. A prefix will never be a valid
> >> > word instruction. A suffix may be an existing word instruction or a
> >> > new instruction.
> >> >
> >> > This series enables prefixed instructions and extends the instruction
> >> > emulation to support them. Then the places where prefixed instructions
> >> > might need to be emulated are updated.
> >> >
> >> > The series is based on top of:
> >> > https://patchwork.ozlabs.org/patch/1232619/ as this will effect
> >> > kprobes.
> >> >
> >> > v4 is based on feedback from Nick Piggins, Christophe Leroy and Daniel Axtens.
> >> > The major changes:
> >> > - Move xmon breakpoints from data section to text section
> >> > - Introduce a data type for instructions on powerpc
> >>
> >> Thanks for doing this, looks like a lot of work, I hope it works out :)
> >>
> > Yes it did end up touching a lot of places. I started thinking that
> > that maybe it would be simpler to just use a u64 instead of the struct
> > for instructions.
> > If we always keep the word instruction / prefix in the lower bytes,
> > all of the current masking should still work and we can use operators
> > again instead of ppc_inst_equal(), etc.
>
> Yeah.. I think now that you've done it, I prefer it this way.
Sorry, just to be clear which way do you mean?
>
> > It also makes printing easier. We could just #define INST_FMT %llx or
> > #define INST_FMT %x on powerpc32 and use that for printing out
> > instructions.
>
> Well, not sure about that. Would it make endian concerns more
> complicated? Print format for prefix might be '%016llx', but we
> don't want that for all instructions only prefixed ones, and I
> don't know if that is the way to go either.
Hm yeah that is true.
>
> We'll want to adopt some convention for displaying prefixed
> instruction bytes, but I don't know what what works best. I wonder
> if binutils or any userspace tools have a convention.
binutils-gdb upstream has supports disassembling prefixed instructions.
Here is what objdump looks like:
44: 00 00 00 60 nop
48: 00 00 00 07 pnop
4c: 00 00 00 00
50: 01 00 20 39 li r9,1
54: 00 00 00 06 paddi r4,r9,3
58: 03 00 89 38
5c: 00 00 62 3c addis r3,r2,0
>
> Which reminds me, you might have missed show_instructions()?
> Although maybe you don't need that until we start using them in
> the kernel.
You are right I missed that here.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: Skip kvmppc_uvmem_free if Ultravisor is not supported
From: Paul Mackerras @ 2020-03-24 2:56 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: linuxppc-dev, groug, kvm-ppc, bharata
In-Reply-To: <20200319225510.945603-1-farosas@linux.ibm.com>
On Thu, Mar 19, 2020 at 07:55:10PM -0300, Fabiano Rosas wrote:
> kvmppc_uvmem_init checks for Ultravisor support and returns early if
> it is not present. Calling kvmppc_uvmem_free at module exit will cause
> an Oops:
>
> $ modprobe -r kvm-hv
>
> Oops: Kernel access of bad area, sig: 11 [#1]
> <snip>
> NIP: c000000000789e90 LR: c000000000789e8c CTR: c000000000401030
> REGS: c000003fa7bab9a0 TRAP: 0300 Not tainted (5.6.0-rc6-00033-g6c90b86a745a-dirty)
> MSR: 9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 24002282 XER: 00000000
> CFAR: c000000000dae880 DAR: 0000000000000008 DSISR: 40000000 IRQMASK: 1
> GPR00: c000000000789e8c c000003fa7babc30 c0000000016fe500 0000000000000000
> GPR04: 0000000000000000 0000000000000006 0000000000000000 c000003faf205c00
> GPR08: 0000000000000000 0000000000000001 000000008000002d c00800000ddde140
> GPR12: c000000000401030 c000003ffffd9080 0000000000000001 0000000000000000
> GPR16: 0000000000000000 0000000000000000 000000013aad0074 000000013aaac978
> GPR20: 000000013aad0070 0000000000000000 00007fffd1b37158 0000000000000000
> GPR24: 000000014fef0d58 0000000000000000 000000014fef0cf0 0000000000000001
> GPR28: 0000000000000000 0000000000000000 c0000000018b2a60 0000000000000000
> NIP [c000000000789e90] percpu_ref_kill_and_confirm+0x40/0x170
> LR [c000000000789e8c] percpu_ref_kill_and_confirm+0x3c/0x170
> Call Trace:
> [c000003fa7babc30] [c000003faf2064d4] 0xc000003faf2064d4 (unreliable)
> [c000003fa7babcb0] [c000000000400e8c] dev_pagemap_kill+0x6c/0x80
> [c000003fa7babcd0] [c000000000401064] memunmap_pages+0x34/0x2f0
> [c000003fa7babd50] [c00800000dddd548] kvmppc_uvmem_free+0x30/0x80 [kvm_hv]
> [c000003fa7babd80] [c00800000ddcef18] kvmppc_book3s_exit_hv+0x20/0x78 [kvm_hv]
> [c000003fa7babda0] [c0000000002084d0] sys_delete_module+0x1d0/0x2c0
> [c000003fa7babe20] [c00000000000b9d0] system_call+0x5c/0x68
> Instruction dump:
> 3fc2001b fb81ffe0 fba1ffe8 fbe1fff8 7c7f1b78 7c9c2378 3bde4560 7fc3f378
> f8010010 f821ff81 486249a1 60000000 <e93f0008> 7c7d1b78 712a0002 40820084
> ---[ end trace 5774ef4dc2c98279 ]---
>
> So this patch checks if kvmppc_uvmem_init actually allocated anything
> before running kvmppc_uvmem_free.
>
> Fixes: ca9f4942670c ("KVM: PPC: Book3S HV: Support for running secure guests")
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Thanks, applied to my kvm-ppc-next branch (Michael Ellerman decided
that he didn't need to take it as the crash only occurs with
CONFIG_PPC_UV=n, which is not the default).
Paul.
^ permalink raw reply
* Re: [PATCH 0/2] Fix SVM hang at startup
From: Paul Mackerras @ 2020-03-24 2:54 UTC (permalink / raw)
To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20200320102643.15516-1-ldufour@linux.ibm.com>
On Fri, Mar 20, 2020 at 11:26:41AM +0100, Laurent Dufour wrote:
> This series is fixing a SVM hang occurring when starting a SVM requiring
> more secure memory than available. The hang happens in the SVM when calling
> UV_ESM.
>
> The following is happening:
>
> 1. SVM calls UV_ESM
> 2. Ultravisor (UV) calls H_SVM_INIT_START
> 3. Hypervisor (HV) calls UV_REGISTER_MEM_SLOT
> 4. UV returns error because there is not enough free secure memory
> 5. HV enter the error path in kvmppc_h_svm_init_start()
> 6. In the return path, since kvm->arch.secure_guest is not yet set hrfid is
> called
> 7. As the HV doesn't know the SVM calling context hrfid is jumping to
> unknown address in the SVM leading to various expections.
>
> This series fixes the setting of kvm->arch.secure_guest in
> kvmppc_h_svm_init_start() to ensure that UV_RETURN is called on the return
> path to get back to the UV.
>
> In addition to ensure that a malicious VM will not call UV reserved Hcall,
> a check of the Secure bit in the calling MSR is addded to reject such a
> call.
>
> It is assumed that the UV will filtered out such Hcalls made by a malicious
> SVM.
>
> Laurent Dufour (2):
> KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls
> KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN
>
> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++----------
> arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++-
> 2 files changed, 23 insertions(+), 12 deletions(-)
Thanks, series applied to my kvm-ppc-next branch.
Paul.
^ permalink raw reply
* Re: [PATCH v4 03/16] powerpc: Use a datatype for instructions
From: Michael Ellerman @ 2020-03-24 2:58 UTC (permalink / raw)
To: Nicholas Piggin, Jordan Niethe
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
In-Reply-To: <1584956855.oidzv2r2fr.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Jordan Niethe's on March 23, 2020 7:28 pm:
>> On Mon, Mar 23, 2020 at 5:27 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>> Jordan Niethe's on March 20, 2020 3:17 pm:
>>> > Currently unsigned ints are used to represent instructions on powerpc.
>>> > This has worked well as instructions have always been 4 byte words.
>>> > However, a future ISA version will introduce some changes to
>>> > instructions that mean this scheme will no longer work as well. This
>>> > change is Prefixed Instructions. A prefixed instruction is made up of a
>>> > word prefix followed by a word suffix to make an 8 byte double word
>>> > instruction. No matter the endianess of the system the prefix always
>>> > comes first. Prefixed instructions are only planned for powerpc64.
>>> >
>>> > Introduce a ppc_inst type to represent both prefixed and word
>>> > instructions on powerpc64 while keeping it possible to exclusively have
>>> > word instructions on powerpc32, A latter patch will expand the type to
>>> > include prefixed instructions but for now just typedef it to a u32.
>>> >
>>> > Later patches will introduce helper functions and macros for
>>> > manipulating the instructions so that powerpc64 and powerpc32 might
>>> > maintain separate type definitions.
>>>
>>> ppc_inst_t I would slightly prefer for a typedef like this.
>> Are _t types meant to be reserved?
>
> No, just convention that structs are not normally typedefed unless
> they are a pervasive interface that gets passed around a lot but
> does not get accessed without accessor functions much. When you do
> typedef them, add a _t (or less frequently _s/_u/etc). pte_t,
> cpumask_t, atomic_t.
Ideally we wouldn't use a typedef, we'd just have:
struct ppc_inst {
u32 val;
#ifdef CONFIG_PPC64
u32 suffix;
#endif
};
That may make the conversion harder though, because you more or less
have to update all usages at once.
cheers
^ permalink raw reply
* Re: [PATCH v4 00/16] Initial Prefixed Instruction support
From: Jordan Niethe @ 2020-03-24 2:59 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <CACzsE9qrNpfvoLKfdeXths4rKJ8jQcUic3=dFZ57ntogdeaMug@mail.gmail.com>
On Tue, Mar 24, 2020 at 1:54 PM Jordan Niethe <jniethe5@gmail.com> wrote:
>
> On Mon, Mar 23, 2020 at 9:21 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > Jordan Niethe's on March 23, 2020 7:25 pm:
> > > On Mon, Mar 23, 2020 at 5:22 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> > >>
> > >> Jordan Niethe's on March 20, 2020 3:17 pm:
> > >> > A future revision of the ISA will introduce prefixed instructions. A
> > >> > prefixed instruction is composed of a 4-byte prefix followed by a
> > >> > 4-byte suffix.
> > >> >
> > >> > All prefixes have the major opcode 1. A prefix will never be a valid
> > >> > word instruction. A suffix may be an existing word instruction or a
> > >> > new instruction.
> > >> >
> > >> > This series enables prefixed instructions and extends the instruction
> > >> > emulation to support them. Then the places where prefixed instructions
> > >> > might need to be emulated are updated.
> > >> >
> > >> > The series is based on top of:
> > >> > https://patchwork.ozlabs.org/patch/1232619/ as this will effect
> > >> > kprobes.
> > >> >
> > >> > v4 is based on feedback from Nick Piggins, Christophe Leroy and Daniel Axtens.
> > >> > The major changes:
> > >> > - Move xmon breakpoints from data section to text section
> > >> > - Introduce a data type for instructions on powerpc
> > >>
> > >> Thanks for doing this, looks like a lot of work, I hope it works out :)
> > >>
> > > Yes it did end up touching a lot of places. I started thinking that
> > > that maybe it would be simpler to just use a u64 instead of the struct
> > > for instructions.
> > > If we always keep the word instruction / prefix in the lower bytes,
> > > all of the current masking should still work and we can use operators
> > > again instead of ppc_inst_equal(), etc.
> >
> > Yeah.. I think now that you've done it, I prefer it this way.
> Sorry, just to be clear which way do you mean?
> >
> > > It also makes printing easier. We could just #define INST_FMT %llx or
> > > #define INST_FMT %x on powerpc32 and use that for printing out
> > > instructions.
> >
> > Well, not sure about that. Would it make endian concerns more
> > complicated? Print format for prefix might be '%016llx', but we
> > don't want that for all instructions only prefixed ones, and I
> > don't know if that is the way to go either.
> Hm yeah that is true.
> >
> > We'll want to adopt some convention for displaying prefixed
> > instruction bytes, but I don't know what what works best. I wonder
> > if binutils or any userspace tools have a convention.
> binutils-gdb upstream has supports disassembling prefixed instructions.
> Here is what objdump looks like:
> 44: 00 00 00 60 nop
> 48: 00 00 00 07 pnop
> 4c: 00 00 00 00
> 50: 01 00 20 39 li r9,1
> 54: 00 00 00 06 paddi r4,r9,3
> 58: 03 00 89 38
> 5c: 00 00 62 3c addis r3,r2,0
And this is what it looks like if you use objdump with -w
44: 00 00 00 60 nop
48: 00 00 00 07 00 00 00 00 pnop
50: 01 00 20 39 li r9,1
54: 00 00 00 06 03 00 89 38 paddi r4,r9,3
5c: 00 00 62 3c addis r3,r2,0 5c:
R_PPC64_TOC16_HA .toc+0x10
> >
> > Which reminds me, you might have missed show_instructions()?
> > Although maybe you don't need that until we start using them in
> > the kernel.
> You are right I missed that here.
> >
> > Thanks,
> > Nick
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-24 3:05 UTC (permalink / raw)
To: Christoph Hellwig, Aneesh Kumar K.V
Cc: Greg Kroah-Hartman, Joerg Roedel, linuxppc-dev, linux-kernel,
iommu, Robin Murphy, Lu Baolu
In-Reply-To: <20200323172256.GB31269@lst.de>
On 24/03/2020 04:22, Christoph Hellwig wrote:
> On Mon, Mar 23, 2020 at 09:07:38PM +0530, Aneesh Kumar K.V wrote:
>>
>> This is what I was trying, but considering I am new to DMA subsystem, I
>> am not sure I got all the details correct. The idea is to look at the
>> cpu addr and see if that can be used in direct map fashion(is
>> bus_dma_limit the right restriction here?) if not fallback to dynamic
>> IOMMU mapping.
>
> I don't think we can throw all these complications into the dma
> mapping code. At some point I also wonder what the point is,
> especially for scatterlist mappings, where the iommu can coalesce.
This is for persistent memory which you can DMA to/from but yet it does
not appear in the system as a normal memory and therefore requires
special handling anyway (O_DIRECT or DAX, I do not know the exact
mechanics). All other devices in the system should just run as usual,
i.e. use 1:1 mapping if possible.
--
Alexey
^ permalink raw reply
* Re: [PATCH v4 08/16] powerpc: Use an accessor for word instructions
From: Jordan Niethe @ 2020-03-24 3:18 UTC (permalink / raw)
To: Balamuruhan S
Cc: Alistair Popple, linuxppc-dev, Nicholas Piggin, Daniel Axtens
In-Reply-To: <ffad7904022c281493da7e56036689e70d312c0b.camel@linux.ibm.com>
On Mon, Mar 23, 2020 at 10:13 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Fri, 2020-03-20 at 16:18 +1100, Jordan Niethe wrote:
> > In preparation for prefixed instructions where all instructions are no
> > longer words, use an accessor for getting a word instruction as a u32
> > from the instruction data type.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/kernel/align.c | 2 +-
> > arch/powerpc/kernel/kprobes.c | 2 +-
> > arch/powerpc/kernel/trace/ftrace.c | 16 +-
> > arch/powerpc/lib/code-patching.c | 2 +-
> > arch/powerpc/lib/feature-fixups.c | 4 +-
> > arch/powerpc/lib/sstep.c | 270 ++++++++++++++-------------
> > arch/powerpc/lib/test_emulate_step.c | 4 +-
> > arch/powerpc/xmon/xmon.c | 4 +-
> > 8 files changed, 153 insertions(+), 151 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> > index 77c49dfdc1b4..b246ca124931 100644
> > --- a/arch/powerpc/kernel/align.c
> > +++ b/arch/powerpc/kernel/align.c
> > @@ -309,7 +309,7 @@ int fix_alignment(struct pt_regs *regs)
> > /* We don't handle PPC little-endian any more... */
> > if (cpu_has_feature(CPU_FTR_PPC_LE))
> > return -EIO;
> > - instr = PPC_INST(swab32(instr));
> > + instr = PPC_INST(swab32(ppc_inst_word(instr)));
> > }
> >
> > #ifdef CONFIG_SPE
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 4c2b656615a6..0c600b6e4ead 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -242,7 +242,7 @@ static int try_to_emulate(struct kprobe *p, struct
> > pt_regs *regs)
> > * So, we should never get here... but, its still
> > * good to catch them, just in case...
> > */
> > - printk("Can't step on instruction %x\n", insn);
> > + printk("Can't step on instruction %x\n", ppc_inst_word(insn));
> > BUG();
> > } else {
> > /*
> > diff --git a/arch/powerpc/kernel/trace/ftrace.c
> > b/arch/powerpc/kernel/trace/ftrace.c
> > index b3645b664819..7614a9f537fd 100644
> > --- a/arch/powerpc/kernel/trace/ftrace.c
> > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > @@ -74,7 +74,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst old, ppc_inst
> > new)
> > /* Make sure it is what we expect it to be */
> > if (!ppc_inst_equal(replaced, old)) {
> > pr_err("%p: replaced (%#x) != old (%#x)",
> > - (void *)ip, replaced, old);
> > + (void *)ip, ppc_inst_word(replaced), ppc_inst_word(old));
> > return -EINVAL;
> > }
> >
> > @@ -136,7 +136,7 @@ __ftrace_make_nop(struct module *mod,
> >
> > /* Make sure that that this is still a 24bit jump */
> > if (!is_bl_op(op)) {
> > - pr_err("Not expected bl: opcode is %x\n", op);
> > + pr_err("Not expected bl: opcode is %x\n", ppc_inst_word(op));
> > return -EINVAL;
> > }
> >
> > @@ -171,7 +171,7 @@ __ftrace_make_nop(struct module *mod,
> > /* We expect either a mflr r0, or a std r0, LRSAVE(r1) */
> > if (!ppc_inst_equal(op, PPC_INST(PPC_INST_MFLR)) &&
> > !ppc_inst_equal(op, PPC_INST(PPC_INST_STD_LR))) {
> > - pr_err("Unexpected instruction %08x around bl _mcount\n", op);
> > + pr_err("Unexpected instruction %08x around bl _mcount\n",
> > ppc_inst_word(op));
> > return -EINVAL;
> > }
> > #else
> > @@ -201,7 +201,7 @@ __ftrace_make_nop(struct module *mod,
> > }
> >
> > if (!ppc_inst_equal(op, PPC_INST(PPC_INST_LD_TOC))) {
> > - pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, op);
> > + pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC,
> > ppc_inst_word(op));
> > return -EINVAL;
> > }
> > #endif /* CONFIG_MPROFILE_KERNEL */
> > @@ -401,7 +401,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace
> > *rec, unsigned long addr)
> >
> > /* Make sure that that this is still a 24bit jump */
> > if (!is_bl_op(op)) {
> > - pr_err("Not expected bl: opcode is %x\n", op);
> > + pr_err("Not expected bl: opcode is %x\n", ppc_inst_word(op));
> > return -EINVAL;
> > }
> >
> > @@ -525,7 +525,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long
> > addr)
> >
> > if (!expected_nop_sequence(ip, op[0], op[1])) {
> > pr_err("Unexpected call sequence at %p: %x %x\n",
> > - ip, op[0], op[1]);
> > + ip, ppc_inst_word(op[0]), ppc_inst_word(op[1]));
> > return -EINVAL;
> > }
> >
> > @@ -644,7 +644,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace
> > *rec, unsigned long addr)
> > }
> >
> > if (!ppc_inst_equal(op, PPC_INST(PPC_INST_NOP))) {
> > - pr_err("Unexpected call sequence at %p: %x\n", ip, op);
> > + pr_err("Unexpected call sequence at %p: %x\n", ip,
> > ppc_inst_word(op));
> > return -EINVAL;
> > }
> >
> > @@ -723,7 +723,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned
> > long old_addr,
> >
> > /* Make sure that that this is still a 24bit jump */
> > if (!is_bl_op(op)) {
> > - pr_err("Not expected bl: opcode is %x\n", op);
> > + pr_err("Not expected bl: opcode is %x\n", ppc_inst_word(op));
> > return -EINVAL;
> > }
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-
> > patching.c
> > index ec3abe1a6927..849eee63df3d 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -233,7 +233,7 @@ bool is_conditional_branch(ppc_inst instr)
> > if (opcode == 16) /* bc, bca, bcl, bcla */
> > return true;
> > if (opcode == 19) {
> > - switch ((instr >> 1) & 0x3ff) {
> > + switch ((ppc_inst_word(instr) >> 1) & 0x3ff) {
> > case 16: /* bclr, bclrl */
> > case 528: /* bcctr, bcctrl */
> > case 560: /* bctar, bctarl */
> > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-
> > fixups.c
> > index 552106d1f64a..fe8ec099aa96 100644
> > --- a/arch/powerpc/lib/feature-fixups.c
> > +++ b/arch/powerpc/lib/feature-fixups.c
> > @@ -54,8 +54,8 @@ static int patch_alt_instruction(unsigned int *src,
> > unsigned int *dest,
> >
> > /* Branch within the section doesn't need translating */
> > if (target < alt_start || target > alt_end) {
> > - instr = translate_branch(dest, src);
> > - if (ppc_inst_null(instr))
> > + instr = ppc_inst_word(translate_branch((ppc_inst
> > *)dest, (ppc_inst *)src));
> > + if (!instr)
> > return 1;
> > }
> > }
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index 1d9c766a89fe..bae878a83fa5 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -1169,26 +1169,28 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > unsigned long int imm;
> > unsigned long int val, val2;
> > unsigned int mb, me, sh;
> > + unsigned int word;
> > long ival;
> >
> > + word = ppc_inst_word(instr);
> > op->type = COMPUTE;
> >
> > - opcode = instr >> 26;
> > + opcode = word >> 26;
> > switch (opcode) {
> > case 16: /* bc */
> > op->type = BRANCH;
> > - imm = (signed short)(instr & 0xfffc);
> > - if ((instr & 2) == 0)
> > + imm = (signed short)(word & 0xfffc);
> > + if ((word & 2) == 0)
> > imm += regs->nip;
> > op->val = truncate_if_32bit(regs->msr, imm);
> > - if (instr & 1)
> > + if (word & 1)
> > op->type |= SETLK;
> > - if (branch_taken(instr, regs, op))
> > + if (branch_taken(word, regs, op))
> > op->type |= BRTAKEN;
> > return 1;
> > #ifdef CONFIG_PPC64
> > case 17: /* sc */
> > - if ((instr & 0xfe2) == 2)
> > + if ((word & 0xfe2) == 2)
> > op->type = SYSCALL;
> > else
> > op->type = UNKNOWN;
> > @@ -1196,21 +1198,21 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > #endif
> > case 18: /* b */
> > op->type = BRANCH | BRTAKEN;
> > - imm = instr & 0x03fffffc;
> > + imm = word & 0x03fffffc;
> > if (imm & 0x02000000)
> > imm -= 0x04000000;
> > - if ((instr & 2) == 0)
> > + if ((word & 2) == 0)
> > imm += regs->nip;
> > op->val = truncate_if_32bit(regs->msr, imm);
> > - if (instr & 1)
> > + if (word & 1)
> > op->type |= SETLK;
> > return 1;
> > case 19:
> > - switch ((instr >> 1) & 0x3ff) {
> > + switch ((word >> 1) & 0x3ff) {
> > case 0: /* mcrf */
> > op->type = COMPUTE + SETCC;
> > - rd = 7 - ((instr >> 23) & 0x7);
> > - ra = 7 - ((instr >> 18) & 0x7);
> > + rd = 7 - ((word >> 23) & 0x7);
> > + ra = 7 - ((word >> 18) & 0x7);
> > rd *= 4;
> > ra *= 4;
> > val = (regs->ccr >> ra) & 0xf;
> > @@ -1220,11 +1222,11 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 16: /* bclr */
> > case 528: /* bcctr */
> > op->type = BRANCH;
> > - imm = (instr & 0x400)? regs->ctr: regs->link;
> > + imm = (word & 0x400)? regs->ctr: regs->link;
> > op->val = truncate_if_32bit(regs->msr, imm);
> > - if (instr & 1)
> > + if (word & 1)
> > op->type |= SETLK;
> > - if (branch_taken(instr, regs, op))
> > + if (branch_taken(word, regs, op))
> > op->type |= BRTAKEN;
> > return 1;
> >
> > @@ -1247,23 +1249,23 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 417: /* crorc */
> > case 449: /* cror */
> > op->type = COMPUTE + SETCC;
> > - ra = (instr >> 16) & 0x1f;
> > - rb = (instr >> 11) & 0x1f;
> > - rd = (instr >> 21) & 0x1f;
> > + ra = (word >> 16) & 0x1f;
> > + rb = (word >> 11) & 0x1f;
> > + rd = (word >> 21) & 0x1f;
>
> can't we use your accessors for all these operations ?
I felt that since we are doing so many bit operations here it was
simpliest to just leave these as uints.
>
> > ra = (regs->ccr >> (31 - ra)) & 1;
> > rb = (regs->ccr >> (31 - rb)) & 1;
> > - val = (instr >> (6 + ra * 2 + rb)) & 1;
> > + val = (word >> (6 + ra * 2 + rb)) & 1;
> > op->ccval = (regs->ccr & ~(1UL << (31 - rd))) |
> > (val << (31 - rd));
> > return 1;
> > }
> > break;
> > case 31:
> > - switch ((instr >> 1) & 0x3ff) {
> > + switch ((word >> 1) & 0x3ff) {
> > case 598: /* sync */
> > op->type = BARRIER + BARRIER_SYNC;
> > #ifdef __powerpc64__
> > - switch ((instr >> 21) & 3) {
> > + switch ((word >> 21) & 3) {
> > case 1: /* lwsync */
> > op->type = BARRIER + BARRIER_LWSYNC;
> > break;
> > @@ -1285,20 +1287,20 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > if (!FULL_REGS(regs))
> > return -1;
> >
> > - rd = (instr >> 21) & 0x1f;
> > - ra = (instr >> 16) & 0x1f;
> > - rb = (instr >> 11) & 0x1f;
> > - rc = (instr >> 6) & 0x1f;
> > + rd = (word >> 21) & 0x1f;
> > + ra = (word >> 16) & 0x1f;
> > + rb = (word >> 11) & 0x1f;
> > + rc = (word >> 6) & 0x1f;
>
> same here and in similar such places.
>
> -- Bala
> >
> > switch (opcode) {
> > #ifdef __powerpc64__
> > case 2: /* tdi */
> > - if (rd & trap_compare(regs->gpr[ra], (short) instr))
> > + if (rd & trap_compare(regs->gpr[ra], (short) word))
> > goto trap;
> > return 1;
> > #endif
> > case 3: /* twi */
> > - if (rd & trap_compare((int)regs->gpr[ra], (short) instr))
> > + if (rd & trap_compare((int)regs->gpr[ra], (short) word))
> > goto trap;
> > return 1;
> >
> > @@ -1307,7 +1309,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > if (!cpu_has_feature(CPU_FTR_ARCH_300))
> > return -1;
> >
> > - switch (instr & 0x3f) {
> > + switch (word & 0x3f) {
> > case 48: /* maddhd */
> > asm volatile(PPC_MADDHD(%0, %1, %2, %3) :
> > "=r" (op->val) : "r" (regs->gpr[ra]),
> > @@ -1335,16 +1337,16 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > #endif
> >
> > case 7: /* mulli */
> > - op->val = regs->gpr[ra] * (short) instr;
> > + op->val = regs->gpr[ra] * (short) word;
> > goto compute_done;
> >
> > case 8: /* subfic */
> > - imm = (short) instr;
> > + imm = (short) word;
> > add_with_carry(regs, op, rd, ~regs->gpr[ra], imm, 1);
> > return 1;
> >
> > case 10: /* cmpli */
> > - imm = (unsigned short) instr;
> > + imm = (unsigned short) word;
> > val = regs->gpr[ra];
> > #ifdef __powerpc64__
> > if ((rd & 1) == 0)
> > @@ -1354,7 +1356,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 1;
> >
> > case 11: /* cmpi */
> > - imm = (short) instr;
> > + imm = (short) word;
> > val = regs->gpr[ra];
> > #ifdef __powerpc64__
> > if ((rd & 1) == 0)
> > @@ -1364,35 +1366,35 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 1;
> >
> > case 12: /* addic */
> > - imm = (short) instr;
> > + imm = (short) word;
> > add_with_carry(regs, op, rd, regs->gpr[ra], imm, 0);
> > return 1;
> >
> > case 13: /* addic. */
> > - imm = (short) instr;
> > + imm = (short) word;
> > add_with_carry(regs, op, rd, regs->gpr[ra], imm, 0);
> > set_cr0(regs, op);
> > return 1;
> >
> > case 14: /* addi */
> > - imm = (short) instr;
> > + imm = (short) word;
> > if (ra)
> > imm += regs->gpr[ra];
> > op->val = imm;
> > goto compute_done;
> >
> > case 15: /* addis */
> > - imm = ((short) instr) << 16;
> > + imm = ((short) word) << 16;
> > if (ra)
> > imm += regs->gpr[ra];
> > op->val = imm;
> > goto compute_done;
> >
> > case 19:
> > - if (((instr >> 1) & 0x1f) == 2) {
> > + if (((word >> 1) & 0x1f) == 2) {
> > /* addpcis */
> > - imm = (short) (instr & 0xffc1); /* d0 + d2 fields */
> > - imm |= (instr >> 15) & 0x3e; /* d1 field */
> > + imm = (short) (word & 0xffc1); /* d0 + d2 fields */
> > + imm |= (word >> 15) & 0x3e; /* d1 field */
> > op->val = regs->nip + (imm << 16) + 4;
> > goto compute_done;
> > }
> > @@ -1400,65 +1402,65 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 0;
> >
> > case 20: /* rlwimi */
> > - mb = (instr >> 6) & 0x1f;
> > - me = (instr >> 1) & 0x1f;
> > + mb = (word >> 6) & 0x1f;
> > + me = (word >> 1) & 0x1f;
> > val = DATA32(regs->gpr[rd]);
> > imm = MASK32(mb, me);
> > op->val = (regs->gpr[ra] & ~imm) | (ROTATE(val, rb) & imm);
> > goto logical_done;
> >
> > case 21: /* rlwinm */
> > - mb = (instr >> 6) & 0x1f;
> > - me = (instr >> 1) & 0x1f;
> > + mb = (word >> 6) & 0x1f;
> > + me = (word >> 1) & 0x1f;
> > val = DATA32(regs->gpr[rd]);
> > op->val = ROTATE(val, rb) & MASK32(mb, me);
> > goto logical_done;
> >
> > case 23: /* rlwnm */
> > - mb = (instr >> 6) & 0x1f;
> > - me = (instr >> 1) & 0x1f;
> > + mb = (word >> 6) & 0x1f;
> > + me = (word >> 1) & 0x1f;
> > rb = regs->gpr[rb] & 0x1f;
> > val = DATA32(regs->gpr[rd]);
> > op->val = ROTATE(val, rb) & MASK32(mb, me);
> > goto logical_done;
> >
> > case 24: /* ori */
> > - op->val = regs->gpr[rd] | (unsigned short) instr;
> > + op->val = regs->gpr[rd] | (unsigned short) word;
> > goto logical_done_nocc;
> >
> > case 25: /* oris */
> > - imm = (unsigned short) instr;
> > + imm = (unsigned short) word;
> > op->val = regs->gpr[rd] | (imm << 16);
> > goto logical_done_nocc;
> >
> > case 26: /* xori */
> > - op->val = regs->gpr[rd] ^ (unsigned short) instr;
> > + op->val = regs->gpr[rd] ^ (unsigned short) word;
> > goto logical_done_nocc;
> >
> > case 27: /* xoris */
> > - imm = (unsigned short) instr;
> > + imm = (unsigned short) word;
> > op->val = regs->gpr[rd] ^ (imm << 16);
> > goto logical_done_nocc;
> >
> > case 28: /* andi. */
> > - op->val = regs->gpr[rd] & (unsigned short) instr;
> > + op->val = regs->gpr[rd] & (unsigned short) word;
> > set_cr0(regs, op);
> > goto logical_done_nocc;
> >
> > case 29: /* andis. */
> > - imm = (unsigned short) instr;
> > + imm = (unsigned short) word;
> > op->val = regs->gpr[rd] & (imm << 16);
> > set_cr0(regs, op);
> > goto logical_done_nocc;
> >
> > #ifdef __powerpc64__
> > case 30: /* rld* */
> > - mb = ((instr >> 6) & 0x1f) | (instr & 0x20);
> > + mb = ((word >> 6) & 0x1f) | (word & 0x20);
> > val = regs->gpr[rd];
> > - if ((instr & 0x10) == 0) {
> > - sh = rb | ((instr & 2) << 4);
> > + if ((word & 0x10) == 0) {
> > + sh = rb | ((word & 2) << 4);
> > val = ROTATE(val, sh);
> > - switch ((instr >> 2) & 3) {
> > + switch ((word >> 2) & 3) {
> > case 0: /* rldicl */
> > val &= MASK64_L(mb);
> > break;
> > @@ -1478,7 +1480,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > } else {
> > sh = regs->gpr[rb] & 0x3f;
> > val = ROTATE(val, sh);
> > - switch ((instr >> 1) & 7) {
> > + switch ((word >> 1) & 7) {
> > case 0: /* rldcl */
> > op->val = val & MASK64_L(mb);
> > goto logical_done;
> > @@ -1493,8 +1495,8 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > case 31:
> > /* isel occupies 32 minor opcodes */
> > - if (((instr >> 1) & 0x1f) == 15) {
> > - mb = (instr >> 6) & 0x1f; /* bc field */
> > + if (((word >> 1) & 0x1f) == 15) {
> > + mb = (word >> 6) & 0x1f; /* bc field */
> > val = (regs->ccr >> (31 - mb)) & 1;
> > val2 = (ra) ? regs->gpr[ra] : 0;
> >
> > @@ -1502,7 +1504,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > goto compute_done;
> > }
> >
> > - switch ((instr >> 1) & 0x3ff) {
> > + switch ((word >> 1) & 0x3ff) {
> > case 4: /* tw */
> > if (rd == 0x1f ||
> > (rd & trap_compare((int)regs->gpr[ra],
> > @@ -1536,17 +1538,17 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > op->reg = rd;
> > /* only MSR_EE and MSR_RI get changed if bit 15 set */
> > /* mtmsrd doesn't change MSR_HV, MSR_ME or MSR_LE */
> > - imm = (instr & 0x10000)? 0x8002: 0xefffffffffffeffeUL;
> > + imm = (word & 0x10000)? 0x8002: 0xefffffffffffeffeUL;
> > op->val = imm;
> > return 0;
> > #endif
> >
> > case 19: /* mfcr */
> > imm = 0xffffffffUL;
> > - if ((instr >> 20) & 1) {
> > + if ((word >> 20) & 1) {
> > imm = 0xf0000000UL;
> > for (sh = 0; sh < 8; ++sh) {
> > - if (instr & (0x80000 >> sh))
> > + if (word & (0x80000 >> sh))
> > break;
> > imm >>= 4;
> > }
> > @@ -1560,7 +1562,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > val = regs->gpr[rd];
> > op->ccval = regs->ccr;
> > for (sh = 0; sh < 8; ++sh) {
> > - if (instr & (0x80000 >> sh))
> > + if (word & (0x80000 >> sh))
> > op->ccval = (op->ccval & ~imm) |
> > (val & imm);
> > imm >>= 4;
> > @@ -1568,7 +1570,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 1;
> >
> > case 339: /* mfspr */
> > - spr = ((instr >> 16) & 0x1f) | ((instr >> 6) & 0x3e0);
> > + spr = ((word >> 16) & 0x1f) | ((word >> 6) & 0x3e0);
> > op->type = MFSPR;
> > op->reg = rd;
> > op->spr = spr;
> > @@ -1578,7 +1580,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 0;
> >
> > case 467: /* mtspr */
> > - spr = ((instr >> 16) & 0x1f) | ((instr >> 6) & 0x3e0);
> > + spr = ((word >> 16) & 0x1f) | ((word >> 6) & 0x3e0);
> > op->type = MTSPR;
> > op->val = regs->gpr[rd];
> > op->spr = spr;
> > @@ -1948,7 +1950,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 826: /* sradi with sh_5 = 0 */
> > case 827: /* sradi with sh_5 = 1 */
> > op->type = COMPUTE + SETREG + SETXER;
> > - sh = rb | ((instr & 2) << 4);
> > + sh = rb | ((word & 2) << 4);
> > ival = (signed long int) regs->gpr[rd];
> > op->val = ival >> sh;
> > op->xerval = regs->xer;
> > @@ -1964,7 +1966,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > if (!cpu_has_feature(CPU_FTR_ARCH_300))
> > return -1;
> > op->type = COMPUTE + SETREG;
> > - sh = rb | ((instr & 2) << 4);
> > + sh = rb | ((word & 2) << 4);
> > val = (signed int) regs->gpr[rd];
> > if (sh)
> > op->val = ROTATE(val, sh) & MASK64(0, 63 - sh);
> > @@ -1979,34 +1981,34 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > */
> > case 54: /* dcbst */
> > op->type = MKOP(CACHEOP, DCBST, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > return 0;
> >
> > case 86: /* dcbf */
> > op->type = MKOP(CACHEOP, DCBF, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > return 0;
> >
> > case 246: /* dcbtst */
> > op->type = MKOP(CACHEOP, DCBTST, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > op->reg = rd;
> > return 0;
> >
> > case 278: /* dcbt */
> > op->type = MKOP(CACHEOP, DCBTST, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > op->reg = rd;
> > return 0;
> >
> > case 982: /* icbi */
> > op->type = MKOP(CACHEOP, ICBI, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > return 0;
> >
> > case 1014: /* dcbz */
> > op->type = MKOP(CACHEOP, DCBZ, 0);
> > - op->ea = xform_ea(instr, regs);
> > + op->ea = xform_ea(word, regs);
> > return 0;
> > }
> > break;
> > @@ -2019,14 +2021,14 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > op->update_reg = ra;
> > op->reg = rd;
> > op->val = regs->gpr[rd];
> > - u = (instr >> 20) & UPDATE;
> > + u = (word >> 20) & UPDATE;
> > op->vsx_flags = 0;
> >
> > switch (opcode) {
> > case 31:
> > - u = instr & UPDATE;
> > - op->ea = xform_ea(instr, regs);
> > - switch ((instr >> 1) & 0x3ff) {
> > + u = word & UPDATE;
> > + op->ea = xform_ea(word, regs);
> > + switch ((word >> 1) & 0x3ff) {
> > case 20: /* lwarx */
> > op->type = MKOP(LARX, 0, 4);
> > break;
> > @@ -2271,25 +2273,25 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > #ifdef CONFIG_VSX
> > case 12: /* lxsiwzx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 4);
> > op->element_size = 8;
> > break;
> >
> > case 76: /* lxsiwax */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, SIGNEXT, 4);
> > op->element_size = 8;
> > break;
> >
> > case 140: /* stxsiwx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 4);
> > op->element_size = 8;
> > break;
> >
> > case 268: /* lxvx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 16;
> > op->vsx_flags = VSX_CHECK_VEC;
> > @@ -2298,33 +2300,33 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 269: /* lxvl */
> > case 301: { /* lxvll */
> > int nb;
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->ea = ra ? regs->gpr[ra] : 0;
> > nb = regs->gpr[rb] & 0xff;
> > if (nb > 16)
> > nb = 16;
> > op->type = MKOP(LOAD_VSX, 0, nb);
> > op->element_size = 16;
> > - op->vsx_flags = ((instr & 0x20) ? VSX_LDLEFT : 0) |
> > + op->vsx_flags = ((word & 0x20) ? VSX_LDLEFT : 0) |
> > VSX_CHECK_VEC;
> > break;
> > }
> > case 332: /* lxvdsx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 8);
> > op->element_size = 8;
> > op->vsx_flags = VSX_SPLAT;
> > break;
> >
> > case 364: /* lxvwsx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 4);
> > op->element_size = 4;
> > op->vsx_flags = VSX_SPLAT | VSX_CHECK_VEC;
> > break;
> >
> > case 396: /* stxvx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 16;
> > op->vsx_flags = VSX_CHECK_VEC;
> > @@ -2333,118 +2335,118 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 397: /* stxvl */
> > case 429: { /* stxvll */
> > int nb;
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->ea = ra ? regs->gpr[ra] : 0;
> > nb = regs->gpr[rb] & 0xff;
> > if (nb > 16)
> > nb = 16;
> > op->type = MKOP(STORE_VSX, 0, nb);
> > op->element_size = 16;
> > - op->vsx_flags = ((instr & 0x20) ? VSX_LDLEFT : 0) |
> > + op->vsx_flags = ((word & 0x20) ? VSX_LDLEFT : 0) |
> > VSX_CHECK_VEC;
> > break;
> > }
> > case 524: /* lxsspx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 4);
> > op->element_size = 8;
> > op->vsx_flags = VSX_FPCONV;
> > break;
> >
> > case 588: /* lxsdx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 8);
> > op->element_size = 8;
> > break;
> >
> > case 652: /* stxsspx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 4);
> > op->element_size = 8;
> > op->vsx_flags = VSX_FPCONV;
> > break;
> >
> > case 716: /* stxsdx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 8);
> > op->element_size = 8;
> > break;
> >
> > case 780: /* lxvw4x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 4;
> > break;
> >
> > case 781: /* lxsibzx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 1);
> > op->element_size = 8;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 812: /* lxvh8x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 2;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 813: /* lxsihzx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 2);
> > op->element_size = 8;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 844: /* lxvd2x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 8;
> > break;
> >
> > case 876: /* lxvb16x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 1;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 908: /* stxvw4x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 4;
> > break;
> >
> > case 909: /* stxsibx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 1);
> > op->element_size = 8;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 940: /* stxvh8x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 2;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 941: /* stxsihx */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 2);
> > op->element_size = 8;
> > op->vsx_flags = VSX_CHECK_VEC;
> > break;
> >
> > case 972: /* stxvd2x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 8;
> > break;
> >
> > case 1004: /* stxvb16x */
> > - op->reg = rd | ((instr & 1) << 5);
> > + op->reg = rd | ((word & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 1;
> > op->vsx_flags = VSX_CHECK_VEC;
> > @@ -2457,80 +2459,80 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 32: /* lwz */
> > case 33: /* lwzu */
> > op->type = MKOP(LOAD, u, 4);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 34: /* lbz */
> > case 35: /* lbzu */
> > op->type = MKOP(LOAD, u, 1);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 36: /* stw */
> > case 37: /* stwu */
> > op->type = MKOP(STORE, u, 4);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 38: /* stb */
> > case 39: /* stbu */
> > op->type = MKOP(STORE, u, 1);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 40: /* lhz */
> > case 41: /* lhzu */
> > op->type = MKOP(LOAD, u, 2);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 42: /* lha */
> > case 43: /* lhau */
> > op->type = MKOP(LOAD, SIGNEXT | u, 2);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 44: /* sth */
> > case 45: /* sthu */
> > op->type = MKOP(STORE, u, 2);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 46: /* lmw */
> > if (ra >= rd)
> > break; /* invalid form, ra in range to load
> > */
> > op->type = MKOP(LOAD_MULTI, 0, 4 * (32 - rd));
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 47: /* stmw */
> > op->type = MKOP(STORE_MULTI, 0, 4 * (32 - rd));
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > #ifdef CONFIG_PPC_FPU
> > case 48: /* lfs */
> > case 49: /* lfsu */
> > op->type = MKOP(LOAD_FP, u | FPCONV, 4);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 50: /* lfd */
> > case 51: /* lfdu */
> > op->type = MKOP(LOAD_FP, u, 8);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 52: /* stfs */
> > case 53: /* stfsu */
> > op->type = MKOP(STORE_FP, u | FPCONV, 4);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> >
> > case 54: /* stfd */
> > case 55: /* stfdu */
> > op->type = MKOP(STORE_FP, u, 8);
> > - op->ea = dform_ea(instr, regs);
> > + op->ea = dform_ea(word, regs);
> > break;
> > #endif
> >
> > @@ -2538,14 +2540,14 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > case 56: /* lq */
> > if (!((rd & 1) || (rd == ra)))
> > op->type = MKOP(LOAD, 0, 16);
> > - op->ea = dqform_ea(instr, regs);
> > + op->ea = dqform_ea(word, regs);
> > break;
> > #endif
> >
> > #ifdef CONFIG_VSX
> > case 57: /* lfdp, lxsd, lxssp */
> > - op->ea = dsform_ea(instr, regs);
> > - switch (instr & 3) {
> > + op->ea = dsform_ea(word, regs);
> > + switch (word & 3) {
> > case 0: /* lfdp */
> > if (rd & 1)
> > break; /* reg must be even */
> > @@ -2569,8 +2571,8 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > #ifdef __powerpc64__
> > case 58: /* ld[u], lwa */
> > - op->ea = dsform_ea(instr, regs);
> > - switch (instr & 3) {
> > + op->ea = dsform_ea(word, regs);
> > + switch (word & 3) {
> > case 0: /* ld */
> > op->type = MKOP(LOAD, 0, 8);
> > break;
> > @@ -2586,16 +2588,16 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > #ifdef CONFIG_VSX
> > case 61: /* stfdp, lxv, stxsd, stxssp, stxv */
> > - switch (instr & 7) {
> > + switch (word & 7) {
> > case 0: /* stfdp with LSB of DS field = 0 */
> > case 4: /* stfdp with LSB of DS field = 1 */
> > - op->ea = dsform_ea(instr, regs);
> > + op->ea = dsform_ea(word, regs);
> > op->type = MKOP(STORE_FP, 0, 16);
> > break;
> >
> > case 1: /* lxv */
> > - op->ea = dqform_ea(instr, regs);
> > - if (instr & 8)
> > + op->ea = dqform_ea(word, regs);
> > + if (word & 8)
> > op->reg = rd + 32;
> > op->type = MKOP(LOAD_VSX, 0, 16);
> > op->element_size = 16;
> > @@ -2604,7 +2606,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > case 2: /* stxsd with LSB of DS field = 0 */
> > case 6: /* stxsd with LSB of DS field = 1 */
> > - op->ea = dsform_ea(instr, regs);
> > + op->ea = dsform_ea(word, regs);
> > op->reg = rd + 32;
> > op->type = MKOP(STORE_VSX, 0, 8);
> > op->element_size = 8;
> > @@ -2613,7 +2615,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > case 3: /* stxssp with LSB of DS field = 0 */
> > case 7: /* stxssp with LSB of DS field = 1 */
> > - op->ea = dsform_ea(instr, regs);
> > + op->ea = dsform_ea(word, regs);
> > op->reg = rd + 32;
> > op->type = MKOP(STORE_VSX, 0, 4);
> > op->element_size = 8;
> > @@ -2621,8 +2623,8 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > break;
> >
> > case 5: /* stxv */
> > - op->ea = dqform_ea(instr, regs);
> > - if (instr & 8)
> > + op->ea = dqform_ea(word, regs);
> > + if (word & 8)
> > op->reg = rd + 32;
> > op->type = MKOP(STORE_VSX, 0, 16);
> > op->element_size = 16;
> > @@ -2634,8 +2636,8 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> >
> > #ifdef __powerpc64__
> > case 62: /* std[u] */
> > - op->ea = dsform_ea(instr, regs);
> > - switch (instr & 3) {
> > + op->ea = dsform_ea(word, regs);
> > + switch (word & 3) {
> > case 0: /* std */
> > op->type = MKOP(STORE, 0, 8);
> > break;
> > @@ -2663,7 +2665,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 0;
> >
> > logical_done:
> > - if (instr & 1)
> > + if (word & 1)
> > set_cr0(regs, op);
> > logical_done_nocc:
> > op->reg = ra;
> > @@ -2671,7 +2673,7 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > return 1;
> >
> > arith_done:
> > - if (instr & 1)
> > + if (word & 1)
> > set_cr0(regs, op);
> > compute_done:
> > op->reg = rd;
> > diff --git a/arch/powerpc/lib/test_emulate_step.c
> > b/arch/powerpc/lib/test_emulate_step.c
> > index 486e057e5be1..d6275a9b8ce6 100644
> > --- a/arch/powerpc/lib/test_emulate_step.c
> > +++ b/arch/powerpc/lib/test_emulate_step.c
> > @@ -851,7 +851,7 @@ static int __init emulate_compute_instr(struct pt_regs
> > *regs,
> >
> > if (analyse_instr(&op, regs, instr) != 1 ||
> > GETTYPE(op.type) != COMPUTE) {
> > - pr_info("emulation failed, instruction = 0x%08x\n", instr);
> > + pr_info("emulation failed, instruction = 0x%08x\n",
> > ppc_inst_word(instr));
> > return -EFAULT;
> > }
> >
> > @@ -871,7 +871,7 @@ static int __init execute_compute_instr(struct pt_regs
> > *regs,
> > /* Patch the NOP with the actual instruction */
> > patch_instruction_site(&patch__exec_instr, instr);
> > if (exec_instr(regs)) {
> > - pr_info("execution failed, instruction = 0x%08x\n", instr);
> > + pr_info("execution failed, instruction = 0x%08x\n",
> > ppc_inst_word(instr));
> > return -EFAULT;
> > }
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index d045e583f1c9..dec522fa8201 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -2871,9 +2871,9 @@ generic_inst_dump(unsigned long adr, long count, int
> > praddr,
> > dotted = 0;
> > last_inst = inst;
> > if (praddr)
> > - printf(REG" %.8x", adr, inst);
> > + printf(REG" %.8x", adr, ppc_inst_word(inst));
> > printf("\t");
> > - dump_func(inst, adr);
> > + dump_func(ppc_inst_word(inst), adr);
> > printf("\n");
> > }
> > return adr - first_adr;
>
^ permalink raw reply
* Re: [PATCH v4 03/16] powerpc: Use a datatype for instructions
From: Jordan Niethe @ 2020-03-24 3:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Alistair Popple, Nicholas Piggin, Balamuruhan S, linuxppc-dev,
Daniel Axtens
In-Reply-To: <87369yzces.fsf@mpe.ellerman.id.au>
On Tue, Mar 24, 2020 at 1:58 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Nicholas Piggin <npiggin@gmail.com> writes:
> > Jordan Niethe's on March 23, 2020 7:28 pm:
> >> On Mon, Mar 23, 2020 at 5:27 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>> Jordan Niethe's on March 20, 2020 3:17 pm:
> >>> > Currently unsigned ints are used to represent instructions on powerpc.
> >>> > This has worked well as instructions have always been 4 byte words.
> >>> > However, a future ISA version will introduce some changes to
> >>> > instructions that mean this scheme will no longer work as well. This
> >>> > change is Prefixed Instructions. A prefixed instruction is made up of a
> >>> > word prefix followed by a word suffix to make an 8 byte double word
> >>> > instruction. No matter the endianess of the system the prefix always
> >>> > comes first. Prefixed instructions are only planned for powerpc64.
> >>> >
> >>> > Introduce a ppc_inst type to represent both prefixed and word
> >>> > instructions on powerpc64 while keeping it possible to exclusively have
> >>> > word instructions on powerpc32, A latter patch will expand the type to
> >>> > include prefixed instructions but for now just typedef it to a u32.
> >>> >
> >>> > Later patches will introduce helper functions and macros for
> >>> > manipulating the instructions so that powerpc64 and powerpc32 might
> >>> > maintain separate type definitions.
> >>>
> >>> ppc_inst_t I would slightly prefer for a typedef like this.
> >> Are _t types meant to be reserved?
> >
> > No, just convention that structs are not normally typedefed unless
> > they are a pervasive interface that gets passed around a lot but
> > does not get accessed without accessor functions much. When you do
> > typedef them, add a _t (or less frequently _s/_u/etc). pte_t,
> > cpumask_t, atomic_t.
>
> Ideally we wouldn't use a typedef, we'd just have:
>
> struct ppc_inst {
> u32 val;
> #ifdef CONFIG_PPC64
> u32 suffix;
> #endif
> };
>
> That may make the conversion harder though, because you more or less
> have to update all usages at once.
Okay I will give that a try.
>
> cheers
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-24 3:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, Joerg Roedel, linuxppc-dev, linux-kernel,
iommu, Aneesh Kumar K.V, Robin Murphy, Lu Baolu
In-Reply-To: <20200323172014.GA31269@lst.de>
On 24/03/2020 04:20, Christoph Hellwig wrote:
> On Mon, Mar 23, 2020 at 07:58:01PM +1100, Alexey Kardashevskiy wrote:
>>>> 0x100.0000.0000 .. 0x101.0000.0000
>>>>
>>>> 2x4G, each is 1TB aligned. And we can map directly only the first 4GB
>>>> (because of the maximum IOMMU table size) but not the other. And 1:1 on
>>>> that "pseries" is done with offset=0x0800.0000.0000.0000.
>>>>
>>>> So we want to check every bus address against dev->bus_dma_limit, not
>>>> dev->coherent_dma_mask. In the example above I'd set bus_dma_limit to
>>>> 0x0800.0001.0000.0000 and 1:1 mapping for the second 4GB would not be
>>>> tried. Does this sound reasonable? Thanks,
>>>
>>> bus_dma_limit is just another limiting factor applied on top of
>>> coherent_dma_mask or dma_mask respectively.
>>
>> This is not enough for the task: in my example, I'd set bus limit to
>> 0x0800.0001.0000.0000 but this would disable bypass for all RAM
>> addresses - the first and the second 4GB blocks.
>
> So what about something like the version here:
>
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-bypass.3
dma_alloc_direct() and dma_map_direct() do the same thing now which is
good, did I miss anything else?
This lets us disable bypass automatically if this weird memory appears
in the system but does not let us have 1:1 after that even for normal
RAM. Thanks,
--
Alexey
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox