Thanks for your review, Abner. Version 2 patch is sent. I also fixed uncrustify issue together.
Regards, Nickle -----Original Message----- From: Chang, Abner <abner.ch...@amd.com> Sent: Thursday, February 16, 2023 8:56 PM To: Nickle Wang <nick...@nvidia.com>; devel@edk2.groups.io Cc: Igor Kulchytskyy <ig...@ami.com>; Nick Ramirez <nrami...@nvidia.com> Subject: RE: [edk2-staging][PATCH] RedfishClientPkg: Add mechanism to reboot system if config is changed External email: Use caution opening links or attachments [AMD Official Use Only - General] > -----Original Message----- > From: Nickle Wang <nick...@nvidia.com> > Sent: Tuesday, February 14, 2023 8:43 PM > To: devel@edk2.groups.io > Cc: Chang, Abner <abner.ch...@amd.com>; Igor Kulchytskyy > <ig...@ami.com>; Nick Ramirez <nrami...@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 <nick...@nvidia.com> > Cc: Abner Chang <abner.ch...@amd.com> > Cc: Igor Kulchytskyy <ig...@ami.com> > Cc: Nick Ramirez <nrami...@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/RedfishFeatureUtil > ityLib > .c > b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtil > ityLib > .c > index bfd6fff2a7..9883a4d919 100644 > --- > a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtil > ityLib > .c > +++ b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeature > +++ Ut > +++ 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/RedfishFeatureUtil > ityLib > .inf > b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtil > ityLib > .inf > index 84f338e680..d556990b63 100644 > --- > a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtil > ityLib > .inf > +++ b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeature > +++ Ut > +++ 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.PcdEdkIIRedfishFeatureDriverStartup > E > ventGuid|{0xB3, 0x8F, 0xE8, 0x7C, 0xD7, 0x4B, 0x79, 0x46, 0x87, 0xA8, > ventGuid|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.PcdEdkIIRedfishFeatureDriverStartup > E > ventGuid > + gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootRequired > + gEfiRedfishClientPkgTokenSpaceGuid.PcdSystemRebootTimeout > > [Depex] > TRUE > -- > 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100589): https://edk2.groups.io/g/devel/message/100589 Mute This Topic: https://groups.io/mt/96958702/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-