Re:[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

Re:[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Vijay Khemka]

Lgtm except one small comment below.

On 8/6/19, 5:22 PM, "openbmc on behalf of Tao Ren" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of taoren@fb.com> wrote:

    Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
    MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
    doesn't work for platforms with different BMC MAC offset: for example,
    Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
    MAC address ("BaseMAC + 1" is reserved for Host use).
    
    This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
    between NIC's Base MAC address and BMC's MAC address. Its default value is
    set to 1 to avoid breaking existing users.
    
    Signed-off-by: Tao Ren <taoren@fb.com>
    ---
     net/ncsi/Kconfig    |  8 ++++++++
     net/ncsi/ncsi-rsp.c | 15 +++++++++++++--
     2 files changed, 21 insertions(+), 2 deletions(-)
    
    diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
    index 2f1e5756c03a..be8efe1ed99e 100644
    --- a/net/ncsi/Kconfig
    +++ b/net/ncsi/Kconfig
    @@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
     	---help---
     	  This allows to get MAC address from NCSI firmware and set them back to
     		controller.
    +config NET_NCSI_MC_MAC_OFFSET
    +	int
    +	prompt "Offset of Management Controller's MAC Address"
    +	depends on NCSI_OEM_CMD_GET_MAC
    +	default 1
    +	help
    +	  This defines the offset between Network Controller's (base) MAC
    +	  address and Management Controller's MAC address.
    diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
    index 7581bf919885..24a791f9ebf5 100644
    --- a/net/ncsi/ncsi-rsp.c
    +++ b/net/ncsi/ncsi-rsp.c
    @@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
     	struct ncsi_rsp_oem_pkt *rsp;
     	struct sockaddr saddr;
     	int ret = 0;
    +#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
    +	int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
    +#else
    +	int mac_offset = 1;
    +#endif
     
     	/* Get the response header */
     	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
    @@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
     	saddr.sa_family = ndev->type;
     	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
     	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
    -	/* Increase mac address by 1 for BMC's address */
    -	eth_addr_inc((u8 *)saddr.sa_data);
    +
    +	/* Management Controller's MAC address is calculated by adding
    +	 * the offset to Network Controller's (base) MAC address.
    +	 * Note: negative offset is "ignored", and BMC will use the Base
Just mention negative and zero offset is ignored. As you are ignoring 0 as well.

    +	 * MAC address in this case.
    +	 */
    +	while (mac_offset-- > 0)
    +		eth_addr_inc((u8 *)saddr.sa_data);
     	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
     		return -ENXIO;
     
    -- 
    2.17.1
    
    

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/7/19 10:36 AM, Vijay Khemka wrote:
> Lgtm except one small comment below.
> 
> On 8/6/19, 5:22 PM, "openbmc on behalf of Tao Ren" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of taoren@fb.com> wrote:
> 
>     Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
>     MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
>     doesn't work for platforms with different BMC MAC offset: for example,
>     Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
>     MAC address ("BaseMAC + 1" is reserved for Host use).
>     
>     This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
>     between NIC's Base MAC address and BMC's MAC address. Its default value is
>     set to 1 to avoid breaking existing users.
>     
>     Signed-off-by: Tao Ren <taoren@fb.com>
>     ---
>      net/ncsi/Kconfig    |  8 ++++++++
>      net/ncsi/ncsi-rsp.c | 15 +++++++++++++--
>      2 files changed, 21 insertions(+), 2 deletions(-)
>     
>     diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
>     index 2f1e5756c03a..be8efe1ed99e 100644
>     --- a/net/ncsi/Kconfig
>     +++ b/net/ncsi/Kconfig
>     @@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
>      	---help---
>      	  This allows to get MAC address from NCSI firmware and set them back to
>      		controller.
>     +config NET_NCSI_MC_MAC_OFFSET
>     +	int
>     +	prompt "Offset of Management Controller's MAC Address"
>     +	depends on NCSI_OEM_CMD_GET_MAC
>     +	default 1
>     +	help
>     +	  This defines the offset between Network Controller's (base) MAC
>     +	  address and Management Controller's MAC address.
>     diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
>     index 7581bf919885..24a791f9ebf5 100644
>     --- a/net/ncsi/ncsi-rsp.c
>     +++ b/net/ncsi/ncsi-rsp.c
>     @@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>      	struct ncsi_rsp_oem_pkt *rsp;
>      	struct sockaddr saddr;
>      	int ret = 0;
>     +#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
>     +	int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
>     +#else
>     +	int mac_offset = 1;
>     +#endif
>      
>      	/* Get the response header */
>      	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
>     @@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>      	saddr.sa_family = ndev->type;
>      	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>      	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
>     -	/* Increase mac address by 1 for BMC's address */
>     -	eth_addr_inc((u8 *)saddr.sa_data);
>     +
>     +	/* Management Controller's MAC address is calculated by adding
>     +	 * the offset to Network Controller's (base) MAC address.
>     +	 * Note: negative offset is "ignored", and BMC will use the Base
> Just mention negative and zero offset is ignored. As you are ignoring 0 as well.

Thank you Vijay for the review.

Zero offset is not ignored: users get what they want when setting offset to 0 (BMC-MAC = Base-MAC).


Thanks,

Tao

[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
doesn't work for platforms with different BMC MAC offset: for example,
Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
MAC address ("BaseMAC + 1" is reserved for Host use).

This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
between NIC's Base MAC address and BMC's MAC address. Its default value is
set to 1 to avoid breaking existing users.

Signed-off-by: Tao Ren <taoren@fb.com>
---
 net/ncsi/Kconfig    |  8 ++++++++
 net/ncsi/ncsi-rsp.c | 15 +++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 2f1e5756c03a..be8efe1ed99e 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
 	---help---
 	  This allows to get MAC address from NCSI firmware and set them back to
 		controller.
+config NET_NCSI_MC_MAC_OFFSET
+	int
+	prompt "Offset of Management Controller's MAC Address"
+	depends on NCSI_OEM_CMD_GET_MAC
+	default 1
+	help
+	  This defines the offset between Network Controller's (base) MAC
+	  address and Management Controller's MAC address.
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 7581bf919885..24a791f9ebf5 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	int ret = 0;
+#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
+	int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
+#else
+	int mac_offset = 1;
+#endif
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
@@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
 	saddr.sa_family = ndev->type;
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
-	/* Increase mac address by 1 for BMC's address */
-	eth_addr_inc((u8 *)saddr.sa_data);
+
+	/* Management Controller's MAC address is calculated by adding
+	 * the offset to Network Controller's (base) MAC address.
+	 * Note: negative offset is "ignored", and BMC will use the Base
+	 * MAC address in this case.
+	 */
+	while (mac_offset-- > 0)
+		eth_addr_inc((u8 *)saddr.sa_data);
 	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
 		return -ENXIO;
 
-- 
2.17.1

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Jakub Kicinski]

