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

[Alexey Kardashevskiy]

On 01/17/2014 07:51 AM, Michael Roth wrote:
> Quoting Alexey Kardashevskiy (2013-12-15 22:54:42)
>> On 12/16/2013 01:59 PM, Alexey Kardashevskiy wrote:
>>> On 12/06/2013 09:32 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         |  132 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/ppc/spapr.h |   33 ++++++++++++
>>>>  2 files changed, 165 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 7e53a5f..ec3ba43 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -81,6 +81,7 @@
>>>>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>>>>  
>>>>  sPAPREnvironment *spapr;
>>>> +DrcEntry drc_table[SPAPR_DRC_TABLE_SIZE];
>>>>  
>>>>  int spapr_allocate_irq(int hint, bool lsi)
>>>>  {
>>>> @@ -276,6 +277,130 @@ static size_t create_page_sizes_prop(CPUPPCState 
>>>> *env, uint32_t *prop,
>>>>      return (p - prop) * sizeof(uint32_t);
>>>>  }
>>>>  
>>>> +static void spapr_init_drc_table(void)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    memset(drc_table, 0, sizeof(drc_table));
>>>> +
>>>> +    /* For now we only care about PHB entries */
>>>> +    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
>>>> +        drc_table[i].drc_index = 0x2000001 + i;
>>>> +    }
>>>> +}
>>>> +
>>>> +DrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t state)
>>>> +{
>>>> +    DrcEntry *empty_drc = NULL;
>>>> +    DrcEntry *found_drc = NULL;
>>>> +    int i, phb_index;
>>>> +
>>>> +    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
>>>> +        if (drc_table[i].phb_buid == 0) {
>>>> +            empty_drc = &drc_table[i];
>>>> +        }
>>>> +
>>>> +        if (drc_table[i].phb_buid == buid) {
>>>> +            found_drc = &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(DrcEntry) * 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] = 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");
>>>> +    }
>>>> +
>>>> +    /* 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);                                           \
>>>> @@ -307,6 +432,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>      int i, smt = kvmppc_smt_threads();
>>>>      unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>>>>  
>>>> +    spapr_init_drc_table();
>>>> +
>>>>      fdt = g_malloc0(FDT_MAX_SIZE);
>>>>      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
>>>>  
>>>> @@ -590,6 +717,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>>>      int ret;
>>>>      void *fdt;
>>>>      sPAPRPHBState *phb;
>>>> +    DrcEntry *drc_entry;
>>>>  
>>>>      fdt = g_malloc(FDT_MAX_SIZE);
>>>>  
>>>> @@ -609,6 +737,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>>>      }
>>>>  
>>>>      QLIST_FOREACH(phb, &spapr->phbs, list) {
>>>> +        drc_entry = spapr_add_phb_to_drc_table(phb->buid, 2 /* Unusable 
>>>> */);
>>>> +        g_assert(drc_entry);
>>>>          ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
>>>>      }
>>>>  
>>>> @@ -633,6 +763,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) {
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index b2f11e9..0f2e705 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -299,6 +299,39 @@ typedef struct sPAPREnvironment {
>>>>  #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
>>>>  #define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
>>>>  
>>>> +/* For dlparable/hotpluggable slots */
>>>> +#define SPAPR_DRC_TABLE_SIZE    32
>>>> +#define SPAPR_DRC_PHB_SLOT_MAX  32
>>>> +#define SPAPR_DRC_DEV_ID_BASE   0x40000000
>>>> +
>>>> +typedef struct ConfigureConnectorState {
>>>> +    void *fdt;
>>>> +    int offset_start;
>>>> +    int offset;
>>>> +    int depth;
>>>> +    PCIDevice *dev;
>>>> +    enum {
>>>> +        CC_STATE_IDLE = 0,
>>>> +        CC_STATE_PENDING = 1,
>>>> +        CC_STATE_ACTIVE,
>>>> +    } state;
>>>> +} ConfigureConnectorState;
>>>> +
>>>> +typedef struct DrcEntry DrcEntry;
>>>> +
>>>> +struct DrcEntry {
>>>> +    uint32_t drc_index;
>>>> +    uint64_t phb_buid;
>>>> +    void *fdt;
>>>> +    int fdt_offset;
>>>> +    uint32_t state;
>>>> +    ConfigureConnectorState cc_state;
>>>> +    DrcEntry *child_entries;
>>>> +};
>>>> +
>>>> +extern DrcEntry drc_table[SPAPR_DRC_TABLE_SIZE];
>>>> +DrcEntry *spapr_add_phb_to_drc_table(uint64_t buid, uint32_t state);
>>>> +
>>>>  extern sPAPREnvironment *spapr;
>>>
>>> So far we were trying to keep everything sPAPR-related in sPAPREnvironment.
>>> Is @drc_table really that special?
>>
>>
>> One more note - we are trying to add a "spapr" or "sPAPR" prefix to all
>> global types defines in headers (such as sPAPRPHBState, spapr_pci_lsi,
>> VIOsPAPRBus, sPAPREnvironment), it would be nice to have "spapr" in some
>> form in these new types too.
>>
>> Or we could move the whole patch (except spapr_create_drc_dt_entries()) to
>> hw/ppc/spapr_pci.c (and keep the original names) as it seems to be the only
>> user of the whole DrcEntry and ConfigureConnectorState thing.
>> And put a pointer to drc_table[] into @spapr (or make it static?)
> 
> That would work, but I think we'd need to move spapr_create_drc_dt_entries()
> as well, or the bits that rely on DrcEntry at least. Though I worry
> about scoping DrcEntry to spapr_pci.c at this early stage, as DR-capable
> components other than PCI may come to rely on state that's captured by the
> DrcEntry nodes, such as boot-time FDT generation and run-time management
> (via ibm,configure-connector) of CPUs and memory.
> 
> Assuming that seems like a reasonable expectation, I think I'd prefer the
> first option of using spapr-specific prefixes for global types and moving
> drc_table into sPAPREnvironment


I did not realize DRC is not just for PCI. How hard would it be to add hot
plug support for a whole PHB? The current QEMU trend is to make QEMU
monitor's "device_add" equal to the command line's "-device" which is not
(yet) true for PHB but could be. Thanks.



>> The only remaining user of DrcEntry is spapr_hotplug_req_event() but this
>> can be easily fixed by small helper like this:
>>
>> int spapr_phb_slot_to_drc_index(uint64_t buid, int slot)
>> {
>>         DrcEntry *drc_entry = spapr_phb_to_drc_entry(phb->buid);
>>         if (!drc_entry) {
>>                 drc_entry = spapr_add_phb_to_drc_table(phb->buid, 2);
>>         }
>>         return drc_entry->child_entries[slot].drc_index;
>> }




-- 
Alexey