From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
alistair.francis@wdc.com, bmeng@tinylab.org,
liweiwei@iscas.ac.cn, zhiwei_liu@linux.alibaba.com,
palmer@rivosinc.com, Andrew Jones <ajones@ventanamicro.com>
Subject: Re: [PATCH 0/6] riscv: RVA22U64 profile support
Date: Fri, 29 Sep 2023 13:52:56 +0100 [thread overview]
Message-ID: <ZRbIqHuefKCBNudv@redhat.com> (raw)
In-Reply-To: <b6256c0e-5000-2af1-5ffa-caae55d9f694@ventanamicro.com>
On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 9/29/23 08:55, Daniel P. Berrangé wrote:
> > 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.
>
> This would leave us with:
>
> - $profile=off => disable all extensions. Let users hit themselves in the foot if they
> don't enable any other extensions. Note that disabling a profile and enabling extensions
> on top of it is very sensitive to left-to-right ordering, so it would be good to have
> a way to enforce this ordering somehow (feature groups always first);
It is also order sensitive if 2 profiles have overlap in the
extensions they represent. So might also require an ordering
of profiles themselves to be defined if you permit multiple
profiles.
If we dont want to think about this immediately that, then
we should make $profile=off into a fatal error rather than
silently ignoring it
> - $profile=on => only valid for generic CPUs;
>
> - $profile=check -> valid for all CPUs, would only check if the CPU implements the profile.
>
>
> I think this is fine. Drew, care to weight in?
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 :|
next prev parent reply other threads:[~2023-09-29 12:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-26 19:49 [PATCH 0/6] riscv: RVA22U64 profile support Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 1/6] target/riscv/cpu.c: add zicntr extension flag Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 2/6] target/riscv/cpu.c: add zihpm " Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 3/6] target/riscv: add rva22u64 profile definition Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 4/6] target/riscv/tcg: implement rva22u64 profile Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 5/6] target/riscv/tcg-cpu.c: enable profile support for vendor CPUs Daniel Henrique Barboza
2023-09-26 19:49 ` [PATCH 6/6] target/riscv/kvm: add 'rva22u64' flag as unavailable Daniel Henrique Barboza
2023-09-29 10:10 ` [PATCH 0/6] riscv: RVA22U64 profile support Andrea Bolognani
2023-09-29 10:46 ` Daniel P. Berrangé
2023-09-29 11:29 ` Daniel Henrique Barboza
2023-09-29 11:55 ` Daniel P. Berrangé
2023-09-29 12:49 ` Daniel Henrique Barboza
2023-09-29 12:52 ` Daniel P. Berrangé [this message]
2023-09-29 13:26 ` Daniel Henrique Barboza
2023-09-29 13:32 ` Daniel P. Berrangé
2023-10-09 2:32 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZRbIqHuefKCBNudv@redhat.com \
--to=berrange@redhat.com \
--cc=ajones@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=bmeng@tinylab.org \
--cc=dbarboza@ventanamicro.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@rivosinc.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).