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(-)
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;
- 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
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?
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
>
>
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
>>
>>
On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza 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 > > 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. left-to-right ordering is how the rest of QEMU properties work and scripts depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS where $MORE_PROPS can not only add more props but also override default props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on). left-to-right also works with multiple -cpu parameters, i.e. -cpu $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props with my props. I don't think profiles should be treated special with regard to this. They should behave the same as any property. If one does profileA=off,profileB=on and there are overlapping extensions then a sanity check in cpu-finalize should catch that and error out. Otherwise, why not. Profiles are just like big 'G' extensions and 'G' would behave the same way. Thanks, drew
On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza 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
> >
> > 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.
>
> left-to-right ordering is how the rest of QEMU properties work and scripts
> depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS
> where $MORE_PROPS can not only add more props but also override default
> props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on).
> left-to-right also works with multiple -cpu parameters, i.e. -cpu
> $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props
> with my props.
That seems like the way to go then
>
> I don't think profiles should be treated special with regard to this. They
> should behave the same as any property. If one does
> profileA=off,profileB=on and there are overlapping extensions then a
But what does this mean? What intent is the user saying here?
For example if a user says:
RVA22U64=off,RVA24U64=on
They only want the extensions that were added in RVA24U64? What about
G and the standard extensions?
To me it just seems really strange to have more than 1 profile.
Profiles are there to help software and users have common platforms.
Why would a user want to mix-n-match them
Alistair
> sanity check in cpu-finalize should catch that and error out. Otherwise,
> why not. Profiles are just like big 'G' extensions and 'G' would behave
> the same way.
>
> Thanks,
> drew
On Tue, Oct 17, 2023 at 01:55:47PM +1000, Alistair Francis wrote: > On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza 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 > > > > > > 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. > > > > left-to-right ordering is how the rest of QEMU properties work and scripts > > depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS > > where $MORE_PROPS can not only add more props but also override default > > props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on). > > left-to-right also works with multiple -cpu parameters, i.e. -cpu > > $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props > > with my props. > > That seems like the way to go then > > > > > I don't think profiles should be treated special with regard to this. They > > should behave the same as any property. If one does > > profileA=off,profileB=on and there are overlapping extensions then a > > But what does this mean? What intent is the user saying here? > > For example if a user says: > > RVA22U64=off,RVA24U64=on > > They only want the extensions that were added in RVA24U64? What about > G and the standard extensions? Disabling a profile is certainly odd, because I wouldn't expect profiles to be used with any CPU type other than a base type, i.e. they should be used to enable extensions on a barebones CPU model, which means setting them off would be noops. And, if a profile is set off for a cpu model where extensions are set either by the model or by previous profile and extension properties, then it also seems like an odd use, but that's at least consistent with how other properties would work, so I'm not sure we need to forbid it. > > To me it just seems really strange to have more than 1 profile. > Profiles are there to help software and users have common platforms. > Why would a user want to mix-n-match them I think it's possible users will want to describe platforms which are compatible with more than one profile, e.g. RVA22U64=on,RVA24U64=on. The example I gave Andrea about this was that C may get demoted from mandatory to optional in later profiles. If a platform is built which conforms to an older profile with C and to the later profile where C is only optional, then enabling both profiles will ensure that C is enabled, whereas only enabling the later profile will not, and then C must be added manually after inspecting the older profile to see what will be missed. OIOW, the burden of individual extension management will still be present if only single profiles may be enabled at a time. (And, even if the later profile was a strict superset of the older one, then, if a user wants to explicitly describe a platform which claims compatibility with both profiles, they probably shouldn't be punished for it, even if the resulting extension enablement would be equivalent to only enabling the later profile.) Thanks, drew
On 10/17/23 05:08, Andrew Jones wrote: > On Tue, Oct 17, 2023 at 01:55:47PM +1000, Alistair Francis wrote: >> On Mon, Oct 16, 2023 at 7:03 PM Andrew Jones <ajones@ventanamicro.com> wrote: >>> >>> On Thu, Oct 12, 2023 at 04:07:50PM -0300, Daniel Henrique Barboza 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 >>>> >>>> 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. >>> >>> left-to-right ordering is how the rest of QEMU properties work and scripts >>> depend on it. For example, one can do -cpu $MODEL,$DEFAULT_PROPS,$MORE_PROPS >>> where $MORE_PROPS can not only add more props but also override default >>> props (DEFAULT_PROPS='foo=off', MORE_PROPS='foo=on' - foo will be on). >>> left-to-right also works with multiple -cpu parameters, i.e. -cpu >>> $MODEL,$DEFAULT_PROPS -cpu $MODEL,$MY_PROPS will replace default props >>> with my props. >> >> That seems like the way to go then >> >>> >>> I don't think profiles should be treated special with regard to this. They >>> should behave the same as any property. If one does >>> profileA=off,profileB=on and there are overlapping extensions then a >> >> But what does this mean? What intent is the user saying here? >> >> For example if a user says: >> >> RVA22U64=off,RVA24U64=on >> >> They only want the extensions that were added in RVA24U64? What about >> G and the standard extensions? > > Disabling a profile is certainly odd, because I wouldn't expect profiles > to be used with any CPU type other than a base type, i.e. they should be > used to enable extensions on a barebones CPU model, which means setting > them off would be noops. And, if a profile is set off for a cpu model > where extensions are set either by the model or by previous profile and > extension properties, then it also seems like an odd use, but that's at > least consistent with how other properties would work, so I'm not sure we > need to forbid it. It's weird to add a flag that users can set to 'off' and nothing happens. That said, I'm considering profile disablement a debug/development option. I am thinking about adding a warning when users disables a profile like: "disabling a profile is recommended only for troubleshooting and is discouraged for regular use" And also mention something along those lines in the docs as well. We might be compelled into implementing profile disablement because it's weird otherwise, but we're not obligated to promote it. In fact I want to actively discourage it. Thanks, Daniel > >> >> To me it just seems really strange to have more than 1 profile. >> Profiles are there to help software and users have common platforms. >> Why would a user want to mix-n-match them > > I think it's possible users will want to describe platforms which are > compatible with more than one profile, e.g. RVA22U64=on,RVA24U64=on. > > The example I gave Andrea about this was that C may get demoted from > mandatory to optional in later profiles. If a platform is built which > conforms to an older profile with C and to the later profile where C > is only optional, then enabling both profiles will ensure that C is > enabled, whereas only enabling the later profile will not, and then > C must be added manually after inspecting the older profile to see > what will be missed. OIOW, the burden of individual extension management > will still be present if only single profiles may be enabled at a time. > (And, even if the later profile was a strict superset of the older one, > then, if a user wants to explicitly describe a platform which claims > compatibility with both profiles, they probably shouldn't be punished > for it, even if the resulting extension enablement would be equivalent > to only enabling the later profile.) > > Thanks, > drew
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
> >>
> >>
© 2016 - 2024 Red Hat, Inc.