From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web11.3344.1585100229494138554 for ; Tue, 24 Mar 2020 18:37:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=T5vqq/s9; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585100228; 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=bLnHyhtYOTElVgy2JGd/Ws29cZB8kAkxbgIE0V6Xs8k=; b=T5vqq/s9TzIRjYDGbArUgvXi3t2ovN0JtY4Bsjxw6Go5uc4mEYUGpnJq9dNF+mmEP6R4Zv DpNy1fgL2xKrhU1koN+cpPzAaX4jE/1VTAMXOmCdRba5dkk3ctUptEX/MwPckF27gXNnvM 276xrojwxdTUA1boJzJ73nbaspUe3+A= 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-212-UR7Ak8ZiNsuxEQGe5u-xfg-1; Tue, 24 Mar 2020 21:37:05 -0400 X-MC-Unique: UR7Ak8ZiNsuxEQGe5u-xfg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id DA37318A5500; Wed, 25 Mar 2020 01:37:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0B8471001938; Wed, 25 Mar 2020 01:37:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval To: "Kinney, Michael D" , "Ni, Ray" , "Zeng, Star" , "Wu, Hao A" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Brian J . Johnson" References: <20200324063328.8812-1-hao.a.wu@intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310405D4087@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310405D411B@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C4AA5E3@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <8243c17d-25dc-d91a-bd28-e4382d7455b2@redhat.com> Date: Wed, 25 Mar 2020 02:37:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/24/20 16:59, Kinney, Michael D wrote: > How was the milliseconds units selected? I suggested msecs for continuity with the pre-patch unit: #define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > We have other PCDs that provide timer intervals in > 100ns unit and my microseconds. I would prefer we > be consistent so platform developers do not have > to keep track of so many different units for time > intervals. Sounds OK to me. What's more: your suggestion points at a bug in the v2 patch. The patch replaces the argument for the gBS->SetTimer() service's "TriggerTime" parameter, namely: Status = gBS->SetTimer ( mCheckAllApsEvent, TimerPeriodic, - AP_CHECK_INTERVAL + PcdGet64 (PcdCpuApStatusCheckInterval) ); But that parameter is taken in units of 100ns, per spec. Pre-patch, AP_CHECK_INTERVAL takes that into account: #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) MultU64x32((UINT64)(Milliseconds), 10000) but post-patch, the PCD does not: + gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x0000001E So this patch would shorten the interval by a divisor of 10000 at once, namely from (100 * 10000) * 100ns, to 100 * 100ns. The fix is to update the PCD comments according to your remark, and to change the default PCD value to (100 * 10000) = 1000,000. Thanks, Laszlo