On Tue, 6 Aug 2019 17:21:18 -0700, Tao Ren wrote:
> Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
> MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
> doesn't work for platforms with different BMC MAC offset: for example,
> Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
> MAC address ("BaseMAC + 1" is reserved for Host use).
> 
> This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
> between NIC's Base MAC address and BMC's MAC address. Its default value is
> set to 1 to avoid breaking existing users.
> 
> Signed-off-by: Tao Ren <taoren@fb.com>

Maybe someone more knowledgeable like Andrew has an opinion here, 
but to me it seems a bit strange to encode what seems to be platfrom
information in the kernel config :(

> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 2f1e5756c03a..be8efe1ed99e 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
>  	---help---
>  	  This allows to get MAC address from NCSI firmware and set them back to
>  		controller.
> +config NET_NCSI_MC_MAC_OFFSET
> +	int
> +	prompt "Offset of Management Controller's MAC Address"
> +	depends on NCSI_OEM_CMD_GET_MAC
> +	default 1
> +	help
> +	  This defines the offset between Network Controller's (base) MAC
> +	  address and Management Controller's MAC address.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 7581bf919885..24a791f9ebf5 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>  	struct ncsi_rsp_oem_pkt *rsp;
>  	struct sockaddr saddr;
>  	int ret = 0;
> +#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
> +	int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
> +#else
> +	int mac_offset = 1;
> +#endif
>  
>  	/* Get the response header */
>  	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> @@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>  	saddr.sa_family = ndev->type;
>  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>  	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
> -	/* Increase mac address by 1 for BMC's address */
> -	eth_addr_inc((u8 *)saddr.sa_data);
> +
> +	/* Management Controller's MAC address is calculated by adding
> +	 * the offset to Network Controller's (base) MAC address.
> +	 * Note: negative offset is "ignored", and BMC will use the Base
> +	 * MAC address in this case.
> +	 */
> +	while (mac_offset-- > 0)
> +		eth_addr_inc((u8 *)saddr.sa_data);
>  	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
>  		return -ENXIO;
>  

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Andrew Lunn]

