Patchwork [2/3] hw/virtio-net.c: set config size using host features

login
register
mail settings
Submitter Jesse Larrew
Date Feb. 5, 2013, 11:47 p.m.
Message ID <1360108037-9211-3-git-send-email-jlarrew@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/218463/
State New
Headers show

Comments

Jesse Larrew - Feb. 5, 2013, 11:47 p.m.
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 <jlarrew@linux.vnet.ibm.com>
---
 hw/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)
Michael S. Tsirkin - Feb. 7, 2013, 8:55 a.m.
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 <jlarrew@linux.vnet.ibm.com>
> ---
>  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.
How about we remove the packed attribute from config space structures?
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.


>  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
>
Stefan Hajnoczi - Feb. 7, 2013, 1:59 p.m.
On Tue, Feb 05, 2013 at 05:47:16PM -0600, Jesse Larrew wrote:
> 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;

size_t

>  } 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))

This isn't really necessary, just use offsetof() with the next field or
sizeof() for the last field.  Better to avoid this kind of hackery.
Laszlo Ersek - Feb. 7, 2013, 2:43 p.m.
On 02/07/13 09:55, Michael S. Tsirkin wrote:
> 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 <jlarrew@linux.vnet.ibm.com>
>> ---
>>  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.

Yes, it is.

(a) Padding inside structures is in the compiler's jurisdiction (except
before the first member, where it must be 0 -- C99 6.7.2.1p13). If you
tell the compiler to pack members, then it's your responsibility to make
sure no problems will come from accesses to non-naturally aligned fields.

(b) If the member is not a bitfield, you can take its address (C99
6.5.3.2p1). Since these unaligned fields are accessed *anyway* without
problems in practice, it's safe to dereference their addresses in practice.

IOW, purely by these fields being unaligned and already well-accessible
in practice, you don't commit a greater crime by taking their addresses.


Furthermore, taking the pointer to "one past" &field, ie. (&field + 1),
is safe, provided &field itself is well-defined. This comes from two
bits in the ISO C standard, sloppily paraphrased:

- Given an array with N elements, it's allowed to form a pointer to one
past the last element in it. IOW, &array[N], or (array + N), or
(&array[0] + N). You can't dereference it, but the pointer itself is
valid. (C99 6.5.6p8-9.)

- For the purposes of pointer arithmetic, (&x) works like (&x_array[0]),
where x is an object of type X, outside of an array, and x_array has
element type X and N=1 element. (C99 6.5.6p7.)

Since you're allowed to evaluate ((&x_array[0]) + 1), you're also
allowed to eval ((&x) + 1).


Anyway the above expression could be reworked for more portability with
offsetof and sizeof. As-is, it contains a pointer-to-non-void to
intptr_t conversion; what's more, with the resultant integer value meant
to be used numerically later on (ie. not for converting it back to
(void*), C99 7.18.1.4p). (The use of the null pointer is valid, the
dereference is not evaluated because it's the operand of &, C99
6.5.2.3p4 and 6.5.3.2p3.)

Instead, what about

#define endof(container, field) \
    (offsetof(container, field) + sizeof ((container *)0)->field)

Laszlo
Stefan Hajnoczi - Feb. 7, 2013, 3:22 p.m.
On Thu, Feb 7, 2013 at 3:43 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> Instead, what about
>
> #define endof(container, field) \
>     (offsetof(container, field) + sizeof ((container *)0)->field)

As mentioned in my reply, I think endof() isn't necessary.

Just use offsetof() the *next* field or sizeof() the entire struct
(for the last field).  That way you let someone else do the dirty
pointer tricks.

Stefan
Laszlo Ersek - Feb. 7, 2013, 3:30 p.m.
On 02/07/13 15:43, Laszlo Ersek wrote:
> On 02/07/13 09:55, Michael S. Tsirkin wrote:
>> On Tue, Feb 05, 2013 at 05:47:16PM -0600, Jesse Larrew wrote:

>>> +#define endof(container, field) \
>>> +    ((intptr_t)(&(((container *)0)->field)+1))
>>

> Furthermore, taking the pointer to "one past" &field, ie. (&field + 1),
> is safe, provided &field itself is well-defined. This comes from two
> bits in the ISO C standard, sloppily paraphrased:
> 
> - Given an array with N elements, it's allowed to form a pointer to one
> past the last element in it. IOW, &array[N], or (array + N), or
> (&array[0] + N). You can't dereference it, but the pointer itself is
> valid. (C99 6.5.6p8-9.)
> 
> - For the purposes of pointer arithmetic, (&x) works like (&x_array[0]),
> where x is an object of type X, outside of an array, and x_array has
> element type X and N=1 element. (C99 6.5.6p7.)
> 
> Since you're allowed to evaluate ((&x_array[0]) + 1), you're also
> allowed to eval ((&x) + 1).

