[libvirt] [RFC v3] Export KVM Host Power Management capabilities

[Eric Blake]

On 08/08/2011 09:18 AM, Srivatsa S. Bhat wrote:
> Open issues:
> -----------
> 1. Design new APIs in libvirt to actually exploit the host power management
>     features instead of relying on external programs. This was discussed in
>     [4].

As pointed out in [4], until we have additional use for this feature, 
then this feature in isolation is unlikely to be pushed.  That is not 
meant to discourage you from developing further patches, rather it is 
just stating that this patch will probably be deferred until it can be 
pushed as part of a larger series.

> 2. Decide on whether to include "pm-utils" package in the libvirt.spec
>     file considering the fact that the package name (pm-utils) may differ
>     from one Linux distribution to another.

No decision to make - libvirt.spec is solely for Fedora, where the 
package is always named pm-utils.  Other distros use other packaging 
files, and can adjust accordingly, it's just that none of those other 
packaging files have been contributed for inclusion in upstream libvirt.git.

> +  <define name='power_management'>
> +    <choice>

No need for a <choice> here.

> +      <element name='power_management'>
> +        <optional>
> +          <element name='S3'>
> +            <empty/>
> +          </element>
> +        </optional>
> +        <optional>
> +          <element name='S4'>
> +            <empty/>
> +          </element>
> +        </optional>
> +      </element>

The two <optional> blocks are sufficient for allowing 
<power_management/> as an empty element, without any further rng grammar.

>
> +/**
> + * virCapabilitiesAddHostPowerManagement:
> + * @caps: capabilities to extend
> + * @feature: the power management feature to be added
> + *
> + * Registers a new host power management feature, eg: 'S3' or 'S4'
> + */
> +int
> +virCapabilitiesAddHostPowerManagement(virCapsPtr caps,
> +                                      int feature)
> +{
> +    if(VIR_RESIZE_N(caps->host.powerMgmt, caps->host.npowerMgmt_max,
> +                   caps->host.npowerMgmt, 1)<  0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    caps->host.powerMgmt[caps->host.npowerMgmt] = feature;
> +    caps->host.npowerMgmt++;

After thinking about this more, I'm wondering if it would be smarter to 
represent power management as a bitmask:

caps->hst.powerMgmt |= 1U << feature;

at which point, you still need isPMQuerySuccess (but see below on 
naming), but you don't need npowerMgmt.  Instead, output would be:

> @@ -686,6 +717,25 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>
>       virBufferAddLit(&xml, "</cpu>\n");
>
> +    if(caps->host.isPMQuerySuccess) {

if (caps->host.powerMgmt) {
   unsigned int pm = caps->host.powerMgmt;
   virBufferAddLit(&xml, "<power_management>\n");
   while (pm) {
     int bit = ffs(pm) - 1;
     virBufferAsprintf(&xml, "<%s/>\n",
       virHostPMCapabilityTypeToString(bit);
     pm &= ~bit;
   }
   virBufferAddLit(&xml, "</power_management>\n");
} else {
   virBufferAddLit(&xml, "<power_management/>\n");
}

> +++ b/src/conf/capabilities.h
> @@ -105,6 +105,10 @@ struct _virCapsHost {
>       size_t nfeatures;
>       size_t nfeatures_max;
>       char **features;
> +    bool isPMQuerySuccess;

This isn't typical naming.  For consistency, I'm thinking:

bool powerMgmt_valid;

defaulting to false (powerMgmt is irrelavant), but set to true when 
powerMgmt contains valid data (where 0 can include valid data).

> +    size_t npowerMgmt;
> +    size_t npowerMgmt_max;
> +    int *powerMgmt;    /* enum virHostPMCapability */

See above about just using 'unsigned int powerMgmt' as a bit-mask of 
valid capabilities, rather than a dynamically allocated array of ints.

> @@ -824,6 +825,32 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
>           old_caps->host.cpu = NULL;
>       }
>
> +    /* Add the power management features of the host */
> +
> +    /* Check for Suspend-to-RAM support (S3) */
> +    status = virCheckPMCapability(VIR_HOST_PM_S3);
> +    if(status<  0) {
> +        caps->host.isPMQuerySuccess = false;
> +        VIR_WARN("Failed to get host power management features");
> +    } else {
> +        /* The PM Query succeeded */
> +        caps->host.isPMQuerySuccess = true;
> +        if(status == 1)    /* S3 is supported */
> +            virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S3);
> +    }
> +
> +    /* Check for Suspend-to-Disk support (S4) */
> +    status = virCheckPMCapability(VIR_HOST_PM_S4);
> +    if(status<  0) {
> +        caps->host.isPMQuerySuccess = false;
> +        VIR_WARN("Failed to get host power management features");
> +    } else {
> +        /* The PM Query succeeded */
> +        caps->host.isPMQuerySuccess = true;
> +        if(status == 1)    /* S4 is supported */
> +            virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S4);
> +    }

You don't want to set caps->host.isPMQuerySuccess (by whatever name) to 
true unless both commands succeeded.  If the first command succeeds but 
the second fails, then you are better off treating the entire operation 
as failed.

Hmm, maybe virCheckPMCapability should take no arguments, and return the 
bitmask directly, rather than making qemuCapsInit have to compute the 
right bitmask.

>   }
> +
> +/**
> + * Check the Power Management Capabilities of the host system.
> + * The script 'pm-is-supported' (from the pm-utils package) is run
> + * to find out if the capability is supported by the host.
> + *
> + * @capability: capability to check for
> + * VIR_HOST_PM_S3: Check for Suspend-to-RAM support
> + * VIR_HOST_PM_S4: Check for Suspend-to-Disk support
> + *
> + * Return values:
> + * 1 if the capability is supported.
> + * 0 if the query was successful but the capability is
> + *   not supported by the host.
> + * -1 on error like 'pm-is-supported' is not found.
> + */
> +int
> +virCheckPMCapability(int capability)
> +{

As I mentioned above, it would be nicer to have this return the bitmask 
up front, rather than to make each caller have to repeatedly call this 
function for as many capabilities as are currently defined.  -1 for 
unknown, 0 for success but no support, or positive for a bitmask of 
1<<bit for each supported bit.

> +++ b/src/util/virterror.c
> @@ -148,6 +148,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
>           case VIR_FROM_CPU:
>               dom = "CPU ";
>               break;
> +        case VIR_FROM_CAPABILITIES:
> +            dom = "Capabilities ";
> +            break;
>           case VIR_FROM_NWFILTER:
>               dom = "Network Filter ";
>               break;

Better to make these case statements line up with declaration order 
(VIR_FROM_CAPABILITIES is new, so it should be at the end of the list 
just before default:).

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org