Re: [PATCH v5] RISC-V: Show accurate per-hart isa in /proc/cpuinfo

[Evan Green]

On Sat, Aug 26, 2023 at 2:56 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Sat, Aug 26, 2023 at 12:26:25AM +0100, Conor Dooley wrote:
> > On Fri, Aug 25, 2023 at 04:11:38PM -0700, Evan Green wrote:
> > > In /proc/cpuinfo, most of the information we show for each processor is
> > > specific to that hart: marchid, mvendorid, mimpid, processor, hart,
> > > compatible, and the mmu size. But the ISA string gets filtered through a
> > > lowest common denominator mask, so that if one CPU is missing an ISA
> > > extension, no CPUs will show it.
> > >
> > > Now that we track the ISA extensions for each hart, let's report ISA
> > > extension info accurately per-hart in /proc/cpuinfo. We cannot change
> > > the "isa:" line, as usermode may be relying on that line to show only
> > > the common set of extensions supported across all harts. Add a new "hart
> > > isa" line instead, which reports the true set of extensions for that
> > > hart.
> > >
> > > Signed-off-by: Evan Green <evan@xxxxxxxxxxxx>
> > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> >
> > > Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> >
> > Can you drop this if you repost?

Will do.

> >
> > > +"isa" vs "hart isa" lines in /proc/cpuinfo
> > > +------------------------------------------
> > > +
> > > +The "isa" line in /proc/cpuinfo describes the lowest common denominator of
> > > +RISC-V ISA extensions recognized by the kernel and implemented on all harts. The
> > > +"hart isa" line, in contrast, describes the set of extensions recognized by the
> > > +kernel on the particular hart being described, even if those extensions may not
> > > +be present on all harts in the system.
> >
> > > In both cases, the presence of a feature
> > > +in these lines guarantees only that the hardware has the described capability.
> > > +Additional kernel support or policy control changes may be required before a
> > > +feature is fully usable by userspace programs.
> >
> > I do not think that "in both cases" matches the expectations of
> > userspace for the existing line. It's too late at night for me to think
> > properly, but I think our existing implementation does work like you
> > have documented for FD/V. I think I previously mentioned that it could
> > misreport things for vector during the review of the vector series but
> > forgot about it until now.
>
> I went and checked, and yes it does currently do that for vector. I
> don't think that that is what userspace would expect, that Google
> cpu_features project for example would draw incorrect conclusions.

I'm lost, could you explain a little more? My goal was to say that
there's no blanket guarantee that the feature is 100% ready to go for
userspace just because it's seen here. For some extensions, it may in
fact end up meaning just that (hence the "additional ... may be
required" rather than "is required"). This is true for FD (maybe,
depending on history?), or extensions whose minimal/zero kernel
support was unconditionally added at the same time as its parsing for
it. But it's not true solely by virtue of being in /proc/cpuinfo. In
other words, I'm trying to establish the floor of what /proc/cpuinfo
guarantees, without fully specifying the ceiling. Are you saying that
we need to spell out the guarantees for each extension? Or are you
saying the floor I've defined in general is incorrect or insufficient?
I'm also open to direct suggestions of wording if you've got something
in mind :)
-Evan