* [PATCH 2/2] ipoib: show VF broadcast address
2019-06-13 14:20 [PATCH 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
@ 2019-06-13 14:20 ` Denis Kirjanov
2019-06-13 14:20 ` [PATCH net-next v2 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
2019-06-13 14:20 ` [PATCH net-next v2 2/2] ipoib: show VF broadcast address Denis Kirjanov
2 siblings, 0 replies; 7+ messages in thread
From: Denis Kirjanov @ 2019-06-13 14:20 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
in IPoIB case we can't see a VF broadcast address for but
can see for PF
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: add the IFLA_VF_BROADCAST constant
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
net/core/rtnetlink.c | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5b225ff63b48..1f36dd3a45d6 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -681,6 +681,7 @@ enum {
enum {
IFLA_VF_UNSPEC,
IFLA_VF_MAC, /* Hardware queue specific attributes */
+ IFLA_VF_BROADCAST,
IFLA_VF_VLAN, /* VLAN ID and QoS */
IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
@@ -704,6 +705,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cec60583931f..88304212f127 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -908,6 +908,7 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
size += num_vfs *
(nla_total_size(0) +
nla_total_size(sizeof(struct ifla_vf_mac)) +
+ nla_total_size(sizeof(struct ifla_vf_broadcast)) +
nla_total_size(sizeof(struct ifla_vf_vlan)) +
nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST */
nla_total_size(MAX_VLAN_LIST_LEN *
@@ -1197,6 +1198,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
struct ifla_vf_vlan vf_vlan;
struct ifla_vf_rate vf_rate;
struct ifla_vf_mac vf_mac;
+ struct ifla_vf_broadcast vf_broadcast;
struct ifla_vf_info ivi;
memset(&ivi, 0, sizeof(ivi));
@@ -1231,6 +1233,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
vf_trust.vf = ivi.vf;
memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
+ memcpy(vf_broadcast.broadcast, dev->broadcast, dev->addr_len);
vf_vlan.vlan = ivi.vlan;
vf_vlan.qos = ivi.qos;
vf_vlan_info.vlan = ivi.vlan;
@@ -1247,8 +1250,10 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
if (!vf)
goto nla_put_vfinfo_failure;
if (nla_put(skb, IFLA_VF_MAC, sizeof(vf_mac), &vf_mac) ||
+ nla_put(skb, IFLA_VF_BROADCAST, sizeof(vf_broadcast), &vf_broadcast) ||
nla_put(skb, IFLA_VF_VLAN, sizeof(vf_vlan), &vf_vlan) ||
nla_put(skb, IFLA_VF_RATE, sizeof(vf_rate),
+
&vf_rate) ||
nla_put(skb, IFLA_VF_TX_RATE, sizeof(vf_tx_rate),
&vf_tx_rate) ||
@@ -1753,6 +1758,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
+ [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
[IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
[IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
[IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
--
2.12.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH net-next v2 1/2] ipoib: correcly show a VF hardware address
2019-06-13 14:20 [PATCH 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
2019-06-13 14:20 ` [PATCH 2/2] ipoib: show VF broadcast address Denis Kirjanov
@ 2019-06-13 14:20 ` Denis Kirjanov
2019-06-13 15:24 ` Doug Ledford
2019-06-13 14:20 ` [PATCH net-next v2 2/2] ipoib: show VF broadcast address Denis Kirjanov
2 siblings, 1 reply; 7+ messages in thread
From: Denis Kirjanov @ 2019-06-13 14:20 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
in the case of IPoIB with SRIOV enabled hardware
ip link show command incorrecly prints
0 instead of a VF hardware address. To correcly print the address
add a new field to specify an address length.
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 9b5e11d3fb85..04ea7db08e87 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1998,6 +1998,7 @@ static int ipoib_get_vf_config(struct net_device *dev, int vf,
return err;
ivf->vf = vf;
+ memcpy(ivf->mac, dev->dev_addr, dev->addr_len);
return 0;
}
--
2.12.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net-next v2 1/2] ipoib: correcly show a VF hardware address
2019-06-13 14:20 ` [PATCH net-next v2 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
@ 2019-06-13 15:24 ` Doug Ledford
0 siblings, 0 replies; 7+ messages in thread
From: Doug Ledford @ 2019-06-13 15:24 UTC (permalink / raw)
To: Denis Kirjanov, davem; +Cc: netdev, linux-rdma, mkubecek
[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]
On Thu, 2019-06-13 at 16:20 +0200, Denis Kirjanov wrote:
> in the case of IPoIB with SRIOV enabled hardware
> ip link show command incorrecly prints
> 0 instead of a VF hardware address. To correcly print the address
> add a new field to specify an address length.
>
> Before:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state
> disable,
> trust off, query_rss off
> ...
> After:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
> checking off, link-state disable, trust off, query_rss off
>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 9b5e11d3fb85..04ea7db08e87 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -1998,6 +1998,7 @@ static int ipoib_get_vf_config(struct
> net_device *dev, int vf,
> return err;
>
> ivf->vf = vf;
> + memcpy(ivf->mac, dev->dev_addr, dev->addr_len);
>
> return 0;
> }
I'm ok with the patch, but your commit message does not match what the
patch does at all. You need to correct the commit message.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 2/2] ipoib: show VF broadcast address
2019-06-13 14:20 [PATCH 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
2019-06-13 14:20 ` [PATCH 2/2] ipoib: show VF broadcast address Denis Kirjanov
2019-06-13 14:20 ` [PATCH net-next v2 1/2] ipoib: correcly show a VF hardware address Denis Kirjanov
@ 2019-06-13 14:20 ` Denis Kirjanov
2019-06-13 16:39 ` Michal Kubecek
2 siblings, 1 reply; 7+ messages in thread
From: Denis Kirjanov @ 2019-06-13 14:20 UTC (permalink / raw)
To: davem, dledford; +Cc: netdev, linux-rdma, mkubecek, Denis Kirjanov
in IPoIB case we can't see a VF broadcast address for but
can see for PF
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
net/core/rtnetlink.c | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5b225ff63b48..1f36dd3a45d6 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -681,6 +681,7 @@ enum {
enum {
IFLA_VF_UNSPEC,
IFLA_VF_MAC, /* Hardware queue specific attributes */
+ IFLA_VF_BROADCAST,
IFLA_VF_VLAN, /* VLAN ID and QoS */
IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
@@ -704,6 +705,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cec60583931f..88304212f127 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -908,6 +908,7 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
size += num_vfs *
(nla_total_size(0) +
nla_total_size(sizeof(struct ifla_vf_mac)) +
+ nla_total_size(sizeof(struct ifla_vf_broadcast)) +
nla_total_size(sizeof(struct ifla_vf_vlan)) +
nla_total_size(0) + /* nest IFLA_VF_VLAN_LIST */
nla_total_size(MAX_VLAN_LIST_LEN *
@@ -1197,6 +1198,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
struct ifla_vf_vlan vf_vlan;
struct ifla_vf_rate vf_rate;
struct ifla_vf_mac vf_mac;
+ struct ifla_vf_broadcast vf_broadcast;
struct ifla_vf_info ivi;
memset(&ivi, 0, sizeof(ivi));
@@ -1231,6 +1233,7 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
vf_trust.vf = ivi.vf;
memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
+ memcpy(vf_broadcast.broadcast, dev->broadcast, dev->addr_len);
vf_vlan.vlan = ivi.vlan;
vf_vlan.qos = ivi.qos;
vf_vlan_info.vlan = ivi.vlan;
@@ -1247,8 +1250,10 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb,
if (!vf)
goto nla_put_vfinfo_failure;
if (nla_put(skb, IFLA_VF_MAC, sizeof(vf_mac), &vf_mac) ||
+ nla_put(skb, IFLA_VF_BROADCAST, sizeof(vf_broadcast), &vf_broadcast) ||
nla_put(skb, IFLA_VF_VLAN, sizeof(vf_vlan), &vf_vlan) ||
nla_put(skb, IFLA_VF_RATE, sizeof(vf_rate),
+
&vf_rate) ||
nla_put(skb, IFLA_VF_TX_RATE, sizeof(vf_tx_rate),
&vf_tx_rate) ||
@@ -1753,6 +1758,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
+ [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
[IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
[IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
[IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
--
2.12.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net-next v2 2/2] ipoib: show VF broadcast address
2019-06-13 14:20 ` [PATCH net-next v2 2/2] ipoib: show VF broadcast address Denis Kirjanov
@ 2019-06-13 16:39 ` Michal Kubecek
2019-06-14 13:37 ` Denis Kirjanov
0 siblings, 1 reply; 7+ messages in thread
From: Michal Kubecek @ 2019-06-13 16:39 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: davem, dledford, netdev, linux-rdma
On Thu, Jun 13, 2019 at 04:20:03PM +0200, Denis Kirjanov wrote:
> in IPoIB case we can't see a VF broadcast address for but
> can see for PF
>
> Before:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
> trust off, query_rss off
> ...
>
> After:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
> checking off, link-state disable, trust off, query_rss off
>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
> include/uapi/linux/if_link.h | 5 +++++
> net/core/rtnetlink.c | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 5b225ff63b48..1f36dd3a45d6 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -681,6 +681,7 @@ enum {
> enum {
> IFLA_VF_UNSPEC,
> IFLA_VF_MAC, /* Hardware queue specific attributes */
> + IFLA_VF_BROADCAST,
> IFLA_VF_VLAN, /* VLAN ID and QoS */
> IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
> IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
Oops, I forgot to mention one important point when reviewing v1: the new
attribute type must be added at the end (just before __IFLA_VF_MAX) so
that you do not change value of existing IFLA_VF_* constants (this would
break compatibility).
> @@ -704,6 +705,10 @@ struct ifla_vf_mac {
> __u8 mac[32]; /* MAX_ADDR_LEN */
> };
>
> +struct ifla_vf_broadcast {
> + __u8 broadcast[32];
> +};
> +
> struct ifla_vf_vlan {
> __u32 vf;
> __u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
My first idea was that to question the need of a wrapping structure as
we couldn't modify that structure in the future anyway so that there
does not seem to be any gain against simply passing the address as a
binary with attribute length equal to address length (like we do with
IFLA_ADDRESS and IFLA_BROADCAST).
But then I checked other IFLA_VF_* attributes and I'm confused. The
structure seems to be
IFLA_VF_INFO_LIST
IFLA_VF_INFO
IFLA_VF_MAC
IFLA_VF_VLAN
...
IFLA_VF_INFO
IFLA_VF_MAC
IFLA_VF_VLAN
...
...
Each IFLA_VF_INFO corresponds to one virtual function but its number is
not determined by an attribute within this nest. Instead, each of the
neste IFLA_VF_* attributes is a structure containing "__u32 vf" and it's
only matter of convention that within one IFLA_VF_INFO nest, all data
belongs to the same VF, neither do_setlink() nor do_setvfinfo() check
it.
I guess you should either follow this weird pattern or introduce proper
IFLA_VF_ID to be used for IFLA_VF_BROADCAST and all future IFLA_VF_*
attributes. However, each new attribute makes IFLA_VF_INFO bigger and
lowers the number of VFs that can be stored in an IFLA_VF_INFO_LIST nest
without exceeding the hard limit of 65535 bytes so that we cannot afford
to add too many.
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index cec60583931f..88304212f127 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
...
> @@ -1753,6 +1758,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
>
> static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
> [IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
> + [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
> [IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
> [IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
> [IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
As you do not implement setting the broadcast address (is that possible
at all?), NLA_REJECT would be more appropriate so that the request isn't
silently ignored.
Michal
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net-next v2 2/2] ipoib: show VF broadcast address
2019-06-13 16:39 ` Michal Kubecek
@ 2019-06-14 13:37 ` Denis Kirjanov
0 siblings, 0 replies; 7+ messages in thread
From: Denis Kirjanov @ 2019-06-14 13:37 UTC (permalink / raw)
To: Michal Kubecek; +Cc: davem, dledford, netdev, linux-rdma
On 6/13/19, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Thu, Jun 13, 2019 at 04:20:03PM +0200, Denis Kirjanov wrote:
>> in IPoIB case we can't see a VF broadcast address for but
>> can see for PF
>>
>> Before:
>> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
>> state UP mode DEFAULT group default qlen 256
>> link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
>> vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
>> trust off, query_rss off
>> ...
>>
>> After:
>> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
>> state UP mode DEFAULT group default qlen 256
>> link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
>> vf 0 link/infiniband
>> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
>> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
>> checking off, link-state disable, trust off, query_rss off
>>
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>> ---
>> include/uapi/linux/if_link.h | 5 +++++
>> net/core/rtnetlink.c | 6 ++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index 5b225ff63b48..1f36dd3a45d6 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -681,6 +681,7 @@ enum {
>> enum {
>> IFLA_VF_UNSPEC,
>> IFLA_VF_MAC, /* Hardware queue specific attributes */
>> + IFLA_VF_BROADCAST,
>> IFLA_VF_VLAN, /* VLAN ID and QoS */
>> IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
>> IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
>
> Oops, I forgot to mention one important point when reviewing v1: the new
> attribute type must be added at the end (just before __IFLA_VF_MAX) so
> that you do not change value of existing IFLA_VF_* constants (this would
> break compatibility).
Right, I've also missed that that the change breaks KABI.
>
>> @@ -704,6 +705,10 @@ struct ifla_vf_mac {
>> __u8 mac[32]; /* MAX_ADDR_LEN */
>> };
>>
>> +struct ifla_vf_broadcast {
>> + __u8 broadcast[32];
>> +};
>> +
>> struct ifla_vf_vlan {
>> __u32 vf;
>> __u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
>
> My first idea was that to question the need of a wrapping structure as
> we couldn't modify that structure in the future anyway so that there
> does not seem to be any gain against simply passing the address as a
> binary with attribute length equal to address length (like we do with
> IFLA_ADDRESS and IFLA_BROADCAST).
>
> But then I checked other IFLA_VF_* attributes and I'm confused. The
> structure seems to be
>
> IFLA_VF_INFO_LIST
> IFLA_VF_INFO
> IFLA_VF_MAC
> IFLA_VF_VLAN
> ...
> IFLA_VF_INFO
> IFLA_VF_MAC
> IFLA_VF_VLAN
> ...
> ...
>
> Each IFLA_VF_INFO corresponds to one virtual function but its number is
> not determined by an attribute within this nest. Instead, each of the
> neste IFLA_VF_* attributes is a structure containing "__u32 vf" and it's
> only matter of convention that within one IFLA_VF_INFO nest, all data
> belongs to the same VF, neither do_setlink() nor do_setvfinfo() check
> it.
>
> I guess you should either follow this weird pattern or introduce proper
> IFLA_VF_ID to be used for IFLA_VF_BROADCAST and all future IFLA_VF_*
> attributes. However, each new attribute makes IFLA_VF_INFO bigger and
> lowers the number of VFs that can be stored in an IFLA_VF_INFO_LIST nest
> without exceeding the hard limit of 65535 bytes so that we cannot afford
> to add too many.
I've just put it as other attrs for now.
>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index cec60583931f..88304212f127 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
> ...
>> @@ -1753,6 +1758,7 @@ static const struct nla_policy
>> ifla_info_policy[IFLA_INFO_MAX+1] = {
>>
>> static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>> [IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
>> + [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
>> [IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
>> [IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
>> [IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
>
> As you do not implement setting the broadcast address (is that possible
> at all?),
According to rfc4391 it's formed from the components like p_key,
q_key, mtu and other.
NLA_REJECT would be more appropriate so that the request isn't
> silently ignored.
Anyway, I've sent v3.
Thanks!
>
> Michal
>
^ permalink raw reply [flat|nested] 7+ messages in thread