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
