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

[Laszlo Ersek <lersek@redhat.com>]

On 03/28/17 12:51, Ard Biesheuvel wrote:
> 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.
> 

Good point.