On Wed, Aug 07, 2019 at 11:25:18AM -0700, Jakub Kicinski wrote:
> On Tue, 6 Aug 2019 17:21:18 -0700, Tao Ren wrote:
> > Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
> > MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
> > doesn't work for platforms with different BMC MAC offset: for example,
> > Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
> > MAC address ("BaseMAC + 1" is reserved for Host use).
> > 
> > This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
> > between NIC's Base MAC address and BMC's MAC address. Its default value is
> > set to 1 to avoid breaking existing users.
> > 
> > Signed-off-by: Tao Ren <taoren@fb.com>
> 
> Maybe someone more knowledgeable like Andrew has an opinion here, 
> but to me it seems a bit strange to encode what seems to be platfrom
> information in the kernel config :(

Yes, this is not a good idea. It makes it impossible to have a 'BMC
distro' kernel which you install on a number of different BMCs.

A device tree property would be better. Ideally it would be the
standard local-mac-address, or mac-address.

  Andrew

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/7/19 11:41 AM, Andrew Lunn wrote:
> On Wed, Aug 07, 2019 at 11:25:18AM -0700, Jakub Kicinski wrote:
>> On Tue, 6 Aug 2019 17:21:18 -0700, Tao Ren wrote:
>>> Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
>>> MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
>>> doesn't work for platforms with different BMC MAC offset: for example,
>>> Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
>>> MAC address ("BaseMAC + 1" is reserved for Host use).
>>>
>>> This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
>>> between NIC's Base MAC address and BMC's MAC address. Its default value is
>>> set to 1 to avoid breaking existing users.
>>>
>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>
>> Maybe someone more knowledgeable like Andrew has an opinion here, 
>> but to me it seems a bit strange to encode what seems to be platfrom
>> information in the kernel config :(
> 
> Yes, this is not a good idea. It makes it impossible to have a 'BMC
> distro' kernel which you install on a number of different BMCs.
> 
> A device tree property would be better. Ideally it would be the
> standard local-mac-address, or mac-address.

Thank you Andrew and Jakub for the feedback. I picked up kconfig approach mainly because it's an OEM-only extention which is used only when NCSI_OEM_CMD_GET_MAC is enabled.

Let me prepare patch v2 using device tree. I'm not sure if standard "mac-address" fits this situation because all we need is an offset (integer) and BMC MAC is calculated by adding the offset to NIC's MAC address. Anyways, let me work out v2 patch we can discuss more then.


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Andrew Lunn]

> Let me prepare patch v2 using device tree. I'm not sure if standard
> "mac-address" fits this situation because all we need is an offset
> (integer) and BMC MAC is calculated by adding the offset to NIC's
> MAC address. Anyways, let me work out v2 patch we can discuss more
> then.

Hi Tao

I don't know BMC terminology. By NICs MAC address, you are referring
to the hosts MAC address? The MAC address the big CPU is using for its
interface?  Where does this NIC get its MAC address from? If the BMCs
bootloader has access to it, it can set the mac-address property in
the device tree.

    Andrew

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

Hi Andrew,

On 8/8/19 6:32 AM, Andrew Lunn wrote:
>> Let me prepare patch v2 using device tree. I'm not sure if standard
>> "mac-address" fits this situation because all we need is an offset
>> (integer) and BMC MAC is calculated by adding the offset to NIC's
>> MAC address. Anyways, let me work out v2 patch we can discuss more
>> then.
> 
> Hi Tao
> 
> I don't know BMC terminology. By NICs MAC address, you are referring
> to the hosts MAC address? The MAC address the big CPU is using for its
> interface?  Where does this NIC get its MAC address from? If the BMCs
> bootloader has access to it, it can set the mac-address property in
> the device tree.

Sorry for the confusion and let me clarify more:

The NIC here refers to the Network controller which provide network connectivity for both BMC (via NC-SI) and Host (for example, via PCIe).

On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an ethernet packet) to the Network Controller while bringing up eth0, and the (Broadcom) Network Controller replies with the Base MAC Address reserved for the platform. As for Yamp, Base-MAC and Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 are assigned to BMC. In my opinion, Base MAC and MAC address assignments are controlled by Network Controller, which is transparent to both BMC and Host.

