Re: [edk2-staging][PATCH] RedfishClientPkg: Add mechanism to reboot system if config is changed

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Nickle Wang <nicklew@nvidia.com>
> Sent: Tuesday, February 14, 2023 8:43 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
> Subject: [edk2-staging][PATCH] RedfishClientPkg: Add mechanism to reboot
> system if config is changed
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> When system configuration is updated from RESTful interface, we need a
> system reboot so that the changes can be applied. Introduce PCD
> "PcdSystemRebootRequired" to RedfishClientPkg. RedfishFeatureUtility
> library will enable this flag when system config is updated.
> RedfishFeatureCore driver will check this flag and perform cold reboot after
> all Redfish operations are finished. PCD "PcdSystemRebootTimeout"
> is used to specify how many second BIOS will wait before reboot system.
> 
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Nick Ramirez <nramirez@nvidia.com>
> ---
>  .../Library/RedfishFeatureUtilityLib.h        |  3 +
>  .../RedfishFeatureUtilityLib.c                | 65 ++++++++++++++++---
>  .../RedfishFeatureUtilityLib.inf              |  2 +
>  RedfishClientPkg/RedfishClientPkg.dec         |  8 ++-
>  .../RedfishFeatureCoreDxe.c                   | 18 +++++
>  .../RedfishFeatureCoreDxe.h                   |  3 +
>  .../RedfishFeatureCoreDxe.inf                 |  5 ++
>  7 files changed, 93 insertions(+), 11 deletions(-)
> 
> diff --git a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> index 1325976d8c..bb5dc4f4ac 100644
> --- a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> +++ b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> @@ -2,6 +2,7 @@
>    This file defines the Redfish Feature Utility Library interface.
> 
>    (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -14,6 +15,8 @@
>  #include <Protocol/EdkIIRedfishPlatformConfig.h>
>  #include <RedfishJsonStructure/RedfishCsCommon.h>
> 
> +#define REDFISH_ENABLE_SYSTEM_REBOOT()
> +PcdSetBoolS(PcdSystemRebootRequired, TRUE)
> +
>  //
>  // Definition of REDFISH_FEATURE_ARRAY_TYPE_CONFIG_LANG
>  //
> diff --git
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .c
> b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .c
> index bfd6fff2a7..9883a4d919 100644
> ---
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .c
> +++ b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUt
> +++ ilityLib.c
> @@ -2,7 +2,7 @@
>    Redfish feature utility library implementation
> 
>    (C) Copyright 2020-2022 Hewlett Packard Enterprise Development LP<BR>
> -  Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -324,7 +324,12 @@ ApplyFeatureSettingsStringType (
>        RedfishValue.Value.Buffer = FeatureValue;
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s to %s failed: %r\n",
> __FUNCTION__, ConfigureLang, FeatureValue, Status));
>        }
>      } else {
> @@ -385,7 +390,12 @@ ApplyFeatureSettingsNumericType (
>        RedfishValue.Value.Integer = (INT64)FeatureValue;
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s to 0x%x failed: %r\n",
> __FUNCTION__, ConfigureLang, FeatureValue, Status));
>        }
>      } else {
> @@ -446,7 +456,12 @@ ApplyFeatureSettingsBooleanType (
>        RedfishValue.Value.Boolean = FeatureValue;
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s to %a failed: %r\n",
> __FUNCTION__, ConfigureLang, (FeatureValue ? "True" : "False"), Status));
>        }
>      } else {
> @@ -561,7 +576,12 @@ ApplyFeatureSettingsVagueType (
>            FreePool (RedfishValue.Value.Buffer);
>            RedfishValue.Value.Buffer = CurrentVagueValuePtr->Value-
> >DataValue.CharPtr;
>            Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureKeyLang, RedfishValue);
> -          if (EFI_ERROR (Status)) {
> +          if (!EFI_ERROR (Status)) {
> +            //
> +            // Configuration changed. Enable system reboot flag.
> +            //
> +            REDFISH_ENABLE_SYSTEM_REBOOT();
> +          } else {
>              DEBUG ((DEBUG_ERROR, "%a, apply %a to %a failed: %r\n",
> __FUNCTION__, ConfigureKeyLang, CurrentVagueValuePtr->Value-
> >DataValue.CharPtr, Status));
>            }
>          } else {
> @@ -585,7 +605,12 @@ ApplyFeatureSettingsVagueType (
> 
>            RedfishValue.Value.Boolean = (BOOLEAN)*CurrentVagueValuePtr-
> >Value->DataValue.BoolPtr;
>            Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureKeyLang, RedfishValue);
> -          if (EFI_ERROR (Status)) {
> +          if (!EFI_ERROR (Status)) {
> +            //
> +            // Configuration changed. Enable system reboot flag.
> +            //
> +            REDFISH_ENABLE_SYSTEM_REBOOT();
> +          } else {
>              DEBUG ((DEBUG_ERROR, "%a, apply %s to %a failed: %r\n",
> __FUNCTION__, ConfigureKeyLang, (*CurrentVagueValuePtr->Value-
> >DataValue.BoolPtr ? "True" : "False"), Status));
>            }
>          } else {
> @@ -603,7 +628,12 @@ ApplyFeatureSettingsVagueType (
> 
>            RedfishValue.Value.Integer = (INT64)*CurrentVagueValuePtr->Value-
> >DataValue.Int64Ptr;
>            Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureKeyLang, RedfishValue);
> -          if (EFI_ERROR (Status)) {
> +          if (!EFI_ERROR (Status)) {
> +            //
> +            // Configuration changed. Enable system reboot flag.
> +            //
> +            REDFISH_ENABLE_SYSTEM_REBOOT();
> +          } else {
>              DEBUG ((DEBUG_ERROR, "%a, apply %s to 0x%x failed: %r\n",
> __FUNCTION__, ConfigureKeyLang, *CurrentVagueValuePtr->Value-
> >DataValue.Int64Ptr, Status));
>            }
>          } else {
> @@ -775,7 +805,12 @@ ApplyFeatureSettingsStringArrayType (
>        ASSERT (Index <= RedfishValue.ArrayCount);
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n",
> __FUNCTION__, ConfigureLang, Status));
>        }
>      } else {
> @@ -869,7 +904,12 @@ ApplyFeatureSettingsNumericArrayType (
>        ASSERT (Index <= RedfishValue.ArrayCount);
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n",
> __FUNCTION__, ConfigureLang, Status));
>        }
>      } else {
> @@ -963,7 +1003,12 @@ ApplyFeatureSettingsBooleanArrayType (
>        ASSERT (Index <= RedfishValue.ArrayCount);
> 
>        Status = RedfishPlatformConfigSetValue (Schema, Version,
> ConfigureLang, RedfishValue);
> -      if (EFI_ERROR (Status)) {
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT();
> +      } else {
>          DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n",
> __FUNCTION__, ConfigureLang, Status));
>        }
>      } else {
> diff --git
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .inf
> b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .inf
> index 84f338e680..d556990b63 100644
> ---
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib
> .inf
> +++ b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUt
> +++ ilityLib.inf
> @@ -1,6 +1,7 @@
>  ## @file
>  #
>  #  (C) Copyright 2020-2022 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -49,6 +50,7 @@
>    gEdkIIRedfishConfigLangMapProtocolGuid  ## CONSUMED ##
> 
>  [Pcd]
> +  gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootRequired
> 
>  [Guids]
> 
> diff --git a/RedfishClientPkg/RedfishClientPkg.dec
> b/RedfishClientPkg/RedfishClientPkg.dec
> index d3c97ecf68..634456e5d5 100644
> --- a/RedfishClientPkg/RedfishClientPkg.dec
> +++ b/RedfishClientPkg/RedfishClientPkg.dec
> @@ -2,7 +2,7 @@
>  # Redfish Client Package
>  #
>  # (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP<BR>
> -# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
> +# Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  ## @@ -65,3 +65,9 @@
> 
> gEfiRedfishClientPkgTokenSpaceGuid.PcdEdkIIRedfishFeatureDriverStartupE
> ventGuid|{0xB3, 0x8F, 0xE8, 0x7C, 0xD7, 0x4B, 0x79, 0x46, 0x87, 0xA8, 0xA8,
> 0xD8, 0xDE, 0xE5, 0x0D, 0x2B}|VOID*|0x10000003
>    ## Default Redfish version string
> 
> gEfiRedfishClientPkgTokenSpaceGuid.PcdDefaultRedfishVersion|L"v1"|VOID
> *|0x10000004
> +  ## The number of seconds that the firmware will wait before system
> + reboot
> +
> +
> gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootTimeout|5|UINT16|
> 0x2
> + 0000002
Can we name PcdSystemRebootTimeout as PcdRedfishSystemRebootTimeout? So it won't be consider as a generic PCD for any use case. This is only for Redfish. 

> +
> +[PcdsDynamicEx]
> +  ## The flag used to indicate that system reboot is required due to
> +system configuration change
> +
> +gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootRequired|FALSE|B
> OOLEA
> +N|0x20000001
Same as above, we can name it as PcdRedfishSystemRebootRequired

I have no problem with other changes.
Thanks
Abner

> diff --git
> a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> index 3414f0c942..b2b1307b6c 100644
> --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.c
> @@ -3,6 +3,7 @@
>    for EDK2 Redfish Feature driver registration.
> 
>    (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -230,8 +231,10 @@ RedfishFeatureDriverStartup(
>    )
>  {
>    REDFISH_FEATURE_STARTUP_CONTEXT *StartupContext;
> +  UINT16                          RebootTimeout;
> 
>    StartupContext = (REDFISH_FEATURE_STARTUP_CONTEXT *)Context;
> +  RebootTimeout = PcdGet16 (PcdSystemRebootTimeout);
>    //
>    // Invoke EDK2 Redfish feature driver callback to start up
>    // the Redfish operations.
> @@ -249,6 +252,11 @@ RedfishFeatureDriverStartup(
>      return;
>    }
> 
> +  //
> +  // Reset PcdSystemRebootRequired flag  //  PcdSetBoolS
> + (PcdSystemRebootRequired, FALSE);
> +
>    //
>    // Signal event before doing provisioning
>    //
> @@ -263,6 +271,16 @@ RedfishFeatureDriverStartup(
>    // Signal event after provisioning finished
>    //
>    SignalAfterProvisioningEvent ();
> +
> +  //
> +  // If system configuration is changed, reboot system.
> +  //
> +  if (PcdGetBool (PcdSystemRebootRequired)) {
> +    Print (L"System configuration is changed from RESTful interface. Reboot
> system in %d seconds...\n", RebootTimeout);
> +    gBS->Stall (RebootTimeout * 1000000U);
> +    gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> +    CpuDeadLoop ();
> +  }
>  }
> 
>  /**
> diff --git
> a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> index 84b5e456d1..af274c01ca 100644
> --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.h
> @@ -2,6 +2,7 @@
>    Definitions of RedfishFeatureCoreDxe
> 
>    (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -17,6 +18,8 @@
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>  #include
> <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
>  #include <Library/RedfishEventLib.h>
>  #include <Library/RedfishFeatureUtilityLib.h>
> 
> diff --git
> a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.inf
> b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.inf
> index ddcf991006..6cf35694fb 100644
> --- a/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.inf
> +++ b/RedfishClientPkg/RedfishFeatureCoreDxe/RedfishFeatureCoreDxe.inf
> @@ -4,6 +4,8 @@
>  #  drivers for the registration.
>  #
>  #  (C) Copyright 2021-2022 Hewlett Packard Enterprise Development LP<BR>
> +#  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved.
> +#
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent  #  ## @@ -40,6 +42,7 @@
>    RedfishFeatureUtilityLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiRuntimeServicesTableLib
>    UefiLib
> 
>  [Protocols]
> @@ -47,6 +50,8 @@
> 
>  [Pcd]
> 
> gEfiRedfishClientPkgTokenSpaceGuid.PcdEdkIIRedfishFeatureDriverStartupE
> ventGuid
> +  gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootRequired
> +  gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootTimeout
> 
>  [Depex]
>    TRUE
> --
> 2.39.1.windows.1