On 30/11/20 13:25, Kevin Wolf wrote:

+## +# @IothreadProperties: +# +# Properties for iothread objects. +# +# @poll-max-ns: the maximum number of nanoseconds to busy wait for events. +# 0 means polling is disabled (default: 32768 on POSIX hosts, +# 0 otherwise) +# +# @poll-grow: the multiplier used to increase the polling time when the +# algorithm detects it is missing events due to not polling long +# enough. 0 selects a default behaviour (default: 0) +# +# @poll-shrink: the divisor used to decrease the polling time when the +# algorithm detects it is spending too long polling without +# encountering events. 0 selects a default behaviour (default: 0) +# +# Since: 6.0 +## +{ 'struct': 'IothreadProperties', + 'data': { '*poll-max-ns': 'int', + '*poll-grow': 'int', + '*poll-shrink': 'int' } } +

Documentation is the main advantage of the ObjectOptions concept. However, please use the version where each object and property was introduced for the "since" value. Otherwise the documentation will appear to show that none of these objects was available before 6.0. Yes, there is no documentation at all right now for QOM objects. However, wrong documentation sometimes is worse than non-existing documentation. Paolo

On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:

On 30/11/20 13:25, Kevin Wolf wrote:

...

This series adds a QAPI type for the properties of all user creatable QOM types and finally makes QMP object-add use the new ObjectOptions union so that QAPI introspection can be used for user creatable objects.

After this series, there is least one obvious next step that needs to be done: Change HMP and all of the command line parser to use ObjectOptions, too, so that the QAPI schema is consistently enforced in all external interfaces. I am planning to send another series to address this.

In a third step, we can try to start deduplicating and integrating things better between QAPI and the QOM implementation, e.g. by generating parts of the QOM boilerplate from the QAPI schema.

With this series it's basically pointless to have QOM properties at all. Instead, you are basically having half of QEMU's backend data model into a single struct.

So the question is, are we okay with shoveling half of QEMU's backend data model into a single struct? If so, there are important consequences.

In theory they should have the same set of options, but nothing in this series will enforce that. So we're introducing the danger that QMP object-add will miss some property, and thus be less functional than the CLI -object. If we convert CLI -object to use the QAPI parser too, we eliminate that danger, but we still have the struct duplication.

1) QOM basically does not need properties anymore except for devices and machines (accelerators could be converted to QAPI as well). All user-creatable objects can be changed to something like chardev's "get a struct and use it fill in the fields", and only leave properties to devices and machines.

2) User-creatable objects can have a much more flexible schema. This means there's no reason to have block device creation as its own command and struct for example.

The problem with this series is that you are fine with deduplicating things as a third step, but you cannot be sure that such deduplication is possible at all. So while I don't have any problems in principle with the ObjectOptions concept, I don't think it should be committed without a clear idea of how to do the third step.

I feel like we should at least aim to kill the struct duplication, even if we ignore the bigger QOM stuff like setters/getters/constructors/etc. The generated structs are not far off being usable. eg for the secret object we have the QAPI schema { 'struct': 'SecretCommonProperties', 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] }, '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } } { 'struct': 'SecretProperties', 'base': 'SecretCommonProperties', 'data': { '*data': 'str', '*file': 'str' } } IIUC this will resulting in a QAPI generated flattened struct: struct SecretProperties { bool loaded; QCryptoSecretFormat format; char *keyid; char *iv; char *data; char *file; }; vs the QOM manually written structs struct QCryptoSecretCommon { Object parent_obj; uint8_t *rawdata; size_t rawlen; QCryptoSecretFormat format; char *keyid; char *iv; }; struct QCryptoSecret { QCryptoSecretCommon parent_obj; char *data; char *file; }; The key differences - The parent struct is embedded, rather than flattened - The "loaded" property doesn't need to exist - Some extra fields are live state (rawdata, rawlen) Lets pretend we just kill "loaded" entirely, so ignore that. We could simply make QOM "Object" a well known built-in type, so we can reference it as a "parent". Then any struct with "Object" as a parent could use struct embedding rather flattening and thus just work. Can we invent a "state" field for fields that are internal only, separate from the public "data" fields. eg the secret QAPI def would only need a couple of changes: { 'struct': 'QCryptoSecretCommon', 'base': 'Object', 'state': { 'rawdata': '*uint8_t', 'rawlen': 'size_t' }, 'data': { '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } } { 'struct': 'QCryptoSecret', 'base': 'QCryptoSecretCommon', 'data': { '*data': 'str', '*file': 'str' } } There would need to be a void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj) method defined manually by the programmer to take care of free'ing any pointers in the "state". Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Nov 30, 2020 at 05:13:57PM +0100, Kevin Wolf wrote:

Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:

...

On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:

...

On 30/11/20 13:25, Kevin Wolf wrote:

...

This series adds a QAPI type for the properties of all user creatable QOM types and finally makes QMP object-add use the new ObjectOptions union so that QAPI introspection can be used for user creatable objects.

After this series, there is least one obvious next step that needs to be done: Change HMP and all of the command line parser to use ObjectOptions, too, so that the QAPI schema is consistently enforced in all external interfaces. I am planning to send another series to address this.

In a third step, we can try to start deduplicating and integrating things better between QAPI and the QOM implementation, e.g. by generating parts of the QOM boilerplate from the QAPI schema.

With this series it's basically pointless to have QOM properties at all. Instead, you are basically having half of QEMU's backend data model into a single struct.

So the question is, are we okay with shoveling half of QEMU's backend data model into a single struct? If so, there are important consequences.

In theory they should have the same set of options, but nothing in this series will enforce that. So we're introducing the danger that QMP object-add will miss some property, and thus be less functional than the CLI -object. If we convert CLI -object to use the QAPI parser too, we eliminate that danger, but we still have the struct duplication.

I think converting the CLI is doable in the short term. I already have the patch for qemu-storage-daemon, but decided to keep it for a separate series.

The most difficult part is probably -readconfig, but with Paolo's RFC patches to move it away from QemuOpts, even that shouldn't be very hard.

...

...

1) QOM basically does not need properties anymore except for devices and machines (accelerators could be converted to QAPI as well). All user-creatable objects can be changed to something like chardev's "get a struct and use it fill in the fields", and only leave properties to devices and machines.

2) User-creatable objects can have a much more flexible schema. This means there's no reason to have block device creation as its own command and struct for example.

The problem with this series is that you are fine with deduplicating things as a third step, but you cannot be sure that such deduplication is possible at all. So while I don't have any problems in principle with the ObjectOptions concept, I don't think it should be committed without a clear idea of how to do the third step.

I feel like we should at least aim to kill the struct duplication, even if we ignore the bigger QOM stuff like setters/getters/constructors/etc. The generated structs are not far off being usable.

eg for the secret object we have the QAPI schema

{ 'struct': 'SecretCommonProperties', 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] }, '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } }

{ 'struct': 'SecretProperties', 'base': 'SecretCommonProperties', 'data': { '*data': 'str', '*file': 'str' } }

IIUC this will resulting in a QAPI generated flattened struct:

struct SecretProperties { bool loaded; QCryptoSecretFormat format; char *keyid; char *iv; char *data; char *file; };

vs the QOM manually written structs

struct QCryptoSecretCommon { Object parent_obj; uint8_t *rawdata; size_t rawlen; QCryptoSecretFormat format; char *keyid; char *iv; };

struct QCryptoSecret { QCryptoSecretCommon parent_obj; char *data; char *file; };

The key differences

- The parent struct is embedded, rather than flattened - The "loaded" property doesn't need to exist - Some extra fields are live state (rawdata, rawlen)