I'm not sure if I understand your suggestion correctly: do you mean we should move the logic (GET_MAC from Network Controller, adding offset and configuring BMC MAC) from kernel to boot loader?

Sam posted several ncsi patches for u-boot recently. Sam, do we support the work (implemented in this patch) in uboot? Or are we planning to do so?


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Andrew Lunn]

On Thu, Aug 08, 2019 at 07:02:54PM +0000, Tao Ren wrote:
> Hi Andrew,
> 
> On 8/8/19 6:32 AM, Andrew Lunn wrote:
> >> Let me prepare patch v2 using device tree. I'm not sure if standard
> >> "mac-address" fits this situation because all we need is an offset
> >> (integer) and BMC MAC is calculated by adding the offset to NIC's
> >> MAC address. Anyways, let me work out v2 patch we can discuss more
> >> then.
> > 
> > Hi Tao
> > 
> > I don't know BMC terminology. By NICs MAC address, you are referring
> > to the hosts MAC address? The MAC address the big CPU is using for its
> > interface?  Where does this NIC get its MAC address from? If the BMCs
> > bootloader has access to it, it can set the mac-address property in
> > the device tree.
> 
> Sorry for the confusion and let me clarify more:
> 

> The NIC here refers to the Network controller which provide network
> connectivity for both BMC (via NC-SI) and Host (for example, via
> PCIe).
> 

> On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an
> ethernet packet) to the Network Controller while bringing up eth0,
> and the (Broadcom) Network Controller replies with the Base MAC
> Address reserved for the platform. As for Yamp, Base-MAC and
> Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 are assigned to
> BMC. In my opinion, Base MAC and MAC address assignments are
> controlled by Network Controller, which is transparent to both BMC
> and Host.

Hi Tao

I've not done any work in the BMC field, so thanks for explaining
this.

In a typical embedded system, each network interface is assigned a MAC
address by the vendor. But here, things are different. The BMC SoC
network interface has not been assigned a MAC address, it needs to ask
the network controller for its MAC address, and then do some magical
transformation on the answer to derive a MAC address for
itself. Correct?

It seems like a better design would of been, the BMC sends a
NCSI_OEM_GET_BMC_MAC and the answer it gets back is the MAC address
the BMC should use. No magic involved. But i guess it is too late to
do that now.

> I'm not sure if I understand your suggestion correctly: do you mean
> we should move the logic (GET_MAC from Network Controller, adding
> offset and configuring BMC MAC) from kernel to boot loader?

In general, the kernel is generic. It probably boots on any ARM system
which is has the needed modules for. The bootloader is often much more
specific. It might not be fully platform specific, but it will be at
least specific to the general family of BMC SoCs. If you consider the
combination of the BMC bootloader and the device tree blob, you have
something specific to the platform. This magical transformation of
adding 2 seems to be very platform specific. So having this magic in
the bootloader+DT seems like the best place to put it.

