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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Mon, Feb 16, 2015 at 08:27:38AM -0600, Michael Roth wrote:
> This device emulates a firmware abstraction used by pSeries guests to
> manage hotplug/dynamic-reconfiguration of host-bridges, PCI devices,
> memory, and CPUs. It is conceptually similar to an SHPC device,
> complete with LED indicators to identify individual slots to physical
> physical users and indicate when it is safe to remove a device. In
> some cases it is also used to manage virtualized resources, such a
> memory, CPUs, and physical-host bridges, which in the case of pSeries
> guests are virtualized resources where the physical components are
> managed by the host.
> 
> Guests communicate with these DR Connectors using RTAS calls,
> generally by addressing the unique DRC index associated with a
> particular connector for a particular resource. For introspection
> purposes we expose this state initially as QOM properties, and
> in subsequent patches will introduce the RTAS calls that make use of
> it. This constitutes to the 'guest' interface.
> 
> On the QEMU side we provide an attach/detach interface to associate
> or cleanup a DeviceState with a particular sPAPRDRConnector in
> response to hotplug/unplug, respectively. This constitutes the
> 'physical' interface to the DR Connector.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Two tiny warts:

[...]
> +static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> +{
> +    uint32_t shift = 0;
> +
> +    g_assert(type != SPAPR_DR_CONNECTOR_TYPE_ANY);

assert(is_power_of_2(type)) maybe?  Nicer than an infinite loop if a
bad value is passed in here.

[...]
> +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) {
> +            /* 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;
> +    }
> +
> +    if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +        /* PCI devices, and only PCI devices, use PRESENT

s/PRESENT/EMPTY/ ??

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]