From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::141; helo=mail-it1-x141.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x141.google.com (mail-it1-x141.google.com [IPv6:2607:f8b0:4864:20::141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 42791202E53C1 for ; Mon, 18 Feb 2019 01:33:06 -0800 (PST) Received: by mail-it1-x141.google.com with SMTP id l15so26691191iti.4 for ; Mon, 18 Feb 2019 01:33:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KC6ZWoX0/mb3nkp/no+2m2swsDspvixbVeFGmzXHbfE=; b=jlHoFLDd/hwtdGtbdGnhiSOVScaT7/V2EngY5rxUCFm1NhVclhSCdCmT9KK5ZI1tm7 FUSqeOewauiuInVyX88ieQS7WlmeeAviOYt0q7Gg9IwwlX4hPP7eOPP4FSKMn26a4IiW 8Ju5qJzYqPBiEwt8psxVoF31/J5+XQjX2sJar3sWGqG5yHxJ2ierFmRVvDMc/9N1Gm95 Mq2ZnDKUrkQ3Rn8qN5wW6fP8E9goUlow6ASoB1bdAJwS9KRafI5dbbO2l+8IPnRsgKqM NyDuEba0fkPYJ33WHLMjSn8pkRi+Lgfb5f1axbK9O6CASTCPBvuFjzvxnGANT8C7hXEH 2XSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KC6ZWoX0/mb3nkp/no+2m2swsDspvixbVeFGmzXHbfE=; b=F1vVv5zsGPLMZ3Ds8g2Z6aLae9TAppfl3kAClj+d5NcsQOnqqs4PfwbRJWE4WrESEF XWhRryBktcmOZjvk0mBXPTXGH4qXOZZPL3pD+GnKLtZwHW3hCCiNhLMh0TW9hBmU4YF0 RyAI3ZUhD/KilQP+XIFmqt4OhV5phA5KdlkCEtKc/0UiP6mLV2vYnnDQ67LdH/7jJcr/ w5gIsD8UxcR7w/E++WEi5KxU64tHXKdWOkavKMRQfVrFAFpHTVUith3+23hIMo5a8S7J YGMoU0lgtPknEaR4f2g30jGWxHw5Q7H/2HKbGruNzQZPF68Z1/zIOE4Bf3/D9D5bTpyy mXnw== X-Gm-Message-State: AHQUAubyuzWyCeoDAzYuo4oPUa6RnSCMPI8Pu5qsfB2rzpWHN7eVxcre hTjkox550NxX9dFGC3PSXHJTZorL6GdZyzdlB5T8Sg== X-Google-Smtp-Source: AHgI3Ib12f71AcyB3E6Sd++P2loLoVe3NRMC/LyKOfAVnpFfY9a7vIE5SoXOOZBzJgdcRt7BAfAHIV+s5qdsQZ23La0= X-Received: by 2002:a24:56d0:: with SMTP id o199mr373966itb.158.1550482385228; Mon, 18 Feb 2019 01:33:05 -0800 (PST) MIME-Version: 1.0 References: <20190218041141.21363-1-jordan.l.justen@intel.com> <20190218041141.21363-7-jordan.l.justen@intel.com> <155048090465.22654.1079629797155553207@jljusten-skl> In-Reply-To: <155048090465.22654.1079629797155553207@jljusten-skl> From: Ard Biesheuvel Date: Mon, 18 Feb 2019 10:32:53 +0100 Message-ID: To: Jordan Justen Cc: "edk2-devel@lists.01.org" , Laszlo Ersek , Anthony Perard , Julien Grall Subject: Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 09:33:06 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 18 Feb 2019 at 10:08, Jordan Justen wrote: > > On 2019-02-17 23:53:01, Ard Biesheuvel wrote: > > On Mon, 18 Feb 2019 at 05:12, Jordan Justen wrote: > > > > > > > This needs an explanation why optimization needs to be disabled. > > I'm not sure this is required. The reason I added these patches is to > hopefully prevent the compiler from removing the frame pointer. We > adjust the frame pointer in the code, and that is a little sketchy if > the frame pointer isn't being used. > > Unfortunately, it can reasonably be argued that the > TemporaryRamSupport PPI definition ultimately makes it unsafe to write > the migration code in C. > > I tried reverting both the EmulatorPkg and OvmfPkg patches for > disabling the optimizations, and with my setup there was no impact. I > think there is a good change that we'd be pretty safe to just drop > these two patches to wait and see if someone encounters a situation > that requires it. > > Ok, so based on this explanation, do you think I should add info to > the commit message and keep the patches, or just drop them? > I think 'little sketchy' is an understatement here (as is setjmp/longjmp in general), but it is the reality we have to deal with when writing startup code in C. Looking at the code, I agree that the fact that [re]bp is assigned directly implies that we should not permit it to be used as a general purpose register, especially when you throw LTO into the mix, which could produce all kinds of surprising results when it decides to inline functions being called from here. For GCC/Clang, I don't think it is correct to assume that changing the optimization level will result in -fno-omit-frame-pointer to be set, so I'd prefer setting that option directly, either via the pragma, or for the whole file. For MSVC, I have no idea how to tweak the compiler to force it to emit frame pointers. > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Jordan Justen > > > Cc: Laszlo Ersek > > > Cc: Ard Biesheuvel > > > Cc: Anthony Perard > > > Cc: Julien Grall > > > --- > > > OvmfPkg/Sec/SecMain.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > > > index 46ac739862..86c22a2ac9 100644 > > > --- a/OvmfPkg/Sec/SecMain.c > > > +++ b/OvmfPkg/Sec/SecMain.c > > > @@ -873,6 +873,13 @@ SecStartupPhase2( > > > CpuDeadLoop (); > > > } > > > > > > +#ifdef __GNUC__ > > > +#pragma GCC push_options > > > +#pragma GCC optimize ("O0") > > > +#else > > > +#pragma optimize ("", off) > > > +#endif > > > + > > > EFI_STATUS > > > EFIAPI > > > TemporaryRamMigration ( > > > @@ -946,3 +953,8 @@ TemporaryRamMigration ( > > > return EFI_SUCCESS; > > > } > > > > > > +#ifdef __GNUC__ > > > +#pragma GCC pop_options > > > +#else > > > +#pragma optimize ("", on) > > > +#endif > > > > I can't tell from the context if this is the end of the file, but if > > it is not, aren't you turning on optimization here for non-GCC even if > > it was not enabled on the command line to begin with?