Re: [edk2-devel] Question about the boundary and difference between System Firmware and UEFI CXL drivers

[Yuquan Wang]

Addition: [the link of  CXL Memory Device SW Guide]
https://cdrdv2-public.intel.com/643805/643805_CXL%20Memory%20Device%20SW%20Guide_Rev1p0.pdf


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112093): https://edk2.groups.io/g/devel/message/112093
Mute This Topic: https://groups.io/mt/103008846/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Question about the boundary and difference between System Firmware and UEFI CXL drivers

[Yuquan Wang]

Hi, folks
 
CXL Memory Device SW Guide [1] rev1.0 2.4 provides little description about the 
difference between System Firmware and
UEFI CXL drivers. IIRC, the UEFI drivers are part of system firmware, so I am 
confused about the boundary on them.
 
I greatly appreciate insight/help in this regard!

Many thanks
Yuquan



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112092): https://edk2.groups.io/g/devel/message/112092
Mute This Topic: https://groups.io/mt/103008846/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] IpmiFeaturePkg/GenericIpmi: Sync change from SMM

[Huang, Li-Xia]

Sync change from SMM to StandaloneMm GenericIpmi driver.
Update SmmIpmbInterface and SmmSsifInterface Lib to support
MM_STANDALONE. And Format code with uncrustify.

Cc: Abner Chang 
Cc: Nate DeSimone 

Signed-off-by: Lixia Huang 
---
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
| 315 +---
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/BmcInterfaceCommonAccess/IpmbInterfaceLib/SmmIpmbInterfaceLib.c
   |   4 +-
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/BmcInterfaceCommonAccess/SsifInterfaceLib/SmmSsifInterfaceLib.c
   |  64 ++--
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.inf
  |  10 +
 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/IpmiFeature.dsc  
 |   2 +-
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/BmcInterfaceCommonAccess/IpmbInterfaceLib/SmmIpmbInterfaceLib.inf
 |   4 +-
 
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/BmcInterfaceCommonAccess/SsifInterfaceLib/SmmSsifInterfaceLib.inf
 |   4 +-
 7 files changed, 326 insertions(+), 77 deletions(-)

diff --git 
a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
 
b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
index d808e2517c99..1b9841e4b745 100644
--- 
a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
+++ 
b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
@@ -19,17 +19,157 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "IpmiHooks.h"
 #include "IpmiBmcCommon.h"
 #include "IpmiBmc.h"
 
-IPMI_BMC_INSTANCE_DATA *mIpmiInstance;
-EFI_HANDLE mHandle;
+IPMI_BMC_INSTANCE_DATA  *mIpmiInstance;
+EFI_HANDLE  mIpmiTransportHandle;
+EFI_HANDLE  mIpmiTransport2Handle;
 
 /**
+
+Routine Description:
+  Initialize the API and parameters for IPMI Transport2 Instance
+
+Arguments:
+  IpmiInstance- Pointer to IPMI Instance.
+
+Returns:
+  VOID- Nothing.
+
+**/
+VOID
+InitIpmiTransport2 (
+  IN  IPMI_BMC_INSTANCE_DATA  *IpmiInstance
+  )
+{
+  IpmiInstance->IpmiTransport2.InterfaceType   = FixedPcdGet8 
(PcdDefaultSystemInterface);
+  IpmiInstance->IpmiTransport2.IpmiTransport2BmcStatus = BmcStatusOk;
+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2  = IpmiSendCommand2;
+  IpmiInstance->IpmiTransport2.IpmiSubmitCommand2Ex= IpmiSendCommand2Ex;
+
+  if (FixedPcdGet8 (PcdBtInterfaceSupport) == 1) {
+InitBtInterfaceData (>IpmiTransport2);
+  }
+
+  if (FixedPcdGet8 (PcdSsifInterfaceSupport) == 1) {
+InitSsifInterfaceData (>IpmiTransport2);
+  }
+
+  if (FixedPcdGet8 (PcdIpmbInterfaceSupport) == 1) {
+InitIpmbInterfaceData (>IpmiTransport2);
+  }
+}
+
+/**
+
+Routine Description:
+  Notify call back to initialize the interfaces and install SMM IPMI
+  protocol.
+
+Arguments:
+  Protocol- Pointer to the protocol guid.
+  Interface   - Pointer to the protocol instance.
+  Handle  - Handle on which the protocol is installed.
+
+Returns:
+  Status of Notify call back.
+
+**/
+EFI_STATUS
+EFIAPI
+SmmNotifyCallback (
+  IN CONST  EFI_GUID*Protocol,
+  INVOID*Interface,
+  INEFI_HANDLE  Handle
+  )
+{
+  EFI_STATUSStatus;
+  IPMI_INTERFACE_STATE  InterfaceState;
+
+  InterfaceState = IpmiInterfaceNotReady;
+
+  if (FixedPcdGet8 (PcdSsifInterfaceSupport) == 1) {
+InitSsifInterfaceData (>IpmiTransport2);
+
+if (mIpmiInstance->IpmiTransport2.Interface.Ssif.InterfaceState == 
IpmiInterfaceInitialized) {
+  InterfaceState = IpmiInterfaceInitialized;
+}
+  }
+
+  if (FixedPcdGet8 (PcdIpmbInterfaceSupport) == 1) {
+InitIpmbInterfaceData (>IpmiTransport2);
+  }
+
+  if (mIpmiInstance->IpmiTransport2.Interface.Ipmb.InterfaceState == 
IpmiInterfaceInitialized) {
+InterfaceState = IpmiInterfaceInitialized;
+  }
+
+  if (InterfaceState != IpmiInterfaceInitialized) {
+return EFI_SUCCESS;
+  }
+
+  // Default Interface data should be initialized to install Ipmi Transport2 
Protocol.
+  if (InterfaceState == IpmiInterfaceInitialized) {
+mIpmiTransport2Handle = NULL;
+Status= gMmst->MmInstallProtocolInterface (
+ ,
+ ,
+ EFI_NATIVE_INTERFACE,
+ >IpmiTransport2
+ );
+  }
+
+  ASSERT_EFI_ERROR (Status);
+  return EFI_SUCCESS;
+}
+
+/**
+
+Routine Description:
+  Registers Protocol call back.
+
+Arguments:
+  ProtocolGuid- Pointer to Protocol GUID to register call back.
+
+Returns:
+  Status.
+
+**/

Re: [edk2-devel] [PATCH] [Patch V2]IntelFsp2Pkg\Tools\ConfigEditor: Added new USF config workstream.

[Chiu, Chasel]

Hi Arun,

This looks good.
Reviewed-by: Chasel Chiu 
We may merge this first version for basic functionality, and below are just 
some small suggestions that we may consider in following patches.

By reviewing the outcome of test_vfr_yaml.yml:
1. if aligning with VFR output, the formset title should be "Bluetooth 
Configuration" and Help string is "Config the Bluetooth parameter", current 
version it showed YAML file subject "Bluetooth Connection Manager"
2. currently "title data" and "configurable data" both showing like 
configurable, may consider to show title data as static information. (for 
example, "Active Device: " without edit box)
3. I think configurable fields maximum and minimum values are helpful so maybe 
show it as part of help string or in certain way so users could see it.

Thanks,
Chasel


