| Message ID | 1365168490-29616-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Kevin Wolf <kwolf@redhat.com> writes: > From: KONRAD Frederic <fred.konrad@greensocs.com> > > The virtio-blk-x configuration is not in sync with virtio-blk configuration. > So this patch remove the virtio-blk-x configuration field, and use virtio-blk > one for setting the properties. > > This also remove a useless configuration copy in virtio_blk_device_init. > > Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> > Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> This breaks -M pc-1.0 as it breaks global properties. Specifically, we have stuff like: },{\ .driver = "virtio-blk-pci",\ .property = "config-wce",\ .value = "off",\ } for -M pc-1.0 But: [aliguori@ccnode4 qemu]$ x86_64-softmmu/qemu-system-x86_64 -vnc none -monitor stdio -M pc-1.0 -drive file=dummy.img,snapshot=on,if=virtio QEMU 1.4.50 monitor - type 'help' for more information (qemu) info qtree bus: main-system-bus type System <snip> dev: i440FX-pcihost, id "" irq 0 bus: pci.0 type PCI dev: virtio-blk-pci, id "" class = 0x0 ioeventfd = off vectors = 2 x-data-plane = off indirect_desc = on event_idx = on drive = virtio0 logical_block_size = 512 physical_block_size = 512 min_io_size = 0 opt_io_size = 0 bootindex = -1 discard_granularity = 4294967295 cyls = 16383 heads = 16 secs = 63 serial = <null> config-wce = on ^^^ This should be config-wce = off when we use -M pc-1.0. Regards, Anthony Liguori > --- > hw/s390x/s390-virtio-bus.c | 4 ++-- > hw/s390x/s390-virtio-bus.h | 1 - > hw/s390x/virtio-ccw.c | 4 ++-- > hw/s390x/virtio-ccw.h | 1 - > hw/virtio-blk.c | 7 ------- > hw/virtio-blk.h | 2 -- > hw/virtio-pci.c | 7 ++++--- > hw/virtio-pci.h | 1 - > 8 files changed, 8 insertions(+), 19 deletions(-) > > diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c > index 8c529c1..3824181 100644 > --- a/hw/s390x/s390-virtio-bus.c > +++ b/hw/s390x/s390-virtio-bus.c > @@ -166,7 +166,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev) > { > VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > - virtio_blk_set_conf(vdev, &(dev->blk)); > + > qdev_set_parent_bus(vdev, BUS(&s390_dev->bus)); > if (qdev_init(vdev) < 0) { > return -1; > @@ -443,7 +443,7 @@ static const TypeInfo s390_virtio_net = { > }; > > static Property s390_virtio_blk_properties[] = { > - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), > + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, vdev.blk), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h > index ebe8794..06f9479 100644 > --- a/hw/s390x/s390-virtio-bus.h > +++ b/hw/s390x/s390-virtio-bus.h > @@ -127,7 +127,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev); > typedef struct VirtIOBlkS390 { > VirtIOS390Device parent_obj; > VirtIOBlock vdev; > - VirtIOBlkConf blk; > } VirtIOBlkS390; > > /* virtio-scsi-s390 */ > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 5dce791..78a8f0e 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -574,7 +574,7 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev) > { > VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > - virtio_blk_set_conf(vdev, &(dev->blk)); > + > qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); > if (qdev_init(vdev) < 0) { > return -1; > @@ -761,7 +761,7 @@ static const TypeInfo virtio_ccw_net = { > static Property virtio_ccw_blk_properties[] = { > DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id), > DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]), > - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk), > + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, vdev.blk), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h > index d580510..5179374 100644 > --- a/hw/s390x/virtio-ccw.h > +++ b/hw/s390x/virtio-ccw.h > @@ -113,7 +113,6 @@ typedef struct VirtIOSCSICcw { > typedef struct VirtIOBlkCcw { > VirtioCcwDevice parent_obj; > VirtIOBlock vdev; > - VirtIOBlkConf blk; > } VirtIOBlkCcw; > > /* virtio-balloon-ccw */ > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index f2143fd..0414a8a 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -619,12 +619,6 @@ static const BlockDevOps virtio_block_ops = { > .resize_cb = virtio_blk_resize, > }; > > -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk) > -{ > - VirtIOBlock *s = VIRTIO_BLK(dev); > - memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); > -} > - > static int virtio_blk_device_init(VirtIODevice *vdev) > { > DeviceState *qdev = DEVICE(vdev); > @@ -656,7 +650,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev) > vdev->reset = virtio_blk_reset; > s->bs = blk->conf.bs; > s->conf = &blk->conf; > - memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); > s->rq = NULL; > s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; > > diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h > index 8c6c78b..e228057 100644 > --- a/hw/virtio-blk.h > +++ b/hw/virtio-blk.h > @@ -148,6 +148,4 @@ typedef struct VirtIOBlock { > DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true) > #endif /* __linux__ */ > > -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk); > - > #endif > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index fb20722..fbcc5f6 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -1303,10 +1303,11 @@ static Property virtio_blk_pci_properties[] = { > VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > - DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false), > + DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, vdev.blk.data_plane, 0, > + false), > #endif > DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), > - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk), > + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, vdev.blk), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -1314,7 +1315,7 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev) > { > VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > - virtio_blk_set_conf(vdev, &(dev->blk)); > + > qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > if (qdev_init(vdev) < 0) { > return -1; > diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h > index f99f2eb..108ed12 100644 > --- a/hw/virtio-pci.h > +++ b/hw/virtio-pci.h > @@ -115,7 +115,6 @@ struct VirtIOSCSIPCI { > struct VirtIOBlkPCI { > VirtIOPCIProxy parent_obj; > VirtIOBlock vdev; > - VirtIOBlkConf blk; > }; > > /* > -- > 1.8.1.4
On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> The virtio-blk-x configuration is not in sync with virtio-blk configuration. >> So this patch remove the virtio-blk-x configuration field, and use virtio-blk >> one for setting the properties. >> >> This also remove a useless configuration copy in virtio_blk_device_init. >> >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > This breaks -M pc-1.0 as it breaks global properties. The first level analysis of why this happens: (1) global values are set as part of DeviceState's instance_init function [and we set the config_wce field to 0 here] (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second (3) it calls object_initialize on its embedded virtio-blk object (4) the DeviceState instance_init for that child object sets the properties to their init values [thus resetting config_wce to 1] The effect is that at the moment you can't have a property on a parent object whose value is stored in the child object (unless you do something hacky like read the default before calling object_initialize and restore it afterwards]. It seems to me that the underlying problem here is that we set global values too early -- they should be set after a device is completely instance_init'd, not halfway through init. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> From: KONRAD Frederic <fred.konrad@greensocs.com> >>> >>> The virtio-blk-x configuration is not in sync with virtio-blk configuration. >>> So this patch remove the virtio-blk-x configuration field, and use virtio-blk >>> one for setting the properties. >>> >>> This also remove a useless configuration copy in virtio_blk_device_init. >>> >>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> >> This breaks -M pc-1.0 as it breaks global properties. > > The first level analysis of why this happens: > > (1) global values are set as part of DeviceState's instance_init function > [and we set the config_wce field to 0 here] > (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second > (3) it calls object_initialize on its embedded virtio-blk object > (4) the DeviceState instance_init for that child object sets the > properties to their init values [thus resetting config_wce to 1] > > The effect is that at the moment you can't have a property on > a parent object whose value is stored in the child object > (unless you do something hacky like read the default before > calling object_initialize and restore it afterwards]. > > It seems to me that the underlying problem here is that we > set global values too early -- they should be set after a > device is completely instance_init'd, not halfway through init. Here is an in-progress series that forwards properties from the virtio-blk-pci to virtio-blk. I think it's the best solution. The approach needs some polishing but I wanted to share early. https://github.com/aliguori/qemu/commits/qom-forward.1 Regards, Anthony Liguori > > -- PMM
On 05/04/2013 21:24, Anthony Liguori wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 5 April 2013 17:47, Anthony Liguori <anthony@codemonkey.ws> wrote: >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> From: KONRAD Frederic <fred.konrad@greensocs.com> >>>> >>>> The virtio-blk-x configuration is not in sync with virtio-blk configuration. >>>> So this patch remove the virtio-blk-x configuration field, and use virtio-blk >>>> one for setting the properties. >>>> >>>> This also remove a useless configuration copy in virtio_blk_device_init. >>>> >>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>>> Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> This breaks -M pc-1.0 as it breaks global properties. >> The first level analysis of why this happens: >> >> (1) global values are set as part of DeviceState's instance_init function >> [and we set the config_wce field to 0 here] >> (2) virtio_blk_pci_instance_init is a subclass instance_init so runs second >> (3) it calls object_initialize on its embedded virtio-blk object >> (4) the DeviceState instance_init for that child object sets the >> properties to their init values [thus resetting config_wce to 1] >> >> The effect is that at the moment you can't have a property on >> a parent object whose value is stored in the child object >> (unless you do something hacky like read the default before >> calling object_initialize and restore it afterwards]. >> >> It seems to me that the underlying problem here is that we >> set global values too early -- they should be set after a >> device is completely instance_init'd, not halfway through init. > Here is an in-progress series that forwards properties from the > virtio-blk-pci to virtio-blk. > > I think it's the best solution. The approach needs some polishing but I > wanted to share early. > > https://github.com/aliguori/qemu/commits/qom-forward.1 > > Regards, > > Anthony Liguori > >> -- PMM Seems, it's what we need here. I think it is the same think for all other backends? Fred
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index 8c529c1..3824181 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -166,7 +166,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev) { VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev); DeviceState *vdev = DEVICE(&dev->vdev); - virtio_blk_set_conf(vdev, &(dev->blk)); + qdev_set_parent_bus(vdev, BUS(&s390_dev->bus)); if (qdev_init(vdev) < 0) { return -1; @@ -443,7 +443,7 @@ static const TypeInfo s390_virtio_net = { }; static Property s390_virtio_blk_properties[] = { - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk), + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, vdev.blk), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h index ebe8794..06f9479 100644 --- a/hw/s390x/s390-virtio-bus.h +++ b/hw/s390x/s390-virtio-bus.h @@ -127,7 +127,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev); typedef struct VirtIOBlkS390 { VirtIOS390Device parent_obj; VirtIOBlock vdev; - VirtIOBlkConf blk; } VirtIOBlkS390; /* virtio-scsi-s390 */ diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 5dce791..78a8f0e 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -574,7 +574,7 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev) { VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - virtio_blk_set_conf(vdev, &(dev->blk)); + qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); if (qdev_init(vdev) < 0) { return -1; @@ -761,7 +761,7 @@ static const TypeInfo virtio_ccw_net = { static Property virtio_ccw_blk_properties[] = { DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id), DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]), - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk), + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, vdev.blk), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h index d580510..5179374 100644 --- a/hw/s390x/virtio-ccw.h +++ b/hw/s390x/virtio-ccw.h @@ -113,7 +113,6 @@ typedef struct VirtIOSCSICcw { typedef struct VirtIOBlkCcw { VirtioCcwDevice parent_obj; VirtIOBlock vdev; - VirtIOBlkConf blk; } VirtIOBlkCcw; /* virtio-balloon-ccw */ diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index f2143fd..0414a8a 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -619,12 +619,6 @@ static const BlockDevOps virtio_block_ops = { .resize_cb = virtio_blk_resize, }; -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk) -{ - VirtIOBlock *s = VIRTIO_BLK(dev); - memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); -} - static int virtio_blk_device_init(VirtIODevice *vdev) { DeviceState *qdev = DEVICE(vdev); @@ -656,7 +650,6 @@ static int virtio_blk_device_init(VirtIODevice *vdev) vdev->reset = virtio_blk_reset; s->bs = blk->conf.bs; s->conf = &blk->conf; - memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h index 8c6c78b..e228057 100644 --- a/hw/virtio-blk.h +++ b/hw/virtio-blk.h @@ -148,6 +148,4 @@ typedef struct VirtIOBlock { DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true) #endif /* __linux__ */ -void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk); - #endif diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index fb20722..fbcc5f6 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -1303,10 +1303,11 @@ static Property virtio_blk_pci_properties[] = { VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE - DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false), + DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, vdev.blk.data_plane, 0, + false), #endif DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), - DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk), + DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, vdev.blk), DEFINE_PROP_END_OF_LIST(), }; @@ -1314,7 +1315,7 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev) { VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - virtio_blk_set_conf(vdev, &(dev->blk)); + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); if (qdev_init(vdev) < 0) { return -1; diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h index f99f2eb..108ed12 100644 --- a/hw/virtio-pci.h +++ b/hw/virtio-pci.h @@ -115,7 +115,6 @@ struct VirtIOSCSIPCI { struct VirtIOBlkPCI { VirtIOPCIProxy parent_obj; VirtIOBlock vdev; - VirtIOBlkConf blk; }; /*