However, how you pass the resulting MAC address to the kernel should
be as generic as possible. The DT "mac-address" property is very
generic, many MAC drivers understand it. Using it also allows for
vendors which actually assign a MAC address to the BMC to pass it to
the BMC, avoiding all this NCSI_OEM_GET_MAC handshake. Having an API
which just passing '2' is not generic at all.

    Andrew

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/8/19 2:16 PM, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 07:02:54PM +0000, Tao Ren wrote:
>> Hi Andrew,
>>
>> On 8/8/19 6:32 AM, Andrew Lunn wrote:
>>>> Let me prepare patch v2 using device tree. I'm not sure if standard
>>>> "mac-address" fits this situation because all we need is an offset
>>>> (integer) and BMC MAC is calculated by adding the offset to NIC's
>>>> MAC address. Anyways, let me work out v2 patch we can discuss more
>>>> then.
>>>
>>> Hi Tao
>>>
>>> I don't know BMC terminology. By NICs MAC address, you are referring
>>> to the hosts MAC address? The MAC address the big CPU is using for its
>>> interface?  Where does this NIC get its MAC address from? If the BMCs
>>> bootloader has access to it, it can set the mac-address property in
>>> the device tree.
>>
>> Sorry for the confusion and let me clarify more:
>>
> 
>> The NIC here refers to the Network controller which provide network
>> connectivity for both BMC (via NC-SI) and Host (for example, via
>> PCIe).
>>
> 
>> On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an
>> ethernet packet) to the Network Controller while bringing up eth0,
>> and the (Broadcom) Network Controller replies with the Base MAC
>> Address reserved for the platform. As for Yamp, Base-MAC and
>> Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 are assigned to
>> BMC. In my opinion, Base MAC and MAC address assignments are
>> controlled by Network Controller, which is transparent to both BMC
>> and Host.
> 
> Hi Tao
> 
> I've not done any work in the BMC field, so thanks for explaining
> this.
> 
> In a typical embedded system, each network interface is assigned a MAC
> address by the vendor. But here, things are different. The BMC SoC
> network interface has not been assigned a MAC address, it needs to ask
> the network controller for its MAC address, and then do some magical
> transformation on the answer to derive a MAC address for
> itself. Correct?

Yes. It's correct.

> It seems like a better design would of been, the BMC sends a
> NCSI_OEM_GET_BMC_MAC and the answer it gets back is the MAC address
> the BMC should use. No magic involved. But i guess it is too late to
> do that now.

Some NCSI Network Controllers support such OEM command (Get Provisioned BMC MAC Address), but unfortunately it's not supported on Yamp.

>> I'm not sure if I understand your suggestion correctly: do you mean
>> we should move the logic (GET_MAC from Network Controller, adding
>> offset and configuring BMC MAC) from kernel to boot loader?
> 
> In general, the kernel is generic. It probably boots on any ARM system
> which is has the needed modules for. The bootloader is often much more
> specific. It might not be fully platform specific, but it will be at
> least specific to the general family of BMC SoCs. If you consider the
> combination of the BMC bootloader and the device tree blob, you have
> something specific to the platform. This magical transformation of
> adding 2 seems to be very platform specific. So having this magic in
> the bootloader+DT seems like the best place to put it.

I understand your concern now. Thank you for the explanation.

> However, how you pass the resulting MAC address to the kernel should
> be as generic as possible. The DT "mac-address" property is very
> generic, many MAC drivers understand it. Using it also allows for
> vendors which actually assign a MAC address to the BMC to pass it to
> the BMC, avoiding all this NCSI_OEM_GET_MAC handshake. Having an API
> which just passing '2' is not generic at all.

After giving it more thought, I'm thinking about adding ncsi dt node with following structure (mac/ncsi similar to mac/mdio/phy):

&mac0 {
    /* MAC properties... */

    use-ncsi;
    ncsi {
        /* ncsi level properties if any */

        package@0 {
            /* package level properties if any */

            channel@0 {
                /* channel level properties if any */

                bmc-mac-offset = <2>;
            };

            channel@1 {
                /* channel #1 properties */
            };
        };

        /* package #1 properties start here.. */
    };
};

The reasons behind this are:

1) mac driver doesn't need to parse "mac-offset" stuff: these ncsi-network-controller specific settings should be parsed in ncsi stack.

2) get_bmc_mac_address command is a channel specific command, and technically people can configure different offset/formula for different channels.

Any concerns or suggestions?


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Andrew Lunn]

