Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Shannon Zhao]

On Friday, 17 April 2015, Peter Maydell <address@hidden> wrote:
> On 17 April 2015 at 13:13, Shannon Zhao <address@hidden> wrote:
>> Add virtio_ccw_device_plugged, it can be used to get backend's features.
>>
>> Signed-off-by: Shannon Zhao <address@hidden>
>> Signed-off-by: Shannon Zhao <address@hidden>
>> ---
>>  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.