Re: [PATCH 0/6] riscv: RVA22U64 profile support

[Daniel P . Berrangé]

On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
> Based-on: 20230926183109.165878-1-dbarboza@ventanamicro.com
> ("[PATCH 0/2] riscv: add extension properties for all cpus")
> 
> Hi,
> 
> These patches implements the base profile support for qemu-riscv and the
> first profile, RVA22U64. 
> 
> As discussed in this thread [1] we're aiming for a flag that enables all
> mandatory extensions of a profile. Optional extensions were left behind
> and must be enabled by hand if desired. Since this is the first profile
> we're adding, we'll need to add the base framework as well. 
> 
> The RVA22U64 profile was chosen because qemu-riscv implements all its
> extensions, both mandatory and optional. That includes 'zicntr' and
> 'zihpm', which we support for awhile but aren't adverting to userspace.
> 
> Other design decisions made:
> 
> - disabling a profile flag does nothing, i.e. we won't mass disable
>   mandatory extensions of the rva22U64 profile if the user sets
>   rva22u64=false;

Why shouldn't this be allowed ?

IIUC, a profile is syntactic sugar for a group of features. If
we can disable individual features explicitly, why should we
not allow use of the profile as sugar to disable them en-mass ?


BTW, I would caution that the semantics of mixing groups of
features, with individual features in -cpu is likely to be
ill defined, as you cannot rely on left-to-right processing
of the -cpu arguments.

IOW, if you say  -cpu $foo,$group=on,$feature=off

you might expect this to result in '$feature' being disabled
if it were implied by $group. This is not guaranteed as the
QDict processing of options could result in us effectively
processing   -cpu $foo,$feature=off,$group=on

This brokeness with CPU feature groups and their interaction
with CPU feature flags already impacts s390x:

  https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg00981.html

There is a possible way to fix it by declaring an ordering
such that all groups will be processed fully, before any
individual features are processed, and declaring that group
or feature names must not be repeated. This avoids a reliance
on left-to-right ordering:

  https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01005.html

that is still likely broken, however, if multiple different
groups are listed, and there is overlap in their feature
sets.

TL;DR: feature groups are pretty error prone if more than
one is listed by the user, or they're combined with individual
features.

> 
> - profile support for vendor CPUs consists into checking if the CPU
>   happens to have the mandatory extensions required for it. In case it
>   doesn't we'll error out. This is done to follow the same prerogative
>   we always had of not allowing extensions being enabled for vendor
>   CPUs;

Why shouldn't this be allowed ?

> - the KVM driver doesn't support profiles. In theory we could apply the
>   same logic as for the vendor CPUs, but KVM has a long way to go before
>   that becomes a factor. We'll revisit this decision when KVM is able to
>   support at least one profile.
> 
> Patch 5 ("enable profile support for vendor CPUs") needs the following
> series to be applied beforehand:
> 
> "[PATCH 0/2] riscv: add extension properties for all cpus"
> 
> Otherwise we won't be able to add the profile flag to vendor CPUs.
> 
> [1] 
> https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f43b@ventanamicro.com/
> 
> Daniel Henrique Barboza (6):
>   target/riscv/cpu.c: add zicntr extension flag
>   target/riscv/cpu.c: add zihpm extension flag
>   target/riscv: add rva22u64 profile definition
>   target/riscv/tcg: implement rva22u64 profile
>   target/riscv/tcg-cpu.c: enable profile support for vendor CPUs
>   target/riscv/kvm: add 'rva22u64' flag as unavailable
> 
>  target/riscv/cpu.c         | 25 ++++++++++
>  target/riscv/cpu.h         | 10 ++++
>  target/riscv/cpu_cfg.h     |  2 +
>  target/riscv/kvm/kvm-cpu.c |  5 +-
>  target/riscv/tcg/tcg-cpu.c | 98 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 139 insertions(+), 1 deletion(-)
> 
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|