Alas, I was wrong; C99 6.5.6p7 doesn't seem to apply here on a second
read, because "x", ie. ((container *)0)->field, is not an existent
object here.

> Anyway the above expression could be reworked for more portability with
> offsetof and sizeof. As-is, it contains a pointer-to-non-void to
> intptr_t conversion; what's more, with the resultant integer value meant
> to be used numerically later on (ie. not for converting it back to
> (void*), C99 7.18.1.4p). (The use of the null pointer is valid, the
> dereference is not evaluated because it's the operand of &, C99
> 6.5.2.3p4 and 6.5.3.2p3.)

Again, I was too liberal here wrt. the null pointer; the operand of &,
((container*)0)->field, is neither the result of the unary * operator,
nor is it "an lvalue that designates an object that [...]". (C99 6.5.3.2p1.)

((container*)0)->field is not an object.

> Instead, what about
> 
> #define endof(container, field) \
>     (offsetof(container, field) + sizeof ((container *)0)->field)

This works nonetheless, because the operand of sizeof is an expression
with a non-VLA type, and so it's not evaluated. (C99 6.5.3.4p2.)

Sorry for the noise.

Laszlo
Laszlo Ersek - Feb. 7, 2013, 3:31 p.m.
On 02/07/13 16:22, Stefan Hajnoczi wrote:
> On Thu, Feb 7, 2013 at 3:43 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> Instead, what about
>>
>> #define endof(container, field) \
>>     (offsetof(container, field) + sizeof ((container *)0)->field)
> 
> As mentioned in my reply, I think endof() isn't necessary.
> 
> Just use offsetof() the *next* field or sizeof() the entire struct
> (for the last field).  That way you let someone else do the dirty
> pointer tricks.

Apologies; I approached endof() in isolation.

Laszlo
Michael S. Tsirkin - Feb. 7, 2013, 3:32 p.m.
On Thu, Feb 07, 2013 at 03:43:51PM +0100, Laszlo Ersek wrote:
> #define endof(container, field) \
>     (offsetof(container, field) + sizeof ((container *)0)->field)

Indeed, this looks cleaner.
Anthony Liguori - Feb. 7, 2013, 3:45 p.m.
"Michael S. Tsirkin" <mst@redhat.com> 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 <jlarrew@linux.vnet.ibm.com>
>> ---
>>  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
>>
Anthony Liguori - Feb. 7, 2013, 3:53 p.m.
Laszlo Ersek <lersek@redhat.com> writes:

> On 02/07/13 09:55, Michael S. Tsirkin wrote:
>> 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 <jlarrew@linux.vnet.ibm.com>
>>> ---
>>>  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.
>
> Yes, it is.
>
> (a) Padding inside structures is in the compiler's jurisdiction (except
> before the first member, where it must be 0 -- C99 6.7.2.1p13). If you
> tell the compiler to pack members, then it's your responsibility to make
> sure no problems will come from accesses to non-naturally aligned fields.
>
> (b) If the member is not a bitfield, you can take its address (C99
> 6.5.3.2p1). Since these unaligned fields are accessed *anyway* without
> problems in practice, it's safe to dereference their addresses in practice.
>
> IOW, purely by these fields being unaligned and already well-accessible
> in practice, you don't commit a greater crime by taking their addresses.
>
>
> Furthermore, taking the pointer to "one past" &field, ie. (&field + 1),
> is safe, provided &field itself is well-defined. This comes from two
> bits in the ISO C standard, sloppily paraphrased:
>
> - Given an array with N elements, it's allowed to form a pointer to one
> past the last element in it. IOW, &array[N], or (array + N), or
> (&array[0] + N). You can't dereference it, but the pointer itself is
> valid. (C99 6.5.6p8-9.)

Indeed.  And due to the associativity of addition, it's also equivalent
to &(N + array) or even &N[array] which is my all time favorite weird C
syntax :-)

> - For the purposes of pointer arithmetic, (&x) works like (&x_array[0]),
> where x is an object of type X, outside of an array, and x_array has
> element type X and N=1 element. (C99 6.5.6p7.)
>
> Since you're allowed to evaluate ((&x_array[0]) + 1), you're also
> allowed to eval ((&x) + 1).
>
>
> Anyway the above expression could be reworked for more portability with
> offsetof and sizeof.  As-is, it contains a pointer-to-non-void to
> intptr_t conversion; what's more, with the resultant integer value meant
> to be used numerically later on (ie. not for converting it back to
> (void*), C99 7.18.1.4p). (The use of the null pointer is valid, the
> dereference is not evaluated because it's the operand of &, C99
> 6.5.2.3p4 and 6.5.3.2p3.)
>
> Instead, what about
>
> #define endof(container, field) \
>     (offsetof(container, field) + sizeof ((container *)0)->field)

