| Message ID | 1231351563.7109.130.camel@lappy (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote: > virtio_net: Add MAC fitler table support > > Signed-off-by: Alex Williamson <alex.williamson@hp.com> > --- > > drivers/net/virtio_net.c | 52 +++++++++++++++++++++++++++++++++++++++++--- > include/linux/virtio_net.h | 6 ++++- > 2 files changed, 54 insertions(+), 4 deletions(-) > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index f502edd..d751711 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -505,6 +505,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > struct virtnet_info *vi = netdev_priv(dev); > struct virtio_device *vdev = vi->vdev; > u16 status = vi->status.raw; > + struct dev_addr_list *uc_ptr, *mc_ptr; > + int i; > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) > return; > @@ -519,11 +521,55 @@ static void virtnet_set_rx_mode(struct net_device *dev) > else > status &= ~VIRTIO_NET_S_ALLMULTI; > > - if (dev->uc_count) > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_MAC_TABLE)) { > + if (dev->uc_count) > + status |= VIRTIO_NET_S_PROMISC; > + if (dev->mc_count) > + status |= VIRTIO_NET_S_ALLMULTI; Maybe a goto set_status here? > + if (status != vi->status.raw) { > + vi->status.raw = status; > + vdev->config->set(vdev, > + offsetof(struct virtio_net_config, > + status), &vi->status, > + sizeof(vi->status)); > + } > + return; > + } > + > + if (dev->uc_count > 16) { > status |= VIRTIO_NET_S_PROMISC; > - if (dev->mc_count) > + if (dev->mc_count > 16) > + status |= VIRTIO_NET_S_ALLMULTI; > + } else if (dev->uc_count + dev->mc_count > 16) > status |= VIRTIO_NET_S_ALLMULTI; > > + if ((dev->uc_count && !(status & VIRTIO_NET_S_PROMISC)) || > + (dev->mc_count && !(status & VIRTIO_NET_S_ALLMULTI))) > + status |= VIRTIO_NET_S_MAC_TABLE; > + else > + status &= ~VIRTIO_NET_S_MAC_TABLE; > + > + uc_ptr = dev->uc_list; > + mc_ptr = dev->mc_list; > + > + for (i = 0; i < 16; i++) { > + uint8_t entry[8] = { 0 }; > + > + if (uc_ptr && !(status & VIRTIO_NET_S_PROMISC)) { > + memcpy(entry, uc_ptr->da_addr, 6); Use ETH_ALEN. > + entry[7] = 1; > + uc_ptr = uc_ptr->next; > + } else if (mc_ptr && !(status & VIRTIO_NET_S_ALLMULTI)) { > + memcpy(entry, mc_ptr->da_addr, 6); > + entry[7] = 1; > + mc_ptr = mc_ptr->next; > + } > + > + vdev->config->set(vdev, offsetof(struct virtio_net_config, > + mac_table) + (sizeof(entry) * i), > + &entry, sizeof(entry)); > + } > + > if (status != vi->status.raw) { > vi->status.raw = status; > vdev->config->set(vdev, offsetof(struct virtio_net_config, > @@ -744,7 +790,7 @@ static unsigned int features[] = { > VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6, > VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, > VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */ > - VIRTIO_NET_F_STATUS, > + VIRTIO_NET_F_STATUS, VIRTIO_NET_F_MAC_TABLE, > VIRTIO_F_NOTIFY_ON_EMPTY, > }; > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 5a70edb..905319b 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -21,10 +21,12 @@ > #define VIRTIO_NET_F_HOST_ECN 13 /* Host can handle TSO[6] w/ ECN in. */ > #define VIRTIO_NET_F_HOST_UFO 14 /* Host can handle UFO in. */ > #define VIRTIO_NET_F_STATUS 16 /* virtio_net_config.status available */ > +#define VIRTIO_NET_F_MAC_TABLE 17 /* Additional MAC addresses */ > > #define VIRTIO_NET_S_LINK_UP 1 /* Link is up */ > #define VIRTIO_NET_S_PROMISC 2 /* Promiscuous mode */ > #define VIRTIO_NET_S_ALLMULTI 4 /* All-multicast mode */ > +#define VIRTIO_NET_S_MAC_TABLE 8 /* Enable MAC filter table */ > > struct virtio_net_config > { > @@ -38,8 +40,10 @@ struct virtio_net_config > __u16 link:1; > __u16 promisc:1; > __u16 allmulti:1; > + __u16 mac_table:1; > } bits; > - } status; > + } status; > + __u64 mac_table[16]; You're using two bytes per entry to indicate the flag is valid. Why not an array of 6 byte entries with a count of how many entries are valid? That would also keep the virtio-net I/O space under 128 bytes. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2009-01-09 at 11:34 +0000, Mark McLoughlin wrote: > On Wed, 2009-01-07 at 11:06 -0700, Alex Williamson wrote: > > @@ -38,8 +40,10 @@ struct virtio_net_config > > __u16 link:1; > > __u16 promisc:1; > > __u16 allmulti:1; > > + __u16 mac_table:1; > > } bits; > > - } status; > > + } status; > > + __u64 mac_table[16]; > > You're using two bytes per entry to indicate the flag is valid. Why > not > an array of 6 byte entries with a count of how many entries are valid? > > That would also keep the virtio-net I/O space under 128 bytes. Thanks for the comments, they look correct. For the mac_table, that's exactly how I'm thinking of doing it with virt-queues. The problem with that here is that the driver has direct access to the table and doesn't necessarily have to make it contiguous. If we expose a table, I think the more space efficient way is to make the table size a multiple of 8 and have a bitmap of valid entries. The virt-queue implementation I'm thinking of has 2 interfaces, ALLOC and SET. ALLOC can only be called once by the guest driver and allocates a mac filter table supporting the given number of entries. The SET interface provides a contiguous list of MACs, fitting into the size previously allocated. SET can also be used to clear if called with no entries. I'm steering away from a dynamically changeable size as that implies locking or some kind of linked list implementation, or both. Thanks, Alex
On Thursday 08 January 2009 04:36:03 Alex Williamson wrote:
> virtio_net: Add MAC fitler table support
Ah, I see. You really want multiple mac addresses, not just multicast
filtering?
Anthony, you think a control channel? We can add a virtqueue, but it seems like a lot of work...
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-01-10 at 21:48 +1030, Rusty Russell wrote: > On Thursday 08 January 2009 04:36:03 Alex Williamson wrote: > > virtio_net: Add MAC fitler table support > > Ah, I see. You really want multiple mac addresses, not just multicast > filtering? > > Anthony, you think a control channel? We can add a virtqueue, but it > seems like a lot of work... Right, a MAC filter table followed by a VLAN filter table. I've got a virtqueue control channel mostly coded up, I should have something to send out early next week for comments. Thanks, Alex
Rusty Russell wrote: > On Thursday 08 January 2009 04:36:03 Alex Williamson wrote: > >> virtio_net: Add MAC fitler table support >> > > Ah, I see. You really want multiple mac addresses, not just multicast > filtering? > > Anthony, you think a control channel? We can add a virtqueue, but it seems like a lot of work... > I think it's the only way to solve the problem in a virtio friendly way. Another option would be extending the config space by a very large size. We would have to make some changes to virtio-pci to switch to MMIO but that's easy enough. From a high level perspective, I don't like the idea of having the config space be extremely large and used as a communication mechanism between guests. It really should be for device configuration data that's relatively static. I think we abuse the config space in the virtio-balloon driver. Ideally, you'd have an area of guest memory sized by the guest (so there was no intrinsic limit on table size) that was given to the host to use as the filter tables. The only way this works with virtio is if you send this over a virtqueue in the form of messages. You could write a pfn to the config space but then you lose all the mapping/unmapping abstraction that virtqueue gives you (even though we don't do anything useful with that abstraction today :-)). So yeah, I think a control queue is the way to go. Regards, Anthony Liguori > Rusty. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-01-10 at 12:18 -0600, Anthony Liguori wrote: > Ideally, you'd have an area of guest memory sized by the guest (so there > was no intrinsic limit on table size) that was given to the host to use > as the filter tables. The only way this works with virtio is if you > send this over a virtqueue in the form of messages. You could write a > pfn to the config space but then you lose all the mapping/unmapping > abstraction that virtqueue gives you (even though we don't do anything > useful with that abstraction today :-)). Hmm, that's not quite how I was implementing it. The uc_list and mc_list are stored up in the netdev level, so there's not much point in duplicating it in the guest virtio-net driver. The interface I was working on has two commands. The first tells the host to allocate the MAC filter table for a guest provided number of entries (perhaps a module parameter, with reasonable default). The other is a set command with an sg entry providing a buffer of all the MAC entries for the table. If sg entries are no more than a page, this limits us to ~680 MAC table entries, which I think is far more than any piece of real hardware (and large enough that you'd probably want to turn on promiscuous already). The VLAN equivalent is a bit easier since by definition there are 4k possible VLANs. There I think a set bit/clear bit message interface is appropriate (and maybe a clear all for a reset condition). Let me know if that sounds reasonable. Thanks, Alex
Alex Williamson wrote: > Hmm, that's not quite how I was implementing it. The uc_list and > mc_list are stored up in the netdev level, so there's not much point in > duplicating it in the guest virtio-net driver. The interface I was > working on has two commands. The first tells the host to allocate the > MAC filter table for a guest provided number of entries (perhaps a > module parameter, with reasonable default). The other is a set command > with an sg entry providing a buffer of all the MAC entries for the > table. If sg entries are no more than a page, this limits us to ~680 > MAC table entries, which I think is far more than any piece of real > hardware (and large enough that you'd probably want to turn on > promiscuous already). Yeah, this is what I would have done although maybe it's worth allowing a partial update of the filter table. Once you're using a command interface, a protocol like you describe makes sense. I was simply going the through the logic that led me to suggest a command interface in the first place. > The VLAN equivalent is a bit easier since by > definition there are 4k possible VLANs. There I think a set bit/clear > bit message interface is appropriate (and maybe a clear all for a reset > condition). Let me know if that sounds reasonable. Thanks, > Yeah, sounds reasonable to me. Regards, Anthony Liguori > Alex > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index f502edd..d751711 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -505,6 +505,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) struct virtnet_info *vi = netdev_priv(dev); struct virtio_device *vdev = vi->vdev; u16 status = vi->status.raw; + struct dev_addr_list *uc_ptr, *mc_ptr; + int i; if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) return; @@ -519,11 +521,55 @@ static void virtnet_set_rx_mode(struct net_device *dev) else status &= ~VIRTIO_NET_S_ALLMULTI; - if (dev->uc_count) + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_MAC_TABLE)) { + if (dev->uc_count) + status |= VIRTIO_NET_S_PROMISC; + if (dev->mc_count) + status |= VIRTIO_NET_S_ALLMULTI; + if (status != vi->status.raw) { + vi->status.raw = status; + vdev->config->set(vdev, + offsetof(struct virtio_net_config, + status), &vi->status, + sizeof(vi->status)); + } + return; + } + + if (dev->uc_count > 16) { status |= VIRTIO_NET_S_PROMISC; - if (dev->mc_count) + if (dev->mc_count > 16) + status |= VIRTIO_NET_S_ALLMULTI; + } else if (dev->uc_count + dev->mc_count > 16) status |= VIRTIO_NET_S_ALLMULTI; + if ((dev->uc_count && !(status & VIRTIO_NET_S_PROMISC)) || + (dev->mc_count && !(status & VIRTIO_NET_S_ALLMULTI))) + status |= VIRTIO_NET_S_MAC_TABLE; + else + status &= ~VIRTIO_NET_S_MAC_TABLE; + + uc_ptr = dev->uc_list; + mc_ptr = dev->mc_list; + + for (i = 0; i < 16; i++) { + uint8_t entry[8] = { 0 }; + + if (uc_ptr && !(status & VIRTIO_NET_S_PROMISC)) { + memcpy(entry, uc_ptr->da_addr, 6); + entry[7] = 1; + uc_ptr = uc_ptr->next; + } else if (mc_ptr && !(status & VIRTIO_NET_S_ALLMULTI)) { + memcpy(entry, mc_ptr->da_addr, 6); + entry[7] = 1; + mc_ptr = mc_ptr->next; + } + + vdev->config->set(vdev, offsetof(struct virtio_net_config, + mac_table) + (sizeof(entry) * i), + &entry, sizeof(entry)); + } + if (status != vi->status.raw) { vi->status.raw = status; vdev->config->set(vdev, offsetof(struct virtio_net_config, @@ -744,7 +790,7 @@ static unsigned int features[] = { VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6, VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_ECN, /* We don't yet handle UFO input. */ - VIRTIO_NET_F_STATUS, + VIRTIO_NET_F_STATUS, VIRTIO_NET_F_MAC_TABLE, VIRTIO_F_NOTIFY_ON_EMPTY, }; diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 5a70edb..905319b 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -21,10 +21,12 @@ #define VIRTIO_NET_F_HOST_ECN 13 /* Host can handle TSO[6] w/ ECN in. */ #define VIRTIO_NET_F_HOST_UFO 14 /* Host can handle UFO in. */ #define VIRTIO_NET_F_STATUS 16 /* virtio_net_config.status available */ +#define VIRTIO_NET_F_MAC_TABLE 17 /* Additional MAC addresses */ #define VIRTIO_NET_S_LINK_UP 1 /* Link is up */ #define VIRTIO_NET_S_PROMISC 2 /* Promiscuous mode */ #define VIRTIO_NET_S_ALLMULTI 4 /* All-multicast mode */ +#define VIRTIO_NET_S_MAC_TABLE 8 /* Enable MAC filter table */ struct virtio_net_config { @@ -38,8 +40,10 @@ struct virtio_net_config __u16 link:1; __u16 promisc:1; __u16 allmulti:1; + __u16 mac_table:1; } bits; - } status; + } status; + __u64 mac_table[16]; } __attribute__((packed)); /* This is the first element of the scatter-gather list. If you don't
virtio_net: Add MAC fitler table support Signed-off-by: Alex Williamson <alex.williamson@hp.com> --- drivers/net/virtio_net.c | 52 +++++++++++++++++++++++++++++++++++++++++--- include/linux/virtio_net.h | 6 ++++- 2 files changed, 54 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html