From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id E881BD801B1 for ; Thu, 7 Dec 2023 13:37:55 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=O6qfiWLzvuw/ume4Pru4t4n9XKUYpRhiBNrCZkQPIYQ=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1701956274; v=1; b=TnnDBSPq7+GuiqGe2WGZBT+JbWcn5wRxTvndC06GITtzW8F8GTFtIS2ZO4A7ZI1kOzn5L3Ti 9b/tAAPrh+LG203aujD5CrlQ7NHPPqf0t7cNcn8vStct/fHi94m+/aFx5XqsR2acCeHI7jApF4h hoOQxifKopHzz3hbMIB/tSvw= X-Received: by 127.0.0.2 with SMTP id 0OF8YY7687511xgaqCtRj2RT; Thu, 07 Dec 2023 05:37:54 -0800 X-Received: from mail-yb1-f172.google.com (mail-yb1-f172.google.com [209.85.219.172]) by mx.groups.io with SMTP id smtpd.web11.83737.1701956273603827027 for ; Thu, 07 Dec 2023 05:37:53 -0800 X-Received: by mail-yb1-f172.google.com with SMTP id 3f1490d57ef6-db548da6e3bso1806603276.0 for ; Thu, 07 Dec 2023 05:37:53 -0800 (PST) X-Gm-Message-State: lZbXoswnSG1rqJLIXyuAkUaJx7686176AA= X-Google-Smtp-Source: AGHT+IG0K2Cp7rzWC7ZF/bn22d3B2y06YwmXkQyDc7txG6JwcLLfx/LZvEnomLEIrD3AHUn1iPrKZs2dAy/Nb2sEgJA= X-Received: by 2002:a25:216:0:b0:db9:8825:c7f9 with SMTP id 22-20020a250216000000b00db98825c7f9mr3220717ybc.46.1701956272747; Thu, 07 Dec 2023 05:37:52 -0800 (PST) MIME-Version: 1.0 References: <20231206150513.1429-1-abner.chang@amd.com> In-Reply-To: <20231206150513.1429-1-abner.chang@amd.com> From: "Mike Maslenkin" Date: Thu, 7 Dec 2023 16:37:16 +0300 Message-ID: Subject: Re: [edk2-devel] [PATCH] RedfishPkg/RedfishDicovery: Remedy Redfish service discovery flow To: abner.chang@amd.com Cc: devel@edk2.groups.io, Nickle Wang , Igor Kulchytskyy Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,mike.maslenkin@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=TnnDBSPq; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none) Hi Abner, please see my comments below On Wed, Dec 6, 2023 at 6:06=E2=80=AFPM wrote: > > From: Abner Chang > > 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=E2=80=99t > 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, =E2=80=9CEFI 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=E2=80=99t skip any network > interface. In RedfishServiceGetNetworkInterface, we don=E2=80=99t > 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=E2=80=99t 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 > Cc: Nickle Wang > Cc: Igor Kulchytskyy > Cc: Mike Maslenkin > --- > .../RedfishDiscoverDxe/RedfishDiscoverDxe.c | 105 ++++++------------ > 1 file changed, 37 insertions(+), 68 deletions(-) > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/Redfish= Pkg/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 =3D NULL; > - DeviceDescriptor =3D NULL; > - if (mSmbios =3D=3D NULL) { > - Status =3D gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID= **)&mSmbios); > - if (EFI_ERROR (Status)) { > - return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN; > - } > - } > - > - Status =3D RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescri= ptor, &Data); // Search for SMBIOS type 42h > - if (!EFI_ERROR (Status) && (Data !=3D NULL) && > - (Data->HostIpAssignmentType =3D=3D 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 =3D RedfishGetHostInterfaceProtocolData (mSmbios, &DeviceDescri= ptor, &Data); // Search for SMBIOS type 42h > - if (!EFI_ERROR (Status) && (Data !=3D NULL) && (DeviceDescriptor !=3D = NULL)) { > + if (EFI_ERROR (Status) || (Data =3D=3D NULL) || (DeviceDescriptor =3D= =3D NULL)) { > + DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is fai= led.\n", __func__)); > + return Status; > + } else { > // Check IP Type and skip an unnecessary network protocol if does no= t match > if (FilterProtocol (Instance->NetworkInterface->NetworkProtocolType,= Data->HostIpAddressFormat)) { > return EFI_UNSUPPORTED; > @@ -737,10 +703,7 @@ DiscoverRedfishHostInterface ( > IsHttps > ); > } > - } else { > - DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is fai= led.\n", __func__)); > } > - > return Status; > } > > @@ -891,6 +854,12 @@ AddAndSignalNewRedfishService ( > DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n", Di= scoveredInstance->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 !=3D NULL) { > DiscoveredInstance->Information.Uuid =3D (CHAR16 *)AllocatePool (A= sciiStrSize ((const CHAR8 *)Uuid) * sizeof (CHAR16)); > AsciiStrToUnicodeStrS ((const CHAR8 *)Uuid, DiscoveredInstance->In= formation.Uuid, AsciiStrSize ((const CHAR8 *)Uuid) * sizeof (CHAR16)); > @@ -1015,9 +984,11 @@ AddAndSignalNewRedfishService ( > (EFI_REST_EX_CONFIG_DATA)(UINT8 *)RestExHttpCon= figData > ); > 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 =3D 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 =3D=3D NULL) || (NetworkIntfInstances =3D=3D NULL) || (Numbe= rOfNetworkIntfs =3D=3D NULL) || > (ImageHandle =3D=3D NULL)) > { > @@ -1244,29 +1217,23 @@ RedfishServiceGetNetworkInterface ( > while (TRUE) { > // If Get Subnet Info failed then skip this interface > Status =3D NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, = ImageHandle); // Get subnet info > - if (EFI_ERROR (Status)) { > - if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, &ThisNetwor= kInterfaceIntn->Entry)) { > - break; > + if (!EFI_ERROR (Status)) { > + if (!ThisNetworkInterface->IsIpv6) { > + IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4, &ThisNetwo= rkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information. > + } else { > + IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6, &ThisNetwo= rkInterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in IPv6 address= information. > } > > - ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFA= CE_INTERNAL *)GetNextNode (&mEfiRedfishDiscoverNetworkInterface, &ThisNetwo= rkInterfaceIntn->Entry); > - continue; > + ThisNetworkInterface->SubnetPrefixLength =3D ThisNetworkInterfaceI= ntn->SubnetPrefixLength; > } > > + CopyMem ((VOID *)&ThisNetworkInterface->MacAddress, &ThisNetworkInte= rfaceIntn->MacAddress, ThisNetworkInterfaceIntn->HwAddressSize); > ThisNetworkInterface->IsIpv6 =3D FALSE; > if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) { > ThisNetworkInterface->IsIpv6 =3D TRUE; > } > > - CopyMem ((VOID *)&ThisNetworkInterface->MacAddress, &ThisNetworkInte= rfaceIntn->MacAddress, ThisNetworkInterfaceIntn->HwAddressSize); > - if (!ThisNetworkInterface->IsIpv6) { > - IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4, &ThisNetwork= InterfaceIntn->SubnetAddr.v4); // IPv4 subnet information. > - } else { > - IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6, &ThisNetwork= InterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in IPv6 address i= nformation. > - } > - > - ThisNetworkInterface->SubnetPrefixLength =3D ThisNetworkInterfaceInt= n->SubnetPrefixLength; > - ThisNetworkInterface->VlanId =3D ThisNetworkInterfaceInt= n->VlanId; > + ThisNetworkInterface->VlanId =3D ThisNetworkInterfaceIntn->VlanId; > RestExInstance->NumberOfNetworkInterfaces++; > if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkI= nterfaceIntn->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 =3D=3D 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 =3D NetworkInterfaceGetSubnetInfo (TargetNetworkInterfaceI= nternal, ImageHandle); > if (EFI_ERROR (Status1)) { > - DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", __fun= c__)); > - FreePool (Instance); > - continue; > + // > + // Get subnet information could be failed for EFI_REDFISH_DISCOV= ER_HOST_INTERFACE case. > + // We will configure network in AddAndSignalNewRedfishService. S= o don't skip this > + // target network interface. > + // > + if ((Flags & EFI_REDFISH_DISCOVER_HOST_INTERFACE) =3D=3D 0) { > + DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n", __f= unc__)); > + FreePool (Instance); > + continue; > + } > } > > NewInstance =3D TRUE; > @@ -1448,6 +1422,7 @@ RedfishServiceAbortAcquire ( > IN EFI_REDFISH_DISCOVER_NETWORK_INTERFACE *TargetNetworkInterface OPT= IONAL > ) > { > + DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__)); > // This function is used to abort Redfish service discovery through SS= DP > // on the network interface. SSDP is optionally suppoted by EFI_REDFIS= H_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", __fu= nc__)); > return EFI_NOT_FOUND; > @@ -1723,22 +1700,13 @@ BuildupNetworkInterface ( > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance; > EFI_TPL OldTpl; > BOOLEAN NewNetworkInterfaceIn= stalled; > - UINT8 IpType; > UINTN ListCount; > > ListCount =3D (sizeof (mRequiredProtocol) / sizeof = (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); > NewNetworkInterfaceInstalled =3D FALSE; > Index =3D 0; > > - // Get IP Type to filter out unnecessary network protocol if possible > - IpType =3D GetHiIpProtocolType (); > - > for (Index =3D 0; Index < ListCount; Index++) { > - // Check IP Type and skip an unnecessary network protocol if does no= t match > - if (FilterProtocol (mRequiredProtocol[Index].ProtocolType, IpType)) = { > - continue; > - } > - > Status =3D 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. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-