Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

Christian Borntraeger <address@hidden> writes:

>> You're misreading how this works.
>> 
>> Host features are set based on command line arguments.  This is
>> advertised to the guest.  The vdev->get_config() call then sanitizes
>> features.  For instance, look at:
>> 
>> static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t 
>> features)
>> {
>>     VirtIONet *n = to_virtio_net(vdev);
>>     NetClientState *nc = qemu_get_queue(n->nic);
>> 
>>     features |= (1 << VIRTIO_NET_F_MAC);
>> 
>>     if (!peer_has_vnet_hdr(n)) {
>>         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
>> <snip>
>> 
>> This removes the VIRTIO_NET_F_CSUM feature if the peer doesn't support
>> it.  It's presupposing that the feature bit is set.
>> 
>> It's a bug in both virtio-ccw that features=0 when get_features is
>> called.  You can also tell this with:
>> 
>> [10:02 AM] address@hidden:~/git/qemu/hw/s390x$ grep 
>> DEFINE_VIRTIO_NET_FEATURES *
>> virtio-ccw.c:    DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, 
>> host_features[0]),
>> 
>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it
>> right.
>
> There is a  parse error in your statement (error in both virtio-ccw).
> Is virtio-ccw ok or not?

Sorry, virtio-ccw is okay.

> At least, this patch seems to work. (That also implies, that a transport
> must not hide virtio feature bits).

Looks correct to me.

Reviewed-by: Anthony Liguori <address@hidden>

Regards,

Anthony Liguori

>
>
> From: Christian Borntraeger <address@hidden>
> Date: Thu, 7 Mar 2013 17:21:41 +0100
> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
>
> Enable all virtio-net features for the legacy s390 virtio bus.
> This also fixes
> kernel BUG at 
> /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
>  hw/s390x/s390-virtio-bus.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 1200691..a8a8e19 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
>  
>  static Property s390_virtio_net_properties[] = {
>      DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
>      DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
>                         net.txtimer, TX_TIMER_INTERVAL),
>      DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
> -- 
> 1.8.0.1