Amos Kong <ak...@redhat.com> writes: > We want to implement mac programming over macvtap through Libvirt, > related rx-filter configuration contains main mac, some of rx-mode > and mac-table. > > The previous patch adds QMP event to notify management of rx-filter > change. This patch adds a monitor command to query rx-filter > information. > > A flag is used to avoid events flooding, if user don't query > rx-filter after receives one event, new events won't be sent > to qmp monitor. > > (qemu) info rx-filter vnet0 > vnet0: > \ promiscuous: on > \ multicast: normal > \ unicast: normal > \ broadcast-allowed: off > \ multicast-overflow: off > \ unicast-overflow: off > \ main-mac: 52:54:00:12:34:56 > \ unicast-table: > \ multicast-table: > 01:00:5e:00:00:01 > 33:33:00:00:00:01 > 33:33:ff:12:34:56 > > Signed-off-by: Amos Kong <ak...@redhat.com> > --- > hmp-commands.hx | 2 ++ > hmp.c | 49 ++++++++++++++++++++++++++++ > hmp.h | 1 + > hw/net/virtio-net.c | 93 > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/net/net.h | 2 ++ > monitor.c | 8 +++++ > net/net.c | 47 +++++++++++++++++++++++++++ > qapi-schema.json | 73 +++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 55 +++++++++++++++++++++++++++++++ > 9 files changed, 330 insertions(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 9cea415..b7863eb 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1639,6 +1639,8 @@ show qdev device model list > show roms > @item info tpm > show the TPM device > +@item info rx-filter [net client name] > +show the rx-filter information for all nics (or for the given nic)
Humor me: spell it NICs and NIC, because it's an acronym. > @end table > ETEXI > > diff --git a/hmp.c b/hmp.c > index 4fb76ec..5b47382 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict) > qapi_free_TPMInfoList(info_list); > } > > +void hmp_info_rx_filter(Monitor *mon, const QDict *qdict) > +{ > + RxFilterInfoList *filter_list, *entry; > + strList *str_entry; > + bool has_name = qdict_haskey(qdict, "name"); > + const char *name = NULL; > + > + if (has_name) { > + name = qdict_get_str(qdict, "name"); > + } > + > + filter_list = qmp_query_rx_filter(has_name, name, NULL); Rather roundabout way to do const char *name = qdict_get_str(qdict, "name"); filter_list = qmp_query_rx_filter(name != NULL, name); > + entry = filter_list; > + > + while (entry) { Recommend to put the loop control in one place: for (entry = filter_list; entry; entry = entry->next) { > + monitor_printf(mon, "%s:\n", entry->value->name); > + monitor_printf(mon, " \\ promiscuous: %s\n", > + entry->value->promiscuous ? "on" : "off"); > + monitor_printf(mon, " \\ multicast: %s\n", > + RxState_lookup[entry->value->multicast]); > + monitor_printf(mon, " \\ unicast: %s\n", > + RxState_lookup[entry->value->unicast]); > + monitor_printf(mon, " \\ broadcast-allowed: %s\n", > + entry->value->broadcast_allowed ? "on" : "off"); > + monitor_printf(mon, " \\ multicast-overflow: %s\n", > + entry->value->multicast_overflow ? "on" : "off"); > + monitor_printf(mon, " \\ unicast-overflow: %s\n", > + entry->value->unicast_overflow ? "on" : "off"); > + monitor_printf(mon, " \\ main-mac: %s\n", entry->value->main_mac); > + > + str_entry = entry->value->unicast_table; > + monitor_printf(mon, " \\ unicast-table:\n"); > + while (str_entry) { > + monitor_printf(mon, " %s\n", str_entry->value); > + str_entry = str_entry->next; > + } > + > + str_entry = entry->value->multicast_table; > + monitor_printf(mon, " \\ multicast-table:\n"); > + while (str_entry) { > + monitor_printf(mon, " %s\n", str_entry->value); > + str_entry = str_entry->next; > + } > + > + entry = entry->next; > + } > + qapi_free_RxFilterInfoList(filter_list); > +} > + > void hmp_quit(Monitor *mon, const QDict *qdict) > { > monitor_suspend(mon); > diff --git a/hmp.h b/hmp.h > index 95fe76e..9af733e 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict); > void hmp_info_pci(Monitor *mon, const QDict *qdict); > void hmp_info_block_jobs(Monitor *mon, const QDict *qdict); > void hmp_info_tpm(Monitor *mon, const QDict *qdict); > +void hmp_info_rx_filter(Monitor *mon, const QDict *qdict); > void hmp_quit(Monitor *mon, const QDict *qdict); > void hmp_stop(Monitor *mon, const QDict *qdict); > void hmp_system_reset(Monitor *mon, const QDict *qdict); > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 1ea9556..f93e021 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -21,6 +21,8 @@ > #include "hw/virtio/virtio-net.h" > #include "net/vhost_net.h" > #include "hw/virtio/virtio-bus.h" > +#include "qapi/qmp/qjson.h" > +#include "monitor/monitor.h" > > #define VIRTIO_NET_VM_VERSION 11 > > @@ -192,6 +194,90 @@ static void virtio_net_set_link_status(NetClientState > *nc) > virtio_net_set_status(vdev, vdev->status); > } > > +static bool notify_enabled = true; > + > +static void rxfilter_notify(const char *name) > +{ > + QObject *event_data; > + > + if (notify_enabled) { > + event_data = qobject_from_jsonf("{ 'name': %s }", name); > + monitor_protocol_event(QEVENT_RX_FILTER_CHANGED, event_data); > + qobject_decref(event_data); > + /* disable event notification to avoid events flooding */ > + notify_enabled = false; > + } > +} > + > +static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) > +{ > + VirtIONet *n = qemu_get_nic_opaque(nc); > + RxFilterInfo *info; > + strList *str_list = NULL; > + strList *entry; > + int i; > + > + info = g_malloc0(sizeof(*info)); > + info->name = g_strdup(nc->name); > + info->promiscuous = n->promisc; > + > + if (n->nouni) { > + info->unicast = RX_STATE_NO; > + } else if (n->alluni) { > + info->unicast = RX_STATE_ALL; > + } else { > + info->unicast = RX_STATE_NORMAL; > + } > + > + if (n->nomulti) { > + info->multicast = RX_STATE_NO; > + } else if (n->allmulti) { > + info->multicast = RX_STATE_ALL; > + } else { > + info->multicast = RX_STATE_NORMAL; > + } Makes me wonder whether replacing VirtIONet members noFOO and allFOO by an enum RxState would make things clearer there. Outside the scope of this patch. > + > + info->broadcast_allowed = n->nobcast; > + info->multicast_overflow = n->mac_table.multi_overflow; > + info->unicast_overflow = n->mac_table.uni_overflow; > + info->main_mac = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", > + n->mac[0], n->mac[1], n->mac[2], > + n->mac[3], n->mac[4], n->mac[5]); > + Please initialize str_list here rather than at its declaration, because that'll make the loop's workings more locally obvious, and because... > + for (i = 0; i < n->mac_table.first_multi; i++) { > + entry = g_malloc0(sizeof(*entry)); > + entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", > + n->mac_table.macs[i * ETH_ALEN], > + n->mac_table.macs[i * ETH_ALEN + 1], > + n->mac_table.macs[i * ETH_ALEN + 2], > + n->mac_table.macs[i * ETH_ALEN + 3], > + n->mac_table.macs[i * ETH_ALEN + 4], > + n->mac_table.macs[i * ETH_ALEN + 5]); > + entry->next = str_list; > + str_list = entry; > + } > + info->unicast_table = str_list; > + ... it's how this loop works. Actually, the two loops are duplicates. Consider factoring out a helper function. > + str_list = NULL; > + for (i = n->mac_table.first_multi; i < n->mac_table.in_use; i++) { > + entry = g_malloc0(sizeof(*entry)); > + entry->value = g_strdup_printf("%.2x:%.2x:%.2x:%.2x:%.2x:%.2x", > + n->mac_table.macs[i * ETH_ALEN], > + n->mac_table.macs[i * ETH_ALEN + 1], > + n->mac_table.macs[i * ETH_ALEN + 2], > + n->mac_table.macs[i * ETH_ALEN + 3], > + n->mac_table.macs[i * ETH_ALEN + 4], > + n->mac_table.macs[i * ETH_ALEN + 5]); > + entry->next = str_list; > + str_list = entry; > + } > + info->multicast_table = str_list; > + /* enable event notification after query */ > + notify_enabled = true; Combined with the optional argument, this leads to rather weird behavior: 1. Querying all NICs reenables the event for all NICs. Okay. 2. Querying a single virtio-net NIC reenables the event for all virtio-net NICs. Weird. Currently, only virtio-net NICs support the event, so the weirdness isn't visible. Regardless, please clean this up: * Either move notify_enabled into device state, or * Move notify_enabled somewhere global, along with rxfilter_notify(). > + > + return info; > +} > + > static void virtio_net_reset(VirtIODevice *vdev) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -442,6 +528,8 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, > uint8_t cmd, > return VIRTIO_NET_ERR; > } > > + rxfilter_notify(n->netclient_name); > + > return VIRTIO_NET_OK; > } > > @@ -495,6 +583,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t > cmd, > s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac)); > assert(s == sizeof(n->mac)); > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > + rxfilter_notify(n->netclient_name); > + > return VIRTIO_NET_OK; > } > > @@ -559,6 +649,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t > cmd, > n->mac_table.multi_overflow = 1; > } > > + rxfilter_notify(n->netclient_name); > + > return VIRTIO_NET_OK; > } > The error returns don't trigger the event. We can fail after clearing n->mac_table. Why is that okay? > @@ -1312,6 +1404,7 @@ static NetClientInfo net_virtio_info = { > .receive = virtio_net_receive, > .cleanup = virtio_net_cleanup, > .link_status_changed = virtio_net_set_link_status, > + .query_rx_filter = virtio_net_query_rxfilter, > }; > > static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx) > diff --git a/include/net/net.h b/include/net/net.h > index 43d85a1..0af5ba3 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -49,6 +49,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const > struct iovec *, int); > typedef void (NetCleanup) (NetClientState *); > typedef void (LinkStatusChanged)(NetClientState *); > typedef void (NetClientDestructor)(NetClientState *); > +typedef RxFilterInfo *(QueryRxFilter)(NetClientState *); > > typedef struct NetClientInfo { > NetClientOptionsKind type; > @@ -59,6 +60,7 @@ typedef struct NetClientInfo { > NetCanReceive *can_receive; > NetCleanup *cleanup; > LinkStatusChanged *link_status_changed; > + QueryRxFilter *query_rx_filter; > NetPoll *poll; > } NetClientInfo; > > diff --git a/monitor.c b/monitor.c > index 4f7bd48..81ac50c 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2764,6 +2764,14 @@ static mon_cmd_t info_cmds[] = { > .mhandler.cmd = hmp_info_tpm, > }, > { > + .name = "rx-filter", nic-rx-filter? > + .args_type = "name:s?", > + .params = "[net client name]", > + .help = "show the rx-filter information for all nics (or" > + " for the given nic)", NICs, NIC. > + .mhandler.cmd = hmp_info_rx_filter, > + }, > + { > .name = NULL, > }, > }; > diff --git a/net/net.c b/net/net.c > index 43a74e4..7b73a10 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState *nc) > nc->info_str); > } > > +RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name, > + Error **errp) > +{ > + NetClientState *nc; > + RxFilterInfoList *filter_list = NULL, *last_entry = NULL; > + > + QTAILQ_FOREACH(nc, &net_clients, next) { > + RxFilterInfoList *entry; > + RxFilterInfo *info; > + > + if (nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC) { > + continue; > + } > + if (has_name && strcmp(nc->name, name) != 0) { > + continue; > + } > + > + if (nc->info->query_rx_filter) { > + info = nc->info->query_rx_filter(nc); > + entry = g_malloc0(sizeof(*entry)); > + entry->value = info; > + > + if (!filter_list) { > + filter_list = entry; > + } else { > + last_entry->next = entry; > + } > + last_entry = entry; > + } else if (has_name) { > + error_setg(errp, "net client(%s) doesn't support" > + " rx-filter querying", name); > + break; > + } > + > + } > + > + if (filter_list == NULL && !error_is_set(errp)) { > + if (has_name) { > + error_setg(errp, "invalid net client name: %s", name); > + } else { > + error_setg(errp, "no net client supports rx-filter querying"); Why is this an error? > + } > + } > + > + return filter_list; > +} > + > void do_info_network(Monitor *mon, const QDict *qdict) > { > NetClientState *nc, *peer; > diff --git a/qapi-schema.json b/qapi-schema.json > index 664b31f..cac3e16 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3619,3 +3619,76 @@ > '*cpuid-input-ecx': 'int', > 'cpuid-register': 'X86CPURegister32', > 'features': 'int' } } > + > +## > +# @RxState: > +# > +# Packets receiving state > +# > +# @normal: filter assigned packets according to the mac-table > +# > +# @no: don't receive any assigned packet > +# > +# @all: receive all assigned packets > +# > +## > +{ 'enum': 'RxState', 'data': [ 'normal', 'no', 'all' ] } > + > +## > +# @RxFilterInfo: > +# > +# Rx-filter information for a net client, it contains main mac, some > +# of rx-mode items and mac-table. > +# > +# @name: net client name > +# > +# @promiscuous: whether to ether promiscuous mode > +# > +# @multicast: multicast receive state > +# > +# @unicast: unicast receive state > +# > +# @broadcast-allowed: whether to receive broadcast > +# > +# @multicast-overflow: multicast table is overflow or not > +# > +# @unicast-overflow: unicast table is overflow or not > +# > +# @main-mac: the main macaddr string > +# > +# @unicast-table: a list of unicast macaddr string > +# > +# @multicast-table: a list of multicast macaddr string > +# > +# Since 1.6 > +## > + > +{ 'type': 'RxFilterInfo', > + 'data': { > + 'name': 'str', > + 'promiscuous': 'bool', > + 'multicast': 'RxState', > + 'unicast': 'RxState', > + 'broadcast-allowed': 'bool', > + 'multicast-overflow': 'bool', > + 'unicast-overflow': 'bool', > + 'main-mac': 'str', > + 'unicast-table': ['str'], > + 'multicast-table': ['str'] }} > + > +## > +# @query-rx-filter: > +# > +# Return rx-filter information for all nics (or for the given nic). > +# > +# @name: #optional net client name > +# > +# Returns: list of @RxFilterInfo for all nics (or for the given nic). > +# Returns an error if the given @name doesn't exist, or given > +# nic doesn't support rx-filter querying, or no net client > +# supports rx-filter querying NICs, NIC. The case "no net client supports rx-filter querying" should return an empty list, not throw an error. I think the case "@name exists but isn't a NIC" should be a separate error. > +# > +# Since: 1.6 > +## > +{ 'command': 'query-rx-filter', 'data': { '*name': 'str' }, query-nic-rx-filter? Do we want to complicate the QMP interface with the optional argument? Discussed elsewhere in the thread, no need to answer again here. > + 'returns': ['RxFilterInfo'] } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ffd130e..817460b 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2932,3 +2932,58 @@ Example: > <- { "return": {} } > > EQMP > + { > + .name = "query-rx-filter", > + .args_type = "name:s?", > + .mhandler.cmd_new = qmp_marshal_input_query_rx_filter, > + }, > + > +SQMP > +query-rx-filter > +--------------- > + > +Show rx-filter information. > + > +Returns a json-array of rx-filter information for all nics (or for the > +given nic), returning an error if the given nic doesn't exist, or > +given nic doesn't support rx-filter querying, or no net client > +supports rx-filter querying. Comments on @query-rx-filter in qapi-schema.json apply. > + > +Each array entry contains the following: > + > +- "name": net client name (jaso-string) json-string > +- "promiscuous": enter promiscuous mode (json-bool) > +- "multicast": multicast receive state (one of 'normal', 'no', 'all') > +- "unicast": unicast receive state (one of 'normal', 'no', 'all') > +- "broadcast-allowed": allow to receive broadcast (json-bool) > +- "multicast-overflow": multicast table is overflow (json-bool) > +- "unicast-overflow": unicast table is overflow (json-bool) > +- "main-mac": main macaddr string (jaso-string) json-string > +- "unicast-table": a json-array of unicast macaddr string > +- "multicast-table": a json-array of multicast macaddr string > + > +Example: > + > +-> { "execute": "query-rx-filter", "arguments": { "name": "vnet0" }} > +<- { "return": [ > + { > + "promiscuous": true, > + "name": "vnet0", > + "main-mac": "52:54:00:12:34:56", > + "unicast": "normal", > + "unicast-table": [ > + ], > + "multicast": "normal", > + "multicast-overflow": false, > + "unicast-overflow": false, > + "multicast-table": [ > + "01:00:5e:00:00:01", > + "33:33:00:00:00:01", > + "33:33:ff:12:34:56" > + ], > + "broadcast-allowed": false > + } > + ] > + } > + > +EQMP This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? 2. If the former, are you reasonably sure this object will do for other NICs?