From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 68A4D2034CF8C for ; Thu, 26 Oct 2017 00:49:46 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CEC262C9C; Thu, 26 Oct 2017 07:53:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CEC262C9C Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-110.rdu2.redhat.com [10.10.120.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4E0277F490; Thu, 26 Oct 2017 07:53:30 +0000 (UTC) To: "Zeng, Star" , "Yao, Jiewen" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" References: <1508984017-11740-1-git-send-email-jiewen.yao@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE2AB@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA0491C@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE3FE@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA04CE4@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE45B@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA04F32@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE4D0@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 26 Oct 2017 09:53:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE4D0@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 26 Oct 2017 07:53:32 +0000 (UTC) Subject: Re: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Oct 2017 07:49:46 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/26/17 08:54, Zeng, Star wrote: > Ok, please add more description into the commit log, for example, "PCI device should disable BME at NOTIFY", etc. Last time we discussed this question, the consensus was that edk2 should not present any requirement for PCI drivers that is not required by the UEFI spec. UEFI drivers for PCI devices come from third parties as well, and those drivers will only care about the UEFI spec (as they should), not about edk2. In fact, I think this additional requirement is not necessary: * In the earlier discussion (for the SEV IoMmuDxe in OVMF), it was really necessary to delay the IoMmuDxe ExitBootServices() callback after all the PCI driver callbacks. The reason for this was that the IoMmuDxe ExitBootServices() callback was going to *lock down* all RAM from devices, and pending DMA had to be aborted before this lock-down. * In comparison, the VTdDxe callback at EBS does the opposite: it "disable[s] the protection and allow[s] all DMA access", in Jiewen's words from up-thread. So, IMO, neither the PCI driver requirement, nor this patch, are necessary -- there is never an IOMMU state that conflicts with a correctly written PCI driver's pending DMA operation. Thanks Laszlo > > With that, Reviewed-by: Star Zeng > > > Thanks, > Star > -----Original Message----- > From: Yao, Jiewen > Sent: Thursday, October 26, 2017 2:51 PM > To: Zeng, Star ; edk2-devel@lists.01.org > Cc: Laszlo Ersek (lersek@redhat.com) ; Ni, Ruiyu > Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. > > Yes, this PCI patch will be submitted soon. :) > > Thank you > Yao Jiewen > >> -----Original Message----- >> From: Zeng, Star >> Sent: Thursday, October 26, 2017 2:18 PM >> To: Yao, Jiewen ; edk2-devel@lists.01.org >> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, Star >> >> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. >> >> So there will be a guidance for this " PCI device disable BME at >> NOTIFY " to be documented? >> >> Thanks, >> Star >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Thursday, October 26, 2017 2:03 PM >> To: Zeng, Star ; edk2-devel@lists.01.org >> Cc: Laszlo Ersek (lersek@redhat.com) >> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. >> >> Right. In the future, we will let PCI device disable BME at NOTIFY. >> >> So we let IOMMU use CALLBACK, to make sure BME is disabled before >> IOMMU is disabled. >> >> Thank you >> Yao Jiewen >> >>> -----Original Message----- >>> From: Zeng, Star >>> Sent: Thursday, October 26, 2017 1:55 PM >>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, Star >>> >>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>> >>> I am confused. >>> >>> Is this patch to make the device driver's EBS event notification to >>> be run before IntelVTdDxe's EBS event notification? >>> >>> If yes, this patch seemingly can only make sure the behavior when >>> the device driver's EBS event notification is at NOTIFY, but not CALLBACK. >>> >>> >>> Thanks, >>> Star >>> -----Original Message----- >>> From: Yao, Jiewen >>> Sent: Thursday, October 26, 2017 1:16 PM >>> To: Zeng, Star ; edk2-devel@lists.01.org >>> Cc: Laszlo Ersek (lersek@redhat.com) >>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>> >>> That is fine. >>> >>> Here, disabling IOMMU means to disable the protection and allow all >>> DMA access. >>> I do not think it will bring any functional impact. >>> >>> Thank you >>> Yao Jiewen >>> >>> >>>> -----Original Message----- >>>> From: Zeng, Star >>>> Sent: Thursday, October 26, 2017 12:58 PM >>>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>>> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, >>>> Star >>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL >>>> to >>> CALLBACK. >>>> >>>> Some device driver may also have exit boot service event at >>>> CALLBACK, for example AtaPassThruExitBootServices() that was added >>>> by >> Laszlo. >>>> >>>> >>>> Thanks, >>>> Star >>>> -----Original Message----- >>>> From: Yao, Jiewen >>>> Sent: Thursday, October 26, 2017 10:14 AM >>>> To: edk2-devel@lists.01.org >>>> Cc: Zeng, Star >>>> Subject: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. >>>> >>>> Change ExitBootServices TPL to CALLBACK, so that a device can >>>> disable BME before IOMMU grants access right. >>>> >>>> Cc: Star Zeng >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jiewen Yao >>>> --- >>>> IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git >>>> a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>> index f5de01f..4a4d82e 100644 >>>> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>> @@ -483,7 +483,7 @@ InitializeDmaProtection ( >>>> >>>> Status = gBS->CreateEventEx ( >>>> EVT_NOTIFY_SIGNAL, >>>> - TPL_NOTIFY, >>>> + TPL_CALLBACK, >>>> OnExitBootServices, >>>> NULL, >>>> &gEfiEventExitBootServicesGuid, @@ -492,7 >>>> +492,7 @@ InitializeDmaProtection ( >>>> ASSERT_EFI_ERROR (Status); >>>> >>>> Status = EfiCreateEventLegacyBootEx ( >>>> - TPL_NOTIFY, >>>> + TPL_CALLBACK, >>>> OnLegacyBoot, >>>> NULL, >>>> &LegacyBootEvent >>>> -- >>>> 2.7.4.windows.1 >