From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id D7D7DD80A5F for ; Mon, 13 Nov 2023 10:43:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bBheNYMpH62JXwwTlZLMWoWkGqyBmX/XNaVZZlT4qwo=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699872202; v=1; b=P8uAA9+RLY6QYDhYTTdpPEA8PyMHE/37HXYwJfRNKEHweY0PbpEcuk5aiFf0x2Y/vB9nWO/a pX4Bz/FsRHdH+XUlJfcdKsByILX2Y+KE/sEEw6Qpk3f2mJJNcK6fQNbO4jd91d4lmq8dzJVhMFo hO3jV6+u4GJ2y40k494jp/as= X-Received: by 127.0.0.2 with SMTP id KpvzYY7687511xv2SaKDcACv; Mon, 13 Nov 2023 02:43:22 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.34197.1699872201880638450 for ; Mon, 13 Nov 2023 02:43:22 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-29-X_AeVpSZO_GI9dWg5S9J2Q-1; Mon, 13 Nov 2023 05:43:19 -0500 X-MC-Unique: X_AeVpSZO_GI9dWg5S9J2Q-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E361888D4E8; Mon, 13 Nov 2023 10:43:18 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8905A36EE; Mon, 13 Nov 2023 10:43:17 +0000 (UTC) Message-ID: Date: Mon, 13 Nov 2023 11:43:16 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class To: "Ni, Ray" , "devel@edk2.groups.io" , "Wu, Jiaxin" Cc: "Dong, Eric" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" References: <20231103153012.3704-1-jiaxin.wu@intel.com> <20231103153012.3704-4-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: QCi9UAakWw13JGYUHtY1vkRmx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=P8uAA9+R; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 11/13/23 04:15, Ni, Ray wrote: > I provided 4 comments in below, starting with "[Ray". > > Sorry for the poor new Outlook that I am not able to put ">" in the > original email. > > Thanks, > Ray > > ------------------------------------------------------------------------ > > (1) I agree that the internals of the context should be hidden, but > (VOID*) is not the right way. Instead, please use an incomplete > structure type. > > That is, in the library header class file, do the following: > >   typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX; > > and then make all these functions return and take > >   SMM_CPU_SYNC_CTX * > > The library users will still not know what is inside SMM_CPU_SYNC_CTX, > just like with (VOID*), but the compiler will be able to perform much > stronger type checking than with (VOID*). > > And then in the library *instance(s)*, you can complete the incomplete type: > >   struct SMM_CPU_SYNC_CTX { >     ... >   }; > > [Ray.1] Good suggestion. I still remember you taught me this technique > when I > wrote the FrameBufferBltLib. > > (3) By definition, in a function that resets the internals of an object, > you cannot specify "IN" for that function. It must be OUT. > > [Ray.2] I have a different opinion about IN/OUT.  I think we should use > "IN OUT". OK! I can see that "reset" may rely on previous information in the structure. Furthermore, "IN OUT" may indeed better reflect the requirement that the object being reset must have been initialized previously (i.e. that it must be a valid object at the time of the "reset" call). > > > Please add a large comment to the top of the library class header that > explains the operation / concepts of the context. What operations and > behaviors are defined for this data type? > > [Ray.3] Good suggestions. > The lib provides 3 sets of APIs: > 1. Init/Deinit > Init() is called in driver's entrypoint for allocating the storage and > initialize the content of sync object. > Deinit() is called in driver's unload function for undoing what has been > done in Init(). > > 2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount > When SMI happens, all processors including BSP enter to SMM mode by > calling CheckInCpu(). > The elected BSP calls LockDoor() so that CheckInCpu() after that returns > failure code. > CheckOutCpu() is called in error handling flow for the CPU who calls > CheckInCpu() earlier. > GetArrivedCpuCount() returns the number of checked-in CPUs. > > 3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp > First 2 APIs are called from BSP. > Latter 2 APIs are called from APs. > The 4 APIs are used to synchronize the running flow among BSP and APs. This sounds really nice in fact; I'd be glad to see it captured in the comments! > >> + >> +  @return     CPU arrival count number. > > (14) why is it necessary for outputting the arrived number when locking? > [Ray.4] As I explained above, when BSP locks the door, it might need to > know how many CPUs are in the SMM "room". > Maybe today the number is not cared by BSP. > This helps, thanks. Please do capture it in the comments. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111134): https://edk2.groups.io/g/devel/message/111134 Mute This Topic: https://groups.io/mt/102366300/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-