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

[Richard Henderson <richard.henderson@linaro.org>]

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?

The actual code looks good.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~