RFC: QMP configuration - allocating/setting qdev array properties?

RFC: QMP configuration - allocating/setting qdev array properties?

[Mirela Grujic]

Hi,


While working on a prototype and configuring a whole machine using QMP 
we run into the following scenario.


Some device models use array properties. The array is allocated when 
len-<arrayname> property is set, then, individual elements of the array 
can be set as any other property (see description above the 
DEFINE_PROP_ARRAY definition in qdev-properties.h for more details). We 
need to do both (allocate the array and set its elements) before the 
device can be realized. Attempting to set len-<arrayname> and array 
elements in a single device_add command does not work because the order 
of setting properties is not guaranteed, i.e. we're likely attempting to 
set an element of the array that's not yet allocated.

Allowing the device initialize and realize phases to be split would 
solve this problem. For example, the device_add would be issued with 
'realized=false', we can set the len-<arrayname> in the same device_add 
command or a following qom-set command, then we would use a sequence of 
qom-set commands to set array elements, and at the end, we would realize 
the device by issuing qom-set path=<device_id> property=realized 
value=true. This is what we currently do in our prototype.

Another situation where we found that splitting initialize and realize 
phases would solve a problem has been presented here: 
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07272.html


We would appreciate your feedback, any other proposals for solving both 
problems are welcome.


Thanks,

Mirela

Re: RFC: QMP configuration - allocating/setting qdev array properties?

[Damien Hedde]

Hi Mirela,

On 1/11/22 17:54, Mirela Grujic wrote:
> Hi,
> 
> 
> While working on a prototype and configuring a whole machine using QMP 
> we run into the following scenario.
> 
> 
> Some device models use array properties. The array is allocated when 
> len-<arrayname> property is set, then, individual elements of the array 
> can be set as any other property (see description above the 
> DEFINE_PROP_ARRAY definition in qdev-properties.h for more details). We 
> need to do both (allocate the array and set its elements) before the 
> device can be realized. Attempting to set len-<arrayname> and array 
> elements in a single device_add command does not work because the order 
> of setting properties is not guaranteed, i.e. we're likely attempting to 
> set an element of the array that's not yet allocated.

It happens because device options are stored in an optdict. When this 
optdict is traversed to set the qdev-properties, no specific order is used.

Better json format support would probably solve this issue in the 
long-term. But right now, we are stuck with the optdict in the middle 
which do not support advanced structure like lists or dictionaries.

We could solve this by being more "smart" in when setting the 
properties. I'm not sure we can be really smart here and detect which 
options is an array length but we could at least have some heuristic and 
for example: set first "len-xxx" properties so that array will be 
allocated before being filled.

> 
> Allowing the device initialize and realize phases to be split would 
> solve this problem. For example, the device_add would be issued with 
> 'realized=false', we can set the len-<arrayname> in the same device_add 
> command or a following qom-set command, then we would use a sequence of 
> qom-set commands to set array elements, and at the end, we would realize 
> the device by issuing qom-set path=<device_id> property=realized 
> value=true. This is what we currently do in our prototype.

I think that is a bad idea. Because then the user would have access to a 
"not-realized" device (which is really a not-constructed object).
It could then do anything with the object (which might work or not 
might). And at the end, maybe realize will fail and that would leave 
qemu in a inconsistent state: the object will be used somewhere and at 
the same time it will be a state where it is not usable.

Thanks,
Damien

Re: RFC: QMP configuration - allocating/setting qdev array properties?

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Hi Mirela,
>
> On 1/11/22 17:54, Mirela Grujic wrote:
>> Hi,
>> 
>> While working on a prototype and configuring a whole machine using
>> QMP we run into the following scenario.
>> 
>> Some device models use array properties.

A gift that keeps on giving...

>>                                          The array is allocated when
>> len-<arrayname> property is set, then, individual elements of the
>> array can be set as any other property (see description above the 
>> DEFINE_PROP_ARRAY definition in qdev-properties.h for more
>> details). We need to do both (allocate the array and set its
>> elements) before the device can be realized. Attempting to set
>> len-<arrayname> and array elements in a single device_add command
>> does not work because the order of setting properties is not
>> guaranteed, i.e. we're likely attempting to set an element of the
>> array that's not yet allocated.
>
> It happens because device options are stored in an optdict. When this
> optdict is traversed to set the qdev-properties, no specific order is
> used.

To be precise: it's stored in a QDict[*]

qdev_device_add_from_qdict() sets properties with
object_set_properties_from_qdict(), which iterates over the QDict in
unspecified order.

> Better json format support would probably solve this issue in the
> long-term. But right now, we are stuck with the optdict in the middle 
> which do not support advanced structure like lists or dictionaries.

I figure you mean actual array-valued properties, like

    'foo': [ 1, 2, 3 ]

