Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao <shannon.zhao@linaro.org>]

On 2015/4/20 19:32, Cornelia Huck wrote:
> On Mon, 20 Apr 2015 16:20:00 +0800
> shannon.zhao@linaro.org wrote:
>
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net.
>> The transports just sync the host features from backend.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>   hw/net/virtio-net.c            | 4 ++++
>>   hw/s390x/s390-virtio-bus.c     | 1 -
>>   hw/s390x/virtio-ccw.c          | 1 -
>>   hw/virtio/virtio-pci.c         | 1 -
>>   include/hw/virtio/virtio-net.h | 1 +
>>   5 files changed, 5 insertions(+), 3 deletions(-)
>
> I need the following change to make this work for virtio-ccw:
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 2252789..7a2bdff 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -779,7 +779,6 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
>       DeviceState *vdev = DEVICE(&dev->vdev);
>       Error *err = NULL;
>
> -    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
>       virtio_net_set_netclient_name(&dev->vdev, qdev->id,
>                                     object_get_typename(OBJECT(qdev)));
>       qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
> @@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
>       }
>
>       virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
> +    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
>   }
>
>   static void virtio_ccw_net_instance_init(Object *obj)
>
> host_features used to be statically populated, so
> virtio_net_set_config_size() was able to use the various feature bits
> for its decisions.
>
> It does not seem quite right, however, since the set of feature bits
> had not been through virtio-net's ->get_features() routine (or the
> feature bit manipulations in virtio-ccw's realize() routine) - it was
> just good enough.
>
> Maybe the right place for calling set_config_size() would be in a
> virtio-net specific ->plugged() callback?
>
> I'm not sure why virtio-pci works, but they have a different topology
> with pci device and virtio-pci device separate, so it might work out
> there.
>

The virtio-pci works because it calls device_plugged hook to get the 
features and this hook is called before realized function. When calling 
virtio_net_set_config_size the features are already synced, so it works.

I think maybe we should call device_plugged hook to get the features in 
virtio-ccw other than get them in realized function. So the virtio-ccw, 
virtio-pci and virtio-mmio use same ways.

Thanks,
Shannon