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 B34777803D0 for ; Tue, 12 Dec 2023 21:31:53 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=rKwE4IVInx2KjpUWp23CVWcywLux9NjQa6ytpOtm6Oc=; 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=1702416712; v=1; b=YbAnMiPhES3GnmsI3q+Ipg79NATI1cuEkxOkh38BCCq4tNN8z648IunPH6rq8qJJfuXLp/Fo eqWA1az5FGPghI5Gz61E3ZCzY/lsHx+hW3Xeh2I92WCtquynmI8cqvtaC0GzbTQqhMXhwCiQ5yS BqqWYWClQZdX+/bRwe36oWhc= X-Received: by 127.0.0.2 with SMTP id gILJYY7687511xNYZAh1EOin; Tue, 12 Dec 2023 13:31:52 -0800 X-Received: from mail-yw1-f181.google.com (mail-yw1-f181.google.com [209.85.128.181]) by mx.groups.io with SMTP id smtpd.web10.8596.1702416711863731682 for ; Tue, 12 Dec 2023 13:31:52 -0800 X-Received: by mail-yw1-f181.google.com with SMTP id 00721157ae682-5d05ff42db0so61574317b3.2 for ; Tue, 12 Dec 2023 13:31:51 -0800 (PST) X-Gm-Message-State: 6QaUOWpauclJzVMMeLUjAQRnx7686176AA= X-Google-Smtp-Source: AGHT+IGjswndbUEHjDJPCC9TRdP6M02cr6phyxWYvopORqW169pKPMpxR3Y3jBnEIMQ5iRV2kFc5YXTWQxsG0doXE6o= X-Received: by 2002:a0d:c801:0:b0:5d7:1941:2c17 with SMTP id k1-20020a0dc801000000b005d719412c17mr5251637ywd.68.1702416710890; Tue, 12 Dec 2023 13:31:50 -0800 (PST) MIME-Version: 1.0 References: <20231211145850.1553-1-abner.chang@amd.com> In-Reply-To: <20231211145850.1553-1-abner.chang@amd.com> From: "Mike Maslenkin" Date: Wed, 13 Dec 2023 00:31:14 +0300 Message-ID: Subject: Re: [edk2-devel] [PATCH V2] 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=YbAnMiPh; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Mon, Dec 11, 2023 at 5:59=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. > > Change-Id: Ibb38ddcd17459ad4b23fcb4fcd8a771a0f63987a > Signed-off-by: Abner Chang > Cc: Nickle Wang > Cc: Igor Kulchytskyy > Cc: Mike Maslenkin > --- > .../RedfishDiscoverDxe/RedfishDiscoverDxe.c | 109 +++++++----------- > 1 file changed, 39 insertions(+), 70 deletions(-) > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/Redfish= Pkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c > index 833ae2b969f..06d8d00da7f 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; > @@ -675,6 +641,9 @@ DiscoverRedfishHostInterface ( > IsHttps =3D FALSE; > if (Data->RedfishServiceIpPort =3D=3D 443) { > IsHttps =3D TRUE; > + DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 443\n")); > + } else { > + DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 80\n")); > } > I thought it could be unconditional trace: DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: %d\n", Data->RedfishServiceIpPort)); > StrSize =3D sizeof (UuidStr); > @@ -737,8 +706,6 @@ DiscoverRedfishHostInterface ( > IsHttps > ); > } > - } else { > - DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is fai= led.\n", __func__)); > } > > return Status; > @@ -1015,9 +982,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 +1176,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)) > { > @@ -1242,31 +1213,27 @@ RedfishServiceGetNetworkInterface ( > > ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_I= NTERNAL *)GetFirstNode (&mEfiRedfishDiscoverNetworkInterface); > 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; > - } > - > - ThisNetworkInterfaceIntn =3D (EFI_REDFISH_DISCOVER_NETWORK_INTERFA= CE_INTERNAL *)GetNextNode (&mEfiRedfishDiscoverNetworkInterface, &ThisNetwo= rkInterfaceIntn->Entry); > - continue; > - } > - > 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. > + // > + // If Get Subnet Info failed then skip this interface > + // > + Status =3D NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, = ImageHandle); // Get subnet info > + 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. > + } > + > + ThisNetworkInterface->SubnetPrefixLength =3D ThisNetworkInterfaceI= ntn->SubnetPrefixLength; > } > > - 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; > @@ -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 > With or without unified "Redfish service port" trace Acked-by: Mike Maslenkin -=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 (#112456): https://edk2.groups.io/g/devel/message/112456 Mute This Topic: https://groups.io/mt/103109949/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-