From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52123) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZ5au-0004f8-Oz for qemu-devel@nongnu.org; Tue, 30 Sep 2014 18:08:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZ5am-0003sn-Ve for qemu-devel@nongnu.org; Tue, 30 Sep 2014 18:08:28 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:37013) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZ5am-0003sB-PF for qemu-devel@nongnu.org; Tue, 30 Sep 2014 18:08:20 -0400 Received: from /spool/local by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 30 Sep 2014 16:08:19 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <53FC7159.6040406@suse.de> References: <1408407718-10835-1-git-send-email-mdroth@linux.vnet.ibm.com> <1408407718-10835-5-git-send-email-mdroth@linux.vnet.ibm.com> <53FC7159.6040406@suse.de> Message-ID: <20140930220813.19243.5956@loki> Date: Tue, 30 Sep 2014 17:08:13 -0500 Subject: Re: [Qemu-devel] [PATCH 04/12] spapr_pci: add set-indicator RTAS interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , qemu-devel@nongnu.org Cc: aik@ozlabs.ru, ncmike@ncultra.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com Quoting Alexander Graf (2014-08-26 06:36:57) > On 19.08.14 02:21, Michael Roth wrote: > > From: Mike Day > > = > > Signed-off-by: Mike Day > > Signed-off-by: Michael Roth > > --- > > hw/ppc/spapr_pci.c | 119 +++++++++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/ppc/spapr.h | 3 ++ > > 2 files changed, 122 insertions(+) > > = > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 924d488..23a3477 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -36,6 +36,16 @@ > > = > > #include "hw/pci/pci_bus.h" > > = > > +/* #define DEBUG_SPAPR */ > > + > > +#ifdef DEBUG_SPAPR > > +#define DPRINTF(fmt, ...) \ > > + do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0) > > +#else > > +#define DPRINTF(fmt, ...) \ > > + do { } while (0) > > +#endif > > + > > /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */ > > #define RTAS_QUERY_FN 0 > > #define RTAS_CHANGE_FN 1 > > @@ -47,6 +57,31 @@ > > #define RTAS_TYPE_MSI 1 > > #define RTAS_TYPE_MSIX 2 > > = > > +/* For set-indicator RTAS interface */ > > +#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 */ > > +#define INDICATOR_IDENTIFY_MASK 0x0008 /* 9007 one bit */ > > +#define INDICATOR_RESET_MASK 0x0010 /* 9009 one bit */ > > +#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 INDICATOR_ISOLATION_SHIFT 0x00 /* bit 0 */ > > +#define INDICATOR_GLOBAL_INTERRUPT_SHIFT 0x01 /* bit 1 */ > > +#define INDICATOR_ERROR_LOG_SHIFT 0x02 /* bit 2 */ > > +#define INDICATOR_IDENTIFY_SHIFT 0x03 /* bit 3 */ > > +#define INDICATOR_RESET_SHIFT 0x04 /* bit 4 */ > > +#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 DECODE_DRC_STATE(state, m, s) \ > > + ((((uint32_t)(state) & (uint32_t)(m))) >> (s)) > > + > > +#define ENCODE_DRC_STATE(val, m, s) \ > > + (((uint32_t)(val) << (s)) & (uint32_t)(m)) > > + > > static sPAPRPHBState *find_phb(sPAPREnvironment *spapr, uint64_t buid) > > { > > sPAPRPHBState *sphb; > > @@ -402,6 +437,80 @@ static void rtas_ibm_query_interrupt_source_number= (PowerPCCPU *cpu, > > rtas_st(rets, 2, 1);/* 0 =3D=3D level; 1 =3D=3D edge */ > > } > > = > > +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPREnvironment *spap= r, > > + uint32_t token, uint32_t nargs, > > + target_ulong args, uint32_t nret, > > + target_ulong rets) > > +{ > > + uint32_t indicator =3D rtas_ld(args, 0); > > + uint32_t drc_index =3D rtas_ld(args, 1); > > + uint32_t indicator_state =3D rtas_ld(args, 2); > > + uint32_t encoded =3D 0, shift =3D 0, mask =3D 0; > > + uint32_t *pind; > > + sPAPRDrcEntry *drc_entry =3D NULL; > = > This rtas call does not have any idea what a PHB is. Why does it live in > spapr_pci.c? spapr_rtas.c does seem like a better home > = > > + > > + if (drc_index =3D=3D 0) { /* platform indicator */ > > + pind =3D &spapr->state; > > + } else { > > + drc_entry =3D spapr_find_drc_entry(drc_index); > > + if (!drc_entry) { > > + DPRINTF("rtas_set_indicator: unable to find drc_entry for = %x", > > + drc_index); > > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > > + return; > > + } > > + pind =3D &drc_entry->state; > > + } > > + > > + switch (indicator) { > > + 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: /* Allocation State */ > > + shift =3D INDICATOR_ALLOCATION_SHIFT; > > + mask =3D INDICATOR_ALLOCATION_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: > > + DPRINTF("rtas_set_indicator: indicator not implemented: %d", > > + indicator); > > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > > + return; > > + } > > + > > + encoded =3D ENCODE_DRC_STATE(indicator_state, mask, shift); > > + /* clear the current indicator value */ > > + *pind &=3D ~mask; > > + /* set the new value */ > > + *pind |=3D encoded; > > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > +} > > + > > static int pci_spapr_swizzle(int slot, int pin) > > { > > return (slot + pin) % PCI_NUM_PINS; > > @@ -624,6 +733,14 @@ static void spapr_phb_realize(DeviceState *dev, Er= ror **errp) > > sphb->lsi_table[i].irq =3D irq; > > } > > = > > + /* make sure the platform EPOW sensor is initialized - the > > + * guest will probe it when there is a hotplug event. > > + */ > > + spapr->state &=3D ~(uint32_t)INDICATOR_EPOW_MASK; > > + spapr->state |=3D ENCODE_DRC_STATE(0, > > + INDICATOR_EPOW_MASK, > > + INDICATOR_EPOW_SHIFT); > > + > > if (!info->finish_realize) { > > error_setg(errp, "finish_realize not defined"); > > return; > > @@ -1056,6 +1173,8 @@ void spapr_pci_rtas_init(void) > > spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi", > > rtas_ibm_change_msi); > > } > > + spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator", > > + rtas_set_indicator); > > } > > = > > static void spapr_pci_register_types(void) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 0ac1a19..fac85f8 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -72,6 +72,9 @@ typedef struct sPAPREnvironment { > > = > > /* state for Dynamic Reconfiguration Connectors */ > > sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE]; > > + > > + /* Platform state - sensors and indicators */ > > + uint32_t state; > = > Do you think it'd be possible to create a special DRC device that > contains all of its tables and global state and also exposes sensors and > indicators? That device could then get linked via qom links to the PHBs > for their slots. Sorry for the delay, I've been going back through the code with this suggestion in mind and there does seem to be a lot of state that can be nicely encapsulated by modeling the DR Connectors as a QOM "device" (though I haven't gone as far as to make them actual DeviceState's since it's more of a firmware abstraction than real hardware) I'm not sure what the best way to plumb things together is, as a first run, since each DRC must have a index drc_index as per spec, I've moved put them under /machine/DRConnector as a flat list, where top-level PHB/CPU/MEMORY DRCs would be allocated statically during sPAPR machine init (since the corresponding DRC indexes/types/etc are hard-coded into the top-level of the boot-time DT anyway, though I guess we could also allocate these on the fly...seems messier though than just plugging new resources into existing DRCs) PHB's in turn will associate themselves with a DRC via an attach/detach method as part of realize (and in the future, hotplug hooks, though that's not part of the series). The PHBs in turn will create a DRC for each hotpluggable PCI slot. Creation is via: sPAPRDRConnector *spapr_dr_connector_new(sPAPRDRConnectorType type, uint32_t id); where the code computes the drc index based on (one of phb, cpu, pci, memory, etc) and , and sticks them under /machine/dr-Connector/ Any pci/phb/cpu hotplug hooks can then fetch the DRC via type/id, and hotplug/unplug via attach()/detach() methods. attach() adds the attached/hotplugged DeviceState as a link property of the DRC object, and sets the initial sensor state. rtas calls can fetch DRCs via drc_index, and set/get sensor state via DRC sensor get/set methods. Hotplug event delivery still lives outside of DRC implementation for now. I thought of moving them into DRC, but decisions like whether we should emit events during coldplug/initial boot seemed to require pushing a lot of general machine state into DRCs and making the encapsulation seem superficial. Things end up looking like this (2xxxxxxx are PHBs, 4xxxxxxx are PCI slots): mdroth@loki:~/w/qom/machine/dr-connector$ ls 20000000 40000018 40000038 40000058 40000078 40000098 400000b8 40000= 0d8 400000f8 40000000 40000020 40000040 40000060 40000080 400000a0 400000c0 40000= 0e0 type 40000008 40000028 40000048 40000068 40000088 400000a8 400000c8 40000= 0e8 40000010 40000030 40000050 40000070 40000090 400000b0 400000d0 40000= 0f0 mdroth@loki:~/w/qom/machine/dr-connector$ cd 40000000/ mdroth@loki:~/w/qom/machine/dr-connector/40000000$ ls -l total 0 -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 allocation-state lrwxr-xr-x 2 mdroth mdroth 4096 Dec 31 1969 device -> ../../../machine/per= ipheral/hp0 -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 drc-index -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 entity-sense -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 indicator-state -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 isolation-state -rw-r--r-- 1 mdroth mdroth 4096 Dec 31 1969 type mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat allocation-state = 1 mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat indicator-state = 1 mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat ../../../machine/per= ipheral/hp0/type = virtio-net-pci mdroth@loki:~/w/qom/machine/dr-connector/40000000$ Hopefully this is sort of the approach you were thinking of? > = > = > Alex