Messages in this thread | | | Subject | Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init | From | Alexey Kardashevskiy <> | Date | Fri, 19 Jul 2019 10:09:56 +1000 |
| |
On 19/07/2019 07:28, Thiago Jung Bauermann wrote: > > Hello Segher, > > Thanks for your review and suggestions! > > Segher Boessenkool <segher@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@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.
What about this one? :)
>>> >>>> + >>>> + 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. >
-- Alexey
| |