> After giving it more thought, I'm thinking about adding ncsi dt node
> with following structure (mac/ncsi similar to mac/mdio/phy):
> 
> &mac0 {
>     /* MAC properties... */
> 
>     use-ncsi;

This property seems to be specific to Faraday FTGMAC100. Are you going
to make it more generic? 

>     ncsi {
>         /* ncsi level properties if any */
> 
>         package@0 {

You should get Rob Herring involved. This is not really describing
hardware, so it might get rejected by the device tree maintainer.

> 1) mac driver doesn't need to parse "mac-offset" stuff: these
> ncsi-network-controller specific settings should be parsed in ncsi
> stack.

> 2) get_bmc_mac_address command is a channel specific command, and
> technically people can configure different offset/formula for
> different channels.

Does that mean the NCSA code puts the interface into promiscuous mode?
Or at least adds these unicast MAC addresses to the MAC receive
filter? Humm, ftgmac100 only seems to support multicast address
filtering, not unicast filters, so it must be using promisc mode, if
you expect to receive frames using this MAC address.

	   Andrew

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/8/19 4:03 PM, Andrew Lunn wrote:
>> After giving it more thought, I'm thinking about adding ncsi dt node
>> with following structure (mac/ncsi similar to mac/mdio/phy):
>>
>> &mac0 {
>>     /* MAC properties... */
>>
>>     use-ncsi;
> 
> This property seems to be specific to Faraday FTGMAC100. Are you going
> to make it more generic? 

I'm also using ftgmac100 on my platform, and I don't have plan to change this property.

>>     ncsi {
>>         /* ncsi level properties if any */
>>
>>         package@0 {
> 
> You should get Rob Herring involved. This is not really describing
> hardware, so it might get rejected by the device tree maintainer.

Got it. Thank you for the sharing, and let me think it over :-)

>> 1) mac driver doesn't need to parse "mac-offset" stuff: these
>> ncsi-network-controller specific settings should be parsed in ncsi
>> stack.
> 
>> 2) get_bmc_mac_address command is a channel specific command, and
>> technically people can configure different offset/formula for
>> different channels.
> 
> Does that mean the NCSA code puts the interface into promiscuous mode?
> Or at least adds these unicast MAC addresses to the MAC receive
> filter? Humm, ftgmac100 only seems to support multicast address
> filtering, not unicast filters, so it must be using promisc mode, if
> you expect to receive frames using this MAC address.

Uhh, I actually didn't think too much about this: basically it's how to configure frame filtering when there are multiple packages/channels active: single BMC MAC or multiple BMC MAC is also allowed?
I don't have the answer yet, but will talk to NCSI expert and figure it out.


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Terry Duncan]

Tao, in your new patch will it be possible to disable the setting of the 
BMC MAC?  I would like to be able to send NCSI_OEM_GET_MAC perhaps with 
netlink (TBD) to get the system address without it affecting the BMC 
address.

I was about to send patches to add support for the Intel adapters when I 
saw this thread.

Thanks,

Terry

>>> 	After giving it more thought, I'm thinking about adding ncsi dt node
>>> with following structure (mac/ncsi similar to mac/mdio/phy):
>>>
>>> &mac0 {
>>>      /* MAC properties... */
>>>
>>>      use-ncsi;
>> This property seems to be specific to Faraday FTGMAC100. Are you going
>> to make it more generic?
> I'm also using ftgmac100 on my platform, and I don't have plan to change this property.
>
>>>      ncsi {
>>>          /* ncsi level properties if any */
>>>
>>>          package@0 {
>> You should get Rob Herring involved. This is not really describing
>> hardware, so it might get rejected by the device tree maintainer.
> Got it. Thank you for the sharing, and let me think it over :-)
>
>>> 1) mac driver doesn't need to parse "mac-offset" stuff: these
>>> ncsi-network-controller specific settings should be parsed in ncsi
>>> stack.
>>> 2) get_bmc_mac_address command is a channel specific command, and
>>> technically people can configure different offset/formula for
>>> different channels.
>> Does that mean the NCSA code puts the interface into promiscuous mode?
>> Or at least adds these unicast MAC addresses to the MAC receive
>> filter? Humm, ftgmac100 only seems to support multicast address
>> filtering, not unicast filters, so it must be using promisc mode, if
>> you expect to receive frames using this MAC address.
> Uhh, I actually didn't think too much about this: basically it's how to configure frame filtering when there are multiple packages/channels active: single BMC MAC or multiple BMC MAC is also allowed?
> I don't have the answer yet, but will talk to NCSI expert and figure it out.
>
>
> Thanks,
>
> Tao
>
>

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Terry Duncan]

