StandaloneMmPkg comments

StandaloneMmPkg comments

[Marvin H?user]

Good day,

I have been reading through the recently imported StandaloneMmPkg and found three odd things.


  1.  GUID prefixes: GUIDs defined in StandaloneMmPkg.dec either have no common prefix at all ("gMmFv") or use the "gEfi" prefix. Maybe the MdeModulePkg-style "gEdkii" prefix could be used for a uniform style?
  2.  The "gEfiMmConfigurationProtocolGuid" name is common between Standalone (StandaloneMmPkg.dec) and Traditional (MdePkg.dec) MM context despite having a different value of course. Shouldn't the naming reflect which is traditional and which is Standalone? I haven't checked in depth, but which is chosen when a module consumes both MdePkg and StandaloneMmPkg?
  3.  While .dec already uses the "Mmram" naming scheme, its header declares the MemoryReserve GUID as "gEfiMmPeiS(!)mramMemoryReserveGuid". Furthermore, the header references the SMM CIS (which has been replaced with "MM CIS" as part of the renaming), while the GUID has changed and the structure does not match the deprecated specification anyway. May I suggest to turn this GUID into a "gEdkii"-style GUID and remove all references to the SMM CIS? Maybe use the "EDKII_" prefix for "EFI_MMRAM_HOB_DESCRIPTOR_BLOCK" too? I wanted to prepare a patch, but I cannot compile the package at the moment and don't want to risk submitting anything broken.

Thanks for your time!

Regards,
Marvin.

Re: StandaloneMmPkg comments

[Ard Biesheuvel]

On 21 July 2018 at 03:57, Marvin H?user <Marvin.Haeuser@outlook.com> wrote:
> Good day,
>
> I have been reading through the recently imported StandaloneMmPkg and found three odd things.
>
>
>   1.  GUID prefixes: GUIDs defined in StandaloneMmPkg.dec either have no common prefix at all ("gMmFv") or use the "gEfi" prefix. Maybe the MdeModulePkg-style "gEdkii" prefix could be used for a uniform style?

The gEdkii prefix is used for GUIDs that are not defined in the PI or
UEFI specs. The gEfi  prefix is used for GUIDs that are defined in
either of those specs. I'm not sure what the rule is for prefixless
GUIDs though.

>   2.  The "gEfiMmConfigurationProtocolGuid" name is common between Standalone (StandaloneMmPkg.dec) and Traditional (MdePkg.dec) MM context despite having a different value of course. Shouldn't the naming reflect which is traditional and which is Standalone? I haven't checked in depth, but which is chosen when a module consumes both MdePkg and StandaloneMmPkg?

That smells like a bug to me.

>   3.  While .dec already uses the "Mmram" naming scheme, its header declares the MemoryReserve GUID as "gEfiMmPeiS(!)mramMemoryReserveGuid". Furthermore, the header references the SMM CIS (which has been replaced with "MM CIS" as part of the renaming), while the GUID has changed and the structure does not match the deprecated specification anyway. May I suggest to turn this GUID into a "gEdkii"-style GUID and remove all references to the SMM CIS? Maybe use the "EDKII_" prefix for "EFI_MMRAM_HOB_DESCRIPTOR_BLOCK" too? I wanted to prepare a patch, but I cannot compile the package at the moment and don't want to risk submitting anything broken.
>

As mentioned above, EDKII prefixes are inappropriate here, since
standalone MM is defined in the PI spec. I will let others comment on
the SMM CIS thing.

Re: StandaloneMmPkg comments

[Marvin Häuser]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Saturday, July 21, 2018 5:51 PM
> To: Marvin H?user <Marvin.Haeuser@outlook.com>
> Cc: edk2-devel@lists.01.org; achin.gupta@arm.com; jiewen.yao@intel.com;
> supreeth.venkatesh@arm.com
> Subject: Re: [edk2] StandaloneMmPkg comments
> 
> On 21 July 2018 at 03:57, Marvin H?user <Marvin.Haeuser@outlook.com>
> wrote:
> > Good day,
> >
> > I have been reading through the recently imported StandaloneMmPkg and
> found three odd things.
> >
> >
> >   1.  GUID prefixes: GUIDs defined in StandaloneMmPkg.dec either have no
> common prefix at all ("gMmFv") or use the "gEfi" prefix. Maybe the
> MdeModulePkg-style "gEdkii" prefix could be used for a uniform style?
> 
> The gEdkii prefix is used for GUIDs that are not defined in the PI or UEFI
> specs. The gEfi  prefix is used for GUIDs that are defined in either of those
> specs. I'm not sure what the rule is for prefixless GUIDs though.

I'm not aware of the GUIDs (here and for 3.) being defined in any UEFI specification, they seem to be package-defined, hence I thought "gEdkii" would be appropriate.

