Re: [v5 PATCH] RISC-V: Fix unsupported isa string info.

[Atish Patra]

On Sun, 2019-08-18 at 11:16 -0700, hch@xxxxxxxxxxxxx wrote:
> On Fri, Aug 16, 2019 at 07:21:52PM +0000, Atish Patra wrote:
> > > > +	if (isa[0] != '\0') {
> > > > +		/* Add remainging isa strings */
> > > > +		for (e = isa; *e != '\0'; ++e) {
> > > > +#if !defined(CONFIG_VIRTUALIZATION)
> > > > +			if (e[0] != 'h')
> > > > +#endif
> > > > +				seq_write(f, e, 1);
> > > > +		}
> > > > +	}
> > > 
> > > This one I don't get.  Why do we want to check
> > > CONFIG_VIRTUALIZATION?
> > > 
> > 
> > If CONFIG_VIRTUALIZATION is not enabled, it shouldn't print that
> > hypervisor extension "h" in isa extensions.
> 
> CONFIG_VIRTUALIZATION doesn't change anything in the kernels
> capabilities, it just enables other config options. 

Yes. But it must be enabled if virtualization is supported in kernel.
The idea was to let userspace know that if virtualization can be used
or not. 


>  But more
> importantly the 'h' extension is only relevant for S-mode software
> anyway.
> 

Do you think we should just print all the extensions available in isa
string as it is ?

> > This is just an information to the userspace that some of the
> > mandatory
> > ISA extensions ("mafdcsu") are not supported in kernel which may
> > lead
> > to undesirable results.
> 
> I think we need to sit down decide what the purpose of /proc/cpuinfo
> is.  IIRC on other architectures is just prints what the hardware
> supports, not what you can actually make use of.  How else would you
> find out that you'd need to enable more kernel options to fully
> utilize the hardware?
> 
> Also printing this warning to the kernel log when someone reads the
> procfs file is very strange.

Agreed. May be something like this ?

Let's say f/d is enabled in kernel but cpu doesn't support it.
"unsupported isa" will only appear if there are any unsupported isa.

processor       : 3
hart            : 4
isa             : rv64imac
unsupported isa	: fd
mmu             : sv39
uarch           : sifive,u54-mc

May be I am just trying over optimize one corner case :) :).
/proc/cpuinfo should just print all the isa string. That's it.

Regards,
Atish