Re: [dpdk-dev] [PATCH] net/ixgbe: add or remove MAC address

["Sun, GuinanX" <guinanx.sun@intel.com>]

Hi Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Monday, December 23, 2019 3:25 PM
> To: Sun, GuinanX <guinanx.sun@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: add or remove MAC address
> 
> Hi, guinan
> 
> For the title, better to use
> 
> Add support for vf MAC address add and remove
> 
> or something like so.

I agree with you and I will fix it in V2's patch

> 
> On 12/03, Guinan Sun wrote:
> >Ixgbe PMD pf host code needs to support ixgbevf mac address add and
> >remove. For this purpose, a response was added between pf and vf to
> >update the mac address.
> 
> Does this mean each one vf can have multiple MAC addresses after this patch, or
> this `add` actually means to replace the old mac address with the new one?

It means each one vf can have multiple MAC addresses after this patch

> 
> >
> >Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> >---
> > drivers/net/ixgbe/ixgbe_ethdev.h |  1 +
> > drivers/net/ixgbe/ixgbe_pf.c     | 35 ++++++++++++++++++++++++++++++++
> > 2 files changed, 36 insertions(+)
> >
> >diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> >b/drivers/net/ixgbe/ixgbe_ethdev.h
> >index 76a1b9d18..e1cd8fd16 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.h
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> >@@ -270,6 +270,7 @@ struct ixgbe_vf_info {
> > 	uint8_t api_version;
> > 	uint16_t switch_domain_id;
> > 	uint16_t xcast_mode;
> >+	uint16_t mac_count;
> 
> How is this mac_count initialized?

This variable is initialized to 0 when the ixgbe_adapter structure is initialized, and the method is similar to vlan_count.

> 
> > };
> >
> > /*
> >diff --git a/drivers/net/ixgbe/ixgbe_pf.c
> >b/drivers/net/ixgbe/ixgbe_pf.c index d0d85e138..76dbed2ab 100644
> >--- a/drivers/net/ixgbe/ixgbe_pf.c
> >+++ b/drivers/net/ixgbe/ixgbe_pf.c
> >@@ -748,6 +748,37 @@ ixgbe_set_vf_mc_promisc(struct rte_eth_dev *dev,
> uint32_t vf, uint32_t *msgbuf)
> > 	return 0;
> > }
> >
> >+static int
> >+ixgbe_set_vf_macvlan_msg(struct rte_eth_dev *dev, uint32_t vf,
> >+uint32_t *msgbuf) {
> >+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >+	struct ixgbe_vf_info *vf_info =
> >+		*(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data-
> >dev_private));
> >+	uint8_t *new_mac = (uint8_t *)(&msgbuf[1]);
> >+	int index = (msgbuf[0] & IXGBE_VT_MSGINFO_MASK) >>
> >+		    IXGBE_VT_MSGINFO_SHIFT;
> >+
> >+	if (index) {
> >+		if (!rte_is_valid_assigned_ether_addr(
> >+			(struct rte_ether_addr *)new_mac)) {
> >+			RTE_LOG(ERR, PMD, "set invalid mac vf:%d\n", vf);
> >+			return -1;
> >+		}
> >+
> >+		if (new_mac == NULL)
> >+			return -1;
> 
> I feel the null check should be in front of valid ether addr check, otherwise there
> might be null pointer reference issue.

Ok,I will fix it in V2's patch.

> 
> Thanks,
> Xiaolong
> 
> >+
> >+		vf_info[vf].mac_count++;
> >+
> >+		hw->mac.ops.set_rar(hw, vf_info[vf].mac_count,
> >+				new_mac, vf, IXGBE_RAH_AV);
> >+	} else {
> >+		hw->mac.ops.clear_rar(hw, vf_info[vf].mac_count);
> >+		vf_info[vf].mac_count = 0;
> >+	}
> >+	return 0;
> >+}
> >+
> > static int
> > ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev, uint16_t vf)  { @@
> >-835,6 +866,10 @@ ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev, uint16_t
> vf)
> > 		if (retval == RTE_PMD_IXGBE_MB_EVENT_PROCEED)
> > 			retval = ixgbe_set_vf_mc_promisc(dev, vf, msgbuf);
> > 		break;
> >+	case IXGBE_VF_SET_MACVLAN:
> >+		if (retval == RTE_PMD_IXGBE_MB_EVENT_PROCEED)
> >+			retval = ixgbe_set_vf_macvlan_msg(dev, vf, msgbuf);
> >+		break;
> > 	default:
> > 		PMD_DRV_LOG(DEBUG, "Unhandled Msg %8.8x",
> (unsigned)msgbuf[0]);
> > 		retval = IXGBE_ERR_MBX;
> >--
> >2.17.1
> >