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

[Suraj Jitindar Singh]

On Tue, 2017-12-12 at 14:27 +1100, David Gibson wrote:
> 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.

True, fine as is then

> 
> > > +{
> > > +    /* 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).

Yep, makes more sense having read the next patch

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