Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 28 Nov 2018 at 23:54, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/28/18 20:16, Ard Biesheuvel wrote:
> > The primary FV contains the firmware boot image, which is not
> > runtime updatable in our case. So exposing it to the NOR flash
> > driver is undesirable, since it may attempt to modify the NOR
> > flash contents.
>
> With you so far.
>
> > It is also rather pointless, since we don't
> > keep anything there that we don't already expose via the FVB
> > protocol instances that DXE core creates for us based on the
> > FV HOBs
>
> I don't follow -- the DXE core does rely on the FV HOBs that we create for it, but I don't remember the DXE core creating FVB protocol instances. An FVB ("firmware volume block") protocol instance is usually created by a flash driver. What am I missing?
>
> Do you mean handles with MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) device paths on them? That point into firmware volumes (that have been supposedly decompressed from flash to RAM)?
>

The DXE core dispatcher calls CoreProcessFvImageFile() for all FV
images it encounters, but looking closer, that only happens for
embedded FV images, not the primary one.

But that still means the primary FV contains nothing we actually need.

> > (and so there is nothing the partition or file system
> > drivers could potentially attach to via the block I/O and disk
> > I/O protocol instances that the NOR flash driver creates)
>
> Ugh, NorFlashDxe creates BlockIo and DiskIo interfaces itself???
>
> Let's see...
>
> /*
>   Although DiskIoDxe will automatically install the DiskIO protocol whenever
>   we install the BlockIO protocol, its implementation is sub-optimal as it reads
>   and writes entire blocks using the BlockIO protocol. In fact we can access
>   NOR flash with a finer granularity than that, so we can improve performance
>   by directly producing the DiskIO protocol.
> */
>
> Umm... this flash driver does a lot more than I thought it did... or should. :)
>
>
> Anyway I think it should suffice to say in the commit message that we don't want to expose the first flash device as an FVB protocol instance, because (a) it's read-only, and (b) in the DXE phase, we don't use anything from that flash device. It contains:
> - the reset vector,
> - the SEC module,
> - (for ArmVirtQemu) the non-compressed PEI core, and PEIMs,
> - and a compressed bunch of DXE modules (incl. the DXE core) which are decompressed to RAM anyway.
>
> > So let's disregard the NOR flash block that covers the primary
> > FV.
>
> OK.
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf |  5 +++++
> >  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 13 +++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> > index d86ff36dbd58..c5752a243e6b 100644
> > --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> > +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> > @@ -28,6 +28,7 @@ [Sources.common]
> >  [Packages]
> >    MdePkg/MdePkg.dec
> >    ArmPlatformPkg/ArmPlatformPkg.dec
> > +  ArmPkg/ArmPkg.dec
> >    ArmVirtPkg/ArmVirtPkg.dec
> >
> >  [LibraryClasses]
> > @@ -40,3 +41,7 @@ [Protocols]
> >
> >  [Depex]
> >    gFdtClientProtocolGuid
> > +
> > +[Pcd]
> > +  gArmTokenSpaceGuid.PcdFvBaseAddress
> > +  gArmTokenSpaceGuid.PcdFvSize
> > diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> > index 2678f57eaaad..72b47bdb5a78 100644
> > --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> > +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> > @@ -75,13 +75,22 @@ NorFlashPlatformGetDevices (
> >        Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
> >        Reg += 4;
> >
> > +      PropSize -= 4 * sizeof (UINT32);
> > +
> > +      //
> > +      // Disregard any flash devices that overlap with the primary FV.
> > +      // The firmware is not updatable from inside the guest anyway.
> > +      //
> > +      if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) >= Base) &&
> > +          (Base + Size) >= PcdGet64 (PcdFvBaseAddress)) {
> > +        continue;
> > +      }
> > +
>
> The overlap condition is expressed correctly, in general, I think; however, both subconditions are off-by-one each. In each, we compare an exclusive limit (one's end) with an inclusive limit (the other's base). And, when exclusive equals inclusive, there is no overlap; they are directly adjacent only. I'd drop the equal signs.
>
>
> >        mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
> >        mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
> >        mNorFlashDevices[Num].Size              = (UINTN)Size;
> >        mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
> >        Num++;
> > -
> > -      PropSize -= 4 * sizeof (UINT32);
> >      }
> >    }
> >
> >
>
> Thanks
> Laszlo