From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.11629.1579866043677586727 for ; Fri, 24 Jan 2020 03:40:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OThpGoWE; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579866042; 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=tCZ6oVgZKjE/K8q6ifGgPiBmCTGcR34cbA+Fx5Mto74=; b=OThpGoWEKXpl4hWfELYt0l4vtSJrUKLpwo9W6fqv8gXOLfp7/FVsXvEZ2sCNYqUYFpDwkL PaMfWJd7Qgen7gD2BT7ZoNeDm1+SDKrm6+JprkES0VITZD7wshTZR+9z1dVYbXlF79RVvg Q8ZQRHKkAJ2P2x+PJQdvXL1sPKbsxFU= 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-232-P4HlecrWMDqcEywnhIQnPw-1; Fri, 24 Jan 2020 06:40:38 -0500 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 A5A1D190D342; Fri, 24 Jan 2020 11:40:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-77.ams2.redhat.com [10.36.117.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1BE6A8579D; Fri, 24 Jan 2020 11:40:33 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers To: devel@edk2.groups.io, philmd@redhat.com Cc: Ard Biesheuvel , Igor Mammedov , Jordan Justen References: <20191022221554.14963-1-lersek@redhat.com> <20191022221554.14963-3-lersek@redhat.com> <4cbe775a-ca0f-2c49-302a-50965360e508@redhat.com> <78640e55-de54-2c02-98d8-2fbdc7922a73@redhat.com> From: "Laszlo Ersek" Message-ID: <7eb17416-c10f-95ae-9e2e-2b9220180644@redhat.com> Date: Fri, 24 Jan 2020 12:40:33 +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.79 on 10.5.11.11 X-MC-Unique: P4HlecrWMDqcEywnhIQnPw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Hi Phil, On 10/24/19 17:12, Philippe Mathieu-Daud=E9 wrote: > On 10/24/19 12:29 PM, Laszlo Ersek wrote: >> On 10/23/19 14:05, Philippe Mathieu-Daud=E9 wrote: >>> Hi Laszlo, >>> >>> On 10/23/19 12:15 AM, Laszlo Ersek wrote: >>>> In v1.5.0, QEMU's "pc" (i440fx) board gained a "CPU present bitmap" >>>> register block. In v2.0.0, this was extended to the "q35" board. >>>> >>>> In v2.7.0, a new (read/write) register interface was laid over the "C= PU >>>> present bitmap", with an option for the guest to switch the register >>>> block >>>> to the new (a.k.a. modern) interface. >>> >>> This historical information is helpful to understand when these QEMU >>> models started to diverge from the original chipset datasheet. >>> >>>> Both interfaces are documented in "docs/specs/acpi_cpu_hotplug.txt" i= n >>>> the >>>> QEMU tree. >>>> >>>> Add macros for a minimal subset of the modern interface, just so we c= an >>>> count the possible CPUs (as opposed to boot CPUs) in a later patch in >>>> this >>>> series. >>>> >>>> Cc: Ard Biesheuvel >>>> Cc: Igor Mammedov >>>> Cc: Jordan Justen >>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1515 >>>> Signed-off-by: Laszlo Ersek >>>> --- >>>> >>>> Notes: >>>> =A0=A0=A0=A0=A0 v2: >>>> =A0=A0=A0=A0=A0 - use QEMU's existent CPU hotplug register block, rat= her than >>>> a new >>>> =A0=A0=A0=A0=A0=A0=A0 named file in fw_cfg [Igor] >>>> >>>> =A0=A0 OvmfPkg/Include/IndustryStandard/I440FxPiix4.h=A0=A0=A0 |=A0 5= +++ >>>> =A0=A0 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h=A0=A0=A0=A0 |=A0= 2 + >>>> =A0=A0 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 43 >>>> ++++++++++++++++++++ >>>> =A0=A0 3 files changed, 50 insertions(+) >>>> >>>> diff --git a/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h >>>> b/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h >>>> index e7d7fde14c65..3973ff0a95b4 100644 >>>> --- a/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h >>>> +++ b/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h >>>> @@ -44,6 +44,11 @@ >>>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= = =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 BIT10 | BIT9=A0 | BIT8=A0 | BIT7= =A0 | >>>> BIT6) >>>> =A0=A0 =A0 #define PIIX4_PMREGMISC=A0=A0=A0=A0=A0=A0=A0 0x80 >>>> =A0=A0 #define PIIX4_PMREGMISC_PMIOSE=A0=A0 BIT0 >>>> =A0=A0 +// >>>> +// IO ports >>>> +// >>>> +#define PIIX4_CPU_HOTPLUG_BASE 0xAF00 >>> >>> OK >>> >>>> + >>>> =A0=A0 #endif >>>> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h >>>> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h >>>> index 391cb4622226..2ac16f19c62e 100644 >>>> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h >>>> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h >>>> @@ -104,10 +104,12 @@ >>>> =A0=A0 // IO ports >>>> =A0=A0 // >>>> =A0=A0 #define ICH9_APM_CNT 0xB2 >>>> =A0=A0 #define ICH9_APM_STS 0xB3 >>>> =A0=A0 +#define ICH9_CPU_HOTPLUG_BASE 0x0CD8 >>> >>> OK >>> >>>> + >>>> =A0=A0 // >>>> =A0=A0 // IO ports relative to PMBASE >>>> =A0=A0 // >>>> =A0=A0 #define ICH9_PMBASE_OFS_SMI_EN 0x30 >>>> =A0=A0 #define ICH9_SMI_EN_APMC_EN=A0=A0=A0=A0=A0 BIT5 >>>> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >>>> b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >>>> new file mode 100644 >>>> index 000000000000..cf0745610f2c >>>> --- /dev/null >>>> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >>>> @@ -0,0 +1,43 @@ >>>> +/** @file >>>> +=A0 Macros for accessing QEMU's CPU hotplug register block. >>>> + >>>> +=A0 Copyright (C) 2019, Red Hat, Inc. >>>> + >>>> +=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >>>> + >>>> +=A0 @par Specification Reference: >>>> + >>>> +=A0 - "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source tree. >>>> + >>>> +=A0=A0=A0 The original (now "legacy") CPU hotplug interface appeared= in >>>> QEMU v1.5.0. >>>> +=A0=A0=A0 The new ("modern") hotplug interface appeared in QEMU v2.7= .0. >>>> + >>>> +=A0=A0=A0 The macros in this header file map to the minimal subset o= f the >>>> modern >>>> +=A0=A0=A0 interface that OVMF needs. >>>> +**/ >>>> + >>>> +#ifndef QEMU_CPU_HOTPLUG_H_ >>>> +#define QEMU_CPU_HOTPLUG_H_ >>>> + >>>> +#include >>>> + >>>> +// >>>> +// Each register offset is: >>>> +// - relative to the board-dependent IO base address of the register >>>> block, >>>> +// - named QEMU_CPUHP_(R|W|RW)_*, according to the possible access >>>> modes of the >>>> +//=A0=A0 register, >>>> +// - followed by distinguished bitmasks or values in the register. >>>> +// >>>> +#define QEMU_CPUHP_R_CMD_DATA2=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 0x0 >>> >>> I don't understand this one. Read register 0x0 is marked "reserved" >>> in the spec, and CMD_DATA are registers [0x8 .. 0xb]. >> >> The documentation (spec) is being cleaned up, and also extended. Please >> see the following QEMU patch set (warning: long discussion): >> >> =A0=A0 [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD comma= nd to >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 cpu hotplug MMIO interface >> >> =A0=A0 http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redh= at.com >> >> In that qemu RFC series, the first two patches are of primary interest >> here, for this edk2 / OvmfPkg patch set. The first two patches will not >> extend the modern interface, they will just document it better (clean u= p >> the documentation for the existent behavior). >=20 > OK I read it, but it is still RFC, so this patch shouldn't get merged > until the QEMU counterpart get merged, right? >=20 >> If you look at the next patch in this series, you'll see that the >> OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both* >> situations, that is, >> - when the offset range [0..3] is marked as reserved for reading, >> - and when it is specified as Command Data 2 (by the last patch in the >> QEMU RFC series). >> >> Therefore, we could call this register >> >> =A0=A0 QEMU_CPUHP_R_RESERVED_0 >> >> for now, but very soon we'd have to rename it to the above-proposed >> >> =A0=A0 QEMU_CPUHP_R_CMD_DATA2 >> >> anyway. Again, the code considers both cases. >> >> (In fact, the code in the next patch considers *four* cases: >> - hotplug register block absent, >> - hotplug register block present, but only legacy mode is supported, >> - hotplug register block present, modern mode available, but offset 0 i= s >> still reserved for reading, >> - hotplug register block present, modern mode available, offset 0 >> defined as Cmd Data 2.) >=20 > Understood. >=20 > This patch is fine then. >=20 > Since the QEMU spec is RFC and not merged, I'm worried it might > change. I'd rather review this patch again once the QEMU spec > update is accepted/merged. Igor's QEMU work is now upstream: 3e08b2b9cb64..3a61c8db9d25 Can you please re-check this patch? No changes should be necessary. In particular, - QEMU commit e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2' field", 2020-01-22) covers the definition of QEMU_CPUHP_R_CMD_DATA2, in this patch, - QEMU commit ae340aa3d256 ("acpi: cpuhp: spec: add typical usecases", 2020-01-22), section "detect and enable modern CPU hotplug interface", matches the QEMU_CPUHP_R_CMD_DATA2 usage in the next patch of this series. Thanks! Laszlo >=20 > Reviewed-by: Philippe Mathieu-Daude >=20 >>> >>>> + >>>> +#define QEMU_CPUHP_R_CPU_STAT=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= = =A0=A0 0x4 >>>> +#define QEMU_CPUHP_STAT_ENABLED=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 BIT0 >>> >>> OK >>> >>>> + >>>> +#define QEMU_CPUHP_RW_CMD_DATA=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 0x8 >>> >>> Here the spec says "DWORD access" but the implementation use >>> "BYTE access" (see AcpiCpuHotplug_ops in hw/acpi/cpu_hotplug.c). >>> I understand this as "there are 4 consecutive BYTE register). >> >> You are looking at the legacy mode of the register block (documented >> near the top of the "docs/specs/acpi_cpu_hotplug.txt" file). >> >> The operations for modern mode are in "hw/acpi/cpu.c", object >> "cpu_hotplug_ops". >> >> >>> Not this patch problem although. >>> >>>> + >>>> +#define QEMU_CPUHP_W_CPU_SEL=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 0x0 >>> >>> Another "DWORD access". OK. >>> >>>> + >>>> +#define QEMU_CPUHP_W_CMD=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 0x5 >>>> +#define QEMU_CPUHP_CMD_GET_PENDING=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 0x0 >>> >>> 'PENDING' is more meaningful than "CPU device with >>> inserting/removing events". OK >> >> Thanks! >> Laszlo >> >>> >>>> + >>>> +#endif // QEMU_CPU_HOTPLUG_H_ >>>> >=20 >=20 >=20