From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.2843.1612385731889680442 for ; Wed, 03 Feb 2021 12:55:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=C3ohp3wS; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1612385731; 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=1TVxBHXJ8eGYSGTg1ZoW7d7o6zpvhxI+1XQuCmw/Pvk=; b=C3ohp3wSw5uQRJHbxhLUaXfmiOcdkcDxXAKGeq4+EJ+QRgSVbNbhUkIaLlRK5AuwymgGiL 81SIlZ1QD2DG3IvvZMlXdO+qMzfEd6PJoGBNYcsuwpAGGdI3jdKRmPRHApvdvsCvXrcFUb 6JBLb+MwNnh5uLb1LZxQCZjy3Mpahos= 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-402-LzjX1MkMM9KtDdyVNAr5zQ-1; Wed, 03 Feb 2021 15:55:29 -0500 X-MC-Unique: LzjX1MkMM9KtDdyVNAr5zQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CE3D9CE646; Wed, 3 Feb 2021 20:55:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-32.ams2.redhat.com [10.36.113.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0ABB45C233; Wed, 3 Feb 2021 20:55:24 +0000 (UTC) Subject: Re: [PATCH v6 7/9] OvmfPkg/CpuHotplugSmm: add CpuEject() To: Ankur Arora , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210129005950.467638-1-ankur.a.arora@oracle.com> <20210129005950.467638-8-ankur.a.arora@oracle.com> <14068eb6-dc43-c244-5985-709d685fc750@oracle.com> <3fc5ff97-6ea9-e943-523f-9a7462072c59@oracle.com> From: "Laszlo Ersek" Message-ID: <21c215e8-b21c-7642-4bbc-b3538f94e513@redhat.com> Date: Wed, 3 Feb 2021 21:55:24 +0100 MIME-Version: 1.0 In-Reply-To: <3fc5ff97-6ea9-e943-523f-9a7462072c59@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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/03/21 07:13, Ankur Arora wrote: > On 2021-02-02 6:00 a.m., Laszlo Ersek wrote: >> On 02/01/21 21:12, Ankur Arora wrote: >>> On 2021-02-01 11:08 a.m., Laszlo Ersek wrote: >>>> (16) This function uses a data structure for communication between BSP >>>> and APs -- mCpuHotEjectData->ApicIdMap is modified in UnplugCpus() on >>>> the BSP, and checked above by the APs (too). >>>> >>>> What guarantees the visibility of mCpuHotEjectData->ApicIdMap? >>> >>> I was banking on SmiRendezvous() explicitly signalling that all >>> processing on the BSP was done before any AP will look at >>> mCpuHotEjectData in SmmCpuFeaturesRendezvousExit(). >>> >>> 1716     // >>> 1717     // Wait for BSP's signal to exit SMI >>> 1718     // >>> 1719     while (*mSmmMpSyncData->AllCpusInSync) { >>> 1720       CpuPause (); >>> 1721     } >>> 1722   } >>> 1723 >>> 1724 Exit: >>> 1725   SmmCpuFeaturesRendezvousExit (CpuIndex); >> >> Right; it's a general pattern in edk2: volatile UINT8 (aka BOOLEAN) >> objects are considered atomic. (See >> SMM_DISPATCHER_MP_SYNC_DATA.AllCpusInSync -- it's a pointer to a >> volatile BOOLEAN.) >> >> But our UINT64 values are neither volatile nor UINT8, and I got suddenly >> doubtful about "AllCpusInSync" working as a multiprocessor barrier. >> >> (I could be unjustifiedly worried, as a bunch of other fields in >> SMM_DISPATCHER_MP_SYNC_DATA are volatile, wider than UINT8, and *not* >> accessed with InterlockedCompareExchageXx().) > > Thanks for pointing me to this code. There's a curious comment in > about making this structure uncache-able in the declaration here > (though I couldn't figure out how that is done): > > 418 typedef struct { > 419   // > 420   // Pointer to an array. The array should be located immediately > after this structure > 421   // so that UC cache-ability can be set together. > 422   // This is probably through SMRR manipulation. The "UefiCpuPkg/Library/SmmCpuFeaturesLib" instance contains SMRR support. The "OvmfPkg/Library/SmmCpuFeaturesLib" instance contains no SMRR support. (Just search both source files for "SMRR".) > 423   SMM_CPU_DATA_BLOCK            *CpuData; > 424   volatile UINT32               *Counter; > 425   volatile UINT32               BspIndex; > 426   volatile BOOLEAN              *InsideSmm; > 427   volatile BOOLEAN              *AllCpusInSync; > 428   volatile SMM_CPU_SYNC_MODE    EffectiveSyncMode; > 429   volatile BOOLEAN              SwitchBsp; > 430   volatile BOOLEAN              *CandidateBsp; > 431   EFI_AP_PROCEDURE              StartupProcedure; > 432   VOID                          *StartupProcArgs; > 433 } SMM_DISPATCHER_MP_SYNC_DATA; > > Also, is there an expectation that these fields (at least some of > them) switch over when a new leader is chosen? Yes, see for example the "Elect BSP" section in SmiRendezvous(). > Otherwise I'm not sure why for instance, AllCpusInSync would be > a pointer. TBH I can't explain that; I'm not too familiar with those parts... >>> CpuEject(): >>> 218   ApicId = mCpuHotEjectData->ApicIdMap[ProcessorNum]; >>> >>> For the to-be-ejected-AP, this value can only move from >>>     valid-APIC-ID (=> wait in CpuDeadLoop()) -> CPU_EJECT_INVALID. >>> >>> Given that, by the time the worker does the write on line 254, this >>> AP is guaranteed to be dead already, I don't think there's any >>> scenario where the to-be-ejected-AP can see anything other than >>> a valid-APIC-ID. >> >> The scenario I had in mind was different: what guarantees that the >> effect of >> >>     375        mCpuHotEjectData->ApicIdMap[ProcessorNum] = >> (UINT64)RemoveApicId; >> >> which is performed by the BSP in UnplugCpus(), is visible by the AP on >> line 218 (see your quote above)? >> >> What if the AP gets to line 218 before the BSP's write on line 375 >> *propagates* sufficiently? > > I understand. That does make sense. And, as you said elsewhere, a real > memory fence would come in useful here. > > We could use AsmCpuid() as a poor man's mfence, but that seems overkill > given that x86 at least guarantees store-order. Right -- I don't recall any examples of AsmCpuid() being used like that in edk2. Thanks! Laszlo