That's fine too and probably a lot easier to understand.

Regards,

Anthony Liguori

>
> Laszlo
Anthony Liguori - Feb. 7, 2013, 3:55 p.m.
Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Thu, Feb 7, 2013 at 3:43 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> Instead, what about
>>
>> #define endof(container, field) \
>>     (offsetof(container, field) + sizeof ((container *)0)->field)
>
> As mentioned in my reply, I think endof() isn't necessary.
>
> Just use offsetof() the *next* field or sizeof() the entire struct
> (for the last field).  That way you let someone else do the dirty
> pointer tricks.

You don't always have a next field so it would get syntactically
awkward.

Regards,

Anthony Liguori

>
> Stefan
Michael S. Tsirkin - Feb. 7, 2013, 3:56 p.m.
On Thu, Feb 07, 2013 at 09:45:54AM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> 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 <jlarrew@linux.vnet.ibm.com>
> >> ---
> >>  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.

Sorry, with which version?

It looks like if I run qemu 1.3 with .status=off
windows drivers work. If I do this on 1.4 they break.
I don't see much value in knowingly breaking working setups
like this.
Michael S. Tsirkin - Feb. 7, 2013, 3:57 p.m.
On Thu, Feb 07, 2013 at 05:56:16PM +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 07, 2013 at 09:45:54AM -0600, Anthony Liguori wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> 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 <jlarrew@linux.vnet.ibm.com>
> > >> ---
> > >>  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.
> 
> Sorry, with which version?
> 
> It looks like if I run qemu 1.3 with .status=off
> windows drivers work. If I do this on 1.4 they break.
> I don't see much value in knowingly breaking working setups
> like this.

Well there is some value. It will catch anyone trying to
hard-code BAR size checks in the driver in the future :)

> -- 
> MST
Michael S. Tsirkin - Feb. 7, 2013, 3:59 p.m.
On Thu, Feb 07, 2013 at 04:22:45PM +0100, Stefan Hajnoczi wrote:
> On Thu, Feb 7, 2013 at 3:43 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> > Instead, what about
> >
> > #define endof(container, field) \
> >     (offsetof(container, field) + sizeof ((container *)0)->field)
> 
> As mentioned in my reply, I think endof() isn't necessary.
> 
> Just use offsetof() the *next* field or sizeof() the entire struct
> (for the last field).  That way you let someone else do the dirty
> pointer tricks.
> 
> Stefan

Yes but that's very confusing, you need to look at struct order
to see what's going on.
Stefan Hajnoczi - Feb. 7, 2013, 4:01 p.m.
On Thu, Feb 7, 2013 at 4:55 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Thu, Feb 7, 2013 at 3:43 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Instead, what about
>>>
>>> #define endof(container, field) \
>>>     (offsetof(container, field) + sizeof ((container *)0)->field)
>>
>> As mentioned in my reply, I think endof() isn't necessary.
>>
>> Just use offsetof() the *next* field or sizeof() the entire struct
>> (for the last field).  That way you let someone else do the dirty
>> pointer tricks.
>
> You don't always have a next field so it would get syntactically
> awkward.

Use sizeof() the entire struct for the last field.

Stefan
Anthony Liguori - Feb. 7, 2013, 5:46 p.m.
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 07, 2013 at 09:45:54AM -0600, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> This is about bug-for-bug compatibility with older QEMU.
>
> Sorry, with which version?
>
> It looks like if I run qemu 1.3 with .status=off
> windows drivers work. If I do this on 1.4 they break.
> I don't see much value in knowingly breaking working setups
> like this.

The whole discussion is moot because there's no version of QEMU that we
support backwards compatibility for that didn't have status enabled by
default.

What we need to worry about supporting is compat machines.  Not random
combination of feature flags that no actual user uses.

As a general rule, we need to make sure that the devices we expose look
the same when doing -M.  That means the config space size needs to be
the same as it was in previous machines.  This is a pretty simple
thing.

Regards,

Anthony Liguori

