Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

["Ranbir Singh" <rsingh@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 12244 bytes --]

On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/9/23 18:39, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > 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 <ray.ni@intel.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  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 <lersek@redhat.com>
>
> 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    = 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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 15761 bytes --]