From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vocjj-0001ya-Gp for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:29:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vocja-00058g-Pv for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:29:15 -0500 Received: from e39.co.us.ibm.com ([32.97.110.160]:45637) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vocja-00058P-ID for qemu-devel@nongnu.org; Thu, 05 Dec 2013 12:29:06 -0500 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 5 Dec 2013 10:29:05 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <529FE958.2010905@ozlabs.ru> References: <1386206394-21092-1-git-send-email-mdroth@linux.vnet.ibm.com> <1386206394-21092-4-git-send-email-mdroth@linux.vnet.ibm.com> <529FE958.2010905@ozlabs.ru> Message-ID: <20131205172900.5523.40152@loki> Date: Thu, 05 Dec 2013 11:29:00 -0600 Subject: Re: [Qemu-devel] [PATCH 03/14] spapr_pci: add get-sensor-state RTAS interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: agraf@suse.de, ncmike@ncultra.org, paulus@samba.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, qemu-ppc@nongnu.org Quoting Alexey Kardashevskiy (2013-12-04 20:47:52) > On 12/05/2013 12:19 PM, Michael Roth wrote: > > From: Mike Day > > = > > Signed-off-by: Mike Day > > Signed-off-by: Michael Roth > > --- > > hw/ppc/spapr_pci.c | 72 ++++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/ppc/spapr.h | 6 +++- > > 2 files changed, 77 insertions(+), 1 deletion(-) > > = > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 64077f9..95336f7 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -498,6 +498,77 @@ static void rtas_get_power_level(PowerPCCPU *cpu, = sPAPREnvironment *spapr, > > rtas_st(rets, 1, 100); > > } > > = > > +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPREnvironment *s= papr, > > + uint32_t token, uint32_t nargs, > > + target_ulong args, uint32_t nret, > > + target_ulong rets) > > +{ > > + uint32_t sensor =3D rtas_ld(args, 0); > > + uint32_t drc_index =3D rtas_ld(args, 1); > > + uint32_t sensor_state =3D 0, decoded =3D 0, ccode =3D NO_SUCH_INDI= CATOR; > > + uint32_t shift =3D 0, mask =3D 0; > > + DrcEntry *drc_entry =3D NULL; > > + > > + if (drc_index =3D=3D 0) { /* platform state sensor/indicator */ > > + sensor_state =3D spapr->state; > > + } else { /* we should have a drc entry */ > > + drc_entry =3D spapr_find_drc_entry(drc_index); > > + if (!drc_entry) { > > + g_warning("unable to find DRC entry for index %x", drc_ind= ex); > = > = > I did not see QEMU using g_warning(), it is error_report() or custom > defined DPRINTF or fprintf(stderr,...) or a trace point from ./trace-even= ts > (traces which can be switched on/off in run-time). > = > It the case like this, I'd make it DPRINTF or trace-event, the current RT= AS > handlers do not print error messages on errors by default. Yah, DPRINTF seems reasonable, invalid guest-specified params aren't really a QEMU issue, this is more for debugging. > = > = > = > > + sensor_state =3D 0; /* empty */ > > + /* ccode is already set to -3 */ > > + rtas_st(rets, 0, ccode); > > + return; > > + } > > + sensor_state =3D drc_entry->state; > > + } > > + switch (sensor) { > > + case 9: /* EPOW */ > > + shift =3D INDICATOR_EPOW_SHIFT; > > + mask =3D INDICATOR_EPOW_MASK; > > + break; > > + case 9001: /* Isolation state */ > > + /* encode the new value into the correct bit field */ > > + shift =3D INDICATOR_ISOLATION_SHIFT; > > + mask =3D INDICATOR_ISOLATION_MASK; > > + break; > > + case 9002: /* DR */ > > + shift =3D INDICATOR_DR_SHIFT; > > + mask =3D INDICATOR_DR_MASK; > > + break; > > + case 9003: /* entity sense */ > > + shift =3D SENSOR_ENTITY_SENSE_SHIFT; > > + mask =3D SENSOR_ENTITY_SENSE_MASK; > > + break; > > + case 9005: /* global interrupt */ > > + shift =3D INDICATOR_GLOBAL_INTERRUPT_SHIFT; > > + mask =3D INDICATOR_GLOBAL_INTERRUPT_MASK; > > + break; > > + case 9006: /* error log */ > > + shift =3D INDICATOR_ERROR_LOG_SHIFT; > > + mask =3D INDICATOR_ERROR_LOG_MASK; > > + break; > > + case 9007: /* identify */ > > + shift =3D INDICATOR_IDENTIFY_SHIFT; > > + mask =3D INDICATOR_IDENTIFY_MASK; > > + break; > > + case 9009: /* reset */ > > + shift =3D INDICATOR_RESET_SHIFT; > > + mask =3D INDICATOR_RESET_MASK; > > + break; > > + default: > > + g_warning("rtas_get_sensor_state: sensor not implemented: %d", > > + sensor); > > + rtas_st(rets, 0, -3); > > + return; > > + } > > + > > + decoded =3D DECODE_DRC_STATE(sensor_state, mask, shift); > > + ccode =3D 0; > > + rtas_st(rets, 0, ccode); > > + rtas_st(rets, 1, decoded); > > +} > > + > > static int pci_spapr_swizzle(int slot, int pin) > > { > > return (slot + pin) % PCI_NUM_PINS; > > @@ -961,6 +1032,7 @@ void spapr_pci_rtas_init(void) > > spapr_rtas_register("set-indicator", rtas_set_indicator); > > spapr_rtas_register("set-power-level", rtas_set_power_level); > > spapr_rtas_register("get-power-level", rtas_get_power_level); > > + spapr_rtas_register("get-sensor-state", rtas_get_sensor_state); > > } > > = > > static void spapr_pci_register_types(void) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index d8c7de4..cadf95f 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -302,7 +302,7 @@ typedef struct sPAPREnvironment { > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1) > > #define KVMPPC_HCALL_MAX KVMPPC_H_LOGICAL_MEMOP > > = > > -/* For set-indicator RTAS interface */ > > +/* For set-indicator/get-sensor-state RTAS interfaces */ > > #define INDICATOR_ISOLATION_MASK 0x0001 /* 9001 one bit */ > > #define INDICATOR_GLOBAL_INTERRUPT_MASK 0x0002 /* 9005 one bit */ > > #define INDICATOR_ERROR_LOG_MASK 0x0004 /* 9006 one bit */ > > @@ -311,6 +311,7 @@ typedef struct sPAPREnvironment { > > #define INDICATOR_DR_MASK 0x00e0 /* 9002 three bit= s */ > > #define INDICATOR_ALLOCATION_MASK 0x0300 /* 9003 two bits = */ > > #define INDICATOR_EPOW_MASK 0x1c00 /* 9 three bits */ > > +#define SENSOR_ENTITY_SENSE_MASK 0xe000 /* 9003 three bit= s */ > = > = > This one starts with "SENSOR_" when others with "INDICATOR_". Is it for a > reason? This mostly due to how it was transcribed from PAPR documentation, we call them "sensors" WRT to get-sensor-state, and "indicators" for set-indicator-state. But since we re-use the INDICATOR_* values for get-sensor-state as well, I agree we should probably stick with the INDICATOR_ prefix. > = > = > > #define INDICATOR_ISOLATION_SHIFT 0x00 /* bit 0 */ > > #define INDICATOR_GLOBAL_INTERRUPT_SHIFT 0x01 /* bit 1 */ > > @@ -320,8 +321,11 @@ typedef struct sPAPREnvironment { > > #define INDICATOR_DR_SHIFT 0x05 /* bits 5-7 */ > > #define INDICATOR_ALLOCATION_SHIFT 0x08 /* bits 8-9 */ > > #define INDICATOR_EPOW_SHIFT 0x0a /* bits 10-12 */ > > +#define SENSOR_ENTITY_SENSE_SHIFT 0x0d /* bits 13-15 */ > > = > > #define NO_SUCH_INDICATOR -3 > > +#define DR_ENTITY_SENSE_EMPTY 0 > > +#define DR_ENTITY_SENSE_PRESENT 1 > > = > > #define DECODE_DRC_STATE(state, m, s) \ > > ((((uint32_t)(state) & (uint32_t)(m))) >> (s)) > > = > = > = > -- = > Alexey