Lets pretend we just kill "loaded" entirely, so ignore that.

We could simply make QOM "Object" a well known built-in type, so we can reference it as a "parent". Then any struct with "Object" as a parent could use struct embedding rather flattening and thus just work.

Can we invent a "state" field for fields that are internal only, separate from the public "data" fields.

eg the secret QAPI def would only need a couple of changes:

{ 'struct': 'QCryptoSecretCommon', 'base': 'Object', 'state': { 'rawdata': '*uint8_t', 'rawlen': 'size_t' }, 'data': { '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } }

{ 'struct': 'QCryptoSecret', 'base': 'QCryptoSecretCommon', 'data': { '*data': 'str', '*file': 'str' } }

I haven't given much though to the details yet, but I was thinking of introducing a new QAPI entity type for objects. We could include additional fields there, where the type would just directly be a C type rather than being interpreted by QAPI.

Maybe like this:

{ 'object': 'secret-common', 'abstract': true, 'properties': 'SecretCommonProperties', 'state': { 'rawdata': '*uint8_t', 'rawlen': 'size_t' } }

{ 'object': 'secret', 'parent': 'secret-common', 'properties': 'SecretProperties' } }

Maybe it would actually be nicer to have 'state' just as a string property that contains the C type name of the state struct and then QAPI just adds a pointer to it.

Yep, it would be nice to have clear separation of the "state" from the "config", as that also makes it more obvious what is derived info.

Either way, there is some duplication there because we have a parent-child relationship both between the object types themselves and between their property classes. Either we remove the base from SecretProperties (which would make object-add and the CLI more complicated) or we just let the QAPI generator check that they are consistent.

I don't really like the duplicate hierarchies there either. I did consider, a new 'object' entity instead of 'struct', but if we go that way we should exclusively use "object" for the QOM types. eg an "object" entity type would be a specialization of the "struct" entity type, rather than something bolted onto the side. Basically I think the QOM struct and the properties struct should remain one & the same thing. If we think of "object" as a specialization of 'struct' then and have "state" as a separate struct, we avoid the duplicate hierarchies { 'object': 'QCryptoSecretCommon', 'state': 'QCryptoSecretCommonState', 'data': { '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } } { 'object': 'QCryptoSecret', 'base': 'QCryptoSecretCommon', 'data': { '*data': 'str', '*file': 'str' } } there's not really much difference to this than just carrying on using "struct" entity type though, and having the special "Object" parent type as a built-in type.

...

There would need to be a

void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)

method defined manually by the programmer to take care of free'ing any pointers in the "state".

Isn't this the job of the normal .instance_finalize method?

Yes, but I was thinking the fact how to wire into the free methods that QAPI generates. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Paolo Bonzini <pbonzini@redhat.com> writes:

On 30/11/20 16:30, Daniel P. Berrangé wrote:

...

{ 'struct': 'QCryptoSecretCommon', 'base': 'Object', 'state': { 'rawdata': '*uint8_t', 'rawlen': 'size_t' }, 'data': { '*format': 'QCryptoSecretFormat', '*keyid': 'str', '*iv': 'str' } } { 'struct': 'QCryptoSecret', 'base': 'QCryptoSecretCommon', 'data': { '*data': 'str', '*file': 'str' } }

No, please don't do this. I want to keep writing C code, not a weird JSON syntax for C.

I'd much rather have a FooOptions field in my struct so that I can just do visit_FooOptions in the UserCreatable.complete() method, that's feasible.

It should be, because it's what we've done elsewhere, isn't it? Yes, we can extend QAPI to let us embed opaques outside the QAPI schema's scope ("state"), along with means to create, destroy, and clone. This is new. But we can also invert: put the QAPI-generated C type ("config") in a handwritten C type that marries it to "state". Code to create and destroy is handwritten, and uses QAPI-generated code for the "config" part. A generic interface to handwritten creation is possible: Take the QAPI-generated "config" type as argument, extract enough information to call the appropriate constructor, return its value. Generic destruction is also possible: all it needs is a map from instance to destructor function. None of this is new in QEMU.

On 30/11/20 16:46, Kevin Wolf wrote:

Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:

...

With this series it's basically pointless to have QOM properties at all.

Not entirely, because there are still some writable properties that can be changed later on.

Are there really any (that are not bugs like opened/loaded)? That's also why Eduardo and I discussed a class-wide allow_set function for his field properties series.

So with this in mind, I think I'm in favour of completely leaving the initialisation of properties on object creation to QAPI, and only providing individual setters if we actually intend to allow property changes after creation.

The main problem is that it wouldn't extend well, if at all, to machines and devices. So those would still not be integrated into the QAPI schema.

...

So the question is, are we okay with shoveling half of QEMU's backend data model into a single struct? If so, there are important consequences.

Yeah, the single struct bothers me a bit, both in the QAPI schema and in the C source.

The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add. So I start to wonder if QOM as it exists now is the right solution for all kind of objects: - backends and other object-add types (not per-target, relatively few classes and even fewer hierarchies) - machine (per-target, many classes, no hierarchy) - device (can be treated as not per-target, many many classes, very few hierarchies) - accelerator (per-target, few classes, no hierarchy) - chardev (ok those are the same as the first category) If QOM is the right solution, this patch goes in the wrong direction. If QOM is the wrong solution, this patch is okay but then we have another problem to solve. :)

...

The problem with this series is that you are fine with deduplicating things as a third step, but you cannot be sure that such deduplication is possible at all. So while I don't have any problems in principle with the ObjectOptions concept, I don't think it should be committed without a clear idea of how to do the third step.

Do you have any specific concerns why the deduplication might not possible, or just because it's uncharted territory?

Mostly because it's more or less the same issue that you have with BlockdevOptions, with the extra complication that this series only deals with the easy one of the four above categories.

Maybe if we don't want to commit to keeping the ObjectOptions schema, the part that should wait is object-add and I should do the command line options first? Then the schema remains an implementation detail for now that is invisible in introspection.

I don't see much benefit in converting _any_ of the three actually. The only good reason I see for QAPIfying this is the documentation, and the promise of deduplicating QOM boilerplate. The latter is only a promise, but documentation alone is a damn good reason and it's enough to get this work into a mergeable shape as soon as possible! But maybe, we could start in the opposite direction: start with the use QAPI to eliminate QOM boilerplate. Basing your work on Eduardo's field properties series, you could add a new 'object' "kind" to QAPI that would create an array of field properties (e.g. a macro expanding to a compound literal?) . Something like +{ 'object': 'InputBarrier', + 'data': { 'name': 'str', + 'x-origin': 'int16', + 'y-origin': 'int16', + 'width': 'int16', + 'height': 'int16' }, + 'properties': { 'server': 'str', + 'port': 'str' } } would create a macro QOM_InputBarrier_FIELDS defining properties for the following fields of the InputBarrier struct: gchar *name; int16_t x_origin, y_origin; int16_t width, height; while server and port would only appear in the documentation (or alternatively you could leave them out completely, as you wish). The advantages would be noticeable: 1) the information would be moved in the QAPI schema JSON from the beginning, decoupling the conflict-heavy part from the complex question of how to expose the QOM schema in the introspection data 2) there would not be any more duplication than before (there would be duplication between structs and QAPI schema, but not between structs and C code that defines properties). 3) it would be opt-in, so it doesn't put on you the burden of keeping the series in sync with new objects that are added (I have one for the qtest server for example). At the same time it would be quite appealing for owners of QOM code to convert their objects to field properties and get documentation for free. 4) we could special-case 'object' definitions and generate them in the system manual as well, since they are also useful to document -object. Yes it's a huge change but you have the boring part already done. What do you think? Paolo

