On 9/29/23 07:46, Daniel P. Berrangé wrote:
On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza wrote:
Based-on: 20230926183109.165878-1-dbarb...@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 ?
In theory there's no harm in allowing mass disabling of extensions but, given
it's a whole profile, we would end up disabling most/all CPU extensions and
the guest would do nothing.
There is a thread in the ML:
https://lore.kernel.org/qemu-riscv/cabjz62nyvnu4z1qmcg7myjkgg_9ywxjufhhwjmoqep6unrr...@mail.gmail.com/
Where we discussed the possibility of having a minimal CPU extension set. We
didn't
reach a consensus because the definition of "minimal CPU extension set" vary
between
OSes (Linux requires IMAFD, FreeBSD might require something differ).
Assuming we reach a consensus on what a minimal set is, we could allow
disabling mass
extensions via probile but keeping this minimal set, for example. At very least
we
shouldn't allow users to disable 'I' because that would kill the CPU, so RV64I
is
the minimum set that I would assume for now.
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.
Just read the discussion. I agree with restricting the command line flexibility
to make things more consistent, but IIRC libvirt (and others) uses a lot of
command line appending and restricting these use cases now will break stuff
up. I recall pc64 domains using <qemu:commandline> to add extra cpu/machine
flags to the domain back in the day. Surely we can claim (I guess) that command
line pass-through is unstable and users shouldn't expect extended support for
it, but we all know that users will be less than pleased when their domains
start breaking because QEMU restricted the command line.
As for the current RISC-V case, one thing we can do is to postpone feature group
processing to realize() time, since in that stage we're already done with the
command line processing. We can make a sanity check between the feature group
flags and error out if there's something off. That would make things a little
less fragile that what they are, albeit I'm sure that there will be some cases
that will be left uncovered.
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 ?
There's no technical reason to not allow it. The reason it's forbid is to be
closer to what the real hardware would do. E.g. the real hardware doesn't allow
users to enable Vector if the hardware doesn't support it. Vendor CPUs also has
a privileged spec restriction as well, so if a CPU is running in an older spec
it can't enable extensions that were added later.
If vendor CPUs from x86 and others behave in a different way with feature
enablement
I'd like to hear about it. I can say that what RISC-V is doing in this regard is
not that far from what PowerPC does.
Thanks,
Daniel
- 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-c72d77d5f...@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
