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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f5fd0c8-0320-00b1-4ddf-b1225be352c8@wdc.com>
Date:   Fri, 8 Feb 2019 15:02:52 -0800
From:   Atish Patra <atish.patra@....com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        Patrick Stählin <me@...ki.ch>,
        Albert Ou <aou@...s.berkeley.edu>,
        Damien Le Moal <Damien.LeMoal@....com>,
        Jason Cooper <jason@...edaemon.net>,
        Alan Kao <alankao@...estech.com>,
        Dmitriy Cherkasov <dmitriy@...-tech.org>,
        Anup Patel <anup@...infault.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Zong Li <zongbox@...il.com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Andreas Schwab <schwab@...e.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [v3 PATCH 8/8] RISC-V: Assign hwcap only according to boot cpu.

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;
> 	}
> 
>