Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
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 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
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
Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
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 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
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):
{
/* 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: [Potential Spoof] Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
On 8/8/19, 3:27 PM, "openbmc on behalf of Tao Ren"
wrote:
On 8/8/19 2:16 PM, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 07:02:54PM +, 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):
{
/* MAC properties... */
use-ncsi;
ncsi {
/* ncsi level properties if any */
Tao, I like this idea but keep this only hardware specific.
package@0 {
/* package level properties if any */
channel@0 {
/* channel level properties if any */
bmc-mac-offset = <2>;
Every NIC vendor doesn't need this offset so it is specific to BCM card only as
you see this
increment has been done only for BCM card so you may want to pass bcm specific
only.
};
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
Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
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):
>>
>> {
>> /* 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
> After giving it more thought, I'm thinking about adding ncsi dt node
> with following structure (mac/ncsi similar to mac/mdio/phy):
>
> {
> /* 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
On 8/8/19 2:16 PM, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 07:02:54PM +, 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):
{
/* 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
On Thu, Aug 08, 2019 at 07:02:54PM +, 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
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
> 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
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"
> tao...@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
> ---
> 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, >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
Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
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
>>
>> 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
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
>
> 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
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
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, >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
Lgtm except one small comment below.
On 8/6/19, 5:22 PM, "openbmc on behalf of 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
---
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, >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
[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
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
---
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, >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
