public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Christoph Muellner <cmuellner@ventanamicro.com>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: "Andrew Waterman" <andrew@sifive.com>,
	"Shihua Liao" <shihua@iscas.ac.cn>,
	binutils@sourceware.org, "Kito Cheng" <kito.cheng@sifive.com>,
	"Jim Wilson" <jim.wilson.gcc@gmail.com>,
	"Wei Wu (吴伟)" <lazyparser@gmail.com>, jiawei <jiawei@iscas.ac.cn>,
	"Nelson Chu" <nelson.chu@sifive.com>
Subject: Re: [PATCH] RISC-V: Support ZTSO extension
Date: Tue, 15 Mar 2022 21:22:04 +0100	[thread overview]
Message-ID: <CAHYeh+qSvFjVSq2HYgO0ZpDbP7qJmtXoYDsg1=uT8QskEB6sSw@mail.gmail.com> (raw)
In-Reply-To: <mhng-cd4e1c1c-ae04-41ee-92e7-16956bcbc467@palmer-ri-x1c9>

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?

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
> >>>
>

  parent reply	other threads:[~2022-03-15 20:22 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-15  2:43 shihua
2022-03-15 16:24 ` Christoph Muellner
2022-03-15 18:41   ` Andrew Waterman
2022-03-15 18:52     ` Palmer Dabbelt
2022-03-15 19:07       ` Andrew Waterman
2022-03-15 20:22       ` Christoph Muellner [this message]
2022-03-15 20:41         ` Palmer Dabbelt
2022-03-15 21:42           ` Christoph Muellner
2022-09-15 19:49 ` Vineet Gupta
2022-09-15 20:05   ` Christoph Muellner
2022-09-16 11:36     ` Palmer Dabbelt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHYeh+qSvFjVSq2HYgO0ZpDbP7qJmtXoYDsg1=uT8QskEB6sSw@mail.gmail.com' \
    --to=cmuellner@ventanamicro.com \
    --cc=andrew@sifive.com \
    --cc=binutils@sourceware.org \
    --cc=jiawei@iscas.ac.cn \
    --cc=jim.wilson.gcc@gmail.com \
    --cc=kito.cheng@sifive.com \
    --cc=lazyparser@gmail.com \
    --cc=nelson.chu@sifive.com \
    --cc=palmer@dabbelt.com \
    --cc=shihua@iscas.ac.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).