Re: [Qemu-ppc] [PATCH 2/4] ppc/spapr-caps: Add support for custom spapr_capabilities

[David Gibson]

On Thu, Mar 01, 2018 at 03:45:01PM +1100, Suraj Jitindar Singh wrote:
> There are currently 2 implemented types of spapr-caps, boolean and
> tristate. However there may be a need for caps which don't fit either of
> these options. Add a custom capability type for which a list of custom
> valid strings can be specified and implement the get/set functions for
> these. Also add a field for help text to describe the available options.
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>

Looks good except for some trivia:

Eventually it might be worth implementing the standard tristate values
using the same mechanism, but that can be done later.

> ---
>  hw/ppc/spapr_caps.c | 68 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 99a4b71d19..449e30bae0 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -32,6 +32,20 @@
>  
>  #include "hw/ppc/spapr.h"
>  
> +typedef struct sPAPRCapPossible {
> +    int num;            /* size of vals array below */
> +    const char *help;   /* help text for vals */
> +    /*
> +     * Note:
> +     * - because of the way compatibility is determined vals MUST be ordered
> +     *   such that proceeding options are a superset of (all) preceeding 
> options

ITYM "such that later options are a superset of all preceding options"

(note spelling of "preceding" as well).

> +     * - the order of vals must be preserved, that is their index is 
> important,
> +     *   however vals may be added to the end of the list so long as the 
> above
> +     *   point is observed
> +     */
> +    const char *vals[];
> +} sPAPRCapPossible;
> +
>  typedef struct sPAPRCapabilityInfo {
>      const char *name;
>      const char *description;
> @@ -41,6 +55,8 @@ typedef struct sPAPRCapabilityInfo {
>      ObjectPropertyAccessor *get;
>      ObjectPropertyAccessor *set;
>      const char *type;
> +    /* Possible values if this is a custom string type */
> +    sPAPRCapPossible *possible;
>      /* Make sure the virtual hardware can support this capability */
>      void (*apply)(sPAPRMachineState *spapr, uint8_t val, Error **errp);
>  } sPAPRCapabilityInfo;
> @@ -133,6 +149,58 @@ out:
>      g_free(val);
>  }
>  
> +static void spapr_cap_get_string(Object *obj, Visitor *v, const char *name,
> +                                 void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    char *val = NULL;
> +    uint8_t value = spapr_get_cap(spapr, cap->index);
> +
> +    if (value >= cap->possible->num) {
> +        error_setg(errp, "Invalid value (%d) for cap-%s", value, cap->name);
> +        return;
> +    }
> +
> +    val = g_strdup(cap->possible->vals[value]);
> +
> +    visit_type_str(v, name, &val, errp);
> +    g_free(val);
> +}
> +
> +static void spapr_cap_set_string(Object *obj, Visitor *v, const char *name,
> +                                 void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    Error *local_err = NULL;
> +    uint8_t i;
> +    char *val;
> +
> +    visit_type_str(v, name, &val, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (!strcasecmp(val, "help")) {
> +        error_setg(errp, "%s", cap->possible->help);
> +        goto out;
> +    }
> +    for (i = 0; i < cap->possible->num; i++) {
> +        if (!strcasecmp(val, cap->possible->vals[i])) {
> +            spapr->cmd_line_caps[cap->index] = true;
> +            spapr->eff.caps[cap->index] = i;
> +            goto out;
> +        }
> +    }
> +
> +    error_setg(errp, "Invalid capability mode \"%s\" for cap-%s", val,
> +               cap->name);
> +out:
> +    g_free(val);
> +}
> +
>  static void cap_htm_apply(sPAPRMachineState *spapr, uint8_t val, Error 
> **errp)
>  {
>      if (!val) {

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

signature.asc
Description: PGP signature