[Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions

Alistair Francis posted 23 patches 6 years, 11 months ago
There is a newer version of this series
[Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Alistair Francis 6 years, 11 months ago
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Michael Clark <mjc@sifive.com>
---
 tcg/riscv/tcg-target.inc.c | 51 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tcg/riscv/tcg-target.inc.c b/tcg/riscv/tcg-target.inc.c
index d402e48cbf..475feca906 100644
--- a/tcg/riscv/tcg-target.inc.c
+++ b/tcg/riscv/tcg-target.inc.c
@@ -371,6 +371,57 @@ static void tcg_out_opc_jump(TCGContext *s, RISCVInsn opc,
     tcg_out32(s, encode_uj(opc, rd, imm));
 }
 
+/*
+ * Relocations
+ */
+
+static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
+{
+    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
+    tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1);
+
+    code_ptr[0] |= encode_sbimm12(offset);
+}
+
+static void reloc_jimm20(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
+{
+    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
+    tcg_debug_assert(offset == sextract32(offset, 1, 20) << 1);
+
+    code_ptr[0] |= encode_ujimm12(offset);
+}
+
+static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
+{
+    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
+    tcg_debug_assert(offset == (int32_t)offset);
+
+    int32_t hi20 = ((offset + 0x800) >> 12) << 12;
+    int32_t lo12 = offset - hi20;
+
+    code_ptr[0] |= encode_uimm20(hi20);
+    code_ptr[1] |= encode_imm12(lo12);
+}
+
+static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+                        intptr_t value, intptr_t addend)
+{
+    tcg_debug_assert(addend == 0);
+    switch (type) {
+    case R_RISCV_BRANCH:
+        reloc_sbimm12(code_ptr, (tcg_insn_unit *)value);
+        break;
+    case R_RISCV_JAL:
+        reloc_jimm20(code_ptr, (tcg_insn_unit *)value);
+        break;
+    case R_RISCV_CALL:
+        reloc_call(code_ptr, (tcg_insn_unit *)value);
+        break;
+    default:
+        tcg_abort();
+    }
+}
+
 void tb_target_set_jmp_target(uintptr_t tc_ptr, uintptr_t jmp_addr,
                               uintptr_t addr)
 {
-- 
2.19.1


Re: [Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Richard Henderson 6 years, 11 months ago
On 11/15/18 11:35 PM, Alistair Francis wrote:
> +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> +{
> +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
> +    tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1);
> +
> +    code_ptr[0] |= encode_sbimm12(offset);
> +}

FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool
for relocation success, which will move these asserts.

http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html


> +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> +{
> +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
> +    tcg_debug_assert(offset == (int32_t)offset);
> +
> +    int32_t hi20 = ((offset + 0x800) >> 12) << 12;
> +    int32_t lo12 = offset - hi20;
> +
> +    code_ptr[0] |= encode_uimm20(hi20);
> +    code_ptr[1] |= encode_imm12(lo12);
> +}
> +

This is ok for patching during generation, but it is not ok for
tb_target_set_jmp_target from patch 9.

Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit?
 If not, you may be just better off using the indirect method.


r~

Re: [Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Alistair Francis 6 years, 11 months ago
On Fri, Nov 16, 2018 at 12:33 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/15/18 11:35 PM, Alistair Francis wrote:
> > +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> > +{
> > +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
> > +    tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1);
> > +
> > +    code_ptr[0] |= encode_sbimm12(offset);
> > +}
>
> FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool
> for relocation success, which will move these asserts.
>
> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html

Thanks, I'll keep an eye on this.

>
>
> > +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> > +{
> > +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
> > +    tcg_debug_assert(offset == (int32_t)offset);
> > +
> > +    int32_t hi20 = ((offset + 0x800) >> 12) << 12;
> > +    int32_t lo12 = offset - hi20;
> > +
> > +    code_ptr[0] |= encode_uimm20(hi20);
> > +    code_ptr[1] |= encode_imm12(lo12);
> > +}
> > +
>
> This is ok for patching during generation, but it is not ok for
> tb_target_set_jmp_target from patch 9.
>
> Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit?
>  If not, you may be just better off using the indirect method.

I'm not sure. Is the indirect method just using atomic set, because
that is what I have now?

Alistair

>
>
> r~

Re: [Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Richard Henderson 6 years, 11 months ago
On 11/21/18 2:15 AM, Alistair Francis wrote:
>> Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit?
>>  If not, you may be just better off using the indirect method.
> 
> I'm not sure. Is the indirect method just using atomic set, because
> that is what I have now?

It's controlled by TCG_TARGET_HAS_direct_jump, and goes through here:

> +    case INDEX_op_goto_tb:
> +        if (s->tb_jmp_insn_offset) {
> +            /* direct jump method */
> +            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
> +            /* should align on 64-bit boundary for atomic patching */
> +            tcg_out_opc_upper(s, OPC_AUIPC, TCG_REG_TMP0, 0);
> +            tcg_out_opc_imm(s, OPC_JALR, TCG_REG_ZERO, TCG_REG_TMP0, 0);
> +        } else {
> +            /* indirect jump method */
> +            tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP0, TCG_REG_ZERO,
> +                       (uintptr_t)(s->tb_jmp_target_addr + a0));
> +            tcg_out_opc_imm(s, OPC_JALR, TCG_REG_ZERO, TCG_REG_TMP0, 0);
> +        }
> +        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
> +        break;

where as you've correctly implemented, the indirect jump loads a destination
from memory (atomically, via a single load insn), and then branches to it.

The direct jump method avoids the load from memory, but then one has to be able
to modify the insn(s) that perform the jump with a single atomic_set.  Which in
this case, means that the two insns must be aligned so that we can perform a
single aligned 64-bit store.

I recommend at least starting with the indirect method because it's easier.


r~

Re: [Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Palmer Dabbelt 6 years, 11 months ago
On Tue, 20 Nov 2018 17:15:11 PST (-0800), alistair23@gmail.com wrote:
> On Fri, Nov 16, 2018 at 12:33 AM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 11/15/18 11:35 PM, Alistair Francis wrote:
>> > +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>> > +{
>> > +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
>> > +    tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1);
>> > +
>> > +    code_ptr[0] |= encode_sbimm12(offset);
>> > +}
>>
>> FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool
>> for relocation success, which will move these asserts.
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html
>
> Thanks, I'll keep an eye on this.
>
>>
>>
>> > +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>> > +{
>> > +    intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
>> > +    tcg_debug_assert(offset == (int32_t)offset);
>> > +
>> > +    int32_t hi20 = ((offset + 0x800) >> 12) << 12;
>> > +    int32_t lo12 = offset - hi20;
>> > +
>> > +    code_ptr[0] |= encode_uimm20(hi20);
>> > +    code_ptr[1] |= encode_imm12(lo12);
>> > +}
>> > +
>>
>> This is ok for patching during generation, but it is not ok for
>> tb_target_set_jmp_target from patch 9.
>>
>> Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit?
>>  If not, you may be just better off using the indirect method.
>
> I'm not sure. Is the indirect method just using atomic set, because
> that is what I have now?

Per the memory model chapter (which is being ratified now), FSD is not atomic on rv32i:

    An FLD or FSD instruction for which XLEN<64 may also be decomposed into a 
    set of component memory operations of any granularity.

So they should not be used on RV32I hosts to provide atomicity, even if they 
may be atomic on some microarchitectures.

Re: [Qemu-devel] [RFC v1 11/23] riscv: tcg-target: Add the relocation functions
Posted by Richard Henderson 6 years, 11 months ago
On 11/21/18 4:53 PM, Palmer Dabbelt wrote:
> 
> Per the memory model chapter (which is being ratified now), FSD is not atomic
> on rv32i:
> 
>    An FLD or FSD instruction for which XLEN<64 may also be decomposed into a   
> set of component memory operations of any granularity.

Ah, thanks.  I knew I should have checked the manual.  Anyway, that means
*only* the indirect goto_tb method is viable for rv32.


r~