[PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
Thiago Jung Bauermann
bauerman at linux.ibm.com
Fri Jul 19 07:28:36 AEST 2019
Thanks for your review and suggestions!
Segher Boessenkool <segher at kernel.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 at us.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
More information about the Linuxppc-dev