From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) by mx.groups.io with SMTP id smtpd.web10.13877.1685025859476437447 for ; Thu, 25 May 2023 07:44:19 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=NcznAA97; spf=pass (domain: ventanamicro.com, ip: 209.85.218.51, mailfrom: tphan@ventanamicro.com) Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-96f7bf29550so112668766b.3 for ; Thu, 25 May 2023 07:44:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685025858; x=1687617858; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=BFXm3OXqFjNVCFfJFCfIw5ij/vKAn1x4kMyLn/K3oJg=; b=NcznAA97zd3OH25A3Oo2K8vA/+51shFm+7DRKyqSVsXiRe+o2d040T+kBIBAsB7yL7 43BOHOsUMLMSx30XrZw1OY4RX0Q9qndmvFoIr2HdG+jGtUa89dy9I14mvf6zQCe1dgJK oOhoPvPLNEY7LZCcDvsMmzCpIVj5GV85s+8zTg5p3RgLom6aTdPaOxcVGUgVGS3YU5qm lIiUHTEi0ZIT/hUJSLONnULVmotoZL36hPLiWQFhqMr5X4mNBysUtz1+r/LRB9vdPC/R wLHqcdCixj62KDIcPVc1OP0KtnJNic04woWV+BPupfmx4EWogoWf2ghySEz4aC6pNJ12 Wiew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685025858; x=1687617858; 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=BFXm3OXqFjNVCFfJFCfIw5ij/vKAn1x4kMyLn/K3oJg=; b=Hbi9yIEgcnNKrVYciOdsTQF+1k5qKhk8puzjTTjTjJ9dqS2Ey4guIm2/vm+nlY3clZ Q/GCgOajPJUNs9nLOiitsZXsJzXqIhjrQ9ezpT3VEFfSs5JfleecM/j9mPzq55uSuztW 1eInp3WIetJTA5xFmQrnKgUR3ConXbjZu3GQYXW5GkgZ6kXIYNlPBKfvspk0twf3gayo HQpDFzUryyXFCqB5ZBqALSsccUMuHSYSlQaQQr/KC3kC/wMIqoJAXeQGxIrl+4LGGq2C /XocfwJSF6X23uKI2BHvj8T6mCcVDBHCGfK1JrN9DEcWRDO7sz4AmH6vgYXrvC/vp/Ei s8Gw== X-Gm-Message-State: AC+VfDwMqzJo9M5auEHBeBI4cw1m3acWesnQIKX7GlLXvLtCd+UcjFrs KSkDywumIwOz1KMHYWEUzbOGqvaBzp3JakTITB+1Eg== X-Google-Smtp-Source: ACHHUZ70Ioku5UnVabD1aexE2RFoYK7QbAmPLW4gLB/8wBebFsJnYpHXBtrqyetJ9utvgdDjyNZmjFChrLQ5fltSTKc= X-Received: by 2002:a17:907:36cc:b0:966:17b2:5b0b with SMTP id bj12-20020a17090736cc00b0096617b25b0bmr1882420ejc.49.1685025857803; Thu, 25 May 2023 07:44:17 -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: Thu, 25 May 2023 07:44:06 -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="000000000000b12f0105fc85a71b" --000000000000b12f0105fc85a71b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, May 25, 2023 at 7:27=E2=80=AFAM Ard Biesheuvel wr= ote: > On Wed, 24 May 2023 at 20:13, Tuan Phan wrote: > > > > > > > > On Mon, Mar 6, 2023 at 9:53=E2=80=AFAM Ard Biesheuvel = wrote: > >> > >> 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. > > > > So how does the SEC phase create GCD regions to begin with? > > This really sounds like there is a problem elsewhere tbh. > The SEC phase just simply adds that region to the memory hob with BuildResourceDescriptorHob. Agree, the problem is VariableRuntimeDxe.efi accessing the region before this VirtNorFlashDxe starts so when MMU enabled, edk2 will hang due to page fault exception. > > > >> > >> > 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] > >> > ------------ > >> > > >> > > > > >=20 > --000000000000b12f0105fc85a71b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, May 25, 2023 at 7:27=E2=80=AF= AM Ard Biesheuvel <ardb@kernel.org> wrote:
On Wed, 24 May 2023 at 20:13, Tuan P= han <tphan@v= entanamicro.com> wrote:
>
>
>
> On Mon, Mar 6, 2023 at 9:53=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:<= br> >> >
>> > 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 re= gion
>> 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 SetMemorySp= aceAttributes.
>

So how does the SEC phase create GCD regions to begin with?

This really sounds like there is a problem elsewhere tbh.
<= div>
The SEC phase just simply adds that region to the memory= hob with BuildResourceDescriptorHob.
Agree, the problem is Varia= bleRuntimeDxe.efi accessing the region before this VirtNorFlashDxe starts s= o when
MMU enabled, edk2 will hang due to page fault exception.


>>
>> > 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/Ovmf= Pkg/VirtNorFlashDxe/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 RuntimeMmioRe= gionSize;
>> > +=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 b= ase of the flash region (ie: Instance->DeviceBaseAddress)
>> >=C2=A0 =C2=A0 RuntimeMmioRegionSize =3D (Instance->RegionBa= seAddress - 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 EfiGcdMemoryTypeMemoryMappedIo,
>> > +=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 Instance->DeviceBaseAddress,
>> > -=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 EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 &Desc
>> >=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 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/11= 31722
>> > Group Owner: devel+owner@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub<= /a> [ardb@kernel.org]
>> > ------------
>> >
>> >
>
--000000000000b12f0105fc85a71b--