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 2436D7803E3 for ; Tue, 14 Nov 2023 16:11:18 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=XzJk5s7dc0Ks71Pd92dEeRTxMyNIRnei2rw8gAer53w=; 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; s=20140610; t=1699978277; v=1; b=vXApOUqdgL7bQVjPRLqz0Z3N7A/1nJ8GCkX0rnZ6QY4pYolTFqgq9n+KZiuUH9KR3VhY9UcY 8F6i96fBQgY3FWDiDvPfUIykoxZP0gVZvkp+k0PTcU8mEjAGi7cJkn8mbkaglYupsuNPdavC55m 4DQgCv6gYIfIgRpYXDOxuM/g= X-Received: by 127.0.0.2 with SMTP id Ko0iYY7687511xaByBkuHGWk; Tue, 14 Nov 2023 08:11:17 -0800 X-Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mx.groups.io with SMTP id smtpd.web11.16600.1699978276818816203 for ; Tue, 14 Nov 2023 08:11:16 -0800 X-Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-28039ee1587so4407067a91.2 for ; Tue, 14 Nov 2023 08:11:16 -0800 (PST) X-Gm-Message-State: TgoQkedRiAb4wpR7MXU7uOkkx7686176AA= X-Google-Smtp-Source: AGHT+IFIZiVpgVi0BwxTKrwNpEFxrtUaE12D52RfFlRvgy8RENNSStzWnAHIpoZDSnfZFGg0tY6OfiVPnZI3njsjFaM= X-Received: by 2002:a17:90b:38c5:b0:27d:375d:c16e with SMTP id nn5-20020a17090b38c500b0027d375dc16emr7695891pjb.42.1699978275259; Tue, 14 Nov 2023 08:11:15 -0800 (PST) MIME-Version: 1.0 References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-3-rsingh@ventanamicro.com> <4a14bdb3-a9a1-c55a-0230-5d60eb179f88@redhat.com> In-Reply-To: <4a14bdb3-a9a1-c55a-0230-5d60eb179f88@redhat.com> From: "Ranbir Singh" Date: Tue, 14 Nov 2023 21:41:04 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue To: Laszlo Ersek Cc: devel@edk2.groups.io, Ray Ni 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,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000391a34060a1f090d" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=vXApOUqd; dmarc=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 --000000000000391a34060a1f090d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 13, 2023 at 10:03=E2=80=AFPM Laszlo Ersek w= rote: > On 11/9/23 18:39, Ranbir Singh wrote: > > From: Ranbir Singh > > > > The function SubmitResources has a switch-case code in which the > > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to > > case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check. > > > > While this may be intentional, it may not be evident to any general cod= e > > reader/developer or static analyis tool why there is no break in betwee= n. > > > > SubmitResources function is supposed to handle only Mem or IO resources= . > > So, update the ResType parameter check reflecting that and re-model the > > switch-case code in contention using just one if condition to further > > handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM. > > This leads to mostly indentation level code changes. Few ASSERT's later > > present are henceforth not required. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4212 > > > > Cc: Ray Ni > > Signed-off-by: Ranbir Singh > > --- > > MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60 > +++++++++----------- > > 1 file changed, 26 insertions(+), 34 deletions(-) > > I have applied this patch locally, and displayed it with > > git show -W -b > > Let me make comments on that: > > > /** > > > > Submits the I/O and memory resource requirements for the specified > PCI Root Bridge. > > > > @param This The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION= _ > PROTOCOL instance. > > - @param RootBridgeHandle The PCI Root Bridge whose I/O and memory > resource requirements. > > + @param RootBridgeHandle The PCI Root Bridge whose I/O and memory > resource requirements > > are being submitted. > > @param Configuration The pointer to the PCI I/O and PCI memory > resource descriptor. > > > > @retval EFI_SUCCESS Succeed. > > @retval EFI_INVALID_PARAMETER Wrong parameters passed in. > > **/ > > @@ -1460,137 +1460,129 @@ EFIAPI > > SubmitResources ( > > IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL *This, > > IN EFI_HANDLE RootBridgeHandl= e, > > IN VOID *Configuration > > ) > > { > > LIST_ENTRY *Link; > > PCI_HOST_BRIDGE_INSTANCE *HostBridge; > > PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; > > PCI_RESOURCE_TYPE Type; > > > > // > > // Check the input parameter: Configuration > > // > > if (Configuration =3D=3D NULL) { > > return EFI_INVALID_PARAMETER; > > } > > > > HostBridge =3D PCI_HOST_BRIDGE_FROM_THIS (This); > > for (Link =3D GetFirstNode (&HostBridge->RootBridges) > > ; !IsNull (&HostBridge->RootBridges, Link) > > ; Link =3D GetNextNode (&HostBridge->RootBridges, Link) > > ) > > { > > RootBridge =3D ROOT_BRIDGE_FROM_LINK (Link); > > if (RootBridgeHandle =3D=3D RootBridge->Handle) { > > DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for %s\n", > RootBridge->DevicePathStr)); > > // > > // Check the resource descriptors. > > // If the Configuration includes one or more invalid resource > descriptors, all the resource > > // descriptors are ignored and the function returns > EFI_INVALID_PARAMETER. > > // > > for (Descriptor =3D (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR > *)Configuration; Descriptor->Desc =3D=3D ACPI_ADDRESS_SPACE_DESCRIPTOR; > Descriptor++) { > > - if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) { > > + if ((Descriptor->ResType !=3D ACPI_ADDRESS_SPACE_TYPE_MEM) && > (Descriptor->ResType !=3D ACPI_ADDRESS_SPACE_TYPE_IO)) { > > return EFI_INVALID_PARAMETER; > > } > > This is a slight logic change. > > Previously, the code did the following: > > - Any resource types that were different from MEM, IO, and BUS, would be > rejected with EFI_INVALID_PARAMETER. > > - MEM and IO would be handled. > > - BUS resource types would trigger an ASSERT(). > > In effect, the code claimed that BUS resource types were *impossible* > here, due to prior filtering or whatever. > > The logic change is that now we reject BUS resource types explicitly. > > I think that's not ideal. Here's why: > > - If the preexistent ASSERT() about BUS resource types being impossible > was right, then now we are occluding that fact, and pretending / > suggesting that BUS types are something we *should* handle. This > creates a confusion about data flow. > > - If the preexistent ASSERT() about BUS resource types being impossible > was wrong (i.e., dependent on input data, we could actuall trigger the > ASSERT()), then this change is very welcome, but *then* it is a bugfix > in its own right! And therefore should be documented separately. > > Anyway. I'm getting exhausted. > My interpretation was SubmitResources function is supposed to handle only Mem or IO resources, so > ACPI_ADDRESS_SPACE_TYPE_BUS check should actually have been >=3D ACPI_ADDRESS_SPACE_TYPE_BUS. The function header also states Mem or IO resources only and hence I considered it documented. Also, considering only the RELEASE builds, if ResType comes as ACPI_ADDRESS_SPACE_TYPE_BUS (even if hypothetically), then switch block code lands in default, the ASSERT there is compiled out and after break normal execution will carry on which at the following code point in the for loop also contains ASSERT but that also gets compiled out (I am talking RELEASE build only behaviour here) leading to it getting treated as IO which doesn't seem correct at all to me. ... } else { ASSERT (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_IO); Type =3D TypeIo; } There was no other handling in the function wrt BUS resource types. If the BUS resource needs to be ASSERT'ed as before, then it can be done by including one more if check. However, I don't think code should progress ahead (even if hypothetically and inadvertently) to treating it as IO. > > > > DEBUG (( > > DEBUG_INFO, > > " %s: Granularity/SpecificFlag =3D %ld / %02x%s\n", > > mAcpiAddressSpaceTypeStr[Descriptor->ResType], > > Descriptor->AddrSpaceGranularity, > > Descriptor->SpecificFlag, > > (Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0 ? L= " > (Prefetchable)" : L"" > > )); > > DEBUG ((DEBUG_INFO, " Length/Alignment =3D 0x%lx / 0x%lx\= n", > Descriptor->AddrLen, Descriptor->AddrRangeMax)); > > - switch (Descriptor->ResType) { > > - case ACPI_ADDRESS_SPACE_TYPE_MEM: > > + > > + if (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_MEM) { > > if ((Descriptor->AddrSpaceGranularity !=3D 32) && > (Descriptor->AddrSpaceGranularity !=3D 64)) { > > return EFI_INVALID_PARAMETER; > > } > > > > if ((Descriptor->AddrSpaceGranularity =3D=3D 32) && > (Descriptor->AddrLen >=3D SIZE_4GB)) { > > return EFI_INVALID_PARAMETER; > > } > > > > // > > // If the PCI root bridge does not support separate windows > for nonprefetchable and > > // prefetchable memory, then the PCI bus driver needs to > include requests for > > // prefetchable memory in the nonprefetchable memory pool. > > // > > if (((RootBridge->AllocationAttributes & > EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) !=3D 0) && > > ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0) > > ) > > { > > return EFI_INVALID_PARAMETER; > > } > > + } > > > > - case ACPI_ADDRESS_SPACE_TYPE_IO: > > // > > // Check aligment, it should be of the form 2^n-1 > > // > > if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) !=3D > (Descriptor->AddrRangeMax + 1)) { > > return EFI_INVALID_PARAMETER; > > } > > - > > - break; > > - default: > > - ASSERT (FALSE); > > - break; > > - } > > } > > The patch is good and clever thus far. We restrict the types we handle > to MEM and IO. Then we have a block of code that runs only for MEM, and > then another that -- due to being unrestricted -- runs for both MEM and > IO. > > > > > if (Descriptor->Desc !=3D ACPI_END_TAG_DESCRIPTOR) { > > return EFI_INVALID_PARAMETER; > > } > > > > for (Descriptor =3D (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR > *)Configuration; Descriptor->Desc =3D=3D ACPI_ADDRESS_SPACE_DESCRIPTOR; > Descriptor++) { > > if (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_MEM) { > > if (Descriptor->AddrSpaceGranularity =3D=3D 32) { > > if ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0) { > > Type =3D TypePMem32; > > } else { > > Type =3D TypeMem32; > > } > > } else { > > - ASSERT (Descriptor->AddrSpaceGranularity =3D=3D 64); > > if ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0) { > > Type =3D TypePMem64; > > } else { > > Type =3D TypeMem64; > > } > > } > > } else { > > - ASSERT (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_I= O); > > Type =3D TypeIo; > > } > > But the removal of these ASSERT()s is hard to justify. > > Yes, these predicates are always TRUE at this point, due to checks > performed above. > > But that's *exactly the reason* for including an ASSERT() in the code! > To remind the reader that a (perhaps non-obvious) predicate always holds > at that location! > > So, the argument > > ASSERT(X) is unneeded because X always holds > > is backwards. You do an ASSERT(X) *because* X always holds, and X is > non-trivial! > > The only reason for removing an ASSERT(X) is that X, while true, is > *trivial*. > > In this particular case, we do indeed explicitly restrict > Descriptor->AddrSpaceGranularity to 32 or 64 above, on the MEM branch. > > We also ensure that the only resource type other than MEM can be IO. > > In other words, the assertions are true. > > Now the question is: are those true statements *trivial*? If they are > trivial, then removing them is OK. If they are not trivial, then they > are worth keeping. > > In my opinion, they are worth keeping. They are *reminders* that we > performed those checks above. > > But I'm not going to die on this hill, so if you don't want to respin: > > Reviewed-by: Laszlo Ersek > > Laszlo > > Yes, I felt that to be trivial given the checks are already in place in the same function earlier. However, I am open to re-spin and restore these ASSERT's. > > > > RootBridge->ResAllocNode[Type].Length =3D Descriptor->AddrL= en; > > RootBridge->ResAllocNode[Type].Alignment =3D > Descriptor->AddrRangeMax; > > RootBridge->ResAllocNode[Type].Status =3D ResSubmitted; > > } > > > > RootBridge->ResourceSubmitted =3D TRUE; > > return EFI_SUCCESS; > > } > > } > > > > return EFI_INVALID_PARAMETER; > > } > > -=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 (#111205): https://edk2.groups.io/g/devel/message/111205 Mute This Topic: https://groups.io/mt/102490514/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- --000000000000391a34060a1f090d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Nov 13, 2023 at 10:03=E2=80= =AFPM Laszlo Ersek <lersek@redhat.c= om> wrote:
https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4212
>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60 +++++= ++++-----------
>=C2=A0 1 file changed, 26 insertions(+), 34 deletions(-)

I have applied this patch locally, and displayed it with

=C2=A0 git show -W -b

Let me make comments on that:

>=C2=A0 /**
>
>=C2=A0 =C2=A0 Submits the I/O and memory resource requirements for the = specified PCI Root Bridge.
>
>=C2=A0 =C2=A0 @param This=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_ PROTOCOL instance.
> -=C2=A0 @param RootBridgeHandle=C2=A0 The PCI Root Bridge whose I/O an= d memory resource requirements.
> +=C2=A0 @param RootBridgeHandle=C2=A0 The PCI Root Bridge whose I/O an= d memory resource requirements
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0are being submitted.
>=C2=A0 =C2=A0 @param Configuration=C2=A0 =C2=A0 =C2=A0The pointer to th= e PCI I/O and PCI memory resource descriptor.
>
>=C2=A0 =C2=A0 @retval EFI_SUCCESS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 Succeed.
>=C2=A0 =C2=A0 @retval EFI_INVALID_PARAMETER=C2=A0 Wrong parameters pass= ed in.
>=C2=A0 **/
> @@ -1460,137 +1460,129 @@ EFIAPI
>=C2=A0 SubmitResources (
>=C2=A0 =C2=A0 IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL=C2=A0= *This,
>=C2=A0 =C2=A0 IN EFI_HANDLE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 RootBridgeHandle,
>=C2=A0 =C2=A0 IN VOID=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Configuration
>=C2=A0 =C2=A0 )
>=C2=A0 {
>=C2=A0 =C2=A0 LIST_ENTRY=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*Link;
>=C2=A0 =C2=A0 PCI_HOST_BRIDGE_INSTANCE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0*HostBridge;
>=C2=A0 =C2=A0 PCI_ROOT_BRIDGE_INSTANCE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0*RootBridge;
>=C2=A0 =C2=A0 EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR=C2=A0 *Descriptor;
>=C2=A0 =C2=A0 PCI_RESOURCE_TYPE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 Type;
>
>=C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 // Check the input parameter: Configuration
>=C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 if (Configuration =3D=3D NULL) {
>=C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
>=C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 HostBridge =3D PCI_HOST_BRIDGE_FROM_THIS (This);
>=C2=A0 =C2=A0 for (Link =3D GetFirstNode (&HostBridge->RootBridg= es)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0; !IsNull (&HostBridge->RootBr= idges, Link)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0; Link =3D GetNextNode (&HostBrid= ge->RootBridges, Link)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0)
>=C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 RootBridge =3D ROOT_BRIDGE_FROM_LINK (Link);
>=C2=A0 =C2=A0 =C2=A0 if (RootBridgeHandle =3D=3D RootBridge->Handle)= {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((DEBUG_INFO, "PciHostBridge: Su= bmitResources for %s\n", RootBridge->DevicePathStr));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 // Check the resource descriptors.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 // If the Configuration includes one or mor= e invalid resource descriptors, all the resource
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 // descriptors are ignored and the function= returns EFI_INVALID_PARAMETER.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 for (Descriptor =3D (EFI_ACPI_ADDRESS_SPACE= _DESCRIPTOR *)Configuration; Descriptor->Desc =3D=3D ACPI_ADDRESS_SPACE_= DESCRIPTOR; Descriptor++) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Descriptor->ResType > ACPI_ADDR= ESS_SPACE_TYPE_BUS) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((Descriptor->ResType !=3D ACPI_ADD= RESS_SPACE_TYPE_MEM) && (Descriptor->ResType !=3D ACPI_ADDRESS_S= PACE_TYPE_IO)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

This is a slight logic change.

Previously, the code did the following:

- Any resource types that were different from MEM, IO, and BUS, would be =C2=A0 rejected with EFI_INVALID_PARAMETER.

- MEM and IO would be handled.

- BUS resource types would trigger an ASSERT().

In effect, the code claimed that BUS resource types were *impossible*
here, due to prior filtering or whatever.

The logic change is that now we reject BUS resource types explicitly.

I think that's not ideal. Here's why:

- If the preexistent ASSERT() about BUS resource types being impossible
=C2=A0 was right, then now we are occluding that fact, and pretending /
=C2=A0 suggesting that BUS types are something we *should* handle. This
=C2=A0 creates a confusion about data flow.

- If the preexistent ASSERT() about BUS resource types being impossible
=C2=A0 was wrong (i.e., dependent on input data, we could actuall trigger t= he
=C2=A0 ASSERT()), then this change is very welcome, but *then* it is a bugf= ix
=C2=A0 in its own right! And therefore should be documented separately.

Anyway. I'm getting exhausted.

My i= nterpretation was SubmitResources function is supposed to handle only Mem o= r IO resources, so > ACPI_ADDRESS_SPACE_TYPE_BUS check should actually h= ave been >=3D ACPI_ADDRESS_SPACE_TYPE_BUS. The function header also stat= es Mem or IO resources=C2=A0only and hence I considered it documented.

Also, considering only the RELEASE builds, if ResType = comes as ACPI_ADDRESS_SPACE_TYPE_BUS (even if hypothetically), then switch = block code lands in default, the ASSERT there is compiled out and after bre= ak normal execution will carry on which at the following code point in the = for loop also contains ASSERT but that also gets compiled out (I am talking= RELEASE build only behaviour here) leading to it getting treated as IO whi= ch doesn't seem correct at all to me.

=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ...
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 } else {
=C2=A0=C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0ASSE= RT (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_IO);
=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypeIo;
=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 }

There was no other handling i= n the function wrt BUS resource types.

If the BUS = resource needs=C2=A0to be ASSERT'ed as before, then it can be done by i= ncluding one more if check. However, I don't think code should progress= ahead (even if hypothetically and inadvertently) to treating it as IO.


>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG_INFO,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 " %s: Granularity/Specif= icFlag =3D %ld / %02x%s\n",
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mAcpiAddressSpaceTypeStr[Desc= riptor->ResType],
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Descriptor->AddrSpaceGranu= larity,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Descriptor->SpecificFlag,<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (Descriptor->SpecificFlag = & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0= ? L" (Prefetchable)" : L""
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((DEBUG_INFO, "=C2=A0 =C2= =A0 =C2=A0 Length/Alignment =3D 0x%lx / 0x%lx\n", Descriptor->AddrL= en, Descriptor->AddrRangeMax));
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (Descriptor->ResType) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case ACPI_ADDRESS_SPACE_TYPE_MEM:<= br> > +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Descriptor->ResType =3D=3D ACPI_AD= DRESS_SPACE_TYPE_MEM) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((Descriptor->AddrSpace= Granularity !=3D 32) && (Descriptor->AddrSpaceGranularity !=3D 6= 4)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PAR= AMETER;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((Descriptor->AddrSpace= Granularity =3D=3D 32) && (Descriptor->AddrLen >=3D SIZE_4GB)= ) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PAR= AMETER;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // If the PCI root bridge doe= s not support separate windows for nonprefetchable and
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // prefetchable memory, then = the PCI bus driver needs to include requests for
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // prefetchable memory in the= nonprefetchable memory pool.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (((RootBridge->Allocati= onAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) !=3D 0) &&=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ((Descriptor-&g= t;SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFE= TCHABLE) !=3D 0)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PAR= AMETER;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case ACPI_ADDRESS_SPACE_TYPE_IO: >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // Check aligment, it should be of t= he form 2^n-1
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (GetPowerOfTwo64 (Descriptor->= AddrRangeMax + 1) !=3D (Descriptor->AddrRangeMax + 1)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> -
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 default:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ASSERT (FALSE);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

The patch is good and clever thus far. We restrict the types we handle
to MEM and IO. Then we have a block of code that runs only for MEM, and
then another that -- due to being unrestricted -- runs for both MEM and
IO.

>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Descriptor->Desc !=3D ACPI_END_TAG_D= ESCRIPTOR) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 for (Descriptor =3D (EFI_ACPI_ADDRESS_SPACE= _DESCRIPTOR *)Configuration; Descriptor->Desc =3D=3D ACPI_ADDRESS_SPACE_= DESCRIPTOR; Descriptor++) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Descriptor->ResType =3D=3D AC= PI_ADDRESS_SPACE_TYPE_MEM) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (Descriptor->AddrSpaceG= ranularity =3D=3D 32) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((Descriptor->Sp= ecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHA= BLE) !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypePM= em32;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypeMe= m32;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ASSERT (Descriptor->Addr= SpaceGranularity =3D=3D 64);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((Descriptor->Sp= ecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHA= BLE) !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypePM= em64;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypeMe= m64;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ASSERT (Descriptor->ResType =3D= =3D ACPI_ADDRESS_SPACE_TYPE_IO);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Type =3D TypeIo;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

But the removal of these ASSERT()s is hard to justify.

Yes, these predicates are always TRUE at this point, due to checks
performed above.

But that's *exactly the reason* for including an ASSERT() in the code!<= br> To remind the reader that a (perhaps non-obvious) predicate always holds at that location!

So, the argument

=C2=A0 ASSERT(X) is unneeded because X always holds

is backwards. You do an ASSERT(X) *because* X always holds, and X is
non-trivial!

The only reason for removing an ASSERT(X) is that X, while true, is
*trivial*.
lersek@redhat.com>

Laszlo


Yes, I felt that to be trivial gi= ven the checks are already in place in the same function earlier.
However, I am open to re-spin and restore these ASSERT's.
<= div>=C2=A0
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RootBridge->ResAllocNode[Type].Le= ngth=C2=A0 =C2=A0 =3D Descriptor->AddrLen;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RootBridge->ResAllocNode[Type].Al= ignment =3D Descriptor->AddrRangeMax;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RootBridge->ResAllocNode[Type].St= atus=C2=A0 =C2=A0 =3D ResSubmitted;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 RootBridge->ResourceSubmitted =3D TRUE;<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 return EFI_SUCCESS;
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
>=C2=A0 }

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111205) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000391a34060a1f090d--