subject:"\[edk2\] \[PATCH 7\/8\] OvmfPkg\/SmmAccessPei\: Update MM PPI usages."

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

2018-07-24 Thread Laszlo Ersek

On 07/24/18 14:16, Marvin Häuser wrote:
> Hey Laszlo,
> 
>> -Original Message-

>> (1) Please post patch sets using a cover letter message:
>>
>> git config format.coverletter true
>> git config format.numberedtrue
>> git config sendemail.chainreplyto false
>> git config sendemail.thread   true
>>
>> Otherwise the individual patches in the series will get intermixed with other
>> messages / threads on the mailing list (and in people's inboxes), if the
>> recipient uses a threaded view in their MUA (which most people should do --
>> patches and discussions are very hard to follow otherwise).
> 
> Is V2 fine in this aspect? Outlook has never threaded multiple parts of a 
> patch series for me but only replies to their original message, hence I did 
> not notice something was off.

It doesn't look right to me, looking at the following patches patches:

[edk2] [PATCH v2 0/2] Introduce UEFI PI 1.5 MM PPI.
[edk2] [PATCH v2 1/2] MdePkg: Add PI 1.5 MM PPI declarations.
[edk2] [PATCH v2 2/2] MdeModulePkg: Change SMM PPIs to shim MM PPIs.

* v2 0/2:

Message-ID:

* v2 1/2:

Message-ID:

References: 

In-Reply-To: 

* v2 2/2:

Message-ID:

References: 

In-Reply-To: 

Here's what I think happened: the original message-id (formatted by your
local tooling) for the cover letter was

Accordingly, your local tools placed this message ID *correctly* into
both "In-Reply-To" and "References" headers of the actual patches #1 and
#2. This is the proper way to ensure threading.

Unfortunately, outlook.com's SMTP server seems to have overwritten your
Message-ID, on the cover letter (and the rest of the emails too),
despite your tools generating and embedding Message-ID in the first
place. Obviously this breaks threading. This is a bug in outlook.com's
SMTP service, most likely.

Can you open a support ticket with them, or else use something else for
posting patches than outlook.com?

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

2018-07-24 Thread Marvin Häuser

Hey Laszlo,

