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.web09.180.1614100698427875945 for ; Tue, 23 Feb 2021 09:18:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Z2+3Rb+p; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: pbonzini@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614100697; 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=Yolhfe9bsErsoTj6twRpTKJtynC6G1coE5lYGrpuYBc=; b=Z2+3Rb+pf2objFrUxB4nJ1QkzpZYEGv8RDLsSyZQU6tku2eVdKB9p3f9CAM6nWacMk6us+ LK54gHHokLLC4ZWjNMc50iL2/hvUP7pj+qNlwGaltPcVX04gfHROpO3LGPkhrwUVyzIfbP +RQMFBZUQN3dOJ8mGZtix3uj3BkBdhI= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-527-wzY9g_RxOxaGhG-d03rKMQ-1; Tue, 23 Feb 2021 12:18:15 -0500 X-MC-Unique: wzY9g_RxOxaGhG-d03rKMQ-1 Received: by mail-wm1-f69.google.com with SMTP id q24so1470511wmc.1 for ; Tue, 23 Feb 2021 09:18:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Yolhfe9bsErsoTj6twRpTKJtynC6G1coE5lYGrpuYBc=; b=tx4rApQVA4srvDEt/KEqoZbsSZmr+9ld+WE4j1IoHqzDtULSSoWOmnZCQzYq76Uf7B LV/znipDXrUYN9QM+5Ay0YCnbvsQ3QjysXi169guDD4luQ3yCqW2OX9PG0mguSE5OJ73 v5p6iNaG8g8aQ7lRxo90kXl/f7NR8vHqmtoyNNDquacR7YxhR4Y7Zi+3qzk58sGoRSm6 g5skmaqQIsEY5gW5he0q5flh4j6Zrg9E8Bf/gLo6XYx8e25jPjcXFEQ9Ert9k2T59k/y vrKnjjTApq8oLpwe7Pf72FVHL1jtkqccIdwHbupg8032/xOO0RGU7ymYGVLdUCOAOame G1nA== X-Gm-Message-State: AOAM531ouV8iRicTWFyNOIlISWh8kx+VOrZHQz1/wVEYpo+gBDAgojKF HofOf2hydPVIph9dXA5O1rkBkMum7rUeEjEtNOakNj6dqpMP7MG85GbJibQqbhNqoBeY1VKOOJN 2EWH/fg1h5QJtFQ== X-Received: by 2002:adf:d239:: with SMTP id k25mr28575023wrh.308.1614100694655; Tue, 23 Feb 2021 09:18:14 -0800 (PST) X-Google-Smtp-Source: ABdhPJyTCwqsCMkyeYCQNHohreqcJ854iE18UbNAhDYDVN3UCXygT8rnpb4Sg+oTFLl3dtTkBc8JcQ== X-Received: by 2002:adf:d239:: with SMTP id k25mr28575002wrh.308.1614100694423; Tue, 23 Feb 2021 09:18:14 -0800 (PST) Return-Path: Received: from ?IPv6:2001:b07:add:ec09:c399:bc87:7b6c:fb2a? ([2001:b07:add:ec09:c399:bc87:7b6c:fb2a]) by smtp.gmail.com with ESMTPSA id j16sm32895290wra.17.2021.02.23.09.18.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Feb 2021 09:18:13 -0800 (PST) Subject: Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler To: Laszlo Ersek , 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: "Paolo Bonzini" Message-ID: <36d02edb-088e-5276-1a6e-435b5417e82c@redhat.com> Date: Tue, 23 Feb 2021 18:18:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=pbonzini@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 23/02/21 18:06, Laszlo Ersek wrote: > 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.) Ok, that's what I was missing. However, your code below has *two* loads of mCpuHotEjectData and the fence would have to go after the second (between the load of mCpuHotEjectData and the load of the Handler field). Therefore I would still use a local variable even if you decide to put the fence inside the "if", which I agree is the clearest. Paolo > 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 >