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

[Michael Roth]

Quoting Alexey Kardashevskiy (2014-08-26 02:55:05)
> 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];
> ?

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 = NULL;
    int i, phb_index;

    for (i = 0; i < SPAPR_DRC_TABLE_SIZE; i++) {
        g_assert(spapr->drc_table[i].phb_buid != buid);
        if (spapr->drc_table[i].phb_buid == 0) {
            empty_drc = &spapr->drc_table[i];
            break;
        }
    }

    if (!empty_drc) {
        return NULL;
    }

    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;
}

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

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] = 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?

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-types"
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 available
to the OS, but may possibly be made available to the OS by calling set-indicator
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 unused
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 == -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 = 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