Re: [PATCH v1 1/1] FmpDevicePkg: FmpDeviceLib interface change for Driver Unload support

["John Rahn" <jrahn@nvidia.com>]

Driver unload in the sample FmpDxe is currently configured to directly call UninstallFmpInstance  (UNLOAD_IMAGE) with the ImageHandle and EFI_UNSUPPORTED is returned because the driver binding provided by FmpDeviceLib is managing the instance on the DeviceHandle, not the ImageHandle.
The FmpDeviceLib supporting the UEFI Driver Binding model is never notified of the driver unload and the Fmp Instance(s) are not uninstalled and the driver is not unloaded.
The sample FmpDxe does not provide a interface for the required driver unload notification to be passed to the FmpDeviceLib implementation.
Attempting to have the UninstallFmpInstance in the sample FmpDxe check Fmp instances for drivers managing the device and calling DriverStop with the DeviceHandle didn't align with the standard driver implementation model and was not as straightforward of an approach as exposing a driver unload interface for FmpDeviceLib use for the driver unload notification, so that the FmpDeviceLib can properly stop on it's managed devices and clean up any internal context/private data.

A FmpDeviceLib following the driver binding model and implementing the proposed interface can unload the Fmp instance(s) being managed and clean up any allocations before exit. Any remaining allocations in the FmpDxe are properly dealt with in the UninstallFmpInstance path.

I was able to successfully build a functional FmpDeviceLib that will properly unload the installed Fmp instance(s) using the proposed interface change.

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Tuesday, April 20, 2021 3:37 PM
To: John Rahn <JRahn@nvidia.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>
Subject: RE: [PATCH v1 1/1] FmpDevicePkg: FmpDeviceLib interface change for Driver Unload support

External email: Use caution opening links or attachments


Hi John,

The FmpDeviceLib provides the RegisterFmpInstaller() and RegisterFmpUninstaller() APIs for UEFI Driver Model drivers to manage the FMP contexts.

Why does the Unload need to be extended into the FmpDeviceLib when these APIs are used?  I would think that a UEFI Driver Model Driver that supports FMP could call Stop() on all instances when an Unload() request is received and the Stop() services would use the FMP Uninstaller path to clean up the FMP instances.  If all Stops() succeed, then the UEFI Driver that is producing the FMP instances can be unloaded,


/**
  Callback function that installs a Firmware Management Protocol instance onto
  a handle.

  @param[in]  Handle  The device handle to install a Firmware Management
                      Protocol instance.

  @retval  EFI_SUCCESS            A Firmware Management Protocol instance was
                                  installed onto Handle.
  @retval  EFI_INVALID_PARAMETER  Handle is invalid
  @retval  other                  A Firmware Management Protocol instance could
                                  not be installed onto Handle.

**/
typedef
EFI_STATUS
(EFIAPI *FMP_DEVICE_LIB_REGISTER_FMP_INSTALLER)(
  IN  EFI_HANDLE      Handle
  );

/**
  Callback function that uninstalls a Firmware Management Protocol instance from
  a handle.

  @param[in]  Handle  The device handle to uninstall a Firmware Management
                      Protocol instance.

  @retval  EFI_SUCCESS            A Firmware Management Protocol instance was
                                  uninstalled from Handle.
  @retval  EFI_INVALID_PARAMETER  Handle is invalid
  @retval  other                  A Firmware Management Protocol instance could
                                  not be uninstalled from Handle.

**/
typedef
EFI_STATUS
(EFIAPI *FMP_DEVICE_LIB_REGISTER_FMP_UNINSTALLER)(
  IN  EFI_HANDLE      Handle
  );

/**
  Provide a function to install the Firmware Management Protocol instance onto a
  device handle when the device is managed by a driver that follows the UEFI
  Driver Model.  If the device is not managed by a driver that follows the UEFI
  Driver Model, then EFI_UNSUPPORTED is returned.

  @param[in] FmpInstaller  Function that installs the Firmware Management
                           Protocol.

  @retval EFI_SUCCESS      The device is managed by a driver that follows the
                           UEFI Driver Model.  FmpInstaller must be called on
                           each Driver Binding Start().
  @retval EFI_UNSUPPORTED  The device is not managed by a driver that follows
                           the UEFI Driver Model.
  @retval other            The Firmware Management Protocol for this firmware
                           device is not installed.  The firmware device is
                           still locked using FmpDeviceLock().

**/
EFI_STATUS
EFIAPI
RegisterFmpInstaller (
  IN FMP_DEVICE_LIB_REGISTER_FMP_INSTALLER  FmpInstaller
  );

/**
  Provide a function to uninstall the Firmware Management Protocol instance from a
  device handle when the device is managed by a driver that follows the UEFI
  Driver Model.  If the device is not managed by a driver that follows the UEFI
  Driver Model, then EFI_UNSUPPORTED is returned.

  @param[in] FmpUninstaller  Function that installs the Firmware Management
                             Protocol.

  @retval EFI_SUCCESS      The device is managed by a driver that follows the
                           UEFI Driver Model.  FmpUninstaller must be called on
                           each Driver Binding Stop().
  @retval EFI_UNSUPPORTED  The device is not managed by a driver that follows
                           the UEFI Driver Model.
  @retval other            The Firmware Management Protocol for this firmware
                           device is not installed.  The firmware device is
                           still locked using FmpDeviceLock().

**/
EFI_STATUS
EFIAPI
RegisterFmpUninstaller (
  IN FMP_DEVICE_LIB_REGISTER_FMP_UNINSTALLER  FmpUninstaller
  );

Best regards,

Mike

