[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