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 03E287803CE for ; Mon, 13 Nov 2023 16:33:59 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=B5jmLfzziFF+wUZZPSJVYPBrRJXp7bakUBMbkW5wQJk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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=1699893238; v=1; b=pzV4QcvkwiotMQRFvDrbneRF7KM/WvhT+ZABk8l85sHd96H5qE/9ZBm2DzQdGXl7GUzVYlYl 0QUbG5XHlsqvgkraG8qHoyGYHSkX8rx1NIqm7cMh0gDuUyfedPudOdg9PAj8eTfzIlvoAO+ZcYa z0WTtBqQ4KwhDiXyd/uezGJ0= X-Received: by 127.0.0.2 with SMTP id mCAdYY7687511xhI0UM9J2Ef; Mon, 13 Nov 2023 08:33:58 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.41604.1699893238118888411 for ; Mon, 13 Nov 2023 08:33:58 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-473-aR6fg6c9N56ep95__KqJsg-1; Mon, 13 Nov 2023 11:33:53 -0500 X-MC-Unique: aR6fg6c9N56ep95__KqJsg-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8C67280F830; Mon, 13 Nov 2023 16:33:53 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A61CD1C060AE; Mon, 13 Nov 2023 16:33:52 +0000 (UTC) Message-ID: <4a14bdb3-a9a1-c55a-0230-5d60eb179f88@redhat.com> Date: Mon, 13 Nov 2023 17:33:51 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue To: devel@edk2.groups.io, rsingh@ventanamicro.com Cc: Ray Ni References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-3-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20231109173908.364630-3-rsingh@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: QsgBfnt8rdVQcFI7RhITdjuKx7686176AA= Content-Language: en-US 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=pzV4Qcvk; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.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 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 code > reader/developer or static analyis tool why there is no break in between. > > 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 reso= urce requirements. > + @param RootBridgeHandle The PCI Root Bridge whose I/O and memory reso= urce requirements > are being submitted. > @param Configuration The pointer to the PCI I/O and PCI memory res= ource 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 RootBridgeHandle, > 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", Roo= tBridge->DevicePathStr)); > // > // Check the resource descriptors. > // If the Configuration includes one or more invalid resource desc= riptors, all the resource > // descriptors are ignored and the function returns EFI_INVALID_PA= RAMETER. > // > for (Descriptor =3D (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Configura= tion; 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) && (D= escriptor->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. > > 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) && (Descripto= r->AddrLen >=3D SIZE_4GB)) { > return EFI_INVALID_PARAMETER; > } > > // > // If the PCI root bridge does not support separate windows fo= r nonprefetchable and > // prefetchable memory, then the PCI bus driver needs to inclu= de requests for > // prefetchable memory in the nonprefetchable memory pool. > // > if (((RootBridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_C= OMBINE_MEM_PMEM) !=3D 0) && > ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPEC= IFIC_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 (Descrip= tor->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 *)Configura= tion; 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_SPE= CIFIC_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_SPE= CIFIC_FLAG_CACHEABLE_PREFETCHABLE) !=3D 0) { > Type =3D TypePMem64; > } else { > Type =3D TypeMem64; > } > } > } else { > - ASSERT (Descriptor->ResType =3D=3D ACPI_ADDRESS_SPACE_TYPE_IO)= ; > 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 > > RootBridge->ResAllocNode[Type].Length =3D Descriptor->AddrLen= ; > RootBridge->ResAllocNode[Type].Alignment =3D Descriptor->AddrRan= geMax; > 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 (#111165): https://edk2.groups.io/g/devel/message/111165 Mute This Topic: https://groups.io/mt/102490514/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-