Re: [Qemu-devel] How do we do user input bitmap properties?

[Markus Armbruster]

Andrew Jones <address@hidden> writes:

> On Tue, May 14, 2019 at 06:54:03AM +0200, Markus Armbruster wrote:
>> Andrew Jones <address@hidden> writes:
>> 
>> > On Thu, Apr 18, 2019 at 07:48:09PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <address@hidden> writes:
>> >> 
>> >> > On Thu, Apr 18, 2019 at 11:28:41AM +0200, Andrew Jones wrote:
>> >> >> Hi all,
>> >> >> 
>> >> >> First some background:
>> >> >> 
>> >> >> For the userspace side of AArch64 guest SVE support we need to
>> >> >> expose KVM's allowed vector lengths bitmap to the user and allow
>> >> >> the user to choose a subset of that bitmap. Since bitmaps are a
>> >> >> bit awkward to work with then we'll likely want to expose it as
>> >> >> an array of vector lengths instead. Also, assuming we want to
>> >> >> expose the lengths as number-of-quadwords (quadword == 128 bits
>> >> >> for AArch64 and vector lengths must be multiples of quadwords)
>> >> >> rather than number-of-bits, then an example array (which will
>> >> >> always be a sequence) might be
>> >> >> 
>> >> >>  [ 8, 16, 32 ]
>> >> >> 
>> >> >> The user may choose a subsequence, but only through truncation,
>> >> >> i.e. [ 8, 32 ] is not valid, but [ 8, 16 ] is.
>> >> >> 
>> >> >> Furthermore, different hosts may support different sequences
>> >> >> which have the same maximum. For example, if the above sequence
>> >> >> is for Host_A, then Host_B could be
>> >> >> 
>> >> >>  [ 8, 16, 24, 32 ]
>> >> >> 
>> >> >> The host must support all lengths in the sequence, which means
>> >> >> that while Host_A supports 32, since it doesn't support 24 and
>> >> >> we can only truncate sequences, we must use either [ 8 ] or
>> >> >> [ 8, 16 ] for a compatible sequence if we intend to migrate
>> >> >> between the hosts.
>> >> >> 
>> >> >> Now to the $SUBJECT question:
>> >> >> 
>> >> >> My feeling is that we should require the sequence to be
>> >> >> provided on the command line as a cpu property. Something
>> >> >> like
>> >> >> 
>> >> >>   -cpu host,sve-vl-list=8:16
>> >> >> 
>> >> >> (I chose ':' for the delimiter because ',' can't work, but
>> >> >> if there's a better choice, then that's fine by me.)
>> >> >> 
>> >> >> Afaict a property list like this will require a new parser,
>> >> 
>> >> We had 20+ of those when I last counted.  Among the more annoying
>> >> reasons CLI QAPIfication is hard[1].
>> >> 
>> >> >> which feels a bit funny since it seems we should already
>> >> >> have support for this type of thing somewhere in QEMU. So,
>> >> >> the question is: do we? I see we have array properties, but
>> >> >> I don't believe that works with the command line. Should we
>> >> >> only use QMP for this? We already want some QMP in order to
>> >> >> query the supported vector lengths. Maybe we should use QMP
>> >> >> to set the selection too? But then what about command line
>> >> >> support for developers? And if the property is on the command
>> >> >> line then we don't have to add it to the migration stream.
>> >> >
>> >> > You should be able to use arrays from the CLI with QemuOpts by repeating
>> >> > the same option name many times, though I can't say it is a very
>> >> > nice approach if you have many values to list as it gets very 
>> >> > repetative.
>> >> 
>> >> Yes, this is one of the ways the current CLI does lists.  It's also one
>> >> of the more annoying reasons CLI QAPIfication is hard[2].
>> >> 
>> >> QemuOpts let the last param=value win the stupidest way that could
>> >> possibly work (I respect that): add to the front of the list, search it
>> >> front to back.
>> >> 
>> >> Then somebody discovered that if you search the list manually, you can
>> >> see them all, and abuse that to get a list-valued param.  I'm sure that
>> >> felt clever at the time.
>> >> 
>> >> Another way to do lists the funky list feature of string input and opts
>> >> visitor.  Yet another annoying reason CLI QAPIfication is hard[3].
>> >> 
>> >> We use the opts visitor's list feature for -numa node,cpus=...  Hmm,
>> >> looks like we even combine it with the "multiple param=value build up a
>> >> list" technique: -smp node,cpus=0-1,cpus=4-5 denotes [0,1,4,5].
>> >> 
>> >> > That's the curse of not having a good CLI syntax for non-scalar data in
>> >> > QemuOpts & why Markus believes we should switch to JSON for the CLI too
>> >> >
>> >> >      -cpu host,sve-vl=8,sve-vl=16
>> >> 
>> >> We actually have CLI syntax for non-scalar data: dotted keys.  Dotted
>> >> keys are syntactic sugar for JSON.  It looks friendlier than JSON for
>> >> simple cases, then gets uglier as things get more complex, and then it
>> >> falls apart: it can't quite express all of JSON.
>> >> 
>> >> Example: sve-vl.0=8,sve-vl.1=16
>> >>     gets desugared into {"sve": [8, 16]}
>> >>     if the QAPI schema has 'sve': ['int'].
>> >> 
>> >> The comment at the beginning of util/keyval.c explains it in more
>> >> detail.
>> >> 
>> >> It powers -blockdev and -display.  Both options accept either JSON or
>> >> dotted keys.  If the option argument starts with '{', it's JSON.
>> >> Management applications should stick to JSON.
>> >> 
>> >> 
>> >> [1] Towards a more expressive and introspectable QEMU command line
>> >> https://www.linux-kvm.org/images/f/f2/Armbru-qapi-cmdline_1.pdf
>> >> Slide 34 "Backward compatibility" item 1
>> >> 
>> >> [2] ibid, item 4
>> >> 
>> >> [3] ibid, item 3
>> >>
>> >
>> > Sorry I forgot to follow up to this earlier. I looked at the examples
>> > provided and saw they were all for independent command line options,
>> > rather than command line options like '-cpu' that then accepts additional
>> > properties. I couldn't see how I could use ',' to separate array members
>> > when using properties or to use an array property input on the command
>> > line.
>> 
>> The argument of -cpu is parsed ad hoc.  Unlike QemuOpts and dotted keys,
>> parse_cpu_option() doesn't seem to support escaping ','.  Not that
>> escaping would be a user-friendly solution.
>> 
>> >       In the end I opted to use a single uint64_t for a bitmap, as 64 is
>> > big enough for now,
>> 
>> Do you think it'll remain big enough?
>
> Probably not forever, and TBH I can't even give an estimate for how long.
> Based on the current state, I "feel" like it'll be quite some time though.
> I think we can extend this map by adding more ad hoc parsing to -cpu
> later. If we added dotted key support then each array member could be
> another bitmap word, for example.