> -Original Message-
> From: Soundara Pandian, Arun SuraX 
> Sent: Thursday, November 23, 2023 11:29 PM
> To: devel@edk2.groups.io
> Cc: Soundara Pandian, Arun SuraX ;
> Chiu, Chasel ; Duggapu, Chinni B
> ; Desimone, Nathaniel L
> ; Ng, Ray Han Lim
> ; Zeng, Star ; Kuo, Ted
> ; S, Ashraf Ali ; Mohapatra, 
> Susovan
> 
> Subject: [PATCH] [Patch V2]IntelFsp2Pkg\Tools\ConfigEditor: Added new USF
> config workstream.
> 
> Config Editor utility addition/changes:
> Support to enable config editor tool to have a new feature that can load and 
> view
> the configuration data of compiled VFR or HFR in form of YAML.
> This can help users to understand and track the configuration data when
> modifications are made.
> 
> Requires compiled vfr file as input in YAML format.
> 
> Running Configuration Editor:
> python ConfigEditor.py
> 
> Cc: Chasel Chiu 
> Cc: Duggapu Chinni B 
> Cc: Nate DeSimone 
> Cc: Ray Han Lim Ng 
> Cc: Star Zeng 
> Cc: Ted Kuo 
> Cc: Ashraf Ali S 
> Cc: Susovan Mohapatra 
> Signed-off-by: Arun Sura 
> ---
>  IntelFsp2Pkg/Tools/ConfigEditor/ConfigEditor.py  | 226
> +
> +
> +--
> -
>  IntelFsp2Pkg/Tools/ConfigEditor/GenYamlCfg.py| 197
> +
> +
> +++
>  IntelFsp2Pkg/Tools/Tests/test_vfr_yaml.yml   | 233
> +
> +
> +
> ++
>  IntelFsp2Pkg/Tools/UserManuals/ConfigEditorUserManual.md |   2 ++
>  4 files changed, 603 insertions(+), 55 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/Tools/ConfigEditor/ConfigEditor.py
> b/IntelFsp2Pkg/Tools/ConfigEditor/ConfigEditor.py
> index 5271504282..228ebe91a6 100644
> --- a/IntelFsp2Pkg/Tools/ConfigEditor/ConfigEditor.py
> +++ b/IntelFsp2Pkg/Tools/ConfigEditor/ConfigEditor.py
> @@ -1015,6 +1015,10 @@ class application(tkinter.Frame):
>   "Unsupported file '%s' !" % path)
>  return
> 
> +# VFR Format Page modification
> +def page_construct(self):
> +self.left.bind("<>",
> + self.on_config_page_select_change)
> +
>  def search_bar(self):
>  # get data from text box
>  self.search_text = self.edit.get() @@ -1165,7 +1169,8 @@ class
> application(tkinter.Frame):
>  page_id = next(iter(page))
>  # Put CFG items into related page list
>  self.page_list[page_id] = self.cfg_data_obj.get_cfg_list(page_id)
> -self.page_list[page_id].sort(key=lambda x: x['order'])
> +if self.mode == 'fsp':
> +self.page_list[page_id].sort(key=lambda x: x['order'])
>  page_name = self.cfg_data_obj.get_page_title(page_id)
>  child = self.left.insert(
>  parent, 'end',
> @@ -1199,17 +1204,23 @@ class application(tkinter.Frame):
>  for item in self.get_current_config_data():
>  disp_list.append(item)
>  row = 0
> -disp_list.sort(key=lambda x: x['order'])
> -for item in disp_list:
> -self.add_config_item(item, row)
> -row += 2
> -if self.invalid_values:
> -string = 'The following contails invalid options/values \n\n'
> -for i in self.invalid_values:
> -string += i + ": " + str(self.invalid_values[i]) + "\n"
> -reply = messagebox.showwarning('Warning!', string)
> -if reply == 'ok':
> -self.invalid_values.clear()
> +if self.mode == 'fsp':
> +disp_list.sort(key=lambda x: x['order'])
> +for item in disp_list:
> +

Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance

[Wu, Jiaxin]

> > +struct SMM_CPU_SYNC_CTX  {
> 
> 1. How about "SMM_CPU_SYNC_CONTEXT"?


Agree.

> 
> > +  ///
> > +  ///  All global semaphores' pointer in SMM CPU Sync
> > +  ///
> > +  SMM_CPU_SYNC_SEMAPHORE_GLOBAL*GlobalSem;
> 
> 2. There is only one GlobalSem. Can you directly use "volatile UINT32
> *Counter" instead of "GlobalSem"?

I think it's not the big deal to combine into one structure or separate into 
different structures.

Separating different attribute field into different structures will benefit the 
code readability & maintainability & expansibility. It's easy to understand 
which field is for GlobalSem, and which field is for CpuSem.

With separated structures, it will very easy coding the later logic for 
semaphore memory allocation/free. Because you know we need 64 bytes alignment 
for semaphore, in the later coding, Run[cpuIndex] can't meet requirement for 
that since it's UINT32 type. That will bring a lot of inconvenience for coding 
and very easy to make mistake. Besides, we only need allcoate/free one 
continuous Sembuffer for all semaphores, and easy for consumer point to next 
with CpuSem[CpuIndex].Run.

> 
> > +  ///
> > +  ///  All semaphores for each processor in SMM CPU Sync
> > +  ///
> > +  SMM_CPU_SYNC_SEMAPHORE_CPU   *CpuSem;
> 
> 3. Can we use "volatile UINT32 **Run" instead of pointing to another
> structure?
> Run points to an array where each element is a UINT32 *. Count of array
> equals to NumberOfCpus.
> 
> 

Same as above explained.

> 
> > +  ///
> > +  /// The number of processors in the system.
> > +  /// This does not indicate the number of processors that entered SMM.
> > +  ///
> > +  UINTNNumberOfCpus;
> > +  ///
> > +  /// Address of global and each CPU semaphores
> > +  ///
> > +  UINTN*SemBlock;
> 4. How about "SemBuffer"?
> 

Agree. 

> 
> 
> > +  ///
> > +  /// Size of global and each CPU semaphores
> > +  ///
> > +  UINTNSemBlockPages;
> 
> 5. How about "SemBufferSize"?


Agree, you want it to be bytes instead of pages?

> 
> > +};
> > +
> > +EFIAPI
> > +SmmCpuSyncContextInit (
> > +  IN   UINTN NumberOfCpus,
> > +  OUT  SMM_CPU_SYNC_CTX  **SmmCpuSyncCtx
> > +  )
> > +{
> > +  RETURN_STATUS  Status;
> > +
> > +  UINTN  CpuSemInCtxSize;
> > +  UINTN  CtxSize;
> > +
> > +  UINTN  OneSemSize;
> > +  UINTN  GlobalSemSize;
> > +  UINTN  OneCpuSemSize;
> > +  UINTN  CpuSemSize;
> > +  UINTN  TotalSemSize;
> > +
> > +  UINTN  SemAddr;
> > +  UINTN  CpuIndex;
> 
> 6. Can you remove the empty lines among the local variable declarations?

Agree. I just wanted it to be more readable: no empty lines means it's group 
usage for some purpose, that was my original coding style, but I can change it 
since you don't like it. :).


> 
> > +
> > +  if (SmmCpuSyncCtx == NULL) {
> > +return RETURN_INVALID_PARAMETER;
> > +  }
> > +
> > +  //
> > +  // Count the CtxSize
> > +  //
> > +  Status = SafeUintnMult (NumberOfCpus, sizeof
> > (SMM_CPU_SYNC_SEMAPHORE_CPU), );
> 
> 7. Is there a reason to use SafeUintxxx()? I don't believe the multiplication
> could exceed MAX_UINTN.
> But using SafeUintxxx() makes code hard to read.
> 


This is comments from Laszlo:

Please use SafeIntLib for all calculations, to prevent overflows. This
means primarily (but not exclusively) memory size calculations, for
allocations

>From API perspective, it's possible since the NumberOfCpus defined as UNITN, 
>right?


> > +  //
> > +  // Allocate CtxSize buffer for the *SmmCpuSyncCtx
> > +  //
> > +  *SmmCpuSyncCtx = NULL;
> 8. This is not needed.
> 
> > +  Status = SafeUintnMult (OneSemSize, sizeof
> > (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *),
> );
> > +  if (EFI_ERROR (Status)) {
> > +goto ON_ERROR;
> 
> 9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the
> "goto".
> 

Yes, we can discuss whether need SafeUintxxx(), for me, it's both ok. 
SafeUintxxx doesn't do bad things.

> 
> > +/**
> > +  Performs an atomic operation to check in CPU.
> > +
> > +  When SMI happens, all processors including BSP enter to SMM mode by
> > calling SmmCpuSyncCheckInCpu().
> > +
> > +  @param[in,out]  SmmCpuSyncCtx Pointer to the SMM CPU Sync
> context
> > object.
> > +  @param[in]  CpuIndex  Check in CPU index.
> > +
> > +  @retval RETURN_SUCCESSCheck in CPU (CpuIndex) successfully.
> > +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> > +  @retval RETURN_ABORTEDCheck in CPU failed due to
> > SmmCpuSyncLockDoor() has been called by one elected CPU.
> > +
> > +**/
> > +RETURN_STATUS
> > +EFIAPI
> > +SmmCpuSyncCheckInCpu (
> > +  IN OUT SMM_CPU_SYNC_CTX  *SmmCpuSyncCtx,
> > +  IN UINTN CpuIndex
> > +  )
> > +{
> > +  SMM_CPU_SYNC_CTX  *Ctx;
> > +
> > +  if (SmmCpuSyncCtx == NULL) {
> > +return RETURN_INVALID_PARAMETER;
> > +  }
> 
> 10. Can we use "ASSERT" instead of if-check? We can explicitly mention the
> 

Re: [edk2-devel] [Patch V2] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present.

[Wu, Jiaxin]

> (1) Here's why I don't like this:
> 
> we already have a function that is supposed to do this, and it is
> SmmWaitForApArrival().
> 
> SmmWaitForApArrival() is called in two contexts. One, in BSPHandler().
> Two, here.
> 
> Consider the following condition:
> 
>   (SyncMode == SmmCpuSyncModeTradition) ||
>   SmmCpuFeaturesNeedConfigureMtrrs ()
> 
> If this condition evaluates to true, then BSPHandler() calls
> SmmWaitForApArrival(), and SmmCpuRendezvous() doesn't.
> 
> (This is what the "else" branch in SmmCpuRendezvous() states, in a
> comment, too.)
> 
> And if the condition evaluates to false, then SmmCpuRendezvous() calls
> SmmWaitForApArrival(), and BSPHandler() doesn't.
> 
> This patch adds extra waiting logic to *one* of both call sites. And I
> don't understand why the *other* call site (in BSPHandler()) does not
> need the exact same logic.
> 
> In my opinion, this is a sign that SmmWaitForApArrival() isn't "strong
> enough". It is not doing all of the work.
> 
> In my opinion, *both* call sites should receive this logic (i.e., ensure
> that all AP's are "present"), but then in turn, the additional waiting /
> checking should be pushed down into SmmWaitForApArrival().
> 
> What's your opinion on that?


Existing SmmWaitForApArrival() only make sure all Aps enter SMI except SMI 
blocked & disabled Aps, not consider the "Present" state. I think this is the 
original implementation purpose. It won't require all Aps must set the Present 
flag.

As you also commented there is a later loop for the Present flag:
WaitForAllAPs (ApCount)

Here, i still prefer to keep existing way instead of making SmmWaitForApArrival 
return until all aps set the Present flag, because that will be duplicate work 
within SmmWaitForApArrival() & existing  WaitForAllAPs (). We can't delete the 
WaitForAllAPs for the later sync to make sure all APs to get ready for 
programming MTRRs. MTRRs programming need all CPUs in the same start line. 

  WaitForAllAPs() has two purpose:
   1. Make sure all Aps have set the Present.
   2. Get ready for programming MTRRs to make sure cpus in the same start line.

if so, that will be better as existing logic, it can also save some time for 
the Present flag check in SmmWaitForApArrival

> 
> (2) I notice that a similar "busy loop", waiting for Present flags, is
> in BSPHandler().
> 
> Do you think we could call CpuPause() in all such "busy loops" (just
> before the end of the "while" body)? I think that's supposed to improve
> the system's throughput, considered as a whole. The function's comment
> says,
> 
>   Requests CPU to pause for a short period of time. Typically used in MP
>   systems to prevent memory starvation while waiting for a spin lock.
> 

Do you mean the below WaitForAllAPs()? There is already has the CpuPause check 
within WaitForSemaphore().

//
// Wait for all APs to get ready for programming MTRRs
//
WaitForAllAPs (ApCount);


Thanks,
Jiaxin 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112088): https://edk2.groups.io/g/devel/message/112088
Mute This Topic: https://groups.io/mt/102556528/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance

[Ni, Ray]

> +typedef struct {
> +  ///
> +  /// Indicate how many CPU entered SMM.
> +  ///
> +  volatile UINT32*Counter;
> +} SMM_CPU_SYNC_SEMAPHORE_GLOBAL;
> +
> +typedef struct {
> +  ///
> +  /// Used for control each CPU continue run or wait for signal
> +  ///
> +  volatile UINT32*Run;
> +} SMM_CPU_SYNC_SEMAPHORE_CPU;
> +
> +struct SMM_CPU_SYNC_CTX  {

1. How about "SMM_CPU_SYNC_CONTEXT"?

> +  ///
> +  ///  All global semaphores' pointer in SMM CPU Sync
> +  ///
> +  SMM_CPU_SYNC_SEMAPHORE_GLOBAL*GlobalSem;

2. There is only one GlobalSem. Can you directly use "volatile UINT32 *Counter" 
instead of "GlobalSem"?

> +  ///
> +  ///  All semaphores for each processor in SMM CPU Sync
> +  ///
> +  SMM_CPU_SYNC_SEMAPHORE_CPU   *CpuSem;

3. Can we use "volatile UINT32 **Run" instead of pointing to another structure?
Run points to an array where each element is a UINT32 *. Count of array equals 
to NumberOfCpus.



> +  ///
> +  /// The number of processors in the system.
> +  /// This does not indicate the number of processors that entered SMM.
> +  ///
> +  UINTNNumberOfCpus;
> +  ///
> +  /// Address of global and each CPU semaphores
> +  ///
> +  UINTN*SemBlock;
4. How about "SemBuffer"?



> +  ///
> +  /// Size of global and each CPU semaphores
> +  ///
> +  UINTNSemBlockPages;

5. How about "SemBufferSize"?

> +};
> +
> +EFIAPI
> +SmmCpuSyncContextInit (
> +  IN   UINTN NumberOfCpus,
> +  OUT  SMM_CPU_SYNC_CTX  **SmmCpuSyncCtx
> +  )
> +{
> +  RETURN_STATUS  Status;
> +
> +  UINTN  CpuSemInCtxSize;
> +  UINTN  CtxSize;
> +
> +  UINTN  OneSemSize;
> +  UINTN  GlobalSemSize;
> +  UINTN  OneCpuSemSize;
> +  UINTN  CpuSemSize;
> +  UINTN  TotalSemSize;
> +
> +  UINTN  SemAddr;
> +  UINTN  CpuIndex;

6. Can you remove the empty lines among the local variable declarations?

> +
> +  if (SmmCpuSyncCtx == NULL) {
> +return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Count the CtxSize
> +  //
> +  Status = SafeUintnMult (NumberOfCpus, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_CPU), );

7. Is there a reason to use SafeUintxxx()? I don't believe the multiplication 
could exceed MAX_UINTN.
But using SafeUintxxx() makes code hard to read.

> +  //
> +  // Allocate CtxSize buffer for the *SmmCpuSyncCtx
> +  //
> +  *SmmCpuSyncCtx = NULL;
8. This is not needed.

> +  Status = SafeUintnMult (OneSemSize, sizeof
> (SMM_CPU_SYNC_SEMAPHORE_GLOBAL) / sizeof (VOID *), );
> +  if (EFI_ERROR (Status)) {
> +goto ON_ERROR;

9. If you don't use SafeUintxxx(), you don't need "ON_ERROR" label and the 
"goto".


> +/**
> +  Performs an atomic operation to check in CPU.
> +
> +  When SMI happens, all processors including BSP enter to SMM mode by
> calling SmmCpuSyncCheckInCpu().
> +
> +  @param[in,out]  SmmCpuSyncCtx Pointer to the SMM CPU Sync context
> object.
> +  @param[in]  CpuIndex  Check in CPU index.
> +
> +  @retval RETURN_SUCCESSCheck in CPU (CpuIndex) successfully.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval RETURN_ABORTEDCheck in CPU failed due to
> SmmCpuSyncLockDoor() has been called by one elected CPU.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SmmCpuSyncCheckInCpu (
> +  IN OUT SMM_CPU_SYNC_CTX  *SmmCpuSyncCtx,
> +  IN UINTN CpuIndex
> +  )
> +{
> +  SMM_CPU_SYNC_CTX  *Ctx;
> +
> +  if (SmmCpuSyncCtx == NULL) {
> +return RETURN_INVALID_PARAMETER;
> +  }

10. Can we use "ASSERT" instead of if-check? We can explicitly mention the 
behavior in API comments.

> +
> +  Ctx = (SMM_CPU_SYNC_CTX *)SmmCpuSyncCtx;

11. Why do we need the type cast?

> +
> +/**
> +  Performs an atomic operation lock door for CPU checkin or checkout.
> +
> +  After this function:
> +  CPU can not check in via SmmCpuSyncCheckInCpu().
> +  CPU can not check out via SmmCpuSyncCheckOutCpu().
> +  CPU can not get number of arrived CPU in SMI via
> SmmCpuSyncGetArrivedCpuCount(). The number of
> +  arrived CPU in SMI will be returned in CpuCount.
12. I agree that after locking, cpu cannot "checkin".
But can cpu "checkout" or "getArrivedcount"? I need to double check.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112087): https://edk2.groups.io/g/devel/message/112087
Mute This Topic: https://groups.io/mt/102889293/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class

[Ni, Ray]

Patch is good to me.
Only one comment: The file header comments should have 2 space chars in the 
beginning of each line.


Thanks,
Ray
> -Original Message-
> From: Wu, Jiaxin 
> Sent: Thursday, November 30, 2023 2:32 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek ; Dong, Eric ; Ni,
> Ray ; Zeng, Star ; Gerd Hoffmann
> ; Kumar, Rahul R 
> Subject: [PATCH v2 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class
> 
> Intel is planning to provide different SMM CPU Sync implementation
> along with some specific registers to improve the SMI performance,
> hence need SmmCpuSyncLib Library for Intel.
> 
> This patch is to:
> 1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
> 2.Adds SmmCpuSyncLib.h function declaration header file.
> 
> For the new SmmCpuSyncLib, it provides 3 sets of APIs:
> 
> 1. ContextInit/ContextDeinit/ContextReset:
> ContextInit() is called in driver's entrypoint to allocate and
> initialize the SMM CPU Sync context. ContextDeinit() is called in
> driver's unload function to deinitialize SMM CPU Sync context.
> ContextReset() is called before CPU exist SMI, which allows CPU to
> check into the next SMI from this point.
> 
> 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu(). The elected BSP calls LockDoor() so that
> CheckInCpu() will return the error code after that. CheckOutCpu() can
> be called in error handling flow for the CPU who calls CheckInCpu()
> earlier. GetArrivedCpuCount() returns the number of checked-in CPUs.
> 
> 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number
> of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are
> called from APs to wait and release BSP. The 4 APIs are used to
> synchronize the running flow among BSP and APs. BSP and AP Sync flow
> can be easy understand as below:
> BSP: ReleaseOneAp  -->  AP: WaitForBsp
> BSP: WaitForAPs<--  AP: ReleaseBsp
> 
> Cc: Laszlo Ersek 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Zeng Star 
> Cc: Gerd Hoffmann 
> Cc: Rahul Kumar 
> Signed-off-by: Jiaxin Wu 
> ---
>  UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 278
> +
>  UefiCpuPkg/UefiCpuPkg.dec  |   3 +
>  2 files changed, 281 insertions(+)
>  create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> 
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> new file mode 100644
> index 00..22935cc006
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
> @@ -0,0 +1,278 @@
> +/** @file
> +Library that provides SMM CPU Sync related operations.
> +The lib provides 3 sets of APIs:
> +1. ContextInit/ContextDeinit/ContextReset:
> +ContextInit() is called in driver's entrypoint to allocate and initialize 
> the SMM
> CPU Sync context.
> +ContextDeinit() is called in driver's unload function to deinitialize the SMM
> CPU Sync context.
> +ContextReset() is called before CPU exist SMI, which allows CPU to check
> into the next SMI from this point.
> +
> +2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor:
> +When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu().
> +The elected BSP calls LockDoor() so that CheckInCpu() will return the error
> code after that.
> +CheckOutCpu() can be called in error handling flow for the CPU who calls
> CheckInCpu() earlier.
> +GetArrivedCpuCount() returns the number of checked-in CPUs.
> +
> +3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> +WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number of
> APs and release one specific AP.
> +WaitForBsp() & ReleaseBsp() are called from APs to wait and release BSP.
> +The 4 APIs are used to synchronize the running flow among BSP and APs. BSP
> and AP Sync flow can be
> +easy understand as below:
> +BSP: ReleaseOneAp  -->  AP: WaitForBsp
> +BSP: WaitForAPs<--  AP: ReleaseBsp
> +
> +Copyright (c) 2023, Intel Corporation. All rights reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef SMM_CPU_SYNC_LIB_H_
> +#define SMM_CPU_SYNC_LIB_H_
> +
> +#include 
> +
> +//
> +// Opaque structure for SMM CPU Sync context.
> +//
> +typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX;
> +
> +/**
> +  Create and initialize the SMM CPU Sync context.
> +
> +  SmmCpuSyncContextInit() function is to allocate and initialize the SMM
> CPU Sync context.
> +
> +  @param[in]  NumberOfCpus  The number of Logical
> Processors in the system.
> +  @param[out] SmmCpuSyncCtx Pointer to the new created and
> initialized SMM CPU Sync context object.
> +NULL will be returned if any
> error happen during init.
> +
> +  @retval RETURN_SUCCESSThe SMM CPU Sync context
> was successful created and initialized.
> +  @retval RETURN_INVALID_PARAMETER  SmmCpuSyncCtx is NULL.
> +  @retval 

Re: [edk2-devel] [PATCH v2 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance

[Guo, Gua]

Reviewed-by: Gua Guo 

-Original Message-
From: Wu, Jiaxin  
Sent: Thursday, November 30, 2023 2:32 PM
To: devel@edk2.groups.io
Cc: Laszlo Ersek ; Dong, Guo ; Rhodes, 
Sean ; Lu, James ; Guo, Gua 
; Ni, Ray ; Zeng, Star 

Subject: [PATCH v2 5/6] UefiPayloadPkg: Specifies SmmCpuSyncLib instance

This patch is to specify SmmCpuSyncLib instance for UefiPayloadPkg.

Cc: Laszlo Ersek 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Cc: Gua Guo 
Cc: Ray Ni 
Cc: Zeng Star 
Signed-off-by: Jiaxin Wu 
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc 
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index a65f9d5b83..b8b13ad201 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -253,10 +253,11 @@
   #
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
   LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf
+  SmmCpuSyncLib|UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
 
   #
   # Platform
   #
 !if $(CPU_TIMER_LIB_ENABLE) == TRUE && $(UNIVERSAL_PAYLOAD) == TRUE
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112085): https://edk2.groups.io/g/devel/message/112085
Mute This Topic: https://groups.io/mt/102889295/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib

[Wu, Jiaxin]

Hi All, 

Could you help review the series patches in v2? 

Thank you very much!
Jiaxin 

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Wu,
> Jiaxin
> Sent: Thursday, November 30, 2023 2:32 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek ; Dong, Eric ; Ni,
> Ray ; Zeng, Star ; Gerd Hoffmann
> ; Kumar, Rahul R ; Ard
> Biesheuvel ; Yao, Jiewen
> ; Justen, Jordan L ; Dong,
> Guo ; Rhodes, Sean ; Lu,
> James ; Guo, Gua 
> Subject: [edk2-devel] [PATCH v2 0/6] Refine SMM CPU Sync flow and abstract
> SmmCpuSyncLib
> 
> The series patches are to refine SMM CPU Sync flow.
> After the refinement, SmmCpuSyncLib is abstracted for
> any user to provide different SMM CPU Sync implementation.
> 
> Compared to V1, has following refinement & changes:
> 1. Remove below patch from this series patches:
> UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM
> Exit
> Reason: Plan to separate that patch into another patch set.
> 2. Refine the patch according Laszlo & Ray's comments:
> a. Removed Change-Id in the patches
> b. Optimized the description to avoid the confusing.
> c. Fixed wrong function comment to make it correct and understandable.
> d. Use an incomplete structure type to aviod the VOID*.
> e. Corrected all functions params "in" & "out".
> f. Added lots of comments for the library to explain the
> operation & behavior.
> g. Fixed inconsistent return types from lib APIs.
> h. Use SafeIntLib for all calculations to prevent overflows.
> i. Remove the UefiCpuPkg/UefiCpuLibs.dsc.inc
> 
> Cc: Laszlo Ersek 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Zeng Star 
> Cc: Gerd Hoffmann 
> Cc: Rahul Kumar 
> Cc: Ard Biesheuvel 
> Cc: Jiewen Yao 
> Cc: Jordan Justen 
> Cc: Guo Dong 
> Cc: Sean Rhodes 
> Cc: James Lu 
> Cc: Gua Guo 
> Signed-off-by: Jiaxin Wu 
> 
> 
> Jiaxin Wu (6):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP
> and AP
>   UefiCpuPkg: Adds SmmCpuSyncLib library class
>   UefiCpuPkg: Implements SmmCpuSyncLib library instance
>   OvmfPkg: Specifies SmmCpuSyncLib instance
>   UefiPayloadPkg: Specifies SmmCpuSyncLib instance
>   UefiCpuPkg/PiSmmCpuDxeSmm: Consume SmmCpuSyncLib
> 
>  OvmfPkg/CloudHv/CloudHvX64.dsc |   2 +
>  OvmfPkg/OvmfPkgIa32.dsc|   2 +
>  OvmfPkg/OvmfPkgIa32X64.dsc |   2 +
>  OvmfPkg/OvmfPkgX64.dsc |   1 +
>  UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 278 +
>  UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c   | 690
> +
>  UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf |  39 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 275 
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   6 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |   1 +
>  UefiCpuPkg/UefiCpuPkg.dec  |   3 +
>  UefiCpuPkg/UefiCpuPkg.dsc  |   3 +
>  UefiPayloadPkg/UefiPayloadPkg.dsc  |   1 +
>  13 files changed, 1132 insertions(+), 171 deletions(-)
>  create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.inf
> 
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112084): https://edk2.groups.io/g/devel/message/112084
Mute This Topic: https://groups.io/mt/102889290/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] TianoCore Community Meeting call for topics

[Michael D Kinney]

Are there any topics for the TianoCore Community Meeting this week?

Thanks,

Mike


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112083): https://edk2.groups.io/g/devel/message/112083
Mute This Topic: https://groups.io/mt/103002996/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 1/1] MdePkg:Add NVME Sanitize command support to Nvme.h

[Michael D Kinney]

Merged: https://github.com/tianocore/edk2/pull/5108


> -Original Message-
> From: Kinney, Michael D 
> Sent: Wednesday, November 15, 2023 6:50 PM
> To: Chen, Tina ; devel@edk2.groups.io
> Cc: Ni, Ray ; Chen, Xiao X ; Chen,
> Arthur G ; Gao, Liming ;
> Liu, Zhiguang ; Sean Brogan
> ; Kinney, Michael D 
> Subject: RE: [PATCH v4 1/1] MdePkg:Add NVME Sanitize command support to
> Nvme.h
> 
> Reviewed-by: Michael D Kinney 
> 
> > -Original Message-
> > From: Chen, Tina 
> > Sent: Wednesday, November 8, 2023 11:51 PM
> > To: devel@edk2.groups.io
> > Cc: Chen, Tina ; Ni, Ray ;
> > Chen, Xiao X ; Chen, Arthur G
> > ; Gao, Liming ;
> > Liu, Zhiguang ; Sean Brogan
> > ; Kinney, Michael D
> > 
> > Subject: [PATCH v4 1/1] MdePkg:Add NVME Sanitize command support to
> > Nvme.h
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4591
> >
> > 1. Refer NVME spec 2.0c chapter 5.24, add Sanitize Command related
> > definition.
> > 2. Refer NVME spec 2.0c chapter 5.16, add Get Log Page Command related
> > definition for Sanitize status support.
> >
> > Cc: Ray Ni 
> > Cc: Xiao X Chen 
> > Cc: Arthur Chen 
> > Cc: Liming Gao 
> > Cc: Zhiguang Liu 
> > Cc: Sean Brogan 
> > Cc: Michael D Kinney 
> > Signed-off-by: Tina Chen 
> > ---
> >  MdePkg/Include/IndustryStandard/Nvme.h | 121 ++--
> >  1 file changed, 110 insertions(+), 11 deletions(-)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Nvme.h
> > b/MdePkg/Include/IndustryStandard/Nvme.h
> > index 8b8a1bb7f3..67f2196bc7 100644
> > --- a/MdePkg/Include/IndustryStandard/Nvme.h
> > +++ b/MdePkg/Include/IndustryStandard/Nvme.h
> > @@ -1,5 +1,5 @@
> >  /** @file
> >
> > -  Definitions based on NVMe spec. version 1.1.
> >
> > +  Definitions based on NVMe spec. version 2.0c.
> >
> >
> >
> >(C) Copyright 2016 Hewlett Packard Enterprise Development LP
> >
> >Copyright (c) 2017 - 2023, Intel Corporation. All rights
> > reserved.
> >
> > @@ -9,6 +9,7 @@
> >NVMe Specification 1.1
> >
> >NVMe Specification 1.4
> >
> >NVMe Specification 2.0
> >
> > +  NVMe Specification 2.0c
> >
> >
> >
> >  **/
> >
> >
> >
> > @@ -354,6 +355,15 @@ typedef struct {
> >UINT8 Rsvd7[16];  /* Reserved as of Nvm Express 1.1 Spec */
> >
> >  } NVME_PSDESCRIPTOR;
> >
> >
> >
> > +typedef struct {
> >
> > +  UINT32Ces : 1;/* Crypto Erase Supported */
> >
> > +  UINT32Bes : 1;/* Block Erase Supported */
> >
> > +  UINT32Ows : 1;/* Overwrite Supported */
> >
> > +  UINT32Rsvd1   : 26;   /* Reserved as of NVM Express 2.0c Spec
> > */
> >
> > +  UINT32Ndi : 1;/* No-Deallocate Inhibited */
> >
> > +  UINT32Nodmmas : 2;/* No-Deallocate Modifies Media After
> > Sanitize */
> >
> > +} NVME_SANICAP;
> >
> > +
> >
> >  //
> >
> >  //  Identify Controller Data
> >
> >  //
> >
> > @@ -403,7 +413,12 @@ typedef struct {
> >UINT16   Edstt;   /* Extended Device Self-test Time
> > */
> >
> >UINT8Dsto;/* Device Self-test Options  */
> >
> >UINT8Fwug;/* Firmware Update Granularity */
> >
> > -  UINT8Rsvd2[192];  /* Reserved as of Nvm Express 1.4
> > Spec */
> >
> > +  UINT16   Kas; /* Keep Alive Support */
> >
> > +  UINT16   Hctma;   /* Host Controlled Thermal
> > Management Attributes */
> >
> > +  UINT16   Mntmt;   /* Minimum Thermal Management
> > Temperature */
> >
> > +  UINT16   Mxtmt;   /* Maximum Thermal Management
> > Temperature */
> >
> > +  NVME_SANICAP Sanicap; /* Sanitize Capabilities */
> >
> > +  UINT8Rsvd2[180];  /* Reserved as of Nvm Express 1.4
> > Spec */
> >
> >//
> >
> >// NVM Command Set Attributes
> >
> >//
> >
> > @@ -687,10 +702,11 @@ typedef struct {
> >// CDW 10
> >
> >//
> >
> >UINT32Lid   : 8;/* Log Page Identifier */
> >
> > -  #define LID_ERROR_INFO0x1
> >
> > -  #define LID_SMART_INFO0x2
> >
> > -  #define LID_FW_SLOT_INFO  0x3
> >
> > -  #define LID_BP_INFO   0x15
> >
> > +  #define LID_ERROR_INFO0x1
> >
> > +  #define LID_SMART_INFO0x2
> >
> > +  #define LID_FW_SLOT_INFO  0x3
> >
> > +  #define LID_BP_INFO   0x15
> >
> > +  #define LID_SANITIZE_STATUS_INFO  0x81
> >
> >UINT32Rsvd1 : 8;
> >
> >UINT32Numd  : 12;   /* Number of Dwords */
> >
> >UINT32Rsvd2 : 4;/* Reserved as of Nvm Express 1.1 Spec
> > */
> >
> > @@ -708,6 +724,31 @@ typedef struct {
> >UINT32Sv: 1;/* Save */
> >
> >  } NVME_ADMIN_SET_FEATURES;
> >
> >
> >
> > +//
> >
> > +// NvmExpress Admin Sanitize Command
> >
> > +//
> >
> > +typedef struct {
> >
> > +  //
> >
> > +  // CDW 10
> >
> > +  //
> >
> > +  UINT32Sanact : 3;   /* Sanitize Action */
> >
> > +  UINT32Ause   : 1;   /* Allow Unrestricted Sanitize Exit */
> >
> > +  UINT32Owpass : 

[edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

[Gerd Hoffmann]

Extend the ValidateFvHeader function, additionally to the header checks
walk over the list of variables and sanity check them.

In case we find inconsistencies indicating variable store corruption
return EFI_NOT_FOUND so the variable store will be re-initialized.

Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 82 +--
 1 file changed, 77 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c 
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
index 5ee98e9b595a..72a197e5aa20 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -185,11 +185,16 @@ ValidateFvHeader (
   IN  NOR_FLASH_INSTANCE  *Instance
   )
 {
-  UINT16  Checksum;
-  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
-  VARIABLE_STORE_HEADER   *VariableStoreHeader;
-  UINTN   VariableStoreLength;
-  UINTN   FvLength;
+  UINT16 Checksum;
+  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
+  VARIABLE_STORE_HEADER  *VariableStoreHeader;
+  UINTN  VarOffset;
+  AUTHENTICATED_VARIABLE_HEADER  *VarHeader;
+  UINTN  VarSize;
+  CHAR16 *VarName;
+  CHAR8  *VarState;
+  UINTN  VariableStoreLength;
+  UINTN  FvLength;
 
   FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
 
@@ -260,6 +265,73 @@ ValidateFvHeader (
 return EFI_NOT_FOUND;
   }
 
+  // check variables
+  DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
+  VarOffset = sizeof (*VariableStoreHeader);
+  while (VarOffset + sizeof (*VarHeader) < VariableStoreHeader->Size) {
+VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
+if (VarHeader->StartId != 0x55aa) {
+  DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__));
+  break;
+}
+
+VarSize = sizeof (*VarHeader) + VarHeader->NameSize + VarHeader->DataSize;
+if (VarOffset + VarSize > VariableStoreHeader->Size) {
+  DEBUG ((
+DEBUG_ERROR,
+"%a: invalid variable size: 0x%x + 0x%x + 0x%x + 0x%x > 0x%x\n",
+__func__,
+VarOffset,
+sizeof (*VarHeader),
+VarHeader->NameSize,
+VarHeader->DataSize,
+VariableStoreHeader->Size
+));
+  return EFI_NOT_FOUND;
+}
+
+VarName = (VOID *)((UINTN)VariableStoreHeader + VarOffset
+   + sizeof (*VarHeader));
+switch (VarHeader->State) {
+  case VAR_HEADER_VALID_ONLY:
+VarState = "header-ok";
+VarName  = L"";
+break;
+  case VAR_ADDED:
+VarState = "ok";
+break;
+  case VAR_ADDED _IN_DELETED_TRANSITION:
+VarState = "del-in-transition";
+break;
+  case VAR_ADDED _DELETED:
+  case VAR_ADDED _DELETED _IN_DELETED_TRANSITION:
+VarState = "deleted";
+break;
+  default:
+DEBUG ((
+  DEBUG_ERROR,
+  "%a: invalid variable state: 0x%x\n",
+  __func__,
+  VarState
+  ));
+return EFI_NOT_FOUND;
+}
+
+DEBUG ((
+  DEBUG_VERBOSE,
+  "%a: +0x%04x: name=0x%x data=0x%x '%s' (%a)\n",
+  __func__,
+  VarOffset,
+  VarHeader->NameSize,
+  VarHeader->DataSize,
+  VarName,
+  VarState
+  ));
+
+VarSize= (VarSize + 3) & ~3; // align
+VarOffset += VarSize;
+  }
+
   return EFI_SUCCESS;
 }
 
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112081): https://edk2.groups.io/g/devel/message/112081
Mute This Topic: https://groups.io/mt/102991507/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Gerd Hoffmann]

  Hi,

> I know of 3 categories:
> 
> 1) U-Boot
> 2) EC2
> 3) Windows ARM notebooks

Thanks.

> > Well, no.  One problem is install media, where you really have only
> > one entry point (EFI/BOOT/BOOT${ARCH}.EFI) which must work under all
> > circumstances.  Which must be shim with microsoft signature (and ideally
> > distro signature too) on x64.
> 
> What if that shim immediately on start tries to see if it can load and
> execute another binary at the same path instead?

One more possible code path.  I have my doubts that (a) the shim
maintainers like the idea, and (b) that it actually improves the overall
situation.

Also note that it isn't a fixed binary.  While grub.efi is the default
you can pass something else on the command line, for example fwupd.efi
or an UKI (unified kernel image).  Also depending on circumstances shim
decides to load fallback,efi or mokmanager.efi instead of grub.efi.  So
it's more complex that "try load grub.efi using standard efi protocols
and if that works I'm done".

> > Except for https://github.com/rhboot/shim/blob/main/SBAT.md
> 
> I'm not quite sure what you're getting at :).
> 
> First, the fundamental problem with huge dbx files is *precisely* because
> the MS key is used in too many places. Smaller set of key scope means
> smaller dbx.
> 
> Second, I think the basic concept of introducing a new abstracted "version"
> that dbx can match against is great. It would make dbx updates a lot more
> efficient. So why don't we include that concept in the UEFI spec?

Not sure what the status of this is, whenever it is just some kind of
agreement between shim maintainers and microsoft, trying to keep dbx
grow under control, or whenever there are any plans to add that to the
uefi specs.  Peter?

Adding that to the uefi specs (and subsequently implement it in edk2)
has the advantage that it wouldn't depend on shim.efi, and have the
drawback that it'll take ages to have an effect due to the firmware
update support for a big chunks of physical hardware being shitty (not
that shim updates look that much better right now ...)

My impression is that microsoft tries to improve the firmware security
situation without depending on firmware updates if possible.  Being more
strict on the PE binaries they are willing to sign for secure boot is
one step which goes into that category.

> > IMHO it essentially it comes down to standardize a few things.  Most
> > importantly placing the distro secure boot certificate on some
> > well-known location on the install media, so tooling like virt-install
> > can pre-load it into 'db' of the guest it is going to install.
> > Something similar for cloud images, so OpenStack / EC2 / whatever can do
> > the same.
> 
> I don't know if putting it on a standardized location is really necessary.
> If you boot with SB disabled and just as part of the installation process
> install the distro keys, everything should fall into place nicely, no?

Yes.  Defining the key enrollment being job of the installer would work,
at least for the cases where an installation actually happens.

> For OpenStack/EC2/whatever, you don't run the installer, so you don't need a
> standardized location for it. For EC2 the target image is completely opaque.
> We don't even have (or want to have) a storage or file system driver to
> access it. That's why it's in metadata (UefiData), separate from the disk
> image.

There is no standard for metadata though, along the lines of "when using
this disk image with secure boot you should use that uefi varstore".
What we have today is ec2-specific, you have to take care when creating
the AMI.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112080): https://edk2.groups.io/g/devel/message/112080
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg

[Chao Li]

Hi Ray,


Thanks,
Chao
On 2023/12/5 16:27, Ni, Ray wrote:


Thanks,

Ray

*From:* devel@edk2.groups.io  *On Behalf Of *Chao Li
*Sent:* Monday, December 4, 2023 3:32 PM
*To:* devel@edk2.groups.io; Ni, Ray 
*Cc:* Dong, Eric ; Kumar, Rahul R 
; Gerd Hoffmann ; Leif 
Lindholm ; Ard Biesheuvel 
; Sami Mujawar ; 
Sunil V L ; Warkentin, Andrei 

*Subject:* Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add 
CpuMmuLib.h to UefiCpuPkg


Hi Ray,

For this patch, I checked again and here are my opinions:

 1. (Set|Get)MemoryRegionAttribute is difficult to merge together,
because the parameters between the tow APIs are not similar. So I
suggest they be independent.

[Ray] What I mean is to merge SetMemoryRegion(NonExec|ReadOly) to 
SetMemoryRegionAttribute(). Similarly, GetXXX can be merged as well. 
What’s your opinion?



Ok, I already said it in point 2, other APIs will be removed.


 2. The EfiAttributeConverse, GetMemoryRegionAttribute,
SetMemoryRegionAttributes and ConfigureMemoryManagementUnit will
be retained and other APIs will be removed. Because the functions
expressed by other APIs can be completed though the retained API.

[Ray] I didn’t notice EfiAttributeConverse(). I guess callers may not 
need to know the architectural specific attributes. So 
EfiAttributeConverse() might be not needed as a public API.



I agree, the EfiAttributeCoverse() complete by caller or as a private API.


 3. You pointed out MEMORY_REGION_DESCRIPTOR have no one to construct
it, do I need add a new API to construct it? Could it be named
GetMemoryMapPolicy and accept a parameter with
MEMORY_REGION_DESCRIPTOR** ?

[Ray] So the GetMemoryRegionAttribute() and 
SetMemoryRegionAttributes() are performed on the active translation 
table. ConfigureMemoryManagementUint() is to create a translation 
table with a list of memory attributes. How about the following idea?


[Ray] (Set|Get)MemoryRegionAttribute() are performed on a translation 
table buffer. And caller calls SetMemoryRegionAttribute() to modify 
the translation table buffer supplied as the parameter. With this, 
ConfigureMemoryManagementUnit() is not needed.


Ah, I think you may have some misunderstanding, the 
ConfigureMemoryManagementUint is a function that to initialize the MMU. 
The MEMORY_REGION_DESCRIPTOR will created by the private API, and then 
the caller will call the ConfigureMemoryManagementUnit to initialize the 
MMU first(may be fill the static page tables and so on).


But I thought about it again and it seems you are right, the 
ConfigureMemoryManagementUnit and discrptor creater as the public APIs 
are not appropriate. They are more suitable as some private APIs.


So, (Get|Set)MemoryRegionAttribute() will be the public APIs and the 
parameters will be the same with this change?



Hope to hear from you! :)

Thanks,
Chao

On 2023/11/30 10:25, Chao Li wrote:

Hi Ray,

Thanks for review, here are some of my thoughts:

On 2023/11/30 08:59, Ni, Ray wrote:

Chao,

Since the lib class is so general, I'd like to understand more details 
to make sure it can properly fit into any CPU arch.

In X86, cache setting is through MSRs and Page tables, and memory 
access control (read-only, not-present, non-executable) is through page tables.

Let me understand, 'cache setting' means does it access a certain
address(probably a memory address) via cache? If so, I'd say the
'cache setting' should be a part of attributes.

This CpuMmuLib is to provide both services. How does LoongArch64 manage 
the cache settings and memory access control?

Is it proper to combine both services into one lib?

In LoongArch64, cache settings and memory access control are
performed via page tables. Please check the patch 14 of this series.

If the backend silicon IP is the same one that supports the "one" lib 
design, can we refine the lib API a bit?

Yes, I think Attribute's instance family can be bear the memory
access and cache setting. So what are you suggestions if we
improve the lib API?

We have (Set|Get)MemoryRegionAttribute() and 
(Set|Clear)MemoryRegion(NoExec|ReadOnly). Can we merge them together?

Do you means the (Set|Get) merge together(differentiate Get or Set
operations by parameters)? If so, I think it's OK, but maybe some
existing instances will be modified together.

And the API ConfigureMemoryManagementUint() accepts 
MEMORY_REGION_DESCRIPTOR but none of other APIs helps to construct the 
descriptor.

Yes, currently, no one helps construct MEMORY_REGION_DESCRIPTOR. I
think the construction of descriptors is not part of the API, it
should be the localized or private when I design them. Do I need
to add an API to construct descripters?

It seems to me the MmuLib is simply a combination of different random 
APIs.

It's not a well-designed library class.

We need more discussion to make it 

Re: [edk2-devel] [PATCH v5 6/6] ShellPkg/AcpiView: Add MPAM Parser

[Sami Mujawar]

Hi Rohit,

Thank you for this patch.

Please see my feedback inline marked [SAMI].

Regards,

Sami Mujawar

On 02/10/2023 06:17 pm, Rohit Mathew wrote:

Add a parser for the MPAM (Memory system resource partitioning and
monitoring) ACPI table. This parser would parse all MPAM related
structures embedded as part of the ACPI table. Necessary validations are
also performed where and when required.

Signed-off-by: Rohit Mathew
Cc: James Morse
Cc: Sami Mujawar
Cc: Thomas Abraham
Cc: Yeo Reum Yun
Cc: Zhichao Gao
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
|   21 +
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c   
| 1276 
  ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c   
|3 +-
  ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf 
|3 +-
  ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni 
|3 +-
  5 files changed, 1303 insertions(+), 3 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 89930fdc6d..4d2764065d 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -879,6 +879,27 @@ ParseAcpiMcfg (
IN UINT8AcpiTableRevision
);
  
+/**

+  This function parses the ACPI MPAM table.
+  When trace is enabled this function parses the MPAM table and
+  traces the ACPI table fields.
+
+  This function also performs validation of the ACPI table fields.
+
+  @param [in] Trace  If TRUE, trace the ACPI fields.
+  @param [in] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength of the ACPI table.
+  @param [in] AcpiTableRevision  Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiMpam (
+  IN BOOLEAN  Trace,
+  IN UINT8*Ptr,
+  IN UINT32   AcpiTableLength,
+  IN UINT8AcpiTableRevision
+  );
+
  /**
This function parses the ACPI PCCT table including its sub-structures
of type 0 through 4.
diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c
new file mode 100644
index 00..2d94f9e5eb
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c
@@ -0,0 +1,1276 @@
+/** @file
+  MPAM table parser
+
+  Copyright (c) 2023, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Specification Reference:
+   - [1] ACPI for Memory System Resource Partitioning and Monitoring 2.0
+ (https://developer.arm.com/documentation/den0065/latest)
+
+  @par Glossary:
+- MPAM - Memory System Resource Partitioning And Monitoring
+- MSC  - Memory System Component
+- PCC  - Platform Communication Channel
+- RIS  - Resource Instance Selection
+- SMMU - Arm System Memory Management Unit
+ **/
+
+#include 
+#include 
+#include 
+#include "AcpiParser.h"
+#include "AcpiView.h"
+#include "AcpiViewConfig.h"
+
+// Local variables
+STATIC CONST UINT8   *ResourceLocatorType;
+STATIC UINT32MpamMscNodeLengthCumulative;
+STATIC UINT32MpamMscNodeStart;
+STATIC UINT32ResourceDataSize;
[SAMI] I would prefer to remove ResourceDataSiz, see my suggestions 
further in the code.

+STATIC UINT32HeaderSize;
[SAMI] HeaderSize is only used in ParseAcpiMpam(). Can this be defined 
as a local variable, please?

+STATIC CONST UINT16  *MpamMscNodeLength;
+STATIC CONST UINT32  *MscInterfaceType;
+STATIC CONST UINT32  *NumberOfMscResources;
+STATIC CONST UINT32  *NumberOfFunctionalDependencies;
+STATIC CONST UINT32  *NumberOfInterconnectDescriptors;
+STATIC CONST UINT64  *InterconnectTableOffset;
+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
+
+// Array of locator type names. New types should be added keeping the order
+// preserved as locator type is used to index into the array while parsing.
+STATIC CONST CHAR16  *MpamMscLocatorTitles[] = {
+  L"Processor cache",
+  L"Memory",
+  L"SMMU",
+  L"Memory cache",
+  L"ACPI device",
+  L"Interconnect"
+};
+
+/**
+  When the length of the table is insufficient to be parsed, this function 
could
+  be used to display an appropriate error message.
+
+  @param [in] ErrorMsg  Error message string that has to be appended to the
+main error log. This string could explain the 
reason
+why a insufficient length error was encountered in
+the first place.
+**/
+STATIC
+VOID
+EFIAPI
+MpamLengthError (
+  IN CONST CHAR16  *ErrorMsg
+  )
+{
+  IncrementErrorCount ();
+  Print (L"\nERROR : ");
+  Print 

Re: [edk2-devel] [PATCH v5 5/6] ShellPkg: acpiview: Add routines to print reserved fields

[Sami Mujawar]

Hi Rohit,

I have a minor suggestion marked inline as [SAMI].

Otherwise this patch looks good to me.

With that addressed,

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

On 02/10/2023 06:17 pm, Rohit Mathew wrote:

Most of the ACPI tables have fields that are marked reserved. Implement
functions "DumpReserved" and "DumpReservedBits" aligning with the
print-formatter prototype to print out reserved fields.

Signed-off-by: Rohit Mathew
Cc: James Morse
Cc: Sami Mujawar
Cc: Thomas Abraham
Cc: Zhichao Gao
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 126 

  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h |  38 ++
  2 files changed, 164 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 20d817e357..cc520d9b27 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -498,6 +498,132 @@ Dump16Chars (
  );
  }
  
+/**

+  This function traces reserved fields up to 8 bytes in length.
+
+  Format string is ignored by this function as the reserved field is printed
+  byte by byte with intermittent spacing . Use DumpxChars for any
+  other use case.
+  @param [in] Format  Optional format string for tracing the data.
+  @param [in] Ptr Pointer to the start of the buffer.
+  @param [in] Length  Length of the field.
+**/
+VOID
+EFIAPI
+DumpReserved (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr,
+  IN UINT32Length
+  )
+{
+  switch (Length) {
+case 8:
+  Print (
+L"%u %u %u %u %u %u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2],
+Ptr[3],
+Ptr[4],
+Ptr[5],
+Ptr[6],
+Ptr[7]
+);
+  break;
+case 7:
+  Print (
+L"%u %u %u %u %u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2],
+Ptr[3],
+Ptr[4],
+Ptr[5],
+Ptr[6]
+);
+  break;
+case 6:
+  Print (
+L"%u %u %u %u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2],
+Ptr[3],
+Ptr[4],
+Ptr[5]
+);
+  break;
+case 5:
+  Print (
+L"%u %u %u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2],
+Ptr[3],
+Ptr[4]
+);
+  break;
+case 4:
+  Print (
+L"%u %u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2],
+Ptr[3]
+);
+  break;
+case 3:
+  Print (
+L"%u %u %u",
+Ptr[0],
+Ptr[1],
+Ptr[2]
+);
+  break;
+case 2:
+  Print (
+L"%u %u",
+Ptr[0],
+Ptr[1]
+);
+  break;
+case 1:
+  Print (
+L"%u",
+Ptr[0]
+);
+  break;
+default:
+  return;
+  }
+}
+
+/**
+  This function traces reserved fields up to 64 bits in length.
+
+  Format string is ignored by this function as the reserved field is printed
+  byte by byte with intermittent spacing. eg: <0 0 0 0>. When the field length
+  isn't a multiple of 8, the number of bytes are "ceil"-ed by one. eg for 27
+  bits <0 0 0 0>
+
+  @param [in] Format  Optional format string for tracing the data.
+  @param [in] Ptr Pointer to the start of the buffer.
+  @param [in] Length  Length of the field.
[SAMI] I think the documentation for the Length filed should say 
something like 'Length of the field as number of bits.'

+**/
+VOID
+EFIAPI
+DumpReservedBits (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr,
+  IN UINT32Length
+  )
+{
+  UINT32  ByteLength;
+
+  ByteLength = (Length + 7) >> 3;
+  DumpReserved (Format, Ptr, ByteLength);
+}
+
  /**
This function indents and prints the ACPI table Field Name.
  
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

index 80bac6a584..89930fdc6d 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -230,6 +230,44 @@ Dump16Chars (
IN UINT32Length
);
  
+/**

+  This function traces reserved fields up to 8 bytes in length.
+
+  Format string is ignored by this function as the reserved field is printed
+  byte by byte with intermittent spacing . Use DumpxChars for any
+  other use case.
+  @param [in] Format  Optional format string for tracing the data.
+  @param [in] Ptr Pointer to the start of the buffer.
+  @param [in] Length  Length of the field.
+**/
+VOID
+EFIAPI
+DumpReserved (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr,
+  IN UINT32Length
+  );
+
+/**
+  This function traces reserved fields up to 64 bits in length.
+
+  Format string is ignored by this function as the reserved field is printed
+  byte by byte with intermittent spacing. eg: <0 0 0 0>. When the field length
+ 

Re: [edk2-devel] [PATCH v5 3/6] ShellPkg/AcpiView: Update print-formatter prototype

[Sami Mujawar]

Hi Rohit,

Thank you for this patch.
These changes look good to me.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

On 02/10/2023, 18:16, "Rohit Mathew" mailto:rohit.mat...@arm.com>> wrote:


As of now, the print-formatter implemented by the FNPTR_PRINT_FORMATTER
function pointer takes two parameters, the format string and the pointer
to the field. For cases where the print-formatter has to have access to
the length of the field, there is no clean way to currently do it. In
order to resolve this, update the print-formatter's prototype to take
the length of the field as a third parameter. This change should improve
the overall robustness and flexibility of AcpiView.


Signed-off-by: Rohit Mathew mailto:rohit.mat...@arm.com>>
Cc: James Morse mailto:james.mo...@arm.com>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Thomas Abraham mailto:thomas.abra...@arm.com>>
Cc: Zhichao Gao mailto:zhichao@intel.com>>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 28 
++--
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 27 
++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Aest/AestParser.c | 4 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c | 8 
--
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 4 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c | 4 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 4 ++-
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 12 
++---
8 files changed, 67 insertions(+), 24 deletions(-)


diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 6823ba60cf..e0ad78250a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -319,12 +319,14 @@ DumpUint64 (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
Dump3Chars (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
Print (
@@ -343,12 +345,14 @@ Dump3Chars (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
Dump4Chars (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
Print (
@@ -368,12 +372,14 @@ Dump4Chars (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
Dump6Chars (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
Print (
@@ -395,12 +401,14 @@ Dump6Chars (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
Dump8Chars (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
Print (
@@ -424,12 +432,14 @@ Dump8Chars (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
Dump12Chars (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
Print (
@@ -587,7 +597,7 @@ ParseAcpi (
// the Format for printing
PrintFieldName (2, Parser[Index].NameStr);
if (Parser[Index].PrintFormatter != NULL) {
- Parser[Index].PrintFormatter (Parser[Index].Format, Ptr);
+ Parser[Index].PrintFormatter (Parser[Index].Format, Ptr, 
Parser[Index].Length);
} else if (Parser[Index].Format != NULL) {
switch (Parser[Index].Length) {
case 1:
@@ -681,12 +691,14 @@ DumpGasStruct (


@param [in] Format Optional format string for tracing the data.
@param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the field.
**/
VOID
EFIAPI
DumpGas (
IN CONST CHAR16 *Format OPTIONAL,
- IN UINT8 *Ptr
+ IN UINT8 *Ptr,
+ IN UINT32 Length
)
{
DumpGasStruct (Ptr, 2, sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE));
@@ -892,7 +904,7 @@ ParseAcpiBitFields (
// the Format for printing
PrintFieldName (2, Parser[Index].NameStr);
if (Parser[Index].PrintFormatter != NULL) {
- Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *));
+ Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *), 
Parser[Index].Length);
} else if (Parser[Index].Format != NULL) {
// convert bit length to byte length
switch ((Parser[Index].Length + 7) >> 3) {
diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 6d8b44d94a..e760467a9d 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ 

Re: [edk2-devel] [PATCH v5 2/6] ShellPkg/AcpiView: Update field-validator prototype

[Sami Mujawar]

Hi Rohit,

Thank you for this patch.

I haev a minor suggestion to marked inline as [SAMI] which involves 
moving the Length as the second parameter in the validator function.


Unfortunately this would mean changes across the entire patch. But I 
think it provides better grouping of parameters.


With that addressed,

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

On 02/10/2023 06:15 pm, Rohit Mathew wrote:

As of now, the field-validator implemented by FNPTR_FIELD_VALIDATOR
function pointer takes two parameters, the pointer to the field and a
context pointer. For cases where the validator has to have access to the
length of the field, there is no clean way to currently do it. In order
to resolve this, this commit updates the field-validator's prototype to
take the length of the field as a third parameter.

This enhancement allows field validators to perform more comprehensive
validation, especially when the length of the field is critical to the
validation logic. This change should improve the overall robustness and
flexibility of AcpiView.

Signed-off-by: Rohit Mathew
Cc: James Morse
Cc: Sami Mujawar
Cc: Thomas Abraham
Cc: Zhichao Gao
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c  |  6 
+--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h  |  5 +-
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Aest/AestParser.c | 32 
+++
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |  8 
+--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c | 20 
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 20 
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 14 
+++--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c |  8 
+--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 32 
+++
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 14 
+++--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.c | 56 
+---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 38 
-
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 14 
+++--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c | 14 
+++--
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 14 
+++--
  15 files changed, 190 insertions(+), 105 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index eac9286176..6823ba60cf 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -1,7 +1,7 @@
  /** @file
ACPI parser
  
-  Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.

+  Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
Copyright (c) 2022, AMD Incorporated. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
  **/
@@ -616,7 +616,7 @@ ParseAcpi (
if (GetConsistencyChecking () &&
(Parser[Index].FieldValidator != NULL))
{
-Parser[Index].FieldValidator (Ptr, Parser[Index].Context);
+Parser[Index].FieldValidator (Ptr, Parser[Index].Context, 
Parser[Index].Length);
}
  
Print (L"\n");

@@ -927,7 +927,7 @@ ParseAcpiBitFields (
if (GetConsistencyChecking () &&
(Parser[Index].FieldValidator != NULL))
{
-Parser[Index].FieldValidator ((UINT8 *), Parser[Index].Context);
+Parser[Index].FieldValidator ((UINT8 *), Parser[Index].Context, 
Parser[Index].Length);
}
  
Print (L"\n");

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 4b4397961b..6d8b44d94a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -2,7 +2,7 @@
Header file for ACPI parser
  
Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.

-  Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
+  Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
Copyright (c) 2022, AMD Incorporated. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
  **/
@@ -237,8 +237,9 @@ typedef VOID (EFIAPI *FNPTR_PRINT_FORMATTER)(CONST CHAR16 
*Format, UINT8 *Ptr);
@param [in] Context Pointer to context specific information as specified by
the 'Context' member of the ACPI_PARSER.
e.g. this could be a pointer to the ACPI table header.
+  @param [in] Length  Length of the field.
  **/
-typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context);
+typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context, UINT32 
Length);

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Marcin Juszkiewicz]

W dniu 4.12.2023 o 13:58, Ard Biesheuvel pisze:

On Mon, 4 Dec 2023 at 13:38, Alexander Graf  wrote:

On 04.12.23 13:20, Gerd Hoffmann wrote:



I don't think it helps to go off on a tangent about why shim exists
and why it is so terrible, as I don't think there is actually any
disagreement about that. But now that we are, let me add my 2c as well
:-)

