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

[Gonglei (Arei)]

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, 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.

Best regards,
-Gonglei