Re: [PATCH v2] qdict: Preserve order for iterating qdict elements

[Daniel P . Berrangé]

On Wed, Sep 06, 2023 at 12:25:30PM +0200, David Hildenbrand wrote:
> On 04.09.23 18:38, Daniel P. Berrangé wrote:
> > On Sat, Sep 02, 2023 at 05:40:40PM +0800, William Tsai wrote:
> > > Changing the structure of qdict so that it can preserve order when
> > > iterating qdict. This will fix array_properties as it relies on `len-`
> > > prefixed argument to be set first.
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
> > > Signed-off-by: William Tsai <williamtsai1111@gmail.com>
> > 
> > This is a variation of what Markus illustrated a year ago
> > 
> >    https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> > 
> > I wasn't a particular fan of that approach at the time.
> > 
> > I've made an alternative proposal here which avoids the broader
> > impact of this QDict change:
> > 
> >    https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00652.html
> 
> Just a note regarding s390x CPU models (and how they are also affected, but
> it probably doesn't matter because it never 100% worked that way on all 
> interfaces).

Oh, that's interesting to see a ordering scenario that isn't related
to the PROP_ARRAY stuff we already identified.

> I recall that I thought the order of parameters worked for s390x CPU models,
> where we support feature groups (due to the huge number of CPU features). But 
> this
> might only have worked for the "-cpu" parameter, which parses them in-order 
> and
> sets properties in-order.
> 
> So when mixing a feature group with contained features, the end result might 
> not
> be deterministic in other cases thatn "-pu" (CPU hotplug via "-device", but
> also qapi CPU model operations).
> 
> For example, one might want to enable all but some features of a group, or
> disable all but some features of a group. Note that I doubt that there are 
> really
> users of that, but it's possible on the QEMU cmdline.
> 
> I guess it never really worked with qapi CPU model operations in general
> (baseline, comparison, expansion, ...) because these
> operations all rely on qdict as well (see cpu_model_from_info()). So they 
> should
> never return something non-deterministic.
> 
> 
> To highlight one case that could now fail:
> 
> $ ./qemu-system-s390x -smp 1,maxcpus=2 -cpu qemu,msa2=off,kimd-sha-512=on 
> -nographic -nodefaults -monitor stdio -S -device 
> qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on
> QEMU 8.1.50 monitor - type 'help' for more information
> qemu-system-s390x: warning: 'msa5-base' requires 'klmd-sha-512'.
> qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: 
> warning: 'msa5-base' requires 'kimd-sha-512'.
> qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: 
> warning: 'msa5-base' requires 'klmd-sha-512'.
> qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: 
> Mixed CPU models are not supported on s390x.
> 
> Note that using "-device qemu-s390x-cpu,core-id=1" instead works as expected, 
> as
> cpu_common_parse_features() registers all settings as global properties for
> that CPU type.
> 
> 
> Further, feature groups might not be used by actual users that way. CPU model 
> expansion (s390_feat_bitmap_to_ascii()) only reports a feature group when all
> features are contained, so most of libvirt should be fine, unless someone 
> decides to
> manually specify a non-deterministic CPU model as above.
> 
> So maybe one can conclude that specifying "msa2=off,kimd-sha-512=on" is 
> similar to
> "kimd-sha-512=off,kimd-sha-512=on", and which setting "wins" is not 
> deterministic.

Right now you can have arbitrary ordering of CPU feature groups and
individual features, with both on/off. Historically you could even have
the same feature name repeated with different on/off values and the last
occurrance would "win". This is incredibly flexible, but I think we could
argue this level of flexibility is overkill in practice for 99% of QEMU
deployments.

If we were to define two semantic rules for CPU specification:

  * All feature groups are processed before individiual features
  * A given feature group name or individual feature name may
    only appear once.

That would allow you to provide an implementation whose semantics
are not sensitive to the ordering of parameters, while still keeping
sufficient flexibility for all typical real world use cases.

The only thing I see lacking here is that it prevents users from doing
a "quick hack" of blindly appending  "foo=off" to an existing -cpu
argument - they would have to first check if 'foo' already appeared
in the existing -cpu arg and modify it in place. IMHO that's an
acceptable loss

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|