From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMIBK-0001pz-Vh for qemu-devel@nongnu.org; Tue, 26 Aug 2014 10:57:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMIBA-0000S5-Bg for qemu-devel@nongnu.org; Tue, 26 Aug 2014 10:57:10 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:37408) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMIBA-0000Qt-4K for qemu-devel@nongnu.org; Tue, 26 Aug 2014 10:57:00 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 26 Aug 2014 08:56:57 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <53FC3D59.1020702@ozlabs.ru> References: <1408407718-10835-1-git-send-email-mdroth@linux.vnet.ibm.com> <1408407718-10835-2-git-send-email-mdroth@linux.vnet.ibm.com> <53FC3D59.1020702@ozlabs.ru> Message-ID: <20140826145651.21832.79008@loki> Date: Tue, 26 Aug 2014 09:56:51 -0500 Subject: Re: [Qemu-devel] [PATCH 01/12] spapr: populate DRC entries for root dt node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: ncmike@ncultra.org, nfont@linux.vnet.ibm.com, qemu-ppc@nongnu.org, agraf@suse.de, tyreld@linux.vnet.ibm.com Quoting Alexey Kardashevskiy (2014-08-26 02:55:05) > On 08/19/2014 10:21 AM, Michael Roth wrote: > > From: Nathan Fontenot > > = > > This add entries to the root OF node to advertise our PHBs as being > > DR-capable in according with PAPR specification. > > = > > Each PHB is given a name of PHB, advertised as a PHB type, > > and associated with a power domain of -1 (indicating to guests that > > power management is handled automatically by hardware). > > = > > We currently allocate entries for up to 32 DR-capable PHBs, though > > this limit can be increased later. > > = > > DrcEntry objects to track the state of the DR-connector associated > > with each PHB are stored in a 32-entry array, and each DrcEntry has > > in turn have a dynamically-sized number of child DR-connectors, > > which we will use later to track the state of DR-connectors > > associated with a PHB's physical slots. > > = > > Signed-off-by: Nathan Fontenot > > Signed-off-by: Michael Roth > > --- > > hw/ppc/spapr.c | 143 +++++++++++++++++++++++++++++++++++++++++= ++++++++ > > hw/ppc/spapr_pci.c | 1 + > > include/hw/ppc/spapr.h | 35 ++++++++++++ > > 3 files changed, 179 insertions(+) > > = > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 5c92707..d5e46c3 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -296,6 +296,143 @@ static hwaddr spapr_node0_size(void) > > return ram_size; > > } > > = > > +sPAPRDrcEntry *spapr_phb_to_drc_entry(uint64_t buid) > > +{ > > + int i; > > + > > + for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { > > + if (spapr->drc_table[i].phb_buid =3D=3D buid) { > > + return &spapr->drc_table[i]; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static void spapr_init_drc_table(void) > > +{ > > + int i; > > + > > + memset(spapr->drc_table, 0, sizeof(spapr->drc_table)); > > + > > + /* For now we only care about PHB entries */ > > + for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { > > + spapr->drc_table[i].drc_index =3D 0x2000001 + i; > > + } > > +} > > + > > +sPAPRDrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t stat= e) > > +{ > > + sPAPRDrcEntry *empty_drc =3D NULL; > > + sPAPRDrcEntry *found_drc =3D NULL; > > + int i, phb_index; > > + > > + for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { > > + if (spapr->drc_table[i].phb_buid =3D=3D 0) { > > + empty_drc =3D &spapr->drc_table[i]; > > + } > > + > > + if (spapr->drc_table[i].phb_buid =3D=3D buid) { > > + found_drc =3D &spapr->drc_table[i]; > = > return &spapr->drc_table[i]; > ? That makes sense. Looking at this again though I think maybe I should drop the found_drc stuff completely, or maybe even assert if we attempt to re-add a phb. Current callers already do spapr_phb_to_drc_entry beforehand anyway, which should cover the case where it's already there. So maybe something like this? sPAPRDrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t state) { sPAPRDrcEntry *empty_drc =3D NULL; int i, phb_index; for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { g_assert(spapr->drc_table[i].phb_buid !=3D buid); if (spapr->drc_table[i].phb_buid =3D=3D 0) { empty_drc =3D &spapr->drc_table[i]; break; } } if (!empty_drc) { return NULL; } empty_drc->phb_buid =3D buid; empty_drc->state =3D state; empty_drc->cc_state.fdt =3D NULL; empty_drc->cc_state.offset =3D 0; empty_drc->cc_state.depth =3D 0; empty_drc->cc_state.state =3D CC_STATE_IDLE; empty_drc->child_entries =3D g_malloc0(sizeof(sPAPRDrcEntry) * SPAPR_DRC_PHB_SLOT_MAX); phb_index =3D buid - SPAPR_PCI_BASE_BUID; for (i =3D 0; i < SPAPR_DRC_PHB_SLOT_MAX; i++) { empty_drc->child_entries[i].drc_index =3D SPAPR_DRC_DEV_ID_BASE + (phb_index << 8) + (i << 3); } return empty_drc; } > = > = > > + break; > > + } > > + } > > + > > + if (found_drc) { > > + return found_drc; > > + } > = > if (!empty_drc) { > return NULL; > } > = > ? > = > = > > + > > + if (empty_drc) { > = > and no need in this :) > = > = > > + empty_drc->phb_buid =3D buid; > > + empty_drc->state =3D state; > > + empty_drc->cc_state.fdt =3D NULL; > > + empty_drc->cc_state.offset =3D 0; > > + empty_drc->cc_state.depth =3D 0; > > + empty_drc->cc_state.state =3D CC_STATE_IDLE; > > + empty_drc->child_entries =3D > > + g_malloc0(sizeof(sPAPRDrcEntry) * SPAPR_DRC_PHB_SLOT_MAX); > > + phb_index =3D buid - SPAPR_PCI_BASE_BUID; > > + for (i =3D 0; i < SPAPR_DRC_PHB_SLOT_MAX; i++) { > > + empty_drc->child_entries[i].drc_index =3D > > + SPAPR_DRC_DEV_ID_BASE + (phb_index << 8) + (i << 3); > > + } > > + return empty_drc; > > + } > > + > > + return NULL; > > +} > > + > > +static void spapr_create_drc_dt_entries(void *fdt) > > +{ > > + char char_buf[1024]; > > + uint32_t int_buf[SPAPR_DRC_TABLE_SIZE + 1]; > > + uint32_t *entries; > > + int offset, fdt_offset; > > + int i, ret; > > + > > + fdt_offset =3D fdt_path_offset(fdt, "/"); > > + > > + /* ibm,drc-indexes */ > > + memset(int_buf, 0, sizeof(int_buf)); > > + int_buf[0] =3D SPAPR_DRC_TABLE_SIZE; > > + > > + for (i =3D 1; i <=3D SPAPR_DRC_TABLE_SIZE; i++) { > > + int_buf[i] =3D spapr->drc_table[i-1].drc_index; > > + } > > + > > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-indexes", int_buf, > > + sizeof(int_buf)); > > + if (ret) { > > + fprintf(stderr, "Couldn't finalize ibm,drc-indexes property\n"= ); > = > return here and below in the same error cases? Yah, that's clearly more sensible. I suppose if we introduce an error exit = case this should stop being a void function as well. > = > > + } > > + > > + /* ibm,drc-power-domains */ > > + memset(int_buf, 0, sizeof(int_buf)); > > + int_buf[0] =3D SPAPR_DRC_TABLE_SIZE; > > + > > + for (i =3D 1; i <=3D SPAPR_DRC_TABLE_SIZE; i++) { > > + int_buf[i] =3D 0xffffffff; > > + } > > + > > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-power-domains", int_= buf, > > + sizeof(int_buf)); > > + if (ret) { > > + fprintf(stderr, "Couldn't finalize ibm,drc-power-domains prope= rty\n"); > > + } > > + > > + /* ibm,drc-names */ > > + memset(char_buf, 0, sizeof(char_buf)); > > + entries =3D (uint32_t *)&char_buf[0]; > > + *entries =3D SPAPR_DRC_TABLE_SIZE; > > + offset =3D sizeof(*entries); > > + > > + for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { > > + offset +=3D sprintf(char_buf + offset, "PHB %d", i + 1); > > + char_buf[offset++] =3D '\0'; > > + } > > + > > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-names", char_buf, of= fset); > > + if (ret) { > > + fprintf(stderr, "Couldn't finalize ibm,drc-names property\n"); > > + } > > + > > + /* ibm,drc-types */ > > + memset(char_buf, 0, sizeof(char_buf)); > > + entries =3D (uint32_t *)&char_buf[0]; > > + *entries =3D SPAPR_DRC_TABLE_SIZE; > > + offset =3D sizeof(*entries); > > + > > + for (i =3D 0; i < SPAPR_DRC_TABLE_SIZE; i++) { > > + offset +=3D sprintf(char_buf + offset, "PHB"); > > + char_buf[offset++] =3D '\0'; > > + } > > + > > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-types", char_buf, of= fset); > > + if (ret) { > > + fprintf(stderr, "Couldn't finalize ibm,drc-types property\n"); > > + } > > +} > > + > > #define _FDT(exp) \ > > do { \ > > int ret =3D (exp); \ > > @@ -731,6 +868,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *sp= apr, > > char *bootlist; > > void *fdt; > > sPAPRPHBState *phb; > > + sPAPRDrcEntry *drc_entry; > > = > > fdt =3D g_malloc(FDT_MAX_SIZE); > > = > > @@ -750,6 +888,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *sp= apr, > > } > > = > > QLIST_FOREACH(phb, &spapr->phbs, list) { > > + drc_entry =3D spapr_phb_to_drc_entry(phb->buid); > > + g_assert(drc_entry); > > ret =3D spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt); > > } > > = > > @@ -789,6 +929,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *sp= apr, > > spapr_populate_chosen_stdout(fdt, spapr->vio_bus); > > } > > = > > + spapr_create_drc_dt_entries(fdt); > > + > > _FDT((fdt_pack(fdt))); > > = > > if (fdt_totalsize(fdt) > FDT_MAX_SIZE) { > > @@ -1443,6 +1585,7 @@ static void ppc_spapr_init(MachineState *machine) > > spapr_pci_msi_init(spapr, SPAPR_PCI_MSI_WINDOW); > > spapr_pci_rtas_init(); > > = > > + spapr_init_drc_table(); > > phb =3D spapr_create_phb(spapr, 0); > > = > > for (i =3D 0; i < nb_nics; i++) { > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 9ed39a9..e85134f 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -531,6 +531,7 @@ static void spapr_phb_realize(DeviceState *dev, Err= or **errp) > > + sphb->index * SPAPR_PCI_WINDOW_SPACING; > > sphb->mem_win_addr =3D windows_base + SPAPR_PCI_MMIO_WIN_OFF; > > sphb->io_win_addr =3D windows_base + SPAPR_PCI_IO_WIN_OFF; > > + spapr_add_phb_to_drc_table(sphb->buid, 2 /* Unusable */); > = > = > What exactly does "unusable" mean here? Macro? That's the 9003/"DR-entity-sense" for the DRC entry corresponding to the PHB itself (as opposed to the sensors for each of its slots). In the case of the slots, we advertise them as 'physical' DR resources in the PHB's "ibm,drc-t= ypes" property. In the case of the PHBs we advertise them as 'logical'/dlpar DR resources in the root DT node's "ibm,drc-types" property. We do not actually support 'logical' DR operations in this implementation though (though we may in the future to support PHB hotplug), so we've set this to 'unusable' for now. But according to PAPR 2.7 13.5.3.3 this corresponds to: "Returned for logical DR entities when the DR entity is not currently avail= able to the OS, but may possibly be made available to the OS by calling set-indi= cator with the allocation-state indicator, setting that indicator to usable." So maybe it makes more sense to just set it to present/(1)? I don't think the PHB sensors will actually get read unless we attempt dlpar operations in the guest (as opposed to pci hp), so it's probably mostly unu= sed now, but seems like a more sensible default. Will test and change it if it doesn't break anything. Macros make sense... we actually already have: #define INDICATOR_ENTITY_SENSE_EMPTY 0 #define INDICATOR_ENTITY_SENSE_PRESENT 1 so not adding 'unused' was an oversight. I'll add it either way for completeness. > = > = > = > > } > > = > > if (sphb->buid =3D=3D -1) { > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 36e8e51..c93794b 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -10,6 +10,36 @@ struct sPAPRNVRAM; > > = > > #define HPTE64_V_HPTE_DIRTY 0x0000000000000040ULL > > = > > +/* For dlparable/hotpluggable slots */ > > +#define SPAPR_DRC_TABLE_SIZE 32 > > +#define SPAPR_DRC_PHB_SLOT_MAX 32 > > +#define SPAPR_DRC_DEV_ID_BASE 0x40000000 > = > = > Is this SPAPR_DRC_DEV_ID_BASE really necessary (can it be zero)? Is that > global id or per PCI bus or per PHB? > = > = > > + > > +typedef struct sPAPRConfigureConnectorState { > > + void *fdt; > > + int offset_start; > > + int offset; > > + int depth; > > + PCIDevice *dev; > > + enum { > > + CC_STATE_IDLE =3D 0, > > + CC_STATE_PENDING =3D 1, > > + CC_STATE_ACTIVE, > > + } state; > > +} sPAPRConfigureConnectorState; > > + > > +typedef struct sPAPRDrcEntry sPAPRDrcEntry; > > + > > +struct sPAPRDrcEntry { > > + uint32_t drc_index; > > + uint64_t phb_buid; > > + void *fdt; > > + int fdt_offset; > > + uint32_t state; > > + sPAPRConfigureConnectorState cc_state; > > + sPAPRDrcEntry *child_entries; > > +}; > > + > > typedef struct sPAPREnvironment { > > struct VIOsPAPRBus *vio_bus; > > QLIST_HEAD(, sPAPRPHBState) phbs; > > @@ -39,6 +69,9 @@ typedef struct sPAPREnvironment { > > int htab_save_index; > > bool htab_first_pass; > > int htab_fd; > > + > > + /* state for Dynamic Reconfiguration Connectors */ > > + sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE]; > > } sPAPREnvironment; > > = > > #define H_SUCCESS 0 > > @@ -481,5 +514,7 @@ int spapr_dma_dt(void *fdt, int node_off, const cha= r *propname, > > uint32_t liobn, uint64_t window, uint32_t size); > > int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname, > > sPAPRTCETable *tcet); > > +sPAPRDrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t stat= e); > > +sPAPRDrcEntry *spapr_phb_to_drc_entry(uint64_t buid); > > = > > #endif /* !defined (__HW_SPAPR_H__) */ > > = > = > = > -- = > Alexey