For the patch under discussion here, I think that Gerd's suggestion is
to have both a PCD and a QEMU variable, and use the PCD unless the
variable has a value. I'm on the fence here: I would like to
accommodate users, but adding another control that the distros are
just going to set and forget is just going to make the mess bigger.

What is even worse: arm64 system firmware will have to deal with this
as well, and disable the protocol in order to run distro installers.
And once the tightened MS requirements for NX compat come into effect,
they will have to add another workaround for this as well, and so
we'll probably end up with generations of arm64 hardware with a
'enable memory attributes protocol' option in their BIOS menus. And
guess what the default setting is likely to be?

I am quite disappointed with the complete lack of involvement from the
folks who develop and deploy shim, and instead, third parties (and
users) are the ones chasing me and people like Gerd (who work on QEMU
or EDK2 rather than shim) to clean up the mess.


I use 'sbsa-ref' with QEMU and upstream EDK2. And cannot use either RHEL 
9.3 nor CentOS Stream 9 installers because they hang.


And this is not the only platform where upstream EDK2 is used.

Sure, I can hack something, use Grub from Debian or Fedora and get 
things working but that's not solution.


Adding flags in 'Broken OS support' section of EDK2 settings feels like 
bad idea. Especially when EFI app generating issues is developed by 
company known for FOSS work.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112073): https://edk2.groups.io/g/devel/message/112073
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled

[Gerd Hoffmann]

  Hi,

> > I'm with you on that.  Unfortunately the boot loader team is not.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=2108083
> >
> > tl;dr: You can't boot Fedora in secure boot mode without microsoft keys
> > today.  grub.efi refuses to work without shim.efi, and shim.efi exists
> > only in a microsoft-signed version (which differed from rhel were a
> > second, redhat-signed shim binary exists).
> >
> > Oh, and the above applies to x86 only.  On arm fedora shim.efi is not
> > signed and grub.efi is signed with the (public) redhat test keys.
> 
> So what is holding Fedora back from providing a fixed shim binary if
> it doesn't need to be signed by Microsoft?

I'd love to have an serious answer for that question, but I havn't.
Usually I get either no answer, or something along the lines of
"-ENOTIME because of $otherwork".

Right now waiting for v6.7-final before sending a new shim.efi to
microsoft for signing and the desire to keep all archs in sync also
plays a role (I guess).

Technically there is no good reason, fedora even has a separate
shim-unsigned-$arch.rpm which could be up-to-date all the time on all
architectures, independent from the microsoft signing process.  But that
is right now at v15.6, which is not even the latest (v15.7) release.
And 15.7 is more than one year old already ...

> To be honest (and I know I am preaching to the choir here), the more i
> learn about this, the less I am inclined to make *any* accommodations
> whatsoever, given that the boot loader team obviously does not give a
> shit about their crappy boot chain.