...

In the meanwhile, of course I have no problem with deprecating the opened and loaded properties.

If we decide that we don't want to have the schema at all (which I hope we won't decide), I can split the deprecation into separate patches.

Kevin

On 30/11/20 19:10, Kevin Wolf wrote:

Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben:

...

The main problem is that it wouldn't extend well, if at all, to machines and devices. So those would still not be integrated into the QAPI schema.

What do you think is the biggest difference there? Don't devices work the same as user creatable objects in that you first set a bunch of properties (which would now be done through QAPI instead) and then call the completion/realize method that actually makes use of them?

For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place.

...

The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add.

But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create.

There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code.

I'm also pretty sure that QOM as it exists now is not the right solution for any of them because it has some major shortcomings. It's too easy to get things wrong (like the writable properties after creation), its introspection is rather weak and separated from the QAPI introspection, it doesn't encourage documentation, and it involves quite a bit of boilerplate and duplicated code between class implementations.

A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI.

I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts.

...

Mostly because it's more or less the same issue that you have with BlockdevOptions, with the extra complication that this series only deals with the easy one of the four above categories.

I'm not sure which exact problem with BlockdevOptions you mean. The reason why the block layer doesn't use BlockdevOptions everywhere is -drive support.

You don't have a single BlockdevOptions* field in the structs of block/. My interpretation of this is that BlockdevOptions* is a good schema for configuring things but not for run-time state.

...

...

Maybe if we don't want to commit to keeping the ObjectOptions schema, the part that should wait is object-add and I should do the command line options first? Then the schema remains an implementation detail for now that is invisible in introspection.

I don't see much benefit in converting _any_ of the three actually. The only good reason I see for QAPIfying this is the documentation, and the promise of deduplicating QOM boilerplate. The latter is only a promise, but documentation alone is a damn good reason and it's enough to get this work into a mergeable shape as soon as possible!

I think having some input validation done by QAPI instead of in each QOM object individually is nice, too. You get it after CLI, QMP and HMP all go through QAPI.

But the right way to do that is to use Eduardo's field properties: they make QOM do the right thing, adding another layer of parsing is just adding more complexity. Are there any validation bugs that you're fixing? Is that something that cannot be fixed elsewhere, or are you papering over bad QOM coding? (Again, I'm not debating that QOM properties are hard to write correctly).

...

But maybe, we could start in the opposite direction: start with the use QAPI to eliminate QOM boilerplate. Basing your work on Eduardo's field properties series, you could add a new 'object' "kind" to QAPI that would create an array of field properties (e.g. a macro expanding to a compound literal?)

There is a very simple reason why I don't want to start with the QAPI generator: John has multiple series pending that touch basically every part of the QAPI generator. This means not only that I need to be careful about merge conflict (or base my work on top of five other series, which feels adventurous), but also that I would be competing with John for Markus' reviewer capacity, further slowing things down.

That's something that you have to discuss with John and Markus. It may be that John has to shelve part of his work or rebase on top of yours instead. By adding an extra parsing layer you're adding complexity that may not be needed in the end, and we're very bad of removing it later. So I'm worried that this series will become technical debt in just a few months.

Well, two reasons: Also because this series for the external interface of the objects already exists and it's an incremental step towards your proposal: The types for 'properties' will already exist then and I won't have to convert both internal state and external interfaces at the same time.

I don't think converting internal state to QAPI is useful. QAPI and QOM are external interfaces and that's what they should remain; anything that is not an external interface should (must) remain plain C code. Paolo

On 01/12/20 17:20, Kevin Wolf wrote:

Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:

...

For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place.

Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally.

There's also the question that most devices are not present in all binaries. So QAPI introspection would tell you what properties are supported but not what devices are. Also the marshaling/unmarshaling code for hundreds of devices would bloat the QEMU executables unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd not be possible to compile it out, even with LTO). Plus the above issue with machine types.

...

...

...

The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add.

But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create.

There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code.

Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state.

Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum.

I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal.

Right, I don't want to have the final state now but I'd like to have a rough idea of the plan: 1) whether to generate _all_ boilerplate or only properties 2) whether we want to introduce a separation between configuration schema and run-time state 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. These questions have a substantial effect on how to handle this series. Without answers to this questions I cannot understand the long term and its interaction with other long term plans for QOM.

...

...

A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI.

I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts.

QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces.

Yes, but QAPI is also about interfacing with existing code. Also, QAPI does not generate only structs, it also generate C function prototypes. I'm not sure whether a QOM object's more similar to the struct case (what you do here) or to the QMP command case: - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete implementations - there's a registry of object types just like there's one for QMP commands. So another possibility could be that the QAPI generator essentially generated the user_creatable_add_type function that called out to user-provided functions qom_scsi_pr_helper_complete, qom_throttle_group_complete, etc. The arguments to those functions would be the configuration. That is a very interesting prospect (though one that would require changes to the QAPI code generator).

BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case.

Right. It may well be that we decide that, as a result of this series, QOM's property interface is declared essentially a failed experiment. I wouldn't be surprised, and that's why I want to understand better where we want to go. For example, Eduardo is focusing specifically on external interfaces that correspond 1:1 to the internal implementation. If we decide that non-1:1-mappings and checks on mandatory properties are an important part of QOM, that may make large parts of his work moot. If we decide that most QOM objects need no properties at all, then we don't want to move more qdev-specific stuff from to QOM.

QAPI is already here and it's going to stay. QOM doesn't have to duplicate input validation that existing code can already perform.

I'm not sure which complexity you think I'm introducing: QAPI is already there. I'm adding the schema, which you agree is valuable documentation, so we want to have it either case. The actual change to QOM that we have in this series is this:

The complexity is that properties used to be split in two places, and now they're split in three places. It may very well be that this is a good first step to at least have classes described in the QAPI schema. But since _in the short term_ there are things that the series makes worse (and has a risk of bringing things out of sync), I'd like to understand the long term plan and ensure that the QAPI maintainers are on board with it. Can you at least add a comment to all UserCreatable classes that says "if you add a property, remember to modify ... as well in the QAPI schema"?

...

Are there any validation bugs that you're fixing? Is that something that cannot be fixed elsewhere, or are you papering over bad QOM coding? (Again, I'm not debating that QOM properties are hard to write correctly).

Yes, I found bugs that the QAPI schema would have prevented. They were generally about not checking whether mandatory propertes are actually set.

Okay, I found your series at https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good to know. So that's another useful thing that can be chalked to this series at least if -object and object_add are converted (and also, another thing against QOM properties and 1:1 mappings between configuration schema and run-time state). Paolo

Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:

On 01/12/20 17:20, Kevin Wolf wrote:

...

Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:

...

For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place.

Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally.

There's also the question that most devices are not present in all binaries. So QAPI introspection would tell you what properties are supported but not what devices are. Also the marshaling/unmarshaling code for hundreds of devices would bloat the QEMU executables unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd not be possible to compile it out, even with LTO).

I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. What probably needs a solution is that an explicit 'if' in the schema would duplicate information that is actually defined in the build system configuration. So we would somehow need a common source for both.

Plus the above issue with machine types.

As I said, I don't know the machine type code well, so I'm not really sure about the best way there. But maybe QAPI isn't for version-specific machine types. I think they never have additional properties, but only different init functions. So their description in QAPI would be essentially empty anyway. So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves?

...

...

...

...

The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add.

But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create.

There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code.

Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state.

Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum.

I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal.

