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

[Alexey Kardashevskiy]

On 19/07/2019 07:28, Thiago Jung Bauermann wrote:
> Hello Segher,
>
> Thanks for your review and suggestions!
>
> Segher Boessenkool <segher@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > +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