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

[Daniel Henrique Barboza]

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

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