On 07/01/16 09:28, Ladi Prosek wrote: > 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?
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 <lpro...@redhat.com> > Thanks! Laszlo _______________________________________________ ipxe-devel mailing list ipxe-devel@lists.ipxe.org https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel