Re: [PATCH v7 12/12] powerpc/vas: Define copy/paste interfaces
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2017-08-25 10:56:19
Also in:
lkml
Hi Suka, A few more things ... Sukadev Bhattiprolu [off-list ref] writes:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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