On Thu, Jun 30, 2016 at 2:37 PM, Laszlo Ersek <ler...@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 <mc...@ipxe.org>
> Cc: Gary Lin <g...@suse.com>
> Cc: Ladi Prosek <lpro...@redhat.com>
> Ref: http://thread.gmane.org/gmane.comp.bios.edk2.devel/13494
> Signed-off-by: Laszlo Ersek <ler...@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?
> +
> + 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 <lpro...@redhat.com>
_______________________________________________
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel
