When compiling with toolchains that haven't yet been taught about
new instructions we need to encode them ourselves. This series
creates a new file where support for instruction definitions can
evolve. For starters the file is initiated with a macro for R-type
encodings. The series then applies the R-type encoding macro to all
instances of hard coded instruction definitions in KVM.
Not only should using instruction encoding macros improve readability
and maintainability of code, but we should also gain potential for
more optimized code after compilation as the compiler will have control
over the input and output registers used, which may provide more
opportunities for inlining.
I grepped for other places we may want to use these macros and the
only place I found was ALT_CMO_OP(), but I didn't dare touch it :-)
I do suggest we apply this to the Svinal support [1] as we won't
want to frustrate the compiler's inlining efforts with hard coded
register selection.
[1] https://lore.kernel.org/linux-riscv/[email protected]/
Andrew Jones (4):
riscv: Add X register names to gpr-nums
riscv: Introduce support for defining instructions
riscv: KVM: Apply insn-def to hfence encodings
riscv: KVM: Apply insn-def to hlv encodings
arch/riscv/Kconfig | 3 +
arch/riscv/include/asm/gpr-num.h | 8 ++
arch/riscv/include/asm/insn-def.h | 104 ++++++++++++++++++++++++++
arch/riscv/kvm/tlb.c | 117 ++++--------------------------
arch/riscv/kvm/vcpu_exit.c | 29 ++------
5 files changed, 133 insertions(+), 128 deletions(-)
create mode 100644 arch/riscv/include/asm/insn-def.h
--
2.37.1
When encoding instructions it's sometimes necessary to set a
register field to a precise number. This is easiest to do using
the x<num> naming.
Signed-off-by: Andrew Jones <[email protected]>
---
arch/riscv/include/asm/gpr-num.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/riscv/include/asm/gpr-num.h b/arch/riscv/include/asm/gpr-num.h
index dfee2829fc7c..efeb5edf8a3a 100644
--- a/arch/riscv/include/asm/gpr-num.h
+++ b/arch/riscv/include/asm/gpr-num.h
@@ -3,6 +3,11 @@
#define __ASM_GPR_NUM_H
#ifdef __ASSEMBLY__
+
+ .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
+ .equ .L__gpr_num_x\num, \num
+ .endr
+
.equ .L__gpr_num_zero, 0
.equ .L__gpr_num_ra, 1
.equ .L__gpr_num_sp, 2
@@ -39,6 +44,9 @@
#else /* __ASSEMBLY__ */
#define __DEFINE_ASM_GPR_NUMS \
+" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31\n" \
+" .equ .L__gpr_num_x\\num, \\num\n" \
+" .endr\n" \
" .equ .L__gpr_num_zero, 0\n" \
" .equ .L__gpr_num_ra, 1\n" \
" .equ .L__gpr_num_sp, 2\n" \
--
2.37.1
When compiling with toolchains that haven't yet been taught about
new instructions we need to encode them ourselves. Create a new file
where support for instruction definitions will evolve. We initiate
the file with a macro called INSN_R(), which implements the R-type
instruction encoding. INSN_R() will use the assembler's .insn
directive when available, which should give the assembler a chance
to do some validation. When .insn is not available we fall back to
manual encoding.
Not only should using instruction encoding macros improve readability
and maintainability of code over the alternative of inserting
instructions directly (e.g. '.word 0xc0de'), but we should also gain
potential for more optimized code after compilation because the
compiler will have control over the input and output registers used.
Signed-off-by: Andrew Jones <[email protected]>
---
arch/riscv/Kconfig | 3 ++
arch/riscv/include/asm/insn-def.h | 82 +++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+)
create mode 100644 arch/riscv/include/asm/insn-def.h
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index ed66c31e4655..f8f3b316b838 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -227,6 +227,9 @@ config RISCV_DMA_NONCOHERENT
select ARCH_HAS_SETUP_DMA_OPS
select DMA_DIRECT_REMAP
+config AS_HAS_INSN
+ def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero)
+
source "arch/riscv/Kconfig.socs"
source "arch/riscv/Kconfig.erratas"
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
new file mode 100644
index 000000000000..4cd0208068dd
--- /dev/null
+++ b/arch/riscv/include/asm/insn-def.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_INSN_DEF_H
+#define __ASM_INSN_DEF_H
+#include <asm/asm.h>
+
+#define INSN_R_FUNC7_SHIFT 25
+#define INSN_R_RS2_SHIFT 20
+#define INSN_R_RS1_SHIFT 15
+#define INSN_R_FUNC3_SHIFT 12
+#define INSN_R_RD_SHIFT 7
+#define INSN_R_OPCODE_SHIFT 0
+
+#ifdef __ASSEMBLY__
+
+#ifdef CONFIG_AS_HAS_INSN
+
+ .macro insn_r, opcode, func3, func7, rd, rs1, rs2
+ .insn r \opcode, \func3, \func7, \rd, \rs1, \rs2
+ .endm
+
+#else
+
+#include <asm/gpr-num.h>
+
+ .macro insn_r, opcode, func3, func7, rd, rs1, rs2
+ .4byte ((\opcode << INSN_R_OPCODE_SHIFT) | \
+ (\func3 << INSN_R_FUNC3_SHIFT) | \
+ (\func7 << INSN_R_FUNC7_SHIFT) | \
+ (.L__gpr_num_\rd << INSN_R_RD_SHIFT) | \
+ (.L__gpr_num_\rs1 << INSN_R_RS1_SHIFT) | \
+ (.L__gpr_num_\rs2 << INSN_R_RS2_SHIFT))
+ .endm
+
+#endif
+
+#define INSN_R(...) insn_r __VA_ARGS__
+
+#else /* ! __ASSEMBLY__ */
+
+#ifdef CONFIG_AS_HAS_INSN
+
+#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
+ ".insn r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n"
+
+#else
+
+#include <linux/stringify.h>
+#include <asm/gpr-num.h>
+
+#define DEFINE_INSN_R \
+ __DEFINE_ASM_GPR_NUMS \
+" .macro insn_r, opcode, func3, func7, rd, rs1, rs2\n" \
+" .4byte ((\\opcode << " __stringify(INSN_R_OPCODE_SHIFT) ") |" \
+" (\\func3 << " __stringify(INSN_R_FUNC3_SHIFT) ") |" \
+" (\\func7 << " __stringify(INSN_R_FUNC7_SHIFT) ") |" \
+" (.L__gpr_num_\\rd << " __stringify(INSN_R_RD_SHIFT) ") |" \
+" (.L__gpr_num_\\rs1 << " __stringify(INSN_R_RS1_SHIFT) ") |" \
+" (.L__gpr_num_\\rs2 << " __stringify(INSN_R_RS2_SHIFT) "))\n" \
+" .endm\n"
+
+#define UNDEFINE_INSN_R \
+" .purgem insn_r\n"
+
+#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
+ DEFINE_INSN_R \
+ "insn_r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n" \
+ UNDEFINE_INSN_R
+
+#endif
+
+#endif /* ! __ASSEMBLY__ */
+
+#define OPCODE(v) __ASM_STR(v)
+#define FUNC3(v) __ASM_STR(v)
+#define FUNC7(v) __ASM_STR(v)
+#define __REG(v) __ASM_STR(x ## v)
+#define RD(v) __REG(v)
+#define RS1(v) __REG(v)
+#define RS2(v) __REG(v)
+
+#endif /* __ASM_INSN_DEF_H */
--
2.37.1
Introduce hfence instruction encodings and apply them to KVM's use.
With the self-documenting nature of the instruction encoding macros,
and a spec always within arm's reach, it's safe to remove the
comments, so we do that too.
Signed-off-by: Andrew Jones <[email protected]>
---
arch/riscv/include/asm/insn-def.h | 8 ++
arch/riscv/kvm/tlb.c | 117 ++++--------------------------
2 files changed, 21 insertions(+), 104 deletions(-)
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index 4cd0208068dd..cd1c0d365f47 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -79,4 +79,12 @@
#define RS1(v) __REG(v)
#define RS2(v) __REG(v)
+#define OPCODE_SYSTEM OPCODE(115)
+
+#define HFENCE_VVMA(vaddr, asid) \
+ INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(17), RD(0), vaddr, asid)
+
+#define HFENCE_GVMA(gaddr, vmid) \
+ INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
+
#endif /* __ASM_INSN_DEF_H */
diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
index 1a76d0b1907d..f742a0d888e1 100644
--- a/arch/riscv/kvm/tlb.c
+++ b/arch/riscv/kvm/tlb.c
@@ -12,22 +12,7 @@
#include <linux/kvm_host.h>
#include <asm/cacheflush.h>
#include <asm/csr.h>
-
-/*
- * Instruction encoding of hfence.gvma is:
- * HFENCE.GVMA rs1, rs2
- * HFENCE.GVMA zero, rs2
- * HFENCE.GVMA rs1
- * HFENCE.GVMA
- *
- * rs1!=zero and rs2!=zero ==> HFENCE.GVMA rs1, rs2
- * rs1==zero and rs2!=zero ==> HFENCE.GVMA zero, rs2
- * rs1!=zero and rs2==zero ==> HFENCE.GVMA rs1
- * rs1==zero and rs2==zero ==> HFENCE.GVMA
- *
- * Instruction encoding of HFENCE.GVMA is:
- * 0110001 rs2(5) rs1(5) 000 00000 1110011
- */
+#include <asm/insn-def.h>
void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
gpa_t gpa, gpa_t gpsz,
@@ -41,31 +26,14 @@ void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
}
for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
- /*
- * rs1 = a0 (GPA >> 2)
- * rs2 = a1 (VMID)
- * HFENCE.GVMA a0, a1
- * 0110001 01011 01010 000 00000 1110011
- */
- asm volatile ("srli a0, %0, 2\n"
- "add a1, %1, zero\n"
- ".word 0x62b50073\n"
- :: "r" (pos), "r" (vmid)
- : "a0", "a1", "memory");
+ asm volatile (HFENCE_GVMA("%0", "%1")
+ : : "r" (pos >> 2), "r" (vmid) : "memory");
}
}
void kvm_riscv_local_hfence_gvma_vmid_all(unsigned long vmid)
{
- /*
- * rs1 = zero
- * rs2 = a0 (VMID)
- * HFENCE.GVMA zero, a0
- * 0110001 01010 00000 000 00000 1110011
- */
- asm volatile ("add a0, %0, zero\n"
- ".word 0x62a00073\n"
- :: "r" (vmid) : "a0", "memory");
+ asm volatile(HFENCE_GVMA("zero", "%0") : : "r" (vmid) : "memory");
}
void kvm_riscv_local_hfence_gvma_gpa(gpa_t gpa, gpa_t gpsz,
@@ -79,45 +47,16 @@ void kvm_riscv_local_hfence_gvma_gpa(gpa_t gpa, gpa_t gpsz,
}
for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
- /*
- * rs1 = a0 (GPA >> 2)
- * rs2 = zero
- * HFENCE.GVMA a0
- * 0110001 00000 01010 000 00000 1110011
- */
- asm volatile ("srli a0, %0, 2\n"
- ".word 0x62050073\n"
- :: "r" (pos) : "a0", "memory");
+ asm volatile(HFENCE_GVMA("%0", "zero")
+ : : "r" (pos >> 2) : "memory");
}
}
void kvm_riscv_local_hfence_gvma_all(void)
{
- /*
- * rs1 = zero
- * rs2 = zero
- * HFENCE.GVMA
- * 0110001 00000 00000 000 00000 1110011
- */
- asm volatile (".word 0x62000073" ::: "memory");
+ asm volatile(HFENCE_GVMA("zero", "zero") : : : "memory");
}
-/*
- * Instruction encoding of hfence.gvma is:
- * HFENCE.VVMA rs1, rs2
- * HFENCE.VVMA zero, rs2
- * HFENCE.VVMA rs1
- * HFENCE.VVMA
- *
- * rs1!=zero and rs2!=zero ==> HFENCE.VVMA rs1, rs2
- * rs1==zero and rs2!=zero ==> HFENCE.VVMA zero, rs2
- * rs1!=zero and rs2==zero ==> HFENCE.VVMA rs1
- * rs1==zero and rs2==zero ==> HFENCE.VVMA
- *
- * Instruction encoding of HFENCE.VVMA is:
- * 0010001 rs2(5) rs1(5) 000 00000 1110011
- */
-
void kvm_riscv_local_hfence_vvma_asid_gva(unsigned long vmid,
unsigned long asid,
unsigned long gva,
@@ -134,17 +73,8 @@ void kvm_riscv_local_hfence_vvma_asid_gva(unsigned long vmid,
hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
for (pos = gva; pos < (gva + gvsz); pos += BIT(order)) {
- /*
- * rs1 = a0 (GVA)
- * rs2 = a1 (ASID)
- * HFENCE.VVMA a0, a1
- * 0010001 01011 01010 000 00000 1110011
- */
- asm volatile ("add a0, %0, zero\n"
- "add a1, %1, zero\n"
- ".word 0x22b50073\n"
- :: "r" (pos), "r" (asid)
- : "a0", "a1", "memory");
+ asm volatile(HFENCE_VVMA("%0", "%1")
+ : : "r" (pos), "r" (asid) : "memory");
}
csr_write(CSR_HGATP, hgatp);
@@ -157,15 +87,7 @@ void kvm_riscv_local_hfence_vvma_asid_all(unsigned long vmid,
hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
- /*
- * rs1 = zero
- * rs2 = a0 (ASID)
- * HFENCE.VVMA zero, a0
- * 0010001 01010 00000 000 00000 1110011
- */
- asm volatile ("add a0, %0, zero\n"
- ".word 0x22a00073\n"
- :: "r" (asid) : "a0", "memory");
+ asm volatile(HFENCE_VVMA("zero", "%0") : : "r" (asid) : "memory");
csr_write(CSR_HGATP, hgatp);
}
@@ -184,15 +106,8 @@ void kvm_riscv_local_hfence_vvma_gva(unsigned long vmid,
hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
for (pos = gva; pos < (gva + gvsz); pos += BIT(order)) {
- /*
- * rs1 = a0 (GVA)
- * rs2 = zero
- * HFENCE.VVMA a0
- * 0010001 00000 01010 000 00000 1110011
- */
- asm volatile ("add a0, %0, zero\n"
- ".word 0x22050073\n"
- :: "r" (pos) : "a0", "memory");
+ asm volatile(HFENCE_VVMA("%0", "zero")
+ : : "r" (pos) : "memory");
}
csr_write(CSR_HGATP, hgatp);
@@ -204,13 +119,7 @@ void kvm_riscv_local_hfence_vvma_all(unsigned long vmid)
hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
- /*
- * rs1 = zero
- * rs2 = zero
- * HFENCE.VVMA
- * 0010001 00000 00000 000 00000 1110011
- */
- asm volatile (".word 0x22000073" ::: "memory");
+ asm volatile(HFENCE_VVMA("zero", "zero") : : : "memory");
csr_write(CSR_HGATP, hgatp);
}
--
2.37.1
Introduce hlv instruction encodings and apply them to KVM's use.
We're careful not to introduce hlv.d to 32-bit builds. Indeed,
we ensure the build fails if someone tries to use it.
Signed-off-by: Andrew Jones <[email protected]>
---
arch/riscv/include/asm/insn-def.h | 14 ++++++++++++++
arch/riscv/kvm/vcpu_exit.c | 29 +++++------------------------
2 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index cd1c0d365f47..c66d5745c5b4 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -87,4 +87,18 @@
#define HFENCE_GVMA(gaddr, vmid) \
INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
+#define HLVX_HU(dest, addr) \
+ INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(50), dest, addr, RS2(3))
+
+#define HLV_W(dest, addr) \
+ INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(52), dest, addr, RS2(0))
+
+#ifdef CONFIG_64BIT
+#define HLV_D(dest, addr) \
+ INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(54), dest, addr, RS2(0))
+#else
+#define HLV_D(dest, addr) \
+ __ASM_STR(.error "hlv.d requires 64-bit support")
+#endif
+
#endif /* __ASM_INSN_DEF_H */
diff --git a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c
index d5c36386878a..9cb075e72799 100644
--- a/arch/riscv/kvm/vcpu_exit.c
+++ b/arch/riscv/kvm/vcpu_exit.c
@@ -8,6 +8,7 @@
#include <linux/kvm_host.h>
#include <asm/csr.h>
+#include <asm/insn-def.h>
static int gstage_page_fault(struct kvm_vcpu *vcpu, struct kvm_run *run,
struct kvm_cpu_trap *trap)
@@ -82,22 +83,12 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
".option push\n"
".option norvc\n"
"add %[ttmp], %[taddr], 0\n"
- /*
- * HLVX.HU %[val], (%[addr])
- * HLVX.HU t0, (t2)
- * 0110010 00011 00111 100 00101 1110011
- */
- ".word 0x6433c2f3\n"
+ HLVX_HU("%[val]", "%[addr]")
"andi %[tmp], %[val], 3\n"
"addi %[tmp], %[tmp], -3\n"
"bne %[tmp], zero, 2f\n"
"addi %[addr], %[addr], 2\n"
- /*
- * HLVX.HU %[tmp], (%[addr])
- * HLVX.HU t1, (t2)
- * 0110010 00011 00111 100 00110 1110011
- */
- ".word 0x6433c373\n"
+ HLVX_HU("%[tmp]", "%[addr]")
"sll %[tmp], %[tmp], 16\n"
"add %[val], %[val], %[tmp]\n"
"2:\n"
@@ -121,19 +112,9 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
".option norvc\n"
"add %[ttmp], %[taddr], 0\n"
#ifdef CONFIG_64BIT
- /*
- * HLV.D %[val], (%[addr])
- * HLV.D t0, (t2)
- * 0110110 00000 00111 100 00101 1110011
- */
- ".word 0x6c03c2f3\n"
+ HLV_D("%[val]", "%[addr]")
#else
- /*
- * HLV.W %[val], (%[addr])
- * HLV.W t0, (t2)
- * 0110100 00000 00111 100 00101 1110011
- */
- ".word 0x6803c2f3\n"
+ HLV_W("%[val]", "%[addr]")
#endif
".option pop"
: [val] "=&r" (val),
--
2.37.1
On Fri, Aug 19, 2022 at 04:02:48PM +0200, Andrew Jones wrote:
> When compiling with toolchains that haven't yet been taught about
> new instructions we need to encode them ourselves. Create a new file
> where support for instruction definitions will evolve. We initiate
> the file with a macro called INSN_R(), which implements the R-type
> instruction encoding. INSN_R() will use the assembler's .insn
> directive when available, which should give the assembler a chance
> to do some validation. When .insn is not available we fall back to
> manual encoding.
>
> Not only should using instruction encoding macros improve readability
> and maintainability of code over the alternative of inserting
> instructions directly (e.g. '.word 0xc0de'), but we should also gain
> potential for more optimized code after compilation because the
> compiler will have control over the input and output registers used.
>
> Signed-off-by: Andrew Jones <[email protected]>
> ---
> arch/riscv/Kconfig | 3 ++
> arch/riscv/include/asm/insn-def.h | 82 +++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+)
> create mode 100644 arch/riscv/include/asm/insn-def.h
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index ed66c31e4655..f8f3b316b838 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -227,6 +227,9 @@ config RISCV_DMA_NONCOHERENT
> select ARCH_HAS_SETUP_DMA_OPS
> select DMA_DIRECT_REMAP
>
> +config AS_HAS_INSN
> + def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero)
> +
> source "arch/riscv/Kconfig.socs"
> source "arch/riscv/Kconfig.erratas"
>
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> new file mode 100644
> index 000000000000..4cd0208068dd
> --- /dev/null
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_INSN_DEF_H
> +#define __ASM_INSN_DEF_H
> +#include <asm/asm.h>
> +
> +#define INSN_R_FUNC7_SHIFT 25
> +#define INSN_R_RS2_SHIFT 20
> +#define INSN_R_RS1_SHIFT 15
> +#define INSN_R_FUNC3_SHIFT 12
> +#define INSN_R_RD_SHIFT 7
> +#define INSN_R_OPCODE_SHIFT 0
> +
> +#ifdef __ASSEMBLY__
> +
> +#ifdef CONFIG_AS_HAS_INSN
> +
> + .macro insn_r, opcode, func3, func7, rd, rs1, rs2
> + .insn r \opcode, \func3, \func7, \rd, \rs1, \rs2
> + .endm
> +
> +#else
> +
> +#include <asm/gpr-num.h>
> +
> + .macro insn_r, opcode, func3, func7, rd, rs1, rs2
> + .4byte ((\opcode << INSN_R_OPCODE_SHIFT) | \
> + (\func3 << INSN_R_FUNC3_SHIFT) | \
> + (\func7 << INSN_R_FUNC7_SHIFT) | \
> + (.L__gpr_num_\rd << INSN_R_RD_SHIFT) | \
> + (.L__gpr_num_\rs1 << INSN_R_RS1_SHIFT) | \
> + (.L__gpr_num_\rs2 << INSN_R_RS2_SHIFT))
> + .endm
Doh, checkpatch told me about the spaces vs. tabs before .endm, but I
forgot to refresh before sending. I won't send a v2 until the series
gets more feedback, but I've already got it fixed.
> +
> +#endif
> +
> +#define INSN_R(...) insn_r __VA_ARGS__
> +
> +#else /* ! __ASSEMBLY__ */
> +
> +#ifdef CONFIG_AS_HAS_INSN
> +
> +#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
> + ".insn r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n"
> +
> +#else
> +
> +#include <linux/stringify.h>
> +#include <asm/gpr-num.h>
> +
> +#define DEFINE_INSN_R \
> + __DEFINE_ASM_GPR_NUMS \
> +" .macro insn_r, opcode, func3, func7, rd, rs1, rs2\n" \
> +" .4byte ((\\opcode << " __stringify(INSN_R_OPCODE_SHIFT) ") |" \
^
^ checkpatch didn't tell me those are spaces instead of a
tab, but I found it while fixing the other tab issue.
> +" (\\func3 << " __stringify(INSN_R_FUNC3_SHIFT) ") |" \
> +" (\\func7 << " __stringify(INSN_R_FUNC7_SHIFT) ") |" \
> +" (.L__gpr_num_\\rd << " __stringify(INSN_R_RD_SHIFT) ") |" \
> +" (.L__gpr_num_\\rs1 << " __stringify(INSN_R_RS1_SHIFT) ") |" \
> +" (.L__gpr_num_\\rs2 << " __stringify(INSN_R_RS2_SHIFT) "))\n" \
> +" .endm\n"
> +
> +#define UNDEFINE_INSN_R \
> +" .purgem insn_r\n"
> +
> +#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
> + DEFINE_INSN_R \
> + "insn_r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n" \
> + UNDEFINE_INSN_R
> +
> +#endif
> +
> +#endif /* ! __ASSEMBLY__ */
> +
> +#define OPCODE(v) __ASM_STR(v)
> +#define FUNC3(v) __ASM_STR(v)
> +#define FUNC7(v) __ASM_STR(v)
> +#define __REG(v) __ASM_STR(x ## v)
> +#define RD(v) __REG(v)
> +#define RS1(v) __REG(v)
> +#define RS2(v) __REG(v)
> +
> +#endif /* __ASM_INSN_DEF_H */
> --
> 2.37.1
>
Thanks,
drew
On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
>
> Introduce hfence instruction encodings and apply them to KVM's use.
> With the self-documenting nature of the instruction encoding macros,
> and a spec always within arm's reach, it's safe to remove the
> comments, so we do that too.
>
> Signed-off-by: Andrew Jones <[email protected]>
> ---
> arch/riscv/include/asm/insn-def.h | 8 ++
> arch/riscv/kvm/tlb.c | 117 ++++--------------------------
> 2 files changed, 21 insertions(+), 104 deletions(-)
>
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> index 4cd0208068dd..cd1c0d365f47 100644
> --- a/arch/riscv/include/asm/insn-def.h
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -79,4 +79,12 @@
> #define RS1(v) __REG(v)
> #define RS2(v) __REG(v)
>
> +#define OPCODE_SYSTEM OPCODE(115)
> +
> +#define HFENCE_VVMA(vaddr, asid) \
> + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(17), RD(0), vaddr, asid)
> +
> +#define HFENCE_GVMA(gaddr, vmid) \
> + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
> +
> #endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
> index 1a76d0b1907d..f742a0d888e1 100644
> --- a/arch/riscv/kvm/tlb.c
> +++ b/arch/riscv/kvm/tlb.c
> @@ -12,22 +12,7 @@
> #include <linux/kvm_host.h>
> #include <asm/cacheflush.h>
> #include <asm/csr.h>
> -
> -/*
> - * Instruction encoding of hfence.gvma is:
> - * HFENCE.GVMA rs1, rs2
> - * HFENCE.GVMA zero, rs2
> - * HFENCE.GVMA rs1
> - * HFENCE.GVMA
> - *
> - * rs1!=zero and rs2!=zero ==> HFENCE.GVMA rs1, rs2
> - * rs1==zero and rs2!=zero ==> HFENCE.GVMA zero, rs2
> - * rs1!=zero and rs2==zero ==> HFENCE.GVMA rs1
> - * rs1==zero and rs2==zero ==> HFENCE.GVMA
> - *
> - * Instruction encoding of HFENCE.GVMA is:
> - * 0110001 rs2(5) rs1(5) 000 00000 1110011
> - */
> +#include <asm/insn-def.h>
>
> void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> gpa_t gpa, gpa_t gpsz,
> @@ -41,31 +26,14 @@ void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> }
>
> for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
> - /*
> - * rs1 = a0 (GPA >> 2)
> - * rs2 = a1 (VMID)
> - * HFENCE.GVMA a0, a1
> - * 0110001 01011 01010 000 00000 1110011
> - */
> - asm volatile ("srli a0, %0, 2\n"
> - "add a1, %1, zero\n"
> - ".word 0x62b50073\n"
> - :: "r" (pos), "r" (vmid)
> - : "a0", "a1", "memory");
> + asm volatile (HFENCE_GVMA("%0", "%1")
> + : : "r" (pos >> 2), "r" (vmid) : "memory");
You can drop the for-loop braces "{ }"
> }
> }
>
> void kvm_riscv_local_hfence_gvma_vmid_all(unsigned long vmid)
> {
> - /*
> - * rs1 = zero
> - * rs2 = a0 (VMID)
> - * HFENCE.GVMA zero, a0
> - * 0110001 01010 00000 000 00000 1110011
> - */
> - asm volatile ("add a0, %0, zero\n"
> - ".word 0x62a00073\n"
> - :: "r" (vmid) : "a0", "memory");
> + asm volatile(HFENCE_GVMA("zero", "%0") : : "r" (vmid) : "memory");
> }
>
> void kvm_riscv_local_hfence_gvma_gpa(gpa_t gpa, gpa_t gpsz,
> @@ -79,45 +47,16 @@ void kvm_riscv_local_hfence_gvma_gpa(gpa_t gpa, gpa_t gpsz,
> }
>
> for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
> - /*
> - * rs1 = a0 (GPA >> 2)
> - * rs2 = zero
> - * HFENCE.GVMA a0
> - * 0110001 00000 01010 000 00000 1110011
> - */
> - asm volatile ("srli a0, %0, 2\n"
> - ".word 0x62050073\n"
> - :: "r" (pos) : "a0", "memory");
> + asm volatile(HFENCE_GVMA("%0", "zero")
> + : : "r" (pos >> 2) : "memory");
You can drop the for-loop braces "{ }"
> }
> }
>
> void kvm_riscv_local_hfence_gvma_all(void)
> {
> - /*
> - * rs1 = zero
> - * rs2 = zero
> - * HFENCE.GVMA
> - * 0110001 00000 00000 000 00000 1110011
> - */
> - asm volatile (".word 0x62000073" ::: "memory");
> + asm volatile(HFENCE_GVMA("zero", "zero") : : : "memory");
> }
>
> -/*
> - * Instruction encoding of hfence.gvma is:
> - * HFENCE.VVMA rs1, rs2
> - * HFENCE.VVMA zero, rs2
> - * HFENCE.VVMA rs1
> - * HFENCE.VVMA
> - *
> - * rs1!=zero and rs2!=zero ==> HFENCE.VVMA rs1, rs2
> - * rs1==zero and rs2!=zero ==> HFENCE.VVMA zero, rs2
> - * rs1!=zero and rs2==zero ==> HFENCE.VVMA rs1
> - * rs1==zero and rs2==zero ==> HFENCE.VVMA
> - *
> - * Instruction encoding of HFENCE.VVMA is:
> - * 0010001 rs2(5) rs1(5) 000 00000 1110011
> - */
> -
> void kvm_riscv_local_hfence_vvma_asid_gva(unsigned long vmid,
> unsigned long asid,
> unsigned long gva,
> @@ -134,17 +73,8 @@ void kvm_riscv_local_hfence_vvma_asid_gva(unsigned long vmid,
> hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
>
> for (pos = gva; pos < (gva + gvsz); pos += BIT(order)) {
> - /*
> - * rs1 = a0 (GVA)
> - * rs2 = a1 (ASID)
> - * HFENCE.VVMA a0, a1
> - * 0010001 01011 01010 000 00000 1110011
> - */
> - asm volatile ("add a0, %0, zero\n"
> - "add a1, %1, zero\n"
> - ".word 0x22b50073\n"
> - :: "r" (pos), "r" (asid)
> - : "a0", "a1", "memory");
> + asm volatile(HFENCE_VVMA("%0", "%1")
> + : : "r" (pos), "r" (asid) : "memory");
You can drop the for-loop braces "{ }"
> }
>
> csr_write(CSR_HGATP, hgatp);
> @@ -157,15 +87,7 @@ void kvm_riscv_local_hfence_vvma_asid_all(unsigned long vmid,
>
> hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
>
> - /*
> - * rs1 = zero
> - * rs2 = a0 (ASID)
> - * HFENCE.VVMA zero, a0
> - * 0010001 01010 00000 000 00000 1110011
> - */
> - asm volatile ("add a0, %0, zero\n"
> - ".word 0x22a00073\n"
> - :: "r" (asid) : "a0", "memory");
> + asm volatile(HFENCE_VVMA("zero", "%0") : : "r" (asid) : "memory");
>
> csr_write(CSR_HGATP, hgatp);
> }
> @@ -184,15 +106,8 @@ void kvm_riscv_local_hfence_vvma_gva(unsigned long vmid,
> hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
>
> for (pos = gva; pos < (gva + gvsz); pos += BIT(order)) {
> - /*
> - * rs1 = a0 (GVA)
> - * rs2 = zero
> - * HFENCE.VVMA a0
> - * 0010001 00000 01010 000 00000 1110011
> - */
> - asm volatile ("add a0, %0, zero\n"
> - ".word 0x22050073\n"
> - :: "r" (pos) : "a0", "memory");
> + asm volatile(HFENCE_VVMA("%0", "zero")
> + : : "r" (pos) : "memory");
> }
>
> csr_write(CSR_HGATP, hgatp);
> @@ -204,13 +119,7 @@ void kvm_riscv_local_hfence_vvma_all(unsigned long vmid)
>
> hgatp = csr_swap(CSR_HGATP, vmid << HGATP_VMID_SHIFT);
>
> - /*
> - * rs1 = zero
> - * rs2 = zero
> - * HFENCE.VVMA
> - * 0010001 00000 00000 000 00000 1110011
> - */
> - asm volatile (".word 0x22000073" ::: "memory");
> + asm volatile(HFENCE_VVMA("zero", "zero") : : : "memory");
>
> csr_write(CSR_HGATP, hgatp);
> }
> --
> 2.37.1
>
Apart from a few nits above, this looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
>
> Introduce hlv instruction encodings and apply them to KVM's use.
> We're careful not to introduce hlv.d to 32-bit builds. Indeed,
> we ensure the build fails if someone tries to use it.
>
> Signed-off-by: Andrew Jones <[email protected]>
> ---
> arch/riscv/include/asm/insn-def.h | 14 ++++++++++++++
> arch/riscv/kvm/vcpu_exit.c | 29 +++++------------------------
> 2 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> index cd1c0d365f47..c66d5745c5b4 100644
> --- a/arch/riscv/include/asm/insn-def.h
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -87,4 +87,18 @@
> #define HFENCE_GVMA(gaddr, vmid) \
> INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
>
> +#define HLVX_HU(dest, addr) \
> + INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(50), dest, addr, RS2(3))
> +
> +#define HLV_W(dest, addr) \
> + INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(52), dest, addr, RS2(0))
> +
> +#ifdef CONFIG_64BIT
> +#define HLV_D(dest, addr) \
> + INSN_R(OPCODE_SYSTEM, FUNC3(4), FUNC7(54), dest, addr, RS2(0))
> +#else
> +#define HLV_D(dest, addr) \
> + __ASM_STR(.error "hlv.d requires 64-bit support")
> +#endif
> +
> #endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c
> index d5c36386878a..9cb075e72799 100644
> --- a/arch/riscv/kvm/vcpu_exit.c
> +++ b/arch/riscv/kvm/vcpu_exit.c
> @@ -8,6 +8,7 @@
>
> #include <linux/kvm_host.h>
> #include <asm/csr.h>
> +#include <asm/insn-def.h>
>
> static int gstage_page_fault(struct kvm_vcpu *vcpu, struct kvm_run *run,
> struct kvm_cpu_trap *trap)
> @@ -82,22 +83,12 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
> ".option push\n"
> ".option norvc\n"
> "add %[ttmp], %[taddr], 0\n"
> - /*
> - * HLVX.HU %[val], (%[addr])
> - * HLVX.HU t0, (t2)
> - * 0110010 00011 00111 100 00101 1110011
> - */
> - ".word 0x6433c2f3\n"
> + HLVX_HU("%[val]", "%[addr]")
> "andi %[tmp], %[val], 3\n"
> "addi %[tmp], %[tmp], -3\n"
> "bne %[tmp], zero, 2f\n"
> "addi %[addr], %[addr], 2\n"
> - /*
> - * HLVX.HU %[tmp], (%[addr])
> - * HLVX.HU t1, (t2)
> - * 0110010 00011 00111 100 00110 1110011
> - */
> - ".word 0x6433c373\n"
> + HLVX_HU("%[tmp]", "%[addr]")
> "sll %[tmp], %[tmp], 16\n"
> "add %[val], %[val], %[tmp]\n"
> "2:\n"
> @@ -121,19 +112,9 @@ unsigned long kvm_riscv_vcpu_unpriv_read(struct kvm_vcpu *vcpu,
> ".option norvc\n"
> "add %[ttmp], %[taddr], 0\n"
> #ifdef CONFIG_64BIT
> - /*
> - * HLV.D %[val], (%[addr])
> - * HLV.D t0, (t2)
> - * 0110110 00000 00111 100 00101 1110011
> - */
> - ".word 0x6c03c2f3\n"
> + HLV_D("%[val]", "%[addr]")
> #else
> - /*
> - * HLV.W %[val], (%[addr])
> - * HLV.W t0, (t2)
> - * 0110100 00000 00111 100 00101 1110011
> - */
> - ".word 0x6803c2f3\n"
> + HLV_W("%[val]", "%[addr]")
> #endif
> ".option pop"
> : [val] "=&r" (val),
> --
> 2.37.1
>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
>
> When encoding instructions it's sometimes necessary to set a
> register field to a precise number. This is easiest to do using
> the x<num> naming.
>
> Signed-off-by: Andrew Jones <[email protected]>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
> ---
> arch/riscv/include/asm/gpr-num.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/riscv/include/asm/gpr-num.h b/arch/riscv/include/asm/gpr-num.h
> index dfee2829fc7c..efeb5edf8a3a 100644
> --- a/arch/riscv/include/asm/gpr-num.h
> +++ b/arch/riscv/include/asm/gpr-num.h
> @@ -3,6 +3,11 @@
> #define __ASM_GPR_NUM_H
>
> #ifdef __ASSEMBLY__
> +
> + .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31
> + .equ .L__gpr_num_x\num, \num
> + .endr
> +
> .equ .L__gpr_num_zero, 0
> .equ .L__gpr_num_ra, 1
> .equ .L__gpr_num_sp, 2
> @@ -39,6 +44,9 @@
> #else /* __ASSEMBLY__ */
>
> #define __DEFINE_ASM_GPR_NUMS \
> +" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31\n" \
> +" .equ .L__gpr_num_x\\num, \\num\n" \
> +" .endr\n" \
> " .equ .L__gpr_num_zero, 0\n" \
> " .equ .L__gpr_num_ra, 1\n" \
> " .equ .L__gpr_num_sp, 2\n" \
> --
> 2.37.1
>
On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
>
> When compiling with toolchains that haven't yet been taught about
> new instructions we need to encode them ourselves. Create a new file
> where support for instruction definitions will evolve. We initiate
> the file with a macro called INSN_R(), which implements the R-type
> instruction encoding. INSN_R() will use the assembler's .insn
> directive when available, which should give the assembler a chance
> to do some validation. When .insn is not available we fall back to
> manual encoding.
>
> Not only should using instruction encoding macros improve readability
> and maintainability of code over the alternative of inserting
> instructions directly (e.g. '.word 0xc0de'), but we should also gain
> potential for more optimized code after compilation because the
> compiler will have control over the input and output registers used.
>
> Signed-off-by: Andrew Jones <[email protected]>
> ---
> arch/riscv/Kconfig | 3 ++
> arch/riscv/include/asm/insn-def.h | 82 +++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+)
> create mode 100644 arch/riscv/include/asm/insn-def.h
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index ed66c31e4655..f8f3b316b838 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -227,6 +227,9 @@ config RISCV_DMA_NONCOHERENT
> select ARCH_HAS_SETUP_DMA_OPS
> select DMA_DIRECT_REMAP
>
> +config AS_HAS_INSN
> + def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero)
> +
> source "arch/riscv/Kconfig.socs"
> source "arch/riscv/Kconfig.erratas"
>
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> new file mode 100644
> index 000000000000..4cd0208068dd
> --- /dev/null
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_INSN_DEF_H
> +#define __ASM_INSN_DEF_H
Add a new line here.
> +#include <asm/asm.h>
> +
> +#define INSN_R_FUNC7_SHIFT 25
> +#define INSN_R_RS2_SHIFT 20
> +#define INSN_R_RS1_SHIFT 15
> +#define INSN_R_FUNC3_SHIFT 12
> +#define INSN_R_RD_SHIFT 7
> +#define INSN_R_OPCODE_SHIFT 0
> +
> +#ifdef __ASSEMBLY__
> +
> +#ifdef CONFIG_AS_HAS_INSN
> +
> + .macro insn_r, opcode, func3, func7, rd, rs1, rs2
> + .insn r \opcode, \func3, \func7, \rd, \rs1, \rs2
> + .endm
Maybe use indentation here ?
.macro <xyz>
.insn ...
.endm
> +
> +#else
> +
> +#include <asm/gpr-num.h>
> +
> + .macro insn_r, opcode, func3, func7, rd, rs1, rs2
> + .4byte ((\opcode << INSN_R_OPCODE_SHIFT) | \
> + (\func3 << INSN_R_FUNC3_SHIFT) | \
> + (\func7 << INSN_R_FUNC7_SHIFT) | \
> + (.L__gpr_num_\rd << INSN_R_RD_SHIFT) | \
> + (.L__gpr_num_\rs1 << INSN_R_RS1_SHIFT) | \
> + (.L__gpr_num_\rs2 << INSN_R_RS2_SHIFT))
> + .endm
> +
> +#endif
> +
> +#define INSN_R(...) insn_r __VA_ARGS__
> +
> +#else /* ! __ASSEMBLY__ */
> +
> +#ifdef CONFIG_AS_HAS_INSN
> +
> +#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
> + ".insn r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n"
> +
> +#else
> +
> +#include <linux/stringify.h>
> +#include <asm/gpr-num.h>
> +
> +#define DEFINE_INSN_R \
> + __DEFINE_ASM_GPR_NUMS \
> +" .macro insn_r, opcode, func3, func7, rd, rs1, rs2\n" \
> +" .4byte ((\\opcode << " __stringify(INSN_R_OPCODE_SHIFT) ") |" \
> +" (\\func3 << " __stringify(INSN_R_FUNC3_SHIFT) ") |" \
> +" (\\func7 << " __stringify(INSN_R_FUNC7_SHIFT) ") |" \
> +" (.L__gpr_num_\\rd << " __stringify(INSN_R_RD_SHIFT) ") |" \
> +" (.L__gpr_num_\\rs1 << " __stringify(INSN_R_RS1_SHIFT) ") |" \
> +" (.L__gpr_num_\\rs2 << " __stringify(INSN_R_RS2_SHIFT) "))\n" \
> +" .endm\n"
> +
> +#define UNDEFINE_INSN_R \
> +" .purgem insn_r\n"
> +
> +#define INSN_R(opcode, func3, func7, rd, rs1, rs2) \
> + DEFINE_INSN_R \
> + "insn_r " opcode ", " func3 ", " func7 ", " rd ", " rs1 ", " rs2 "\n" \
> + UNDEFINE_INSN_R
> +
> +#endif
> +
> +#endif /* ! __ASSEMBLY__ */
> +
> +#define OPCODE(v) __ASM_STR(v)
> +#define FUNC3(v) __ASM_STR(v)
> +#define FUNC7(v) __ASM_STR(v)
> +#define __REG(v) __ASM_STR(x ## v)
> +#define RD(v) __REG(v)
> +#define RS1(v) __REG(v)
> +#define RS2(v) __REG(v)
> +
> +#endif /* __ASM_INSN_DEF_H */
> --
> 2.37.1
>
Apart from new nit comments above, this looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On Mon, Aug 29, 2022 at 02:07:59PM +0530, Anup Patel wrote:
> On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
> >
> > Introduce hfence instruction encodings and apply them to KVM's use.
> > With the self-documenting nature of the instruction encoding macros,
> > and a spec always within arm's reach, it's safe to remove the
> > comments, so we do that too.
> >
> > Signed-off-by: Andrew Jones <[email protected]>
> > ---
> > arch/riscv/include/asm/insn-def.h | 8 ++
> > arch/riscv/kvm/tlb.c | 117 ++++--------------------------
> > 2 files changed, 21 insertions(+), 104 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> > index 4cd0208068dd..cd1c0d365f47 100644
> > --- a/arch/riscv/include/asm/insn-def.h
> > +++ b/arch/riscv/include/asm/insn-def.h
> > @@ -79,4 +79,12 @@
> > #define RS1(v) __REG(v)
> > #define RS2(v) __REG(v)
> >
> > +#define OPCODE_SYSTEM OPCODE(115)
> > +
> > +#define HFENCE_VVMA(vaddr, asid) \
> > + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(17), RD(0), vaddr, asid)
> > +
> > +#define HFENCE_GVMA(gaddr, vmid) \
> > + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
> > +
> > #endif /* __ASM_INSN_DEF_H */
> > diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
> > index 1a76d0b1907d..f742a0d888e1 100644
> > --- a/arch/riscv/kvm/tlb.c
> > +++ b/arch/riscv/kvm/tlb.c
> > @@ -12,22 +12,7 @@
> > #include <linux/kvm_host.h>
> > #include <asm/cacheflush.h>
> > #include <asm/csr.h>
> > -
> > -/*
> > - * Instruction encoding of hfence.gvma is:
> > - * HFENCE.GVMA rs1, rs2
> > - * HFENCE.GVMA zero, rs2
> > - * HFENCE.GVMA rs1
> > - * HFENCE.GVMA
> > - *
> > - * rs1!=zero and rs2!=zero ==> HFENCE.GVMA rs1, rs2
> > - * rs1==zero and rs2!=zero ==> HFENCE.GVMA zero, rs2
> > - * rs1!=zero and rs2==zero ==> HFENCE.GVMA rs1
> > - * rs1==zero and rs2==zero ==> HFENCE.GVMA
> > - *
> > - * Instruction encoding of HFENCE.GVMA is:
> > - * 0110001 rs2(5) rs1(5) 000 00000 1110011
> > - */
> > +#include <asm/insn-def.h>
> >
> > void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> > gpa_t gpa, gpa_t gpsz,
> > @@ -41,31 +26,14 @@ void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> > }
> >
> > for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
> > - /*
> > - * rs1 = a0 (GPA >> 2)
> > - * rs2 = a1 (VMID)
> > - * HFENCE.GVMA a0, a1
> > - * 0110001 01011 01010 000 00000 1110011
> > - */
> > - asm volatile ("srli a0, %0, 2\n"
> > - "add a1, %1, zero\n"
> > - ".word 0x62b50073\n"
> > - :: "r" (pos), "r" (vmid)
> > - : "a0", "a1", "memory");
> > + asm volatile (HFENCE_GVMA("%0", "%1")
Thank you for the review, Anup! I'd also like to get opinions on whether
the caller should quote the register tokens or the call should be made as,
e.g. HFENCE_GVMA(%0, %1), and then do the quoting inside the macro for
C callers. I could go either way, but I'm starting to lean towards moving
the quoting into the macros.
Thanks,
drew
On Mon, Aug 29, 2022 at 3:17 PM Andrew Jones <[email protected]> wrote:
>
> On Mon, Aug 29, 2022 at 02:07:59PM +0530, Anup Patel wrote:
> > On Fri, Aug 19, 2022 at 7:32 PM Andrew Jones <[email protected]> wrote:
> > >
> > > Introduce hfence instruction encodings and apply them to KVM's use.
> > > With the self-documenting nature of the instruction encoding macros,
> > > and a spec always within arm's reach, it's safe to remove the
> > > comments, so we do that too.
> > >
> > > Signed-off-by: Andrew Jones <[email protected]>
> > > ---
> > > arch/riscv/include/asm/insn-def.h | 8 ++
> > > arch/riscv/kvm/tlb.c | 117 ++++--------------------------
> > > 2 files changed, 21 insertions(+), 104 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> > > index 4cd0208068dd..cd1c0d365f47 100644
> > > --- a/arch/riscv/include/asm/insn-def.h
> > > +++ b/arch/riscv/include/asm/insn-def.h
> > > @@ -79,4 +79,12 @@
> > > #define RS1(v) __REG(v)
> > > #define RS2(v) __REG(v)
> > >
> > > +#define OPCODE_SYSTEM OPCODE(115)
> > > +
> > > +#define HFENCE_VVMA(vaddr, asid) \
> > > + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(17), RD(0), vaddr, asid)
> > > +
> > > +#define HFENCE_GVMA(gaddr, vmid) \
> > > + INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(49), RD(0), gaddr, vmid)
> > > +
> > > #endif /* __ASM_INSN_DEF_H */
> > > diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
> > > index 1a76d0b1907d..f742a0d888e1 100644
> > > --- a/arch/riscv/kvm/tlb.c
> > > +++ b/arch/riscv/kvm/tlb.c
> > > @@ -12,22 +12,7 @@
> > > #include <linux/kvm_host.h>
> > > #include <asm/cacheflush.h>
> > > #include <asm/csr.h>
> > > -
> > > -/*
> > > - * Instruction encoding of hfence.gvma is:
> > > - * HFENCE.GVMA rs1, rs2
> > > - * HFENCE.GVMA zero, rs2
> > > - * HFENCE.GVMA rs1
> > > - * HFENCE.GVMA
> > > - *
> > > - * rs1!=zero and rs2!=zero ==> HFENCE.GVMA rs1, rs2
> > > - * rs1==zero and rs2!=zero ==> HFENCE.GVMA zero, rs2
> > > - * rs1!=zero and rs2==zero ==> HFENCE.GVMA rs1
> > > - * rs1==zero and rs2==zero ==> HFENCE.GVMA
> > > - *
> > > - * Instruction encoding of HFENCE.GVMA is:
> > > - * 0110001 rs2(5) rs1(5) 000 00000 1110011
> > > - */
> > > +#include <asm/insn-def.h>
> > >
> > > void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> > > gpa_t gpa, gpa_t gpsz,
> > > @@ -41,31 +26,14 @@ void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
> > > }
> > >
> > > for (pos = gpa; pos < (gpa + gpsz); pos += BIT(order)) {
> > > - /*
> > > - * rs1 = a0 (GPA >> 2)
> > > - * rs2 = a1 (VMID)
> > > - * HFENCE.GVMA a0, a1
> > > - * 0110001 01011 01010 000 00000 1110011
> > > - */
> > > - asm volatile ("srli a0, %0, 2\n"
> > > - "add a1, %1, zero\n"
> > > - ".word 0x62b50073\n"
> > > - :: "r" (pos), "r" (vmid)
> > > - : "a0", "a1", "memory");
> > > + asm volatile (HFENCE_GVMA("%0", "%1")
>
> Thank you for the review, Anup! I'd also like to get opinions on whether
> the caller should quote the register tokens or the call should be made as,
> e.g. HFENCE_GVMA(%0, %1), and then do the quoting inside the macro for
> C callers. I could go either way, but I'm starting to lean towards moving
> the quoting into the macros.
I am fine with the current approach but doing quoting inside macors will
certainly be more user friendly.
Regards,
Anup