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

[Anthony Liguori]

"Michael S. Tsirkin" <address@hidden> writes:

> On Tue, Feb 05, 2013 at 05:47:16PM -0600, Jesse Larrew wrote:
>> Currently, the config size for virtio devices is hard coded. When a new
>> feature is added that changes the config size, drivers that assume a static
>> config size will break. For purposes of backward compatibility, there needs
>> to be a way to inform drivers of the config size needed to accommodate the
>> set of features enabled.
>> 
>> Signed-off-by: Jesse Larrew <address@hidden>
>> ---
>>  hw/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index f1c2884..8f521b3 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -73,8 +73,31 @@ typedef struct VirtIONet
>>      int multiqueue;
>>      uint16_t max_queues;
>>      uint16_t curr_queues;
>> +    int config_size;
>>  } VirtIONet;
>>  
>> +/*
>> + * Calculate the number of bytes up to and including the given 'field' of
>> + * 'container'.
>> + */
>> +#define endof(container, field) \
>> +    ((intptr_t)(&(((container *)0)->field)+1))
>
> Hmm I'm worried whether it's legal to take a pointer to a
> field in a packed structure.

It absolutely is.  Packed just means remove padding.  It doesn't imply
that there would be any kind of weird layout.  Ultimately, a pointer to
a member needs to behave the same way that a pointer to that type
declared in an unpacked structure would behave since the knowledge of
the fact that it's in a packed structure is quickly lost.

> How about we remove the packed attribute from config space structures?

It's definitely not needed here AFAICT.

Regards,

Anthony Liguori

> It's not really necessary since fields are laid out reasonably.
>
>
>> +typedef struct VirtIOFeature {
>> +    uint32_t flags;
>> +    size_t end;
>> +} VirtIOFeature;
>> +
>> +static VirtIOFeature feature_sizes[] = {
>> +    {.flags = 1 << VIRTIO_NET_F_MAC,
>> +     .end = endof(struct virtio_net_config, mac)},
>> +    {.flags = 1 << VIRTIO_NET_F_STATUS,
>> +     .end = endof(struct virtio_net_config, status)},
>> +    {.flags = 1 << VIRTIO_NET_F_MQ,
>> +     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>> +    {}
>> +};
>> +
>
> This is not good. This will break old windows drivers
> if mac/status are off, since they hardcode 32 BAR size.

Then old windows drivers are broken on older QEMU instances that never
had those fields in the first place.

This is about bug-for-bug compatibility with older QEMU.

>>  static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
>>  {
>>      VirtIONet *n = qemu_get_nic_opaque(nc);
>> @@ -104,15 +127,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
>> uint8_t *config)
>>      stw_p(&netcfg.status, n->status);
>>      stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
>>      memcpy(netcfg.mac, n->mac, ETH_ALEN);
>> -    memcpy(config, &netcfg, sizeof(netcfg));
>> +    memcpy(config, &netcfg, n->config_size);
>>  }
>>  
>>  static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>  {
>>      VirtIONet *n = to_virtio_net(vdev);
>> -    struct virtio_net_config netcfg;
>> +    struct virtio_net_config netcfg = {};
>>  
>> -    memcpy(&netcfg, config, sizeof(netcfg));
>> +    memcpy(&netcfg, config, n->config_size);
>>  
>>      if (!(n->vdev.guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
>>          memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
>> @@ -1279,16 +1302,21 @@ static void 
>> virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>  }
>>  
>>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>> -                              virtio_net_conf *net,
>> -                              uint32_t host_features)
>> +                              virtio_net_conf *net, uint32_t host_features)
>>  {
>>      VirtIONet *n;
>> -    int i;
>> +    int i, config_size = 0;
>> +
>> +    for (i = 0; feature_sizes[i].flags != 0; i++) {
>> +        if (host_features & feature_sizes[i].flags) {
>> +            config_size = MAX(feature_sizes[i].end, config_size);
>> +        }
>> +    }
>>  
>>      n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
>> -                                        sizeof(struct virtio_net_config),
>> -                                        sizeof(VirtIONet));
>> +                                        config_size, sizeof(VirtIONet));
>>  
>> +    n->config_size = config_size;
>>      n->vdev.get_config = virtio_net_get_config;
>>      n->vdev.set_config = virtio_net_set_config;
>>      n->vdev.get_features = virtio_net_get_features;
>> -- 
>> 1.7.11.7
>>