From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web10.11261.1586943941055672722 for ; Wed, 15 Apr 2020 02:45:41 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.24, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: 2M4V6sWaA7LJnZIMxT93mfSjqw7v1uzZT1QNqRii0tUDPiSr/MZKRWGPMjSws7lfGsgkQPO+n3 JSrzfiO+s2iw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2020 02:45:39 -0700 IronPort-SDR: VZys90NgjC299y5SUq4KRK6y6rcjxot/PydMXgkegPI9eH9X93FnCL11lzPKFVI7bBj21pnW8F hw7z/hG/8qQw== X-IronPort-AV: E=Sophos;i="5.72,386,1580803200"; d="scan'208";a="400260514" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.29.137]) ([10.213.29.137]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2020 02:45:37 -0700 Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK To: devel@edk2.groups.io, michael.kubacki@outlook.com Cc: Siyuan Fu , Jiaxin Wu , Laszlo Ersek References: From: "Maciej Rabeda" Message-ID: Date: Wed, 15 Apr 2020 11:45:33 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl Siyuan, Jiaxin, Laszlo, It would be great to hear especially your opinion on the matter before decisions take place :) On 14-Apr-20 19:08, Michael Kubacki wrote: > Hi Maciej, > > Thank you for summarizing the background. > > I would like to get others feedback as well. If the EBS notification > is kept, I'd like to request this patch be included in edk2-stable202005. > > Thanks, > Michael > > On 4/14/2020 2:59 AM, Rabeda, Maciej wrote: >> Hi Michael, >> >> Some time ago we have introduced a patch in ExitBootServices (EBS) >> area for SnpDxe to allow for EBS event creation control. >> Commit: >> https://github.com/tianocore/edk2/commit/61bb6eeb4d93c0a34c1995d87914ab41398f9550 >> Patch: https://edk2.groups.io/g/devel/message/48899 >> >> Ideally, at EBS stage, SNP should not interface UNDI at all. >> UEFI spec for EVT_SIGNAL_EXIT_BOOT_SERVICES events (should apply to >> event tied to EBS GUID): >> "The notification function for this event is not allowed to >> use the Memory Allocation Services, or call any functions that use >> the Memory Allocation Services and must only call functions that are >> known not to use Memory Allocation Services, because these >> services modify the current memory map." >> >> UEFI spec Appendix E states for UNDI->Stop(): >> "The memory buffer assigned in the Initialize command can be released >> or reassigned." >> >> Furthermore, all UEFI drivers controlling PCI devices are obliged to >> shut down all DMA activity. >> "Call to Action" section in: >> https://software.intel.com/sites/default/files/managed/8d/88/intel-whitepaper-using-iommu-for-dma-protection-in-uefi.pdf >> Quote: "The UEFI device driver should disable bus master and put >> controller to halt state in ExitBootServices." >> That would require UNDI drivers to create their own EBS event and >> shutdown their adapters. >> >> Based on the information above, I was willing to remove the EBS event >> creation altogether from SnpDxe due to misalignment with the UEFI spec. >> The only argument for hesitance was potential backward compatibility >> with older UNDI drivers, hence the event creation control via PCD. >> >> If we are observing issues on SNP<->UNDI line in EBS stage, I think >> the subject is worth revisiting. >> I would love to get any and all community input on that matter. >> >> Thanks, >> Maciej >> >> On 09-Apr-20 20:16, michael.kubacki@outlook.com wrote: >>> From: Michael Kubacki >>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 >>> >>> The current SnpDxe implementation registers its ExitBootServices event >>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>> This >>> function calls PxeShutdown() which issues an UNDI  shutdown operation. >>> Ultimately, this may invoke Shutdown() in EFI_SIMPLE_NETWORK_PROTOCOL. >>> >>> The UEFI specification 2.8A Table 27 "TPL Restrictions" restricts >>> the TPL >>> for Simple Network Protocol to <= TPL_CALLBACK. In addition, it has >>> been >>> observed in some 3rd party UNDI drivers to cause an issue further down >>> the call stack if the TPL is higher than TPL_CALLBACK on invocation. >>> >>> Therefore, this commit changes the TPL of >>> SnpNotifyExitBootServices() to >>> TPL_CALLBACK. >>> >>> Cc: Siyuan Fu >>> Cc: Maciej Rabeda >>> Cc: Jiaxin Wu >>> Signed-off-by: Michael Kubacki >>> --- >>>   NetworkPkg/SnpDxe/Snp.c | 3 ++- >>>   1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c >>> index 078b27cf5edd..fe022e16eacc 100644 >>> --- a/NetworkPkg/SnpDxe/Snp.c >>> +++ b/NetworkPkg/SnpDxe/Snp.c >>> @@ -2,6 +2,7 @@ >>>     Implementation of driver entry point and driver binding protocol. >>>     Copyright (c) 2004 - 2019, Intel Corporation. All rights >>> reserved.
>>> +Copyright (c) Microsoft Corporation.
>>>   SPDX-License-Identifier: BSD-2-Clause-Patent >>>     **/ >>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>>       // >>>       Status = gBS->CreateEventEx ( >>>                       EVT_NOTIFY_SIGNAL, >>> -                    TPL_NOTIFY, >>> +                    TPL_CALLBACK, >>>                       SnpNotifyExitBootServices, >>>                       Snp, >>>                       &gEfiEventExitBootServicesGuid, >> > > >