diff mbox

[v2,1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

Message ID 5535AB48.6010808@linaro.org
State New
Headers show

Commit Message

Shannon Zhao April 21, 2015, 1:43 a.m. UTC 
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:
>


Maybe we can use following patch. This moves virtio_net_set_config_size to
virtio_net_device_realize function. As the features are moved to virtio-net,
so we should set the config_size in virtio-net too. And this can be useful to
virtio-mmio which now doesn't call virtio_net_set_config_size in
virtio-mmio's realize function.

Cornelia, could you check if this works on s390? Thanks.



 
> 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.
>

Comments

Cornelia Huck April 21, 2015, 7:30 a.m. UTC | #1 
On Tue, 21 Apr 2015 09:43:36 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> 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:
> >
> 
> 
> Maybe we can use following patch. This moves virtio_net_set_config_size to
> virtio_net_device_realize function. As the features are moved to virtio-net,
> so we should set the config_size in virtio-net too. And this can be useful to
> virtio-mmio which now doesn't call virtio_net_set_config_size in
> virtio-mmio's realize function.

I think this makes sense.

> 
> Cornelia, could you check if this works on s390? Thanks.

Networking works again via virtio-ccw with this patch on top.

I'll still try to figure out a better sequence for realizing/plugging
virtio-ccw devices, but I think that is orthogonal to this patch.
Shannon Zhao April 21, 2015, 10:33 a.m. UTC | #2 
On 2015/4/21 15:30, Cornelia Huck wrote:
> On Tue, 21 Apr 2015 09:43:36 +0800
> Shannon Zhao <shannon.zhao@linaro.org> wrote:
> 
>> 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:
>>>
>>
>>
>> Maybe we can use following patch. This moves virtio_net_set_config_size to
>> virtio_net_device_realize function. As the features are moved to virtio-net,
>> so we should set the config_size in virtio-net too. And this can be useful to
>> virtio-mmio which now doesn't call virtio_net_set_config_size in
>> virtio-mmio's realize function.
> 
> I think this makes sense.
> 
>>
>> Cornelia, could you check if this works on s390? Thanks.
> 
> Networking works again via virtio-ccw with this patch on top.
> 
> I'll still try to figure out a better sequence for realizing/plugging
> virtio-ccw devices, but I think that is orthogonal to this patch.
> 

Cornelia, thanks for your help :)
Will add this and send them as v3.
diff mbox

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27ec5b1..36ba027 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1588,6 +1588,7 @@  static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     NetClientState *nc;
     int i;

+    virtio_net_set_config_size(n, n->host_features);
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);

     n->max_queues = MAX(n->nic_conf.peers.queues, 1);
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 1987873..b893e02 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -145,7 +145,6 @@  static void s390_virtio_net_realize(VirtIOS390Device *s390_dev, Error **errp)
     DeviceState *vdev = DEVICE(&dev->vdev);
     Error *err = NULL;

-    virtio_net_set_config_size(&dev->vdev, s390_dev->host_features);
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 803526a..1252162 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));
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 772244e..c6b99f9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1369,7 +1369,6 @@  static void virtio_net_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIONetPCI *dev = VIRTIO_NET_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);

-    virtio_net_set_config_size(&dev->vdev, vpci_dev->host_features);
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));