public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Auger Eric" <eric.auger@redhat.com>,
	"Andrew Jones" <drjones@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV
Date: Thu, 29 Nov 2018 00:06:51 +0100	[thread overview]
Message-ID: <CAKv+Gu_0NUvtNwZ7T7b4ovW029DjBd01zkmc80FtcLtbctHUTg@mail.gmail.com> (raw)
In-Reply-To: <7c047366-2b63-08fc-079e-98705c7efa6b@redhat.com>

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


  reply	other threads:[~2018-11-28 23:07 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-28 19:16 [RFC PATCH 0/2] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences Ard Biesheuvel
2018-11-28 19:16 ` [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV Ard Biesheuvel
2018-11-28 22:54   ` Laszlo Ersek
2018-11-28 23:06     ` Ard Biesheuvel [this message]
2018-11-28 23:37       ` Laszlo Ersek
2018-11-28 19:16 ` [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping Ard Biesheuvel
2018-11-28 23:38   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu_0NUvtNwZ7T7b4ovW029DjBd01zkmc80FtcLtbctHUTg@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox