From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 09 Sep 2019 08:31:35 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E1C49300D243; Mon, 9 Sep 2019 15:31:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-196.ams2.redhat.com [10.36.116.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 48AD260BE2; Mon, 9 Sep 2019 15:31:34 +0000 (UTC) Subject: Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction To: devel@edk2.groups.io, michael.a.kubacki@intel.com References: <49AB4ACB9627B8468F29D589A27B745588AA4398@ORSMSX121.amr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <5cb08fa7-b4c4-d8a6-155f-986eaa207f90@redhat.com> Date: Mon, 9 Sep 2019 17:31:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <49AB4ACB9627B8468F29D589A27B745588AA4398@ORSMSX121.amr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Mon, 09 Sep 2019 15:31:34 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/05/19 21:54, Kubacki, Michael A wrote: > Proof-of-Concept Implementation > ---------------------------------------------- > The implementation is available in the following commit - check the commit message for some more details. > https://github.com/makubacki/edk2/commit/d812d43412a26e44581d283382596a863c1ae825 > > Please note this is "POC" level code quality and there will be cleanup of lock interfaces used and some other minor changes. Please feel free to leave any comments on the changes. First some meta thoughts: - If this code is meant for edk2 ultimately, please keep the discussion on the mailing list, and/or in a TianoCore bugzilla. - The size of this feature is significant. According to the github webui, "19 changed files with 2,083 additions and 1,072 deletions". A feature of this size must absolutely be broken up into a fine-grained patch series (assuming the feature targets the edk2 master branch). It's not only that such a huge patch is basically unreviewable: if someone runs into an issue after the feature is committed, they will need the ability to bisect the regression to a well isolated, self contained modification. Then the experts around the feature have a much better chance at root causing the issue from the patch that the bug reporter has identified. An all-or-nothing patch is not bisectable. - Combining the above two points into one, I'd suggest splitting the feature into small patches, and posting it as an RFC series to the list. (Assuming the initial reaction to the feature is interest -- I think it is.) Admittedly, this is a lot of work. Some more on-topic comments: > Why Keep SMM on Variable Writes > ------------------------------------------------ > * SMM provides a fairly ubiquitous isolated execution environment in x86 for authenticated UEFI variables. > * BIOS region SPI flash write restrictions to SMM in platforms today can be retained. Can you confirm that the runtime DXE code would not read flash, only the cache in EfiRuntimeServicesData memory? > Today's UEFI Variable Cache > -------------------------------------- > * Maintained in SMRAM via VariableSmm. > * A "write-through" cache of variable data in the form of a UEFI variable store. > * Non-volatile and volatile variables are maintained in separate buffers (variable stores). I'm unclear on how the items on this list should be interpreted. Are these items from today that we keep, or items that we change? For example, it's quite beneficial that NV and V variables are maintained in separate buffers -- the sizing can be different, and that's good. I believe we shouldn't change that. Thanks! Laszlo