linux-kernel - Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom

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]

Date:   Thu, 18 Jul 2019 18:28:36 -0300
From:   Thiago Jung Bauermann <bauerman@...ux.ibm.com>
To:     Segher Boessenkool <segher@...nel.crashing.org>
Cc:     Alexey Kardashevskiy <aik@...abs.ru>,
        linuxppc-dev@...ts.ozlabs.org,
        Anshuman Khandual <anshuman.linux@...il.com>,
        Mike Anderson <andmike@...ux.ibm.com>,
        Ram Pai <linuxram@...ibm.com>, linux-kernel@...r.kernel.org,
        Claudio Carvalho <cclaudio@...ux.ibm.com>,
        Paul Mackerras <paulus@...ba.org>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init


Hello Segher,

Thanks for your review and suggestions!

Segher Boessenkool <segher@...nel.crashing.org> writes:

> (Sorry to hijack your reply).
>
> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote:
>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
>> >From: Ram Pai <linuxram@...ibm.com>
>> >+static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
>> >+{
>> >+	register uint64_t func asm("r3") = UV_ESM;
>> >+	register uint64_t arg1 asm("r4") = (uint64_t)kbase;
>> >+	register uint64_t arg2 asm("r5") = (uint64_t)fdt;
>> 
>> What does UV do with kbase and fdt precisely? Few words in the commit 
>> log will do.
>> 
>> >+
>> >+	asm volatile("sc 2\n"
>> >+		     : "=r"(func)
>> >+		     : "0"(func), "r"(arg1), "r"(arg2)
>> >+		     :);
>> >+
>> >+	return (int)func;
>> 
>> And why "func"? Is it "function"? Weird name. Thanks,

Yes, I believe func is for function. Perhaps ucall would be clearer
if the variable wasn't reused for the return value as Segher points out.

> Maybe the three vars should just be called "r3", "r4", and "r5" --
> r3 is used as return value as well, so "func" isn't a great name for it.

Yes, that does seem simpler.

> Some other comments about this inline asm:
>
> The "\n" makes the generated asm look funny and has no other function.
> Instead of using backreferences you can use a "+" constraint, "inout".
> Empty clobber list is strange.
> Casts to the return type, like most other casts, are an invitation to
> bugs and not actually useful.
>
> So this can be written
>
> static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
> {
> 	register uint64_t r3 asm("r3") = UV_ESM;
> 	register uint64_t r4 asm("r4") = kbase;
> 	register uint64_t r4 asm("r5") = fdt;
>
> 	asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5));
>
> 	return r3;
> }

I'll adopt your version, it is cleaner inded. Thanks for providing it!

> (and it probably should use u64 instead of both uint64_t and unsigned long?)

Almost all of prom_init.c uses unsigned long, with u64 in just a few
places. uint64_t isn't used anywhere else in the file. I'll switch to
unsigned long everywhere, since this feature is only for 64 bit.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center