> 
> >   2.  The "gEfiMmConfigurationProtocolGuid" name is common between
> Standalone (StandaloneMmPkg.dec) and Traditional (MdePkg.dec) MM
> context despite having a different value of course. Shouldn't the naming
> reflect which is traditional and which is Standalone? I haven't checked in
> depth, but which is chosen when a module consumes both MdePkg and
> StandaloneMmPkg?
> 
> That smells like a bug to me.

The name is actually defined twice in the specification too. I'm assuming the authors intended for the appropriate GUID to be used depending on the consuming module. Maybe something like "[Protocols.common.MM_STANDALONE]" should be introduced?

> 
> >   3.  While .dec already uses the "Mmram" naming scheme, its header
> declares the MemoryReserve GUID as
> "gEfiMmPeiS(!)mramMemoryReserveGuid". Furthermore, the header
> references the SMM CIS (which has been replaced with "MM CIS" as part of
> the renaming), while the GUID has changed and the structure does not
> match the deprecated specification anyway. May I suggest to turn this GUID
> into a "gEdkii"-style GUID and remove all references to the SMM CIS? Maybe
> use the "EDKII_" prefix for "EFI_MMRAM_HOB_DESCRIPTOR_BLOCK" too? I
> wanted to prepare a patch, but I cannot compile the package at the moment
> and don't want to risk submitting anything broken.
> >
> 
> As mentioned above, EDKII prefixes are inappropriate here, since standalone
> MM is defined in the PI spec. I will let others comment on the SMM CIS thing.

Re: StandaloneMmPkg comments

[Supreeth Venkatesh]

On Sun, 2018-07-22 at 00:51 +0900, Ard Biesheuvel wrote:
> On 21 July 2018 at 03:57, Marvin H?user <Marvin.Haeuser@outlook.com>
> wrote:
> > 
> > Good day,
> > 
> > I have been reading through the recently imported StandaloneMmPkg
> > and found three odd things.
> > 
> > 
> >   1.  GUID prefixes: GUIDs defined in StandaloneMmPkg.dec either
> > have no common prefix at all ("gMmFv") or use the "gEfi" prefix.
> > Maybe the MdeModulePkg-style "gEdkii" prefix could be used for a
> > uniform style?
> The gEdkii prefix is used for GUIDs that are not defined in the PI or
> UEFI specs. The gEfi  prefix is used for GUIDs that are defined in
> either of those specs. I'm not sure what the rule is for prefixless
> GUIDs though.
> 
> > 
> >   2.  The "gEfiMmConfigurationProtocolGuid" name is common between
> > Standalone (StandaloneMmPkg.dec) and Traditional (MdePkg.dec) MM
> > context despite having a different value of course. Shouldn't the
> > naming reflect which is traditional and which is Standalone? I
> > haven't checked in depth, but which is chosen when a module
> > consumes both MdePkg and StandaloneMmPkg?
> That smells like a bug to me.
Sorry for the delayed response as I am catching up on emails after
vacation. 
I think that the naming should reflect which is traditional and which
is Standalone even when traditional and standalone are supposed to be
mutually exclusive.
MdePkg.dec has this define
gEfiMmConfigurationProtocolGuid= { 0x26eeb3de, 0xb689, 0x492e, { 0x80,
0xf0, 0xbe, 0x8b, 0xd7, 0xda, 0x4b, 0xa7 }} which is defined in PI Spec
v1.6 vol 4 in section 5.5. 


However, PI spec v1.6 vol 4 defines EFI_MM_CONFIGURATION_PROTOCOL_GUID
once more in section 4.8 (Standalone MM context) as
#define EFI_MM_CONFIGURATION_PROTOCOL_GUID { \
0xc109319, 0xc149, 0x450e, 0xa3, 0xe3, 0xb9, 0xba, 0xdd, 0x9d, 0xc3,
0xa4 \
}

Furthermore, it defines in Appendix A (Management Mode Backward
Compatibility Types):
#define EFI_SMM_CONFIGURATION_PROTOCOL_GUID
EFI_MM_CONFIGURATION_PROTOCOL_GUID.

It seems like an issue with the Specification. I have raised an ECR to
get some clarification.
https://mantis.uefi.org/mantis/view.php?id=1940

 
> 
> > 
> >   3.  While .dec already uses the "Mmram" naming scheme, its header
> > declares the MemoryReserve GUID as
> > "gEfiMmPeiS(!)mramMemoryReserveGuid". Furthermore, the header
> > references the SMM CIS (which has been replaced with "MM CIS" as
> > part of the renaming), while the GUID has changed and the structure
> > does not match the deprecated specification anyway. May I suggest
> > to turn this GUID into a "gEdkii"-style GUID and remove all
> > references to the SMM CIS? Maybe use the "EDKII_" prefix for
> > "EFI_MMRAM_HOB_DESCRIPTOR_BLOCK" too? I wanted to prepare a patch,
> > but I cannot compile the package at the moment and don't want to
> > risk submitting anything broken.
> > 
> As mentioned above, EDKII prefixes are inappropriate here, since
> standalone MM is defined in the PI spec. I will let others comment on
> the SMM CIS thing.