Re: [PATCH v2 00/10] riscv: RVA22U64 profile support

[Alistair Francis <alistair23@gmail.com>]

On Fri, Oct 13, 2023 at 5:07 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/11/23 00:01, Alistair Francis wrote:
> > On Sat, Oct 7, 2023 at 12:23 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Hi,
> >>
> >> Several design changes were made in this version after the reviews and
> >> feedback in the v1 [1]. The high-level summary is:
> >>
> >> - we'll no longer allow users to set profile flags for vendor CPUs. If
> >>    we're to adhere to the current policy of not allowing users to enable
> >>    extensions for vendor CPUs, the profile support would become a
> >>    glorified way of checking if the vendor CPU happens to support a
> >>    specific profile. If a future vendor CPU supports a profile the CPU
> >>    can declare it manually in its cpu_init() function, the flag will
> >>    still be set, but users can't change it;
> >>
> >> - disabling a profile will now disable all the mandatory extensions from
> >>    the CPU;
> >
> > What happens if you enable one profile and disable a different one?
>
> With this implementation as is the profiles will be evaluated by the order they're
> declared in riscv_cpu_profiles[]. Which isn't exactly ideal since we're exchanging
> a left-to-right ordering in the command line by an arbitrary order that we happened
> to set in the code.
>
> I can make some tweaks to make the profiles sensible to left-to-right order between
> them, while keeping regular extension with higher priority. e.g.:
>
>
> -cpu rv64,zicbom=true,profileA=false,profileB=true,zicboz=false
> -cpu rv64,profileA=false,zicbom=true,zicboz=false,profileB=true
> -cpu rv64,profileA=false,profileB=true,zicbom=true,zicboz=false

I think we should just not allow this.

I don't understand why a user would want this and what they mean here.
What if profileA and profileB have overlapping extensions?

Alistair

>
> These would all do the same thing: "keeping zicbom=true and zicboz=false, disable profileA
> and then enable profile B"
>
> Switching the profiles order would have a different result:
>
> -cpu rv64,profileB=true,profileA=false,zicbom=true,zicboz=false
>
> "keeping zicbom=true and zicboz=false, enable profile B and then disable profile A"
>
>
> I'm happy to hear any other alternative/ideas. We'll either deal with some left-to-right
> ordering w.r.t profiles or deal with an internal profile commit ordering. TBH I think
> it's sensible to demand left-to-right command line ordering for profiles only.
>
>
> Thanks,
>
>
> Daniel
>
>
>
>
>
> >
> > Alistair
> >
> >>
> >> - the profile logic was moved to realize() time in a step we're calling
> >>    'commit profile'. This allows us to enable/disable profile extensions
> >>    after considering user input in other individual extensions. The
> >>    result is that we don't care about the order in which the profile flag
> >>    was set in comparison with other extensions in the command line, i.e.
> >>    the following lines are equal:
> >>
> >>    -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> >>
> >>    -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> >>
> >>    and they mean 'enable the rva22u64 profile while keeping zicbom and
> >>    Zifencei disabled'.
> >>
> >>
> >> Other minor changes were needed as result of these design changes. E.g.
> >> we're now having to track MISA extensions set by users (patch 7),
> >> something that we were doing only for multi-letter extensions.
> >>
> >> Changes from v1:
> >> - patch 6 from v1 ("target/riscv/kvm: add 'rva22u64' flag as unavailable"):
> >>      - moved up to patch 4
> >> - patch 5 from v1("target/riscv/tcg-cpu.c: enable profile support for vendor CPUs"):
> >>      - dropped
> >> - patch 6 (new):
> >>    - add riscv_cpu_commit_profile()
> >> - patch 7 (new):
> >>    - add user choice hash for MISA extensions
> >> - patch 9 (new):
> >>    - handle MISA bits user choice when commiting profiles
> >> - patch 8 and 10 (new):
> >>    - helpers to avoid code repetition
> >> - v1 link: https://lore.kernel.org/qemu-riscv/20230926194951.183767-1-dbarboza@ventanamicro.com/
> >>
> >>
> >> Daniel Henrique Barboza (10):
> >>    target/riscv/cpu.c: add zicntr extension flag
> >>    target/riscv/cpu.c: add zihpm extension flag
> >>    target/riscv: add rva22u64 profile definition
> >>    target/riscv/kvm: add 'rva22u64' flag as unavailable
> >>    target/riscv/tcg: add user flag for profile support
> >>    target/riscv/tcg: commit profiles during realize()
> >>    target/riscv/tcg: add MISA user options hash
> >>    target/riscv/tcg: add riscv_cpu_write_misa_bit()
> >>    target/riscv/tcg: handle MISA bits on profile commit
> >>    target/riscv/tcg: add hash table insert helpers
> >>
> >>   target/riscv/cpu.c         |  29 +++++++
> >>   target/riscv/cpu.h         |  12 +++
> >>   target/riscv/cpu_cfg.h     |   2 +
> >>   target/riscv/kvm/kvm-cpu.c |   7 +-
> >>   target/riscv/tcg/tcg-cpu.c | 165 +++++++++++++++++++++++++++++++++----
> >>   5 files changed, 197 insertions(+), 18 deletions(-)
> >>
> >> --
> >> 2.41.0
> >>
> >>