Re: [Qemu-ppc] [RFC for-2.12 2/8] spapr: Capabilities infrastructure

[David Gibson]

On Tue, Dec 12, 2017 at 11:44:34AM +1100, Suraj Jitindar Singh wrote:
> On Mon, 2017-12-11 at 18:08 +1100, David Gibson wrote:
> > Because PAPR is a paravirtual environment access to certain CPU (or
> > other)
> > facilities can be blocked by the hypervisor.  PAPR provides ways to
> > advertise in the device tree whether or not those features are
> > available to
> > the guest.
> > 
> > In some places we automatically determine whether to make a feature
> > available based on whether our host can support it, in most cases
> > this is
> > based on limitations in the available KVM implementation.
> > 
> > Although we correctly advertise this to the guest, it means that host
> > factors might make changes to the guest visible environment which is
> > bad:
> > as well as generaly reducing reproducibility, it means that a 
> s/generaly/generally

Ta.

> > migration
> > between different host environments can easily go bad.
> > 
> > We've mostly gotten away with it because the environments considered
> > mature
> > enough to be well supported (basically, KVM on POWER8) have had
> > consistent
> > feature availability.  But, it's still not right and some limitations
> > on
> > POWER9 is going to make it more of an issue in future.
> > 
> > This introduces an infrastructure for defining "sPAPR
> > capabilities".  These
> > are set by default based on the machine version, masked by the
> > capabilities
> > of the chosen cpu, but can be overriden with machine properties.
> > 
> > The intention is at reset time we verify that the requested
> > capabilities
> > can be supported on the host (considering TCG, KVM and/or host cpu
> > limitations).  If not we simply fail, rather than silently modifying
> > the
> > advertised featureset to the guest.
> > 
> > This does mean that certain configurations that "worked" may now
> > fail, but
> > such configurations were already more subtly broken.
> 
> Overall looks good. Minor nits below.
> 
> Suraj
> 
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/Makefile.objs   |   2 +-
> >  hw/ppc/spapr.c         |   7 ++
> >  hw/ppc/spapr_caps.c    | 170
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  31 +++++++++
> >  4 files changed, 209 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/ppc/spapr_caps.c
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index 7efc686748..1faff853b7 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -1,7 +1,7 @@
> >  # shared objects
> >  obj-y += ppc.o ppc_booke.o fdt.o
> >  # IBM pSeries (sPAPR)
> > -obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > +obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o
> > spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > spapr_rng.o
> >  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 42d6a2302a..a921feeb03 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1467,6 +1467,8 @@ static void spapr_machine_reset(void)
> >      /* Check for unknown sysbus devices */
> >      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> >  
> > +    spapr_caps_reset(spapr);
> > +
> >      if (kvm_enabled() && kvmppc_has_cap_mmu_radix()) {
> >          /* If using KVM with radix mode available, VCPUs can be
> > started
> >           * without a HPT because KVM will start them in radix mode.
> > @@ -2310,6 +2312,8 @@ static void spapr_machine_init(MachineState
> > *machine)
> >      char *filename;
> >      Error *resize_hpt_err = NULL;
> >  
> > +    spapr_validate_caps(spapr, &error_fatal);
> > +
> >      msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> > @@ -3818,6 +3822,9 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> >       * in which LMBs are represented and hot-added
> >       */
> >      mc->numa_mem_align_shift = 28;
> > +
> > +    smc->default_caps = spapr_caps(0);
> > +    spapr_capability_properties(smc, &error_abort);
> >  }
> >  
> >  static const TypeInfo spapr_machine_info = {
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > new file mode 100644
> > index 0000000000..f1721cc68f
> > --- /dev/null
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * QEMU PowerPC pSeries Logical Partition capabilities handling
> > + *
> > + * Copyright (c) 2017 David Gibson, Red Hat Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a copy
> > + * of this software and associated documentation files (the
> > "Software"), to deal
> > + * in the Software without restriction, including without limitation
> > the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense,
> > and/or sell
> > + * copies of the Software, and to permit persons to whom the
> > Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> > SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> > OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qapi/visitor.h"
> > +
> > +#include "hw/ppc/spapr.h"
> > +
> > +typedef struct sPAPRCapabilityInfo {
> > +    const char *name;
> > +    const char *description;
> > +    uint64_t bit;
> > +} sPAPRCapabilityInfo;
> > +
> > +static sPAPRCapabilityInfo capability_table[] = {
> > +};
> > +
> > +static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState
> > *spapr, CPUState *cs)
> > +{
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > +    sPAPRCapabilities caps;
> > +
> > +    caps = smc->default_caps;
> > +
> > +    /* TODO: clamp according to cpu model */
> > +
> > +    return caps;
> > +}
> > +
> > +static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
> 
> Since this is essentially enabling the caps,
> s/spapr_allow_caps/spapr_caps_enable

