[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps
|
From: |
David Gibson |
|
Subject: |
Re: [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation |
|
Date: |
Wed, 10 Jan 2018 15:13:45 +1100 |
|
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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.
> 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'?
> } 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.
> 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.
> + .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.
> +{
> + 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.
> .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.
> +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/
> + * 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);
>
--
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