Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 March 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/28/17 12:43, Laszlo Ersek wrote:
>> Ard,
>>
>> On 03/27/17 12:07, Ard Biesheuvel wrote:
>
> [snip]
>
>>> +/**
>>> +  The entry point for DtPlatformDxe driver.
>>> +
>>> +  @param[in] ImageHandle     The image handle of the driver.
>>> +  @param[in] SystemTable     The system table.
>>> +
>>> +  @retval EFI_ALREADY_STARTED     The driver already exists in system.
>>> +  @retval EFI_OUT_OF_RESOURCES    Fail to execute entry point due to lack of
>>> +                                  resources.
>>> +  @retval EFI_SUCCES              All the related protocols are installed on
>>> +                                  the driver.
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +DtPlatformDxeEntryPoint (
>>> +  IN EFI_HANDLE                   ImageHandle,
>>> +  IN EFI_SYSTEM_TABLE             *SystemTable
>>> +  )
>>> +{
>>> +  EFI_STATUS                      Status;
>>> +  EFI_HANDLE                      DriverHandle;
>>> +  DT_ACPI_VARSTORE_DATA           DtAcpiPref;
>>> +  UINTN                           BufferSize;
>>> +  VOID                            *Dtb;
>>> +  UINTN                           DtbSize;
>>> +  VOID                            *DtbCopy;
>>> +
>>> +  //
>>> +  // Check whether a DTB blob is included in the firmware image.
>>> +  //
>>> +  Dtb = NULL;
>>> +  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
>>> +             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
>>> +  if (EFI_ERROR (Status)) {
>>> +    DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
>>> +      __FUNCTION__));
>>> +    DtAcpiPref.Pref = DT_ACPI_SELECT_ACPI;
>>> +  } else {
>>> +    //
>>> +    // Get the current ACPI/DT preference from the AcpiDtPref variable.
>>> +    //
>>
>> The comment says "AcpiDtPref variable" (which corresponds to the name under which the VFR also refers to the variable), but below the variable services actually get a "DtAcpiPref" argument as variable name.
>>
>> Please fix this inconsistency together with the other instances.
>>
>>> +    BufferSize = sizeof (DtAcpiPref);
>>> +    Status = gRT->GetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid, NULL,
>>> +                    &BufferSize, &DtAcpiPref);
>>> +    if (EFI_ERROR (Status)) {
>>> +      DEBUG ((DEBUG_WARN, "%a: no DT/ACPI preference found, defaulting to DT\n",
>>> +        __FUNCTION__));
>>> +      DtAcpiPref.Pref = DT_ACPI_SELECT_DT;
>>> +    }
>>> +  }
>>> +
>>> +  if (!EFI_ERROR (Status) &&
>>> +      DtAcpiPref.Pref != DT_ACPI_SELECT_ACPI &&
>>> +      DtAcpiPref.Pref != DT_ACPI_SELECT_DT) {
>>> +    DEBUG ((DEBUG_WARN, "%a: illegal value for DtAcpiPref, defaulting to DT\n",
>>> +      __FUNCTION__));
>>
>> Personal request: please replace "illegal" with "invalid".
>>
>>> +    DtAcpiPref.Pref = DT_ACPI_SELECT_DT;
>>> +    Status = EFI_INVALID_PARAMETER; // trigger setvar below
>>> +  }
>>> +
>>> +  //
>>> +  // Write the newly selected default value back to the variable store.
>>> +  //
>>> +  if (EFI_ERROR (Status)) {
>>> +    Status = gRT->SetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid,
>>> +                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
>>> +                    sizeof (DtAcpiPref), &DtAcpiPref);
>>
>> I suggest to use a macro for the variable name...
>>
>>> +    if (EFI_ERROR (Status)) {
>>> +      return Status;
>>> +    }
>>> +  }
>>> +
>>> +  if (DtAcpiPref.Pref == DT_ACPI_SELECT_ACPI) {
>>> +    //
>>> +    // ACPI was selected: install the gEdkiiPlatformHasAcpiGuid GUID as a
>>> +    // NULL protocol to unlock dispatch of ACPI related drivers.
>>> +    //
>>> +    DriverHandle = NULL;
>>> +    Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle,
>>> +                    gEdkiiPlatformHasAcpiGuid, NULL, NULL);
>>> +    if (EFI_ERROR (Status)) {
>>> +      DEBUG ((DEBUG_ERROR,
>>> +        "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n",
>>> +        __FUNCTION__));
>>> +      return Status;
>>> +    }
>>
>> I think you don't need to create a new controller handle for gEdkiiPlatformHasAcpiGuid; using ImageHandle should be fine.
>>
>> (Same for gEfiDevicePathProtocolGuid in InstallHiiPages().)
>>
>> Not critical though, just simpler perhaps.
>>
>>> +  } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
>>> +    //
>>> +    // DT was selected: copy the blob into newly allocated memory and install
>>> +    // a reference to it as the FDT configuration table.
>>> +    //
>>> +    DtbCopy = AllocateCopyPool (DtbSize, Dtb);
>>> +    if (DtbCopy == NULL) {
>>> +      return EFI_OUT_OF_RESOURCES;
>>> +    }
>>> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy);
>>> +    if (EFI_ERROR (Status)) {
>>> +      DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n",
>>> +        __FUNCTION__));
>>> +      FreePool (DtbCopy);
>>> +      return Status;
>>> +    }
>>
>> Looks good. The original DTB (found in the FV section) lives in flash, generally speaking, right?
>>
>>> +  } else {
>>> +    ASSERT (FALSE);
>>> +  }
>>> +
>>> +  //
>>> +  // No point in installing the HII pages if ACPI is the only description
>>> +  // we have
>>> +  //
>>> +  if (Dtb == NULL) {
>>> +    return EFI_SUCCESS;
>>> +  }
>>> +
>>> +  return InstallHiiPages ();
>>> +}
>
> Consider uninstalling the FDT config table / the "has ACPI" protocol if
> InstallHiiPages() fails and the driver exits with error (it gets
> unloaded)? My inner pedant wants you to do this, although I do realize
> that neither the FDT config table nor the NULL "has ACPI" protocol hook
> back into this driver.
>

That was my reasoning, yes. I will add a comment to that effect, but I
am reluctant to break the boot entirely just because we failed to
register the HII pages.