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

["Laszlo Ersek" <lersek@redhat.com>]

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.

>
>          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

>
>          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 (#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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-