Hi Abner,
please see my comments below
On Wed, Dec 6, 2023 at 6:06 PM <abner.ch...@amd.com> wrote:
>
> From: Abner Chang <abner.ch...@amd.com>
>
> Remedy Redfish service discovery flow changes made
> in commit 8736b8fd.
>
> The above fix creates the dependency with SMBIOS 42h record,
> which has a problem as SMBIOS 42h may not be created when
> RedfishDiscovery.Supported() is invoked even all of the
> required protocols are ready on the ControllerHandle. We can’t
> guarantee SMBIOS 42 structure will be always created before
> ConnectController(). USB NIC maybe detected late and it means
> PlatformHostInterfaceBmcUsbNicLib can populate SMBIOS 42h
> information late as well. Calling to
> RedfishServiceGetNetworkInterface with the previous fix may
> result in no network interface for BMC-exposed NIC as SMBIOS
> 42h is not ready yet.This is the first issue.
>
> Second, to skip the network interface when
> NetworkInterfaceGetSubnetInfo() returns a failure also has
> problem, as the NIC may be configured via RestEx->Configure().
> This happens after the Host interface is discovered, as at this
> moment we have the sufficient network information to configure
> BMC-exposed NIC.
>
> Base on Redfish spec in 31.1.5.2, “EFI Redfish Client may provide
> selection UI of network interfaces for Redfish service discovery.",
> This means edk2 Redfish client gets all network interfaces
> through RedfishServiceGetNetworkInterface and choose the desired
> network interface at its discretion for Redfish service.
>
> So the fix here is:
> 1. In BuildNetworkInterface(), we don’t skip any network
> interface. In RedfishServiceGetNetworkInterface, we don’t
> skip any network interface even the subnet information is not
> retrieved. We will still return all of network interfaces to
> client.
> 2. In RedfishServiceAcquireService for
> EFI_REDFISH_RISCOVER_HOST_INTERFACE case, we don’t skip any
> network interface even the subnet information is not
> retrieved.
>
> 3. Added some more debug information.
>
> Note: The subnet information is used for the scenario the system
> is managed by a centralized Redfish service (not on BMC), says
> the multiple Redfish computer system instances. As it mentions
> in 31.1.5.2, Redfish client they may have to know the subnet
> information so they can know the network domain the NIC is
> connected. There may have multiple subnets in the corporation
> network environment. So the subnet information provides client
> an idea when they choose the network interface, so does VLAN ID.
>
> Signed-off-by: Abner Chang <abner.ch...@amd.com>
> Cc: Nickle Wang <nick...@nvidia.com>
> Cc: Igor Kulchytskyy <ig...@ami.com>
> Cc: Mike Maslenkin <mike.maslen...@gmail.com>
> ---
> .../RedfishDiscoverDxe/RedfishDiscoverDxe.c | 105 ++++++------------
> 1 file changed, 37 insertions(+), 68 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 833ae2b969f..1cfcbcdc794 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -487,43 +487,6 @@ CheckIsIpVersion6 (
> return FALSE;
> }
>
> -/**
> - This function returns the IP type supported by the Host Interface.
> -
> - @retval 00h is Unknown
> - 01h is Ipv4
> - 02h is Ipv6
> -
> -**/
> -STATIC
> -UINT8
> -GetHiIpProtocolType (
> - VOID
> - )
> -{
> - EFI_STATUS Status;
> - REDFISH_OVER_IP_PROTOCOL_DATA *Data;
> - REDFISH_INTERFACE_DATA *DeviceDescriptor;
> -
> - Data = NULL;
> - DeviceDescriptor = NULL;
> - if (mSmbios == NULL) {
> - Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID
> **)&mSmbios);
> - if (EFI_ERROR (Status)) {
> - return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> - }
> - }
> -
> - Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor,
> &Data); // Search for SMBIOS type 42h
> - if (!EFI_ERROR (Status) && (Data != NULL) &&
> - (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic))
> - {
> - return Data->HostIpAddressFormat;
> - }
> -
> - return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> -}
> -
> /**
> Check if Network Protocol Type matches with SMBIOS Type 42 IP Address Type.
>
> @@ -583,7 +546,10 @@ DiscoverRedfishHostInterface (
> }
>
> Status = RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescriptor,
> &Data); // Search for SMBIOS type 42h
> - if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) {
> + if (EFI_ERROR (Status) || (Data == NULL) || (DeviceDescriptor == NULL)) {
> + DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is
> failed.\n", __func__));
> + return Status;
> + } else {
> // Check IP Type and skip an unnecessary network protocol if does not
> match
> if (FilterProtocol (Instance->NetworkInterface->NetworkProtocolType,
> Data->HostIpAddressFormat)) {
> return EFI_UNSUPPORTED;
> @@ -737,10 +703,7 @@ DiscoverRedfishHostInterface (
> IsHttps
> );
> }
> - } else {
> - DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is
> failed.\n", __func__));
> }
> -
> return Status;
> }
>
> @@ -891,6 +854,12 @@ AddAndSignalNewRedfishService (
> DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n",
> DiscoveredInstance->Information.Location));
> }
>
> + if (UseHttps) {
> + DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 443\n"));
> + } else {
> + DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 80\n"));
> + }
> +
Do we really need such trace?
It could be a bit misleading, because PcdRedfishServicePort was
introduced previously.
May be we could move this to the caller function
DiscoverRedfishHostInterface() where IsHttps was initialized?
> if (Uuid != NULL) {
> DiscoveredInstance->Information.Uuid = (CHAR16 *)AllocatePool
> (AsciiStrSize ((const CHAR8 *)Uuid) * sizeof (CHAR16));
> AsciiStrToUnicodeStrS ((const CHAR8 *)Uuid,
> DiscoveredInstance->Information.Uuid, AsciiStrSize ((const CHAR8 *)Uuid) *
> sizeof (CHAR16));
> @@ -1015,9 +984,11 @@ AddAndSignalNewRedfishService (
> (EFI_REST_EX_CONFIG_DATA)(UINT8
> *)RestExHttpConfigData
> );
> if (EFI_ERROR (Status)) {
> - DEBUG ((DEBUG_ERROR, "%a:REST EX configured..\n", __func__));
> + DEBUG ((DEBUG_ERROR, "%a: REST EX is not configured..\n", __func__));
> DeleteRestEx = TRUE;
> goto EXIT_FREE_ALL;
> + } else {
> + DEBUG ((DEBUG_MANAGEABILITY, "%a: REST EX is configured..\n",
> __func__));
> }
>
> //
> @@ -1207,6 +1178,8 @@ RedfishServiceGetNetworkInterface (
> EFI_REDFISH_DISCOVER_NETWORK_INTERFACE *ThisNetworkInterface;
> EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance;
>
> + DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> +
> if ((This == NULL) || (NetworkIntfInstances == NULL) ||
> (NumberOfNetworkIntfs == NULL) ||
> (ImageHandle == NULL))
> {
> @@ -1244,29 +1217,23 @@ RedfishServiceGetNetworkInterface (
> while (TRUE) {
> // If Get Subnet Info failed then skip this interface
> Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn,
> ImageHandle); // Get subnet info
> - if (EFI_ERROR (Status)) {
> - if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface,
> &ThisNetworkInterfaceIntn->Entry)) {
> - break;
> + if (!EFI_ERROR (Status)) {
> + if (!ThisNetworkInterface->IsIpv6) {
> + IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4,
> &ThisNetworkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information.
> + } else {
> + IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6,
> &ThisNetworkInterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in IPv6
> address information.
> }
>
> - ThisNetworkInterfaceIntn =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode
> (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entry);
> - continue;
> + ThisNetworkInterface->SubnetPrefixLength =
> ThisNetworkInterfaceIntn->SubnetPrefixLength;
> }
>
> + CopyMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &ThisNetworkInterfaceIntn->MacAddress,
> ThisNetworkInterfaceIntn->HwAddressSize);
> ThisNetworkInterface->IsIpv6 = FALSE;
> if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) {
> ThisNetworkInterface->IsIpv6 = TRUE;
> }
>
> - CopyMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &ThisNetworkInterfaceIntn->MacAddress,
> ThisNetworkInterfaceIntn->HwAddressSize);
> - if (!ThisNetworkInterface->IsIpv6) {
> - IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4,
> &ThisNetworkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information.
> - } else {
> - IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6,
> &ThisNetworkInterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in IPv6
> address information.
> - }
> -
> - ThisNetworkInterface->SubnetPrefixLength =
> ThisNetworkInterfaceIntn->SubnetPrefixLength;
> - ThisNetworkInterface->VlanId =
> ThisNetworkInterfaceIntn->VlanId;
> + ThisNetworkInterface->VlanId = ThisNetworkInterfaceIntn->VlanId;
> RestExInstance->NumberOfNetworkInterfaces++;
> if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface,
> &ThisNetworkInterfaceIntn->Entry)) {
> break;
As can be observed from this hunk the check " if
(!ThisNetworkInterface->IsIpv6) " was moved before the
ThisNetworkInterface->IsIpv6 initialization. Also I would consider
removing IsIpv6 at all.
ThisNetworkInterface->IsIpv6 depends on CheckIsIpVersion6() that is
(ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp6),
so why do we need an additional variable with the same meaning?
ThisNetworkInterface->NetworkProtocolType is initialized early on
RedfishDiscoverDriverBindingStart()->BuildupNetworkInterface() path,
so it always has the correct value.
> @@ -1378,9 +1345,16 @@ RedfishServiceAcquireService (
> //
> Status1 = NetworkInterfaceGetSubnetInfo
> (TargetNetworkInterfaceInternal, ImageHandle);
> if (EFI_ERROR (Status1)) {
> - DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n",
> __func__));
> - FreePool (Instance);
> - continue;
> + //
> + // Get subnet information could be failed for
> EFI_REDFISH_DISCOVER_HOST_INTERFACE case.
> + // We will configure network in AddAndSignalNewRedfishService. So
> don't skip this
> + // target network interface.
> + //
> + if ((Flags & EFI_REDFISH_DISCOVER_HOST_INTERFACE) == 0) {
> + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n",
> __func__));
> + FreePool (Instance);
> + continue;
> + }
> }
>
> NewInstance = TRUE;
> @@ -1448,6 +1422,7 @@ RedfishServiceAbortAcquire (
> IN EFI_REDFISH_DISCOVER_NETWORK_INTERFACE *TargetNetworkInterface OPTIONAL
> )
> {
> + DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> // This function is used to abort Redfish service discovery through SSDP
> // on the network interface. SSDP is optionally suppoted by
> EFI_REDFISH_DISCOVER_PROTOCOL,
> // we dont have implementation for SSDP now.
> @@ -1477,6 +1452,8 @@ RedfishServiceReleaseService (
> EFI_REDFISH_DISCOVERED_INSTANCE *ThisRedfishInstance;
> EFI_REDFISH_DISCOVERED_INTERNAL_LIST *DiscoveredRedfishInstance;
>
> + DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> +
> if (IsListEmpty (&mRedfishInstanceList)) {
> DEBUG ((DEBUG_ERROR, "%a:No any discovered Redfish service.\n",
> __func__));
> return EFI_NOT_FOUND;
> @@ -1723,22 +1700,13 @@ BuildupNetworkInterface (
> EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance;
> EFI_TPL OldTpl;
> BOOLEAN
> NewNetworkInterfaceInstalled;
> - UINT8 IpType;
> UINTN ListCount;
>
> ListCount = (sizeof (mRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> NewNetworkInterfaceInstalled = FALSE;
> Index = 0;
>
> - // Get IP Type to filter out unnecessary network protocol if possible
> - IpType = GetHiIpProtocolType ();
> -
> for (Index = 0; Index < ListCount; Index++) {
> - // Check IP Type and skip an unnecessary network protocol if does not
> match
> - if (FilterProtocol (mRequiredProtocol[Index].ProtocolType, IpType)) {
> - continue;
> - }
> -
> Status = gBS->OpenProtocol (
> // Already in list?
> ControllerHandle,
> @@ -2164,6 +2132,7 @@ RedfishDiscoverDriverBindingStart (
> IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
> )
> {
> + DEBUG ((DEBUG_MANAGEABILITY, "%a:Entry.\n", __func__));
> return BuildupNetworkInterface (This, ControllerHandle);
> }
>
> --
> 2.37.1.windows.1
>
I've tested this patch and it works for me.
Regards,
Mike.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112191): https://edk2.groups.io/g/devel/message/112191
Mute This Topic: https://groups.io/mt/103014230/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
