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: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