Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

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

On Mon, 25 Nov 2019 at 13:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Nov 25, 2019 at 13:52:24 +0100, Ard Biesheuvel wrote:
> > > > -  if (MemoryType == EfiBootServicesData) {
> > > > -    Allocation = AllocateAlignedPages (Pages, Alignment);
> > > > -  } else if (MemoryType == EfiRuntimeServicesData) {
> > > > -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
> > > > +  if (MemoryType == EfiBootServicesData ||
> > > > +      MemoryType == EfiRuntimeServicesData) {
> > > > +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment);
> > > >    } else {
> > > >      return EFI_INVALID_PARAMETER;
> > > >    }
> > > > @@ -479,6 +596,15 @@ NonCoherentDmaLibConstructor (
> > > >  {
> > > >    InitializeListHead (&UncachedAllocationList);
> > > >
> > > > +  //
> > > > +  // Ensure that the combination of DMA addressing offset and limit produces
> > > > +  // a sane value.
> > > > +  //
> > > > +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> > >
> > > Is this worth turning into a hard conditional and error return rather
> > > than an assert? The following statement will end up wrapping downwards
> > > if this condition is not true.
> >
> >
> > Constructor return values are only used in ASSERT_EFI_ERROR() calls in
> > the ProcessLibraryConstructorList() routine that gets generated by the
> > build tools (in AutoGen.c), so returning an error here would
> > essentially be dead code, given that we would only make it to this
> > point on builds that are known to ignore it.
>
> Hmm, ack.
> Print error message?
>

How? Using DEBUG()? That would be dead code as well, since it is only
active in DEBUG builds, which means the ASSERT() would fire first and
prevent the DEBUG() from ever being reached.

So that leaves printing to the console using boot services, which is
not something we should be doing from a DXE_DRIVER type module.

Given that these are both platform specific values defined at platform
creation time, I think policing them only in a DEBUG build is quite
appropriate, tbh.