From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web10.8859.1587050726246985661 for ; Thu, 16 Apr 2020 08:25:26 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.136, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: 3cx6ZuvlQvrfHZjS+PSarwvl2lVgqkKPo+nWKXFFCbqkZBkFQxPEFnn0de2jXY0z7n7NbSyn/h 10w2fPYZ8dpg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Apr 2020 08:25:25 -0700 IronPort-SDR: LxKiAbeYd6w6LewATu8+3Uw8maHzJLbT/QsGjRnvuFQ4Ewze1nbDj/V08iQpQ++HgehFiCTU4W RZYt1fZg048Q== X-IronPort-AV: E=Sophos;i="5.72,391,1580803200"; d="scan'208";a="278025964" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.1.124]) ([10.213.1.124]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Apr 2020 08:25:24 -0700 Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK To: Laszlo Ersek , devel@edk2.groups.io, michael.kubacki@outlook.com Cc: Siyuan Fu , Jiaxin Wu , "Tomas Pilar (tpilar)" References: <44e4dac1-e4b3-e158-e291-3f9776235599@redhat.com> From: "Maciej Rabeda" Message-ID: <83bd9a4c-aeaf-c247-207d-027721f1cc4d@linux.intel.com> Date: Thu, 16 Apr 2020 17:25:17 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <44e4dac1-e4b3-e158-e291-3f9776235599@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl 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 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 > > 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 >