From: Michael Ellerman <mpe@ellerman•id.au>
To: Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel•crashing.org>,
mikey@neuling•org, stewart@linux•vnet.ibm.com,
apopple@au1•ibm.com, hbabu@us•ibm.com, oohall@gmail•com,
linuxppc-dev@ozlabs•org, linux-kernel@vger•kernel.org
Subject: Re: [PATCH v7 12/12] powerpc/vas: Define copy/paste interfaces
Date: Fri, 25 Aug 2017 20:56:01 +1000 [thread overview]
Message-ID: <87shgfuda6.fsf@concordia.ellerman.id.au> (raw)
In-Reply-To: <1503556688-15412-13-git-send-email-sukadev@linux.vnet.ibm.com>
Hi Suka,
A few more things ...
Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com> writes:
> diff --git a/arch/powerpc/platforms/powernv/copy-paste.h b/arch/powerpc/platforms/powernv/copy-paste.h
> new file mode 100644
> index 0000000..7783bb8
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/copy-paste.h
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +/*
> + * Macros taken from tools/testing/selftests/powerpc/context_switch/cp_abort.c
> + */
These are both out of date, they're changed in v3.0B.
> +#define PASTE(RA, RB, L, RC) \
> + .long (0x7c00070c | (RA) << (31-15) | (RB) << (31-20) \
> + | (L) << (31-10) | (RC) << (31-31))
You should define PPC_PASTE() in ppc-opcode.h
We already have PPC_INST_PASTE, so use that.
L and RC are gone.
> +
> +#define COPY(RA, RB, L) \
> + .long (0x7c00060c | (RA) << (31-15) | (RB) << (31-20) \
> + | (L) << (31-10))
Use PPC_COPY().
> +
> +#define CR0_FXM "0x80"
I don't think a #define for this helps readability.
> +#define CR0_SHIFT 28
> +#define CR0_MASK 0xF
Not used.
> +/*
> + * Copy/paste instructions:
> + *
> + * copy RA,RB,L
> + * Copy contents of address (RA) + effective_address(RB)
> + * to internal copy-buffer.
> + *
> + * L == 1 indicates this is the first copy.
> + *
> + * L == 0 indicates its a continuation of a prior first copy.
> + *
> + * paste RA,RB,L
> + * Paste contents of internal copy-buffer to the address
> + * (RA) + effective_address(RB)
> + *
> + * L == 0 indicates its a continuation of a prior paste. i.e.
> + * don't wait for the completion or update status.
> + *
> + * L == 1 indicates this is the last paste in the group (i.e.
> + * wait for the group to complete and update status in CR0).
> + *
> + * For Power9, the L bit must be 'true' in both copy and paste.
> + */
> +
> +static inline int vas_copy(void *crb, int offset, int first)
> +{
> + WARN_ON_ONCE(!first);
Please change the API to not require unused parameters.
Same for offset.
> +
> + __asm__ __volatile(stringify_in_c(COPY(%0, %1, %2))";"
I've never seen __volatile before.
Just use: asm volatile
> + :
> + : "b" (offset), "b" (crb), "i" (1)
> + : "memory");
> +
> + return 0;
> +}
> +
> +static inline int vas_paste(void *paste_address, int offset, int last)
> +{
> + unsigned long long cr;
cr is 32-bits actually.
> + WARN_ON_ONCE(!last);
> +
> + cr = 0;
> + __asm__ __volatile(stringify_in_c(PASTE(%1, %2, 1, 1))";"
> + "mfocrf %0," CR0_FXM ";"
> + : "=r" (cr)
> + : "b" (paste_address), "b" (offset)
> + : "memory");
You need cr0 in the clobbers.
> +
> + return cr;
I think it would be more natural if you just returned CR0, so if you did
shift and mask with the CR0 constants you have above.
> diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> index 70762c3..73081b4 100644
> --- a/arch/powerpc/platforms/powernv/vas-window.c
> +++ b/arch/powerpc/platforms/powernv/vas-window.c
> @@ -1040,6 +1041,57 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> }
> EXPORT_SYMBOL_GPL(vas_tx_win_open);
>
> +int vas_copy_crb(void *crb, int offset, bool first)
> +{
> + if (!vas_initialized())
> + return -1;
> +
> + return vas_copy(crb, offset, first);
> +}
> +EXPORT_SYMBOL_GPL(vas_copy_crb);
> +
> +#define RMA_LSMP_REPORT_ENABLE PPC_BIT(53)
> +int vas_paste_crb(struct vas_window *txwin, int offset, bool last, bool re)
> +{
> + int rc;
> + uint64_t val;
> + void *addr;
> +
> + if (!vas_initialized())
> + return -1;
This is in the fast path, or at least the runtime path. So I don't think
these checks are wanted, how would we have got this far if vas wasn't
initialised?
cheers
next prev parent reply other threads:[~2017-08-25 10:56 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 6:37 [PATCH v7 00/12] Enable VAS Sukadev Bhattiprolu
2017-08-24 6:37 ` [PATCH v7 01/12] powerpc/vas: Define macros, register fields and structures Sukadev Bhattiprolu
2017-08-25 9:47 ` Michael Ellerman
2017-08-24 6:37 ` [PATCH v7 02/12] Move GET_FIELD/SET_FIELD to vas.h Sukadev Bhattiprolu
2017-08-24 6:37 ` [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit() Sukadev Bhattiprolu
2017-08-24 11:51 ` Michael Ellerman
2017-08-24 21:43 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions Sukadev Bhattiprolu
2017-08-25 3:38 ` Michael Ellerman
2017-08-28 4:36 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 05/12] powerpc/vas: Define helpers to init window context Sukadev Bhattiprolu
2017-08-25 9:25 ` Michael Ellerman
2017-08-28 4:44 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free windows Sukadev Bhattiprolu
2017-08-25 9:35 ` Michael Ellerman
2017-08-28 4:52 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 07/12] powerpc/vas: Define vas_win_paste_addr() Sukadev Bhattiprolu
2017-08-25 9:36 ` Michael Ellerman
2017-08-24 6:38 ` [PATCH v7 08/12] powerpc/vas: Define vas_win_id() Sukadev Bhattiprolu
2017-08-25 9:37 ` Michael Ellerman
2017-08-28 4:53 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 09/12] powerpc/vas: Define vas_rx_win_open() interface Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Sukadev Bhattiprolu
2017-08-25 10:02 ` Michael Ellerman
2017-08-28 5:14 ` Sukadev Bhattiprolu
2017-08-28 11:43 ` Michael Ellerman
2017-08-24 6:38 ` [PATCH v7 11/12] powerpc/vas: Define vas_tx_win_open() Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 12/12] powerpc/vas: Define copy/paste interfaces Sukadev Bhattiprolu
2017-08-25 10:56 ` Michael Ellerman [this message]
2017-08-28 5:20 ` Sukadev Bhattiprolu
2017-08-28 11:45 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87shgfuda6.fsf@concordia.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=apopple@au1$(echo .)ibm.com \
--cc=benh@kernel$(echo .)crashing.org \
--cc=hbabu@us$(echo .)ibm.com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=mikey@neuling$(echo .)org \
--cc=oohall@gmail$(echo .)com \
--cc=stewart@linux$(echo .)vnet.ibm.com \
--cc=sukadev@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox