* Re: [PATCH v4 03/16] powerpc: Use a datatype for instructions
From: Nicholas Piggin @ 2020-03-23 9:51 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <CACzsE9qy7dUv+sbcCN-i0EeYiH=DacutULunpBdgV0h8D0m2yQ@mail.gmail.com>
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.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 13/16] powerpc: Support prefixed instructions in alignment handler
From: Jordan Niethe @ 2020-03-23 9:35 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584947091.jsvdec8of0.astroid@bobo.none>
On Mon, Mar 23, 2020 at 6:09 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:18 pm:
> > Alignment interrupts can be caused by prefixed instructions accessing
> > memory. Prefixed instructions are not permitted to cross 64-byte
> > boundaries. If they do the alignment interrupt is invoked with SRR1
> > BOUNDARY bit set. If this occurs send a SIGBUS to the offending process
> > if in user mode. If in kernel mode call bad_page_fault().
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
> > commit (previously in "powerpc sstep: Prepare to support prefixed
> > instructions").
> > - Rename sufx to suffix
> > - Use a macro for calculating instruction length
> > v3: Move __get_user_{instr(), instr_inatomic()} up with the other
> > get_user definitions and remove nested if.
> > v4: Just do the things for alignment_exception(). Other changes handled
> > elsewhere.
> > ---
> > arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index a4764b039749..cd8b3043c268 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > #define REASON_TRAP ESR_PTR
> > +#define REASON_PREFIXED 0
> > +#define REASON_BOUNDARY 0
> > +
> > +#define inst_length(reason) 4
> >
> > /* single-step stuff */
> > #define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
> > @@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
> > #define REASON_ILLEGAL SRR1_PROGILL
> > #define REASON_PRIVILEGED SRR1_PROGPRIV
> > #define REASON_TRAP SRR1_PROGTRAP
> > +#define REASON_PREFIXED SRR1_PREFIXED
> > +#define REASON_BOUNDARY SRR1_BOUNDARY
> > +
> > +#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
>
> Looks good. If you define REASON_BOUNDARY 0, then this will constant
> fold away so no need to define it twice.
Good point.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v4 06/16] powerpc: Use a function for getting the instruction op code
From: Jordan Niethe @ 2020-03-23 9:35 UTC (permalink / raw)
To: Balamuruhan S
Cc: Alistair Popple, Nicholas Piggin, linuxppc-dev, Daniel Axtens
In-Reply-To: <250cbed1e5966c5a740e840b9127ed7964e241d6.camel@linux.ibm.com>
On Mon, Mar 23, 2020 at 5:54 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Fri, 2020-03-20 at 16:17 +1100, Jordan Niethe wrote:
> > In preparation for using a data type for instructions that can not be
> > directly used with the '>>' operator use a function for getting the op
> > code of an instruction.
>
> we need to adopt this in sstep.c and vecemu.c
Thank you, I had forgotten about vecemu.c.
>
> -- Bala
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/kernel/align.c | 4 ++--
> > arch/powerpc/lib/code-patching.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> > index 38542fffa179..77c49dfdc1b4 100644
> > --- a/arch/powerpc/kernel/align.c
> > +++ b/arch/powerpc/kernel/align.c
> > @@ -313,8 +313,8 @@ int fix_alignment(struct pt_regs *regs)
> > }
> >
> > #ifdef CONFIG_SPE
> > - if ((instr >> 26) == 0x4) {
> > - int reg = (instr >> 21) & 0x1f;
> > + if (ppc_inst_opcode(instr) == 0x4) {
> > + int reg = (ppc_inst_word(instr) >> 21) & 0x1f;
> > PPC_WARN_ALIGNMENT(spe, regs);
> > return emulate_spe(regs, reg, instr);
> > }
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-
> > patching.c
> > index e2ba23fd6f4d..04a303c059e2 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -228,7 +228,7 @@ bool is_offset_in_branch_range(long offset)
> > */
> > bool is_conditional_branch(ppc_inst instr)
> > {
> > - unsigned int opcode = instr >> 26;
> > + unsigned int opcode = ppc_inst_opcode(instr);
> >
> > if (opcode == 16) /* bc, bca, bcl, bcla */
> > return true;
> > @@ -286,7 +286,7 @@ unsigned int create_cond_branch(const unsigned int *addr,
> >
> > static unsigned int branch_opcode(ppc_inst instr)
> > {
> > - return (instr >> 26) & 0x3F;
> > + return ppc_inst_opcode(instr) & 0x3F;
> > }
> >
> > static int instr_is_branch_iform(ppc_inst instr)
>
^ permalink raw reply
* Re: [PATCH v4 07/16] powerpc: Introduce functions for instruction nullity and equality
From: Jordan Niethe @ 2020-03-23 9:31 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584945440.a20p67lxd6.astroid@bobo.none>
On Mon, Mar 23, 2020 at 5:46 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:18 pm:
> > In preparation for an instruction data type that can not be directly
> > used with the '==' operator use functions for checking equality and
> > nullity.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/kernel/optprobes.c | 2 +-
> > arch/powerpc/kernel/trace/ftrace.c | 33 +++++++++++++++-------------
> > arch/powerpc/lib/code-patching.c | 16 +++++++-------
> > arch/powerpc/lib/feature-fixups.c | 2 +-
> > arch/powerpc/lib/test_emulate_step.c | 4 ++--
> > arch/powerpc/xmon/xmon.c | 4 ++--
> > 6 files changed, 32 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> > index 1025a7a3b3a8..6027425a85f2 100644
> > --- a/arch/powerpc/kernel/optprobes.c
> > +++ b/arch/powerpc/kernel/optprobes.c
> > @@ -259,7 +259,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
> > (unsigned long)emulate_step_addr,
> > BRANCH_SET_LINK);
> >
> > - if (!branch_op_callback || !branch_emulate_step)
> > + if (ppc_inst_null(branch_op_callback) || ppc_inst_null(branch_emulate_step))
>
> Is an instruction null, or zeroes?
ppc_inst_zero() would probably be clearer.
>
> Oh, most of this comes from create_branch and things. Hmm, would rather
> see those functions modified to take a &insn and return an int err.
Good idea.
>
>
> > @@ -437,7 +438,7 @@ int ftrace_make_nop(struct module *mod,
> > * then we had to use a trampoline to make the call.
> > * Otherwise just update the call site.
> > */
> > - if (test_24bit_addr(ip, addr)) {
> > + if (!ppc_inst_null(test_24bit_addr(ip, addr))) {
> > /* within range */
> > old = ftrace_call_replace(ip, addr, 1);
> > new = PPC_INST(PPC_INST_NOP);
>
> test_24bit_addr shouldn't be passing a ppc_inst back, but a bool.
True.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v4 05/16] powerpc: Use a function for masking instructions
From: Jordan Niethe @ 2020-03-23 9:31 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584944803.77ebm362hp.astroid@bobo.none>
On Mon, Mar 23, 2020 at 5:40 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:17 pm:
> > In preparation for using an instruction data type that can not be used
> > directly with the '&' operator, use a function to mask instructions.
>
> Hmm. ppc_inst_mask isn't such a good interface I think. It takes a
> ppc_inst and a mask, you would expect it to return a ppc_inst, probably
> with some part of its value anded with your mask value but not entirely
> clear.
>
> I would have a ppc_inst_val that is a more mechanical replacement and
> lets you do more things with it, although I like the other helpers you
> add later. Oh you've added ppc_inst_word further down. Why not use that
> here intead of ppc_inst_mask()?
ppc_inst_word() was what I started using first, but I started seeing a
whole lot of them immediately being &'d so I made ppc_inst_mask().
The ppc_inst_word() patch can come first, and I will just get rid of
the ppc_inst_mask() function - it is not very clear.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v4 04/16] powerpc: Use a macro for creating instructions from u32s
From: Jordan Niethe @ 2020-03-23 9:29 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584944668.qhbtv64kb2.astroid@bobo.none>
On Mon, Mar 23, 2020 at 5:30 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:17 pm:
> > In preparation for instructions having a more complex data type start
> > using a macro, PPC_INST(), for making an instruction out of a u32.
> > Currently this does nothing, but it will allow for creating a data type
> > that can represent prefixed instructions.
>
> Where is the macro? And, can it be a static inline (and lowercase)
> instead? No big deal if not.
It is in [PATCH v4 03/16] powerpc: Use a datatype for instructions inst.h.
It can be static inline and lower case, I will change that.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v4 03/16] powerpc: Use a datatype for instructions
From: Jordan Niethe @ 2020-03-23 9:28 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584944554.pe2tzckmyl.astroid@bobo.none>
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?
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
^ permalink raw reply
* Re: [PATCH v4 02/16] xmon: Move out-of-line instructions to text section
From: Jordan Niethe @ 2020-03-23 9:26 UTC (permalink / raw)
To: Balamuruhan S
Cc: Alistair Popple, Nicholas Piggin, linuxppc-dev, Daniel Axtens
In-Reply-To: <2e9df48355d592e1fbeeaff1d19d74c72fc4709f.camel@linux.ibm.com>
On Mon, Mar 23, 2020 at 5:05 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Fri, 2020-03-20 at 16:17 +1100, Jordan Niethe wrote:
> > To execute an instruction out of line after a breakpoint, the NIP is
> > set
> > to the address of struct bpt::instr. Here a copy of the instruction
> > that
> > was replaced with a breakpoint is kept, along with a trap so normal
> > flow
> > can be resumed after XOLing. The struct bpt's are located within the
> > data section. This is problematic as the data section may be marked
> > as
> > no execute.
> >
> > Instead of each struct bpt holding the instructions to be XOL'd, make
> > a
> > new array, bpt_table[], with enough space to hold instructions for
> > the
> > number of supported breakpoints. Place this array in the text
> > section.
> > Make struct bpt::instr a pointer to the instructions in bpt_table[]
> > associated with that breakpoint. This association is a simple
> > mapping:
> > bpts[n] -> bpt_table[n * words per breakpoint].
>
> Can we have it in separate commits ?
> * introduce the array bpt_table[] and make struct bpt::instr a
> pointer to the instructions in bpt_table[].
> * place the array in text section.
Yeah I can split it if that would be clearer.
>
> > Currently we only need
> > the copied instruction followed by a trap, so 2 words per breakpoint.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/kernel/vmlinux.lds.S | 2 +-
> > arch/powerpc/xmon/xmon.c | 22 +++++++++++++---------
> > 2 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/vmlinux.lds.S
> > b/arch/powerpc/kernel/vmlinux.lds.S
> > index b4c89a1acebb..e90845b8c300 100644
> > --- a/arch/powerpc/kernel/vmlinux.lds.S
> > +++ b/arch/powerpc/kernel/vmlinux.lds.S
> > @@ -86,7 +86,7 @@ SECTIONS
> > ALIGN_FUNCTION();
> > #endif
> > /* careful! __ftr_alt_* sections need to be close to
> > .text */
> > - *(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup
> > __ftr_alt_* .ref.text);
> > + *(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup
> > __ftr_alt_* .ref.text .text.xmon_bpts);
> > #ifdef CONFIG_PPC64
> > *(.tramp.ftrace.text);
> > #endif
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 02e3bd62cab4..7875d1a37770 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -97,7 +97,7 @@ static long *xmon_fault_jmp[NR_CPUS];
> > /* Breakpoint stuff */
> > struct bpt {
> > unsigned long address;
> > - unsigned int instr[2];
> > + unsigned int *instr;
> > atomic_t ref_count;
> > int enabled;
> > unsigned long pad;
> > @@ -109,6 +109,7 @@ struct bpt {
> > #define BP_DABR 4
> >
> > #define NBPTS 256
> > +#define BPT_WORDS 2
> > static struct bpt bpts[NBPTS];
> > static struct bpt dabr;
> > static struct bpt *iabr;
> > @@ -116,6 +117,8 @@ static unsigned bpinstr = 0x7fe00008; /* trap
> > */
> >
> > #define BP_NUM(bp) ((bp) - bpts + 1)
> >
> > +static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS *
> > BPT_WORDS];
> > +
> > /* Prototypes */
> > static int cmds(struct pt_regs *);
> > static int mread(unsigned long, void *, int);
> > @@ -852,16 +855,16 @@ static struct bpt *at_breakpoint(unsigned long
> > pc)
> > static struct bpt *in_breakpoint_table(unsigned long nip, unsigned
> > long *offp)
> > {
> > unsigned long off;
> > + unsigned long bp_off;
> >
> > - off = nip - (unsigned long) bpts;
> > - if (off >= sizeof(bpts))
> > + off = nip - (unsigned long) bpt_table;
> > + if (off >= sizeof(bpt_table))
> > return NULL;
> > - off %= sizeof(struct bpt);
> > - if (off != offsetof(struct bpt, instr[0])
> > - && off != offsetof(struct bpt, instr[1]))
> > + bp_off = off % (sizeof(unsigned int) * BPT_WORDS);
> > + if (bp_off != 0 && bp_off != 4)
> > return NULL;
> > - *offp = off - offsetof(struct bpt, instr[0]);
> > - return (struct bpt *) (nip - off);
> > + *offp = bp_off;
> > + return bpts + ((off - bp_off) / (sizeof(unsigned int) *
> > BPT_WORDS));
>
> `(off - bp_off) / (sizeof(unsigned int) * BPT_WORDS)` seems to be the
> actual breakpoint offset. Can we have something like,
>
> #define NBPTS 256
> #define BPT_WORDS 2
> #define BPT_WORDS_SIZE (sizeof(unsigned int) * BPT_WORDS)
> #define BPT_OFFSET(off, bp_word_off) ((off - bp_word_off) / \ BPT_WORDS_SIZE)
> ;
> :::
> :::
> :::
> bp_word_off = off % BPT_WORDS_SIZE;
> if (bp_word_off != 0 && bp_word_off != 4)
> return NULL;
> *offp = bp_word_off;
> return bpts + BPT_OFFSET(off, bp_word_off);
I do agree this is not very clear int terms of the calculations I
don't really want to introduce some macros to be used just once. I
will try to think of a way to make the calculations look more clear.
>
> -- Bala
>
^ permalink raw reply
* Re: [PATCH v4 00/16] Initial Prefixed Instruction support
From: Jordan Niethe @ 2020-03-23 9:25 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
In-Reply-To: <1584944279.gvl0lg5dde.astroid@bobo.none>
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.
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.
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address
From: Cédric Le Goater @ 2020-03-23 9:06 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, herbert, Frederic Barrat, npiggin, hch, oohall, sukadev,
linuxppc-dev, ajd
In-Reply-To: <1584598473.9256.15248.camel@hbabu-laptop>
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 ?
C.
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
> arch/powerpc/platforms/powernv/vas.h | 2 ++
> 2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> index ed9cc6d..168ab68 100644
> --- a/arch/powerpc/platforms/powernv/vas.c
> +++ b/arch/powerpc/platforms/powernv/vas.c
> @@ -15,6 +15,7 @@
> #include <linux/of_address.h>
> #include <linux/of.h>
> #include <asm/prom.h>
> +#include <asm/xive.h>
>
> #include "vas.h"
>
> @@ -25,10 +26,12 @@
>
> static int init_vas_instance(struct platform_device *pdev)
> {
> - int rc, cpu, vasid;
> - struct resource *res;
> - struct vas_instance *vinst;
> struct device_node *dn = pdev->dev.of_node;
> + struct vas_instance *vinst;
> + uint32_t chipid, irq;
> + struct resource *res;
> + int rc, cpu, vasid;
> + uint64_t port;
>
> rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
> if (rc) {
> @@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
> return -ENODEV;
> }
>
> + rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
> + if (rc) {
> + pr_err("No ibm,chip-id property for %s?\n", pdev->name);
> + return -ENODEV;
> + }
> +
> if (pdev->num_resources != 4) {
> pr_err("Unexpected DT configuration for [%s, %d]\n",
> pdev->name, vasid);
> @@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
>
> vinst->paste_win_id_shift = 63 - res->end;
>
> - pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
> - "paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> - vinst->paste_base_addr, vinst->paste_win_id_shift);
> + rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
> + if (rc)
> + return rc;
> +
> + vinst->virq = irq_create_mapping(NULL, irq);
> + if (!vinst->virq) {
> + pr_err("Inst%d: Unable to map global irq %d\n",
> + vinst->vas_id, irq);
> + return -EINVAL;
> + }
> +
> + vinst->irq_port = port;
> + pr_devel("Initialized instance [%s, %d] paste_base 0x%llx paste_win_id_shift 0x%llx IRQ %d Port 0x%llx\n",
> + pdev->name, vasid, vinst->paste_base_addr,
> + vinst->paste_win_id_shift, vinst->virq,
> + vinst->irq_port);
>
> for_each_possible_cpu(cpu) {
> if (cpu_to_chip_id(cpu) == of_get_ibm_chip_id(dn))
> diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> index 5574aec..598608b 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -313,6 +313,8 @@ struct vas_instance {
> u64 paste_base_addr;
> u64 paste_win_id_shift;
>
> + u64 irq_port;
> + int virq;
> struct mutex mutex;
> struct vas_window *rxwin[VAS_COP_TYPE_MAX];
> struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
>
^ permalink raw reply
* Re: [PATCH v8 00/14] powerpc/vas: Page fault handling for user space NX requests
From: Cédric Le Goater @ 2020-03-23 8:59 UTC (permalink / raw)
To: Haren Myneni, mpe, linuxppc-dev
Cc: mikey, herbert, Frederic Barrat, npiggin, hch, oohall, sukadev,
ajd
In-Reply-To: <1584598120.9256.15237.camel@hbabu-laptop>
On 3/19/20 7:08 AM, Haren Myneni wrote:
>
> On power9, Virtual Accelerator Switchboard (VAS) allows user space or
> kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
> instructions. NX provides various functionalities such as compression,
> encryption and etc. But only compression (842 and GZIP formats) is
> supported in Linux kernel on power9.
>
> 842 compression driver (drivers/crypto/nx/nx-842-powernv.c)
> is already included in Linux. Only GZIP support will be available from
> user space.
>
> Applications can issue GZIP compression / decompression requests to NX with
> COPY/PASTE instructions. When NX is processing these requests, can hit
> fault on the request buffer (not in memory). It issues an interrupt and
> pastes fault CRB in fault FIFO. Expects kernel to handle this fault and
> return credits for both send and fault windows after processing.
>
> This patch series adds IRQ and fault window setup, and NX fault handling:
> - Alloc IRQ and trigger port address, and configure IRQ per VAS instance.
Is the model similar to OCXL ?
If so, I suppose that the IRQ for fault handling is allocated by skiboot,
exposed in the DT and automatically mapped in Linux when the driver is
loaded.
Are there other interrupts ? Such as for job completion ?
Thanks,
C.
> - Set port# for each window to generate an interrupt when noticed fault.
> - Set fault window and FIFO on which NX paste fault CRB.
> - Setup IRQ thread fault handler per VAS instance.
> - When receiving an interrupt, Read CRBs from fault FIFO and update
> coprocessor_status_block (CSB) in the corresponding CRB with translation
> failure (CSB_CC_TRANSLATION). After issuing NX requests, process polls
> on CSB address. When it sees translation error, can touch the request
> buffer to bring the page in to memory and reissue NX request.
> - If copy_to_user fails on user space CSB address, OS sends SEGV signal.
>
> Tested these patches with NX-GZIP support and will be posting this series
> soon.
>
> Patches 1 & 2: Define alloc IRQ and get port address per chip which are needed
> to alloc IRQ per VAS instance.
> Patch 3: Define nx_fault_stamp on which NX writes fault status for the fault
> CRB
> Patch 4: Alloc and setup IRQ and trigger port address for each VAS instance
> Patch 5: Setup fault window per each VAS instance. This window is used for
> NX to paste fault CRB in FIFO.
> Patches 6 & 7: Setup threaded IRQ per VAS and register NX with fault window
> ID and port number for each send window so that NX paste fault CRB
> in this window.
> Patch 8: Reference to pid and mm so that pid is not used until window closed.
> Needed for multi thread application where child can open a window
> and can be used by parent later.
> Patches 9 and 10: Process CRBs from fault FIFO and notify tasks by
> updating CSB or through signals.
> Patches 11 and 12: Return credits for send and fault windows after handling
> faults.
> Patch 14:Fix closing send window after all credits are returned. This issue
> happens only for user space requests. No page faults on kernel
> request buffer.
>
> Changelog:
> V2:
> - Use threaded IRQ instead of own kernel thread handler
> - Use pswid instead of user space CSB address to find valid CRB
> - Removed unused macros and other changes as suggested by Christoph Hellwig
>
> V3:
> - Rebased to 5.5-rc2
> - Use struct pid * instead of pid_t for vas_window tgid
> - Code cleanup as suggested by Christoph Hellwig
>
> V4:
> - Define xive alloc and get IRQ info based on chip ID and use these
> functions for IRQ setup per VAS instance. It eliminates skiboot
> dependency as suggested by Oliver.
>
> V5:
> - Do not update CSB if the process is exiting (patch9)
>
> V6:
> - Add interrupt handler instead of default one and return IRQ_HANDLED
> if the fault handling thread is already in progress. (Patch6)
> - Use platform send window ID and CCW[0] bit to find valid CRB in
> fault FIFO (Patch6).
> - Return fault address to user space in BE and other changes as
> suggested by Michael Neuling. (patch9)
> - Rebased to 5.6-rc4
>
> V7:
> - Fix sparse warnings (patches 6,9 and 10)
>
> V8:
> - Move mm_context_remove_copro() before mmdrop() (patch8)
> - Move barrier before csb.flags store and add WARN_ON_ONCE() checks (patch9)
>
> Haren Myneni (14):
> powerpc/xive: Define xive_native_alloc_irq_on_chip()
> powerpc/xive: Define xive_native_alloc_get_irq_info()
> powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
> powerpc/vas: Alloc and setup IRQ and trigger port address
> powerpc/vas: Setup fault window per VAS instance
> powerpc/vas: Setup thread IRQ handler per VAS instance
> powerpc/vas: Register NX with fault window ID and IRQ port value
> powerpc/vas: Take reference to PID and mm for user space windows
> powerpc/vas: Update CSB and notify process for fault CRBs
> powerpc/vas: Print CRB and FIFO values
> powerpc/vas: Do not use default credits for receive window
> powerpc/vas: Return credits after handling fault
> powerpc/vas: Display process stuck message
> powerpc/vas: Free send window in VAS instance after credits returned
>
> arch/powerpc/include/asm/icswx.h | 18 +-
> arch/powerpc/include/asm/xive.h | 11 +-
> arch/powerpc/platforms/powernv/Makefile | 2 +-
> arch/powerpc/platforms/powernv/ocxl.c | 20 +-
> arch/powerpc/platforms/powernv/vas-debug.c | 2 +-
> arch/powerpc/platforms/powernv/vas-fault.c | 332 ++++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/vas-window.c | 185 ++++++++++++++--
> arch/powerpc/platforms/powernv/vas.c | 101 ++++++++-
> arch/powerpc/platforms/powernv/vas.h | 51 ++++-
> arch/powerpc/sysdev/xive/native.c | 29 ++-
> 10 files changed, 704 insertions(+), 47 deletions(-)
> create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c
>
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-03-23 8:58 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: <20200323083705.GA31245@lst.de>
On 23/03/2020 19:37, Christoph Hellwig wrote:
> On Mon, Mar 23, 2020 at 12:28:34PM +1100, Alexey Kardashevskiy wrote:
>
> [full quote deleted, please follow proper quoting rules]
>
>>> +static bool dma_alloc_direct(struct device *dev, const struct dma_map_ops *ops)
>>> +{
>>> + if (!ops)
>>> + return true;
>>> +
>>> + /*
>>> + * Allows IOMMU drivers to bypass dynamic translations if the DMA mask
>>> + * is large enough.
>>> + */
>>> + if (dev->dma_ops_bypass) {
>>> + if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) >=
>>> + dma_direct_get_required_mask(dev))
>>> + return true;
>>> + }
>>
>>
>> Why not do this in dma_map_direct() as well?
>
> Mostly beacuse it is a relatively expensive operation, including a
> fls64.
Ah, ok.
>> Or simply have just one dma_map_direct()?
>
> What do you mean with that?
I mean use dma_alloc_direct() instead of dma_map_direct() everywhere,
you explained just above.
>
>> And one more general question - we need a way to use non-direct IOMMU
>> for RAM above certain limit.
>>
>> Let's say we have a system with:
>> 0 .. 0x1.0000.0000
>> 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.
--
Alexey
^ permalink raw reply
* Re: [PATCH v4 15/16] powerpc sstep: Add support for prefixed load/stores
From: Balamuruhan S @ 2020-03-23 8:54 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, npiggin
In-Reply-To: <20200320051809.24332-16-jniethe5@gmail.com>
On Fri, 2020-03-20 at 16:18 +1100, Jordan Niethe wrote:
> This adds emulation support for the following prefixed integer
> load/stores:
> * Prefixed Load Byte and Zero (plbz)
> * Prefixed Load Halfword and Zero (plhz)
> * Prefixed Load Halfword Algebraic (plha)
> * Prefixed Load Word and Zero (plwz)
> * Prefixed Load Word Algebraic (plwa)
> * Prefixed Load Doubleword (pld)
> * Prefixed Store Byte (pstb)
> * Prefixed Store Halfword (psth)
> * Prefixed Store Word (pstw)
> * Prefixed Store Doubleword (pstd)
> * Prefixed Load Quadword (plq)
> * Prefixed Store Quadword (pstq)
>
> the follow prefixed floating-point load/stores:
> * Prefixed Load Floating-Point Single (plfs)
> * Prefixed Load Floating-Point Double (plfd)
> * Prefixed Store Floating-Point Single (pstfs)
> * Prefixed Store Floating-Point Double (pstfd)
>
> and for the following prefixed VSX load/stores:
> * Prefixed Load VSX Scalar Doubleword (plxsd)
> * Prefixed Load VSX Scalar Single-Precision (plxssp)
> * Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
> * Prefixed Store VSX Scalar Doubleword (pstxsd)
> * Prefixed Store VSX Scalar Single-Precision (pstxssp)
> * Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
LGTM,
Reviewed-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
> v2: - Combine all load/store patches
> - Fix the name of Type 01 instructions
> - Remove sign extension flag from pstd/pld
> - Rename sufx -> suffix
> v3: - Move prefixed loads and stores into the switch statement
> ---
> arch/powerpc/lib/sstep.c | 159 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 159 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index ab4c71c43c8c..daef70eb8e22 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned
> int instr,
> return ea;
> }
>
> +/*
> + * Calculate effective address for a MLS:D-form / 8LS:D-form
> + * prefixed instruction
> + */
> +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> + unsigned int suffix,
> + const struct pt_regs *regs)
> +{
> + int ra, prefix_r;
> + unsigned int dd;
> + unsigned long ea, d0, d1, d;
> +
> + prefix_r = instr & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> +
> + d0 = instr & 0x3ffff;
> + d1 = suffix & 0xffff;
> + d = (d0 << 16) | d1;
> +
> + /*
> + * sign extend a 34 bit number
> + */
> + dd = (unsigned int)(d >> 2);
> + ea = (signed int)dd;
> + ea = (ea << 2) | (d & 0x3);
> +
> + if (!prefix_r && ra)
> + ea += regs->gpr[ra];
> + else if (!prefix_r && !ra)
> + ; /* Leave ea as is */
> + else if (prefix_r && !ra)
> + ea += regs->nip;
> + else if (prefix_r && ra)
> + ; /* Invalid form. Should already be checked for by caller! */
> +
> + return ea;
> +}
> +
> /*
> * Return the largest power of 2, not greater than sizeof(unsigned long),
> * such that x is a multiple of it.
> @@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> ppc_inst instr)
> {
> unsigned int opcode, ra, rb, rc, rd, spr, u;
> + unsigned int suffixopcode, prefixtype, prefix_r;
> unsigned long int imm;
> unsigned long int val, val2;
> unsigned int mb, me, sh;
> @@ -2652,6 +2691,126 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> break;
> }
> break;
> + case 1: /* Prefixed instructions */
> + prefix_r = word & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> + op->update_reg = ra;
> + rd = (suffix >> 21) & 0x1f;
> + op->reg = rd;
> + op->val = regs->gpr[rd];
> +
> + suffixopcode = suffix >> 26;
> + prefixtype = (word >> 24) & 0x3;
> + switch (prefixtype) {
> + case 0: /* Type 00 Eight-Byte Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(word, suffix, regs);
> + switch (suffixopcode) {
> + case 41: /* plwa */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> + break;
> + case 42: /* plxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 43: /* plxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 46: /* pstxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 47: /* pstxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 51: /* plxv1 */
> + op->reg += 32;
> +
> + /* fallthru */
> + case 50: /* plxv0 */
> + op->type = MKOP(LOAD_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 55: /* pstxv1 */
> + op->reg = rd + 32;
> +
> + /* fallthru */
> + case 54: /* pstxv0 */
> + op->type = MKOP(STORE_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 56: /* plq */
> + op->type = MKOP(LOAD, PREFIXED, 16);
> + break;
> + case 57: /* pld */
> + op->type = MKOP(LOAD, PREFIXED, 8);
> + break;
> + case 60: /* stq */
> + op->type = MKOP(STORE, PREFIXED, 16);
> + break;
> + case 61: /* pstd */
> + op->type = MKOP(STORE, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 1: /* Type 01 Eight-Byte Register-to-Register */
> + break;
> + case 2: /* Type 10 Modified Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(word, suffix, regs);
> + switch (suffixopcode) {
> + case 32: /* plwz */
> + op->type = MKOP(LOAD, PREFIXED, 4);
> + break;
> + case 34: /* plbz */
> + op->type = MKOP(LOAD, PREFIXED, 1);
> + break;
> + case 36: /* pstw */
> + op->type = MKOP(STORE, PREFIXED, 4);
> + break;
> + case 38: /* pstb */
> + op->type = MKOP(STORE, PREFIXED, 1);
> + break;
> + case 40: /* plhz */
> + op->type = MKOP(LOAD, PREFIXED, 2);
> + break;
> + case 42: /* plha */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> + break;
> + case 44: /* psth */
> + op->type = MKOP(STORE, PREFIXED, 2);
> + break;
> + case 48: /* plfs */
> + op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
> + break;
> + case 50: /* plfd */
> + op->type = MKOP(LOAD_FP, PREFIXED, 8);
> + break;
> + case 52: /* pstfs */
> + op->type = MKOP(STORE_FP, PREFIXED | FPCONV,
> 4);
> + break;
> + case 54: /* pstfd */
> + op->type = MKOP(STORE_FP, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 3: /* Type 11 Modified Register-to-Register */
> + break;
> + }
> #endif /* __powerpc64__ */
>
> }
^ permalink raw reply
* Re: [PATCH v8 02/14] powerpc/xive: Define xive_native_alloc_get_irq_info()
From: Cédric Le Goater @ 2020-03-23 8:52 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, herbert, Frederic Barrat, npiggin, hch, oohall, sukadev,
linuxppc-dev, ajd
In-Reply-To: <1584598402.9256.15244.camel@hbabu-laptop>
On 3/19/20 7:13 AM, Haren Myneni wrote:
>
> pnv_ocxl_alloc_xive_irq() in ocxl.c allocates IRQ and gets trigger port
> address. VAS also needs this function, but based on chip ID. So moved
> this common function to xive/native.c.
We now have two drivers using the lowlevel routines of the machine
irqchip driver. I am not sure OCXL is doing the right thing by calling
opal_xive_get_irq_info() and not xive_native_populate_irq_data().
C.
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/include/asm/xive.h | 2 ++
> arch/powerpc/platforms/powernv/ocxl.c | 20 ++------------------
> arch/powerpc/sysdev/xive/native.c | 23 +++++++++++++++++++++++
> 3 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index d08ea11..fd337da 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -139,6 +139,8 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
> int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
> bool xive_native_has_queue_state_support(void);
> extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +extern int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq,
> + u64 *trigger_addr);
>
> static inline u32 xive_native_alloc_irq(void)
> {
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 8c65aac..fb8f99a 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
>
> int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
> {
> - __be64 flags, trigger_page;
> - s64 rc;
> - u32 hwirq;
> -
> - hwirq = xive_native_alloc_irq();
> - if (!hwirq)
> - return -ENOENT;
> -
> - rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> - NULL);
> - if (rc || !trigger_page) {
> - xive_native_free_irq(hwirq);
> - return -ENOENT;
> - }
> - *irq = hwirq;
> - *trigger_addr = be64_to_cpu(trigger_page);
> - return 0;
> -
> + return xive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP, irq,
> + trigger_addr);
> }
> EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
>
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 14d4406..abdd892 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -295,6 +295,29 @@ u32 xive_native_alloc_irq_on_chip(u32 chip_id)
> }
> EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>
> +int xive_native_alloc_get_irq_info(u32 chip_id, u32 *irq, u64 *trigger_addr)
> +{
> + __be64 flags, trigger_page;
> + u32 hwirq;
> + s64 rc;
> +
> + hwirq = xive_native_alloc_irq_on_chip(chip_id);
> + if (!hwirq)
> + return -ENOENT;
> +
> + rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> + NULL);
> + if (rc || !trigger_page) {
> + xive_native_free_irq(hwirq);
> + return -ENOENT;
> + }
> + *irq = hwirq;
> + *trigger_addr = be64_to_cpu(trigger_page);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(xive_native_alloc_get_irq_info);
> +
> void xive_native_free_irq(u32 irq)
> {
> for (;;) {
>
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Christoph Hellwig @ 2020-03-23 8:50 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
iommu, Aneesh Kumar K.V, linuxppc-dev, Christoph Hellwig,
Lu Baolu
In-Reply-To: <20200323083705.GA31245@lst.de>
On Mon, Mar 23, 2020 at 09:37:05AM +0100, Christoph Hellwig wrote:
> > > + /*
> > > + * Allows IOMMU drivers to bypass dynamic translations if the DMA mask
> > > + * is large enough.
> > > + */
> > > + if (dev->dma_ops_bypass) {
> > > + if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) >=
> > > + dma_direct_get_required_mask(dev))
> > > + return true;
> > > + }
> >
> >
> > Why not do this in dma_map_direct() as well?
>
> Mostly beacuse it is a relatively expensive operation, including a
> fls64.
Which I guess isn't too bad compared to a dynamic IOMMU mapping. Can
you just send a draft patch for what you'd like to see for ppc?
^ permalink raw reply
* Re: [PATCH v8 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()
From: Cédric Le Goater @ 2020-03-23 8:32 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, herbert, npiggin, hch, oohall, sukadev, linuxppc-dev, ajd
In-Reply-To: <1584598352.9256.15242.camel@hbabu-laptop>
On 3/19/20 7:12 AM, Haren Myneni wrote:
>
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.
The pool of generic interrupt source (IPI) numbers is generally used
by user space application which generally do not care on which chip
the interrupt is allocated. It's used by the CXL driver and KVM for
the guest interrupts. The CPU IPI are the exceptions.
The underlying FW call will try to allocate on the chip of the CPU
first and then on the others. If you specify a chip id, there is no
fallback. Is it what you want ?
Why do you need to allocate a generic interrupt source (IPI) from
a specific chip ? Is it a VAS requirement ?
Could you explain a bit more how it is used because there might be
similar request.
The code is fine.
Thanks,
C.
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
> arch/powerpc/include/asm/xive.h | 9 ++++++++-
> arch/powerpc/sysdev/xive/native.c | 6 +++---
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index 93f982db..d08ea11 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -5,6 +5,8 @@
> #ifndef _ASM_POWERPC_XIVE_H
> #define _ASM_POWERPC_XIVE_H
>
> +#include <asm/opal-api.h>
> +
> #define XIVE_INVALID_VP 0xffffffff
>
> #ifdef CONFIG_PPC_XIVE
> @@ -108,7 +110,6 @@ struct xive_q {
> int xive_native_populate_irq_data(u32 hw_irq,
> struct xive_irq_data *data);
> void xive_cleanup_irq_data(struct xive_irq_data *xd);
> -u32 xive_native_alloc_irq(void);
> void xive_native_free_irq(u32 irq);
> int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>
> @@ -137,6 +138,12 @@ int xive_native_set_queue_state(u32 vp_id, uint32_t prio, u32 qtoggle,
> u32 qindex);
> int xive_native_get_vp_state(u32 vp_id, u64 *out_state);
> bool xive_native_has_queue_state_support(void);
> +extern u32 xive_native_alloc_irq_on_chip(u32 chip_id);
> +
> +static inline u32 xive_native_alloc_irq(void)
> +{
> + return xive_native_alloc_irq_on_chip(OPAL_XIVE_ANY_CHIP);
> +}
>
> #else
>
> diff --git a/arch/powerpc/sysdev/xive/native.c b/arch/powerpc/sysdev/xive/native.c
> index 0ff6b73..14d4406 100644
> --- a/arch/powerpc/sysdev/xive/native.c
> +++ b/arch/powerpc/sysdev/xive/native.c
> @@ -279,12 +279,12 @@ static int xive_native_get_ipi(unsigned int cpu, struct xive_cpu *xc)
> }
> #endif /* CONFIG_SMP */
>
> -u32 xive_native_alloc_irq(void)
> +u32 xive_native_alloc_irq_on_chip(u32 chip_id)
> {
> s64 rc;
>
> for (;;) {
> - rc = opal_xive_allocate_irq(OPAL_XIVE_ANY_CHIP);
> + rc = opal_xive_allocate_irq(chip_id);
> if (rc != OPAL_BUSY)
> break;
> msleep(OPAL_BUSY_DELAY_MS);
> @@ -293,7 +293,7 @@ u32 xive_native_alloc_irq(void)
> return 0;
> return rc;
> }
> -EXPORT_SYMBOL_GPL(xive_native_alloc_irq);
> +EXPORT_SYMBOL_GPL(xive_native_alloc_irq_on_chip);
>
> void xive_native_free_irq(u32 irq)
> {
>
^ permalink raw reply
* Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions
From: Balamuruhan S @ 2020-03-23 8:43 UTC (permalink / raw)
To: Nicholas Piggin, Jordan Niethe, linuxppc-dev; +Cc: alistair, dja
In-Reply-To: <1584946118.tw98vo7hqx.astroid@bobo.none>
On Mon, 2020-03-23 at 18:00 +1000, Nicholas Piggin wrote:
> Jordan Niethe's on March 20, 2020 3:18 pm:
> > Prefixed instructions will mean there are instructions of different
> > length. As a result dereferencing a pointer to an instruction will not
> > necessarily give the desired result. Introduce a function for reading
> > instructions from memory into the instruction data type.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/include/asm/uprobes.h | 4 ++--
> > arch/powerpc/kernel/kprobes.c | 8 ++++----
> > arch/powerpc/kernel/mce_power.c | 2 +-
> > arch/powerpc/kernel/optprobes.c | 6 +++---
> > arch/powerpc/kernel/trace/ftrace.c | 33 +++++++++++++++++++-----------
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/lib/code-patching.c | 22 ++++++++++----------
> > arch/powerpc/lib/feature-fixups.c | 6 +++---
> > arch/powerpc/xmon/xmon.c | 6 +++---
> > 9 files changed, 49 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uprobes.h
> > b/arch/powerpc/include/asm/uprobes.h
> > index 2bbdf27d09b5..fff3c5fc90b5 100644
> > --- a/arch/powerpc/include/asm/uprobes.h
> > +++ b/arch/powerpc/include/asm/uprobes.h
> > @@ -23,8 +23,8 @@ typedef ppc_opcode_t uprobe_opcode_t;
> >
> > struct arch_uprobe {
> > union {
> > - u32 insn;
> > - u32 ixol;
> > + u8 insn[MAX_UINSN_BYTES];
> > + u8 ixol[MAX_UINSN_BYTES];
> > };
> > };
>
> Hmm. I wonder if this should be a different patch. Not sure if raw
> bytes is a good idea here. ppc probes also has a ppc_opcode_t, maybe
> could be replaced with ppc_insn_t and used here instead?
>
> Also can't find where you define ppc_inst_read.
Nick, ppc_inst_read and macro PPC_INST you have asked in patch 4 are defined in
asm/inst.h with patch 3 (powerpc: Use a datatype for instructions)
-- Bala
>
> > diff --git a/arch/powerpc/kernel/trace/ftrace.c
> > b/arch/powerpc/kernel/trace/ftrace.c
> > index 7614a9f537fd..ad451205f268 100644
> > --- a/arch/powerpc/kernel/trace/ftrace.c
> > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > @@ -41,6 +41,12 @@
> > #define NUM_FTRACE_TRAMPS 8
> > static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
> >
> > +static long
> > +read_inst(ppc_inst *inst, const void *src)
> > +{
> > + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
> > +}
>
> Humbly suggest probe_kernel_inst_read.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Christoph Hellwig @ 2020-03-23 8:37 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
iommu, Aneesh Kumar K.V, linuxppc-dev, Christoph Hellwig,
Lu Baolu
In-Reply-To: <2f31d0dd-aa7e-8b76-c8a1-5759fda5afc9@ozlabs.ru>
On Mon, Mar 23, 2020 at 12:28:34PM +1100, Alexey Kardashevskiy wrote:
[full quote deleted, please follow proper quoting rules]
> > +static bool dma_alloc_direct(struct device *dev, const struct dma_map_ops *ops)
> > +{
> > + if (!ops)
> > + return true;
> > +
> > + /*
> > + * Allows IOMMU drivers to bypass dynamic translations if the DMA mask
> > + * is large enough.
> > + */
> > + if (dev->dma_ops_bypass) {
> > + if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) >=
> > + dma_direct_get_required_mask(dev))
> > + return true;
> > + }
>
>
> Why not do this in dma_map_direct() as well?
Mostly beacuse it is a relatively expensive operation, including a
fls64.
> Or simply have just one dma_map_direct()?
What do you mean with that?
> And one more general question - we need a way to use non-direct IOMMU
> for RAM above certain limit.
>
> Let's say we have a system with:
> 0 .. 0x1.0000.0000
> 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.
^ permalink raw reply
* Re: [PATCH v3 9/9] Documentation/powerpc: VAS API
From: Nicholas Piggin @ 2020-03-23 8:32 UTC (permalink / raw)
To: Haren Myneni, herbert; +Cc: mikey, sukadev, linuxppc-dev, linux-crypto
In-Reply-To: <1583541541.9256.50.camel@hbabu-laptop>
Haren Myneni's on March 7, 2020 10:39 am:
>
> Power9 introduced Virtual Accelerator Switchboard (VAS) which allows
> userspace to communicate with Nest Accelerator (NX) directly. But
> kernel has to establish channel to NX for userspace. This document
> describes user space API that application can use to establish
> communication channel.
Agree with Daniel this is good documentation.
But I don't see mention of the word 'signal' anywhere. The signal stuff
is one of the trickiest parts this code being added. It would be great if
that could be documented and even with example code or at least a
description of why it's required and can't be done some other way.
Does something like io_uring require signals in such cases?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions
From: Nicholas Piggin @ 2020-03-23 8:00 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
In-Reply-To: <20200320051809.24332-10-jniethe5@gmail.com>
Jordan Niethe's on March 20, 2020 3:18 pm:
> Prefixed instructions will mean there are instructions of different
> length. As a result dereferencing a pointer to an instruction will not
> necessarily give the desired result. Introduce a function for reading
> instructions from memory into the instruction data type.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v4: New to series
> ---
> arch/powerpc/include/asm/uprobes.h | 4 ++--
> arch/powerpc/kernel/kprobes.c | 8 ++++----
> arch/powerpc/kernel/mce_power.c | 2 +-
> arch/powerpc/kernel/optprobes.c | 6 +++---
> arch/powerpc/kernel/trace/ftrace.c | 33 +++++++++++++++++++-----------
> arch/powerpc/kernel/uprobes.c | 2 +-
> arch/powerpc/lib/code-patching.c | 22 ++++++++++----------
> arch/powerpc/lib/feature-fixups.c | 6 +++---
> arch/powerpc/xmon/xmon.c | 6 +++---
> 9 files changed, 49 insertions(+), 40 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
> index 2bbdf27d09b5..fff3c5fc90b5 100644
> --- a/arch/powerpc/include/asm/uprobes.h
> +++ b/arch/powerpc/include/asm/uprobes.h
> @@ -23,8 +23,8 @@ typedef ppc_opcode_t uprobe_opcode_t;
>
> struct arch_uprobe {
> union {
> - u32 insn;
> - u32 ixol;
> + u8 insn[MAX_UINSN_BYTES];
> + u8 ixol[MAX_UINSN_BYTES];
> };
> };
Hmm. I wonder if this should be a different patch. Not sure if raw
bytes is a good idea here. ppc probes also has a ppc_opcode_t, maybe
could be replaced with ppc_insn_t and used here instead?
Also can't find where you define ppc_inst_read.
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 7614a9f537fd..ad451205f268 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -41,6 +41,12 @@
> #define NUM_FTRACE_TRAMPS 8
> static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
>
> +static long
> +read_inst(ppc_inst *inst, const void *src)
> +{
> + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
> +}
Humbly suggest probe_kernel_inst_read.
Thanks,
Nick
^ permalink raw reply
* [PATCH kernel v2 5/7] powerpc/iommu: Add a window number to iommu_table_group_ops::get_table_size
From: Alexey Kardashevskiy @ 2020-03-23 7:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kvm, Fabiano Rosas, Alexey Kardashevskiy, Alistair Popple,
kvm-ppc, David Gibson
In-Reply-To: <20200323075354.93825-1-aik@ozlabs.ru>
We are about to add an additional offset within a TCE table which is
going to increase the size of the window, prepare for this.
This should cause no behavioral change.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/include/asm/iommu.h | 1 +
arch/powerpc/platforms/powernv/pci.h | 2 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 4 ++--
drivers/vfio/vfio_iommu_spapr_tce.c | 4 ++--
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 479439ef003e..acf64a73ead1 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -161,6 +161,7 @@ struct iommu_table_group;
struct iommu_table_group_ops {
unsigned long (*get_table_size)(
+ int num,
__u32 page_shift,
__u64 window_size,
__u32 levels);
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index ce00278185b0..d8fa2f65517e 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -192,7 +192,7 @@ extern int pnv_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type);
extern void pnv_teardown_msi_irqs(struct pci_dev *pdev);
extern struct pnv_ioda_pe *pnv_ioda_get_pe(struct pci_dev *dev);
extern void pnv_set_msi_irq_chip(struct pnv_phb *phb, unsigned int virq);
-extern unsigned long pnv_pci_ioda2_get_table_size(__u32 page_shift,
+extern unsigned long pnv_pci_ioda2_get_table_size(int num, __u32 page_shift,
__u64 window_size, __u32 levels);
extern int pnv_eeh_post_init(void);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 9928a1618a8b..27a505a5edb4 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2597,7 +2597,7 @@ static long pnv_pci_ioda2_unset_window(struct iommu_table_group *table_group,
#endif
#ifdef CONFIG_IOMMU_API
-unsigned long pnv_pci_ioda2_get_table_size(__u32 page_shift,
+unsigned long pnv_pci_ioda2_get_table_size(int num, __u32 page_shift,
__u64 window_size, __u32 levels)
{
unsigned long bytes = 0;
@@ -2644,7 +2644,7 @@ static long pnv_pci_ioda2_create_table_userspace(
if (!ret)
(*ptbl)->it_allocated_size = pnv_pci_ioda2_get_table_size(
- page_shift, window_size, levels);
+ num, page_shift, window_size, levels);
return ret;
}
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 16b3adc508db..750a0676e9b7 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -613,8 +613,8 @@ static long tce_iommu_create_table(struct tce_container *container,
{
long ret, table_size;
- table_size = table_group->ops->get_table_size(page_shift, window_size,
- levels);
+ table_size = table_group->ops->get_table_size(num, page_shift,
+ window_size, levels);
if (!table_size)
return -EINVAL;
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v2 3/7] powerpc/powernv/ioda: Allow smaller TCE table levels
From: Alexey Kardashevskiy @ 2020-03-23 7:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kvm, Fabiano Rosas, Alexey Kardashevskiy, Alistair Popple,
kvm-ppc, David Gibson
In-Reply-To: <20200323075354.93825-1-aik@ozlabs.ru>
Now the minimum allocation size for a TCE table level is PAGE_SIZE (64k)
as this is the minimum for alloc_pages(). The limit was set in POWER8
where we did not have sparse RAM so we did not need sparse TCE tables.
On POWER9 we have gaps in the phys address space for which using multi
level TCE tables makes sense. The problem with that is that 64K per level
is too much for 2 levels and 1GB pages as it exceeds the hardware limit
of 55bits so we need smaller levels.
This drops the minimum level size to 4K.
For a machine with 2 CPUs, top RAM address 0x4000.0000.0000
(each node gets 32TiB) and 1GiB IOMMU pages:
Before the patch: 512KiB or 8 pages.
After the patch: 3 pages: one level1 + 2xlevel2 tables, each can map
up to 64k>>3<<30 = 8TiB of physical space.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index 5dc6847d5f4c..82e680da9d94 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -37,7 +37,7 @@ static __be64 *pnv_alloc_tce_level(int nid, unsigned int shift)
__be64 *addr;
tce_mem = alloc_pages_node(nid, GFP_ATOMIC | __GFP_NOWARN,
- shift - PAGE_SHIFT);
+ shift > PAGE_SHIFT ? shift - PAGE_SHIFT : 0);
if (!tce_mem) {
pr_err("Failed to allocate a TCE memory, level shift=%d\n",
shift);
@@ -282,7 +282,7 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
/* Adjust direct table size from window_size and levels */
entries_shift = (entries_shift + levels - 1) / levels;
level_shift = entries_shift + 3;
- level_shift = max_t(unsigned int, level_shift, PAGE_SHIFT);
+ level_shift = max_t(unsigned int, level_shift, 12); /* 4K is minimum */
if ((level_shift - 3) * levels + page_shift >= 55)
return -EINVAL;
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v2 1/7] powerpc/powernv/ioda: Move TCE bypass base to PE
From: Alexey Kardashevskiy @ 2020-03-23 7:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kvm, Fabiano Rosas, Alexey Kardashevskiy, Alistair Popple,
kvm-ppc, David Gibson
In-Reply-To: <20200323075354.93825-1-aik@ozlabs.ru>
We are about to allow another location for the second DMA window and
we will need to advertise it outside of the powernv platform code.
This moves bypass base address to iommu_table_group so drivers such as
VFIO SPAPR TCE can see it.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/include/asm/iommu.h | 1 +
arch/powerpc/platforms/powernv/pci.h | 1 -
arch/powerpc/platforms/powernv/npu-dma.c | 1 +
arch/powerpc/platforms/powernv/pci-ioda.c | 26 ++++++++++-------------
4 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 350101e11ddb..479439ef003e 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -194,6 +194,7 @@ struct iommu_table_group {
__u64 pgsizes; /* Bitmap of supported page sizes */
__u32 max_dynamic_windows_supported;
__u32 max_levels;
+ __u64 tce64_start;
struct iommu_group *group;
struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index d3bbdeab3a32..a808dd396522 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -67,7 +67,6 @@ struct pnv_ioda_pe {
/* 64-bit TCE bypass region */
bool tce_bypass_enabled;
- uint64_t tce_bypass_base;
/* MSIs. MVE index is identical for for 32 and 64 bit MSI
* and -1 if not supported. (It's actually identical to the
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index b95b9e3c4c98..97a479848003 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -469,6 +469,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
table_group->tce32_start = pe->table_group.tce32_start;
table_group->tce32_size = pe->table_group.tce32_size;
table_group->max_levels = pe->table_group.max_levels;
+ table_group->tce64_start = pe->table_group.tce64_start;
if (!table_group->pgsizes)
table_group->pgsizes = pe->table_group.pgsizes;
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 22c22cd7bd82..52db10ab4fef 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1777,7 +1777,7 @@ static void pnv_pci_ioda_dma_dev_setup(struct pci_dev *pdev)
pe = &phb->ioda.pe_array[pdn->pe_number];
WARN_ON(get_dma_ops(&pdev->dev) != &dma_iommu_ops);
- pdev->dev.archdata.dma_offset = pe->tce_bypass_base;
+ pdev->dev.archdata.dma_offset = pe->table_group.tce64_start;
set_iommu_table_base(&pdev->dev, pe->table_group.tables[0]);
/*
* Note: iommu_add_device() will fail here as
@@ -1869,7 +1869,8 @@ static bool pnv_pci_ioda_iommu_bypass_supported(struct pci_dev *pdev,
pe = &phb->ioda.pe_array[pdn->pe_number];
if (pe->tce_bypass_enabled) {
- u64 top = pe->tce_bypass_base + memblock_end_of_DRAM() - 1;
+ u64 top = pe->table_group.tce64_start +
+ memblock_end_of_DRAM() - 1;
if (dma_mask >= top)
return true;
}
@@ -1903,7 +1904,7 @@ static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe, struct pci_bus *bus)
list_for_each_entry(dev, &bus->devices, bus_list) {
set_iommu_table_base(&dev->dev, pe->table_group.tables[0]);
- dev->dev.archdata.dma_offset = pe->tce_bypass_base;
+ dev->dev.archdata.dma_offset = pe->table_group.tce64_start;
if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
pnv_ioda_setup_bus_dma(pe, dev->subordinate);
@@ -2361,16 +2362,12 @@ static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable)
top = roundup_pow_of_two(top);
rc = opal_pci_map_pe_dma_window_real(pe->phb->opal_id,
- pe->pe_number,
- window_id,
- pe->tce_bypass_base,
- top);
+ pe->pe_number, window_id,
+ pe->table_group.tce64_start, top);
} else {
rc = opal_pci_map_pe_dma_window_real(pe->phb->opal_id,
- pe->pe_number,
- window_id,
- pe->tce_bypass_base,
- 0);
+ pe->pe_number, window_id,
+ pe->table_group.tce64_start, 0);
}
if (rc)
pe_err(pe, "OPAL error %lld configuring bypass window\n", rc);
@@ -2385,7 +2382,8 @@ static long pnv_pci_ioda2_create_table(struct iommu_table_group *table_group,
struct pnv_ioda_pe *pe = container_of(table_group, struct pnv_ioda_pe,
table_group);
int nid = pe->phb->hose->node;
- __u64 bus_offset = num ? pe->tce_bypass_base : table_group->tce32_start;
+ __u64 bus_offset = num ?
+ pe->table_group.tce64_start : table_group->tce32_start;
long ret;
struct iommu_table *tbl;
@@ -2735,9 +2733,6 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
if (!pnv_pci_ioda_pe_dma_weight(pe))
return;
- /* TVE #1 is selected by PCI address bit 59 */
- pe->tce_bypass_base = 1ull << 59;
-
/* The PE will reserve all possible 32-bits space */
pe_info(pe, "Setting up 32-bit TCE table at 0..%08x\n",
phb->ioda.m32_pci_base);
@@ -2745,6 +2740,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
/* Setup linux iommu table */
pe->table_group.tce32_start = 0;
pe->table_group.tce32_size = phb->ioda.m32_pci_base;
+ pe->table_group.tce64_start = 1UL << 59;
pe->table_group.max_dynamic_windows_supported =
IOMMU_TABLE_GROUP_MAX_TABLES;
pe->table_group.max_levels = POWERNV_IOMMU_MAX_LEVELS;
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v2 0/7] powerpc/powenv/ioda: Allow huge DMA window at 4GB
From: Alexey Kardashevskiy @ 2020-03-23 7:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kvm, Fabiano Rosas, Alexey Kardashevskiy, Alistair Popple,
kvm-ppc, David Gibson
Here is an attempt to support bigger DMA space for devices
supporting DMA masks less than 59 bits (GPUs come into mind
first). POWER9 PHBs have an option to map 2 windows at 0
and select a windows based on DMA address being below or above
4GB.
This adds the "iommu=iommu_bypass" kernel parameter and
supports VFIO+pseries machine - current this requires telling
upstream+unmodified QEMU about this via
-global spapr-pci-host-bridge.dma64_win_addr=0x100000000
or per-phb property. 4/4 advertises the new option but
there is no automation around it in QEMU (should it be?).
For now it is either 1<<59 or 4GB mode; dynamic switching is
not supported (could be via sysfs).
This is a rebased version of
https://lore.kernel.org/kvm/20191202015953.127902-1-aik@ozlabs.ru/
The main change since v1 is that now it is 7 patches with
clearer separation of steps.
This is based on 6c90b86a745a "Merge tag 'mmc-v5.6-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc"
Please comment. Thanks.
Alexey Kardashevskiy (7):
powerpc/powernv/ioda: Move TCE bypass base to PE
powerpc/powernv/ioda: Rework for huge DMA window at 4GB
powerpc/powernv/ioda: Allow smaller TCE table levels
powerpc/powernv/phb4: Use IOMMU instead of bypassing
powerpc/iommu: Add a window number to
iommu_table_group_ops::get_table_size
powerpc/powernv/phb4: Add 4GB IOMMU bypass mode
vfio/spapr_tce: Advertise and allow a huge DMA windows at 4GB
arch/powerpc/include/asm/iommu.h | 3 +
arch/powerpc/include/asm/opal-api.h | 9 +-
arch/powerpc/include/asm/opal.h | 2 +
arch/powerpc/platforms/powernv/pci.h | 4 +-
include/uapi/linux/vfio.h | 2 +
arch/powerpc/platforms/powernv/npu-dma.c | 1 +
arch/powerpc/platforms/powernv/opal-call.c | 2 +
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 4 +-
arch/powerpc/platforms/powernv/pci-ioda.c | 234 ++++++++++++++----
drivers/vfio/vfio_iommu_spapr_tce.c | 17 +-
10 files changed, 213 insertions(+), 65 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH kernel v2 7/7] vfio/spapr_tce: Advertise and allow a huge DMA windows at 4GB
From: Alexey Kardashevskiy @ 2020-03-23 7:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: kvm, Fabiano Rosas, Alexey Kardashevskiy, Alistair Popple,
kvm-ppc, David Gibson
In-Reply-To: <20200323075354.93825-1-aik@ozlabs.ru>
So far the only option for a big 64big DMA window was a window located
at 0x800.0000.0000.0000 (1<<59) which creates problems for devices
supporting smaller DMA masks.
This exploits a POWER9 PHB option to allow the second DMA window to map
at 0 and advertises it with a 4GB offset to avoid overlap with
the default 32bit window.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
include/uapi/linux/vfio.h | 2 ++
drivers/vfio/vfio_iommu_spapr_tce.c | 13 +++++++------
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..c7f89d47335a 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -831,9 +831,11 @@ struct vfio_iommu_spapr_tce_info {
__u32 argsz;
__u32 flags;
#define VFIO_IOMMU_SPAPR_INFO_DDW (1 << 0) /* DDW supported */
+#define VFIO_IOMMU_SPAPR_INFO_DDW_START (1 << 1) /* DDW offset */
__u32 dma32_window_start; /* 32 bit window start (bytes) */
__u32 dma32_window_size; /* 32 bit window size (bytes) */
struct vfio_iommu_spapr_tce_ddw_info ddw;
+ __u64 dma64_window_start;
};
#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 750a0676e9b7..315fd56e51a7 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -691,7 +691,7 @@ static long tce_iommu_create_window(struct tce_container *container,
container->tables[num] = tbl;
/* Return start address assigned by platform in create_table() */
- *start_addr = tbl->it_offset << tbl->it_page_shift;
+ *start_addr = (tbl->it_offset + tbl->it_tceoff) << tbl->it_page_shift;
return 0;
@@ -777,7 +777,7 @@ static long tce_iommu_ioctl(void *iommu_data,
unsigned int cmd, unsigned long arg)
{
struct tce_container *container = iommu_data;
- unsigned long minsz, ddwsz;
+ unsigned long minsz;
long ret;
switch (cmd) {
@@ -842,12 +842,13 @@ static long tce_iommu_ioctl(void *iommu_data,
info.ddw.levels = table_group->max_levels;
}
- ddwsz = offsetofend(struct vfio_iommu_spapr_tce_info, ddw);
+ info.flags |= VFIO_IOMMU_SPAPR_INFO_DDW_START;
+ info.dma64_window_start = table_group->tce64_start;
- if (info.argsz >= ddwsz)
- minsz = ddwsz;
+ if (info.argsz > sizeof(info))
+ info.argsz = sizeof(info);
- if (copy_to_user((void __user *)arg, &info, minsz))
+ if (copy_to_user((void __user *)arg, &info, info.argsz))
return -EFAULT;
return 0;
--
2.17.1
^ permalink raw reply related
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