>
> -- 
> MST
Michael S. Tsirkin - Feb. 7, 2013, 9:02 p.m.
On Thu, Feb 07, 2013 at 11:46:55AM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 07, 2013 at 09:45:54AM -0600, Anthony Liguori wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> This is about bug-for-bug compatibility with older QEMU.
> >
> > Sorry, with which version?
> >
> > It looks like if I run qemu 1.3 with .status=off
> > windows drivers work. If I do this on 1.4 they break.
> > I don't see much value in knowingly breaking working setups
> > like this.
> 
> The whole discussion is moot because there's no version of QEMU that we
> support backwards compatibility for that didn't have status enabled by
> default.
> 
> What we need to worry about supporting is compat machines.  Not random
> combination of feature flags that no actual user uses.
> 
> As a general rule, we need to make sure that the devices we expose look
> the same when doing -M.  That means the config space size needs to be
> the same as it was in previous machines.
> This is a pretty simple thing.
> 
> Regards,
> 
> Anthony Liguori
> >
> > -- 
> > MST

So why do we add extra code who's sole effect is breaking old
windows drivers?
I like the idea but why add code for status and mac features?
Also, would be nicer not to rely on the fact we always set MAC
flag at the moment. What if we make it optional?

We also don't really need the {} tag at the end - we have ARRAY_SIZE
macro.  Why don't we do:

