Message-ID: <20230428-versus-shady-d20735a19d41@wendy>
Date:   Fri, 28 Apr 2023 15:25:31 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Andrew Jones <ajones@...tanamicro.com>
CC:     Conor Dooley <conor@...nel.org>,
        Heiko Stübner <heiko@...ech.de>,
        <palmer@...belt.com>, <linux-riscv@...ts.infradead.org>,
        <paul.walmsley@...ive.com>, <kito.cheng@...ive.com>,
        <jrtc27@...c27.com>, <matthias.bgg@...il.com>,
        <heinrich.schuchardt@...onical.com>, <greentime.hu@...ive.com>,
        <nick.knight@...ive.com>, <christoph.muellner@...ll.eu>,
        <philipp.tomsich@...ll.eu>, <richard.henderson@...aro.org>,
        <arnd@...db.de>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] RISC-V: add support for vendor-extensions via
 AT_BASE_PLATFORM and xthead

On Fri, Apr 28, 2023 at 12:28:24PM +0200, Andrew Jones wrote:
> On Thu, Apr 27, 2023 at 07:28:49PM +0100, Conor Dooley wrote:
> > On Thu, Apr 27, 2023 at 07:15:58PM +0200, Heiko Stübner wrote:
> > > Am Mittwoch, 26. April 2023, 14:29:16 CEST schrieb Conor Dooley:
> > > > On Mon, Apr 24, 2023 at 09:49:11PM +0200, Heiko Stuebner wrote:
> > > > > From: Heiko Stuebner <heiko.stuebner@...ll.eu>
> ...
> > > > What do you mean by virtualisation here? It's the job of the hypervisor
> > > > etc to make sure that what it passes to its guest contains only what it
> > > > wants the guest to see, right?
> > > > IIUC, that's another point against doing what this patch does.
> > > 
> > > I guess I'm still seeing Zbb and friends - with just computational
> > > instructions as always good to have. But I guess you're right that the
> > > hypervisor should be able to control itself which extensions.
> > 
> > Yah, there may not be any obvious downsides to something like Zbb, but I
> > think that taking control away from the hypervisors etc isn't a good
> > idea.
> 
> If there's any chance that a VM will need to migrate from a host with,
> e.g. Zbb, to one without it, then the VM will need Zbb disabled from the
> start.

(Almost) Everything is obvious to someone :)

> > Having a simple policy of blocking things that are known to misbehave
> > would require less maint. than a list of things that are okay to pass
> > through, but both are probably cans-of-worms.
> > I think we need to think carefully about what policy is chosen here.
> > Allowlist will be slower, but at least we'll not tell userspace
> > something that is not usable. Blocklist will be easier to manage, but
> > can only be reactive.
> 
> I have experience [trying] to maintain deny-lists for CPU features,
> both for x86 Xen guests and Arm KVM guests. I don't recommend it. To
> do it right, you need to be proactive, tracking upcoming CPU features
> to add the ones that can't be supported by virt or aren't ready to
> be supported by virt to the deny-list before somebody trips over them.
> In practice, usually somebody trips over it first, causing fires which
> have to be put out. If an allow-list is used, then, when a new feature
> is missed, no fires are started. The worst that can happen is somebody
> expected the feature and didn't see it, so they complain, at which
> point you add it.

Right. Blocking-unless-known is what I suggested when canvassed for an
opinion last week but the complaint was that the kernel having to
maintain a list would be a significant speed-bump for people.
With a lighter-weight method of forwarding to userspace extensions that
the kernel doesn't need to care about (no integration with
.._has_extension[un]likely() etc) hopefully the roadblock would be a
speedbump instead.

I think I would rather speed-bumps & complaints about things being slow,
than having to fight fires.

> > Also, in a world where we do do some sort of passing, should we only
> > forward the vendor extensions, or should we forward the standard ones
> > too?
> 
> I guess we need to forward anything userspace can and should use.

That, combined with what we have now, would mean that userspace would
get told both what the kernel supports and additional other things that
the kernel may not support, but userspace can use without that support
being present.

I think that is a reasonable thing to do, although it'd muddy the waters
a bit with what the output in /proc/cpuinfo means. (I'm kinda taking the
particular bit of the series in isolation, as if /proc/cpuinfo is the
only place in which this information will be exposed.)

> > What about supervisor mode only stuff?
> 
> That's not something userspace can use. If we want to expose which
> supervisor mode features the CPU has to userspace, for information
> purposes, then I think proc or sysfs would be sufficient for that.

Yeah, as above I'm kinda looking at it from a really naive "only
/proc/cpuinfo exists" point of view for the sake of simplicity.
Depending on implementation, reporting supervisor-only stuff that the
kernel supports may make life easier & there's probably some value to
someone in passing that information to userspace too.

> The downside of using an allow-list for what extensions get exposed
> to userspace is that even extensions the kernel can't/won't use
> will need a kernel patch before userspace can use them. But, as
> I stated above, that downside (people complaining a feature they
> expect is missing), is, IMO, better than the alternative of exposing
> things that shouldn't be.

Yeah, I would be in that camp too, but gotta suggest the various options
for the sake of stirring discussion :)

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists