Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> >> >> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
> >> >> command
> >> >> >> >>
> >> >> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800,
> address@hidden
> >> >> wrote:
> >> >> >> >> > From: Gonglei <address@hidden>
> >> >> >> >> >
> >> >> >> >> > 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": "/address@hidden"}}
> >> >> >> >> > <- { "return": {} }
> >> >> >> >> >
> >> >> >> >> > Signed-off-by: Gonglei <address@hidden>
> >> >> >> >> > Signed-off-by: Chenliang <address@hidden>
> >> >> >> >> > ---
> >> >> >> >> >  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.
> 
Hmm... but not every device has reset function. 
This changing will be a big surgery IMO.

> 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);

Agreed.

Best regards,
-Gonglei