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 21E06D801D3 for ; Wed, 15 Nov 2023 08:55:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=aIMeWusG0Vi6X9Syt18eEjZKh28SgI1lPl8/8wZ0kKA=; 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=1700038553; v=1; b=TD0L/v6HIBuAe4UTtk74VSslwCeS7QY5Enb6kfLw3GvWwqqJw6gZxzIoGCv8B2NuSyy34CzX xxcTdvZJMQtwTpJ7T1QFB431WKrZF5ZNBNZgo4nclM8s0QSmf9KgSlhsN7df8TCh65bPAVBTBmg AuI9jKDt5qKdUAtESNBJMkxU= X-Received: by 127.0.0.2 with SMTP id vs5UYY7687511xaPtYd3wzic; Wed, 15 Nov 2023 00:55:53 -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.web10.9197.1700038552925418895 for ; Wed, 15 Nov 2023 00:55:53 -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-209-Lt5696cPPv--zYzVjdtHyQ-1; Wed, 15 Nov 2023 03:55:50 -0500 X-MC-Unique: Lt5696cPPv--zYzVjdtHyQ-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 4B90C85A58B; Wed, 15 Nov 2023 08:55:50 +0000 (UTC) X-Received: from [10.39.192.211] (unknown [10.39.192.211]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 71CADC15983; Wed, 15 Nov 2023 08:55:49 +0000 (UTC) Message-ID: <1baf8af1-ba03-79d6-fe5a-b0352da0ce5c@redhat.com> Date: Wed, 15 Nov 2023 09:55:48 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue To: Ranbir Singh Cc: devel@edk2.groups.io, Ray Ni References: <20231109173908.364630-1-rsingh@ventanamicro.com> <20231109173908.364630-3-rsingh@ventanamicro.com> <4a14bdb3-a9a1-c55a-0230-5d60eb179f88@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: QNzV6IJwlcvWCuTCtjlTCItHx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="TD0L/v6H"; 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=redhat.com (policy=none) On 11/14/23 17:11, Ranbir Singh wrote: > > > On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek > wrote: > > 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=4212 > > > > > 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                                        > 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 == NULL) { > >      return EFI_INVALID_PARAMETER; > >    } > > > >    HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This); > >    for (Link = GetFirstNode (&HostBridge->RootBridges) > >         ; !IsNull (&HostBridge->RootBridges, Link) > >         ; Link = GetNextNode (&HostBridge->RootBridges, Link) > >         ) > >    { > >      RootBridge = ROOT_BRIDGE_FROM_LINK (Link); > >      if (RootBridgeHandle == 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 = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR > *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; > Descriptor++) { > > -        if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) { > > +        if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) > && (Descriptor->ResType != 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 >= 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 == ACPI_ADDRESS_SPACE_TYPE_IO); >             Type = 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 = %ld / %02x%s\n", > >            mAcpiAddressSpaceTypeStr[Descriptor->ResType], > >            Descriptor->AddrSpaceGranularity, > >            Descriptor->SpecificFlag, > >            (Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0 > ? L" (Prefetchable)" : L"" > >            )); > >          DEBUG ((DEBUG_INFO, "      Length/Alignment = 0x%lx / > 0x%lx\n", Descriptor->AddrLen, Descriptor->AddrRangeMax)); > > -        switch (Descriptor->ResType) { > > -          case ACPI_ADDRESS_SPACE_TYPE_MEM: > > + > > +        if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { > >            if ((Descriptor->AddrSpaceGranularity != 32) && > (Descriptor->AddrSpaceGranularity != 64)) { > >              return EFI_INVALID_PARAMETER; > >            } > > > >            if ((Descriptor->AddrSpaceGranularity == 32) && > (Descriptor->AddrLen >= 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) != 0) && > >                ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 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) != > (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 != ACPI_END_TAG_DESCRIPTOR) { > >          return EFI_INVALID_PARAMETER; > >        } > > > >        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR > *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; > Descriptor++) { > >          if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { > >            if (Descriptor->AddrSpaceGranularity == 32) { > >              if ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) { > >                Type = TypePMem32; > >              } else { > >                Type = TypeMem32; > >              } > >            } else { > > -            ASSERT (Descriptor->AddrSpaceGranularity == 64); > >              if ((Descriptor->SpecificFlag & > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) { > >                Type = TypePMem64; > >              } else { > >                Type = TypeMem64; > >              } > >            } > >          } else { > > -          ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO); > >            Type = 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. I'd like you to respin the patch for two reasons, then: - More importantly: you've demonstrated that the change regarding BUS handling is a bugfix. As requested before, please mention it in the commit message explicitly. (I do agree we can keep the logic change in the same patch.) - Less importantly: keeping these last two ASSERT()s would be nice, then. Thanks! Laszlo >   > > > > >          RootBridge->ResAllocNode[Type].Length    = > Descriptor->AddrLen; > >          RootBridge->ResAllocNode[Type].Alignment = > Descriptor->AddrRangeMax; > >          RootBridge->ResAllocNode[Type].Status    = ResSubmitted; > >        } > > > >        RootBridge->ResourceSubmitted = TRUE; > >        return EFI_SUCCESS; > >      } > >    } > > > >    return EFI_INVALID_PARAMETER; > >  } > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111244): https://edk2.groups.io/g/devel/message/111244 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/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-