Right, I don't want to have the final state now but I'd like to have a rough idea of the plan:

1) whether to generate _all_ boilerplate or only properties

I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source.

2) whether we want to introduce a separation between configuration schema and run-time state

You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. Or do you mean between create-time options and properties that can be written at runtime? In that case, yes, I think that would be very desirable because mashing them together has resulted in lots of bugs.

3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation.

How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Outside of QOM, usually have a query-* command that returns the whole state rather than a single property, and possibly individual QMP commands to update the state. blockdev-reopen takes a new configuration (the same structure as blockdev-add) and then applies that whole new configuration to the existing block node. I guess this would be an alternative, though it is somewhat inconvenient because you have to repeat all the options that you don't want to change. The blockdev-reopen way has one important advantage: You can change multiple options at the same time when there are dependencies between the options so that individually changing each option would be invalid, but together it's possible. throttle-group needs this in QOM, and it's easily implemented: It just has a single 'limits' property, which is a struct and is always updated as a single unit. So in summary, it seems to me that the QOM way is more flexible because you can get both models out of it. Whether we actually need this flexibility I can't say. But there is another reason why I didn't intend to remove properties entirely, which is that things are a lot easier as long as you don't have to break external interfaces, which qom-set/get/list are. The way I imagined it so far is roughly like this: * Configuration options are described in the QAPI schema. This is mainly for object creation, but runtime modifiable properties are a subset of this. * Properties are generated for each option. By default, the getter just returns the value from the configuration at creation time, though generation of it can be disabled so that it can be overridden. Also, setters just return an error by default. * Property setters aren't called for object creation. Instead, the relevant ObjectOptions branch is made available to some init method. * Runtime modifiable properties (declared as such in the schema) don't get the default setter, so you have to provide an implementation for them.

These questions have a substantial effect on how to handle this series. Without answers to this questions I cannot understand the long term and its interaction with other long term plans for QOM.

...

...

...

A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI.

I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts.

QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces.

Yes, but QAPI is also about interfacing with existing code. Also, QAPI does not generate only structs, it also generate C function prototypes. I'm not sure whether a QOM object's more similar to the struct case (what you do here) or to the QMP command case:

- there's a 1:1 correspondence between ObjectOptions cases and ucc->complete implementations

- there's a registry of object types just like there's one for QMP commands.

This is an interesting comparison. I never thought of likening objects to commands, but there is some truth to it. I certainly did envision objects as separate type of QAPI entities, which I guess is why I didn't really like Dan's suggestion to make objects an extension of structs, but I couldn't really express why. I think your comparison gives a good answer. However, this series is still not doing the wrong thing: If you think about commands, they don't exist without an arguments struct. Similarly, objects don't exist without a properties struct. So while this series is doing only one part of the whole solution, that the second part is missing doesn't make the first part wrong.

So another possibility could be that the QAPI generator essentially generated the user_creatable_add_type function that called out to user-provided functions qom_scsi_pr_helper_complete, qom_throttle_group_complete, etc. The arguments to those functions would be the configuration. That is a very interesting prospect (though one that would require changes to the QAPI code generator).

I was thinking of using .instance_init because it's more generic, but using ucc->complete (and in the long run realize for qdev) might require less modifications of non-user-creatable objects. One possibly nasty detail to consider there is that we sometimes declare the USER_CREATABLE interface in the base class, so ucc->complete is for the base class rather than the actually instantiated class. If we only instantiate leaf classes (I think this is true), we can move USER_CREATABLE there. I also had in mind just passing the whole configuration struct (essentially always 'boxed': true), but you're right that individual parameters like for commands would be possible. I'm not entirely convinced that they would be better (there was a reason why we introduced 'boxed': true), but it's an option.

...

BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case.

Right. It may well be that we decide that, as a result of this series, QOM's property interface is declared essentially a failed experiment. I wouldn't be surprised, and that's why I want to understand better where we want to go.

For example, Eduardo is focusing specifically on external interfaces that correspond 1:1 to the internal implementation. If we decide that non-1:1-mappings and checks on mandatory properties are an important part of QOM, that may make large parts of his work moot. If we decide that most QOM objects need no properties at all, then we don't want to move more qdev-specific stuff from to QOM.

Hm, it's a good point. 1:1 properties would actually end up in generated code anyway, so simplifying them is maybe not that useful in the end. It would probably result in nicer generated code, but that's it.

...

QAPI is already here and it's going to stay. QOM doesn't have to duplicate input validation that existing code can already perform.

I'm not sure which complexity you think I'm introducing: QAPI is already there. I'm adding the schema, which you agree is valuable documentation, so we want to have it either case. The actual change to QOM that we have in this series is this:

The complexity is that properties used to be split in two places, and now they're split in three places.

It may very well be that this is a good first step to at least have classes described in the QAPI schema. But since _in the short term_ there are things that the series makes worse (and has a risk of bringing things out of sync), I'd like to understand the long term plan and ensure that the QAPI maintainers are on board with it.

Can you at least add a comment to all UserCreatable classes that says "if you add a property, remember to modify ... as well in the QAPI schema"?

I was hoping that by converting object-add in this series, and the CLI options soon afterwards, it would be very obvious if you forget to change the schema because your new property just wouldn't work (at least not during creation). But I can add comments if you think this is still worthwhile.

...

...

Are there any validation bugs that you're fixing? Is that something that cannot be fixed elsewhere, or are you papering over bad QOM coding? (Again, I'm not debating that QOM properties are hard to write correctly).

Yes, I found bugs that the QAPI schema would have prevented. They were generally about not checking whether mandatory propertes are actually set.

Okay, I found your series at https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good to know.

And two more patches of the same type here: https://patchew.org/QEMU/20201117163045.307451-1-kwolf@redhat.com/

So that's another useful thing that can be chalked to this series at least if -object and object_add are converted (and also, another thing against QOM properties and 1:1 mappings between configuration schema and run-time state).

Yes. object-add is already covered by this series and -object should be easy, so we can have this more or less now. Getting the design right for QOM code generation will certainly take more time, discussion and also experimentation. Kevin

On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote:

On 01/12/20 20:35, Kevin Wolf wrote:

...

Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible.

There is some complication due to having different discriminators per target. So yes it should be possible. But probably best left after objects because it's so much bigger a task and because objects have a bit more freedom for experimentation (less ties to other qdev-specific concepts, e.g. the magic "bus" property).

...

So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves?

Yes, maybe.

...

...

1) whether to generate _all_ boilerplate or only properties

I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate.

Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source.

Ok, so that's a bit different from what I am thinking of. I don't care very much about the internal boilerplate, only the external interface for configuration. So I don't care about type registration, dynamic cast macros etc., only essentially the part that leads to ucc->complete.

...

...

2) whether we want to introduce a separation between configuration schema and run-time state

You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things.

I mean moving more towards the blockdev/chardev way of doing things, increasing the separation very much by having separate configuration structs that have (potentially) no link to the run-time state struct.

...

...

3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation.

How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative?

Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore.

Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set).

Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties.

I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system.

...

So in summary, it seems to me that the QOM way is more flexible because you can get both models out of it. Whether we actually need this flexibility I can't say.

I'm thinking there's no need for it, but maybe I'm overly optimistic.

...

* Configuration options are described in the QAPI schema. This is mainly for object creation, but runtime modifiable properties are a subset of this.

* Properties are generated for each option. By default, the getter just returns the value from the configuration at creation time, though generation of it can be disabled so that it can be overridden. Also, setters just return an error by default.

* Property setters aren't called for object creation. Instead, the relevant ObjectOptions branch is made available to some init method.

