| Message ID | 1360108037-9211-3-git-send-email-jlarrew@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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.
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
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
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
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
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.
"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 >>
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
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
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.
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
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.
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
"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
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.
"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
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
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;
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;
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(-)