From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.7891.1615293471671602131 for ; Tue, 09 Mar 2021 04:37:51 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cAav3zew; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615293470; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sd0bswOUgiv4eTKm9bbV5IlbZlzxLu2BuiNY7lJ18rM=; b=cAav3zewnm0ONAw73ao3NffNOchZkB4s7pYn3jaOUjWTAZS86y6wfzfypMWZPPH4+lAhnE qKlY9QKryEJUhFBn14L+gfnvBqIjBZOzzCdFO0w2x8+5WQy3ZhWGXHSkU6BsWNIZQuEeQa DtWQzVWfPcHVFIPQVXVTSgONwKOFdFY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-579-D9eBvlQRPU-ACojaGLqeqw-1; Tue, 09 Mar 2021 07:37:46 -0500 X-MC-Unique: D9eBvlQRPU-ACojaGLqeqw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6B0271842144; Tue, 9 Mar 2021 12:37:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-173.ams2.redhat.com [10.36.115.173]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9F5E65D9DB; Tue, 9 Mar 2021 12:37:42 +0000 (UTC) Subject: Re: [edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR access To: "Kinney, Michael D" , "devel@edk2.groups.io" , "Bi, Dandan" Cc: Liming Gao , "Liu, Zhiguang" References: <20210308051532.13872-1-dandan.bi@intel.com> From: "Laszlo Ersek" Message-ID: <042a8755-011f-f86c-64f1-fa854ff1e96b@redhat.com> Date: Tue, 9 Mar 2021 13:37:41 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/08/21 22:55, Kinney, Michael D wrote: > > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Monday, March 8, 2021 7:38 AM >> To: devel@edk2.groups.io; Bi, Dandan >> Cc: Kinney, Michael D ; Liming Gao ; Liu, Zhiguang >> >> Subject: Re: [edk2-devel] [RFC][patch] Add a new library class RegisterFilterLib in edk2 to filter/trace port IO/MMIO/MSR >> access >> >> On 03/08/21 06:15, Dandan Bi wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3246 >>> >>> 1.Purpose: >>> Skip port IO/MMIO/MSR access in some emulatoion env. >>> Trace port IO/MMIO/MSR access. >>> >>> 2.Plan to do in Edk2: >>> Filter and trace in low level APIs in BaseIoLibIntrinsic and BaseLib. >>> Add a new library class (RegisterFilterLib) for the filter and trace functionality. >>> >>> 3.Plan to filter and trace scope in Edk2 : >>> a. Port IO R/W: IA32 X64 (Only filter/trace for IA32 X64) >>> b. MMIO R/W: IA32 X64 EBC ARM AARCH64 RISCV64 (Filter/trace for the Arches supported in BaseIoLibIntrinsic.inf) >>> c. MSR R/W: IA32 X64 (Only filter/trace for IA32 X64, if other ARCH has similar use case can add new APIs per needs) >>> >>> 4.RegisterFilterLib Library Class: >>> a. Add RegisterFilterLib library class for the filter and trace operation. >>> b. Add RegisterFilterLib.h in MdePkg/Include/Library. >>> c. 12 APIs will be added to filter and trace port IO, MMIO and MSR access. >>> d. Add a NULL instance RegisterFilterLibNull in MdePkg/Library.(Verified that null instance will not impact binary >> size.) >>> e. Platform can implement its own RegisterFilterLib instance. >>> >>> 12 APIs can be divided into 2 categories: >>> 6 [Before] APIs use to check whether need to execute port IO/MMIO/MSR access or do some tracing before access. >>> 6 [After] APIs use to trace after port IO/MMIO/MSR access. >>> The detailed API definitions are included in this patch. >>> >>> For port IO access: >>> FilterBeforeIoRead >>> FilterAfterIoRead >>> FilterBeforeIoWrite >>> FilterAfterIoWrite >>> >>> For MMIO access: >>> FilterBeforeMmIoRead >>> FilterAfterMmIoRead >>> FilterBeforeMmIoWrite >>> FilterAfterMmIoWrite >>> >>> For MSR access: >>> FilterBeforeMsrRead >>> FilterAfterMsrRead >>> FilterBeforeMsrWrite >>> FilterAfterMsrWrite >>> >>> 5.Change and Impact >>> a. Add the RegisterFilterLib libary class and RegisterFilterLibNull instance firstly. >>> b. Update the dsc in edk2 and edk2-platform repo to consume the RegisterFilterLibNull instance. >>> c. Update the BaseLib and IoLib to consume RegisterFilterLib. >>> >>> This is an incompatible change. >>> No code change in BaseLib and IoLib consumers, only need to change dsc to consume new FilterLib instance. >>> Update BaseIoLibIntrinsic.inf and BaseIoLibIntrinsicSev.inf to consume RegisterFilterLib for all supported Arch >>> Update BaseLib.inf to consume RegisterFilterLib only for IA32 and X64 >> >> Seems like a sound plan, but there are more IoLib instances than the above: >> >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf >> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf > > I agree that all 3 of these need to be included in the plan. > >> MdePkg/Library/DxeIoLibCpuIo2/DxeIoLibCpuIo2.inf >> MdePkg/Library/PeiIoLibCpuIo/PeiIoLibCpuIo.inf >> MdePkg/Library/SmmIoLibSmmCpuIo2/SmmIoLibSmmCpuIo2.inf > > The IoLib instances all perform their I/O operation by calling > a dynamic PPI/Protocol services. I would recommend that we do not update > these instances, and instead only apply the RegisterFilterLib to > IoLib instances that perform he direct access to the hardware. > Any IoLib instances that access the hardware through a PPI/Protocol > should not be updated. > > We have a few implementations of the CPI I/O PPI/Protocol that > use the BaseIoLibIntrinsics, so those would actually be covered > by the first set of lib instances. If a platform decides to > implement a new version of the CPU I/O PPI/Protocol that does not > use one of the BaseIoLibInstrinsic instances, then they would > have the option of using the RegisterFilterLib in that new > implementation of the CPI I/O PPI/Protocol. Thanks for the explanation; I agree. Laszlo