diff mbox

[1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

Message ID 1429272826-4145-2-git-send-email-shannon.zhao@linaro.org
State New
Headers show

Commit Message

Shannon Zhao April 17, 2015, 12:13 p.m. UTC
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(+)

Comments

Shannon Zhao April 17, 2015, 2:50 p.m. UTC | #1
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[] = {
>
>
Peter Maydell April 17, 2015, 3:33 p.m. UTC | #2
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
Shannon Zhao April 18, 2015, 1:36 a.m. UTC | #3
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 mbox

Patch

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 = {