Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote:
>> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:

[...]

>> > I recommend to do exactly what we've done before for complex
>> > configuration: define it in the QAPI schema, so we can use both dotted
>> > keys and JSON on the command line, and can have QMP, too.  Examples:
>> > -blockdev, -display, -compat.
>> > 
>> > Questions?
>> 
>> Hi Markus, Daniel,
>> 
>> I'm dealing with similar considerations with some SNP config options
>> relating to CPUID enforcement, so I've started looking into this as
>> well, but am still a little confused on the best way to proceed.
>> 
>> I see that -blockdev supports both JSON command-line arguments (via
>> qobject_input_visitor_new) and dotted keys
>> (via qobject_input_vistior_new_keyval).

Yes.  Convenience function qobject_input_visitor_new_str() provides
this.

>> We could introduce a new config group do the same, maybe something specific
>> to ConfidentialGuestSupport objects, e.g.:
>> 
>>   -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...
>
> We don't wnt to be adding new CLI options anymore. -object with json
> syntx should ultimately be able to cover everything we'll ever need
> to do.

Depends.  When you want a CLI option to create a single QOM object, then
-object is commonly the way to go.

>> and use the same mechanisms to parse the options, but this seems to
>> either involve adding a layer of option translations between command-line
>> and the underlying object properties, or, if we keep the 1:1 mapping
>> between QAPI-defined keys and object properties, it basically becomes a
>> way to pass a different Visitor implementation into object_property_set(),
>> in this case one created by object_input_visitor_new_keyval() instead of
>> opts_visitor_new().

qobject_input_visitor_new_str() provides 1:1, i.e. common abstract
syntax, and concrete syntax either JSON or dotted keys.  Note that the
latter is slightly less expressive (can't do empty arrays and objects,
may fall apart for type 'any').  If you run into these limitations, use
JSON.  Machines should always use JSON.

qobject_input_visitor_new_str() works by wrapping the "right" visitor
around the option argument.  Another way to provide a human-friendly
interface in addition to a machine-friendly one is to translate from
human to the machine interface.  HMP works that way: HMP commands wrap
around QMP commands.  The QMP commands are generated from the QAPI
schema.  The HMP commands are written by hand, which is maximally
flexible, but also laborious.

>> In either case, genericizing it beyond CGS/SEV would basically be
>> introducing:
>> 
>>   -object2 sev-guest,id=sev0,key_a.subkey_b=...

That's because existing -object doesn't use keyval_parse() + the keyval
QObjct input visitor, it uses QemuOpts and the options visitor, for
backward compatibility with all their (barely understood) features and
warts.

Unfortunate, because even new user-creatable objects can't escape
QemuOpts.

QemuOpts needs to go.  But replacing it is difficult and scary in
places.  -object is such a place.

>> Which one seems preferable? Or is the answer neither?
>
> Yep, neither is the answer.

Welcome to the QemuOpts swamp, here's your waders and a leaky bucket.

>> I've also been looking at whether this could all be handled via -object,
>> and it seems -object already supports JSON command-line arguments, and that
>> switching it from using OptsVisitor to QObjectVisitor for non-JSON case
>> would be enough to have it handle dotted keys, but I'm not sure what the
>> fall-out would be compatibility-wise.

It's complicated, and nobody knows for sure.  That's why we didn't dare
to take the jump, and instead grafted on JSON syntax without changing
the old syntax.  Excuse me while I sacrifice another rubber chicken to
the backward compatibility idol.

>> All lot of that falls under making sure the QObject/keyval parser is
>> compatible with existing command-lines parsed via OptsVisitor. One example
>> where there still seems to be a difference is lack of support for ranges
>> such as "cpus=1-4" in keyval parser. Daniel had a series that addressed
>> this:
>> 
>>   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08248.html
>> 
>> but it doesn't seem to have made it into the tree, which is why I feel like
>> maybe there are complications with this approach I haven't considered?
>
> The core problem with QemuOpts is that it has hacked various hacks
> grafted onto it to cope with non-scalar data. My patch was adding
> yet another hack. It very hard to introduce new hacks without risk
> of causing incompatibility with other existing hacks, or introducing
> unexpected edge cases that we don't anticipate.

Some of the thornier compatibility issues with QemuOpts are due to
unforeseen edge cases of, and interactions between features.

> Ultimately I think we've come to the conclusion that QemuOpts is an
> unfixable dead end that should be left alone. Our future is trending
> towards being entirely JSON, configured via the QMP monitor not the
> CLI. As such the json support for -object is a step towards the pure
> JSON world.

QemuOpts served us well for a while, but we've long grown out of its
limitations.  It needs to go.

QemuOpts not providing an adequate CLI language does not imply we can't
have an adequate CLI language.  The "everything QMP" movement is due to
other factors.  I have serious reservations about the idea, actually.

> IOW, if you have things that work today with QemuOpts that's fine.
>
> If, however, you're coming across situations that need non-scalar
> data and it doesn't work with QemuOpts, then just declare that
> -object json syntax is mandatory for that scenario. Don't invest
> time trying to improve QemuOpts for non-scalar data, nor inventing
> new CLI args.

I agree 100% with "don't mess with QemuOpts".  That mess is complete.

Whether a new CLI option makes sense should be decided case by case.

"Must use JSON" feels acceptable for things basically only management
applications use.

> FWIW, specificallly in the case of parameters that take an integer
> range, like cores=1-4, in JSON we'd end up representing that as a
> array of integers and having to specify all values explicitly.
> This is quite verbose, but functionally works.
>
> I think it would have been nice if we defined an explicit 'bitmap'
> scalar data type in QAPI for these kind of things, but at this point
> in time I think that ship has sailed, and trying to add that now
> would create an inconsistent representation across different places.

External representation (i.e. JSON) should be as consistent as we can
make it.  Multiple different internal representations can be okay.  So
far, we represent JSON arrays as linked lists.  Optionally representing
certain arrays of small integers as bit vectors feels feasible.  Whether
it's worth the effort is another question.