From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751282AbdH1FUr (ORCPT ); Mon, 28 Aug 2017 01:20:47 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:54120 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751184AbdH1FUp (ORCPT ); Mon, 28 Aug 2017 01:20:45 -0400 Date: Sun, 27 Aug 2017 22:20:39 -0700 From: Sukadev Bhattiprolu To: Michael Ellerman Cc: Benjamin Herrenschmidt , 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 References: <1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com> <1503556688-15412-13-git-send-email-sukadev@linux.vnet.ibm.com> <87shgfuda6.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87shgfuda6.fsf@concordia.ellerman.id.au> X-Operating-System: Linux 2.0.32 on an i486 User-Agent: Mutt/1.7.1 (2016-10-04) X-TM-AS-GCONF: 00 x-cbid: 17082805-0004-0000-0000-000012D5C361 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007625; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000225; SDB=6.00908660; UDB=6.00455622; IPR=6.00688901; BA=6.00005555; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016893; XFM=3.00000015; UTC=2017-08-28 05:20:43 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17082805-0005-0000-0000-000080E2C9A6 Message-Id: <20170828052039.GG12907@us.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-28_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708280085 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michael Ellerman [mpe@ellerman.id.au] wrote: > Hi Suka, > > A few more things ... > > Sukadev Bhattiprolu 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. Ok. I thought they would come back later, but of course we can update these kernel-only calls then. > > > + > > +#define COPY(RA, RB, L) \ > > + .long (0x7c00060c | (RA) << (31-15) | (RB) << (31-20) \ > > + | (L) << (31-10)) > > Use PPC_COPY(). > Ok > > + > > +#define CR0_FXM "0x80" > > I don't think a #define for this helps readability. > > > +#define CR0_SHIFT 28 > > +#define CR0_MASK 0xF > > Not used. Will need them now to return value in cr0? > > > +/* > > + * 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. Ok, Haren's NX patches will need to drop those parameters as well. > > > + > > + __asm__ __volatile(stringify_in_c(COPY(%0, %1, %2))";" > > I've never seen __volatile before. > > Just use: asm volatile ok > > > > + : > > + : "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. ok > > > + 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. ok > > > + > > + 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. > ok > > > 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? Yes, I have dropped vas_initialized() now. > > > > cheers