Re: [PATCH] RISC-V: Support ZTSO extension

[Palmer Dabbelt <palmer@dabbelt.com>]

On Tue, 15 Mar 2022 13:22:04 PDT (-0700), cmuellner@ventanamicro.com wrote:
> On Tue, Mar 15, 2022 at 7:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>> On Tue, 15 Mar 2022 11:41:14 PDT (-0700), Andrew Waterman wrote:
>> > On Tue, Mar 15, 2022 at 9:24 AM Christoph Muellner
>> > <cmuellner@ventanamicro.com> wrote:
>> >>
>> >>
>> >>
>> >> On Tue, Mar 15, 2022 at 3:44 AM <shihua@iscas.ac.cn> wrote:
>> >>>
>> >>> From: LiaoShihua <shihua@iscas.ac.cn>
>> >>>
>> >>>    ZTSO is the extension of tatol store order model.
>> >>
>> >>
>> >> typo: tatol -> total
>> >>
>> >>>
>> >>>    This extension adds no new instructions to the ISA, and you can use
>> it with arch "ztso".
>> >>>    If you use it, TSO flag will be generate in the ELF header.
>> >>>
>> >>> bfd\ChangeLog:
>> >>>
>> >>>         * elfnn-riscv.c (_bfd_riscv_elf_merge_private_bfd_data):Add
>> support for ztso extension.
>> >>>         * elfxx-riscv.c (riscv_multi_subset_supports):Ditto.
>> >>>
>> >>> binutils\ChangeLog:
>> >>>
>> >>>         * readelf.c (get_machine_flags):Ditto.
>> >>>
>> >>> gas\ChangeLog:
>> >>>
>> >>>         * config/tc-riscv.c (struct riscv_set_options):Ditto.
>> >>>         (riscv_set_tso):Ditto.
>> >>>         (riscv_set_arch):Ditto.
>> >>>
>> >>> include\ChangeLog:
>> >>>
>> >>>         * elf/riscv.h (EF_RISCV_TSO):Ditto.
>> >>>         * opcode/riscv.h (enum riscv_insn_class):Ditto.
>> >>>
>> >>> ---
>> >>>  bfd/elfnn-riscv.c      |  3 +++
>> >>>  bfd/elfxx-riscv.c      |  3 +++
>> >>>  binutils/readelf.c     |  3 +++
>> >>>  gas/config/tc-riscv.c  | 17 +++++++++++++++++
>> >>>  include/elf/riscv.h    |  3 +++
>> >>>  include/opcode/riscv.h |  1 +
>> >>>  6 files changed, 30 insertions(+)
>> >>>
>> >>> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>> >>> index 8f9f0d8a86a..25e8082b957 100644
>> >>> --- a/bfd/elfnn-riscv.c
>> >>> +++ b/bfd/elfnn-riscv.c
>> >>> @@ -3886,6 +3886,9 @@ _bfd_riscv_elf_merge_private_bfd_data (bfd
>> *ibfd, struct bfd_link_info *info)
>> >>>    /* Allow linking RVC and non-RVC, and keep the RVC flag.  */
>> >>>    elf_elfheader (obfd)->e_flags |= new_flags & EF_RISCV_RVC;
>> >>>
>> >>> +  /* Allow linking ZTSO and non-ZTSO, and keep the TSO flag.  */
>> >>> +  elf_elfheader (obfd)->e_flags |= new_flags & EF_RISCV_TSO;
>> >>
>> >>
>> >> Is this flag evaluated anywhere?
>> >> I.e. how do we protect users from executing TSO binaries on RVWMO
>> systems?
>> >> In the case of e.g. compressed instructions, they will run into a
>> SIG_ILL, but here they will observe unpredictable behavior if we don't do
>> anything.
>> >
>> > The intent had always been that the ELF loaders (OS kernel, dynamic
>> > linker, etc.) would enforce this property.  The work hasn't been done
>> > for the Linux kernel or glibc, AFAIK, presumably because Ztso hasn't
>> > been a priority.  The EF_RISCV_TSO bit has long been reserved for this
>> > purpose, though, and so nothing is holding up that work.  (The Linux
>> > kernel will need to be informed about the presence of Ztso via DT, and
>> > the dynamic linker will need to be informed via HWCAP.  Until all of
>> > that plumbing exists, these loaders should conservatively reject Ztso
>> > binaries.)
>>
>> IIUC we're half way there: glibc is already rejecting dynamic libraries
>> with the TSO bit set, but Linux appears to ignore the flags entirely.  I
>> think that means static binaries with the TSO bit would end up loaded,
>> but IMO that's just a Linux bug and it shouldn't be that hard to fix.
>>
>
> Do you think you will find some time to address this in the kernel or
> should somebody else work this out?