instead of

    'len-foo': 3, 'len[0]': 1, 'len[1]': 2, 'len[2]': 3

> We could solve this by being more "smart" in when setting the
> properties. I'm not sure we can be really smart here and detect which 
> options is an array length but we could at least have some heuristic
> and for example: set first "len-xxx" properties so that array will be 
> allocated before being filled.

Ugh!

Another stop gap solution could be making QDict iterate in insertion
order, like Python dict does since 3.6.

>> Allowing the device initialize and realize phases to be split would 
>> solve this problem. For example, the device_add would be issued with
>> 'realized=false', we can set the len-<arrayname> in the same
>> device_add command or a following qom-set command, then we would use
>> a sequence of qom-set commands to set array elements, and at the
>> end, we would realize the device by issuing qom-set path=<device_id>
>> property=realized value=true. This is what we currently do in our
>> prototype.
>
> I think that is a bad idea. Because then the user would have access to
> a "not-realized" device (which is really a not-constructed object).
> It could then do anything with the object (which might work or not
> might). And at the end, maybe realize will fail and that would leave 
> qemu in a inconsistent state: the object will be used somewhere and at
> the same time it will be a state where it is not usable.

I don't regard a not-realized device as not-constructed object.
Construction is qdev_new().  We then configure by setting properties.
Realization makes the device accessible to the guest.

-device / device_add fuse all this into one operation: create device,
set the properties specified by the user, realize.

C code need not fuse like this.  There are places where we create
devices, then abandon them, i.e. destroy them without realizing.

I share your concern that providing the user the basic operations
unfused might expose more bugs.

In today's usage, a fused operation is all we need.  But when wiring up
complex composite devices in configuration rather than C code, we may
get to the point where we need the basic operations unfused.


[*] -device / device_add with a non-JSON argument go to QDict via
QemuOpts.  Doesn't matter here.

Re: RFC: QMP configuration - allocating/setting qdev array properties?

[John Snow]

On Wed, Jan 19, 2022 at 5:12 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Damien Hedde <damien.hedde@greensocs.com> writes:
>
> > Hi Mirela,
> >
> > On 1/11/22 17:54, Mirela Grujic wrote:
> >> Hi,
> >>
> >> While working on a prototype and configuring a whole machine using
> >> QMP we run into the following scenario.
> >>
> >> Some device models use array properties.
>
> A gift that keeps on giving...
>
> >>                                          The array is allocated when
> >> len-<arrayname> property is set, then, individual elements of the
> >> array can be set as any other property (see description above the
> >> DEFINE_PROP_ARRAY definition in qdev-properties.h for more
> >> details). We need to do both (allocate the array and set its
> >> elements) before the device can be realized. Attempting to set
> >> len-<arrayname> and array elements in a single device_add command
> >> does not work because the order of setting properties is not
> >> guaranteed, i.e. we're likely attempting to set an element of the
> >> array that's not yet allocated.
> >
> > It happens because device options are stored in an optdict. When this
> > optdict is traversed to set the qdev-properties, no specific order is
> > used.
>
> To be precise: it's stored in a QDict[*]
>
> qdev_device_add_from_qdict() sets properties with
> object_set_properties_from_qdict(), which iterates over the QDict in
> unspecified order.
>
> > Better json format support would probably solve this issue in the
> > long-term. But right now, we are stuck with the optdict in the middle
> > which do not support advanced structure like lists or dictionaries.
>
> I figure you mean actual array-valued properties, like
>
>     'foo': [ 1, 2, 3 ]
>
> instead of
>
>     'len-foo': 3, 'len[0]': 1, 'len[1]': 2, 'len[2]': 3
>
> > We could solve this by being more "smart" in when setting the
> > properties. I'm not sure we can be really smart here and detect which
> > options is an array length but we could at least have some heuristic
> > and for example: set first "len-xxx" properties so that array will be
> > allocated before being filled.
>
> Ugh!
>
> Another stop gap solution could be making QDict iterate in insertion
> order, like Python dict does since 3.6.
>

I like this idea, I think. Are there any possible downsides here?
Making the order more 'stable' in one regard might lead to people
trusting it "too often" if there are other implementation details that
might impact the order ... but I don't actually have any examples
handy for that. It's just my fear.

--js

Re: RFC: QMP configuration - allocating/setting qdev array properties?

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Jan 19, 2022 at 5:12 AM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> Another stop gap solution could be making QDict iterate in insertion
>> order, like Python dict does since 3.6.
>>
>
> I like this idea, I think. Are there any possible downsides here?
> Making the order more 'stable' in one regard might lead to people
> trusting it "too often" if there are other implementation details that
> might impact the order ... but I don't actually have any examples
> handy for that. It's just my fear.

For what it's worth, it took Python just one release cycle to overcome
this fear :)