* Runtime modifiable properties (declared as such in the schema) don't get the default setter, so you have to provide an implementation for them.

I wouldn't bother with properties at all in the QAPI schema. Just do the first and third bullet. Declaring read-only QOM properties is trivial.

I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu.

...

So while this series is doing only one part of the whole solution, that the second part is missing doesn't make the first part wrong.

Yeah, I think it's clear that for the long term we're not really disagreeing (or perhaps I'm even more radical than you :)). I'm just worried about having yet another incomplete transition.

...

One possibly nasty detail to consider there is that we sometimes declare the USER_CREATABLE interface in the base class, so ucc->complete is for the base class rather than the actually instantiated class. If we only instantiate leaf classes (I think this is true), we can move USER_CREATABLE there.

You can also use a while loop covering each superclass to decide how to dispatch ucc->complete. I don't care much about these details, they're Simple Matter Of Programming. :)

...

I also had in mind just passing the whole configuration struct (essentially always 'boxed': true), but you're right that individual parameters like for commands would be possible. I'm not entirely convinced that they would be better (there was a reason why we introduced 'boxed': true), but it's an option.

Having 'boxed': true by default would be just an implementation choice, nothing to worry about. (When I said the arguments would be the configuration, having a boxed struct as the argument would fit the description just as well).

...

I was hoping that by converting object-add in this series, and the CLI options soon afterwards, it would be very obvious if you forget to change the schema because your new property just wouldn't work (at least not during creation).

Converting the CLI options is not entirely trivial due to -readconfig and friends, so I was expecting that to last until that part of my 6.0 keyval work goes in. (It's almost ready for posting BTW, https://gitlab.com/bonzini/qemu/-/commit/b59288c86c).

As soon as we have an idea of what we want UserCreatable to look in the end, on both the QAPI side and the object implementation side. That's also the part where we have the biggest need to document the schema. With that part at least roughly sketched out (no code needed), I'm okay with this series going in.

I still don't like the triplication, but as George Michael puts it I just gotta have faith---because I must admit, I'm positively surprised at the ideas that came out of the discussion.

Paolo

-- Eduardo

Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:

On 01/12/20 23:08, Eduardo Habkost wrote:

...

...

Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore.

Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set).

Yes.

Are internal uses mostly just right after object creation, or do we make a lot of use of them during runtime?

...

...

Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties.

I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system.

Yeah, "all about exposing properties" includes internal usage. And you're right that some "phase 3" monitor commands do work at the property level (mostly "info qtree", but also "qom-get" because there are some cases of public run-time properties).

...

I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu.

-cpu is not a problem since it's generally created with a static configuration (now done with global properties, in the future it could be a struct).

-machine and -device in principle could be done the same way as -object, just through a different registry (_not_ a huge struct; that's an acceptable stopgap for -object but that's it). The static aka field properties would remain as read-only, with defaults moved to instance_init or realize. But there would be again "triplication" with a trivial conversion:

1) in the QAPI schema, e.g. 'num_queues': 'int16'

2) in the struct, "int16_t num_queues;"

This one is optional, you can use the QAPI type even in the run-time state. I guess this goes back to how much separation you want between the configuration and the internal state.

3) in the realize function,

s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;

So having a mechanism for defaults in the QAPI schema would be good. Maybe 'num_queues': { 'type': 'int16', 'default': '8' }?

Defaults have been on the QAPI wishlist for a long time, and everyone agrees that it would be nice to have them. Maybe it's time to finally implement them.

I also need to review more the part of this code with respect to the application of global properties. I wonder if there are visitor tricks that we can do, so that global properties keep working but correspond to QAPI fields instead of QOM properties.

Kevin

On Wed, Dec 02, 2020 at 10:30:11AM +0100, Paolo Bonzini wrote:

On 01/12/20 23:08, Eduardo Habkost wrote:

...

...

Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore.

Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set).

Yes.

...

...

Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties.

I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system.

Yeah, "all about exposing properties" includes internal usage. And you're right that some "phase 3" monitor commands do work at the property level (mostly "info qtree", but also "qom-get" because there are some cases of public run-time properties).

I still disagree on the "all about" part even for internal usage. But this shouldn't really matter for this discussion, I guess.

...

I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu.

-cpu is not a problem since it's generally created with a static configuration (now done with global properties, in the future it could be a struct).

It is a problem if it requires manually converting existing code defining CPU properties and we don't have a transition plan.

-machine and -device in principle could be done the same way as -object, just through a different registry (_not_ a huge struct; that's an acceptable stopgap for -object but that's it). The static aka field properties would remain as read-only, with defaults moved to instance_init or realize. But there would be again "triplication" with a trivial conversion:

1) in the QAPI schema, e.g. 'num_queues': 'int16'

2) in the struct, "int16_t num_queues;"

3) in the realize function,

s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;

So having a mechanism for defaults in the QAPI schema would be good. Maybe 'num_queues': { 'type': 'int16', 'default': '8' }?

Would a -device conversion also involve non-user-creatable devices, or would we keep existing internal usage of QOM properties? Even if it's just for user-creatable devices, getting rid of QOM property usage in devices sounds like a very ambitious goal. I'd like us to have a good transition plan, in addition to declaring what's our ideal end goal.

I also need to review more the part of this code with respect to the application of global properties. I wonder if there are visitor tricks that we can do, so that global properties keep working but correspond to QAPI fields instead of QOM properties.

Paolo

-- Eduardo

On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:

On 02/12/20 13:51, Eduardo Habkost wrote:

...

...

...

I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu.

-cpu is not a problem since it's generally created with a static configuration (now done with global properties, in the future it could be a struct).

It is a problem if it requires manually converting existing code defining CPU properties and we don't have a transition plan.

We do not have to convert everything _if_ for some objects there are good reasons to do programmatically-generated properties. CPUs might be one of those cases (or if we decide to convert them, they might endure some more code duplication than other devices because they have so many properties).

OK, we just need to agree on what the transition will look like when we do it. I think we should put as much care into transition/glue infrastructure as we put into the new infrastructure.

...

Would a -device conversion also involve non-user-creatable devices, or would we keep existing internal usage of QOM properties?

Even if it's just for user-creatable devices, getting rid of QOM property usage in devices sounds like a very ambitious goal. I'd like us to have a good transition plan, in addition to declaring what's our ideal end goal.

For user-creatable objects Kevin is doing work in lockstep on all classes; but once we have the infrastructure for QAPI object configuration schemas we can proceed in the other direction and operate on one device at a time.

With some handwaving, something like (see create_unimplemented_device)

DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);

qdev_prop_set_string(dev, "name", name); qdev_prop_set_uint64(dev, "size", size); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

might become something like

{ 'object': 'unimplemented-device', 'config': { 'name': 'str', 'size': 'size' }, }

