Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
To:     Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc:     Segher Boessenkool <segher@kernel.crashing.org>,
        linuxppc-dev@lists.ozlabs.org,
        Anshuman Khandual <anshuman.linux@gmail.com>,
        Mike Anderson <andmike@linux.ibm.com>,
        Ram Pai <linuxram@us.ibm.com>, linux-kernel@vger.kernel.org,
        Claudio Carvalho <cclaudio@linux.ibm.com>,
        Paul Mackerras <paulus@samba.org>,
        Christoph Hellwig <hch@lst.de>
References: <20190713060023.8479-1-bauerman@linux.ibm.com>
 <20190713060023.8479-4-bauerman@linux.ibm.com>
 <70f8097f-7222-fe18-78b4-9372c21bfc9d@ozlabs.ru>
 <20190718195850.GU20882@gate.crashing.org>
 <875znz3ud7.fsf@morokweng.localdomain>
From:   Alexey Kardashevskiy <aik@ozlabs.ru>
Message-ID: <f4cba627-8c40-ce95-0ede-b01edf3546dc@ozlabs.ru>
Date:   Fri, 19 Jul 2019 10:09:56 +1000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <875znz3ud7.fsf@morokweng.localdomain>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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