Re: [v3 PATCH 8/8] RISC-V: Assign hwcap only according to boot cpu.

[Atish Patra]

On 2/8/19 1:11 AM, Christoph Hellwig wrote:
> > +	 * We don't support running Linux on hertergenous ISA systems.
> > +	 * But first "okay" processor might not be the boot cpu.
> > +	 * Check the ISA of boot cpu.
>
> Please use up your available 80 characters per line in comments.
I will fix it.

> > +		/*
> > +		 * All "okay" hart should have same isa. We don't know how to
> > +		 * handle if they don't. Throw a warning for now.
> > +		 */
> > +		if (elf_hwcap && temp_hwcap != elf_hwcap)
> > +			pr_warn("isa mismatch: 0x%lx != 0x%lx\n",
> > +				elf_hwcap, temp_hwcap);
> > +
> > +		if (hartid == boot_cpu_hartid)
> > +			boot_hwcap = temp_hwcap;
> > +		elf_hwcap = temp_hwcap;
>
> So we always set elf_hwcap to the capabilities of the previous cpu.
>
> > +		temp_hwcap = 0;
>
> I think tmp_hwcap should be declared and initialized inside the outer loop
> instead having to manually reset it like this.
>
> > +	}
> >   
> > +	elf_hwcap = boot_hwcap;
>
> And then reset it here to the boot cpu.
>
> Shoudn't we only report the features supported by all cores?  Otherwise
> we'll still have problems if the boot cpu supports a feature, but not
> others.

Hmm. The other side of the argument is boot cpu does have a feature that 
is not supported by other hart that didn't even boot.
The user space may execute something based on boot cpu capability but 
that won't be enabled.

At least, in this way we know that we are compatible completely with 
boot cpu capabilities. Thoughts ?

Regards,
Atish
> Something like:
>
> 	for () {
> 		unsigned long this_hwcap = 0;
>
> 		for (i = 0; i < strlen(isa); i++)
> 			this_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
>
> 		if (elf_hwcap)
> 			elf_hwcap &= this_hwcap;
> 		else
> 			elf_hwcap = this_hwcap;
> 	}