On 11/14/23 17:11, Ranbir Singh wrote: > > > On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek <ler...@redhat.com > <mailto:ler...@redhat.com>> wrote: > > On 11/9/23 18:39, Ranbir Singh wrote: > > From: Ranbir Singh <ranbir.sin...@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 > <https://bugzilla.tianocore.org/show_bug.cgi?id=4212> > > > > Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com>> > > Signed-off-by: Ranbir Singh <rsi...@ventanamicro.com > <mailto:rsi...@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 <ler...@redhat.com <mailto:ler...@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.
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
