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

From: Atish Patra <Atish.Patra@wdc.com>
To: "hch@infradead.org" <hch@infradead.org>
Cc: "anup@brainfault.org" <anup@brainfault.org>,
	"paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"palmer@sifive.com" <palmer@sifive.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"johan@kernel.org" <johan@kernel.org>
Subject: Re: [v5 PATCH] RISC-V: Fix unsupported isa string info.
Date: Fri, 6 Sep 2019 23:27:57 +0000	[thread overview]
Message-ID: <4615b682352a2e67094d327fa058ec7dd287423f.camel@wdc.com> (raw)
In-Reply-To: <67e670a4600d7dc7ec3c7a7374e330b9a1dbe654.camel@wdc.com>

On Tue, 2019-08-20 at 00:59 -0700, Atish Patra wrote:
> On Sun, 2019-08-18 at 11:16 -0700, hch@infradead.org 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.
> 

Ping ?

> Regards,
> Atish

-- 
Regards,
Atish