[ipxe-devel] [PATCH] [efi] install the HII config access protocol on a child of the SNP handle
Laszlo Ersek
lersek at redhat.com
Fri Jul 1 12:24:32 UTC 2016
On 07/01/16 09:28, Ladi Prosek wrote:
> On Thu, Jun 30, 2016 at 2:37 PM, Laszlo Ersek <lersek at redhat.com> wrote:
>> In edk2, there are several drivers that associate HII forms (and
>> corresponding config access protocol instances) with each individual
>> network device. (In this context, "network device" means the EFI handle on
>> which the SNP protocol is installed, and on which the device path ending
>> with the MAC() node is installed also.) Such edk2 drivers are, for
>> example: Ip4Dxe, HttpBootDxe, VlanConfigDxe.
>>
>> In UEFI, any given handle can carry at most one instance of a specific
>> protocol (see it e.g. in the specification of the
>> InstallProtocolInterface() boot service). This implies that the class of
>> drivers mentioned above can't install their EFI_HII_CONFIG_ACCESS_PROTOCOL
>> instances on the SNP handle directly -- they would conflict with each
>> other. Accordingly, each of those edk2 drivers creates a "private" child
>> handle under the SNP handle, and installs its config access protocol (and
>> corresponding HII package list) on its child handle.
>>
>> The device path for the child handle is traditionally derived by appending
>> a Hardware Vendor Device Path node after the MAC() node. The VenHw() nodes
>> in question consist of a GUID (by definition), and no trailing data (by
>> choice). The purpose of these VenHw() nodes is only that all the child
>> nodes can be uniquely identified by device path.
>>
>> At the moment iPXE does not follow this pattern. It doesn't run into a
>> conflict when it installs its EFI_HII_CONFIG_ACCESS_PROTOCOL directly on
>> the SNP handle, but that's only because iPXE is the sole driver not
>> following the pattern. This behavior seems risky (one might call it a
>> "latent bug"); better align iPXE with the edk2 custom.
>>
>> Cc: Michael Brown <mcb30 at ipxe.org>
>> Cc: Gary Lin <glin at suse.com>
>> Cc: Ladi Prosek <lprosek at redhat.com>
>> Ref: http://thread.gmane.org/gmane.comp.bios.edk2.devel/13494
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>> src/include/ipxe/efi/efi_snp.h | 4 +++
>> src/interface/efi/efi_snp_hii.c | 76 ++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/include/ipxe/efi/efi_snp.h b/src/include/ipxe/efi/efi_snp.h
>> index 4c5461ec4d01..a9f67cfcbbb0 100644
>> --- a/src/include/ipxe/efi/efi_snp.h
>> +++ b/src/include/ipxe/efi/efi_snp.h
>> @@ -57,6 +57,10 @@ struct efi_snp_device {
>> EFI_HII_CONFIG_ACCESS_PROTOCOL hii;
>> /** HII package list */
>> EFI_HII_PACKAGE_LIST_HEADER *package_list;
>> + /** EFI child handle for HII association */
>> + EFI_HANDLE hii_child_handle;
>> + /** Device path of HII child handle */
>> + EFI_DEVICE_PATH_PROTOCOL *hii_child_path;
>> /** HII handle */
>> EFI_HII_HANDLE hii_handle;
>> /** Device name */
>> diff --git a/src/interface/efi/efi_snp_hii.c b/src/interface/efi/efi_snp_hii.c
>> index 1e87ea15a46e..9f76d61205fb 100644
>> --- a/src/interface/efi/efi_snp_hii.c
>> +++ b/src/interface/efi/efi_snp_hii.c
>> @@ -63,6 +63,7 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
>> #include <ipxe/efi/efi_hii.h>
>> #include <ipxe/efi/efi_snp.h>
>> #include <ipxe/efi/efi_strings.h>
>> +#include <ipxe/efi/efi_utils.h>
>> #include <config/branding.h>
>>
>> /** EFI platform setup formset GUID */
>> @@ -655,6 +656,9 @@ int efi_snp_hii_install ( struct efi_snp_device *snpdev ) {
>> EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
>> int efirc;
>> int rc;
>> + size_t path_prefix_len;
>> + VENDOR_DEVICE_PATH *vendor_path;
>> + EFI_DEVICE_PATH_PROTOCOL *path_end;
>>
>> /* Do nothing if HII database protocol is not supported */
>> if ( ! efihii ) {
>> @@ -674,9 +678,47 @@ int efi_snp_hii_install ( struct efi_snp_device *snpdev ) {
>> goto err_build_package_list;
>> }
>>
>> + /* Create device path and child handle for HII association */
>> + path_prefix_len = efi_devpath_len ( snpdev->path );
>> + snpdev->hii_child_path = zalloc ( path_prefix_len +
>> + sizeof ( *vendor_path ) +
>> + sizeof ( *path_end ) );
>> + if ( ! snpdev->hii_child_path ) {
>> + DBGC ( snpdev,
>> + "SNPDEV %p could not allocate HII child device path\n",
>> + snpdev );
>> + rc = -ENOMEM;
>> + goto err_alloc_child_path;
>> + }
>> +
>> + memcpy ( snpdev->hii_child_path, snpdev->path, path_prefix_len );
>> +
>> + vendor_path = ( ( ( void * ) snpdev->hii_child_path ) +
>> + path_prefix_len );
>> + vendor_path->Header.Type = HARDWARE_DEVICE_PATH;
>> + vendor_path->Header.SubType = HW_VENDOR_DP;
>> + vendor_path->Header.Length[0] = sizeof ( *vendor_path );
>> + efi_snp_hii_random_guid ( &vendor_path->Guid );
>> +
>> + path_end = ( void * ) ( vendor_path + 1 );
>> + path_end->Type = END_DEVICE_PATH_TYPE;
>> + path_end->SubType = END_ENTIRE_DEVICE_PATH_SUBTYPE;
>> + path_end->Length[0] = sizeof ( *path_end );
>
> This is the 4th occurrence of this path terminating pattern in the
> code base. Would it make sense to add a macro?
Macro or inline helper function, maybe. Could be implemented as a
separate series that unifies all the instances.
I also tought of looking up EFI_DEVICE_PATH_UTILITIES_PROTOCOL, and
using its member functions for devpath manipulation more generally:
- GetDevicePathSize
- DuplicateDevicePath
- AppendDevicePath
- AppendDeviceNode
- CreateDeviceNode
Some of the functions from "efi_utils.h" could be replaced with member
functions listed above, I think.
Anyway I felt this was a separate topic :) I wasn't sure how much
Michael wanted to rely on the platform firmware for things like this.
>
>> +
>> + if ( ( efirc = bs->InstallMultipleProtocolInterfaces (
>> + &snpdev->hii_child_handle,
>> + &efi_device_path_protocol_guid, snpdev->hii_child_path,
>> + NULL ) ) != 0 ) {
>> + rc = -EEFI ( efirc );
>> + DBGC ( snpdev,
>> + "SNPDEV %p could not create HII child handle: %s\n",
>> + snpdev, strerror ( rc ) );
>> + goto err_hii_child_handle;
>> + }
>> +
>> /* Add HII packages */
>> if ( ( efirc = efihii->NewPackageList ( efihii, snpdev->package_list,
>> - snpdev->handle,
>> + snpdev->hii_child_handle,
>> &snpdev->hii_handle ) ) != 0 ) {
>> rc = -EEFI ( efirc );
>> DBGC ( snpdev, "SNPDEV %p could not add HII packages: %s\n",
>> @@ -686,7 +728,7 @@ int efi_snp_hii_install ( struct efi_snp_device *snpdev ) {
>>
>> /* Install HII protocol */
>> if ( ( efirc = bs->InstallMultipleProtocolInterfaces (
>> - &snpdev->handle,
>> + &snpdev->hii_child_handle,
>> &efi_hii_config_access_protocol_guid, &snpdev->hii,
>> NULL ) ) != 0 ) {
>> rc = -EEFI ( efirc );
>> @@ -695,15 +737,34 @@ int efi_snp_hii_install ( struct efi_snp_device *snpdev ) {
>> goto err_install_protocol;
>> }
>>
>> + /* Add as child of handle with SNP instance */
>> + if ( ( rc = efi_child_add ( snpdev->handle,
>> + snpdev->hii_child_handle ) ) != 0 ) {
>> + DBGC ( snpdev,
>> + "SNPDEV %p could not adopt HII child handle: %s\n",
>> + snpdev, strerror ( rc ) );
>> + goto err_efi_child_add;
>> + }
>> +
>> return 0;
>>
>> + efi_child_del ( snpdev->handle, snpdev->hii_child_handle );
>> + err_efi_child_add:
>> bs->UninstallMultipleProtocolInterfaces (
>> - snpdev->handle,
>> + snpdev->hii_child_handle,
>> &efi_hii_config_access_protocol_guid, &snpdev->hii,
>> NULL );
>> err_install_protocol:
>> efihii->RemovePackageList ( efihii, snpdev->hii_handle );
>> err_new_package_list:
>> + bs->UninstallMultipleProtocolInterfaces (
>> + snpdev->hii_child_handle,
>> + &efi_device_path_protocol_guid, snpdev->hii_child_path,
>> + NULL );
>> + err_hii_child_handle:
>> + free ( snpdev->hii_child_path );
>> + snpdev->hii_child_path = NULL;
>> + err_alloc_child_path:
>> free ( snpdev->package_list );
>> snpdev->package_list = NULL;
>> err_build_package_list:
>> @@ -724,11 +785,18 @@ void efi_snp_hii_uninstall ( struct efi_snp_device *snpdev ) {
>> return;
>>
>> /* Uninstall protocols and remove package list */
>> + efi_child_del ( snpdev->handle, snpdev->hii_child_handle );
>> bs->UninstallMultipleProtocolInterfaces (
>> - snpdev->handle,
>> + snpdev->hii_child_handle,
>> &efi_hii_config_access_protocol_guid, &snpdev->hii,
>> NULL );
>> efihii->RemovePackageList ( efihii, snpdev->hii_handle );
>> + bs->UninstallMultipleProtocolInterfaces (
>> + snpdev->hii_child_handle,
>> + &efi_device_path_protocol_guid, snpdev->hii_child_path,
>> + NULL );
>> + free ( snpdev->hii_child_path );
>> + snpdev->hii_child_path = NULL;
>> free ( snpdev->package_list );
>> snpdev->package_list = NULL;
>> }
>> --
>> 1.8.3.1
>>
>
> Looks legit!
>
> Reviewed-by: Ladi Prosek <lprosek at redhat.com>
>
Thanks!
Laszlo
More information about the ipxe-devel
mailing list