From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from esa5.hc3370-68.iphmx.com (esa5.hc3370-68.iphmx.com [216.71.155.168]) by mx.groups.io with SMTP id smtpd.web12.30198.1626710846535177892 for ; Mon, 19 Jul 2021 09:07:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@citrix.com header.s=securemail header.b=Vnei6W9V; spf=pass (domain: citrix.com, ip: 216.71.155.168, mailfrom: anthony.perard@citrix.com) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1626710846; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=qpRMZKjzLCAE3vF7LjJCZ+vqQjLgTHQJO5lNVEiqm3U=; b=Vnei6W9VwDVwie/yWxt/yetLNe3gJZt+t6aQFJs7zgCbtjw2xHwjtxJd lWC0kV98HPFLSU5BZHWUcwubM4PD/W2XY09FYzo7RPqVmiwt+Bb1cuSFL trzkJYdVskX3rs/Ts3ZqGg4jFTJlpk03d7SU/7Yp6n0o8J0W9lovRFoDz 0=; Authentication-Results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: DYs+KQuzjwV9/ZOWqilvR+H2i65ozm+9ST31VT4xN2Fmzz6VkqdVSe7+3jN0C066j3Cm7Mqz7n ahNj0wAdj6jL6US9NJvitYy8RhpcLp57pBm3N2f1pnqkxlUzUJ1RCg7cV+cvjPqFZck5awOY4X vvxz7jTrnosiKSdFqNSoDf87KMoO5EScMSwEkSVMeuT7BU9zAn3IYKyV12eSMa2u0H622kOR+s iLODxM3sg2pEHmubtm4RlOxqA1+O8xur4wF/BMbwYvnYHiWzjEWo0hXxlABuS4ZXtVtPlcEHB0 xwCuTVe6lfvSLbvCmrf49YJj X-SBRS: 5.1 X-MesageID: 48272424 X-Ironport-Server: esa5.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED IronPort-HdrOrdr: A9a23:u436N64CvD286/IJHwPXwAzXdLJyesId70hD6qkQc3Fom62j5q WTdZEgvyMc5wx/ZJhNo7690cq7MBHhHPxOgbX5VI3KNGXbUQOTR72KhrGSoAEIdReeygZcv5 0QCZSXCrfLfCVHZRCR2njFLz4iquP3j5xBnY3lvhNQpZkBUdAZ0+9+YDzrdXFedU19KrcSMo GT3cZDryrIQwVtUizqbkN1OdQqvrfw5evbXSI= X-IronPort-AV: E=Sophos;i="5.84,252,1620705600"; d="scan'208";a="48272424" Date: Mon, 19 Jul 2021 17:07:21 +0100 From: "Anthony PERARD" To: Gary Lin CC: , Laszlo Ersek , Ard Biesheuvel , Jordan Justen , Julien Grall , Jim Fehlig Subject: Re: [RFC PATCH] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization Message-ID: References: <20210708040549.8364-1-glin@suse.com> MIME-Version: 1.0 In-Reply-To: <20210708040549.8364-1-glin@suse.com> Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline It would have been nice to have this patch in a patch series with "OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe", mostly to make it simpler to understand the problem needed to be fixed. On Thu, Jul 08, 2021 at 12:05:49PM +0800, Gary Lin wrote: > There are several functions in OvmfPkg/Library using > QemuFwCfgS3Enabled() to detect the S3 support status. However, in > MdeModulePkg, PcdAcpiS3Enable is used to check S3 support. Since > InitializeXenPlatform() didn't set PcdAcpiS3Enable as > InitializePlatform() did, this made the inconsistency between > drivers/functions. > > For example, S3SaveStateDxe checked PcdAcpiS3Enable and skipped > S3BootScript because the default value is FALSE. On the other hand, > PlatformBootManagerBeforeConsole() from OvmfPkg/Library called > QemuFwCfgS3Enabled() and found it returned TRUE, so it invoked > SaveS3BootScript(). However, S3SaveStateDxe skipped S3BootScript, so > SaveS3BootScript() asserted due to EFI_NOT_FOUND. This sounds like OvmfPkg would need to be fixed to use PcdAcpiS3Enable instead of QemuFwCfgS3Enabled() in most placed and have a single place where QemuFwCfgS3Enabled() is used to set PcdAcpiS3Enable. If you feel like trying to fix that, that would be nice, and then we could probably set PcdAcpiS3Enable unconditionally on OvmfXen (and maybe hope that S3 support actually works with Xen). In the mean time, this patch is fine but wants better comments. First two paragraphs are good, but the rest needs explanation on what we are trying to fix/workaround, that is "Direct Kernel Boot" as it is called in "man xl.cfg". > Setting PcdAcpiS3Enable at InitializeXenPlatform() "fixes" the crash > reported by my colleague. The other possible direction is to replace > QemuFwCfgS3Enabled() with PcdAcpiS3Enable. I'm not sure which one is > the right fix. > > Signed-off-by: Gary Lin > --- > diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c > index a811e72ee301..f7edc979486e 100644 > --- a/OvmfPkg/XenPlatformPei/Platform.c > +++ b/OvmfPkg/XenPlatformPei/Platform.c > @@ -26,6 +26,8 @@ > #include > #include > #include > +#include I don't think QemuFwCfgLib.h is needed, can you remove it? > +#include > #include > #include > #include > @@ -433,6 +437,12 @@ InitializeXenPlatform ( > CpuDeadLoop (); > } > > + if (QemuFwCfgS3Enabled ()) { This test needs a comment. QEMU's fwcfg isn't supposed to be available, unless one try to use the Direct Kernel Boot functionality. > + DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n")); > + Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE); > + ASSERT_EFI_ERROR (Status); > + } > + > XenConnect (); > > BootModeInitialization (); > diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > index 597cb6fcd7ff..1e22c0b2e2aa 100644 > --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > @@ -57,6 +57,8 @@ [LibraryClasses] > ResourcePublicationLib > PeiServicesLib > PeimEntryPoint > + QemuFwCfgLib Same here, QemuFwCfgLib doesn't seems to be needed or used. Thanks, -- Anthony PERARD