Can understand that sentiment.  Problem is this hits the wrong people,
and the fallout goes beyond rhel + fedora because the rh team also
maintains upstream shim.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112072): https://edk2.groups.io/g/devel/message/112072
Mute This Topic: https://groups.io/mt/102967690/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg

[Ni, Ray]

Thanks,
Ray
From: devel@edk2.groups.io  On Behalf Of Chao Li
Sent: Monday, December 4, 2023 3:32 PM
To: devel@edk2.groups.io; Ni, Ray 
Cc: Dong, Eric ; Kumar, Rahul R ; 
Gerd Hoffmann ; Leif Lindholm ; 
Ard Biesheuvel ; Sami Mujawar 
; Sunil V L ; Warkentin, Andrei 

Subject: Re: [edk2-devel] [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to 
UefiCpuPkg


Hi Ray,

For this patch, I checked again and here are my opinions:

  1.  (Set|Get)MemoryRegionAttribute is difficult to merge together, because 
the parameters between the tow APIs are not similar. So I suggest they be 
independent.

[Ray] What I mean is to merge SetMemoryRegion(NonExec|ReadOly) to 
SetMemoryRegionAttribute(). Similarly, GetXXX can be merged as well. What’s 
your opinion?

  1.  The EfiAttributeConverse, GetMemoryRegionAttribute, 
SetMemoryRegionAttributes and ConfigureMemoryManagementUnit will be retained 
and other APIs will be removed. Because the functions expressed by other APIs 
can be completed though the retained API.

[Ray] I didn’t notice EfiAttributeConverse(). I guess callers may not need to 
know the architectural specific attributes. So EfiAttributeConverse() might be 
not needed as a public API.

  1.  You pointed out MEMORY_REGION_DESCRIPTOR have no one to construct it, do 
I need add a new API to construct it? Could it be named GetMemoryMapPolicy and 
accept a parameter with MEMORY_REGION_DESCRIPTOR** ?

[Ray] So the GetMemoryRegionAttribute() and SetMemoryRegionAttributes() are 
performed on the active translation table. ConfigureMemoryManagementUint() is 
to create a translation table with a list of memory attributes. How about the 
following idea?

[Ray] (Set|Get)MemoryRegionAttribute() are performed on a translation table 
buffer. And caller calls SetMemoryRegionAttribute() to modify the translation 
table buffer supplied as the parameter. With this, 
ConfigureMemoryManagementUnit() is not needed.

Hope to hear from you! :)

