Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Andrea Bolognani]

On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
[...]
> +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error 
> **errp)
> +{
> +    if (!val) {
> +        /* TODO: We don't support disabling htm yet */
> +        return;
> +    }
>      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"
>              );
>      }
>  }

Changing the command-line interface from off/on to 0/1 seems
unnecessary, given that broken/workaround/fixed are used for the
capabilities you introduce later in the series. off/on look much
better IMHO.

[...]
> -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]);
>          }
>      }

These numeric comparisons make me feel very uneasy :)

What if we need to add more possible values down the line? Should
there be at least some room between existing values to avoid painting
ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...

You clearly know more about the problem than I do, so feel free to
dismiss all of the above... I thought I would bring up my worries
just in case :)

-- 
Andrea Bolognani / Red Hat / Virtualization