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

[Andrew Jones]

On Fri, Aug 02, 2019 at 06:28:51PM -0700, Richard Henderson wrote:
> 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.
>

That's a reasonable suggestion. I do like having self-contained
validation, self-contained, but when cross-dependencies arise, then
it does make sense to have a master validation function, rather
than interconnecting too much. That said, for this series we only
enable the qmp query for aarch64, pmu, and sve* properties. aarch64
and pmu are independent, and thus self-contained, and I consider
all sve* properties one big entity, so their validation is also
self-contained. If we add vfp and neon, then indeed I was wrong
with my suggestion in the commit message. They would need a later
validation check. Should we just cross that bridge when we get there
though? Or would you like me to see how that would work within this
series?

Thanks,
drew