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

[Andrew Jones]

On Thu, Aug 08, 2019 at 11:37:01AM -0700, Richard Henderson wrote:
> On 8/8/19 1:50 AM, Andrew Jones wrote:
> > I'm not sure. Of course I'd need to experiment with it to be sure, but
> > I'm reluctant to go through that exercise, because I believe that a
> > deferred validation will result in less specific errors messages. For
> > example, how would the validator know in which order the sve<N> properties
> > were provided? Which is necessary to complain that one length is not
> > allowed when another has already been disabled, or vice versa.
> 
> My point is that we would not *need* to know in which order the properties are
> provided, and do not care.  Indeed, removing this ordering malarkey is a
> feature not a bug.
> 
> The error message would simply state, e.g., that sve256 may not be disabled
> while sve512 is enabled.  The error message does not need to reference the
> order in which they appeared.

OK, I agree it doesn't make much difference to the user whether the error
hint is the generic "sve256 required with sve512" verse

 sve256=off,sve512=on  "cannot enable sve512 with sve256 disabled"

or

 sve512=on,sve256=off  "cannot disable sve256 with sve512 enabled"

> 
> > Also with deferred validation I think I'd need more vq states, at least
> > for when KVM is enabled. For example, if 384-bit vector lengths are not
> > supported on the KVM host, but the user does sve384=on, and all we do
> > is record that, then we've lost the KVM unsupported information and won't
> > find out until we attempt to enable it later at kvm vcpu init time. We'd
> > need a kvm-unsupported-user-enabled state to track that, which also means
> > we're not blindly recording user input in cpu_arm_set_sve_vq() anymore,
> > but are instead applying logic to decide which state we transition to.
> 
> Or perhaps, less vq states.  You do not need to compute kvm states early.  You
> can wait to collect those until validation time and keep them in their own
> local bitmap.
> 
> bool validate_sve_properties(...)
> {
>     // Populate uninitialized bits in sve_vq_map from sve_max_vq.
>     // Populate uninitialized bits in sve_vq_map from on bits in sve_vq_map.

And disable uninitialized bits in sve_vq_map from OFF bits: auto-disabling

Also we can't do these populate uninitialized bits from on/off steps when
KVM is enabled without first getting the kvm-supported map.

>     // All remaining uninitialized bits are set to off.
>     // Reset sve_max_vq to the maximum bit set in sve_vq_map, plus 1.

I wouldn't always do this. If the user explicitly sets a maximum with
sve-max-vq, then we should generate errors when other inputs would change
that maximum. I would only assert they're the same when both input types
are provided. Of course we do the above when only map input is provided
though.

>     // Diagnose off bits in sve_vq_map from on bits in sve_vq_map.
> 
>     if (kvm_enabled()) {
>         DECLARE_BITMAP(test, ARM_MAX_VQ);
>         kvm_arm_sve_get_vls(CPU(cpu), test);
>         if (!bitmap_equal(test, s->sve_vq_map, s->sve_max_vq))  {
>             bitmap_xor(test, test, s->sve_vq_map, s->sve_max_vq);
>             // Diagnose the differences in TEST:
>             // test[i] & s->sve_vq_map[i] -> i is unsupported by kvm
>             // test[i] & !s->sve_vq_map[i] -> i is required by kvm
>         }
>     }
> }
> 
> If you prefer not to experiment with this, then I will.
>

Ah, the ol' I'll do it if you don't motivator... I do see some
potential for a reduction in complexity with this approach, but
I'm not sure we'll save many lines of code. I could do a quick
hack on top of this series, just to see how well the validator
function looks and works, but I can't get to it until midweek
next week. I won't complain if you beat me to it :-)

Thanks,
drew