Re: [Qemu-devel] [PATCH 01/12] spapr: populate DRC entries for root dt node

[Michael Roth]

Quoting Michael Roth (2014-08-26 10:25:13)
> Quoting Alexey Kardashevskiy (2014-08-26 03:24:08)
> > On 08/26/2014 05:55 PM, Alexey Kardashevskiy wrote:
> > > On 08/19/2014 10:21 AM, Michael Roth wrote:
> > >> From: Nathan Fontenot <address@hidden>
> > >>
> > >> 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<bus#>, 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 <address@hidden>
> > >> Signed-off-by: Michael Roth <address@hidden>
> > >> ---
> > >>  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 = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        if (spapr->drc_table[i].phb_buid == 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 = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        spapr->drc_table[i].drc_index = 0x2000001 + i;
> > >> +    }
> > >> +}
> > >> +
> > >> +sPAPRDrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t state)
> > >> +{
> > >> +    sPAPRDrcEntry *empty_drc = NULL;
> > >> +    sPAPRDrcEntry *found_drc = NULL;
> > >> +    int i, phb_index;
> > >> +
> > >> +    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        if (spapr->drc_table[i].phb_buid == 0) {
> > >> +            empty_drc = &spapr->drc_table[i];
> > >> +        }
> > >> +
> > >> +        if (spapr->drc_table[i].phb_buid == buid) {
> > >> +            found_drc = &spapr->drc_table[i];
> > > 
> > > return &spapr->drc_table[i];
> > > ?
> > > 
> > > 
> > >> +            break;
> > >> +        }
> > >> +    }
> > >> +
> > >> +    if (found_drc) {
> > >> +        return found_drc;
> > >> +    }
> > > 
> > >    if (!empty_drc) {
> > >         return NULL;
> > >    }
> > > 
> > > ?
> > > 
> > > 
> > >> +
> > >> +    if (empty_drc) {
> > > 
> > > and no need in this :)
> > > 
> > > 
> > >> +        empty_drc->phb_buid = buid;
> > >> +        empty_drc->state = state;
> > >> +        empty_drc->cc_state.fdt = NULL;
> > >> +        empty_drc->cc_state.offset = 0;
> > >> +        empty_drc->cc_state.depth = 0;
> > >> +        empty_drc->cc_state.state = CC_STATE_IDLE;
> > >> +        empty_drc->child_entries =
> > >> +            g_malloc0(sizeof(sPAPRDrcEntry) * SPAPR_DRC_PHB_SLOT_MAX);
> > >> +        phb_index = buid - SPAPR_PCI_BASE_BUID;
> > >> +        for (i = 0; i < SPAPR_DRC_PHB_SLOT_MAX; i++) {
> > >> +            empty_drc->child_entries[i].drc_index =
> > >> +                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 = fdt_path_offset(fdt, "/");
> > >> +
> > >> +    /* ibm,drc-indexes */
> > >> +    memset(int_buf, 0, sizeof(int_buf));
> > >> +    int_buf[0] = SPAPR_DRC_TABLE_SIZE;
> > >> +
> > >> +    for (i = 1; i <= SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        int_buf[i] = spapr->drc_table[i-1].drc_index;
> > >> +    }
> > >> +
> > >> +    ret = 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?
> > > 
> > >> +    }
> > >> +
> > >> +    /* ibm,drc-power-domains */
> > >> +    memset(int_buf, 0, sizeof(int_buf));
> > >> +    int_buf[0] = SPAPR_DRC_TABLE_SIZE;
> > >> +
> > >> +    for (i = 1; i <= SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        int_buf[i] = 0xffffffff;
> > >> +    }
> > >> +
> > >> +    ret = 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 
> > >> property\n");
> > >> +    }
> > >> +
> > >> +    /* ibm,drc-names */
> > >> +    memset(char_buf, 0, sizeof(char_buf));
> > >> +    entries = (uint32_t *)&char_buf[0];
> > >> +    *entries = SPAPR_DRC_TABLE_SIZE;
> > >> +    offset = sizeof(*entries);
> > >> +
> > >> +    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        offset += sprintf(char_buf + offset, "PHB %d", i + 1);
> > >> +        char_buf[offset++] = '\0';
> > >> +    }
> > >> +
> > >> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-names", char_buf, 
> > >> offset);
> > >> +    if (ret) {
> > >> +        fprintf(stderr, "Couldn't finalize ibm,drc-names property\n");
> > >> +    }
> > >> +
> > >> +    /* ibm,drc-types */
> > >> +    memset(char_buf, 0, sizeof(char_buf));
> > >> +    entries = (uint32_t *)&char_buf[0];
> > >> +    *entries = SPAPR_DRC_TABLE_SIZE;
> > >> +    offset = sizeof(*entries);
> > >> +
> > >> +    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
> > >> +        offset += sprintf(char_buf + offset, "PHB");
> > >> +        char_buf[offset++] = '\0';
> > >> +    }
> > >> +
> > >> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-types", char_buf, 
> > >> offset);
> > >> +    if (ret) {
> > >> +        fprintf(stderr, "Couldn't finalize ibm,drc-types property\n");
> > >> +    }
> > >> +}
> > >> +
> > >>  #define _FDT(exp) \
> > >>      do { \
> > >>          int ret = (exp);                                           \
> > >> @@ -731,6 +868,7 @@ static void spapr_finalize_fdt(sPAPREnvironment 
> > >> *spapr,
> > >>      char *bootlist;
> > >>      void *fdt;
> > >>      sPAPRPHBState *phb;
> > >> +    sPAPRDrcEntry *drc_entry;
> > >>  
> > >>      fdt = g_malloc(FDT_MAX_SIZE);
> > >>  
> > >> @@ -750,6 +888,8 @@ static void spapr_finalize_fdt(sPAPREnvironment 
> > >> *spapr,
> > >>      }
> > >>  
> > >>      QLIST_FOREACH(phb, &spapr->phbs, list) {
> > >> +        drc_entry = spapr_phb_to_drc_entry(phb->buid);
> > >> +        g_assert(drc_entry);
> > >>          ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
> > >>      }
> > >>  
> > >> @@ -789,6 +929,8 @@ static void spapr_finalize_fdt(sPAPREnvironment 
> > >> *spapr,
> > >>          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 = spapr_create_phb(spapr, 0);
> > >>  
> > >>      for (i = 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, 
> > >> Error **errp)
> > >>              + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> > >>          sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> > >>          sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
> > >> +        spapr_add_phb_to_drc_table(sphb->buid, 2 /* Unusable */);
> > > 
> > > 
> > > What exactly does "unusable" mean here? Macro?
> > > 
> > > 
> > > 
> > >>      }
> > >>  
> > >>      if (sphb->buid == -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?
> > 
> > 
> > Ah. Got it. If it was like below, I would not even ask :)
> > 
> > #define SPAPR_DRC_DEV_ID(phb_index, slot) \
> >         (0x40000000 | ((phb_index)<<8) | ((slot)<<3))
> > 
> > Still not clear why you need this 0x40000000 for. Is it kind of "PHB" DRC 
> > type?
> 
> Yes, it's somewhat ad-hoc...the only requirement I see in PAPR is that this 
> value be
> globally unique across all DR resources. CPUs and memory and such might have 
> different
> ways to compute their DRC indices (so a slot-based macro would need to be 
> specific
> to PCI DR entries). I'm not sure where the 0x40000000 originated honestly. 
> I'm not
> sure it matters for QEMU, since we hold a monopoly on all DRC index 
> assignments and
> don't have to deal with hard-coded firmware values.
> 
> I will say that a base somewhat less common than 0 may prove useful from a 
> debugging
> standpoint, all other things being equal.
> 
> So not sure what best to do here. If we choose to leave it as is, I could at 
> least
> make sure to add a comment about this.

Hmm, this all also applies to the 0x2000001 drc base used for the parent PHB DRC
indices that Alex mentioned.

If we want to do something a little more structured, we could take the hotplug 
types
introduced later:

  #define RTAS_LOG_V6_HP_TYPE_CPU                          1
  #define RTAS_LOG_V6_HP_TYPE_MEMORY                       2
  #define RTAS_LOG_V6_HP_TYPE_SLOT                         3
  #define RTAS_LOG_V6_HP_TYPE_PHB                          4
  #define RTAS_LOG_V6_HP_TYPE_PCI                          5

As the DRC index base. A macro representation would basically be:

#define SPAPR_DRC_DEV_ID_BASE(dr_type) (dr_type << 28)

I don't really like that 'dr_type' doesn't actually correspond to the
ibm,drc-types ofdt property though, which are string values, unfortunately.
("PHB" for PHB, and "28" for PCI slot...). We could do a string->dr_type lookup
in the macro, but passing around "28" doesn't seem very readable either...

> 
> > 
> > 
> > > 
> > >> +
> > >> +typedef struct sPAPRConfigureConnectorState {
> > >> +    void *fdt;
> > >> +    int offset_start;
> > >> +    int offset;
> > >> +    int depth;
> > >> +    PCIDevice *dev;
> > >> +    enum {
> > >> +        CC_STATE_IDLE = 0,
> > >> +        CC_STATE_PENDING = 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 char 
> > >> *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 
> > >> state);
> > >> +sPAPRDrcEntry *spapr_phb_to_drc_entry(uint64_t buid);
> > >>  
> > >>  #endif /* !defined (__HW_SPAPR_H__) */
> > >>
> > > 
> > > 
> > 
> > 
> > -- 
> > Alexey