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 995DBD8114F for ; Thu, 16 Nov 2023 12:15:04 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Xn4A7fq2wpB8N4MqHUw6Tjw20HZUdSgWoLPw0xfe68o=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700136903; v=1; b=CVr0Uduv/fYqOxrVB0UQoMoRhnRVPx9NHYw3Yj6y9oPvtPUywa+q16zyeBncRfVb16AmclQt niCCaOX7wqOO0utwNcw3XlF4+cqWCDtYDJYFDSnW2cKg0KP5rNEAxtPZKGPdCLA/u9g/+6OpES7 tSpaG0N1QhXzExPCCQRyhGWk= X-Received: by 127.0.0.2 with SMTP id xg3sYY7687511xgDbphzW7bk; Thu, 16 Nov 2023 04:15:03 -0800 X-Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web11.5332.1700136902733203411 for ; Thu, 16 Nov 2023 04:15:02 -0800 X-Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AGAfug2030895; Thu, 16 Nov 2023 12:15:00 GMT X-Received: from nasanppmta04.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3udhe1r5rh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Nov 2023 12:14:59 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AGCExcd015371 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Nov 2023 12:14:59 GMT X-Received: from [10.111.130.96] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Thu, 16 Nov 2023 04:14:57 -0800 Message-ID: <3ab53d06-fdcb-4a7b-a96d-4568e7219182@quicinc.com> Date: Thu, 16 Nov 2023 12:14:54 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow To: , , Igor Kulchytskyy CC: Abner Chang , Nickle Wang References: <20231114142815.1604-1-igork@ami.com> <3e8c08dc-a665-41f1-8c51-d4e2743bdac1@quicinc.com> <2f8fbb9f-bca8-4771-9178-6f0507ad089e@quicinc.com> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: CBooJiyl7Uen0P5qTKxl6Yj14jNh7N56 X-Proofpoint-ORIG-GUID: CBooJiyl7Uen0P5qTKxl6Yj14jNh7N56 X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-0031df01.pphosted.com id 3AGAfug2030895 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,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: kSqbmjcEuOmlATsWyAFXRl2px7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed 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=CVr0Uduv; 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=quicinc.com (policy=none) On 2023-11-15 18:27, Mike Maslenkin wrote: > On Wed, Nov 15, 2023 at 4:52=E2=80=AFPM Igor Kulchytskyy = wrote: >> >> Hello Leif and Mike, >> Let me try to explain the idea of the filtering IP. >> That filtering should work only if we know exactly that IP is IPv4 or IP= v6 in SMBIOS Type 42. > Hm. I've already composed a reply below, but then a returned to this > statement... >=20 > Is this a difference in condition between v3 and v5? I came to the > conclusion that at the place we are discussing > SMBIOS table 42h can be absent because > PlatformHostInterfaceInformationReady hasn't been called yet, > so REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN is expected. >=20 >=20 >> And it just helping to reduce the work in case we know the exact type of= IP, which supposed to be used in BIOS BMC communication. >> In that case there is no need to build network interface for the unused = IP Type. >> On some systems IP address could be dynamic and we will not be able to k= now the version of IP from SMBIOS. >> If you see I check HostIpAssignmentType in GetHiIpProtocolType function.= And return IP type UNKNOWN if it is not static. >> If we get an unknown IP type, we should not return from BuildupNetworkIn= terface function, but just proceed and build the network interface for all = IPs. >> So, later Redfish Discover driver can find the right one. >> Based on this logic I'm going to prepare the patch v6. >> Thank you, >> Igor >=20 > Agree.. I was focused on edk2 implementation of > RedfishPlatformHostInterfaceLib and PlatformHostInterfaceBmcUsbNicLib > where HostIpAddressFormat is specified (hardcoded). I guess > HostIpAddressFormat as well as SMBIOS table 42h must be available > by the time RedfishServiceAcquireService()->DiscoverRedfishHostInterface(= ) > call, while it might be not available at the moment > RedfishDiscoverDriverBindingStart()->BuildupNetworkInterface(). So, > condition from v3 looks correct to me. >=20 > My main concern was introduction of defines. Those don't look great. > Those are huge (it even doesn't fit into the screen) and misleading a > bit. > For example: > +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface) > (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > &TargetNetworkInterface->MacAddress, > ThisNetworkInterface->HwAddressSize)) >=20 > The proposed variant is equal to #define MAC_COMPARE(A, B) > (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, > &TargetNetworkInterface->MacAddress, > ThisNetworkInterface->HwAddressSize)), i.e a bit useless. >=20 > I would expect it could be declared at least as: > #define MAC_COMPARE(This, Target) CompareMem ((VOID > *)&(This)->MacAddress, &(Target)->MacAddress, (This)->HwAddressSize) > I.e define should really replace some arguments also reducing the line l= ength. >=20 > BTW: there is a place in ValidateTargetNetworkInterface() where > CompareMem can be replaced with MAC_COMPARE too. >=20 > Also, I found IP6_LINK_EQUAL(Mac1, Mac2) define, that is unused in > edk2. But according to that one, please consider moving "=3D=3D 0" check > to #define declaration. > But I do not think this macro is required at all, because there are 5 > MAC compares left in this module. So, it just brings some > inconsistency. >=20 > Agreed that static helper function would be the best. >=20 > Leif, do you expect something like this? > STATIC > BOOLEAN > FilterInterface ( > IN NETWORK_INTERFACE_PROTOCOL_TYPE ProtocolType, > IN UINT8 HostIpAddressFormat > ) > { > // This is based on v5, but according to the comments above > // v3 is correct as it allows > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN >=20 > if (ProtocolType =3D=3D ProtocolTypeTcp4) { > return IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4= ; > } else if (ProtocolType =3D=3D ProtocolTypeTcp6) { > return IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6= ; > } Yes, this looks ideal. > return false; Although this should be FALSE (upper-case). > } >=20 > and then:: >=20 > // Get IP Type to filter out unnecessary network protocol if possible > IpType =3D GetHiIpProtocolType (); >=20 > for (Index =3D 0; Index < ListCount; Index++) { > // Check IP Type and skip an unnecessary network protocol if does not = match > if (FilterInterface (gRequiredProtocol[Index].ProtocolType, IpType)) { This gives us a big but still readable chunk here, and much neater tests=20 in the helper than they *could* be either in place or in macros. > continue; > } Ship it. / Leif > Regards, > Mike. >=20 >=20 >=20 >=20 >=20 -=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 (#111314): https://edk2.groups.io/g/devel/message/111314 Mute This Topic: https://groups.io/mt/102584140/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-