Thanks,
Chao
On 2023/11/30 10:25, Chao Li wrote:

Hi Ray,

Thanks for review, here are some of my thoughts:
On 2023/11/30 08:59, Ni, Ray wrote:

Chao,

Since the lib class is so general, I'd like to understand more details to make 
sure it can properly fit into any CPU arch.



In X86, cache setting is through MSRs and Page tables, and memory access 
control (read-only, not-present, non-executable) is through page tables.
Let me understand, 'cache setting' means does it access a certain 
address(probably a memory address) via cache? If so, I'd say the 'cache 
setting' should be a part of attributes.


This CpuMmuLib is to provide both services. How does LoongArch64 manage the 
cache settings and memory access control?

Is it proper to combine both services into one lib?
In LoongArch64, cache settings and memory access control are performed via page 
tables. Please check the patch 14 of this series.


If the backend silicon IP is the same one that supports the "one" lib design, 
can we refine the lib API a bit?
Yes, I think Attribute's instance family can be bear the memory access and 
cache setting. So what are you suggestions if we improve the lib API?


We have (Set|Get)MemoryRegionAttribute() and 
(Set|Clear)MemoryRegion(NoExec|ReadOnly). Can we merge them together?
Do you means the (Set|Get) merge together(differentiate Get or Set operations 
by parameters)? If so, I think it's OK, but maybe some existing instances will 
be modified together.


