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

[Tyrel Datwyler]

On 08/26/2014 08:25 AM, Michael Roth wrote:
> 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.

It is ture that PAPR only requires that these values be unique, and I'm
not currently aware of guest tools that make assumptions about the DRC
values for different DRC connectors. However, seeing as we are emulating
a pseries guest I picked base DRC values that matched those used by PHYP.

CPU     0x10000000
VIO     0x30000000
LMB     0x80000000
PHB     0x20000000
PCI     0x40000000

-Tyrel

> 
>>
>>
>>>
>>>> +
>>>> +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