From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.1752.1573634501752575070 for ; Wed, 13 Nov 2019 00:41:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=CDOsTUTm; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573634500; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=MwPFt6MVM6tGfVhJy8FF0Am12/swP4fFxNsy3utKTRs=; b=CDOsTUTm/FCazfbIa7uKAKx+szTZem8GqOgX+HFl7uvd7weyzxGXC+4l78VQ/n9rjmdQDU 06eI6TEuXQNzbbkWwl5GqlRX2I6Stif4aTkuO3ituqmIr0f8garAHtliVsIM/08uX6+WHA Qg4ua0Iry8kOHbsiko14I6fwTvyzqxM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-89-e-cPoQhdO6Or4PBfYwWB2g-1; Wed, 13 Nov 2019 03:41:38 -0500 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9BEF6180496F; Wed, 13 Nov 2019 08:41:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-243.ams2.redhat.com [10.36.116.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 055646117E; Wed, 13 Nov 2019 08:41:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2 1/1] MdeModulePkg/Variable: Fix volatile variable RT cache update logic To: devel@edk2.groups.io, michael.a.kubacki@intel.com Cc: Liming Gao , Michael D Kinney , Jian J Wang , Hao A Wu References: <20191112234038.23092-1-michael.a.kubacki@intel.com> From: "Laszlo Ersek" Message-ID: <0e860224-d481-7802-e751-7830048751ff@redhat.com> Date: Wed, 13 Nov 2019 09:41:35 +0100 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: <20191112234038.23092-1-michael.a.kubacki@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-MC-Unique: e-cPoQhdO6Or4PBfYwWB2g-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 11/13/19 00:40, Kubacki, Michael A wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2333 >=20 > During a SetVariable () invocation, UpdateVariable () is called. > UpdateVariable () contains logic to determine whether a volatile or > non-volatile UEFI variable was set so the corresponding runtime > cache can be updated to reflect the change. The current logic simply > evaluates Variable->Volatile to determine which runtime cache should > be updated. >=20 > The problem is Variable->Volatile does not always reflect whether a > volatile variable is being set. Variable->Volatile is set to TRUE > only in the case a pre-existing variable is found in the volatile > variable store. Therefore, the value is FALSE when a new volatile > variable is written. >=20 > This change updates the logic to take this into account. If a new > variable is written successfully, the Attributes will accurately > reflect whether the variable is non-volatile. If a pre-existing > variable is modified, the Volatile field will reflect the type of > variable (Attributes are not reliable; e.g. 0x0 indicates deletion). >=20 > * Observable symptom: A volatile variable that was set successfully > might return EFI_NOT_FOUND when the variable should be found. >=20 > * The issue is a regression introduced to the variable services only > when the variable runtime cache is enabled by the following PCD > being set to TRUE: > gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache >=20 > * The issue was implemented in commit aab3b9b9a1 but the PCD was not > set to TRUE by default enabling the issue until commit e07b7d024a. >=20 > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706 >=20 > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Jian J Wang > Cc: Hao A Wu > Signed-off-by: Michael Kubacki > --- >=20 > Notes: > V2 changes: > * Sets the runtime cache instance pointer that was updated in V1 > using an if/else statement instead of just an if statement so > it better aligns to the original code style. > * Updates the commit message to include the simple symptom most > commonly observed that this change fixes, an explicit description > that this change is a regression to the variable services, that > it only is present when the runtime variable cache is enabled, and > relevant commit details. Splendid, thank you! Acked-by: Laszlo Ersek Laszlo >=20 > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) >=20 > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeM= odulePkg/Universal/Variable/RuntimeDxe/Variable.c > index 29d6aca993..b0ee5e50d0 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > @@ -2296,10 +2296,10 @@ UpdateVariable ( > =20 > Done: > if (!EFI_ERROR (Status)) { > - if (Variable->Volatile) { > - VolatileCacheInstance =3D &(mVariableModuleGlobal->VariableGlobal.= VariableRuntimeCacheContext.VariableRuntimeVolatileCache); > - } else { > + if ((Variable->CurrPtr !=3D NULL && !Variable->Volatile) || (Attribu= tes & EFI_VARIABLE_NON_VOLATILE) !=3D 0) { > VolatileCacheInstance =3D &(mVariableModuleGlobal->VariableGlobal.= VariableRuntimeCacheContext.VariableRuntimeNvCache); > + } else { > + VolatileCacheInstance =3D &(mVariableModuleGlobal->VariableGlobal.= VariableRuntimeCacheContext.VariableRuntimeVolatileCache); > } > =20 > if (VolatileCacheInstance->Store !=3D NULL) { >=20