From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from de-smtp-delivery-102.mimecast.com (de-smtp-delivery-102.mimecast.com [194.104.109.102]) by mx.groups.io with SMTP id smtpd.web08.4408.1626763944296556175 for ; Mon, 19 Jul 2021 23:52:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=mimecast20200619 header.b=lvlwZiK6; spf=pass (domain: suse.com, ip: 194.104.109.102, mailfrom: glin@suse.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=mimecast20200619; t=1626763941; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=z14nD4w4KAu68HLNUO9c+kEwsQ1lZAK5f54jSlF0A00=; b=lvlwZiK6arKtiGpYk/lfhmLqG8qhmjLpfApkPCgYa0xOCZ4wCzlzU70AL3DzK9zEWmHP/c 68R/be0/B+mxKxsmgZNWlrg6eiJ1bhI5DkdGmff586vlqMAsK+TrCG6MPBD17Aae+CKiao w1UZwDaAxs7CqijUm46qJnCThFkr2Ls= Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05lp2169.outbound.protection.outlook.com [104.47.17.169]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-11-LIFlcKqOP4u4IOcTqhVVfg-1; Tue, 20 Jul 2021 08:52:20 +0200 X-MC-Unique: LIFlcKqOP4u4IOcTqhVVfg-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eHCgi+YSmNDAfJ0jOzYperuHM/iHuXvv69esvvzBhkNPdohn1+/s8Y6UpHtaHg2X3wVJvyGVXjegzwviUFetXH/pblhP/n2+7adjrgoyiezHMTOJ0R6RXmeiSEYmv3MI/0RwLnhKE+fjgWw3A5HbtZ8sxWB3BPjhyhvKbJV52KtiCNij1jXvzyvpQr8uIUgwl1+HH45fd8+PPnOJ9//V1rJ6AX8Sslxwx6zvOsWW83W2LlV1crsNsw1Kk+GhoXmCqsOS30iwcG2e+BWtWFbeT46sUls6tPafulgSIX3bO2QEopdaU/5LhDs4cyryASuDO7psyWZrmN+lO9aD/ihpOg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=z14nD4w4KAu68HLNUO9c+kEwsQ1lZAK5f54jSlF0A00=; b=SGvebP4/gHlfHhWZZhzL0D1DBEMnuLlEbDLaM/D5rcw8iggUCW1g3ZhKRipk0ag3mhVKRR/jVsIDXUD3r6hIxxGMCgAVI5AC/DWdbhPVlzG6gVmUmGiubH6uR8G5zEqkkouHWUb+YrcHLBE3kHo3nuc3BGVqXro+vY/WtHdehk+FMTsMOadrDbSnAmqDLnftdVHrvp5kB8SPdLMCCSwje1/WCfY+Fe506TPUtNd4AOg3LfKvRNdlnyNNNg3BadWWpwTVBSZv9yrDIlFKmewmpamJ5+PF7D02DNRI5InDFnW002vZbrOD9+iXSlG7spkCj26t1jawAruIvHbueITnbg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Authentication-Results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=suse.com; Received: from AM7PR04MB7013.eurprd04.prod.outlook.com (2603:10a6:20b:116::18) by AM6PR0402MB3669.eurprd04.prod.outlook.com (2603:10a6:209:1a::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4331.24; Tue, 20 Jul 2021 06:52:18 +0000 Received: from AM7PR04MB7013.eurprd04.prod.outlook.com ([fe80::a4b6:d6ff:1fc7:1f98]) by AM7PR04MB7013.eurprd04.prod.outlook.com ([fe80::a4b6:d6ff:1fc7:1f98%9]) with mapi id 15.20.4331.034; Tue, 20 Jul 2021 06:52:18 +0000 Date: Tue, 20 Jul 2021 14:52:12 +0800 From: "Gary Lin" To: Anthony PERARD Cc: devel@edk2.groups.io, 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> In-Reply-To: X-ClientProxiedBy: FR3P281CA0040.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:4a::8) To AM7PR04MB7013.eurprd04.prod.outlook.com (2603:10a6:20b:116::18) Return-Path: glin@suse.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from GaryWorkstation (60.251.47.115) by FR3P281CA0040.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:4a::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4352.12 via Frontend Transport; Tue, 20 Jul 2021 06:52:16 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 5f1a0b97-55fb-4ef3-31ec-08d94b4aea61 X-MS-TrafficTypeDiagnostic: AM6PR0402MB3669: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: mSht2NIYklcc2z1jnT6CMKuwyCDnNjJKZKds++0AKUISHcWUbFYjAJszEVmxlnsNNIKnerTKJW8LmjelMhDqUZ+PRIlch+jhYJLDxWeJkirsMjzfP8vjwTdbYiiVHeRgnqn0pVAVmP1LD89lg6ZAa5MRsd2PRz41eG3WKIkE0l4ugxU0oLKCQwPCehz611exM4KESL/dd/fbRUqta8w9+5vFceedB589u7DRdimxj9eRKDVtXoBvaCmiOHIk38ED4tvwXP5IkNYs+LutTHyyRKFJZcMOFc33gUkEeJtgfulNa1EJr2Qv/0974W5x5x1XV0jpgZYZrU3sC9RvbiempCuzHcb6fEy20VXN+rp1BWosbP+zURPNeoeLTIJ6VTL+Jzo7xZgjtvW0oe82JgKf7W2ihNqZxEUM0ZRz6Rgp6uma38SlPMhSWOI9ZOopwpsFAUkYmm3BsIwz57l39wSq5dgIvZin3PD0H4kkvLVT5Y2ExLcSajyj+G6JafYp/a5vi219LkHq+Zmo26O3Axwtds+DPWII/99bZHWhRXA/mQn/FN8buzQn6QOweYFvJXexLCPDBT825La5fwjKZbgREfmQjWwxgTqfFjl/JQHmnNKrUaVYuOhlZw8Dc+hbVEZksuVLgXpm2z7ARwJSMnxGkw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AM7PR04MB7013.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(346002)(366004)(376002)(396003)(136003)(39860400002)(8676002)(83380400001)(2906002)(956004)(6496006)(38100700002)(66556008)(33716001)(66476007)(66946007)(5660300002)(26005)(86362001)(6916009)(55236004)(8936002)(186003)(6666004)(478600001)(54906003)(316002)(4326008)(9686003)(55016002)(107886003);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yZKxKK9qPLKCksB00e9xD7et376SL/OZwei8w7MlTiUNp2l5TmEqCyyH41NC?= =?us-ascii?Q?O/e5uD4ebYCEFB3BBVAtvVUMkKMrkowkSmL/+HBcUK0tRkvpVYCIhxWc8o+c?= =?us-ascii?Q?/28gUdC5TgtO2bloO3Jmd+x/C+boJuhZLauTUGeVH+BfQPLkZOejv/BIQS79?= =?us-ascii?Q?4DhJklph5SABFPCas+7cHGzT4usL8GXgiZNjMz33BBM4pqn6Dr+UflMicHg2?= =?us-ascii?Q?lR7VRl0DpRq50y6w0fgnMRb0K6aaA3JlmnTcEUYEdjuP2OSavf8jQeraKcGz?= =?us-ascii?Q?ybi3S79wcKrPSVv5JoKK1DfBIbux0EJukWcmbOF4VRF9DzzSICHxpOfZlQ9X?= =?us-ascii?Q?j6RYQhHg0DoWnD0v29ilcfyKAfDIHc4y7KOMWP6Bvhol8rhWatXVQps+wov/?= =?us-ascii?Q?Q5Ylq/mYIkfBfc511jqzpNskfW+yPaJkcNu/KOCb2EomOE6p5AmuUxNF4K8Y?= =?us-ascii?Q?LlB1pOKLxef/vx0K7Vvxch9uU0HqFNC//xgiSDX1aGJbEpXRGkdb5GS+5KSd?= =?us-ascii?Q?GMchShTPoRkPqUs25YAiii1Zo8nDQDoNtKsqKUdLZzvMy4w4nQfkN/555N2d?= =?us-ascii?Q?pjM4h6zyiD31KKUTapH6PXQyG6K9/EyRh/FC+eTAReTUvztost9W10/7aP3z?= =?us-ascii?Q?52KJ0hde/94ETtiNKvSyoIHj5ztoVgE1DWc6OtmRPLRG7GvuUUB9e3xNdcTR?= =?us-ascii?Q?z5ol3pHVdJFBQlbAJYG6zXcuwh4Fv6jrZOV2AGi+wNj6DsnSScWK5I4Xbary?= =?us-ascii?Q?MiWvkAqxahVTrASG37QPIdSDK4TNjXD95egUNFgyXbpnyjRkWb5Zs22plB1+?= =?us-ascii?Q?M7J3vaPshFF2XJ0KxJJMiARjax3L6P0rIvoa3SHUKRP/r9xs/cuem0/P1zCZ?= =?us-ascii?Q?uaqTKGdSN3gl936SXkXB7I0klggT89HfIcg3IyfZhgflKGS25dbG31NJ8GH7?= =?us-ascii?Q?WD++HTgQVbA/OZwwLLk6r87lIjpKpaY862eIro6jICe2Ji7LEWletcyBl9eU?= =?us-ascii?Q?42u1YWsHbwomflpZUY70JWGbnMd+yTvL3TOL+GDcNgi4I+DJoqEHaK6QJUOs?= =?us-ascii?Q?nNJhf+JtLwws0vcQkUbedxHo7iRpMkvzuTovfkR2Bp6mcmO9PUrPBl7qXG0v?= =?us-ascii?Q?TNorCCErPq5VicAu5V8mBMziFpkAUC0giT4Yv/fmKN0itpX9Kn+61Jgby7ZN?= =?us-ascii?Q?G5ziLVyfvhst+KQ33WCPgU6K8w98EzSwzZRNOKU3ClpENxKCUJLJ68opPSz/?= =?us-ascii?Q?GStMnrusJv6I3P50KKIDvEf36V5edSFE1i2SMtut+5dDSZgYmw5xSS3Fu+q4?= =?us-ascii?Q?Jbh7HFaM7MUYkZoVDsWEzpf1?= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5f1a0b97-55fb-4ef3-31ec-08d94b4aea61 X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB7013.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jul 2021 06:52:18.2980 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: vhL7ZSB9wwWkT6YLY3MK0jerJrCgBMcd6iF9Qt56fbzbMW7pHwuSDOv9VTiZFuiu X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0402MB3669 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jul 19, 2021 at 05:07:21PM +0100, 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. > To be honest, I don't have Xen environment and didn't realize that it's about direct kernel boot until looking into another bug report. I just compared InitializeXenPlatform() with InitializePlatform() and my colleague told me OvmfXen works again after setting PcdAcpiS3Enable. > 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). > That's why I marked this patch as RFC since the inconsistency could exist in OVMF for KVM, not just Xen, so I would like to have feedbacks from OvmfPkg maintainers. I'll amend the patch set to cover other drivers/libraries in OvmfPkg. > 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". > Thanks for the suggestion. Will amend the comment in v2. > > 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? > Sure, will remove it from v2. > > +#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. > Will remove it from v2. Thanks, Gary Lin