On 8/13/19 11:28 AM, Tao Ren wrote:
> On 8/13/19 9:31 AM, Terry Duncan wrote:
>> Tao, in your new patch will it be possible to disable the setting of the BMC MAC?  I would like to be able to send NCSI_OEM_GET_MAC perhaps with netlink (TBD) to get the system address without it affecting the BMC address.
>>
>> I was about to send patches to add support for the Intel adapters when I saw this thread.
>>
>> Thanks,
>>
>> Terry
> 
> Hi Terry,
> 
> Sounds like you are planning to configure BMC MAC address from user space via netlink? Ben Wei <benwei@fb.com> started a thread "Out-of-band NIC management" in openbmc community for NCSI management using netlink, and you may follow up with him for details.
> 
> I haven't decided what to do in my v2 patch: maybe using device tree, maybe moving the logic to uboot, and I'm also evaluating the netlink option. But it shouldn't impact your patch, because you can disable NCSI_OEM_GET_MAC option from your config file.

Thanks Tao. I see now that disabling the NCSI_OEM_GET_MAC option will do 
what I want.

Best,
Terry

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Terry Duncan]

On 8/13/19 1:54 PM, Terry Duncan wrote:
> 
> On 8/13/19 11:28 AM, Tao Ren wrote:
>> On 8/13/19 9:31 AM, Terry Duncan wrote:
>>> Tao, in your new patch will it be possible to disable the setting of 
>>> the BMC MAC?  I would like to be able to send NCSI_OEM_GET_MAC 
>>> perhaps with netlink (TBD) to get the system address without it 
>>> affecting the BMC address.
>>>
>>> I was about to send patches to add support for the Intel adapters 
>>> when I saw this thread.
>>>
>>> Thanks,
>>>
>>> Terry
>>
>> Hi Terry,
>>
>> Sounds like you are planning to configure BMC MAC address from user 
>> space via netlink? Ben Wei <benwei@fb.com> started a thread 
>> "Out-of-band NIC management" in openbmc community for NCSI management 
>> using netlink, and you may follow up with him for details.
>>
>> I haven't decided what to do in my v2 patch: maybe using device tree, 
>> maybe moving the logic to uboot, and I'm also evaluating the netlink 
>> option. But it shouldn't impact your patch, because you can disable 
>> NCSI_OEM_GET_MAC option from your config file.
> 
> Thanks Tao. I see now that disabling the NCSI_OEM_GET_MAC option will do 
> what I want.
> 
> Best,
> Terry
Hi Tao,

After a second look, it appears that the OEM handlers for Broadcom and 
Melanox in ncsi-rsp.c will set the MAC regardless of the origin of the 
request. Even with NCSI_OEM_GET_MAC disabled, sending an OEM command 
with netlink would result in setting the BMC MAC.

Thanks,
Terry

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Terry Duncan]

On 8/13/19 12:52 PM, Ben Wei wrote:
>> On 8/13/19 9:31 AM, Terry Duncan wrote:
>>> Tao, in your new patch will it be possible to disable the setting of the BMC MAC?  I would like to be able to send NCSI_OEM_GET_MAC perhaps with netlink (TBD) to get the system address without it affecting the BMC address.
>>>
>>> I was about to send patches to add support for the Intel adapters when I saw this thread.
>>> Thanks,
>>> Terry
>>
> Hi Terry,
> Do you plan to monitor and configure NIC through use space programs via netlink?  I'm curious if you have additional use cases.
> I had planned to add some daemon in user space to monitor NIC through NC-SI, primarily to log AENs, check link status and retrieve NIC counters.
> We can collaborate on these if they align with your needs as well.
> 
> Also about Intel NIC, do you not plan to derive system address from BMC MAC? Is the BMC MAC independent of system address?
> 
> Thanks,
> -Ben
> 
Hi Ben,
Intel has in the past programmed BMC MACs with an offset from the system 
MAC address of the first shared interface. We have found this approach 
causes problems and adds complexity when system interfaces / OCP cards 
are added or removed or disabled in BIOS. Our approach in OpenBMC is to 
keep this simple - provide means for the BMC MAC addresses to be set in 
manufacturing and persist them.

We do also have some of the same use cases you mention.
Thanks,
Terry