From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 222E721BC6A25 for ; Tue, 28 Mar 2017 03:51:03 -0700 (PDT) Received: by mail-it0-x22a.google.com with SMTP id e75so51450435itd.1 for ; Tue, 28 Mar 2017 03:51:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Thd0eGS+R1e5tzzPp63Cwuesq/V9qp16DOieDEq3Qsg=; b=YODxYPIJ4n5Cgnr5XQOR4Z0eOTFxTZwnZYoN1dTrNRk0nje73rZf7QQ/RjLQLXT8cC szVNY2RTOKQbySn2cV8K0Yb1Uz6azYb7xuS117+FIMrlV6dMQvKyyUK7LeIiul4i0VTP H5dXMH9RMBGbJp1gIAvAoTJF9Q7I1W/PHyazY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Thd0eGS+R1e5tzzPp63Cwuesq/V9qp16DOieDEq3Qsg=; b=C33LBJMo5IzpxPs2li198Ro75e4ekLGA3y3rcWOJx0FyChQz2u+Qpbc1Lg7AK7BZqQ UVUIUs0oRssSbtdQJN2iI+sNk6SM2wTVOViP9hfFbGAPxJF32AqzclbSD3a+T45E/uIV NaXRvCsLHIJ6PlWulzCuzPwEC6LRzzIXjPQCVQ45rEJ6fd+VZMlqSJHtF7GEpo8mpaTb DQqcS2ubVbO1F2aglOqjNrJqPkE42YGS/bu58VRJy4Fp7AsNfHTccUdDQlrbwTtw4OyG AgUzP8RH/Q2E6zVxu5gK4Olj2x565qgH+B25qzpevGLWuoxDI5x/vCAzFFNhi+eruJwb /ITQ== X-Gm-Message-State: AFeK/H1VHi/35p9u7SwhkFU5gdEjkDZHO2zfD6OHbkGB8M9J1g+ftCOhCtx5ed8HlM8Xd6pP2QnnhxihMLXCxC3c X-Received: by 10.107.168.21 with SMTP id r21mr24371052ioe.45.1490698262178; Tue, 28 Mar 2017 03:51:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Mar 2017 03:51:01 -0700 (PDT) In-Reply-To: <383c1aff-9a8a-b8c4-a0c4-c5d272f52c36@redhat.com> References: <20170327100754.32387-1-ard.biesheuvel@linaro.org> <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> <383c1aff-9a8a-b8c4-a0c4-c5d272f52c36@redhat.com> From: Ard Biesheuvel Date: Tue, 28 Mar 2017 11:51:01 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Marcin Wojtas , Alexander Graf Subject: Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2017 10:51:03 -0000 Content-Type: text/plain; charset=UTF-8 On 28 March 2017 at 11:49, Laszlo Ersek 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.