static VirtIOFeature feature_sizes[] = {
   { .flags = 0, /* default size when no flags are set */
    .end = offsetof(struct virtio_net_config, max_virtqueue_pairs)},
   {.flags = 1 << VIRTIO_NET_F_MQ,
    .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
};


And:

int i, config_size = 0;

  for (i = 0; i < ARRAY_SIZE(feature_sizes); i++) {
      if (!feature_sizes[i].flags || host_features & feature_sizes[i].flags) {
          config_size = MAX(feature_sizes[i].end, config_size);
       }
   }

This handles the windows bug nicely and will work for adding
features going forward, without changing size for -M pc-1.3.
Anthony Liguori - Feb. 7, 2013, 9:15 p.m.
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 07, 2013 at 11:46:55AM -0600, Anthony Liguori wrote:
>
> So why do we add extra code who's sole effect is breaking old
> windows drivers?

A little dramatic, no?

> I like the idea but why add code for status and mac features?

Because it's correct and therefore easier to understand when it's
applied to the remaining virtio drivers (as Jesse is working on right now).;

> Also, would be nicer not to rely on the fact we always set MAC
> flag at the moment. What if we make it optional?

I don't follow what you mean by this.  It's not relying on that fact AFAICT.

> We also don't really need the {} tag at the end - we have ARRAY_SIZE
> macro.  Why don't we do:

Because the second step will be to pass this array instead of
n->config_size in virtio_common_init.

> static VirtIOFeature feature_sizes[] = {
>    { .flags = 0, /* default size when no flags are set */
>     .end = offsetof(struct virtio_net_config, max_virtqueue_pairs)},

Now you've got two spots to touch whenever a new config flag is added.
That's not very nice.

>    {.flags = 1 << VIRTIO_NET_F_MQ,
>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> };
>
>
> And:
>
> int i, config_size = 0;
>
>   for (i = 0; i < ARRAY_SIZE(feature_sizes); i++) {
>       if (!feature_sizes[i].flags || host_features & feature_sizes[i].flags) {
>           config_size = MAX(feature_sizes[i].end, config_size);
>        }
>    }
>
> This handles the windows bug nicely and will work for adding
> features going forward, without changing size for -M pc-1.3.

The Windows driver is checking for BAR0 size == 32.  Bars are always a
power of 2.  The base config registers are 20+ bytes so the BAR0 will
always at least be 32 bytes.  IOW, your concerns about breaking the
Windows drivers are unfounded because BAR0 size won't change.

Regards,

Anthony Liguori

>
>
> -- 
> MST
Michael S. Tsirkin - Feb. 7, 2013, 9:28 p.m.
On Thu, Feb 07, 2013 at 03:15:42PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 07, 2013 at 11:46:55AM -0600, Anthony Liguori wrote:
> >
> > So why do we add extra code who's sole effect is breaking old
> > windows drivers?
> 
> A little dramatic, no?
> 
> > I like the idea but why add code for status and mac features?
> 
> Because it's correct and therefore easier to understand when it's
> applied to the remaining virtio drivers (as Jesse is working on right now).;
> 
> > Also, would be nicer not to rely on the fact we always set MAC
> > flag at the moment. What if we make it optional?
> 
> I don't follow what you mean by this.  It's not relying on that fact AFAICT.
> 
> > We also don't really need the {} tag at the end - we have ARRAY_SIZE
> > macro.  Why don't we do:
> 
> Because the second step will be to pass this array instead of
> n->config_size in virtio_common_init.
> 
> > static VirtIOFeature feature_sizes[] = {
> >    { .flags = 0, /* default size when no flags are set */
> >     .end = offsetof(struct virtio_net_config, max_virtqueue_pairs)},
> 
> Now you've got two spots to touch whenever a new config flag is added.
> That's not very nice.
> 
> >    {.flags = 1 << VIRTIO_NET_F_MQ,
> >     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> > };
> >
> >
> > And:
> >
> > int i, config_size = 0;
> >
> >   for (i = 0; i < ARRAY_SIZE(feature_sizes); i++) {
> >       if (!feature_sizes[i].flags || host_features & feature_sizes[i].flags) {
> >           config_size = MAX(feature_sizes[i].end, config_size);
> >        }
> >    }
> >
> > This handles the windows bug nicely and will work for adding
> > features going forward, without changing size for -M pc-1.3.
> 
> The Windows driver is checking for BAR0 size == 32.  Bars are always a
> power of 2.  The base config registers are 20+ bytes so the BAR0 will
> always at least be 32 bytes.  IOW, your concerns about breaking the
> Windows drivers are unfounded because BAR0 size won't change.
> 
> Regards,
> 
> Anthony Liguori

Good points. This addresses my concerns, thanks.

> >
> >
> > -- 
> > MST
Alexander Graf - March 5, 2013, 4:41 p.m.
On 02/06/2013 12:47 AM, 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<jlarrew@linux.vnet.ibm.com>

This patch breaks virtio-s390:

------------[ cut here ]------------
kernel BUG at 
/usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
illegal operation: 0001 [#1] SMP
Modules linked in: virtio_net(F+) scsi_dh_emc(F) scsi_dh_hp_sw(F) 
scsi_dh_rdac(F) scsi_dh_alua(F) scsi_dh(F) scsi_mod(F) dm_snapshot(F) 
dm_mod(F) ext3(F) mbcache(F) jbd(F)
Supported: No, Unsupported modules are loaded
CPU: 0 Tainted: GF             3.0.58-52-default #1
Process modprobe (pid: 160, task: 0000000005c1a238, ksp: 0000000005c1feb8)
Krnl PSW : 0704200180000000 00000000004172ca (kvm_get+0x86/0x90)
            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:2 PM:0 EA:3
Krnl GPRS: 0000000000000000 0000000000000000 0000000000000006 
0000000000000000
            000000000108d710 0000000000000006 0000000007dbf198 
0000000007dbf000
            0000000000001000 0000000007dbf19e 00000000008499d0 
00000000013e6000
            0000000008000000 000003c0004e80e8 000003c0004e6be8 
0000000005c1fb78
Krnl Code: 00000000004172be: a737fff9        brctg    %r3,4172b0
            00000000004172c2: a7f4ffee        brc    15,41729e
            00000000004172c6: a7f40001        brc    15,4172c8
 >00000000004172ca: a7f40000        brc    15,4172ca
            00000000004172ce: d20040001000    mvc    0(1,%r4),0(%r1)
            00000000004172d4: ebaff0680024    stmg    %r10,%r15,104(%r15)
            00000000004172da: c0d00009ca03    larl    %r13,5506e0
            00000000004172e0: a7f13fc0        tmll    %r15,16320
Call Trace:
([<000003c0004e6b54>] virtnet_probe+0x50/0x4f0 [virtio_net])
  [<00000000003a7df0>] virtio_dev_probe+0x16c/0x1c0
  [<00000000003c80ce>] really_probe+0x8a/0x25c
  [<00000000003c8416>] __driver_attach+0xca/0xd0
  [<00000000003c74cc>] bus_for_each_dev+0x80/0xd4
  [<00000000003c6b82>] bus_add_driver+0x22a/0x2f4
  [<00000000003c8c0e>] driver_register+0x9e/0x1a0
  [<00000000001000ba>] do_one_initcall+0x3a/0x170
  [<000000000019e292>] SyS_init_module+0xe6/0x234
  [<00000000004f9c40>] sysc_noemu+0x16/0x1c
  [<000003fffd4a1046>] 0x3fffd4a1046
Last Breaking-Event-Address:
  [<00000000004172c6>] kvm_get+0x82/0x90

---[ end trace 22332d4912971f08 ]---


> ---
>   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))
> +
> +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)},
> +    {}
> +};
> +
>   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);
> +        }
> +    }

Because down here, host_features doesn't include any features yet. They 
only get set later by calling get_config(0) ...

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

... which only works after get_config got set.


Alex

>       n->vdev.set_config = virtio_net_set_config;
>       n->vdev.get_features = virtio_net_get_features;

Patch

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))
+
+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)},
+    {}
+};
+
 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;