On 11/19/2013 07:58 AM, Alexander Graf wrote:
>
> On 17.11.2013, at 22:09, Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
>
>> This adds very basic handlers for ibm,get-system-parameter and
>> ibm,set-system-parameter RTAS calls.
>>
>> The only parameter handled at the moment is
>> "platform-processor-diagnostics-run-mode" which is always disabled and
>> does not support changing. This is expected to make
>> "ppc64_cpu --run-mode=1" happy.
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * addressed comments from Alex Graf
>> ---
>> hw/ppc/spapr_rtas.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index eb542f2..8053a67 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -224,6 +224,50 @@ static void rtas_stop_self(PowerPCCPU *cpu,
>> sPAPREnvironment *spapr,
>> env->msr = 0;
>> }
>>
>> +#define DIAGNOSTICS_RUN_MODE 42
>> +
>> +static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>> + sPAPREnvironment *spapr,
>> + uint32_t token, uint32_t nargs,
>> + target_ulong args,
>> + uint32_t nret, target_ulong rets)
>> +{
>> + target_ulong papameter = rtas_ld(args, 0);
>> + target_ulong buffer = rtas_ld(args, 1);
>> + target_ulong length = rtas_ld(args, 2);
>> + target_ulong ret = -3; /* System parameter is not supported */
>
> Is this an RTAS function defined return value or a global RTAS return value?
Sorry for my ignorance, I do not understand the question. So far every RTAS
call I looked at defined all return codes right in the description of the call.
Like this (sorry, cut-n-paste from PDF killed formatting but the point is
still valid):
7.3.16.2 ibm,set-system-parameter
Table 96. ibm,set-system-parameter Argument Call Buffer
Parameter Type
Name
Token
Values
Token for ibm,set-system-parameter
Number Inputs
In
2
Number Outputs 1
Parameter
Token number of the target system parameter
buffer
Out
Real address of data buffer
Status 0: Success
-1: Hardware Error
-2: Busy, Try again later
-3: System parameter not supported
-9002: Setting not allowed/authorized
-9999: Parameter Error
990x:Extended delay where x is a number 0-5 (see text below)
>> +
>> + switch (papameter) {
>> + case DIAGNOSTICS_RUN_MODE:
>> + if (length == 1) {
>> + rtas_st(buffer, 0, 0);
>> + ret = 0; /* Success */
>
> I assume this one is RTAS global?
>
>> + }
>> + break;
>> + }
>> +
>> + rtas_st(rets, 0, ret);
>> +}
>> +
>> +static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>> + sPAPREnvironment *spapr,
>> + uint32_t token, uint32_t nargs,
>> + target_ulong args,
>> + uint32_t nret, target_ulong rets)
>> +{
>> + target_ulong papameter = rtas_ld(args, 0);
>> + /* target_ulong buffer = rtas_ld(args, 1); */
>
> Not addressed. Just remove this line until it gets used.
What is bad in having such a comment? I thought by having one return per
function we help other people from the future to add functionality easier
and such comment would help them too.
>> + target_ulong ret = -3; /* System parameter is not supported */
>> +
>> + switch (papameter) {
>> + case DIAGNOSTICS_RUN_MODE:
>> + ret = -9002; /* Setting not allowed/authorized */
>
> -9002 seems to always mean "Not Authorized".
>
> In fact, reading through the PAPR spec it seems as if we can easily give
> return values reasonable names:
>
> #define RTAS_OUT_SUCCESS 0
> #define RTAS_OUT_ERROR -1
> #define RTAS_OUT_BUSY -2
> #define RTAS_OUT_NOT_SUPPORTED -3
> #define RTAS_OUT_NOMEM -5
> #define RTAS_OUT_NOT_AUTHORIZED -9002
> #define RTAS_OUT_PARAM_ERROR -9999
These are RTAS local, noone else really cares about them. The kernel uses
numbers too (arch/powerpc/kernel/rtas.c). Why should I replace easy
understandable (look at spec -> look at gdb -> everything is clear) numbers
with definitions?
> We can't always have a 1:1 map between name and number, but at least
> name -> number should always be unique:
> (ibm,configure-connector)
> #define RTAS_OUT_DR_CONN_UNUSABLE -9003
> #define RTAS_OUT_DR_CONN_NOT_SUPPORTED -9002
> #define RTAS_OUT_DR_SYSTEM_NOT_SUPPORTED -9001
>
> Do you see cases where this wouldn't work out? That way we don't have to
> have explicit comments in the code explaining what a return value really
> means, making the code more easy to read.
I need to skim the whole PAPR to make sure that this is the case and then
skim the guest kernel to make sure it is not broken somewhere. And I do not
see any profit from this job and it will definitely make my life harder as
I really (really) want to see exact numbers in the code when I debug binary
interface.
However you can always ignore my comments and say "do what I say" and I'll
do, no big deal. Thanks.
--
Alexey
