Re: [Qemu-devel] [PATCH v3 03/15] target/arm/monitor: Introduce qmp_query_cpu_model_expansion

[Richard Henderson]

On 8/2/19 9:27 AM, Richard Henderson wrote:
> On 8/2/19 5:25 AM, Andrew Jones wrote:
>> Note, certainly more features may be added to the list of
>> advertised features, e.g. 'vfp' and 'neon'. The only requirement
>> is that their property set accessors fail when invalid
>> configurations are detected. For vfp we would need something like
>>
>>  set_vfp()
>>  {
>>    if (arm_feature(env, ARM_FEATURE_AARCH64) &&
>>        cpu->has_vfp != cpu->has_neon)
>>        error("AArch64 CPUs must have both VFP and Neon or neither")
>>
>> in its set accessor, and the same for neon, rather than doing that
>> check at realize time, which isn't executed at qmp query time.
> 
> How could this succeed?  Either set_vfp or set_neon would be called first, at
> which point the two features are temporarily out of sync, but the error would
> trigger anyway.
> 
> This would seem to require some separate "qmp validate" step that is processed
> after a collection of properties are set.
> 
> I was about to say something about this being moot until someone actually 
> wants
> to be able to disable vfp+neon on aarch64, but then...
> 
>> +A note about CPU feature dependencies
>> +-------------------------------------
>> +
>> +It's possible for features to have dependencies on other features. I.e.
>> +it may be possible to change one feature at a time without error, but
>> +when attempting to change all features at once an error could occur
>> +depending on the order they are processed.  It's also possible changing
>> +all at once doesn't generate an error, because a feature's dependencies
>> +are satisfied with other features, but the same feature cannot be changed
>> +independently without error.  For these reasons callers should always
>> +attempt to make their desired changes all at once in order to ensure the
>> +collection is valid.
> 
> ... this language makes me think that you've already encountered an ordering
> problem that might be better solved with a separate validate step?

It appears to me that we can handle both use cases with a single function to
handle validation of the cross-dependent properties.

It would need to be called at the beginning of arm_cpu_realizefn, for the case
in which we are building a cpu that we wish to instantiate, and

> +        if (!err) {
> +            visit_check_struct(visitor, &err);
> +        }

here, inside qmp_query_cpu_model_expansion for the query case.

Looking at the validation code scattered across multiple functions, across 4
patches, convinces me that the code will be smaller and more readable if we
consolidate them into a single validation function.


r~