And the API ConfigureMemoryManagementUint() accepts MEMORY_REGION_DESCRIPTOR 
but none of other APIs helps to construct the descriptor.
Yes, currently, no one helps construct MEMORY_REGION_DESCRIPTOR. I think the 
construction of descriptors is not part of the API, it should be the localized 
or private when I design them. Do I need to add an API to construct descripters?


It seems to me the MmuLib is simply a combination of different random APIs.

It's not a well-designed library class.



We need more discussion to make it be able to be accommodated by other archs in 
future, at least by figuring out the path of X86, ARM.
Yes, the APIs looks like so fragmented and we should improve them. So we should 
talk more about this API, thanks.


Thanks,

Ray

-Original Message-

From: Chao Li 

Sent: Friday, November 17, 2023 6:00 PM

To: devel@edk2.groups.io

Cc: Dong, Eric ; Ni, Ray 
; Kumar,

Rahul R ; Gerd 
Hoffmann ;

Leif Lindholm ; 
Ard Biesheuvel

; Sami Mujawar 
;

Sunil V L ; 
Warkentin, Andrei



Subject: [PATCH v3 13/39] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg



Add a new header file CpuMmuLib.h, whitch is referenced from

ArmPkg/Include/Library/ArmMmuLib.h. Currently, only support for

