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

[David Hildenbrand <david@redhat.com>]

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).


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.

-- 
Cheers,

David / dhildenb