Hello Segher,
Thanks for your review and suggestions! Segher Boessenkool <seg...@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 <linux...@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