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~
