On Mon, Dec 18, 2017 at 10:58:00AM +0100, Greg Kurz wrote:
> On Mon, 18 Dec 2017 20:20:19 +1100
> David Gibson <da...@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 <gr...@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
signature.asc
Description: PGP signature
