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: <20190208091133.GD16932@infradead.org>
Date:   Fri, 8 Feb 2019 01:11:33 -0800
From:   Christoph Hellwig <hch@...radead.org>
To:     Atish Patra <atish.patra@....com>
Cc:     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, 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.

> +	 * 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.

> +		/*
> +		 * 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.

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