Re: [PATCH 10/10] ArmPkg/MmCommunicationDxe: signal architected PI events into MM context

["Yao, Jiewen" <jiewen.yao@intel.com>]

For current X86 world, we do use both SmmReadyToLock and EndOfDxe, for almost all X86 platform.

So, let me clarify:
If we try to align with PI spec, we can add EndOfDxe/ReadyToBoot/ExitBootService.
If we try to align with security, we can add EndOfDxe/SmmReadyToLock.

It depends upon the what goal we want to achieve. That is why I ask if we have specific use case.

Anyway, I think we can add when it is really needed later.
So I am OK with current patch.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Tuesday, March 5, 2019 8:07 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 10/10] ArmPkg/MmCommunicationDxe: signal
> architected PI events into MM context
> 
> On Tue, 5 Mar 2019 at 17:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > OK. To keep the compatibility of existing MM driver. That makes sense.
> >
> > If it is for security, I think EndOfDxe is the only point.
> > ReadyToBoot and ExitBootService cannot be used for security purpose.
> >
> 
> OK, good to know. I will keep them for the time being - MM drivers may
> be able to release resources or do other useful things when the
> non-secure side enters runtime mode.
> 
> > Then do we need SmmReadyToLock ? :-)
> >
> 
> Good point. It looked fairly x86 specific to me. Do you think it is
> likely to be used in OEM code running in MM mode?
> 
> 
> 
> 
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> Of
> > > Ard Biesheuvel
> > > Sent: Tuesday, March 5, 2019 7:58 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [PATCH 10/10] ArmPkg/MmCommunicationDxe:
> signal
> > > architected PI events into MM context
> > >
> > > On Tue, 5 Mar 2019 at 16:56, Yao, Jiewen <jiewen.yao@intel.com>
> wrote:
> > > >
> > > > Hi
> > > > In original SMM infrastructure, there are lots of interaction that SMM
> has
> > > to know the DXE status.
> > > >
> > > > In StandaloneMm, I don't expect we have many interaction. Is there
> any
> > > specific example?
> > > >
> > > > I am totally OK to add those. And I just want to know more usage.
> > > >
> > > > Reviewed-by: Jiewen.yao@intel.com
> > > >
> > >
> > > Jiewen,
> > >
> > > Thanks for the review.
> > >
> > > It is not 100% clear at the moment, but since existing third party
> > > software designed to run in MM context may make assumptions about
> > > security of the platform (e.g., before vs after end of dxe) based on
> > > these events, we should at least signal the common ones added in this
> > > patch.
> > >
> > >
> > >
> > >
> > > > > -----Original Message-----
> > > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > > > Sent: Tuesday, March 5, 2019 5:33 AM
> > > > > To: edk2-devel@lists.01.org
> > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Achin Gupta
> > > > > <achin.gupta@arm.com>; Supreeth Venkatesh
> > > > > <supreeth.venkatesh@arm.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> > > > > Leif Lindholm <leif.lindholm@linaro.org>; Jagadeesh Ujja
> > > > > <jagadeesh.ujja@arm.com>
> > > > > Subject: [PATCH 10/10] ArmPkg/MmCommunicationDxe: signal
> > > architected
> > > > > PI events into MM context
> > > > >
> > > > > PI defines a few architected events that have significance in the MM
> > > > > context as well as in the non-secure DXE context. So register notify
> > > > > handlers for these events, and relay them into the standalone MM
> world.
> > > > >
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > ---
> > > > >  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf |
> 5
> > > +++
> > > > >  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c   |
> 47
> > > > > +++++++++++++++++++-
> > > > >  2 files changed, 50 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git
> > > a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> > > > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> > > > > index 88beafa39c05..8bf269270f9d 100644
> > > > > --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> > > > > +++
> b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> > > > > @@ -48,6 +48,11 @@ [LibraryClasses]
> > > > >  [Protocols]
> > > > >    gEfiMmCommunicationProtocolGuid              ##
> PRODUCES
> > > > >
> > > > > +[Guids]
> > > > > +  gEfiEndOfDxeEventGroupGuid
> > > > > +  gEfiEventExitBootServicesGuid
> > > > > +  gEfiEventReadyToBootGuid
> > > > > +
> > > > >  [Pcd.common]
> > > > >    gArmTokenSpaceGuid.PcdMmBufferBase
> > > > >    gArmTokenSpaceGuid.PcdMmBufferSize
> > > > > diff --git
> > > a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > > index feb9fa9f4ead..3203cf801a19 100644
> > > > > --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > > @@ -265,6 +265,43 @@ GetMmCompatibility ()
> > > > >    return Status;
> > > > >  }
> > > > >
> > > > > +STATIC EFI_GUID* CONST mGuidedEventGuid[] = {
> > > > > +  &gEfiEndOfDxeEventGroupGuid,
> > > > > +  &gEfiEventExitBootServicesGuid,
> > > > > +  &gEfiEventReadyToBootGuid,
> > > > > +};
> > > > > +
> > > > > +STATIC EFI_EVENT mGuidedEvent[ARRAY_SIZE
> (mGuidedEventGuid)];
> > > > > +
> > > > > +/**
> > > > > +  Event notification that is fired when GUIDed Event Group is
> signaled.
> > > > > +
> > > > > +  @param  Event                 The Event that is being
> > > processed,
> > > > > not used.
> > > > > +  @param  Context               Event Context, not used.
> > > > > +
> > > > > +**/
> > > > > +STATIC
> > > > > +VOID
> > > > > +EFIAPI
> > > > > +MmGuidedEventNotify (
> > > > > +  IN EFI_EVENT  Event,
> > > > > +  IN VOID       *Context
> > > > > +  )
> > > > > +{
> > > > > +  EFI_MM_COMMUNICATE_HEADER   Header;
> > > > > +  UINTN                       Size;
> > > > > +
> > > > > +  //
> > > > > +  // Use Guid to initialize EFI_SMM_COMMUNICATE_HEADER
> structure
> > > > > +  //
> > > > > +  CopyGuid (&Header.HeaderGuid, Context);
> > > > > +  Header.MessageLength = 1;
> > > > > +  Header.Data[0] = 0;
> > > > > +
> > > > > +  Size = sizeof (Header);
> > > > > +  MmCommunicationCommunicate (&mMmCommunication,
> &Header,
> > > > > &Size);
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >    The Entry Point for MM Communication
> > > > >
> > > > > @@ -287,6 +324,7 @@ MmCommunicationInitialize (
> > > > >    )
> > > > >  {
> > > > >    EFI_STATUS                 Status;
> > > > > +  UINTN                      Index;
> > > > >
> > > > >    // Check if we can make the MM call
> > > > >    Status = GetMmCompatibility ();
> > > > > @@ -351,8 +389,13 @@ MmCommunicationInitialize (
> > > > >                    NULL,
> > > > >                    &mSetVirtualAddressMapEvent
> > > > >                    );
> > > > > -  if (Status == EFI_SUCCESS) {
> > > > > -    return Status;
> > > > > +  ASSERT_EFI_ERROR (Status);
> > > > > +
> > > > > +  for (Index = 0; Index < ARRAY_SIZE (mGuidedEventGuid); Index++)
> {
> > > > > +    Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL,
> > > TPL_CALLBACK,
> > > > > +                    MmGuidedEventNotify,
> > > mGuidedEventGuid[Index],
> > > > > +                    mGuidedEventGuid[Index],
> > > > > &mGuidedEvent[Index]);
> > > > > +    ASSERT_EFI_ERROR (Status);
> > > > >    }
> > > > >
> > > > >    gBS->UninstallProtocolInterface (
> > > > > --
> > > > > 2.20.1
> > > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel