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; Thu, 03 Oct 2019 23:38:47 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AAAC1308404E; Fri, 4 Oct 2019 06:38:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-198.rdu2.redhat.com [10.10.120.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id 146FB5D9DC; Fri, 4 Oct 2019 06:38:43 +0000 (UTC) Subject: Re: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support To: "Kubacki, Michael A" , "Wu, Hao A" , "devel@edk2.groups.io" Cc: "Bi, Dandan" , Ard Biesheuvel , "Dong, Eric" , "Gao, Liming" , "Kinney, Michael D" , "Ni, Ray" , "Wang, Jian J" , "Yao, Jiewen" , Andrew Fish References: <20190928014717.31372-1-michael.a.kubacki@intel.com> <20190928014717.31372-8-michael.a.kubacki@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 4 Oct 2019 08:38:43 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Fri, 04 Oct 2019 06:38:46 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/03/19 23:53, Kubacki, Michael A wrote: > #1 - The plan is to remove the polling entirely in V3. > > #2 - I'd prefer to take a definitive direction and reduce validation and maintenance > effort but you and Laszlo both requested this so I'll add a FeaturePCD to control > activation of the runtime cache in this patch series. Perhaps this can be removed > in the future. Thanks! (I'm also happy with the lock / timeout resolution. I had known about the reentrancy restriction in the UEFI spec (I happened to look at something in the kernel just the other day that reminded me of that part of the spec), but it wasn't clear to me that the lock + timeout in the patch series were intended to protect against just that. Kudos to Andrew for the comment!) ( Meanwhile, I've received help from my colleagues wrt. QueryVariableInfo(), but right now it's too early to tell if we'll be able to settle on that in the long term: [PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev http://mid.mail-archive.com/20191003100712.31045-1-javierm@redhat.com ) For the next version of this edk2 patch set (where you plan to include the new FeaturePCD, if I understand correctly), I'd like to request the following: either set the DEC default to FALSE please, or please include a patch for OvmfPkg where you set the PCD to FALSE in all the OvmfPkg DSC files. I think the next stable release should be made like that. Then, for the stable release following that, we can re-evaluate the question, and might decide to invert the PCD in OVMF (enabling the feature), assuming QueryVariableInfo() proves usable in Fedora, by then. Two independent questions: - Has this work been regression-tested on ARM / AARCH64? (For example, ArmVirtPkg platforms use the unified runtime DXE driver, not the split runtime/SMM drivers. So no change in behavior is expected; we should test that.) In the "Testing Performed" section of your blurb, item#3 suggests something similar ("Boot from S5 to EFI shell with DXE variables enabled"), but I figured I'd raise AARCH64 specifically. - Can you please confirm that the handling of *volatile* variables is not affected? ArmVirtPkg and OvmfPkg use quite different sizes for volatile and non-volatile variables; see: - 9c7d0d499296 ("OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot", 2018-03-30) - ffe048a0807b ("ArmVirtPkg: handle NETWORK_TLS_ENABLE in ArmVirtQemu*", 2019-06-28) Thank you! Laszlo