> -Original Message-
> From: Laszlo Ersek 
> Sent: Tuesday, July 24, 2018 11:20 AM
> To: Marvin Häuser ; edk2-
> de...@lists.01.org
> Cc: michael.d.kin...@intel.com; liming@intel.com;
> star.z...@intel.com; eric.d...@intel.com; ruiyu...@intel.com;
> kelly.ste...@intel.com; jordan.l.jus...@intel.com;
> ard.biesheu...@linaro.org
> Subject: Re: [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.
> 
> Hi Marvin,
> 
> On 07/24/18 03:40, Marvin Häuser wrote:
> > Update all references to the SMM PPIs from MdeModulePkg to rather use
> > MdePkg's MM PPI declarations.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser 
> > ---
> >  OvmfPkg/SmmAccess/SmmAccessPei.c   | 90 ++--
> >  OvmfPkg/SmmAccess/SmramInternal.c  |  8 +-
> > OvmfPkg/SmmAccess/SmmAccessPei.inf |  2 +-
> >  3 files changed, 50 insertions(+), 50 deletions(-)
> 
> two comments:
> 
> (1) Please post patch sets using a cover letter message:
> 
> git config format.coverletter true
> git config format.numberedtrue
> git config sendemail.chainreplyto false
> git config sendemail.thread   true
> 
> Otherwise the individual patches in the series will get intermixed with other
> messages / threads on the mailing list (and in people's inboxes), if the
> recipient uses a threaded view in their MUA (which most people should do --
> patches and discussions are very hard to follow otherwise).

Is V2 fine in this aspect? Outlook has never threaded multiple parts of a patch 
series for me but only replies to their original message, hence I did not 
notice something was off.

> 
> (2) More specifically, I disagree with this patch. I don't see what's wrong 
> with
> calling SMM "SMM" on x86 (or with calling SMI "SMI"). I understand that the
> PI spec now calls those things MM and MMI, and I tend to agree that new
> code (especially ARM/AARCH64 code) should use these more generic terms
> from the most recent PI spec releases. But, this is not new code, and it is 
> for
> x86; I don't see why we should forcefully drag it forward. I don't think the
> edk2 headers plan to remove the original / traditional names.
> 
> (Just the other day, I learned that EFI_FILE_HANDLE was basically a typedef
> for "pointer to EFI_FILE_PROTOCOL" -- it's a non-standard type, yet it's
> defined in "MdePkg/Include/Protocol/SimpleFileSystem.h", and it's used in
> e.g.
> - MdeModulePkg/Library/BootMaintenanceManagerUiLib
> - MdeModulePkg/Library/FileExplorerLib
> - MdeModulePkg/Universal/Disk/RamDiskDxe
> - MdeModulePkg/Universal/EbcDxe
> - MdePkg/Library/DxeServicesLib
> - MdePkg/Library/UefiFileHandleLib
> - NetworkPkg/TlsAuthConfigDxe
> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
> 
> Should we use EFI_FILE_HANDLE in new code we write? No, we shouldn't.
> Should we embark on a journey to eradicate EFI_FILE_HANDLE? I would
> disagree with that idea too.)
> 
> Obviously I'm not arguing against migrating other packages to the new
> names, if their respective maintainers like the idea. I don't think it's 
> useful for
> OvmfPkg though.
> 
> Thanks
> Laszlo
> 
> > diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c
> > b/OvmfPkg/SmmAccess/SmmAccessPei.c
> > index 21119f80eefa..340122d6a598 100644
> > --- a/OvmfPkg/SmmAccess/SmmAccessPei.c
> > +++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
> > @@ -3,7 +3,7 @@
> >A PEIM with the following responsibilities:
> >
> >- verify & configure the Q35 TSEG in the entry point,
> > -  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
> > +  - provide MMRAM access by producing EFI_PEI_MM_ACCESS_PPI,
> >- set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG,
> and expose
> >  it via the gEfiAcpiVariableGuid GUID HOB.
> >
> > @@ -32,28 +32,28 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> > +#include 
> >
> >  #include 
> >
> >  #include "SmramInternal.h"
> >
> >  //
> > -// PEI_SMM_ACCESS_PPI implementation.
> > +// EFI_PEI_MM_ACCESS_PPI implementation.
> >  //
> >
> >  /**
> > -  Opens the SMRAM area to be accessible by a PEIM driver.
> > +  Opens the MMRAM area to be accessible by a PEIM driver.
> >
> > -  This function "opens" SMRAM so that it is visible while not inside of 
> > SMM.
> > +  This function "opens" MMRAM so that it is visible while not inside of
> MM.
> >The function should return EFI_UNSUPPORTED if the hardware does not
> > support
> > -  hiding of SMRAM. The function should return EFI_DEVICE_ERROR if the
> > SMRAM
> > +  hiding of MMRAM. The function should return EFI_DEVICE_ERROR if the
> > + MMRAM
> >configuration is locked.
> >
> >@param  PeiServicesGeneral purpose services available to 
> > every
> >   PEIM.
> > -  @param  This   The pointer to the SMM Access Interface.
> > -  @param  DescriptorIndexThe region of SMRAM to Open.
> > +  @param  This   The pointer to the MM Access Interface.
>

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

2018-07-24 Thread Laszlo Ersek

Hi Marvin,

On 07/24/18 03:40, Marvin Häuser wrote:
> Update all references to the SMM PPIs from MdeModulePkg to rather use
> MdePkg's MM PPI declarations.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser 
> ---
>  OvmfPkg/SmmAccess/SmmAccessPei.c   | 90 ++--
>  OvmfPkg/SmmAccess/SmramInternal.c  |  8 +-
>  OvmfPkg/SmmAccess/SmmAccessPei.inf |  2 +-
>  3 files changed, 50 insertions(+), 50 deletions(-)

two comments:

(1) Please post patch sets using a cover letter message:

git config format.coverletter true
git config format.numberedtrue
git config sendemail.chainreplyto false
git config sendemail.thread   true

Otherwise the individual patches in the series will get intermixed with
other messages / threads on the mailing list (and in people's inboxes),
if the recipient uses a threaded view in their MUA (which most people
should do -- patches and discussions are very hard to follow otherwise).

(2) More specifically, I disagree with this patch. I don't see what's
wrong with calling SMM "SMM" on x86 (or with calling SMI "SMI"). I
understand that the PI spec now calls those things MM and MMI, and I
tend to agree that new code (especially ARM/AARCH64 code) should use
these more generic terms from the most recent PI spec releases. But,
this is not new code, and it is for x86; I don't see why we should
forcefully drag it forward. I don't think the edk2 headers plan to
remove the original / traditional names.

(Just the other day, I learned that EFI_FILE_HANDLE was basically a
typedef for "pointer to EFI_FILE_PROTOCOL" -- it's a non-standard type,
yet it's defined in "MdePkg/Include/Protocol/SimpleFileSystem.h", and
it's used in e.g.
- MdeModulePkg/Library/BootMaintenanceManagerUiLib
- MdeModulePkg/Library/FileExplorerLib
- MdeModulePkg/Universal/Disk/RamDiskDxe
- MdeModulePkg/Universal/EbcDxe
- MdePkg/Library/DxeServicesLib
- MdePkg/Library/UefiFileHandleLib
- NetworkPkg/TlsAuthConfigDxe
- SecurityPkg/VariableAuthenticated/SecureBootConfigDxe

Should we use EFI_FILE_HANDLE in new code we write? No, we shouldn't.
Should we embark on a journey to eradicate EFI_FILE_HANDLE? I would
disagree with that idea too.)

Obviously I'm not arguing against migrating other packages to the new
names, if their respective maintainers like the idea. I don't think it's
useful for OvmfPkg though.

Thanks
Laszlo

> diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c 
> b/OvmfPkg/SmmAccess/SmmAccessPei.c
> index 21119f80eefa..340122d6a598 100644
> --- a/OvmfPkg/SmmAccess/SmmAccessPei.c
> +++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
> @@ -3,7 +3,7 @@
>A PEIM with the following responsibilities:
>  
>- verify & configure the Q35 TSEG in the entry point,
> -  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
> +  - provide MMRAM access by producing EFI_PEI_MM_ACCESS_PPI,
>- set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and 
> expose
>  it via the gEfiAcpiVariableGuid GUID HOB.
>  
> @@ -32,28 +32,28 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  #include 
>  
>  #include "SmramInternal.h"
>  
>  //
> -// PEI_SMM_ACCESS_PPI implementation.
> +// EFI_PEI_MM_ACCESS_PPI implementation.
>  //
>  
>  /**
> -  Opens the SMRAM area to be accessible by a PEIM driver.
> +  Opens the MMRAM area to be accessible by a PEIM driver.
>  
> -  This function "opens" SMRAM so that it is visible while not inside of SMM.
> +  This function "opens" MMRAM so that it is visible while not inside of MM.
>The function should return EFI_UNSUPPORTED if the hardware does not support
> -  hiding of SMRAM. The function should return EFI_DEVICE_ERROR if the SMRAM
> +  hiding of MMRAM. The function should return EFI_DEVICE_ERROR if the MMRAM
>configuration is locked.
>  
>@param  PeiServicesGeneral purpose services available to every
>   PEIM.
> -  @param  This   The pointer to the SMM Access Interface.
> -  @param  DescriptorIndexThe region of SMRAM to Open.
> +  @param  This   The pointer to the MM Access Interface.
> +  @param  DescriptorIndexThe region of MMRAM to Open.
>  
>@retval EFI_SUCCESSThe region was successfully opened.
>@retval EFI_DEVICE_ERROR   The region could not be opened because 
> locked
> @@ -64,9 +64,9 @@
>  STATIC
>  EFI_STATUS
>  EFIAPI
> -SmmAccessPeiOpen (
> +MmAccessPeiOpen (
>IN EFI_PEI_SERVICES**PeiServices,
> -  IN PEI_SMM_ACCESS_PPI  *This,
> +  IN EFI_PEI_MM_ACCESS_PPI   *This,
>IN UINTN   DescriptorIndex
>)
>  {
> @@ -82,16 +82,16 @@ SmmAccessPeiOpen (
>  }
>  
>  /**
> -  Inhibits access to the SMRAM.
> +  Inhibits access to the MMRAM.
>  
> -  This function "closes" SMRAM so that it is not visible while outside of 
> SMM.
> +  This function "closes" MMRAM so that it is not visible while outside of MM.
>

[edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

2018-07-23 Thread Marvin Häuser

Update all references to the SMM PPIs from MdeModulePkg to rather use
MdePkg's MM PPI declarations.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser 
---
 OvmfPkg/SmmAccess/SmmAccessPei.c   | 90 ++--
 OvmfPkg/SmmAccess/SmramInternal.c  |  8 +-
 OvmfPkg/SmmAccess/SmmAccessPei.inf |  2 +-
 3 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
index 21119f80eefa..340122d6a598 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.c
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
@@ -3,7 +3,7 @@
   A PEIM with the following responsibilities:
 
   - verify & configure the Q35 TSEG in the entry point,
-  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
+  - provide MMRAM access by producing EFI_PEI_MM_ACCESS_PPI,
   - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose
 it via the gEfiAcpiVariableGuid GUID HOB.
 
@@ -32,28 +32,28 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #include 
 
 #include "SmramInternal.h"
 
 //
-// PEI_SMM_ACCESS_PPI implementation.
+// EFI_PEI_MM_ACCESS_PPI implementation.
 //
 
 /**
-  Opens the SMRAM area to be accessible by a PEIM driver.
+  Opens the MMRAM area to be accessible by a PEIM driver.
 
-  This function "opens" SMRAM so that it is visible while not inside of SMM.
+  This function "opens" MMRAM so that it is visible while not inside of MM.
   The function should return EFI_UNSUPPORTED if the hardware does not support
-  hiding of SMRAM. The function should return EFI_DEVICE_ERROR if the SMRAM
+  hiding of MMRAM. The function should return EFI_DEVICE_ERROR if the MMRAM
   configuration is locked.
 
   @param  PeiServicesGeneral purpose services available to every
  PEIM.
-  @param  This   The pointer to the SMM Access Interface.
-  @param  DescriptorIndexThe region of SMRAM to Open.
+  @param  This   The pointer to the MM Access Interface.
+  @param  DescriptorIndexThe region of MMRAM to Open.
 
   @retval EFI_SUCCESSThe region was successfully opened.
   @retval EFI_DEVICE_ERROR   The region could not be opened because locked
@@ -64,9 +64,9 @@
 STATIC
 EFI_STATUS
 EFIAPI
-SmmAccessPeiOpen (
+MmAccessPeiOpen (
   IN EFI_PEI_SERVICES**PeiServices,
-  IN PEI_SMM_ACCESS_PPI  *This,
+  IN EFI_PEI_MM_ACCESS_PPI   *This,
   IN UINTN   DescriptorIndex
   )
 {
@@ -82,16 +82,16 @@ SmmAccessPeiOpen (
 }
 
 /**
-  Inhibits access to the SMRAM.
+  Inhibits access to the MMRAM.
 
-  This function "closes" SMRAM so that it is not visible while outside of SMM.
+  This function "closes" MMRAM so that it is not visible while outside of MM.
   The function should return EFI_UNSUPPORTED if the hardware does not support
-  hiding of SMRAM.
+  hiding of MMRAM.
 
   @param  PeiServices  General purpose services available to every
PEIM.
-  @param  This The pointer to the SMM Access Interface.
-  @param  DescriptorIndex  The region of SMRAM to Close.
+  @param  This The pointer to the MM Access Interface.
+  @param  DescriptorIndex  The region of MMRAM to Close.
 
   @retval EFI_SUCCESS  The region was successfully closed.
   @retval EFI_DEVICE_ERROR The region could not be closed because
@@ -102,9 +102,9 @@ SmmAccessPeiOpen (
 STATIC
 EFI_STATUS
 EFIAPI
-SmmAccessPeiClose (
+MmAccessPeiClose (
   IN EFI_PEI_SERVICES**PeiServices,
-  IN PEI_SMM_ACCESS_PPI  *This,
+  IN EFI_PEI_MM_ACCESS_PPI   *This,
   IN UINTN   DescriptorIndex
   )
 {
@@ -120,15 +120,15 @@ SmmAccessPeiClose (
 }
 
 /**
-  Inhibits access to the SMRAM.
+  Inhibits access to the MMRAM.
 
-  This function prohibits access to the SMRAM region.  This function is usually
+  This function prohibits access to the MMRAM region.  This function is usually
   implemented such that it is a write-once operation.
 
   @param  PeiServices  General purpose services available to every
PEIM.
-  @param  This The pointer to the SMM Access Interface.
-  @param  DescriptorIndex  The region of SMRAM to Close.
+  @param  This The pointer to the MM Access Interface.
+  @param  DescriptorIndex  The region of MMRAM to Close.
 
   @retval EFI_SUCCESSThe region was successfully locked.
   @retval EFI_DEVICE_ERROR   The region could not be locked because at
@@ -139,9 +139,9 @@ SmmAccessPeiClose (
 STATIC
 EFI_STATUS
 EFIAPI
-SmmAccessPeiLock (
+MmAccessPeiLock (
   IN EFI_PEI_SERVICES**PeiServices,
-  IN PEI_SMM_ACCESS_PPI  *This,
+  IN EFI_PEI_MM_ACCESS_PPI   *This,
   IN UINTN

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

Re: [edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

[edk2] [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

4 matches

Site Navigation

Mail list logo

Footer information