Yeah.. I don't like enable/disable here because that suggests it
changes the state of the cap.  At this point the final value of the
caps is already decided, this is just making the virtual hardware
behave according to those selected caps.

allow/enforce was the best I came up with.  I quite like "apply_caps"
too, but I also wanted to split the +/- parts since one is fatal and
the other isn't.

> > +{
> > +    /* TODO: make sure all requested caps are allowed with the
> > current
> > +     * accelerator, cpu etc. */
> > +}
> > +
> > +static void spapr_enforce_caps(sPAPRMachineState *spapr, Error
> > **errp)
> 
> Again, this is disabling caps,
> s/spapr_enforce_caps/spapr_caps_disable
> 
> > +{
> > +    /* TODO: to the extent possible, prevent the guest from
> > accessing
> > +     * features controlled by disabled caps */
> > +}
> > +
> > +void spapr_caps_reset(sPAPRMachineState *spapr)
> > +{
> > +    Error *err = NULL;
> > +    sPAPRCapabilities caps;
> > +
> > +    caps = default_caps_with_cpu(spapr, first_cpu);
> > +
> > +    caps.mask |= spapr->forced_caps.mask;
> > +    caps.mask &= ~spapr->forbidden_caps.mask;
> > +
> > +    spapr->effective_caps = caps;
> > +
> > +    spapr_allow_caps(spapr, &err);
> > +    if (err) {
> > +        /* Can't support a requested capability, fatal */
> > +        error_report_err(err);
> > +        exit(1);
> > +    }
> > +    spapr_enforce_caps(spapr, &err);
> > +    if (err) {
> > +        /* Can't enforce a disabled capability, warning only */
> > +        error_report_err(err);
> > +    }
> > +}
> > +
> > +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->bit);
> > +
> > +    /* 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->bit;
> > +    } else {
> > +        spapr->forbidden_caps.mask |= cap->bit;
> > +    }
> > +}
> > +
> > +void spapr_validate_caps(sPAPRMachineState *spapr, Error **errp)
> 
> Just for naming consistency,
> s/spapr_validate_caps/spaps_caps_validate

Good idea.

> > +{
> > +    Error *local_err = NULL;
> > +    uint64_t allcaps = 0;
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        g_assert((allcaps & capability_table[i].bit) == 0);
> > +        allcaps |= capability_table[i].bit;
> > +    }
> > +
> > +    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(&local_err, "Some sPAPR capabilities set both on
> > and off");
> > +        return;
> > +    }
> > +
> > +    /* Check for any caps incompatible with other caps.  Nothing to
> > do
> > +     * yet */
> > +}
> > +
> > +void spapr_capability_properties(sPAPRMachineClass *smc, Error
> > **errp)
> 
> s/spapr_capability_properties/spapr_caps_register_properties?

Good idea.

> > +{
> > +    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..fffe10ee72 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;
> 
> I'm not sure about "forced" and "forbidden",
> how about enabled/disabled/effective or
> required/disabled/effective?

forced/forbidden are not the same as enabled/disabled though: they
only contain properties which have been overridden from the defaults
(which becomes important on migration).

> Since the "forced_caps" have been "enabled" with the HV, and the
> "forbidden_caps" disabled with the HV.
> 
> >  };
> >  
> >  #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_validate_caps(sPAPRMachineState *spapr, Error **errp);
> > +void spapr_capability_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