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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 5 Mar 2019 at 17:53, Felix Polyudov <Felixp@ami.com> wrote:
>
> There is an architectural difference between EndOfDxe and SmmReadyToLock events.
> It's important to have both of them.
> Here is what PI specification says:
> ==
> Transition from the environment where all of the components are under the authority of the platform manufacturer to the environment where third party modules are executed is a two-step process:
> 1.End of DXE Event is signaled. This event presents the last opportunity to use resources or interfaces that are going to be locked or disabled in anticipation of the invocation of 3rd party extensible modules.
> 2.DXE SMM Ready to Lock Protocol is installed. PI modules that need to lock or protect their resources in anticipation of the invocation of 3rd party extensible modules should register for notification on installation of this protocol and effect the appropriate protections in their notification handlers

Thanks for pointing that out, Felix. I will add SmmReadyToLock as well.


> ==
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Tuesday, March 05, 2019 11:19 AM
> To: Ard Biesheuvel
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 10/10] ArmPkg/MmCommunicationDxe: signal architected PI events into MM context
>
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.