[27/41] virtio-net: abstract vlans operations

Submitted by Juan Quintela on Dec. 2, 2009, 12:04 p.m.

Details

Message ID 0212bb69ec7e94e572f4f0a7d8718a6131e9ceaa.1259754427.git.quintela@redhat.com
State New
Headers show

Commit Message

Juan Quintela Dec. 2, 2009, 12:04 p.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-net.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

Comments

Michael S. Tsirkin Dec. 2, 2009, 2:49 p.m.
On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |   21 ++++++++++++++++++---
>  1 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 97db0d0..cf13e94 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>   * - we could suppress RX interrupt if we were so inclined.
>   */
> 
> +static void vlan_add(VirtIONet *n, int vid)
> +{
> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +}
> +
> +static void vlan_del(VirtIONet *n, int vid)
> +{
> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +}
> +
> +static bool vlan_is_set(VirtIONet *n, int vid)
> +{
> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));

This one looks wrong. Did you check this does not break vlans?

> +}
> +
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> @@ -306,9 +321,9 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
> 
>      if (cmd == VIRTIO_NET_CTRL_VLAN_ADD)
> -        n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +        vlan_add(n, vid);
>      else if (cmd == VIRTIO_NET_CTRL_VLAN_DEL)
> -        n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +        vlan_del(n, vid);
>      else
>          return VIRTIO_NET_ERR;
> 
> @@ -471,7 +486,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> 
>      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> -        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> +        if (!vlan_is_set(n, vid))
>              return 0;
>      }
> 
> -- 
> 1.6.5.2
Juan Quintela Dec. 2, 2009, 6:26 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |   21 ++++++++++++++++++---
>>  1 files changed, 18 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 97db0d0..cf13e94 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>>   * - we could suppress RX interrupt if we were so inclined.
>>   */
>> 
>> +static void vlan_add(VirtIONet *n, int vid)
>> +{
>> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
>> +}
>> +
>> +static void vlan_del(VirtIONet *n, int vid)
>> +{
>> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
>> +}
>> +
>> +static bool vlan_is_set(VirtIONet *n, int vid)
>> +{
>> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
>
> This one looks wrong. Did you check this does not break vlans?

I don't know how to use vlans (more than one).

Current code is wrong (it is not big<->little endian safe.
And it is not trivial to make it correct and work from big/little endian
old states.

I commented in the cover patch that this needed testing/investigation.
How is that supposde to be tested?

Later, Juan.
Michael S. Tsirkin Dec. 2, 2009, 6:29 p.m.
On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |   21 ++++++++++++++++++---
> >>  1 files changed, 18 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 97db0d0..cf13e94 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -63,6 +63,21 @@ typedef struct VirtIONet
> >>   * - we could suppress RX interrupt if we were so inclined.
> >>   */
> >> 
> >> +static void vlan_add(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static void vlan_del(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static bool vlan_is_set(VirtIONet *n, int vid)
> >> +{
> >> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
> >
> > This one looks wrong. Did you check this does not break vlans?
> 
> I don't know how to use vlans (more than one).
> 
> Current code is wrong (it is not big<->little endian safe.
> And it is not trivial to make it correct and work from big/little endian
> old states.

So migration is broken and you want to fix it, fine,
but please do not break the feature itself.
You can't check whether bit "vid"
is set by doing  & ~(1 << vid), can you?

> I commented in the cover patch that this needed testing/investigation.
> How is that supposde to be tested?
> 
> Later, Juan.

Pass in a packet, see if it makes it in?
Juan Quintela Dec. 2, 2009, 6:53 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  hw/virtio-net.c |   21 ++++++++++++++++++---
>> >>  1 files changed, 18 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> >> index 97db0d0..cf13e94 100644
>> >> --- a/hw/virtio-net.c
>> >> +++ b/hw/virtio-net.c
>> >> @@ -63,6 +63,21 @@ typedef struct VirtIONet
>> >>   * - we could suppress RX interrupt if we were so inclined.
>> >>   */
>> >> 
>> >> +static void vlan_add(VirtIONet *n, int vid)
>> >> +{
>> >> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
>> >> +}
>> >> +
>> >> +static void vlan_del(VirtIONet *n, int vid)
>> >> +{
>> >> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
>> >> +}
>> >> +
>> >> +static bool vlan_is_set(VirtIONet *n, int vid)
>> >> +{
>> >> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
>> >
>> > This one looks wrong. Did you check this does not break vlans?
>> 
>> I don't know how to use vlans (more than one).
>> 
>> Current code is wrong (it is not big<->little endian safe.
>> And it is not trivial to make it correct and work from big/little endian
>> old states.
>
> So migration is broken and you want to fix it, fine,
> but please do not break the feature itself.
> You can't check whether bit "vid"
> is set by doing  & ~(1 << vid), can you?

/me stares at it.  stares something more.
I am enlighted now!!!

/me put brown paper bag on head.

Fixing in next version, thanks.
>
>> I commented in the cover patch that this needed testing/investigation.
>> How is that supposde to be tested?
>> 
>> Later, Juan.
>
> Pass in a packet, see if it makes it in?

I use default networking, and networking works.  but I don't have more
than one vlan.

Later, Juan.

Patch hide | download patch | download mbox

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 97db0d0..cf13e94 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -63,6 +63,21 @@  typedef struct VirtIONet
  * - we could suppress RX interrupt if we were so inclined.
  */

+static void vlan_add(VirtIONet *n, int vid)
+{
+    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
+}
+
+static void vlan_del(VirtIONet *n, int vid)
+{
+    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
+}
+
+static bool vlan_is_set(VirtIONet *n, int vid)
+{
+    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
+}
+
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
@@ -306,9 +321,9 @@  static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;

     if (cmd == VIRTIO_NET_CTRL_VLAN_ADD)
-        n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
+        vlan_add(n, vid);
     else if (cmd == VIRTIO_NET_CTRL_VLAN_DEL)
-        n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
+        vlan_del(n, vid);
     else
         return VIRTIO_NET_ERR;

@@ -471,7 +486,7 @@  static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)

     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
-        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
+        if (!vlan_is_set(n, vid))
             return 0;
     }