DeviceState *dev = qapi_Unimplemented_new(&( (UnimplementedDeviceOptions) { .name = name, .size = size }, &error_fatal); object_unref(dev);

(i.e. all typesafe!) and the underlying code would be something like

[...]

Looks nice as end goal. Then, these are a few questions I would have about the transition plan: Would it require changing both device implementation and device users in lockstep? Should we have a compatibility layer to allow existing qdev_new()+qdev_prop_set_*() code to keep working after the devices are converted to the new system? If not, why not? If we add a compatibility layer, is the end goal to convert all existing qdev_new() users to the new system? If yes, why? If not, why not? What about subclasses? Would base classes need to be converted in lockstep with all subclasses? How would the transition process of (e.g.) PCI devices look like? -- Eduardo

On Wed, Dec 02, 2020 at 04:17:13PM +0100, Kevin Wolf wrote:

Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:

...

On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:

...

On 02/12/20 13:51, Eduardo Habkost wrote:

...

...

...

I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu.

-cpu is not a problem since it's generally created with a static configuration (now done with global properties, in the future it could be a struct).

It is a problem if it requires manually converting existing code defining CPU properties and we don't have a transition plan.

We do not have to convert everything _if_ for some objects there are good reasons to do programmatically-generated properties. CPUs might be one of those cases (or if we decide to convert them, they might endure some more code duplication than other devices because they have so many properties).

OK, we just need to agree on what the transition will look like when we do it. I think we should put as much care into transition/glue infrastructure as we put into the new infrastructure.

...

...

Would a -device conversion also involve non-user-creatable devices, or would we keep existing internal usage of QOM properties?

Even if it's just for user-creatable devices, getting rid of QOM property usage in devices sounds like a very ambitious goal. I'd like us to have a good transition plan, in addition to declaring what's our ideal end goal.

For user-creatable objects Kevin is doing work in lockstep on all classes; but once we have the infrastructure for QAPI object configuration schemas we can proceed in the other direction and operate on one device at a time.

With some handwaving, something like (see create_unimplemented_device)

DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);

qdev_prop_set_string(dev, "name", name); qdev_prop_set_uint64(dev, "size", size); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

might become something like

{ 'object': 'unimplemented-device', 'config': { 'name': 'str', 'size': 'size' }, }

