Re: [Qemu-devel] [PATCH 1/6] spapr: Capabilities infrastructure

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

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

On Mon, Dec 18, 2017 at 10:58:00AM +0100, Greg Kurz wrote:
> On Mon, 18 Dec 2017 20:20:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > +void spapr_caps_reset(sPAPRMachineState *spapr)
> > +{
> > +    Error *local_err = NULL;
> > +    sPAPRCapabilities caps;
> > +    int i;
> > +
> > +    /* First compute the actual set of caps we're running with.. */
> > +    caps = default_caps_with_cpu(spapr, first_cpu);
> > +
> > +    caps.mask |= spapr->forced_caps.mask;
> > +    caps.mask &= ~spapr->forbidden_caps.mask;
> > +
> > +    spapr->effective_caps = caps;
> > +
> > +    /* .. then apply those caps to the virtual hardware */
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        sPAPRCapabilityInfo *info = &capability_table[i];
> > +
> > +        if (spapr->effective_caps.mask & info->flag) {
> > +            /* Failure to allow a cap is fatal - if the guest doesn't
> > +             * have it, we'll be supplying an incorrect environment */
> > +            if (info->allow) {
> > +                info->allow(spapr, &error_fatal);
> > +            }
> > +        } else {
> > +            /* Failure to enforce a cap is only a warning.  The guest
> > +             * shouldn't be using it, since it's not advertised, so it
> > +             * doesn't get to complain about weird behaviour if it
> > +             * goes ahead anyway */
> > +            if (info->disallow) {
> > +                info->disallow(spapr, &local_err);
> > +            }
> > +            if (local_err) {
> > +                warn_report_err(local_err);
> > +                error_free(local_err);
> 
> double free.
> 
> /*
>  * Convenience function to warn_report() and free @err.
>  */
> void warn_report_err(Error *err);

Drat, I've made that mistake before.  Fixed now.

> The rest of the patch is fine, so with the above fixed, you can add:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +                local_err = NULL;
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
> > +                          void *opaque, Error **errp)
> > +{
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    bool value = spapr_has_cap(spapr, cap->flag);
> > +
> > +    /* TODO: Could this get called before effective_caps is finalized
> > +     * in spapr_caps_reset()? */
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cap_set(Object *obj, Visitor *v, const char *name,
> > +                          void *opaque, Error **errp)
> > +{
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    bool value;
> > +    Error *local_err = NULL;
> > +
> > +    visit_type_bool(v, name, &value, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    if (value) {
> > +        spapr->forced_caps.mask |= cap->flag;
> > +    } else {
> > +        spapr->forbidden_caps.mask |= cap->flag;
> > +    }
> > +}
> > +
> > +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> > +{
> > +    uint64_t allcaps = 0;
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        g_assert((allcaps & capability_table[i].flag) == 0);
> > +        allcaps |= capability_table[i].flag;
> > +    }
> > +
> > +    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> > +    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> > +
> > +    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> > +        error_setg(errp, "Some sPAPR capabilities set both on and off");
> > +        return;
> > +    }
> > +
> > +    /* Check for any caps incompatible with other caps.  Nothing to do
> > +     * yet */
> > +}
> > +
> > +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    ObjectClass *klass = OBJECT_CLASS(smc);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        sPAPRCapabilityInfo *cap = &capability_table[i];
> > +        const char *name = g_strdup_printf("cap-%s", cap->name);
> > +
> > +        object_class_property_add(klass, name, "bool",
> > +                                  spapr_cap_get, spapr_cap_set, NULL,
> > +                                  cap, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +
> > +        object_class_property_set_description(klass, name, cap->description,
> > +                                              &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +    }
> > +}
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 14757b805e..5569caf1d4 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -50,6 +50,15 @@ typedef enum {
> >      SPAPR_RESIZE_HPT_REQUIRED,
> >  } sPAPRResizeHPT;
> >  
> > +/**
> > + * Capabilities
> > + */
> > +
> > +typedef struct sPAPRCapabilities sPAPRCapabilities;
> > +struct sPAPRCapabilities {
> > +    uint64_t mask;
> > +};
> > +
> >  /**
> >   * sPAPRMachineClass:
> >   */
> > @@ -66,6 +75,7 @@ struct sPAPRMachineClass {
> >                            hwaddr *mmio32, hwaddr *mmio64,
> >                            unsigned n_dma, uint32_t *liobns, Error **errp);
> >      sPAPRResizeHPT resize_hpt_default;
> > +    sPAPRCapabilities default_caps;
> >  };
> >  
> >  /**
> > @@ -127,6 +137,9 @@ struct sPAPRMachineState {
> >      MemoryHotplugState hotplug_memory;
> >  
> >      const char *icp_type;
> > +
> > +    sPAPRCapabilities forced_caps, forbidden_caps;
> > +    sPAPRCapabilities effective_caps;
> >  };
> >  
> >  #define H_SUCCESS         0
> > @@ -724,4 +737,22 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> >  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >  
> > +/*
> > + * Handling of optional capabilities
> > + */
> > +static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> > +{
> > +    sPAPRCapabilities caps = { mask };
> > +    return caps;
> > +}
> > +
> > +static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
> > +{
> > +    return !!(spapr->effective_caps.mask & cap);
> > +}
> > +
> > +void spapr_caps_reset(sPAPRMachineState *spapr);
> > +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
> > +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
> > +
> >  #endif /* HW_SPAPR_H */
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]