Re: [edk2-devel] [PATCH V2 1/8] RedfishPkg/BmcUsbNicLib: Update BMC USB NIC searching algorithm

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

Hi Abner,

please find my comments below.

On Thu, Nov 23, 2023 at 9:47 AM <abner.chang@amd.com> wrote:
>
> From: Abner Chang <abner.chang@amd.com>
>
> Update BMC USB NIC searching algorithm for IPv4 only.
>
> Signed-off-by: Abner Chang <abner.chang@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Mike Maslenkin <mike.maslenkin@gmail.com>
> ---
>  .../PlatformHostInterfaceBmcUsbNicLib.c       | 188 ++++++++++++------
>  1 file changed, 128 insertions(+), 60 deletions(-)
>
> diff --git a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
> index 95900579118..e5bf70cfd58 100644
> --- a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
> +++ b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
> @@ -368,7 +368,9 @@ RetrievedBmcUsbNicInfo (
>          ));
>        CopyMem ((VOID *)&ThisInstance->RedfishIpAddressIpv4, (VOID *)&DestIpAddress->IpAddress, sizeof (DestIpAddress->IpAddress));
>        //
> -      // According to UEFI spec, the IP address at BMC USB NIC host end is the IP address at BMC end minus 1.
> +      // According to the design spec:
> +      // https://github.com/tianocore/edk2/tree/master/RedfishPkg#platform-with-bmc-and-the-bmc-exposed-usb-network-device
> +      // The IP address at BMC USB NIC host end is the IP address at BMC end minus 1.
>        //
>        CopyMem ((VOID *)&ThisInstance->HostIpAddressIpv4, (VOID *)&DestIpAddress->IpAddress, sizeof (DestIpAddress->IpAddress));
>        ThisInstance->HostIpAddressIpv4[sizeof (ThisInstance->HostIpAddressIpv4) - 1] -= 1;
> @@ -729,8 +731,10 @@ HostInterfaceIpmiCheckMacAddress (
>
>        //
>        // According to design spec in Readme file under RedfishPkg.
> -      // Compare the first five MAC address and
> -      // the 6th MAC address.
> +      // https://github.com/tianocore/edk2/tree/master/RedfishPkg#platform-with-bmc-and-the-bmc-exposed-usb-network-device
> +      // Compare the first five elements of MAC address and the 6th element of MAC address.
> +      // The 6th element of MAC address must be the 6th element of
> +      // IPMI channel MAC address minus 1.
>        //
>        if ((IpmiLanMacAddressSize != UsbNicInfo->MacAddressSize) ||
>            (CompareMem (
> @@ -738,8 +742,8 @@ HostInterfaceIpmiCheckMacAddress (
>               (VOID *)&IpmiLanChannelMacAddress.Addr,
>               IpmiLanMacAddressSize - 1
>               ) != 0) ||
> -          (IpmiLanChannelMacAddress.Addr[IpmiLanMacAddressSize - 1] !=
> -           *(UsbNicInfo->MacAddress + IpmiLanMacAddressSize - 1) - 1)
> +          ((IpmiLanChannelMacAddress.Addr[IpmiLanMacAddressSize - 1] - 1) !=
> +           *(UsbNicInfo->MacAddress + IpmiLanMacAddressSize - 1))
>            )
>        {
>          DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "    MAC address is not matched.\n"));
> @@ -962,6 +966,49 @@ UsbNicSearchUsbIo (
>    return EFI_NOT_FOUND;
>  }
>
> +/**
> +  This function identifies if the USB NIC has MAC address and internet
> +  protocol device path installed. (Only support IPv4)
> +
> +  @param[in] UsbDevicePath     USB device path.
> +
> +  @retval EFI_SUCCESS          Yes, this is IPv4 SNP handle
> +  @retval EFI_NOT_FOUND        No, this is not IPv4 SNP handle
> +
> +**/
> +EFI_STATUS
> +IdentifyNetworkMessageDevicePath (
> +  IN EFI_DEVICE_PATH_PROTOCOL  *UsbDevicePath
> +  )
> +{
> +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> +
> +  DevicePath = UsbDevicePath;
> +  while (TRUE) {
> +    DevicePath = NextDevicePathNode (DevicePath);
> +    if (IsDevicePathEnd (DevicePath)) {
> +      DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "MAC address device path is not found on this handle.\n"));
> +      break;
> +    }
> +
> +    if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && (DevicePath->SubType == MSG_MAC_ADDR_DP)) {
> +      DevicePath = NextDevicePathNode (DevicePath); // Advance to next device path protocol.
> +      if (IsDevicePathEnd (DevicePath)) {
> +        DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "IPv4 device path is not found on this handle.\n"));
> +        break;
> +      }
> +
> +      if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && (DevicePath->SubType == MSG_IPv4_DP)) {
> +        return EFI_SUCCESS;
> +      }
> +
> +      break;
> +    }
> +  }
> +
> +  return EFI_NOT_FOUND;
> +}
> +
>  /**
>    This function identifies if the USB NIC is exposed by BMC as
>    the host-BMC channel.
> @@ -1025,7 +1072,7 @@ IdentifyUsbNicBmcChannel (
>      (VOID *)&Snp->Mode->CurrentAddress,
>      BmcUsbNic->MacAddressSize
>      );
> -  DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "    MAC address (in size %d) for this SNP instance:\n      ", BmcUsbNic->MacAddressSize));
> +  DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "    MAC address (in size %d) for this SNP instance:\n", BmcUsbNic->MacAddressSize));
>    for (Index = 0; Index < BmcUsbNic->MacAddressSize; Index++) {
>      DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "%02x ", *(BmcUsbNic->MacAddress + Index)));
>    }
> @@ -1068,7 +1115,8 @@ CheckBmcUsbNicOnHandles (
>    UINTN                     Index;
>    EFI_STATUS                Status;
>    EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> -  BOOLEAN                   GotOneUsbNIc;
> +  BOOLEAN                   GotBmcUsbNic;
> +  CHAR16                    *DevicePathStr;
>
>    if ((HandleNumer == 0) || (HandleBuffer == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -1076,26 +1124,37 @@ CheckBmcUsbNicOnHandles (
>
>    DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry, #%d SNP handle\n", __func__, HandleNumer));
>
> -  GotOneUsbNIc = FALSE;
> +  GotBmcUsbNic = FALSE;
>    for (Index = 0; Index < HandleNumer; Index++) {
> +    DEBUG ((DEBUG_MANAGEABILITY, "    Locate device path on handle 0x%08x\n", *(HandleBuffer + Index)));
>      Status = gBS->HandleProtocol (
>                      *(HandleBuffer + Index),
>                      &gEfiDevicePathProtocolGuid,
>                      (VOID **)&DevicePath
>                      );
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "    Failed to locate SNP on %d handle.\n", Index));
> +      DEBUG ((DEBUG_ERROR, "    Failed to locate device path on %d handle.\n", __func__, Index));

There is no format for __func__ argument.


>        continue;
>      }
>
> +    DevicePathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE);
> +    if (DevicePathStr != NULL) {
> +      DEBUG ((DEBUG_MANAGEABILITY, "    Device path: %s\n", DevicePathStr));
> +      FreePool (DevicePathStr);
> +    }
> +
>      // Check if this is an BMC exposed USB NIC device.
>      while (TRUE) {
>        if ((DevicePath->Type == MESSAGING_DEVICE_PATH) && (DevicePath->SubType == MSG_USB_DP)) {
> -        Status = IdentifyUsbNicBmcChannel (*(HandleBuffer + Index), DevicePath);
> +        Status = IdentifyNetworkMessageDevicePath (DevicePath);
>          if (!EFI_ERROR (Status)) {
> -          GotOneUsbNIc = TRUE;
> -          break;
> +          Status = IdentifyUsbNicBmcChannel (*(HandleBuffer + Index), DevicePath);

Not related to this patch, but while you are changing this could you
fix memory allocation issue in IdentifyUsbNicBmcChannel()?
The problem string is:
   BmcUsbNic->MacAddress     = AllocateZeroPool (sizeof
(BmcUsbNic->MacAddressSize));
Obviously sizeof() must be removed.
Also, AllocateZeroPool is not required because of the following
CopyMem (BmcUsbNic->MacAddress, &Snp->Mode->CurrentAddress,
BmcUsbNic->MacAddressSize);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111706): https://edk2.groups.io/g/devel/message/111706
Mute This Topic: https://groups.io/mt/102763117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-