DeviceState *dev = qapi_Unimplemented_new(&( (UnimplementedDeviceOptions) { .name = name, .size = size }, &error_fatal); object_unref(dev);

(i.e. all typesafe!) and the underlying code would be something like

[...]

...

Looks nice as end goal. Then, these are a few questions I would have about the transition plan:

Would it require changing both device implementation and device users in lockstep? Should we have a compatibility layer to allow existing qdev_new()+qdev_prop_set_*() code to keep working after the devices are converted to the new system? If not, why not?

Technically, it doesn't strictly require a lockstep update. You can have two code paths leading to a fully initialised device, one being the traditional way of setting properties and finally calling dc->realize, the other being a new method that takes the configuration in its parameters and also sets dev->realized = true at the end.

If at all possible, I would however prefer a lockstep update because having two paths is a weird intermediate state and the code paths could easily diverge. Keeping the old way around for a device also means that property setters are still doing two different jobs (initial configuration and updates at runtime).

I'd like to understand better how that intermediate state would look like and why there's risk of separate code paths diverging. Could we have an intermediate state that doesn't require any duplication and thus have no separate code paths that could diverge?

...

If we add a compatibility layer, is the end goal to convert all existing qdev_new() users to the new system? If yes, why? If not, why not?

My personal goal is covering -object and -device, i.e. the external interfaces. Converting purely internally created devices is not as interesting (especially as long as we focus only on object creation), but might be desirable for consistency.

I wonder how much consistency we will lose and how much confusion we'll cause if we end up with two completely separate methods for creating devices.

...

What about subclasses? Would base classes need to be converted in lockstep with all subclasses? How would the transition process of (e.g.) PCI devices look like?

I don't think so.

If you want to convert base classes first, you may need to take the path shown above where both initialisation paths coexist while the children are converted because instantiation of a child class involves setting properties of the base class. So you can only restrict these properties to runtime-only after all children have been converted.

The other way around might be easier: You will need to describe the properties of base classes in the QAPI schema from the beginning, but that doesn't mean that their initialisation code has to change just yet. The child classes will need to forward the part of their configuration that belongs to the base class. The downside is that this code will have to be changed again when the base class is finally converted.

So we have options there, and we can decide case by case which one is most appropriate for the specific class to be converted (depending on how many child classes exist, how many properties are inherited, etc.)

Right now it's hard for me to understand what those intermediate states would look like. It sounds like it requires too many complicated manual changes to be done by humans, and lots of room for mistakes when maintaining two parallel code paths. I'd prefer to delegate the translation job to a machine. In other words, I'd prefer to have compatibility layer(s) that would make the same implementation work with the both old-style and new-style APIs for creating devices. Maybe this would involve generating QAPI code from QOM/qdev property lists, and/or generating qdev property registration code from the QAPI schema, I don't know. Providing a compatibility layer would also help us be more confident that there are no gaps in the abstractions provided by the two systems (QOM properties and QAPI schema) that we still need to address. -- Eduardo

On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote:

Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:

...

...

...

Looks nice as end goal. Then, these are a few questions I would have about the transition plan:

Would it require changing both device implementation and device users in lockstep? Should we have a compatibility layer to allow existing qdev_new()+qdev_prop_set_*() code to keep working after the devices are converted to the new system? If not, why not?

Technically, it doesn't strictly require a lockstep update. You can have two code paths leading to a fully initialised device, one being the traditional way of setting properties and finally calling dc->realize, the other being a new method that takes the configuration in its parameters and also sets dev->realized = true at the end.

If at all possible, I would however prefer a lockstep update because having two paths is a weird intermediate state and the code paths could easily diverge. Keeping the old way around for a device also means that property setters are still doing two different jobs (initial configuration and updates at runtime).

I'd like to understand better how that intermediate state would look like and why there's risk of separate code paths diverging.

Could we have an intermediate state that doesn't require any duplication and thus have no separate code paths that could diverge?

The one requirement we have for an intermediate state is that it supports both interfaces: The well-know create/set properties/realize dance, and a new DeviceClass method, say .create(), that takes the configuration in parameters instead of relying on previously set properties.

I agree completely.

I assumed two separate implementations of transferring the configuration into the internal state. On second thought, this assumption is maybe wrong.

You can implement the new method as wrapper around the old way: It could just set all the properties and call realize. Of course, you don't win much in terms of improving the class implementation this way, but just support the new interface, but I guess it can be a reasonable intermediate step to resolve complicated dependencies etc.

It would be much nicer to do the wrapper the other way round, i.e. setting properties before the device is realized would update a configuration struct and realize would then call .create() with that struct. To me, this sounds much harder, though also a more useful state.

Comment about this below (look for [1]).

As you have worked a lot with properties recently, maybe you have a good idea how we could get an intermediate state closer to this?

I'd have to re-read this whole thread and think about it.

...

...

...

If we add a compatibility layer, is the end goal to convert all existing qdev_new() users to the new system? If yes, why? If not, why not?

My personal goal is covering -object and -device, i.e. the external interfaces. Converting purely internally created devices is not as interesting (especially as long as we focus only on object creation), but might be desirable for consistency.

I wonder how much consistency we will lose and how much confusion we'll cause if we end up with two completely separate methods for creating devices.

I do think we should follow through and convert everything. It's just not my main motivation, and if the people who work more with qdev think it's better to leave that part unchanged (or that it won't make much of a difference), I won't insist.

This worries me. Converting thousands of lines of code that don't involve user-visible interfaces seems complicated and maybe pointless. On the other hand, having two separate APIs for creating objects internally would cause confusion. Maybe we should accept the fact that the 2 APIs will exist, and address the confusion part: we should guarantee the two APIs to be 100% equivalent, except for the fact that the newer one gives us type safety in the C code. I'd like to avoid a case like qdev vs QOM APIs, where they have similar but slightly different features, and nobody knows which one to use.

...

...

...

What about subclasses? Would base classes need to be converted in lockstep with all subclasses? How would the transition process of (e.g.) PCI devices look like?

I don't think so.

If you want to convert base classes first, you may need to take the path shown above where both initialisation paths coexist while the children are converted because instantiation of a child class involves setting properties of the base class. So you can only restrict these properties to runtime-only after all children have been converted.

The other way around might be easier: You will need to describe the properties of base classes in the QAPI schema from the beginning, but that doesn't mean that their initialisation code has to change just yet. The child classes will need to forward the part of their configuration that belongs to the base class. The downside is that this code will have to be changed again when the base class is finally converted.

So we have options there, and we can decide case by case which one is most appropriate for the specific class to be converted (depending on how many child classes exist, how many properties are inherited, etc.)

Right now it's hard for me to understand what those intermediate states would look like. It sounds like it requires too many complicated manual changes to be done by humans, and lots of room for mistakes when maintaining two parallel code paths. I'd prefer to delegate the translation job to a machine.

Maybe devices are in a better shape, but my conclusion from user creatable objects is that it needs to be done by a human.

I agree with the "done by a human part", but I prefer a different approach: making the APIs converge (using human work), and then making an automated transition. More on this below:

Even just writing a schema for an existing object without actually changing its code (i.e. what this series does for user creatable objects) involves:

* Figuring out which properties even exist.

I guess if you know your way around QOM, this could be automatically be generated for the common case. If property definitions are conditional or dynamic, you'll probably miss them.

With qdev static properties and class-level properties, this stage is easy.

* Finding the right data type for each property.

The const char *type passed to object_(class_)property_add() is often enough wrong that it becomes useless. If object_property_add_<type> is used, chances are that you know the right type, but strings might actually be hidden enums. If not, figuring out the type involves analysing the setter and getter callbacks.

With object_property_add_*() and object_class_property_add_*(), this part is very difficult. With qdev static properties (now known as "field properties" in QOM), this stage is easy. Luckily, most devices use qdev static properties.

* Finding out which properties are mandatory and which are optional.

This is usually some handwritten check in complete/realize that returns an error. Sometimes it's just a segfault that happens sooner or later if the property hasn't been set.

This stage is tricky. But we can start by: - Making all of them optional by default (which is true); - Adding a flag to allow us to declare properties as mandatory.

* Finding the default for documentation.

There are multiple ways to do this. It's code, not data.

In this case, qdev static properties / field properties make that job possible.

* Writing (preferably good) documentation for the property.

This part will require manual work.

I see very little opportunity for automating a significant part of this.

I see a big opportunity to automate this if the device is already using the field property system (which is true for most of the devices). The remaining work that needs to be done by humans is converting existing dynamic properties to field properties. About doing the compatibility layer the other way around[1]: if conversion from qdev properties to QAPI schema is automated, we can simply choose a point in time where the results of the conversion would be saved in the git tree, and the original property list would be deleted from the C source. Then, the compatibility layer would work the other way around.

Once you have this information, going to the intermediate state where .create() is just a wrapper that sets properties and calls realize is fairly easy. Maybe we can have QAPI support for this so that you can request generation of the wrapper function in the schema. Then you would just have to set the pointer to it in .class_init.

Agreed.

...

In other words, I'd prefer to have compatibility layer(s) that would make the same implementation work with the both old-style and new-style APIs for creating devices. Maybe this would involve generating QAPI code from QOM/qdev property lists, and/or generating qdev property registration code from the QAPI schema, I don't know.

Providing a compatibility layer would also help us be more confident that there are no gaps in the abstractions provided by the two systems (QOM properties and QAPI schema) that we still need to address.

qdev properties could be more useful to generate at least a skeleton schema from than generic QOM properties. But there will still be large parts that a human needs to do.

I agree there's lot of manual work to do. My proposal is doing the manual work during in the conversion of dynamic properties to field properties. Conversion from field properties to the new API then would be automated.

My concern with the compatibility layer is just that it could happen more easily that we're stuck with it forever.

I hear you. I'm a pessimistic regarding this. I believe we _will_ be stuck in the transition forever (or for so long that it will feel like forever). But with a good transition/compatibility layer, that transition stage will be less painful. I also believe that writing compatibility layer(s) would be less work than writing and reviewing patches doing manual conversion of device code directly to the new API. -- Eduardo

On Thu, Dec 03, 2020 at 07:46:29AM +0100, Gerd Hoffmann wrote:

Hi,

...

It would be much nicer to do the wrapper the other way round, i.e. setting properties before the device is realized would update a configuration struct and realize would then call .create() with that struct. To me, this sounds much harder, though also a more useful state.

Well, in some places we already have separate config structs. We have NICConf for example, which is typically used like this:

struct USBNetState { USBDevice dev; [ ... ] NICConf conf; [ ... ] };

and

static Property net_properties[] = { DEFINE_NIC_PROPERTIES(USBNetState, conf), DEFINE_PROP_END_OF_LIST(), };

So I think we could:

(1) move *all* properties into structs. (2) generate those structs from qapi schemas. (3) generate Property lists (or functions with object_class_property_add_*() calls) from qapi schema.

We could then convert devices one-by-one without breaking anything or needing two code paths essentially doing the same thing in two different ways.

Sounds great to me. This can also work the other way around for devices that weren't converted yet: we should be able to generate a QAPI schema from the property lists. -- Eduardo

Am 03.12.2020 um 12:11 hat Paolo Bonzini geschrieben:

On 02/12/20 18:35, Kevin Wolf wrote:

...

...

Could we have an intermediate state that doesn't require any duplication and thus have no separate code paths that could diverge?

The one requirement we have for an intermediate state is that it supports both interfaces: The well-know create/set properties/realize dance, and a new DeviceClass method, say .create(), that takes the configuration in parameters instead of relying on previously set properties.

I assumed two separate implementations of transferring the configuration into the internal state. On second thought, this assumption is maybe wrong.

You can implement the new method as wrapper around the old way: It could just set all the properties and call realize. Of course, you don't win much in terms of improving the class implementation this way, but just support the new interface, but I guess it can be a reasonable intermediate step to resolve complicated dependencies etc.

I sketched something at https://wiki.qemu.org/Features/QOM-QAPI_integration.

The main difference with what we discussed so far is that it doesn't subsume the complete/realize step, only the setting of properties. The idea is that user_creatable_add_type does the following:

- calls an oc->configure method on every superclass of the object

- takes what's left of the input visitor and uses it to set properties

- then calls ucc->complete

So in the end the only new step is the first. If the first two steps are bundled in a new function object_configure, they can be reused for devices, machines and accelerators.

The QAPI code generator can also easily wrap them into a C API for QOM object creation.

I glossed completely over the generation of properties within the QAPI code generator. Making properties read-only (or, in the field-properties world, having a default allow_set of "return false") is already a nice improvement over

I don't think this is an intermediate state like Eduardo wants to have. Creating the object, then setting properties, then realize [1] will fail after your change. But keeping it working was the whole point of the exercise. I'm also not really sure why you go from RngEgdOptions to QObject to a visitor, only to reconstruct RngEgdOptions at the end. I think the class implementations should have a normal C interface without visitors and we should be able to just pass the existing RngEgdOptions object (or the individual values for its fields for 'boxed': false). Kevin [1] Or complete for ucc, but the number of these is small enough that we don't really need any intermediate state, except maybe as a proof of concept for the later qdev conversion.

Am 03.12.2020 um 17:50 hat Paolo Bonzini geschrieben:

On 03/12/20 16:15, Kevin Wolf wrote:

...

I don't think this is an intermediate state like Eduardo wants to have. Creating the object, then setting properties, then realize [1] will fail after your change. But keeping it working was the whole point of the exercise.

With the sample code, you must remove object_class_property_set calls at the same time as you remove the setters. Usually that'd be when you convert to QAPI and oc->configure, but it doesn't have to be that way if there are good reasons not to do so.

Okay, thanks, I think I understand now. So I assume that in the common case, we'll never have the state that you describe, but we'll want to directly skip to QAPI generated code. But it's good to know that we can make smaller steps if we need to in more complicated cases.

Also, it still allows you to do so one class at a time, and I *think* the presence of subclasses or superclasses doesn't matter (only whether properties are still writable). We can use chardevs (see ChardevCommon in qapi/char.json) to validate that before tackling devices.

Yes, it looks like it should be working.

(In fact, this means that your series---plus -object and object_add conversion---would be good, pretty much unchanged, as a first step. The second would be adding oc->configure and object_configure, and converting all user-creatable objects to oc->configure. The third would involve QAPI code generation).

I think I'd want to do step 2 and 3 combined, because converting user-creatable objects to oc->configure means manually writing the configure function that will be generated from QAPI in step 3. Writing code just to throw it away isn't my favourite pastime.

...

I'm also not really sure why you go from RngEgdOptions to QObject to a visitor, only to reconstruct RngEgdOptions at the end.

The two visits are just because you cannot create an input visitor directly on C data. I stole that from your patch 18/18 actually, just with object_new+object_configure instead of user_creatable_add_type.

But I wouldn't read too much in the automatically-generated *_new functions since they are already in QAPI code generator territory. Instead the basic object_configure idea can be applied even without having automatic code generation.

Yes, I was just wondering why we're going through visitors at all. But this is what provides the compatibility with the old property system, so it makes sense if you need an intermediate step.

...

I think the class implementations should have a normal C interface without visitors and we should be able to just pass the existing RngEgdOptions object (or the individual values for its fields for 'boxed': false).

Sure, however that requires changes to the QAPI code generator which was only item (3) in your list list. Until then you can already work with a visitor interface:

void rng_egd_configure(Object *obj, Visitor *v, Error **errp) { RngEgd *s = RNG_EGD(obj); s->config = g_new0(MemoryBackendOptions, 1); visit_type_MemoryBackendOptions(v, NULL, &s->config, errp);

s->config->share = (s->config->has_share ? s->config->share : false); ... }

but if you had a QAPI description

{ 'object': 'RngEgd', 'qom-type': 'rng-egd', 'configuration': 'RngEgdOptions', 'boxed': true }

the QAPI generator could produce the oc->configure implementation. Similar to commands, that implementation would be an unmarshaling wrapper that calls out to the natural C interface:

void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp); { Error *local_err = NULL; g_autoptr(MemoryBackendOptions) *config = g_new0(MemoryBackendOptions, 1); visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err); if (local_err) { error_propagate(errp, local_err); return; } qom_rng_egd_configure(RNG_EGD(obj), config, errp); }