Something like this should do it

   diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
   index f53c40026c7a..5d55021a11ff 100644
   --- a/arch/riscv/include/asm/elf.h
   +++ b/arch/riscv/include/asm/elf.h
   @@ -28,8 +28,11 @@
   
    /*
     * This is used to ensure we don't load something for the wrong architecture.
   + * We currently have no TSO implementations and don't probe for it, so just go
   + * ahead and disallow binaries with the TSO flag set.
     */
   -#define elf_check_arch(x) ((x)->e_machine == EM_RISCV)
   +#define EF_RISCV_TSO (1 << 3)
   +#define elf_check_arch(x) ((x)->e_machine == EM_RISCV && !((x)->e_flags & EF_RISCV_TSO))
   
    #define CORE_DUMP_USE_REGSET
    #define ELF_EXEC_PAGESIZE	(PAGE_SIZE)

but I haven't event built it (much less tested it).  It's probably worth 
breaking that out into a function and providing a more useful error 
message too, not sure if there's a smarter way to fit that all together.  
I'm pretty buried right now so if you want to pick it up that's always 
appreciated, otherwise it'll go in the pile -- not sure it's all that 
important, as we don't have any TSO userspace, but it wouldn't hurt.

>
> Thanks,
> Christoph
>
>
>>
>> My bigger worry here is allowing TSO to be flipped on before we actually
>> know what the memory model is.  I'm not sure what the right way to go is
>> here: we've got a "it's TSO" extension ratified, but there's no formal
>> description of the memory model.  I know we're relying on poorly defined
>> semantics for other stuff, but IMO memory model land is just too
>> complicated for that sort of thing.
>>
>> If there's actual hardware implementations of this around then it's a
>> different story, but without that I'd be somewhat reluctant to start
>> generating TSO binaries as then we'll be stuck remaining compatible with
>> whatever's allowed by Ztso v0.1.
>>
>> >
>> >>
>> >>>
>> >>> +
>> >>>    return true;
>> >>>
>> >>>   fail:
>> >>> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>> >>> index 2915b74dd0f..a041c89a623 100644
>> >>> --- a/bfd/elfxx-riscv.c
>> >>> +++ b/bfd/elfxx-riscv.c
>> >>> @@ -1215,6 +1215,7 @@ static struct riscv_supported_ext
>> riscv_supported_std_z_ext[] =
>> >>>    {"zvl16384b",                ISA_SPEC_CLASS_DRAFT,           1, 0,
>> 0 },
>> >>>    {"zvl32768b",                ISA_SPEC_CLASS_DRAFT,           1, 0,
>> 0 },
>> >>>    {"zvl65536b",                ISA_SPEC_CLASS_DRAFT,           1, 0,
>> 0 },
>> >>> +  {"ztso",             ISA_SPEC_CLASS_DRAFT,           0, 1,  0 },
>>
>> Ztso v0.1 is in 20191213, so at least we don't have to call it a draft.
>>
>> >>>    {NULL, 0, 0, 0, 0}
>> >>>  };
>> >>>
>> >>> @@ -2393,6 +2394,8 @@ riscv_multi_subset_supports
>> (riscv_parse_subset_t *rps,
>> >>>               || riscv_subset_supports (rps, "zve32f"));
>> >>>      case INSN_CLASS_SVINVAL:
>> >>>        return riscv_subset_supports (rps, "svinval");
>> >>> +    case INSN_CLASS_ZTSO:
>> >>> +      return riscv_subset_supports (rps, "ztso");
>> >>>      default:
>> >>>        rps->error_handler
>> >>>          (_("internal: unreachable INSN_CLASS_*"));
>> >>> diff --git a/binutils/readelf.c b/binutils/readelf.c
>> >>> index 16efe1dfd2d..ba4d6f9db4f 100644
>> >>> --- a/binutils/readelf.c
>> >>> +++ b/binutils/readelf.c
>> >>> @@ -3975,6 +3975,9 @@ get_machine_flags (Filedata * filedata, unsigned
>> e_flags, unsigned e_machine)
>> >>>
>> >>>           if (e_flags & EF_RISCV_RVE)
>> >>>             strcat (buf, ", RVE");
>> >>> +
>> >>> +         if (e_flags & EF_RISCV_TSO)
>> >>> +           strcat (buf, ", TSO");
>> >>>
>> >>>           switch (e_flags & EF_RISCV_FLOAT_ABI)
>> >>>             {
>> >>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> >>> index 9cc0abfda88..ed33cfa919a 100644
>> >>> --- a/gas/config/tc-riscv.c
>> >>> +++ b/gas/config/tc-riscv.c
>> >>> @@ -222,6 +222,7 @@ struct riscv_set_options
>> >>>    int relax; /* Emit relocs the linker is allowed to relax.  */
>> >>>    int arch_attr; /* Emit architecture and privileged elf attributes.
>> */
>> >>>    int csr_check; /* Enable the CSR checking.  */
>> >>> +  int tso; /* Use TSO model.  */
>> >>>  };
>> >>>
>> >>>  static struct riscv_set_options riscv_opts =
>> >>> @@ -231,6 +232,7 @@ static struct riscv_set_options riscv_opts =
>> >>>    1, /* relax */
>> >>>    DEFAULT_RISCV_ATTR, /* arch_attr */
>> >>>    0, /* csr_check */
>> >>> +  0, /* tso */
>> >>>  };
>> >>>
>> >>>  /* Enable or disable the rvc flags for riscv_opts.  Turn on the rvc
>> flag
>> >>> @@ -245,6 +247,18 @@ riscv_set_rvc (bool rvc_value)
>> >>>    riscv_opts.rvc = rvc_value;
>> >>>  }
>> >>>
>> >>> +/* Enable or disable the tso flags for riscv_opts.  Turn on the tso
>> flag
>> >>> +   for elf_flags once we have enabled ztso extension.  */
>> >>> +
>> >>> +static void
>> >>> +riscv_set_tso (bool tso_value)
>> >>> +{
>> >>> +  if (tso_value)
>> >>> +    elf_flags |= EF_RISCV_TSO;
>> >>> +
>> >>> +  riscv_opts.tso = tso_value;
>> >>> +}
>> >>> +
>> >>>  /* This linked list records all enabled extensions, which are parsed
>> from
>> >>>     the architecture string.  The architecture string can be set by the
>> >>>     -march option, the elf architecture attributes, and the --with-arch
>> >>> @@ -295,6 +309,9 @@ riscv_set_arch (const char *s)
>> >>>    riscv_set_rvc (false);
>> >>>    if (riscv_subset_supports (&riscv_rps_as, "c"))
>> >>>      riscv_set_rvc (true);
>> >>> +
>> >>> +  if (riscv_subset_supports (&riscv_rps_as, "ztso"))
>> >>> +    riscv_set_tso (true);
>> >>>  }
>> >>>
>> >>>  /* Indicate -mabi option is explictly set.  */
>> >>> diff --git a/include/elf/riscv.h b/include/elf/riscv.h
>> >>> index d0acf6886d8..eed3ec5f82e 100644
>> >>> --- a/include/elf/riscv.h
>> >>> +++ b/include/elf/riscv.h
>> >>> @@ -114,6 +114,9 @@ END_RELOC_NUMBERS (R_RISCV_max)
>> >>>  /* File uses the 32E base integer instruction.  */
>> >>>  #define EF_RISCV_RVE 0x0008
>> >>>
>> >>> +/* File uses the TSO model. */
>> >>> +#define EF_RISCV_TSO 0x0010
>> >>> +
>> >>>  /* The name of the global pointer symbol.  */
>> >>>  #define RISCV_GP_SYMBOL "__global_pointer$"
>> >>>
>> >>> diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
>> >>> index 048ab0a5d68..ed81df271c1 100644
>> >>> --- a/include/opcode/riscv.h
>> >>> +++ b/include/opcode/riscv.h
>> >>> @@ -388,6 +388,7 @@ enum riscv_insn_class
>> >>>    INSN_CLASS_V,
>> >>>    INSN_CLASS_ZVEF,
>> >>>    INSN_CLASS_SVINVAL,
>> >>> +  INSN_CLASS_ZTSO,
>> >>>  };
>> >>>
>> >>>  /* This structure holds information for a particular instruction.  */
>> >>> --
>> >>> 2.31.1.windows.1
>> >>>
>>