[PATCH v2 1/1] OvmfPkg/PlatformPei: drop S3Verification()

[PATCH v2 1/1] OvmfPkg/PlatformPei: drop S3Verification()

[Gerd Hoffmann]

Not needed any more, SMM + 64-bit PEI + S3 suspend works now.  While
being at it also remove it from Bhyve (where it is dead code).

Fixed by commits:
 - 8bd2028f9ac3 ("MdeModulePkg: Supporting S3 in 64bit PEI")
 - 6acf72901a2e ("UefiCpuPkg: Supporting S3 in 64bit PEI")
See also https://bugzilla.tianocore.org/show_bug.cgi?id=4195

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 OvmfPkg/Bhyve/PlatformPei/Platform.c | 29 --------------------------
 OvmfPkg/PlatformPei/Platform.c       | 31 ----------------------------
 2 files changed, 60 deletions(-)

diff --git a/OvmfPkg/Bhyve/PlatformPei/Platform.c b/OvmfPkg/Bhyve/PlatformPei/Platform.c
index 2523e49e368a..5bfe435327c1 100644
--- a/OvmfPkg/Bhyve/PlatformPei/Platform.c
+++ b/OvmfPkg/Bhyve/PlatformPei/Platform.c
@@ -491,35 +491,6 @@ DebugDumpCmos (
   }
 }
 
-VOID
-S3Verification (
-  VOID
-  )
-{
- #if defined (MDE_CPU_X64)
-  if (FeaturePcdGet (PcdSmmSmramRequire) && mS3Supported) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: S3Resume2Pei doesn't support X64 PEI + SMM yet.\n",
-      __func__
-      ));
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: Please disable S3 on the QEMU command line (see the README),\n",
-      __func__
-      ));
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: or build OVMF with \"OvmfPkgIa32X64.dsc\".\n",
-      __func__
-      ));
-    ASSERT (FALSE);
-    CpuDeadLoop ();
-  }
-
- #endif
-}
-
 /**
   Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
   Set the mMaxCpuCount variable.
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index c56247e294f2..f5dc41c3a8c4 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -230,36 +230,6 @@ ReserveEmuVariableNvStore (
   ASSERT_RETURN_ERROR (PcdStatus);
 }
 
-STATIC
-VOID
-S3Verification (
-  IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
-  )
-{
- #if defined (MDE_CPU_X64)
-  if (PlatformInfoHob->SmmSmramRequire && PlatformInfoHob->S3Supported) {
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: S3Resume2Pei doesn't support X64 PEI + SMM yet.\n",
-      __func__
-      ));
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: Please disable S3 on the QEMU command line (see the README),\n",
-      __func__
-      ));
-    DEBUG ((
-      DEBUG_ERROR,
-      "%a: or build OVMF with \"OvmfPkgIa32X64.dsc\".\n",
-      __func__
-      ));
-    ASSERT (FALSE);
-    CpuDeadLoop ();
-  }
-
- #endif
-}
-
 STATIC
 VOID
 Q35BoardVerification (
@@ -354,7 +324,6 @@ InitializePlatform (
     ASSERT_EFI_ERROR (Status);
   }
 
-  S3Verification (PlatformInfoHob);
   BootModeInitialization (PlatformInfoHob);
 
   //
-- 
2.40.1

Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: drop S3Verification()

[Laszlo Ersek]

On 5/24/23 11:11, Gerd Hoffmann wrote:
> Not needed any more, SMM + 64-bit PEI + S3 suspend works now.  While
> being at it also remove it from Bhyve (where it is dead code).
> 
> Fixed by commits:
>  - 8bd2028f9ac3 ("MdeModulePkg: Supporting S3 in 64bit PEI")
>  - 6acf72901a2e ("UefiCpuPkg: Supporting S3 in 64bit PEI")
> See also https://bugzilla.tianocore.org/show_bug.cgi?id=4195
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> ---
>  OvmfPkg/Bhyve/PlatformPei/Platform.c | 29 --------------------------
>  OvmfPkg/PlatformPei/Platform.c       | 31 ----------------------------
>  2 files changed, 60 deletions(-)

I disagree with this (v2) update. Bhyve platform code and QEMU platform
code should not be fused into a common patch, if there's a way to avoid
that. The reviewers for these C files are also different. If we want to
modify Bhyve as a courtesy, we can certainly propose a *separate* patch
for that.

If others approve this patch, I can't block it from going in; since I've
been CC'd (and I thought I'd comment this time), this is my view on it.

Laszlo

Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: drop S3Verification()

[Ard Biesheuvel]

On Wed, 24 May 2023 at 14:28, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 5/24/23 11:11, Gerd Hoffmann wrote:
> > Not needed any more, SMM + 64-bit PEI + S3 suspend works now.  While
> > being at it also remove it from Bhyve (where it is dead code).
> >
> > Fixed by commits:
> >  - 8bd2028f9ac3 ("MdeModulePkg: Supporting S3 in 64bit PEI")
> >  - 6acf72901a2e ("UefiCpuPkg: Supporting S3 in 64bit PEI")
> > See also https://bugzilla.tianocore.org/show_bug.cgi?id=4195
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > ---
> >  OvmfPkg/Bhyve/PlatformPei/Platform.c | 29 --------------------------
> >  OvmfPkg/PlatformPei/Platform.c       | 31 ----------------------------
> >  2 files changed, 60 deletions(-)
>
> I disagree with this (v2) update. Bhyve platform code and QEMU platform
> code should not be fused into a common patch, if there's a way to avoid
> that. The reviewers for these C files are also different. If we want to
> modify Bhyve as a courtesy, we can certainly propose a *separate* patch
> for that.
>
> If others approve this patch, I can't block it from going in; since I've
> been CC'd (and I thought I'd comment this time), this is my view on it.
>

Thanks Laszlo. I'll split them out when applying.

Re: [PATCH v2 1/1] OvmfPkg/PlatformPei: drop S3Verification()

[Ard Biesheuvel]

On Wed, 24 May 2023 at 14:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 24 May 2023 at 14:28, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 5/24/23 11:11, Gerd Hoffmann wrote:
> > > Not needed any more, SMM + 64-bit PEI + S3 suspend works now.  While
> > > being at it also remove it from Bhyve (where it is dead code).
> > >
> > > Fixed by commits:
> > >  - 8bd2028f9ac3 ("MdeModulePkg: Supporting S3 in 64bit PEI")
> > >  - 6acf72901a2e ("UefiCpuPkg: Supporting S3 in 64bit PEI")
> > > See also https://bugzilla.tianocore.org/show_bug.cgi?id=4195
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > > ---
> > >  OvmfPkg/Bhyve/PlatformPei/Platform.c | 29 --------------------------
> > >  OvmfPkg/PlatformPei/Platform.c       | 31 ----------------------------
> > >  2 files changed, 60 deletions(-)
> >
> > I disagree with this (v2) update. Bhyve platform code and QEMU platform
> > code should not be fused into a common patch, if there's a way to avoid
> > that. The reviewers for these C files are also different. If we want to
> > modify Bhyve as a courtesy, we can certainly propose a *separate* patch
> > for that.
> >
> > If others approve this patch, I can't block it from going in; since I've
> > been CC'd (and I thought I'd comment this time), this is my view on it.
> >
>
> Thanks Laszlo. I'll split them out when applying.

Merged as #4441