On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
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-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 ?
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.
True, that is just user error though. They could disable a profile
and then manually re-enable individual features, and thus get a
working system.
There is a thread in the ML:
https://lore.kernel.org/qemu-riscv/CABJz62NyVNu4Z1qmCG7MyJkGG_9yWxjUFHHWjmoQEP6unRrHNA@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.
I'd probably just call that user error too.
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.
Real hardware is constrained in not being able to invent arbitrary
new features on chip. Virtual machines are not constrained, so
I don't think the inability of hardware todo this, is an especially
strong reason to limit software emulation.
What I don't like about this, is that (IIUC) the '$profile=on' option
now has different semantics depending on what CPU it is used with.
ie using it with a vendor CPU, $profile=on becomes an assertion
that the vendor CPU contains all the features needed to satisfy
$profile. It won't enable/disable anything, just check it is present.
With a non-vendor CPU, using $profile=on becomes a mechanism to force
enable all the features needed to satisfy $profile, there is no
mechanism to just check for presence.
Having two different semantics for the same syntax is generally considered
bad design practice.
This points towards supporting a tri-state, not boolean. $profile=check
for validation only, and $profile=on for force enablement.