On Wed, Jan 10, 2018 at 11:19:33AM +1100, Suraj Jitindar Singh wrote:
> On Tue, 2018-01-09 at 13:07 +0100, Andrea Bolognani wrote:
> > 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.
>
> These are booleans so they have to be "on"/"off" anyway... 0/1 doesn't
> work. My bad :/ I'll fix the message.
>
> >
> > [...]
> > > -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 :)
>
> For these capabilities I think we're ok to keep it as 0/1/2. In the
> event we need a bigger range another capability can be added with other
> possible values which was the whole point of introducing this generic
> framework. The basic idea is the receiving side must always support a
> higher "level" than the source.
>
> With these new capabilities it's more likely we'll have to add an
> entirly new one than require more possible values. :)
>
> It could even be possible to have a per capability comparison function
> to confirm compatibility in future. But again thats an exercise for
> when/if more complex capabilities are added.
I concur. New capabilities which require a more detailed set of
values are reasonably likely. New values for existing capabilities
are not - to make sense of the capabilities you really need to
understand the set of all possible values when they're defined. So I
think 0..2 is ok.
--
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