Syntax that can support such growth would be nice.

To grow a single unsigned number, we can make it wider (but we don't
have infrastructure for numbers wider than 64 bits), or we can add more
numbers (but under what name?).

Dotted keys syntax could grow more easily, but it's rather awkward.

Looking more closely at your "[PATCH 11/13] target/arm/cpu64: max cpu:
Introduce sve-vls-map"... your syntax reflects your data structure:
property "sve-vls-map" is of type uint64_t, and interpreted as bit set.
This data type would have to grow, too.

We could make widen the integer property (but we don't have
infrastructure for integer properties wider than 64 bits), or we can
turn it into an array of integers (compatibility?), or we can add more
properties to hold the additional integers (yet another silly way to
represent a list/array of integers).

I'm not asking you to complicate things just to future-proof this.  Just
pause and think whether you can pick a data type that's similarly
convenient now, and easier to grow.

Then pick an external syntax for this data type.  You may have to pick a
reasonable compromise between ease of implementation and ease of use.

>> >                     and even though passing some hex number on the command
>> > line isn't user friendly at all, it didn't seem like a long list of a
>> > repeated parameter was that user friendly either. Of course I'm still open
>> > to suggestions to try to find the best balance between user friendliness,
>> > current QEMU command line parsing support, and just getting a bitmap into
>> > cpu state one way or another.
>> 
>> I'd ask for consistency with existing practice no matter how flawed if
>> we had such consistency.
>> 
>> If I understand your "[PATCH 00/13] target/arm/kvm: enable SVE in
>> guests" correctly, the bitmap form of [1, 2, 4] is
>> 
>>     -cpu max,sve-vls-map=11
>> 
>> Observe bit#0 means 1; better document that clearly.
>> 
>> If we used dotted keys to produce an intList, we'd do
>> 
>>     -cpu max,sve-vls-map.0=1,sve-vls-map.1=2,sve-vls-map.2=4
>> 
>> If the option argument is QAPIfied, we additionally get
>> 
>>     -cpu '{"type": "max", "sve-vls-map": [1, 2, 4]}'
>> 
>> for free.
>> 
>> If we did it like -numa (please don't), we'd get something like
>> 
>>     -cpu max,sve-vls-map=1-2,sve-vls-map=4
>> 
>> None of the above is exactly a pinnacle of user-friendliness.  JSON is
>> at least ugly in a regular way.  Your bitmap encoded in a number is at
>> least concise.
>> 
>> If a numerically encoded bitmap is the least bad option here, I wonder
>> why it's not the least bad option for -numa...  Perhaps because there 64
>> isn't big enough.
>> 
>> I'm afraid the numerically encoded bitmap will make its way into the
>> QAPI schema sooner or later.  This will create an unfortunate
>> inconsistency with the [int] encoding already there.
>> 
>> Who's going to use sve-vls-map?  Humans, or pretty much only machines?
>
> My thought is primarily machines. If a human wants to use the command
> line and SVE, then I'm assuming they'll be happy with sve-max-vq or
> figuring out a map they like once and then sticking to it.

Primarily machines means we can accept more verbosity.

If I understand the cover letter of your "[PATCH 00/13] target/arm/kvm:
enable SVE in guests" correctly, then sve-max-vq and sve-vls-map are
alternative interfaces for the same thing.  The latter is more general,
but awkward on the command line and verbose everywhere.  The former
isn't usable with -cpu host.  Correct?

If there wasn't "not usable with -cpu host", I'd ask whether we really
need the generality.