[PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[Ard Biesheuvel]

Commit 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI
events to MM context") update the ARM specific standalone MM client
driver to register for certain events in the entrypoint code, but did
so in a way that makes the entrypoint always return with an error.

Instead, return EFI_SUCCESS if registering for those events succeeds,
and back out the registrations that did succeed if one fails, and
return an error.

Fixes: 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI events to MM context")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index a4ee4dcd864c..6c06b3574861 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -385,8 +385,16 @@ MmCommunication2Initialize (
                     MmGuidedEventNotify, mGuidedEventGuid[Index],
                     mGuidedEventGuid[Index], &mGuidedEvent[Index]);
     ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      while (Index-- > 0) {
+        gBS->CloseEvent (mGuidedEvent[Index]);
+      }
+      goto UninstallProtocol;
+    }
   }
+  return EFI_SUCCESS;
 
+UninstallProtocol:
   gBS->UninstallProtocolInterface (
          mMmCommunicateHandle,
          &gEfiMmCommunication2ProtocolGuid,
-- 
2.17.1

Re: [PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[Leif Lindholm]

On Wed, Feb 26, 2020 at 13:41:03 +0100, Ard Biesheuvel wrote:
> Commit 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI
> events to MM context") update the ARM specific standalone MM client
> driver to register for certain events in the entrypoint code, but did
> so in a way that makes the entrypoint always return with an error.
> 
> Instead, return EFI_SUCCESS if registering for those events succeeds,
> and back out the registrations that did succeed if one fails, and
> return an error.
> 
> Fixes: 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI events to MM context")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Are you looking for this one to make the stable tag? You're not
explicitly asking for it, but you're cc:ing Liming.

> ---
>  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> index a4ee4dcd864c..6c06b3574861 100644
> --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> @@ -385,8 +385,16 @@ MmCommunication2Initialize (
>                      MmGuidedEventNotify, mGuidedEventGuid[Index],
>                      mGuidedEventGuid[Index], &mGuidedEvent[Index]);
>      ASSERT_EFI_ERROR (Status);
> +    if (EFI_ERROR (Status)) {
> +      while (Index-- > 0) {
> +        gBS->CloseEvent (mGuidedEvent[Index]);
> +      }
> +      goto UninstallProtocol;
> +    }
>    }
> +  return EFI_SUCCESS;
>  
> +UninstallProtocol:
>    gBS->UninstallProtocolInterface (
>           mMmCommunicateHandle,
>           &gEfiMmCommunication2ProtocolGuid,
> -- 
> 2.17.1
> 

Re: [PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 13:57, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Feb 26, 2020 at 13:41:03 +0100, Ard Biesheuvel wrote:
> > Commit 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI
> > events to MM context") update the ARM specific standalone MM client
> > driver to register for certain events in the entrypoint code, but did
> > so in a way that makes the entrypoint always return with an error.
> >
> > Instead, return EFI_SUCCESS if registering for those events succeeds,
> > and back out the registrations that did succeed if one fails, and
> > return an error.
> >
> > Fixes: 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI events to MM context")
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>

Thanks.

> Are you looking for this one to make the stable tag? You're not
> explicitly asking for it, but you're cc:ing Liming.
>

I don't think it is terribly useful to get this into the stable tag,
given that the standalone MM support is not complete yet, and stalled
on a PI spec update.
But by the same reasoning, it could go in as well, since it is an
obvious bugfix, and the risk is low.

I don't really care either way tbh, I just wanted to make Liming aware of it.



> > ---
> >  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > index a4ee4dcd864c..6c06b3574861 100644
> > --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > @@ -385,8 +385,16 @@ MmCommunication2Initialize (
> >                      MmGuidedEventNotify, mGuidedEventGuid[Index],
> >                      mGuidedEventGuid[Index], &mGuidedEvent[Index]);
> >      ASSERT_EFI_ERROR (Status);
> > +    if (EFI_ERROR (Status)) {
> > +      while (Index-- > 0) {
> > +        gBS->CloseEvent (mGuidedEvent[Index]);
> > +      }
> > +      goto UninstallProtocol;
> > +    }
> >    }
> > +  return EFI_SUCCESS;
> >
> > +UninstallProtocol:
> >    gBS->UninstallProtocolInterface (
> >           mMmCommunicateHandle,
> >           &gEfiMmCommunication2ProtocolGuid,
> > --
> > 2.17.1
> >

Re: [PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[Leif Lindholm]

On Mon, Mar 02, 2020 at 14:00:54 +0100, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 13:57, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > On Wed, Feb 26, 2020 at 13:41:03 +0100, Ard Biesheuvel wrote:
> > > Commit 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI
> > > events to MM context") update the ARM specific standalone MM client
> > > driver to register for certain events in the entrypoint code, but did
> > > so in a way that makes the entrypoint always return with an error.
> > >
> > > Instead, return EFI_SUCCESS if registering for those events succeeds,
> > > and back out the registrations that did succeed if one fails, and
> > > return an error.
> > >
> > > Fixes: 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI events to MM context")
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> >
> 
> Thanks.
> 
> > Are you looking for this one to make the stable tag? You're not
> > explicitly asking for it, but you're cc:ing Liming.
> >
> 
> I don't think it is terribly useful to get this into the stable tag,
> given that the standalone MM support is not complete yet, and stalled
> on a PI spec update.
> But by the same reasoning, it could go in as well, since it is an
> obvious bugfix, and the risk is low.
> 
> I don't really care either way tbh, I just wanted to make Liming aware of it.

Thanks for explanation.
I'd say leave it then - if tag gets pushed Wednesday as I think the
current plan is, you can push it then.

/
    Leif

Re: [PATCH 1/1] ArmPkg/MmCommunicationDxe: fix logic bug in DXE driver entrypoint

[Ard Biesheuvel]

On Mon, 2 Mar 2020 at 14:12, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Mon, Mar 02, 2020 at 14:00:54 +0100, Ard Biesheuvel wrote:
> > On Mon, 2 Mar 2020 at 13:57, Leif Lindholm <leif@nuviainc.com> wrote:
> > >
> > > On Wed, Feb 26, 2020 at 13:41:03 +0100, Ard Biesheuvel wrote:
> > > > Commit 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI
> > > > events to MM context") update the ARM specific standalone MM client
> > > > driver to register for certain events in the entrypoint code, but did
> > > > so in a way that makes the entrypoint always return with an error.
> > > >
> > > > Instead, return EFI_SUCCESS if registering for those events succeeds,
> > > > and back out the registrations that did succeed if one fails, and
> > > > return an error.
> > > >
> > > > Fixes: 2fe25a74d6fee3c2 ("ArmPkg/MmCommunicationDxe: relay architected PI events to MM context")
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >
> > > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > >
> >
> > Thanks.
> >
> > > Are you looking for this one to make the stable tag? You're not
> > > explicitly asking for it, but you're cc:ing Liming.
> > >
> >
> > I don't think it is terribly useful to get this into the stable tag,
> > given that the standalone MM support is not complete yet, and stalled
> > on a PI spec update.
> > But by the same reasoning, it could go in as well, since it is an
> > obvious bugfix, and the risk is low.
> >
> > I don't really care either way tbh, I just wanted to make Liming aware of it.
>
> Thanks for explanation.
> I'd say leave it then - if tag gets pushed Wednesday as I think the
> current plan is, you can push it then.
>

Pushed as 3be909099cf66f26a5ace282e4cbbdc6bf7cb32b

Thanks