> -----Original Message-----
> From: John Rahn <jrahn@nvidia.com>
> Sent: Tuesday, April 20, 2021 2:36 PM
> To: devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; 
> Xu, Wei6 <wei6.xu@intel.com>
> Subject: [PATCH v1 1/1] FmpDevicePkg: FmpDeviceLib interface change 
> for Driver Unload support
>
>    https://bugzilla.tianocore.org/show_bug.cgi?id=3342
>      FmpDeviceLib interface for Driver Unload is missing
>    Add FmpDeviceLibUnloadImage function declaration and NULL sample.
>    Add FmpDxeUnloadImage function.
>    Replace UNLOAD_IMAGE function in FmpDxe sample with FmpDxeUnloadImage.
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Wei6 Xu <wei6.xu@intel.com>
> Signed-off-by: John Rahn <jrahn@nvidia.com>
> ---
>  FmpDevicePkg/FmpDxe/FmpDxe.inf                       |  2 +-
>  FmpDevicePkg/Include/Library/FmpDeviceLib.h          | 21 +++++++++++++++++
>  FmpDevicePkg/FmpDxe/FmpDxe.c                         | 21 +++++++++++++++++
>  FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c | 24 
> ++++++++++++++++++++
>  4 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.inf 
> b/FmpDevicePkg/FmpDxe/FmpDxe.inf index eeb904a09148..2de9e3e4f2ad 
> 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.inf
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.inf
> @@ -17,7 +17,7 @@ [Defines]
>    MODULE_TYPE                    = DXE_DRIVER
>    VERSION_STRING                 = 1.0
>    ENTRY_POINT                    = FmpDxeEntryPoint
> -  UNLOAD_IMAGE                   = UninstallFmpInstance
> +  UNLOAD_IMAGE                   = BZ3342FmpDxeUnloadImage
>
>  #
>  # The following information is for reference only and not required by the build tools.
> diff --git a/FmpDevicePkg/Include/Library/FmpDeviceLib.h 
> b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> index 6abd99fa1f47..d9da5b940886 100644
> --- a/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> +++ b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> @@ -104,6 +104,27 @@ RegisterFmpUninstaller (
>    IN FMP_DEVICE_LIB_REGISTER_FMP_UNINSTALLER  FmpUninstaller
>    );
>
> +/**
> +  Function that unloads a Firmware Management Device Library based 
> +driver instance when
> +  the FmpDeviceLib supports the driver binding model.
> +  If the FmpDeviceLib does not support the UEFI driver model, the 
> +FmpDeviceLib Unload Image
> +  should return EFI_UNSUPPORTED.
> +
> +  @param[in]  ImageHandle  The driver handle managing the Firmware Management Protocol instance to unload.
> +
> +  @retval EFI_SUCCESS               Driver image was removed successfully.
> +  @retval EFI_UNSUPPORTED           The device is not managed by a driver that follows
> +                                    the UEFI Driver Model.
> +  @retval EFI_INVALID_PARAMETER     ImageHandle is NULL.
> +  @retval EFI_INVALID_PARAMETER     ImageHandle does not match driver image handle.
> +  @retval other                     Driver image was not removed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BZ3342FmpDeviceLibUnloadImage (
> +  IN EFI_HANDLE  ImageHandle
> +  );
> +
>  /**
>    Set the device context for the FmpDeviceLib services when the device is
>    managed by a driver that follows the UEFI Driver Model.  If the 
> device is not diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c 
> b/FmpDevicePkg/FmpDxe/FmpDxe.c index 6b0675ea38f8..6680a381b8fe 100644
> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> @@ -1813,6 +1813,27 @@ UninstallFmpInstance (
>    return EFI_SUCCESS;
>  }
>
> +/**
> +  Unloads the application and its installed protocol.
> +
> +  @param ImageHandle       Handle that identifies the image to be unloaded.
> +
> +  @retval EFI_SUCCESS      The image has been unloaded.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +BZ3342FmpDxeUnloadImage (
> +  IN EFI_HANDLE       ImageHandle
> +  )
> +{
> +  if (mFmpSingleInstance) {
> +    return UninstallFmpInstance (ImageHandle);
> +  } else {
> +    return BZ3342FmpDeviceLibUnloadImage (ImageHandle);
> +  }
> +}
> +
>  /**
>    Unloads the application and its installed protocol.
>
> diff --git a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c 
> b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> index f4f57b29bdb1..4d2cbc64b7c6 100644
> --- a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> +++ b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> @@ -41,6 +41,30 @@ RegisterFmpInstaller (
>    return EFI_UNSUPPORTED;
>  }
>
> +/**
> +  Function that unloads a Firmware Management Device Library based 
> +driver instance when
> +  the FmpDeviceLib supports the driver binding model.
> +  If the FmpDeviceLib does not support the UEFI driver model, the 
> +FmpDeviceLib Unload Image
> +  should return EFI_UNSUPPORTED.
> +
> +  @param[in]  ImageHandle  The driver handle managing the Firmware Management Protocol instance to unload.
> +
> +  @retval EFI_SUCCESS               Driver image was removed successfully.
> +  @retval EFI_UNSUPPORTED           The device is not managed by a driver that follows
> +                                      the UEFI Driver Model.
> +  @retval EFI_INVALID_PARAMETER     ImageHandle is NULL.
> +  @retval EFI_INVALID_PARAMETER     ImageHandle does not match driver image handle.
> +  @retval other                     Driver image was not removed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +BZ3342FmpDeviceLibUnloadImage (
> +  IN EFI_HANDLE  ImageHandle
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
>  /**
>    Provide a function to uninstall the Firmware Management Protocol instance from a
>    device handle when the device is managed by a driver that follows 
> the UEFI
> --
> 2.17.1