From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, paulus@ozlabs.org
Subject: Re: [Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation
Date: Fri, 12 Jan 2018 13:19:25 +1100 [thread overview]
Message-ID: <1515723565.1993.23.camel@gmail.com> (raw)
In-Reply-To: <20180110041345.GQ2131@umbus.fritz.box>
On Wed, 2018-01-10 at 15:13 +1100, David Gibson wrote:
> On Tue, Jan 09, 2018 at 08:21:01PM +1100, Suraj Jitindar Singh wrote:
> > Currently spapr_caps are tied to boolean values (on or off). This
> > patch
> > reworks the caps so that they can have any value between 0 and 127,
> > inclusive. This allows more capabilities with various values to be
> > represented in the same way internally. Capabilities are numbered
> > in
> > ascending order. The internal representation of capability values
> > is an
> > array of uint8s in the sPAPRMachineState, indexed by capability
> > number.
> > Note: The MSB (0x80) of a capability is reserved to track whether
> > the
> > capability was set from the command line.
> >
> > Capabilities can have their own name, description, options, getter
> > and
> > setter functions, type and allow functions. They also each have
> > their own
> > section in the migration stream. Capabilities are only migrated if
> > they
> > were explictly set on the command line, with the assumption that
> > otherwise the default will match.
> >
> > On migration we ensure that the capability value on the destination
> > is greater than or equal to the capability value from the source.
> > So
> > long at this remains the case then the migration is considered
> > compatible and allowed to continue.
> >
> > This patch implements generic getter and setter functions for
> > boolean
> > capabilities. It also converts the existings cap-htm, cap-vsx and
> > cap-dfp capabilities to this new format.
> > ---
> > hw/ppc/spapr.c | 19 +--
> > hw/ppc/spapr_caps.c | 335 ++++++++++++++++++++++++++++---------
> > ------------
> > include/hw/ppc/spapr.h | 41 +++---
> > 3 files changed, 222 insertions(+), 173 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d1acfe8858..7fa45729ba 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -320,7 +320,7 @@ static void
> > spapr_populate_pa_features(sPAPRMachineState *spapr,
> > */
> > pa_features[3] |= 0x20;
> > }
> > - if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> > + if (spapr_get_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
>
> I'd prefer to see explicit >0 or !=0 to emphasise that the returned
> value is now an integer not a bool.
Sure
>
> > pa_features[24] |= 0x80; /* Transactional memory
> > support */
> > }
> > if (legacy_guest && pa_size > 40) {
> > @@ -563,7 +563,7 @@ static void spapr_populate_cpu_dt(CPUState *cs,
> > void *fdt, int offset,
> > *
> > * Only CPUs for which we create core types in
> > spapr_cpu_core.c
> > * are possible, and all of those have VMX */
> > - if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > + if (spapr_get_cap(spapr, SPAPR_CAP_VSX)) {
> > _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
> > } else {
> > _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> > @@ -572,7 +572,7 @@ static void spapr_populate_cpu_dt(CPUState *cs,
> > void *fdt, int offset,
> > /* Advertise DFP (Decimal Floating Point) if available
> > * 0 / no property == no DFP
> > * 1 == DFP available */
> > - if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
> > + if (spapr_get_cap(spapr, SPAPR_CAP_DFP)) {
> > _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> > }
> >
> > @@ -1762,7 +1762,9 @@ static const VMStateDescription vmstate_spapr
> > = {
> > &vmstate_spapr_ov5_cas,
> > &vmstate_spapr_patb_entry,
> > &vmstate_spapr_pending_events,
> > - &vmstate_spapr_caps,
> > + &vmstate_spapr_cap_htm,
> > + &vmstate_spapr_cap_vsx,
> > + &vmstate_spapr_cap_dfp,
> > NULL
> > }
> > };
> > @@ -2323,8 +2325,6 @@ static void spapr_machine_init(MachineState
> > *machine)
> > char *filename;
> > Error *resize_hpt_err = NULL;
> >
> > - spapr_caps_validate(spapr, &error_fatal);
> > -
> > msi_nonbroken = true;
> >
> > QLIST_INIT(&spapr->phbs);
> > @@ -3834,7 +3834,9 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> > */
> > mc->numa_mem_align_shift = 28;
> >
> > - smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
> > + smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> > + smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> > + smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> > spapr_caps_add_properties(smc, &error_abort);
> > }
> >
> > @@ -3916,8 +3918,7 @@ static void
> > spapr_machine_2_11_class_options(MachineClass *mc)
> > sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >
> > spapr_machine_2_12_class_options(mc);
> > - smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
> > - | SPAPR_CAP_DFP);
> > + smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
> > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
> > }
> >
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 9d070a306c..af40f2e469 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -35,33 +35,71 @@
> > typedef struct sPAPRCapabilityInfo {
> > const char *name;
> > const char *description;
> > - uint64_t flag;
> > + const char *options;
> > + int index;
> >
> > + /* Getter and Setter Function Pointers */
> > + ObjectPropertyAccessor *get;
> > + ObjectPropertyAccessor *set;
> > + const char *type;
> > /* Make sure the virtual hardware can support this capability
> > */
> > - void (*allow)(sPAPRMachineState *spapr, Error **errp);
> > -
> > - /* If possible, tell the virtual hardware not to allow the cap
> > to
> > - * be used at all */
> > - void (*disallow)(sPAPRMachineState *spapr, Error **errp);
> > + void (*allow)(sPAPRMachineState *spapr, uint8_t val, Error
> > **errp);
>
> I think we need a new name for this since it can both allow and
> disallow. Maybe 'apply'?
Sure
>
> > } sPAPRCapabilityInfo;
> >
> > -static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void spapr_cap_get_bool(Object *obj, Visitor *v, const char
> > *name,
> > + void *opaque, Error **errp)
> > +{
> > + sPAPRCapabilityInfo *cap = opaque;
> > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > + bool value = spapr_get_cap(spapr, cap->index) == SPAPR_CAP_ON;
> > +
> > + visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cap_set_bool(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;
> > + }
> > +
> > + spapr->cmd_line_caps.caps[cap->index] = (value ? SPAPR_CAP_ON
> > :
> > + SPAPR_CAP_OFF
> > ) |
> > + SPAPR_CAP_CMD_LINE;
> > +}
> > +
> > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > +{
> > + if (!val) {
> > + /* TODO: We don't support disabling htm yet */
> > + return;
> > + }
>
> A downside of fusing allow and disallow is that we can't now apply
> the
> rule that failure to allow is an error but failure to disallow is
> only
> a warning in the common code. Oh well.
Yep, well it's up to the apply function now to either allow it or cause
an error if it thinks it's fatal.
>
> > if (tcg_enabled()) {
> > error_setg(errp,
> > - "No Transactional Memory support in TCG, try
> > cap-htm=off");
> > + "No Transactional Memory support in TCG, try
> > cap-htm=0");
> > } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> > error_setg(errp,
> > -"KVM implementation does not support Transactional Memory, try
> > cap-htm=off"
> > +"KVM implementation does not support Transactional Memory, try
> > cap-htm=0"
> > );
> > }
> > }
> >
> > -static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void cap_vsx_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > CPUPPCState *env = &cpu->env;
> >
> > + if (!val) {
> > + /* TODO: We don't support disabling vsx yet */
> > + return;
> > + }
> > /* Allowable CPUs in spapr_cpu_core.c should already have
> > gotten
> > * rid of anything that doesn't do VMX */
> > g_assert(env->insns_flags & PPC_ALTIVEC);
> > @@ -70,37 +108,51 @@ static void cap_vsx_allow(sPAPRMachineState
> > *spapr, Error **errp)
> > }
> > }
> >
> > -static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > CPUPPCState *env = &cpu->env;
> >
> > + if (!val) {
> > + /* TODO: We don't support disabling dfp yet */
> > + return;
> > + }
> > if (!(env->insns_flags2 & PPC2_DFP)) {
> > error_setg(errp, "DFP support not available, try cap-
> > dfp=off");
> > }
> > }
> >
> > -static sPAPRCapabilityInfo capability_table[] = {
> > - {
> > +
> > +sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > + [SPAPR_CAP_HTM] = {
> > .name = "htm",
> > .description = "Allow Hardware Transactional Memory
> > (HTM)",
> > - .flag = SPAPR_CAP_HTM,
> > + .options = "",
>
> It's not really clear to me what the options field is for.
So for string options it's for the allowed values which go in the
description.
See next patch :)
>
> > + .index = SPAPR_CAP_HTM,
> > + .get = spapr_cap_get_bool,
> > + .set = spapr_cap_set_bool,
> > + .type = "bool",
> > .allow = cap_htm_allow,
> > - /* TODO: add cap_htm_disallow */
> > },
> > - {
> > + [SPAPR_CAP_VSX] = {
> > .name = "vsx",
> > .description = "Allow Vector Scalar Extensions (VSX)",
> > - .flag = SPAPR_CAP_VSX,
> > + .options = "",
> > + .index = SPAPR_CAP_VSX,
> > + .get = spapr_cap_get_bool,
> > + .set = spapr_cap_set_bool,
> > + .type = "bool",
> > .allow = cap_vsx_allow,
> > - /* TODO: add cap_vsx_disallow */
> > },
> > - {
> > + [SPAPR_CAP_DFP] = {
> > .name = "dfp",
> > .description = "Allow Decimal Floating Point (DFP)",
> > - .flag = SPAPR_CAP_DFP,
> > + .options = "",
> > + .index = SPAPR_CAP_DFP,
> > + .get = spapr_cap_get_bool,
> > + .set = spapr_cap_set_bool,
> > + .type = "bool",
> > .allow = cap_dfp_allow,
> > - /* TODO: add cap_dfp_disallow */
> > },
> > };
> >
> > @@ -115,201 +167,192 @@ static sPAPRCapabilities
> > default_caps_with_cpu(sPAPRMachineState *spapr,
> >
> > if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
> > 0, spapr->max_compat_pvr)) {
> > - caps.mask &= ~SPAPR_CAP_HTM;
> > + caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> > }
> >
> > if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
> > 0, spapr->max_compat_pvr)) {
> > - caps.mask &= ~SPAPR_CAP_VSX;
> > - caps.mask &= ~SPAPR_CAP_DFP;
> > + caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;
> > + caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_OFF;
> > }
> >
> > return caps;
> > }
> >
> > -static bool spapr_caps_needed(void *opaque)
> > -{
> > - sPAPRMachineState *spapr = opaque;
> > -
> > - return (spapr->forced_caps.mask != 0) || (spapr-
> > >forbidden_caps.mask != 0);
> > -}
> > -
> > /* This has to be called from the top-level spapr post_load, not
> > the
> > * caps specific one. Otherwise it wouldn't be called when the
> > source
> > * caps are all defaults, which could still conflict with
> > overridden
> > * caps on the destination */
> > int spapr_caps_post_migration(sPAPRMachineState *spapr)
> > {
> > - uint64_t allcaps = 0;
> > int i;
> > bool ok = true;
> > sPAPRCapabilities dstcaps = spapr->effective_caps;
> > sPAPRCapabilities srccaps;
> >
> > srccaps = default_caps_with_cpu(spapr, first_cpu);
> > - srccaps.mask |= spapr->mig_forced_caps.mask;
> > - srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > + for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > + if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > + srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > + }
> > + }
> >
> > - for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > + for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > sPAPRCapabilityInfo *info = &capability_table[i];
> >
> > - allcaps |= info->flag;
> > -
> > - if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > >flag)) {
> > - error_report("cap-%s=on in incoming stream, but off in
> > destination",
> > - info->name);
> > + if (srccaps.caps[i] > dstcaps.caps[i]) {
> > + error_report("cap-%s higher level (%d) in incoming
> > stream than on destination (%d)",
> > + info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> > ok = false;
> > }
> >
> > - if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > >flag)) {
> > - warn_report("cap-%s=off in incoming stream, but on in
> > destination",
> > - info->name);
> > + if (srccaps.caps[i] < dstcaps.caps[i]) {
> > + warn_report("cap-%s lower level (%d) in incoming
> > stream than on destination (%d)",
> > + info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> > }
> > }
> >
> > - if (spapr->mig_forced_caps.mask & ~allcaps) {
> > - error_report(
> > - "Unknown capabilities 0x%"PRIx64" enabled in incoming
> > stream",
> > - spapr->mig_forced_caps.mask & ~allcaps);
> > - ok = false;
> > - }
> > - if (spapr->mig_forbidden_caps.mask & ~allcaps) {
> > - warn_report(
> > - "Unknown capabilities 0x%"PRIx64" disabled in incoming
> > stream",
> > - spapr->mig_forbidden_caps.mask & ~allcaps);
> > - }
> > -
> > return ok ? 0 : -EINVAL;
> > }
> >
> > -static int spapr_caps_pre_save(void *opaque)
> > +static bool spapr_cap_htm_needed(void *opaque)
> > {
> > sPAPRMachineState *spapr = opaque;
> >
> > - spapr->mig_forced_caps = spapr->forced_caps;
> > - spapr->mig_forbidden_caps = spapr->forbidden_caps;
> > + return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_HTM] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> > +
> > +static int spapr_cap_htm_pre_save(void *opaque)
>
> Having to have a separate needed, pre_save and pre_load for each cap
> will be a pain. I hope we can find a way to do this in common.
As discussed on IRC
>
> > +{
> > + sPAPRMachineState *spapr = opaque;
> > +
> > + spapr->mig_caps.caps[SPAPR_CAP_HTM] =
> > + spapr->cmd_line_caps.caps[SPAPR_CAP_HTM];
> > return 0;
> > }
> >
> > -static int spapr_caps_pre_load(void *opaque)
> > +static int spapr_cap_htm_pre_load(void *opaque)
> > {
> > sPAPRMachineState *spapr = opaque;
> >
> > - spapr->mig_forced_caps = spapr_caps(0);
> > - spapr->mig_forbidden_caps = spapr_caps(0);
> > + spapr->mig_caps.caps[SPAPR_CAP_HTM] = 0;
> > return 0;
> > }
> >
> > -const VMStateDescription vmstate_spapr_caps = {
> > - .name = "spapr/caps",
> > +const VMStateDescription vmstate_spapr_cap_htm = {
> > + .name = "spapr/cap_htm",
>
> I'd suggest spapr/caps/htm - and a common vmstate_spapr_caps to hold
> the subsections for convenience rather than putting them straight
> under the master spapr one.
As discussed on IRC
>
> > .version_id = 1,
> > .minimum_version_id = 1,
> > - .needed = spapr_caps_needed,
> > - .pre_save = spapr_caps_pre_save,
> > - .pre_load = spapr_caps_pre_load,
> > + .needed = spapr_cap_htm_needed,
> > + .pre_save = spapr_cap_htm_pre_save,
> > + .pre_load = spapr_cap_htm_pre_load,
> > .fields = (VMStateField[]) {
> > - VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
> > - VMSTATE_UINT64(mig_forbidden_caps.mask,
> > sPAPRMachineState),
> > + VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_HTM],
> > sPAPRMachineState),
> > VMSTATE_END_OF_LIST()
> > },
> > };
> >
> > -void spapr_caps_reset(sPAPRMachineState *spapr)
> > +static bool spapr_cap_vsx_needed(void *opaque)
> > {
> > - Error *local_err = NULL;
> > - sPAPRCapabilities caps;
> > - int i;
> > + sPAPRMachineState *spapr = opaque;
> >
> > - /* First compute the actual set of caps we're running with..
> > */
> > - caps = default_caps_with_cpu(spapr, first_cpu);
> > + return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_VSX] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> >
> > - /* Remove unnecessary forced/forbidden bits (this will help us
> > - * with migration) */
> > - spapr->forced_caps.mask &= ~caps.mask;
> > - spapr->forbidden_caps.mask &= caps.mask;
>
> I don't think you have an equivalent of this in the new code, which
> means that an explicitly set property could prevent migration, even
> if
> it actually has the default value.
As discussed on IRC
>
> > +static int spapr_cap_vsx_pre_save(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> > +
> > + spapr->mig_caps.caps[SPAPR_CAP_VSX] =
> > + spapr->cmd_line_caps.caps[SPAPR_CAP_VSX];
> > + return 0;
> > +}
> >
> > - caps.mask |= spapr->forced_caps.mask;
> > - caps.mask &= ~spapr->forbidden_caps.mask;
> > +static int spapr_cap_vsx_pre_load(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> >
> > - spapr->effective_caps = caps;
> > + spapr->mig_caps.caps[SPAPR_CAP_VSX] = 0;
> > + return 0;
> > +}
> >
> > - /* .. then apply those caps to the virtual hardware */
> > +const VMStateDescription vmstate_spapr_cap_vsx = {
> > + .name = "spapr/cap_vsx",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = spapr_cap_vsx_needed,
> > + .pre_save = spapr_cap_vsx_pre_save,
> > + .pre_load = spapr_cap_vsx_pre_load,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_VSX],
> > sPAPRMachineState),
> > + VMSTATE_END_OF_LIST()
> > + },
> > +};
> >
> > - for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > - sPAPRCapabilityInfo *info = &capability_table[i];
> > +static bool spapr_cap_dfp_needed(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = opaque;
> >
> > - 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);
> > - local_err = NULL;
> > - }
> > - }
> > - }
> > + return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_DFP] &
> > SPAPR_CAP_CMD_LINE);
> > }
> >
> > -static void spapr_cap_get(Object *obj, Visitor *v, const char
> > *name,
> > - void *opaque, Error **errp)
> > +static int spapr_cap_dfp_pre_save(void *opaque)
> > {
> > - 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()? */
> > + sPAPRMachineState *spapr = opaque;
> >
> > - visit_type_bool(v, name, &value, errp);
> > + spapr->mig_caps.caps[SPAPR_CAP_DFP] =
> > + spapr->cmd_line_caps.caps[SPAPR_CAP_DFP];
> > + return 0;
> > }
> >
> > -static void spapr_cap_set(Object *obj, Visitor *v, const char
> > *name,
> > - void *opaque, Error **errp)
> > +static int spapr_cap_dfp_pre_load(void *opaque)
> > {
> > - 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;
> > - }
> > + sPAPRMachineState *spapr = opaque;
> >
> > - if (value) {
> > - spapr->forced_caps.mask |= cap->flag;
> > - } else {
> > - spapr->forbidden_caps.mask |= cap->flag;
> > - }
> > + spapr->mig_caps.caps[SPAPR_CAP_DFP] = 0;
> > + return 0;
> > }
> >
> > -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> > +const VMStateDescription vmstate_spapr_cap_dfp = {
> > + .name = "spapr/cap_dfp",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = spapr_cap_dfp_needed,
> > + .pre_save = spapr_cap_dfp_pre_save,
> > + .pre_load = spapr_cap_dfp_pre_load,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_DFP],
> > sPAPRMachineState),
> > + VMSTATE_END_OF_LIST()
> > + },
> > +};
> > +
> > +void spapr_caps_reset(sPAPRMachineState *spapr)
> > {
> > - uint64_t allcaps = 0;
> > + sPAPRCapabilities caps;
> > int i;
> >
> > - for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > - g_assert((allcaps & capability_table[i].flag) == 0);
> > - allcaps |= capability_table[i].flag;
> > + /* First compute the actual set of caps we're running with..
> > */
> > + caps = default_caps_with_cpu(spapr, first_cpu);
> > +
> > + for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > + /* Check if set from command line and override default if
> > so */
> > + if (spapr->cmd_line_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > + caps.caps[i] = spapr->cmd_line_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > + }
> > }
> >
> > - g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> > - g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> > + spapr->effective_caps = caps;
> >
> > - if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> > - error_setg(errp, "Some sPAPR capabilities set both on and
> > off");
> > - return;
> > + /* .. then apply those caps to the virtual hardware */
> > +
> > + for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > + sPAPRCapabilityInfo *info = &capability_table[i];
> > +
> > + /*
> > + * If the allow function can't set the desired level and
> > think's it's
>
> Nit, s/think's/thinks/
yep
>
> > + * fatal, it should cause that.
> > + */
> > + info->allow(spapr, spapr->effective_caps.caps[i],
> > &error_fatal);
> > }
> > }
> >
> > @@ -322,17 +365,19 @@ void
> > spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> > for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > sPAPRCapabilityInfo *cap = &capability_table[i];
> > const char *name = g_strdup_printf("cap-%s", cap->name);
> > + char *desc;
> >
> > - object_class_property_add(klass, name, "bool",
> > - spapr_cap_get, spapr_cap_set,
> > NULL,
> > - cap, &local_err);
> > + object_class_property_add(klass, name, cap->type,
> > + cap->get, 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);
> > + desc = g_strdup_printf("%s%s", cap->description, cap-
> > >options);
> > + object_class_property_set_description(klass, name, desc,
> > &local_err);
> > + g_free(desc);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 26ac17e641..2804fbbf12 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -57,17 +57,26 @@ typedef enum {
> > /* These bits go in the migration stream, so they can't be
> > reassigned */
> >
> > /* Hardware Transactional Memory */
> > -#define SPAPR_CAP_HTM 0x0000000000000001ULL
> > -
> > +#define SPAPR_CAP_HTM 0x00
> > /* Vector Scalar Extensions */
> > -#define SPAPR_CAP_VSX 0x0000000000000002ULL
> > -
> > +#define SPAPR_CAP_VSX 0x01
> > /* Decimal Floating Point */
> > -#define SPAPR_CAP_DFP 0x0000000000000004ULL
> > +#define SPAPR_CAP_DFP 0x02
> > +/* Num Caps */
> > +#define SPAPR_CAP_NUM (SPAPR_CAP_DFP + 1)
> > +
> > +/*
> > + * Capability Values
> > + * NOTE: All execpt the immediately following MUST be less than
> > 128 (0x80)
> > + */
> > +#define SPAPR_CAP_CMD_LINE 0x80
> > +/* Bool Caps */
> > +#define SPAPR_CAP_OFF 0x00
> > +#define SPAPR_CAP_ON 0x01
> >
> > typedef struct sPAPRCapabilities sPAPRCapabilities;
> > struct sPAPRCapabilities {
> > - uint64_t mask;
> > + uint8_t caps[SPAPR_CAP_NUM];
> > };
> >
> > /**
> > @@ -149,9 +158,8 @@ struct sPAPRMachineState {
> >
> > const char *icp_type;
> >
> > - sPAPRCapabilities forced_caps, forbidden_caps;
> > - sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
> > - sPAPRCapabilities effective_caps;
> > + sPAPRCapabilities cmd_line_caps, effective_caps;
> > + sPAPRCapabilities mig_caps;
> > };
> >
> > #define H_SUCCESS 0
> > @@ -752,21 +760,16 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr,
> > int irq);
> > /*
> > * Handling of optional capabilities
> > */
> > -extern const VMStateDescription vmstate_spapr_caps;
> > -
> > -static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> > -{
> > - sPAPRCapabilities caps = { mask };
> > - return caps;
> > -}
> > +extern const VMStateDescription vmstate_spapr_cap_htm;
> > +extern const VMStateDescription vmstate_spapr_cap_vsx;
> > +extern const VMStateDescription vmstate_spapr_cap_dfp;
> >
> > -static inline bool spapr_has_cap(sPAPRMachineState *spapr,
> > uint64_t cap)
> > +static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int
> > cap)
> > {
> > - return !!(spapr->effective_caps.mask & cap);
> > + return spapr->effective_caps.caps[cap];
> > }
> >
> > void spapr_caps_reset(sPAPRMachineState *spapr);
> > -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
> > void spapr_caps_add_properties(sPAPRMachineClass *smc, Error
> > **errp);
> > int spapr_caps_post_migration(sPAPRMachineState *spapr);
> >
>
>
next prev parent reply other threads:[~2018-01-12 2:19 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-09 9:21 [Qemu-devel] [QEMU-PPC] [RFC 0/3] target/ppc: Rework spapr_caps Suraj Jitindar Singh
2018-01-09 9:21 ` [Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation Suraj Jitindar Singh
2018-01-09 11:13 ` [Qemu-devel] [Qemu-ppc] " Murilo Opsfelder Araújo
2018-01-10 0:21 ` Suraj Jitindar Singh
2018-01-09 12:07 ` Andrea Bolognani
2018-01-10 0:19 ` Suraj Jitindar Singh
2018-01-10 2:51 ` David Gibson
2018-01-10 4:13 ` [Qemu-devel] " David Gibson
2018-01-12 2:19 ` Suraj Jitindar Singh [this message]
2018-01-09 9:21 ` [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch] Suraj Jitindar Singh
2018-01-09 11:15 ` [Qemu-devel] [Qemu-ppc] " Murilo Opsfelder Araújo
2018-01-10 0:25 ` Suraj Jitindar Singh
2018-01-09 12:02 ` [Qemu-devel] " joserz
2018-01-10 0:23 ` Suraj Jitindar Singh
2018-01-10 4:54 ` David Gibson
2018-01-09 9:21 ` [Qemu-devel] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS Suraj Jitindar Singh
2018-01-09 11:19 ` [Qemu-devel] [Qemu-ppc] " Murilo Opsfelder Araújo
2018-01-10 0:26 ` Suraj Jitindar Singh
2018-01-10 5:02 ` [Qemu-devel] " David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1515723565.1993.23.camel@gmail.com \
--to=sjitindarsingh@gmail.com \
--cc=david@gibson.dropbear.id.au \
--cc=paulus@ozlabs.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.