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.web10.12912.1614100015484296153 for ; Tue, 23 Feb 2021 09:06:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=f/gVfQWf; 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=1614100014; 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=xEV3ogl2OCCybpTSWuOBqD2HpuJiXrgkfxrM78AgU10=; b=f/gVfQWfFK2mvd2sISiKAS4WM8VyhZPiUousR+l1XezkZKbB0uTxM1LHDlujrK1NZRd+IM SdBaUFvElcupflyQEbpDAAB8lvPdjOTq03aWB+fD9EbZHllNnlp4eZkSC6CP8YqF/GCvc8 L+zf1cl8Are3nW2wTdFh6vugiKkT6xc= 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-5-DLi_ngeHOzin8BxsllNY5g-1; Tue, 23 Feb 2021 12:06:52 -0500 X-MC-Unique: DLi_ngeHOzin8BxsllNY5g-1 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id 64E83801975; Tue, 23 Feb 2021 17:06:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-109.ams2.redhat.com [10.36.113.109]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6FC871899A; Tue, 23 Feb 2021 17:06:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler To: Paolo Bonzini , devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-8-ankur.a.arora@oracle.com> <746e09c5-8623-d1fe-5871-0960abf09579@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 23 Feb 2021 18:06:48 +0100 MIME-Version: 1.0 In-Reply-To: <746e09c5-8623-d1fe-5871-0960abf09579@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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: 8bit On 02/23/21 08:45, Paolo Bonzini wrote: > On 22/02/21 15:53, Laszlo Ersek wrote: >>> + >>> +  if (mCpuHotEjectData != NULL) { >>> +    CPU_HOT_EJECT_HANDLER Handler; >>> + >>> +    Handler = mCpuHotEjectData->Handler; >> This patch looks otherwise OK to me, but: >> >> In patch v8 08/10, we have a ReleaseMemoryFence(). (For now, it is only >> expressed as a MemoryFence() call; we'll make that more precise later.) >> >> (1) I think that should be paired with an AcquireMemoryFence() call, >> just before loading "mCpuHotEjectData->Handler" above -- for now, also >> expressed as a MemoryFence() call only. > > In Linux terms, there is a control dependency here.  However, it should > at least be a separate statement to load mCpuHotEjectData (which from my > EDK2 reminiscences should be a global) into a local variable.  So > >   EjectData = mCPUHotEjectData; >   // Optional AcquireMemoryFence here >   if (EjectData != NULL) { >     CPU_HOT_EJECT_HANDLER Handler; > >     Handler = EjectData->Handler; >     if (Handler != NULL) { >       Handler (CpuIndex); >     } >   } Yes, "mCPUHotEjectData" is a global. "mCpuHotEjectData" itself is set up on the BSP (from the entry point function of the PiSmmCpuSmmDxe driver), before any other APs have a chance to execute any SMM-related code at all. Furthermore, once set up, mCpuHotEjectData never changes -- it remains set to a particular non-NULL value forever, or it remains NULL forever. (The latter case applies when the possible CPU count is 1; IOW, then there is no AP at all.) Because of that, I thought that the first comparison (mCpuHotEjectData != NULL) would not need any fence -- I thought it was similar to a userspace program that (a) set a global variable in the "main" thread, before calling pthread_create(), (b) treated the global variable as a constant, ever after (meaning all threads). However, mCpuHotEjectData->Handler is changed regularly (modified by the BSP, and read "later" by all processors). That's why I thought the acquire fence was needed in the following location: if (mCpuHotEjectData != NULL) { CPU_HOT_EJECT_HANDLER Handler; // // HERE -- AcquireMemoryFence() // Handler = mCpuHotEjectData->Handler; if (Handler != NULL) { Handler (CpuIndex); } } Thanks! Laszlo