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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo