| Message ID | 1429272826-4145-2-git-send-email-shannon.zhao@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 17 Apr 2015 20:13:43 +0800 Shannon Zhao <shannon.zhao@linaro.org> wrote: > Add virtio_ccw_device_plugged, it can be used to get backend's features. > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > hw/s390x/virtio-ccw.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 130535c..30ca377 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) > return 0; > } > > +/* This is called by virtio-bus just after the device is plugged. */ > +static void virtio_ccw_device_plugged(DeviceState *d) > +{ > + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > + > + /* Only the first 32 feature bits are used. */ > + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, > + dev->host_features[0]); > +} > + So how does this help? We already fetch the host features in the realize function. > /**************** Virtio-ccw Bus Device Descriptions *******************/ > > static Property virtio_ccw_net_properties[] = {
On Friday, 17 April 2015, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Fri, 17 Apr 2015 20:13:43 +0800 > Shannon Zhao <shannon.zhao@linaro.org> wrote: > >> Add virtio_ccw_device_plugged, it can be used to get backend's features. >> >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> --- >> hw/s390x/virtio-ccw.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index 130535c..30ca377 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) >> return 0; >> } >> >> +/* This is called by virtio-bus just after the device is plugged. */ >> +static void virtio_ccw_device_plugged(DeviceState *d) >> +{ >> + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); >> + >> + /* Only the first 32 feature bits are used. */ >> + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, >> + dev->host_features[0]); >> +} >> + > > So how does this help? We already fetch the host features in the > realize function. > please see patch 4/4, in this patch we will move the properties to backends. So the features can't fetch through realize function. If you ask me why we need to move, it's because these properties actually belongs to the backends and then we can support virtio-mmio to have these properties. >> /**************** Virtio-ccw Bus Device Descriptions *******************/ >> >> static Property virtio_ccw_net_properties[] = { > >
On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote: > Add virtio_ccw_device_plugged, it can be used to get backend's features. > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > hw/s390x/virtio-ccw.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 130535c..30ca377 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) > return 0; > } > > +/* This is called by virtio-bus just after the device is plugged. */ > +static void virtio_ccw_device_plugged(DeviceState *d) > +{ > + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > + > + /* Only the first 32 feature bits are used. */ > + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, > + dev->host_features[0]); > +} This means that this transport now calls virtio_bus_get_vdev_features twice, which doesn't look right. In particular, we call it from realize to set dev->host_features[0], and then add some features to dev->host_features[0]. Then I think we will call the 'plugged' method which will throw away those extra features. So I think that if we need to call this from 'plugged' rather than 'realize' we need to move all the code for setting host_features from 'realize' to here. But I'm confused about why this change is necessary -- don't the blk backends already use the "properties are on the backend" approach, and they work with this transport? -- PMM
On Friday, 17 April 2015, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote: >> Add virtio_ccw_device_plugged, it can be used to get backend's features. >> >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> --- >> hw/s390x/virtio-ccw.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index 130535c..30ca377 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) >> return 0; >> } >> >> +/* This is called by virtio-bus just after the device is plugged. */ >> +static void virtio_ccw_device_plugged(DeviceState *d) >> +{ >> + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); >> + >> + /* Only the first 32 feature bits are used. */ >> + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, >> + dev->host_features[0]); >> +} > > This means that this transport now calls virtio_bus_get_vdev_features > twice, which doesn't look right. In particular, we call it from > realize to set dev->host_features[0], and then add some features to > dev->host_features[0]. Then I think we will call the 'plugged' > method which will throw away those extra features. > > So I think that if we need to call this from 'plugged' > rather than 'realize' we need to move all the code for > setting host_features from 'realize' to here. > So sorry, when I reply this mail I'm using mobile phone, no codes on hand. So I didn't confirm that. > But I'm confused about why this change is necessary -- > don't the blk backends already use the "properties are > on the backend" approach, and they work with this transport? > Ok, maybe I missed that the transport already get the features when realized. If so, this change is not necessary.
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 130535c..30ca377 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) return 0; } +/* This is called by virtio-bus just after the device is plugged. */ +static void virtio_ccw_device_plugged(DeviceState *d) +{ + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); + + /* Only the first 32 feature bits are used. */ + dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus, + dev->host_features[0]); +} + /**************** Virtio-ccw Bus Device Descriptions *******************/ static Property virtio_ccw_net_properties[] = { @@ -1711,6 +1721,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data) k->load_queue = virtio_ccw_load_queue; k->save_config = virtio_ccw_save_config; k->load_config = virtio_ccw_load_config; + k->device_plugged = virtio_ccw_device_plugged; } static const TypeInfo virtio_ccw_bus_info = {