Michael Ellerman <m...@ellerman.id.au> writes:
> Nathan Lynch <nath...@linux.ibm.com> writes:
>> Michael Ellerman <m...@ellerman.id.au> writes:
>>> Nathan Lynch <nath...@linux.ibm.com> writes:
>>>> Michael Ellerman <m...@ellerman.id.au> writes:
>>>>> Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm....@kernel.org>
>>>>> writes:
>>>>>> From: Nathan Lynch <nath...@linux.ibm.com>
>>>>>>
>>>>>> On RTAS platforms there is a general restriction that the OS must not
>>>>>> enter RTAS on more than one CPU at a time. This low-level
>>>>>> serialization requirement is satisfied by holding a spin
>>>>>> lock (rtas_lock) across most RTAS function invocations.
>>>>> ...
>>>>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>>>>> index 1fc0b3fffdd1..52f2242d0c28 100644
>>>>>> --- a/arch/powerpc/kernel/rtas.c
>>>>>> +++ b/arch/powerpc/kernel/rtas.c
>>>>>> @@ -581,6 +652,28 @@ static const struct rtas_function
>>>>>> *rtas_token_to_function(s32 token)
>>>>>> return NULL;
>>>>>> }
>>>>>>
>>>>>> +static void __rtas_function_lock(struct rtas_function *func)
>>>>>> +{
>>>>>> + if (func && func->lock)
>>>>>> + mutex_lock(func->lock);
>>>>>> +}
>>>>>
>>>>> This is obviously going to defeat most static analysis tools.
>>>>
>>>> I guess it's not that obvious to me :-) Is it because the mutex_lock()
>>>> is conditional? I'll improve this if it's possible.
>>>
>>> Well maybe I'm not giving modern static analysis tools enough credit :)
>>>
>>> But what I mean that it's not easy to reason about what the function
>>> does in isolation. ie. all you can say is that it may or may not lock a
>>> mutex, and you can't say which mutex.
>>
>> I've pulled the thread on this a little bit and here is what I can do:
>>
>> * Discard rtas_lock_function() and rtas_unlock_function() and make the
>> function mutexes extern as needed. As of now only
>> rtas_ibm_get_vpd_lock will need to be exposed. This enables us to put
>> __acquires(), __releases(), and __must_hold() annotations in
>> papr-vpd.c since it explicitly manipulates the mutex.
>>
>> * Then sys_rtas() becomes the only site that needs
>> __rtas_function_lock() and __rtas_function_unlock(), which can be
>> open-coded and commented (and, one hopes, not emulated elsewhere).
>>
>> This will look something like:
>>
>> SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>> {
>> struct rtas_function *func = rtas_token_to_function(token);
>>
>> if (func->lock)
>> mutex_lock(func->lock);
>>
>> [ ... acquire rtas_lock, enter RTAS, fetch any errors ... ]
>>
>> if (func->lock)
>> mutex_unlock(func->lock);
>>
>> The indirection seems unavoidable since we're working backwards from a
>> token value (supplied by the user and not known at build time) to the
>> function descriptor.
>>
>> Is that tolerable for now?
>
> Yeah. Thanks for looking into it.
>
> I wasn't unhappy with the original version, but just slightly uneasy
> about the locking via pointer.
>
> But that new proposal sounds good, more code will have static lock
> annotations, and only sys_rtas() which is already weird, will have the
> dynamic stuff.
OK, I'll work that up then.
