more /proc/cpuinfo & extension support related woes

[Conor Dooley]

On Wed, Jul 26, 2023 at 10:07:52PM +0530, Anup Patel wrote:
> On Wed, Jul 26, 2023 at 4:11 PM Conor Dooley <conor.dooley at microchip.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 11:04:39AM +0200, Andrew Jones wrote:
> > > On Tue, Jul 25, 2023 at 06:19:36PM +0100, Conor Dooley wrote:

> > >  - The kernel will combine information from one of the two bitmaps listed
> > >    above with other information, such as config information, to decide
> > >    when / if an extension should be used by the kernel and/or exposed to
> > >    userspace
> >
> > Right. This bit (or rather two bits, since I view the kernel and
> > userspace bits here separately) is where we are falling short. I think
> > hwprobe's limited users do the right thing, but we're not doing this for
> > /proc/cpuinfo. For the in-kernel users, grepping shows some suspicious
> > looking things, but how many are problematic I do not yet know. As an
> > example, does
> > static void kvm_riscv_vcpu_update_config(const unsigned long *isa)
> > {
> >         u64 henvcfg = 0;
> >
> >         if (riscv_isa_extension_available(isa, SVPBMT))
> >                 henvcfg |= ENVCFG_PBMTE;
> >
> > work correctly if the SVPBMT Kconfig option is disabled?
> > From a quick check, `isa` here is set from the host ISA
> >         /* Setup ISA features available to VCPU */
> >         for (i = 0; i < ARRAY_SIZE(kvm_isa_ext_arr); i++) {
> >                 host_isa = kvm_isa_ext_arr[i];
> >                 if (__riscv_isa_extension_available(NULL, host_isa) &&
> >                     kvm_riscv_vcpu_isa_enable_allowed(i))
> >                         set_bit(host_isa, vcpu->arch.isa);
> >         }
> > so if the check passes for the host ISA, it'll pass for the guest ISA
> > too, so henvcfg will end up with the PBMTE bit set. I just haven't yet
> > checked what the outcome of this will be, but I figure not good?
> 
> Why is the outcome not good?
> 
> Guest need Svpbmt to support pass-through devices.

It was an example of a case where something is using
__riscv_isa_extension_available() where the behaviour of the kernel
w.r.t. this extension changes based on a Kconfig option being set.
I get why a guest wants to know about Svpbmt, that's not why I mentioned
this bit of code.

> > > (it'd be good to have a consistent API for these types of
> > >    checks which combine extension presence with other information)
> >
> > I figure the best option might just be to make the
> > __riscv_isa_extension_available() into this to avoid disruption? The
> > vast majority of users of that function want to know whether or not it
> > is safe to use the extension on that hart, not whether the hart itself
> > supports it. In fact, outside of Evan's per-hart stuff in /proc/cpuinfo,
> > do we have any users that would even want an API that checks the
> > "unfiltered" versions? I'll have to check the users to answer that I
> > think.
> 
> Each guest VCPU will have its own ISA bitmap so KVM needs the
> __riscv_isa_extension_available() to check VCPU ISA bitmap.

That's not the unfiltered list though, you've assembled this specifically
for your VCPUs based on what KVM knows it can offer. It's the harts in
the host kernel that I am mainly talking about here and, in particular,
the behaviour when NULL is passed as the first argument. Changes can be
made to how __riscv_isa_extension_available() in that regard without
impacting KVM, which is mostly what I meant by "avoid disruption".
Either way, AFAICT, KVM doesn't offer vector support when
CONFIG_RISCV_ISA_V is disabled or fpu support without CONFIG_FPU (AFAICT).
Other than Svpbmt, those are all that the filtered list would remove from
the host isa bitmap anyway.

Hope that clarifies things,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230726/e03b4490/attachment.sig>