From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35169) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Via2o-00041E-KH for qemu-devel@nongnu.org; Mon, 18 Nov 2013 20:24:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Via2h-0008C2-GE for qemu-devel@nongnu.org; Mon, 18 Nov 2013 20:23:58 -0500 Received: from mail-pa0-f48.google.com ([209.85.220.48]:56921) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Via2h-0008Br-7d for qemu-devel@nongnu.org; Mon, 18 Nov 2013 20:23:51 -0500 Received: by mail-pa0-f48.google.com with SMTP id rd3so311245pab.21 for ; Mon, 18 Nov 2013 17:23:49 -0800 (PST) Message-ID: <528ABD9F.8000604@ozlabs.ru> Date: Tue, 19 Nov 2013 12:23:43 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1384744188-11269-1-git-send-email-aik@ozlabs.ru> <0CFB910D-323F-4E29-AE6D-6721BBFF1E0D@suse.de> In-Reply-To: <0CFB910D-323F-4E29-AE6D-6721BBFF1E0D@suse.de> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] spapr: add ibm, (get|set)-system-parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc , QEMU Developers On 11/19/2013 07:58 AM, Alexander Graf wrote: > > On 17.11.2013, at 22:09, Alexey Kardashevskiy 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 >> --- >> 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