From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.2163.1571991696475136723 for ; Fri, 25 Oct 2019 01:21:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=NrVw01nN; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1571991695; 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=z8N3TvyTLbj8ZWVVmaYixg7xARDmerHw1ZQQ8451T7k=; b=NrVw01nNaKg9x1JwDxmTmO43c1Dgkh+e/fdzkCaN2emTlLhEdwvsncBaE4qvM+r9TlvzP3 2eBpng6FEohQKjJmyYuJYxYLVOOxLeWU3/xXGWgglpLFYjPEL/4gU3ojClis6YLOp4lXnt dv4iXN41J0x1QI8yOlq7NF20Qm2eLBY= 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-269-tYuy4qoAMpW_BR0jYeNiQg-1; Fri, 25 Oct 2019 04:21:31 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 31C745E6; Fri, 25 Oct 2019 08:21:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-99.ams2.redhat.com [10.36.116.99]) by smtp.corp.redhat.com (Postfix) with ESMTP id 908115D9CA; Fri, 25 Oct 2019 08:21:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io 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: <077baae3-b0d8-e683-887e-1f0fcf1c0129@redhat.com> Date: Fri, 25 Oct 2019 10:21:25 +0200 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.14 X-MC-Unique: tYuy4qoAMpW_BR0jYeNiQg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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 "CP= U >>>> 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" in >>>> the >>>> QEMU tree. >>>> >>>> Add macros for a minimal subset of the modern interface, just so we ca= n >>>> 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, rath= er 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 of= 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 comman= d 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@redha= t.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 up >> 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 is >> 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. Yes, we can certainly delay pushing this set until the QEMU set is merged. > Reviewed-by: Philippe Mathieu-Daude Thanks! Laszlo >=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_ >>>>