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

[Alexander Graf]

On 26.08.14 18:47, Michael Roth wrote:
> Quoting Alexander Graf (2014-08-26 06:11:24)
>> On 19.08.14 02:21, 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;
>>
>> magic number?
>>
>>> +    }
>>> +}
>>> +
>>> +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];
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (found_drc) {
>>> +        return found_drc;
>>> +    }
>>> +
>>> +    if (empty_drc) {
>>> +        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;
>>
>> Not endian safe.
>>
>>> +    }
>>> +
>>> +    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");
>>> +    }
>>> +
>>> +    /* ibm,drc-power-domains */
>>> +    memset(int_buf, 0, sizeof(int_buf));
>>> +    int_buf[0] = SPAPR_DRC_TABLE_SIZE;
>>
>> Not endian safe.
>>
>>> +
>>> +    for (i = 1; i <= SPAPR_DRC_TABLE_SIZE; i++) {
>>> +        int_buf[i] = 0xffffffff;
>>> +    }
>>
>> memset(-1) instead above?
>>
>>> +
>>> +    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;
>>
>> Not endian safe. I guess you get the idea. I'll stop looking for endian
>> problems here :).
>>
>>> +    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);
>>
>> I would really prefer if we can stick to always use the spapr as
>> function parameter, not use the global.
>>
>>> +
>>>      _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 */);
>>>      }
>>>  
>>>      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
>>
>> Can we make this dynamic so that we can set it to 0 for pseries-2.0 (if
>> necessary) or have an easy tunable to extend the list later?
> 
> We could introduce something like -machine pseries,max-dr-connectors=x maybe,
> and set the default based on current machine. Though it's worth noting future
> stuff like cpu/mem DRC entries will get allocated via the same top-level
> ibm,drc-indexes list property (before or after PHB entries), so
> the meaning of that option would change unless we name it something specific
> to PHBs entries, like max-phb-dr-connectors.

I don't think we'd have to expose this to the user at all, so the naming
doesn't have to stay consistent :).


Alex