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

[Vijay Khemka]

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

    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 */
Tao, I like this idea but keep this only hardware specific. 
    
            package at 0 {
                /* package level properties if any */
    
                channel at 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 at 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