"Gonglei (Arei)" <arei.gong...@huawei.com> writes:
> Hi, Markus
>> >
>> >> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
>> >> command
>> >> >> >>
>> >> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gong...@huawei.com
>> >> wrote:
>> >> >> >> > From: Gonglei <arei.gong...@huawei.com>
>> >> >> >> >
>> >> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
>> >> >> >> >
>> >> >> >> > Example QMP command:
>> >> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
>> >> >> >> > "bootindex":
>> >> >> >> 1, "suffix": "/disk@0"}}
>> >> >> >> > <- { "return": {} }
>> >> >> >> >
>> >> >> >> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
>> >> >> >> > Signed-off-by: Chenliang <chenlian...@huawei.com>
>> >> >> >> > ---
>> >> >> >> > qapi-schema.json | 16 ++++++++++++++++
>> >> >> >> > qmp-commands.hx | 24 ++++++++++++++++++++++++
>> >> >> >> > qmp.c | 17 +++++++++++++++++
>> >> >> >> > 3 files changed, 57 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> >> > index b11aad2..a9ef0be 100644
>> >> >> >> > --- a/qapi-schema.json
>> >> >> >> > +++ b/qapi-schema.json
>> >> >> >> > @@ -1704,6 +1704,22 @@
>> >> >> >> > { 'command': 'device_del', 'data': {'id': 'str'} }
>> >> >> >> >
>> >> >> >> > ##
>> >> >> >> > +# @set-bootindex:
>> >> >> >> > +#
>> >> >> >> > +# set bootindex of a devcie
>> >> >> >> > +#
>> >> >> >> > +# @id: the name of the device
>> >> >> >> > +# @bootindex: the bootindex of the device
>> >> >> >> > +# @suffix: #optional a suffix of the device
>> >> >> >> > +#
>> >> >> >> > +# Returns: Nothing on success
>> >> >> >> > +# If @id is not a valid device, DeviceNotFound
>> >> >> >> > +#
>> >> >> >> > +# Since: 2.2
>> >> >> >> > +##
>> >> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
>> >> >> >> > int', '*suffix':
>> >> >> >> 'str'} }
>> >> >> >> > +
>> >> >> >> > +##
>> >> >> >>
>> >> >> >> I wonder if we could simply use qom-set for that. How many devices
>> >> >> >> actually support having multiple bootindex entries with different
>> >> >> >> suffixes?
>> >> >> >>
>> >> >> > AFAICT, the floppy device support two bootindex with
>> >> >> different suffixes.
>> >> >>
>> >> >> Block device models commonly a single block device. By convention,
>> >> >> property "drive" is the backend, and property "bootindex" the
>> >> >> bootindex,
>> >> >> if the device model supports that.
>> >> >>
>> >> >> The device ID suffices to identify a block device for such device
>> >> >> models.
>> >> >>
>> >> >> The floppy device model is an exception. It folds multiple real-world
>> >> >> objects into one: the controller and the actual drives. Have a look at
>> >> >> -device isa-fdc,help:
>> >> >>
>> >> >> isa-fdc.iobase=uint32
>> >> >> isa-fdc.irq=uint32
>> >> >> isa-fdc.dma=uint32
>> >> >> isa-fdc.driveA=drive
>> >> >> isa-fdc.driveB=drive
>> >> >> isa-fdc.bootindexA=int32
>> >> >> isa-fdc.bootindexB=int32
>> >> >> isa-fdc.check_media_rate=on/off
>> >> >>
>> >> >> The properties ending with 'A' or 'B' apply to the first and the second
>> >> >> drive, the others to the controller.
>> >> >>
>> >> >> Unfortunately, this means the device ID by itself doesn't identify the
>> >> >> floppy device.
>> >> >>
>> >> > Yes.
>> >> >
>> >> >> Arguably, this is lousy modeling --- we really should model separate
>> >> >> real-world objects as separate objects. But making floppies pretty (or
>> >> >> even sane) isn't anyone's priority nowadays.
>> >> >>
>> >> > Hmm... Agreed.
>> >> >
>> >> >> Since qom-set works on *properties*, I can't see why it couldn't be
>> >> >> used
>> >> >> for changing bootindexes. There is no ambiguity even with floppy.
>> >> >
>> >> > Sorry?
>> >> >
>> >> >> You either set bootindexA or bootindexB. No need to fuzz around with
>> >> >> suffixes. Or am I missing something?
>> >> >
>> >> > Your mean that we just need to think about change one bootindex? Either
>> >> > set bootindexA or bootindexB, do not need pass suffix for identifying?
>> >>
>> >> Eduardo suggested to think about using qom-set to update the bootindex.
>> >>
>> >> Could look like
>> >>
>> >> { "execute": "qom-set",
>> >> "arguments": { "path": "/machine/unattached/device[15]",
>> >> "property": "bootindexA", "value": 1 } }
>> >>
>> >> Demonstrates an existing, well-defined way to identify the bootindex to
>> >> change. Do we really want to invent another one, based on suffix?
>> >>
>> >> The value of "path" is the QOM path. I can't remember offhand how to go
>> >> from qdev ID to QOM path. Onboard devices like isa-fdc don't have one
>> >> anyway.
>> >>
>> >> I also don't remember whether there's a nicer QOM path than this one.
>> >
>> > Thanks for your explaining. TBH I haven't used "qom-set" before, so I
>>
>> Few people have :)
>>
>> > don't know what your mean (like Eduardo, sorry again).
>> >
>> > But now, I have a look at the implement of "qom-set" command, and
>> > I find the command is just change a device's property value, do not have
>> > any other logic. For my case, we really change value of the global
>> > fw_boot_order list,
>> > which the device's bootindex property worked for actually. Only in this
>> > way,
>> > we can attach our original target.
>>
>> Why can't we delay the logic to the next reboot? Let me explain.
>>
>> Current code starts with empty fw_boot_order, then lets device realize
>> add to it. Unplugging a device leaves a dangling DeviceState pointer in
>> the list (I think).
>>
> Yes.
>
>> Could we instead build, use and free the list during reboot? That way,
>> qom-set of bootindex affects the next reboot without additional logic.
>
> Yes, we can do this, but it will be too complicated. Firstly the device
> realize
> will not reattach during reboot. So, we should check all the device of
> the global list during reboot,
> but the DeviceState haven't bootindex property, we should cast it into
> idiographic
> device for getting the changed bootindex, such as " VirtIONet *n =
> VIRTIO_NET(dev)" ( this will be a trouble
> for different devices), then we can change fw_boot_order list's
> bootindex using new bootindex,
> then reorder all bootindexes in list. Am I wrong?
>
> So, I don't think it is a good idea.
Moving add_boot_device_path() from realize to reset could do the trick.
Anyway, delaying the logic is not necessary for use of qom-set.
Ordinary qdev properties use common setter functions. QOM is more
general: it lets you define your own setter. For example,
device_initfn() defines property "realized" like this:
object_property_add_bool(obj, "realized",
device_get_realized, device_set_realized, NULL);