LoongArch64 is added, 

Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe

[Ni, Ray]

Reviewed-by: Ray Ni 

I agree that with “static”, the “mAcpiTimerLib” prefix is not really needed. 
But having it does no harm

I remember that coding style doc says “static” instead of “STATIC”. “STATIC” 
was the old requirement.

Thanks,
Ray
From: Desimone, Nathaniel L 
Sent: Tuesday, December 5, 2023 6:13 AM
To: Pedro Falcato ; devel@edk2.groups.io
Cc: Ni, Ray ; Kinney, Michael D 
Subject: RE: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib 
incompatibility with XhciDxe


> -Original Message-

> From: Pedro Falcato mailto:pedro.falc...@gmail.com>>

> Sent: Monday, December 4, 2023 11:59 AM

> To: devel@edk2.groups.io; Desimone, Nathaniel L

> mailto:nathaniel.l.desim...@intel.com>>

> Cc: Ni, Ray mailto:ray...@intel.com>>; Kinney, Michael D

> mailto:michael.d.kin...@intel.com>>

> Subject: Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib

> incompatibility with XhciDxe

>

> On Mon, Dec 4, 2023 at 6:48 PM Nate DeSimone

> mailto:nathaniel.l.desim...@intel.com>> wrote:

> >

> > The DXE & MM standalone variant of AcpiTimerLib defines a global named

> > mPerformanceCounterFrequency. A global with an identical name is also

> > present in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

> >

> > Since XhciDxe has a dependency on TimerLib, this can cause link errors

> > due to the same symbol being defined twice if the platform DSC chooses

> > to use AcpiTimerLib as the TimerLib implementation for any given

> > platform.

> >

> > To resolve this, I have changed made the definition of

> > mPerformanceCounterFrequency to static and renamed it to

> > mAcpiTimerLibTscFrequency. Since this variable is not used outside of

> > the DxeStandaloneMmAcpiTimerLib.c compilation unit, there is no reason

> > to have it exported as a global.

> >

> > Cc: Ray Ni mailto:ray...@intel.com>>

> > Cc: Michael D Kinney 
> > mailto:michael.d.kin...@intel.com>>

> > Signed-off-by: Nate DeSimone 
> > mailto:nathaniel.l.desim...@intel.com>>

> > ---

> >  .../AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c | 18

> > +-

> >  1 file changed, 9 insertions(+), 9 deletions(-)

> >

> > diff --git

> > a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c

> > b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c

> > index 16ac48938f..ccceb8a649 100644

> > ---

> > a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c

> > +++

> b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.

> > +++ c

> > @@ -1,7 +1,7 @@

> >  /** @file

> >ACPI Timer implements one instance of Timer Library.

> >

> > -  Copyright (c) 2013 - 2018, Intel Corporation. All rights

> > reserved.

> > +  Copyright (c) 2013 - 2023, Intel Corporation. All rights

> > + reserved.

> >SPDX-License-Identifier: BSD-2-Clause-Patent

> >

> >  **/

> > @@ -11,6 +11,11 @@

> >  #include 

> >  #include 

> >

> > +//

> > +// Cached performance counter frequency // static UINT64

> > +mAcpiTimerLibTscFrequency = 0;

>

> I'd say you don't need to rename it if it's a static variable. Now the 
> identifier is

> 2x longer with no additional relevant information.



This is in response to Mike K's feedback: 
https://edk2.groups.io/g/devel/message/111966



>

> Aren't we supposed to use STATIC vs static, CONST vs const, etc? Annoyingly

> :/



Apparently not. Honestly, I lose track of what the current coding style is 
sometimes too.



>

> --

> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112070): https://edk2.groups.io/g/devel/message/112070
Mute This Topic: https://groups.io/mt/102976788/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg: Support customized FV Migration Information

[Ni, Ray]

> I prefer to refine the hob usage to
> clude all ToMigrateFvInfo data in one hob (rather than each FV publish their
> own) and a field "MigrateAll" in this hob for those platforms who have no
> performance concern. In future, if we want to retire Migration feature PCD, we
> just need replace "check PCD TRUE or FALSE" with "check Hob exists or not".
> What do you think this way?

I am ok with your approach.

Each FV entry can tell two requests:
* Migrate FV or not
* Preserve RAW FV or not

As long as the global flag can provide the two information, I am ok


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112069): https://edk2.groups.io/g/devel/message/112069
Mute This Topic: https://groups.io/mt/102908572/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-