From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web09.711.1571929970255298835 for ; Thu, 24 Oct 2019 08:12:50 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: philmd@redhat.com) Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7E61F59451 for ; Thu, 24 Oct 2019 15:12:49 +0000 (UTC) Received: by mail-wr1-f71.google.com with SMTP id z9so3884546wrq.11 for ; Thu, 24 Oct 2019 08:12:49 -0700 (PDT) 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=ElmGYPJuzUxNnG/FnlffsCILV4+1TBDQ0vpAa4TPRAI=; b=AMyY4eD0oKnX11roRGbCJQE46z4eTJHrm7Kf0sX2x/KWOT81Gq0E9xGs1UJmgyGlR7 zY7FHm1knS6yw/wmz56haWQz449zMyTIQviBO73a8usQxLS73AexwOg+CpkX4kPs9yKp wecFXv9zzChviy4F3aD9B4jmc7fpRXVj2kuoWXGOE2kd90jZmQH1CO1O1c5zCHTzhyxw QtsmOP0n5/ir3NVWg/6k+yDMdv3OB+06yymftSlJ0OiSxXHPWiCIkyHQPEbhaaKuyRa9 S8xIpIsfd+Qc7eVpoFSdGAgll56Cv+Fve+1e2mqfvHriNJtrCn2TjJD/IBvLxPMiP1wk Q+QA== X-Gm-Message-State: APjAAAXLQW7Vql6lmOjzudpRv+6QApzcsH82FxLUSDoSN+L9VLhOW6L1 rZ5AksxN0yYNNHD+M3/YZbRk5p2jAahljV3pgtXnd5wmGJJpT39vAP6vFaJuUCmZ3eE5No1HuPt IKQugIAL+pkPX/A== X-Received: by 2002:a1c:650b:: with SMTP id z11mr5390344wmb.149.1571929968167; Thu, 24 Oct 2019 08:12:48 -0700 (PDT) X-Google-Smtp-Source: APXvYqzS9A1QIEbfxjRI8Pd+fUQr9CdFjZZlVJVMHnRGdHvslyt9rUhlzCaWgjCGu0U4QCIARe+JgA== X-Received: by 2002:a1c:650b:: with SMTP id z11mr5390323wmb.149.1571929967831; Thu, 24 Oct 2019 08:12:47 -0700 (PDT) Received: from [192.168.1.115] (129.red-83-57-174.dynamicip.rima-tde.net. [83.57.174.129]) by smtp.gmail.com with ESMTPSA id e9sm9349671wrr.13.2019.10.24.08.12.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 24 Oct 2019 08:12:47 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers To: Laszlo Ersek , 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: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Thu, 24 Oct 2019 17:12:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: <78640e55-de54-2c02-98d8-2fbdc7922a73@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 v2: >>> =A0=A0=A0=A0 - use QEMU's existent CPU hotplug register block, rathe= r than a new >>> =A0=A0=A0=A0=A0=A0 named file in fw_cfg [Igor] >>> >>> =A0 OvmfPkg/Include/IndustryStandard/I440FxPiix4.h=A0=A0=A0 |=A0 5 += ++ >>> =A0 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h=A0=A0=A0=A0 |=A0 2= + >>> =A0 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 43 >>> ++++++++++++++++++++ >>> =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 BIT10 | BIT9=A0 | BIT8=A0 | BIT7=A0 = | BIT6) >>> =A0 =A0 #define PIIX4_PMREGMISC=A0=A0=A0=A0=A0=A0=A0 0x80 >>> =A0 #define PIIX4_PMREGMISC_PMIOSE=A0=A0 BIT0 >>> =A0 +// >>> +// IO ports >>> +// >>> +#define PIIX4_CPU_HOTPLUG_BASE 0xAF00 >> >> OK >> >>> + >>> =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 // IO ports >>> =A0 // >>> =A0 #define ICH9_APM_CNT 0xB2 >>> =A0 #define ICH9_APM_STS 0xB3 >>> =A0 +#define ICH9_CPU_HOTPLUG_BASE 0x0CD8 >> >> OK >> >>> + >>> =A0 // >>> =A0 // IO ports relative to PMBASE >>> =A0 // >>> =A0 #define ICH9_PMBASE_OFS_SMI_EN 0x30 >>> =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]. >=20 > The documentation (spec) is being cleaned up, and also extended. Please > see the following QEMU patch set (warning: long discussion): >=20 > [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command t= o > cpu hotplug MMIO interface >=20 > http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.c= om >=20 > 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). OK I read it, but it is still RFC, so this patch shouldn't get merged=20 until the QEMU counterpart get merged, right? > 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). >=20 > Therefore, we could call this register >=20 > QEMU_CPUHP_R_RESERVED_0 >=20 > for now, but very soon we'd have to rename it to the above-proposed >=20 > QEMU_CPUHP_R_CMD_DATA2 >=20 > anyway. Again, the code considers both cases. >=20 > (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.) Understood. This patch is fine then. 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. Reviewed-by: Philippe Mathieu-Daude >> >>> + >>> +#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). >=20 > You are looking at the legacy mode of the register block (documented > near the top of the "docs/specs/acpi_cpu_hotplug.txt" file). >=20 > The operations for modern mode are in "hw/acpi/cpu.c", object > "cpu_hotplug_ops". >=20 >=20 >> 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 >=20 > Thanks! > Laszlo >=20 >> >>> + >>> +#endif // QEMU_CPU_HOTPLUG_H_ >>>