From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by mx.groups.io with SMTP id smtpd.web10.1343.1684951994667561953 for ; Wed, 24 May 2023 11:13:15 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=S5yBEGCz; spf=pass (domain: ventanamicro.com, ip: 209.85.218.48, mailfrom: tphan@ventanamicro.com) Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-96f5d651170so20387466b.1 for ; Wed, 24 May 2023 11:13:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1684951993; x=1687543993; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=vC65SF5gPvduaQpcymnozjIG7coQTU6SdULvA8O3qhU=; b=S5yBEGCzhD0h+fP7/I89F5FhCHZ3W1v4+aVd3ead6GsbS+B8EPDbhO0TUtSNC2PXq3 JVYFsiJkFDNpG1IdpdhNuwuW31LVKRgPA3jrgthWPntnYoFYLysxLePM84mPgOCMqJR3 NwQcESgtRCZo/9Lov3cE8gKz5EUuFbi9odXqq1Vnt4DFEhTLfTlIFIdPQWNviv3RhTe6 z8ydHg1VBkqnF+5GeH7uF0pc6ZzQBrHdc6HgZHh/n+wCJTOA1uHCu3cxFfd1dxeLWjG4 6pNXibcO0KO0KO28JW4uMbawS9djeFsee8q8XSv2VTPCh+1B6hK8UTtQs1vSqCq30tTA PnGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684951993; x=1687543993; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=vC65SF5gPvduaQpcymnozjIG7coQTU6SdULvA8O3qhU=; b=mBZJTjkXcvml6yD09lwZ6h+TbDIIBuT+q0o7eOwsLbK26MKRUKOlerFWnpf2ben6kR 5SZiXVhiGydEX/A7QSkO5aQBHBmT4RTHMq2JHYNDLK7QnAM7qbflM0X8dsi1iJ8oeWq1 gFrlJjdhOT42C5ovMtLekJQE5KCftkYiMbl+kkueSIuopxyGm1IfYNsnhrV01b75Ajvd 3CKV2BFWnZinVuJAR25ULyTnP2dUj6+msPxjq012aH+hGbuKXy3xXndLpeZa4KpdTIVo saS+qtCgrR5V+Wu53IKnD8NTMiSWkkcJ7g3e+8NAzrNfDhOrB7f/gQnnuakawgdlsCoF YV+w== X-Gm-Message-State: AC+VfDxBw6n2fqub1MPauPkDWbfuYz0vjivKNH4IJ9RTalb0/sp4mUM2 rCL0NKkPMnQ6R2KTx/Hrsnbrwq94InE1THlrAkVppg== X-Google-Smtp-Source: ACHHUZ4D/RIdO/kzifjzbBPpDEMsTosnaiZyuFEL85coF1LB1YJG8GCmfKs9oIR4WPDvnbAtJm3Mxo1RicxtUrzIhPg= X-Received: by 2002:a17:907:6d29:b0:973:9443:c5fc with SMTP id sa41-20020a1709076d2900b009739443c5fcmr250932ejc.14.1684951993093; Wed, 24 May 2023 11:13:13 -0700 (PDT) MIME-Version: 1.0 References: <20230306173316.10319-1-tphan@ventanamicro.com> <20230306173316.10319-6-tphan@ventanamicro.com> In-Reply-To: From: "Tuan Phan" Date: Wed, 24 May 2023 11:13:01 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists To: Ard Biesheuvel Cc: devel@edk2.groups.io, michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, sunilvl@ventanamicro.com, git@danielschaefer.me, andrei.warkentin@intel.com Content-Type: multipart/alternative; boundary="0000000000000363b805fc74752e" --0000000000000363b805fc74752e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Mar 6, 2023 at 9:53=E2=80=AFAM Ard Biesheuvel wro= te: > On Mon, 6 Mar 2023 at 18:33, Tuan Phan wrote: > > > > The flash base address can be added to GCD before this driver run. > > So only add it if it has not been done. > > > > How do you end up in this situation? > > You cannot skip this registration, as it is required to get the region > marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be > broken when running under the OS. > Ard, The patch only skips AddMemorySpace if it is already done in the early SEC phase for RiscV platform. The EFI_MEMORY_RUNTIME always be set in the next line with SetMemorySpaceAttributes. > > Signed-off-by: Tuan Phan > > --- > > OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++-------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c > b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c > > index f9a41f6aab0f..8875824f3333 100644 > > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c > > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c > > @@ -372,10 +372,11 @@ NorFlashFvbInitialize ( > > IN NOR_FLASH_INSTANCE *Instance > > ) > > { > > - EFI_STATUS Status; > > - UINT32 FvbNumLba; > > - EFI_BOOT_MODE BootMode; > > - UINTN RuntimeMmioRegionSize; > > + EFI_STATUS Status; > > + UINT32 FvbNumLba; > > + EFI_BOOT_MODE BootMode; > > + UINTN RuntimeMmioRegionSize; > > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc; > > > > DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n")); > > ASSERT ((Instance !=3D NULL)); > > @@ -390,13 +391,19 @@ NorFlashFvbInitialize ( > > // is written as the base of the flash region (ie: > Instance->DeviceBaseAddress) > > RuntimeMmioRegionSize =3D (Instance->RegionBaseAddress - > Instance->DeviceBaseAddress) + Instance->Size; > > > > - Status =3D gDS->AddMemorySpace ( > > - EfiGcdMemoryTypeMemoryMappedIo, > > + Status =3D gDS->GetMemorySpaceDescriptor ( > > Instance->DeviceBaseAddress, > > - RuntimeMmioRegionSize, > > - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > > + &Desc > > ); > > - ASSERT_EFI_ERROR (Status); > > + if (Status =3D=3D EFI_NOT_FOUND) { > > + Status =3D gDS->AddMemorySpace ( > > + EfiGcdMemoryTypeMemoryMappedIo, > > + Instance->DeviceBaseAddress, > > + RuntimeMmioRegionSize, > > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > > + ); > > + ASSERT_EFI_ERROR (Status); > > + } > > > > Status =3D gDS->SetMemorySpaceAttributes ( > > Instance->DeviceBaseAddress, > > -- > > 2.25.1 > > > > > > > > ------------ > > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#100754): > https://edk2.groups.io/g/devel/message/100754 > > Mute This Topic: https://groups.io/mt/97430554/1131722 > > Group Owner: devel+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org] > > ------------ > > > > > --0000000000000363b805fc74752e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Mar 6, 2023 at 9:53=E2=80=AFA= M Ard Biesheuvel <ardb@kernel.org= > wrote:
On Mon, 6 Mar 2023 at 18:33, Tuan Pha= n <tphan@ven= tanamicro.com> wrote:
>
> The flash base address can be added to GCD before this driver run.
> So only add it if it has not been done.
>

How do you end up in this situation?

You cannot skip this registration, as it is required to get the region
marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
broken when running under the OS.

Ard,<= /div>
The patch only skips AddMemorySpace if it is already done=C2=A0in= the early SEC phase for RiscV platform.
The EFI_MEMORY_RUNTIME a= lways be set in the next line with SetMemorySpaceAttributes.

=

> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>=C2=A0 OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--= ------
>=C2=A0 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c b/OvmfPkg/VirtN= orFlashDxe/VirtNorFlashDxe.c
> index f9a41f6aab0f..8875824f3333 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
>=C2=A0 =C2=A0 IN NOR_FLASH_INSTANCE=C2=A0 *Instance
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 EFI_STATUS=C2=A0 =C2=A0 =C2=A0Status;
> -=C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0FvbNumLba;
> -=C2=A0 EFI_BOOT_MODE=C2=A0 BootMode;
> -=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RuntimeMmioRegionSize;=
> +=C2=A0 EFI_STATUS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 Status;
> +=C2=A0 UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 FvbNumLba;
> +=C2=A0 EFI_BOOT_MODE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0BootMode;
> +=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0RuntimeMmioRegionSize;
> +=C2=A0 EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
>
>=C2=A0 =C2=A0 DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n")= );
>=C2=A0 =C2=A0 ASSERT ((Instance !=3D NULL));
> @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
>=C2=A0 =C2=A0 //=C2=A0 =C2=A0 =C2=A0 =C2=A0is written as the base of th= e flash region (ie: Instance->DeviceBaseAddress)
>=C2=A0 =C2=A0 RuntimeMmioRegionSize =3D (Instance->RegionBaseAddress= - Instance->DeviceBaseAddress) + Instance->Size;
>
> -=C2=A0 Status =3D gDS->AddMemorySpace (
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 EfiGcd= MemoryTypeMemoryMappedIo,
> +=C2=A0 Status =3D gDS->GetMemorySpaceDescriptor (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I= nstance->DeviceBaseAddress,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Runtim= eMmioRegionSize,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 EFI_ME= MORY_UC | EFI_MEMORY_RUNTIME
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &D= esc
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )= ;
> -=C2=A0 ASSERT_EFI_ERROR (Status);
> +=C2=A0 if (Status =3D=3D EFI_NOT_FOUND) {
> +=C2=A0 =C2=A0 Status =3D gDS->AddMemorySpace (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= EfiGcdMemoryTypeMemoryMappedIo,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= Instance->DeviceBaseAddress,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= RuntimeMmioRegionSize,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= );
> +=C2=A0 =C2=A0 ASSERT_EFI_ERROR (Status);
> +=C2=A0 }
>
>=C2=A0 =C2=A0 Status =3D gDS->SetMemorySpaceAttributes (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 I= nstance->DeviceBaseAddress,
> --
> 2.25.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#100754): https://edk2.groups.i= o/g/devel/message/100754
> Mute This Topic: https://groups.io/mt/97430554/1131722=
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> ------------
>
>
--0000000000000363b805fc74752e--