| Message ID | 1409906256-11368-25-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > Add a qom property with the same name 'bootindex', > when we remove it form qdev property, things will > continue to work just fine, and we can use qom features > which are not supported by qdev property. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/ide/qdev.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index efab95b..9e2ed40 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > return 0; > } > > +static void ide_dev_instance_init(Object *obj) > +{ > + DeviceState *dev = DEVICE(obj); > + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); > + > + device_add_bootindex_property(obj, &d->conf.bootindex, > + "bootindex", > + d->unit ? "/disk@1" : "/disk@0", > + &d->qdev, NULL); > +} > + > static int ide_hd_initfn(IDEDevice *dev) > { > return ide_dev_initfn(dev, IDE_HD); > @@ -238,6 +249,7 @@ static const TypeInfo ide_hd_info = { > .parent = TYPE_IDE_DEVICE, > .instance_size = sizeof(IDEDrive), > .class_init = ide_hd_class_init, > + .instance_init = ide_dev_instance_init, > }; > > static Property ide_cd_properties[] = { > @@ -260,6 +272,7 @@ static const TypeInfo ide_cd_info = { > .parent = TYPE_IDE_DEVICE, > .instance_size = sizeof(IDEDrive), > .class_init = ide_cd_class_init, > + .instance_init = ide_dev_instance_init, > }; > > static Property ide_drive_properties[] = { > @@ -282,6 +295,7 @@ static const TypeInfo ide_drive_info = { > .parent = TYPE_IDE_DEVICE, > .instance_size = sizeof(IDEDrive), > .class_init = ide_drive_class_init, > + .instance_init = ide_dev_instance_init, > }; If all TYPE_IDE_DEVICE subclasses have the same instance_init, why not just set ide_device_type_info.instance_init = ide_dev_instance_init?
> Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom property > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > Add a qom property with the same name 'bootindex', > > when we remove it form qdev property, things will > > continue to work just fine, and we can use qom features > > which are not supported by qdev property. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/ide/qdev.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > > index efab95b..9e2ed40 100644 > > --- a/hw/ide/qdev.c > > +++ b/hw/ide/qdev.c > > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, > IDEDriveKind kind) > > return 0; > > } > > > > +static void ide_dev_instance_init(Object *obj) > > +{ > > + DeviceState *dev = DEVICE(obj); > > + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); > > + > > + device_add_bootindex_property(obj, &d->conf.bootindex, > > + "bootindex", > > + d->unit ? "/disk@1" : "/disk@0", > > + &d->qdev, NULL); > > +} > > + > > static int ide_hd_initfn(IDEDevice *dev) > > { > > return ide_dev_initfn(dev, IDE_HD); > > @@ -238,6 +249,7 @@ static const TypeInfo ide_hd_info = { > > .parent = TYPE_IDE_DEVICE, > > .instance_size = sizeof(IDEDrive), > > .class_init = ide_hd_class_init, > > + .instance_init = ide_dev_instance_init, > > }; > > > > static Property ide_cd_properties[] = { > > @@ -260,6 +272,7 @@ static const TypeInfo ide_cd_info = { > > .parent = TYPE_IDE_DEVICE, > > .instance_size = sizeof(IDEDrive), > > .class_init = ide_cd_class_init, > > + .instance_init = ide_dev_instance_init, > > }; > > > > static Property ide_drive_properties[] = { > > @@ -282,6 +295,7 @@ static const TypeInfo ide_drive_info = { > > .parent = TYPE_IDE_DEVICE, > > .instance_size = sizeof(IDEDrive), > > .class_init = ide_drive_class_init, > > + .instance_init = ide_dev_instance_init, > > }; > > If all TYPE_IDE_DEVICE subclasses have the same instance_init, why not > just set ide_device_type_info.instance_init = ide_dev_instance_init? > OK. TBH I have not noticed this way, sorry. Great thanks for your help and reviewing, Eduardo. :) Best regards, -Gonglei
Hi, > Subject: RE: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom property > > > Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom > property > > > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gonglei@huawei.com wrote: > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > Add a qom property with the same name 'bootindex', > > > when we remove it form qdev property, things will > > > continue to work just fine, and we can use qom features > > > which are not supported by qdev property. > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > --- > > > hw/ide/qdev.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > > > index efab95b..9e2ed40 100644 > > > --- a/hw/ide/qdev.c > > > +++ b/hw/ide/qdev.c > > > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, > > IDEDriveKind kind) > > > return 0; > > > } > > > > > > +static void ide_dev_instance_init(Object *obj) > > > +{ > > > + DeviceState *dev = DEVICE(obj); > > > + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); > > > + > > > + device_add_bootindex_property(obj, &d->conf.bootindex, > > > + "bootindex", > > > + d->unit ? "/disk@1" : > "/disk@0", > > > + &d->qdev, NULL); > > > +} > > > + Oops, I found a thorny issue that the d->unit parameter had not been initialized in ide_dev_instance_init(). d->unit maybe is a random value, which will against the original purpose in this situation. What's your opinion, Eduardo? Thanks! Best regards, -Gonglei
On Tue, Sep 09, 2014 at 07:51:49AM +0000, Gonglei (Arei) wrote: > Hi, > > > Subject: RE: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom property > > > > > Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom > > property > > > > > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gonglei@huawei.com wrote: > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > Add a qom property with the same name 'bootindex', > > > > when we remove it form qdev property, things will > > > > continue to work just fine, and we can use qom features > > > > which are not supported by qdev property. > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > --- > > > > hw/ide/qdev.c | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > > > > index efab95b..9e2ed40 100644 > > > > --- a/hw/ide/qdev.c > > > > +++ b/hw/ide/qdev.c > > > > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, > > > IDEDriveKind kind) > > > > return 0; > > > > } > > > > > > > > +static void ide_dev_instance_init(Object *obj) > > > > +{ > > > > + DeviceState *dev = DEVICE(obj); > > > > + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); > > > > + > > > > + device_add_bootindex_property(obj, &d->conf.bootindex, > > > > + "bootindex", > > > > + d->unit ? "/disk@1" : > > "/disk@0", > > > > + &d->qdev, NULL); > > > > +} > > > > + > Oops, I found a thorny issue that the d->unit parameter had not been initialized > in ide_dev_instance_init(). d->unit maybe is a random value, which will against > the original purpose in this situation. > > What's your opinion, Eduardo? Thanks! It looks like you can call add_boot_device_path() only on realize, on IDE. If IDE is the only case, we could just change IDE to not use device_add_bootindex_property(), having its own getter/setter and a call to add_boot_device_path() on realize(). Or, to keep the code generic, we could just make get_boot_devices_list() query the device for the suffix, instead of requiring the suffix to be set before device_add_bootindex_property() is called. With something like: typedef struct BootDeviceInfo { int bootindex; const char *suffix; } BootDeviceInfo; struct FWBootEntry { QTAILQ_ENTRY(FWBootEntry) link; DeviceState *dev; BootDeviceInfo *bootdev; }; void add_boot_device_path(DeviceState *dev, BootDeviceInfo *info); void del_boot_device_path(BootDeviceInfo *info); void device_add_bootindex_property(Object *obj, const char *property, DeviceState *dev, BootDeviceInfo *info, Error **errp); This way, the suffix can be set between device_add_bootindex_property() and get_boot_devices_list() if necessary: static void somedevice_instance_init(Object *obj) { SomeDevice *somedev = SOME_DEVICE(obj); device_add_bootindex_property(obj, DEVICE(obj), &somedev->bootinfo, &error_abort); } static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) { [...] dev->bootinfo.suffix = dev->unit ? "/disk@1" : "/disk@0"; }
Hi, > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Subject: Re: [Qemu-devel] [PATCH v7 24/28] ide: add bootindex to qom > > > property > > > > > > > > On Fri, Sep 05, 2014 at 04:37:32PM +0800, arei.gonglei@huawei.com > wrote: > > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > > > Add a qom property with the same name 'bootindex', > > > > > when we remove it form qdev property, things will > > > > > continue to work just fine, and we can use qom features > > > > > which are not supported by qdev property. > > > > > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > > > > --- > > > > > hw/ide/qdev.c | 14 ++++++++++++++ > > > > > 1 file changed, 14 insertions(+) > > > > > > > > > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > > > > > index efab95b..9e2ed40 100644 > > > > > --- a/hw/ide/qdev.c > > > > > +++ b/hw/ide/qdev.c > > > > > @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, > > > > IDEDriveKind kind) > > > > > return 0; > > > > > } > > > > > > > > > > +static void ide_dev_instance_init(Object *obj) > > > > > +{ > > > > > + DeviceState *dev = DEVICE(obj); > > > > > + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); > > > > > + > > > > > + device_add_bootindex_property(obj, &d->conf.bootindex, > > > > > + "bootindex", > > > > > + d->unit ? "/disk@1" : > > > "/disk@0", > > > > > + &d->qdev, NULL); > > > > > +} > > > > > + > > Oops, I found a thorny issue that the d->unit parameter had not been > initialized > > in ide_dev_instance_init(). d->unit maybe is a random value, which will > against > > the original purpose in this situation. > > > > What's your opinion, Eduardo? Thanks! > > It looks like you can call add_boot_device_path() only on realize, on > IDE. If IDE is the only case, we could just change IDE to not use > device_add_bootindex_property(), having its own getter/setter and a call > to add_boot_device_path() on realize(). > > Or, to keep the code generic, we could just make get_boot_devices_list() > query the device for the suffix, instead of requiring the suffix to be > set before device_add_bootindex_property() is called. With something > like: > > typedef struct BootDeviceInfo { > int bootindex; > const char *suffix; > } BootDeviceInfo; > > struct FWBootEntry { > QTAILQ_ENTRY(FWBootEntry) link; > DeviceState *dev; > BootDeviceInfo *bootdev; > }; > > void add_boot_device_path(DeviceState *dev, BootDeviceInfo *info); > void del_boot_device_path(BootDeviceInfo *info); > void device_add_bootindex_property(Object *obj, const char *property, > DeviceState *dev, BootDeviceInfo > *info, > Error **errp); > > This way, the suffix can be set between device_add_bootindex_property() > and get_boot_devices_list() if necessary: > > static void somedevice_instance_init(Object *obj) > { > SomeDevice *somedev = SOME_DEVICE(obj); > device_add_bootindex_property(obj, DEVICE(obj), &somedev->bootinfo, > &error_abort); > } > > static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > { > [...] > dev->bootinfo.suffix = dev->unit ? "/disk@1" : "/disk@0"; > } > Thank you so much! I agree more with #1, because only IDE device has this special case. This is not worth add a property to DeviceState struct for a special device IMO. Best regards, -Gonglei
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index efab95b..9e2ed40 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -191,6 +191,17 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) return 0; } +static void ide_dev_instance_init(Object *obj) +{ + DeviceState *dev = DEVICE(obj); + IDEDevice *d = DO_UPCAST(IDEDevice, qdev, dev); + + device_add_bootindex_property(obj, &d->conf.bootindex, + "bootindex", + d->unit ? "/disk@1" : "/disk@0", + &d->qdev, NULL); +} + static int ide_hd_initfn(IDEDevice *dev) { return ide_dev_initfn(dev, IDE_HD); @@ -238,6 +249,7 @@ static const TypeInfo ide_hd_info = { .parent = TYPE_IDE_DEVICE, .instance_size = sizeof(IDEDrive), .class_init = ide_hd_class_init, + .instance_init = ide_dev_instance_init, }; static Property ide_cd_properties[] = { @@ -260,6 +272,7 @@ static const TypeInfo ide_cd_info = { .parent = TYPE_IDE_DEVICE, .instance_size = sizeof(IDEDrive), .class_init = ide_cd_class_init, + .instance_init = ide_dev_instance_init, }; static Property ide_drive_properties[] = { @@ -282,6 +295,7 @@ static const TypeInfo ide_drive_info = { .parent = TYPE_IDE_DEVICE, .instance_size = sizeof(IDEDrive), .class_init = ide_drive_class_init, + .instance_init = ide_dev_instance_init, }; static void ide_device_class_init(ObjectClass *klass, void *data)