Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

Laszlo,

Thanks for the very thorough analysis - this is why I have called you 
out specifically :)
I will file a bug against SnpDxe EBS for bookkeeping.
Might end up as "won't fix" or "invalid", but a bit more time has to be 
spent to address it correctly.

Michael,

For your patch.
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

Thanks,
Maciej


On 15-Apr-20 18:40, Laszlo Ersek wrote:
> On 04/15/20 18:25, Laszlo Ersek wrote:
>
>> In other words, I personally believe that bug#1974 should have been
>> closed as INVALID (without patching the edk2 source). The EBS handler in
>> SnpDxe is necessary (as long as it does not alter the UEFI memory map
>> itself), because sending the Shutdown CDB at EBS *is* needed, and the
>> UNDI-providing agent that processes the Shutdown CDB *cannot* release
>> UEFI-owned memory.
> For the patch that's being proposed in this thread:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> However, there *is* a bug in SnpNotifyExitBootServices(). Namely, it
> calls PxeShutdown(), and PxeShutdown() does two things:
>
> (a) it sends the Shutdown CDB,
> (b) it calls Snp->PciIo->FreeBuffer().
>
> Step (a) is required in the EBS handler context.
>
> Step (b) is *forbidden* in the EBS handler context.
>
> Therefore, the cut must be made *between* steps (a) and (b).
>
> PxeShutdown() is alright to call from SimpleNetworkDriverStop(), but it
> is *not* fit for an EBS handler.
>
> (SnpNotifyExitBootServices() also calls PxeStop(). I'm not sure if that
> is really necessary, but at least PxeStop() only sends a CDB to UNDI,
> and manipulates "Snp->Mode.State". Those actions do not interfere with
> the UEFI memmap, so they are harmless in this aspect.)
>
> "in my opinion", as always :)
>
> Thanks,
> Laszlo
>