From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web12.485.1626713298949664573 for ; Mon, 19 Jul 2021 09:48:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=kz/bQInI; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 2B6E061351 for ; Mon, 19 Jul 2021 16:48:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1626713298; bh=LF1Dll7DazLOBklcHT+Y191ZMk1QKfOZ3JG5W4/W3Pg=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=kz/bQInIBkfOe1DKkOm9KHG2lQH9pOpJrwaiE8m58CNxStLQ7z/TttZApsF0aLevE Srez2QVqhnGnpqC5KiuYGvah2n3if53OoPAYid1WgSSnUsnnwmmWRQMdOD+wrK4Igs TX8C0Tt78OeQtndzD0ld8s+w6WlOA1dLTgkXYuSvANM/nTKjPfKE1IgzQ7GSrefHf6 ZJ34J2r1pNBT1TpyQdT4eCRDbGPMNOBMooeZcOy3CHIyNPwyOQOPxP9ioRrbwj1WT4 208uS/ChQIE+O6XRy9aSSV0VvgFljuwjZVceL0LwcLXnjPJx7UcavbRYT4Xg2lG+BQ lw1htIdKjEDCw== Received: by mail-ot1-f42.google.com with SMTP id 75-20020a9d08510000b02904acfe6bcccaso18783035oty.12 for ; Mon, 19 Jul 2021 09:48:18 -0700 (PDT) X-Gm-Message-State: AOAM532/lbNtgsb2SdhM+K+34RBF8yEU1EpY52sHzKPidM4E1mh0bVjF CBPOzjTa/BadS9dTDXDN+SmU3vhXDusXx1/041s= X-Google-Smtp-Source: ABdhPJzT/R4Z4qsW135H2ED46uUjRiTeFbkkQSBapVYLUFCClImC9XE9sylcT9DudMPif/NqMy/A10oW8PUyS6YQmmM= X-Received: by 2002:a05:6830:34a6:: with SMTP id c38mr6554658otu.108.1626713297478; Mon, 19 Jul 2021 09:48:17 -0700 (PDT) MIME-Version: 1.0 References: <20210708040549.8364-1-glin@suse.com> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 19 Jul 2021 18:48:06 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization To: Anthony PERARD Cc: Gary Lin , edk2-devel-groups-io , Laszlo Ersek , Ard Biesheuvel , Jordan Justen , Julien Grall , Jim Fehlig Content-Type: text/plain; charset="UTF-8" On Mon, 19 Jul 2021 at 18:07, Anthony PERARD wrote: > > 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). > I don't think S3 suspend is the kind of thing that happens to work by accident :-) > 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". > I don't have that man page. Could you elaborate? > > 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