void qom_rng_egd_configure(RngEng *s, RngEgdOptions *config, Error **errp) { config->share = (config->has_share ? config->share : false); ... s->config = QAPI_CLONE(RngEgdOptions, config); }

Yes, exactly. Kevin

On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote:

On 03/12/20 16:15, Kevin Wolf wrote:

...

I don't think this is an intermediate state like Eduardo wants to have. Creating the object, then setting properties, then realize [1] will fail after your change. But keeping it working was the whole point of the exercise.

With the sample code, you must remove object_class_property_set calls at the

Do you mean object_property_set()?

same time as you remove the setters. Usually that'd be when you convert to QAPI and oc->configure, but it doesn't have to be that way if there are good reasons not to do so.

Having two (or more) similar but incompatible APIs to do exactly the same thing is a mistake we did before, and I wouldn't like us to repeat it. If we can keep qdev_new() + object_property_set() + realize working after the device is converted, we should. I believe we can. If we can make object_new_configure() work with all (or most) device types before we manually convert them to the new system, we should. I believe we can. We may be able avoid these questions with -object because converting all backends at the same time is doable. With devices, API usability and maintainability during the transition period (which could be very long) needs to be taken into account. -- Eduardo

On Thu, Dec 03, 2020 at 07:10:37PM +0100, Paolo Bonzini wrote:

On 03/12/20 18:52, Eduardo Habkost wrote:

...

On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote:

...

On 03/12/20 16:15, Kevin Wolf wrote:

...

I don't think this is an intermediate state like Eduardo wants to have. Creating the object, then setting properties, then realize [1] will fail after your change. But keeping it working was the whole point of the exercise.

With the sample code, you must remove object_class_property_set calls at the

Do you mean object_property_set()?

Yes.

...

...

same time as you remove the setters. Usually that'd be when you convert to QAPI and oc->configure, but it doesn't have to be that way if there are good reasons not to do so.

Having two (or more) similar but incompatible APIs to do exactly the same thing is a mistake we did before, and I wouldn't like us to repeat it.

If we can keep qdev_new() + object_property_set() + realize working after the device is converted, we should. I believe we can.

You can. If you want to do that, you have to give up on removing the setters; but that's not so beneficial for devices because they already use static properties anyway. They have much less boilerplate than -object objects.

Understood. We can also get rid of most setters in -object backends using field properties. Maybe not a necessary step, but a useful intermediate step in case the new API takes time to be ready.

...

If we can make object_new_configure() work with all (or most) device types before we manually convert them to the new system, we should. I believe we can.

Yup, object_new_configure() is the low-level visitor-based API and therefore it supports both properties and oc->configure.

Perfect. That part was not clear yet to me (I just skimmed to the example code you posted on the wiki).

...

We may be able avoid these questions with -object because converting all backends at the same time is doable. With devices, API usability and maintainability during the transition period (which could be very long) needs to be taken into account.

I think we're in violent agreement. :)

Paolo

-- Eduardo

On 02/12/20 11:27, Kevin Wolf wrote:

...

Declaring read-only QOM properties is trivial.

Trivial sounds like it's something the computer should be doing.

Possibly, but not necessarily. There's always a cost to automatic code generation. If things are _too_ trivial and easy to get right, the cost of automatic code generation exceeds the cost of doing things by hand. You pretty much proved that ucc->complete is hard enough to get right, that it benefits from code generation. But I am a bit more conservative about extending that to the rest of QOM.

...

I'm just worried about having yet another incomplete transition.

Would you really feel at home in a QEMU without incomplete transitions?

One can always dream. :)

But since you had already posted RFC patches a while ago to address this, I considered it solved.

Yup, I posted the non-RFC today.

1. This series (mostly for documentation and introspection)

2. -object and HMP object_add (so that we get QAPI's validation, and to make sure that forgetting to update the schema means that the new options just doesn't work)

3. Create a separate 'object' entity in QAPI, generate ucc->complete implementations that call a create function in the C source code with type-specific parameters (like QMP command handlers)

If we agree on all of these that's already a pretty good place to be in. The decreasing length of the replies to the thread suggests that we are. I wouldn't mind an example of (3) that is hand-coded and may only work for one object type (even a simple one such as iothread), to show what the generated QAPI code would look like. It's not a blocker as far as I am concerned, but it would be nice. More important, Markus and John should agree with the plan before (1) is committed. That may also require the aforementioned example, but it's up to them.

What's still open: Should QAPI cover run-time properties, too? Should run-time properties even exist at all in the final state? What is the exact QAPI representation of everything? (The last one includes that I need to have a closer look at how QAPI can best be taught about inheritance.)

Run-time properties will exist but they will probably be cut down substantially, and most of them will be read-only (they already are as far as external code is concerned, i.e. they have a setter but qom-set always fails because it's called too late). Paolo