Re: [Qemu-devel] [PATCH v6 02/15] spapr_drc: initial implementation of sPAPRDRConnector device

[Bharata B Rao]

On Fri, Feb 27, 2015 at 8:41 AM, Michael Roth <address@hidden> wrote:
> +static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> +                   int fdt_start_offset, bool coldplug, Error **errp)
> +{
> +    DPRINTFN("drc: %x, attach", get_index(drc));
> +
> +    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +        error_setg(errp, "an attached device is still awaiting release");
> +        return;
> +    }
> +    g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE);

allocation_state starts with UNUSABLE

> +    g_assert(fdt || coldplug);
> +
> +    /* NOTE: setting initial isolation state to UNISOLATED means we can't
> +     * detach unless guest has a userspace/kernel that moves this state
> +     * back to ISOLATED in response to an unplug event, or this is done
> +     * manually by the admin prior. if we force things while the guest
> +     * may be accessing the device, we can easily crash the guest, so we
> +     * we defer completion of removal in such cases to the reset() hook.
> +     */
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> +    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;

Here is it changed to USABLE.

> +    drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> +
> +    drc->dev = d;
> +    drc->ccs.fdt = fdt;
> +    drc->ccs.fdt_offset = drc->ccs.fdt_start_offset = fdt_start_offset;
> +    drc->ccs.fdt_depth = 0;
> +    drc->ccs.configured = false;
> +
> +    object_property_add_link(OBJECT(drc), "device",
> +                             object_get_typename(OBJECT(drc->dev)),
> +                             (Object **)(&drc->dev),
> +                             NULL, 0, NULL);
> +}
> +

> +/*
> + * dr-entity-sense sensor value
> + * returned via get-sensor-state RTAS calls
> + * as expected by state diagram in PAPR+ 2.7, 13.4
> + * based on the current allocation/indicator/power states
> + * for the DR connector.
> + */
> +static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
> +{
> +    if (drc->dev) {
> +        if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> +            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {

When we come here via get-sensor-state RTAS call, allocation_state is
already USABLE. So the above if condition is never true for CPU
hotplug case...

> +            /* for logical DR, we return a state of UNUSABLE
> +             * iff the allocation state UNUSABLE.
> +             * Otherwise, report the state as USABLE/PRESENT,
> +             * as we would for PCI.
> +             */
> +            return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> +        }
> +
> +        /* this assumes all PCI devices are assigned to
> +         * a 'live insertion' power domain, where QEMU
> +         * manages power state automatically as opposed
> +         * to the guest. present, non-PCI resources are
> +         * unaffected by power state.
> +         */
> +        return SPAPR_DR_ENTITY_SENSE_PRESENT;

... and hence get-sensor-state RTAS call always ends up returning
PRESENT for CPU hotplug case which breaks CPU hotplug.

> +    }
> +
> +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +        /* PCI devices, and only PCI devices, use EMPTY
> +         * in cases where we'd otherwise use UNUSABLE
> +         */
> +        return SPAPR_DR_ENTITY_SENSE_EMPTY;
> +    }
> +    return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> +}
> +
> +