[ipxe-devel] [PATCH] [efi] install the HII config access protocol on a child of the SNP handle

[Laszlo Ersek]

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 );
+
+	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