From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>, devel@edk2.groups.io
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Igor Mammedov <imammedo@redhat.com>,
Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers
Date: Wed, 29 Jan 2020 15:43:07 +0100 [thread overview]
Message-ID: <2fbe95f0-1c8c-5342-691d-d76b0486f06d@redhat.com> (raw)
In-Reply-To: <7eb17416-c10f-95ae-9e2e-2b9220180644@redhat.com>
Hi Laszlo,
On 1/24/20 12:40 PM, Laszlo Ersek wrote:
> On 10/24/19 17:12, Philippe Mathieu-Daudé wrote:
>> On 10/24/19 12:29 PM, Laszlo Ersek wrote:
>>> On 10/23/19 14:05, Philippe Mathieu-Daudé 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 "CPU
>>>>> 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 can
>>>>> count the possible CPUs (as opposed to boot CPUs) in a later patch in
>>>>> this
>>>>> series.
>>>>>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> v2:
>>>>> - use QEMU's existent CPU hotplug register block, rather than
>>>>> a new
>>>>> named file in fw_cfg [Igor]
>>>>>
>>>>> OvmfPkg/Include/IndustryStandard/I440FxPiix4.h | 5 +++
>>>>> OvmfPkg/Include/IndustryStandard/Q35MchIch9.h | 2 +
>>>>> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 43
>>>>> ++++++++++++++++++++
>>>>> 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 @@
>>>>> BIT10 | BIT9 | BIT8 | BIT7 |
>>>>> BIT6)
>>>>> #define PIIX4_PMREGMISC 0x80
>>>>> #define PIIX4_PMREGMISC_PMIOSE BIT0
>>>>> +//
>>>>> +// IO ports
>>>>> +//
>>>>> +#define PIIX4_CPU_HOTPLUG_BASE 0xAF00
>>>>
>>>> OK
>>>>
>>>>> +
>>>>> #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 @@
>>>>> // IO ports
>>>>> //
>>>>> #define ICH9_APM_CNT 0xB2
>>>>> #define ICH9_APM_STS 0xB3
>>>>> +#define ICH9_CPU_HOTPLUG_BASE 0x0CD8
>>>>
>>>> OK
>>>>
>>>>> +
>>>>> //
>>>>> // IO ports relative to PMBASE
>>>>> //
>>>>> #define ICH9_PMBASE_OFS_SMI_EN 0x30
>>>>> #define ICH9_SMI_EN_APMC_EN 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
>>>>> + Macros for accessing QEMU's CPU hotplug register block.
>>>>> +
>>>>> + Copyright (C) 2019, Red Hat, Inc.
>>>>> +
>>>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>> +
>>>>> + @par Specification Reference:
>>>>> +
>>>>> + - "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source tree.
>>>>> +
>>>>> + The original (now "legacy") CPU hotplug interface appeared in
>>>>> QEMU v1.5.0.
>>>>> + The new ("modern") hotplug interface appeared in QEMU v2.7.0.
>>>>> +
>>>>> + The macros in this header file map to the minimal subset of the
>>>>> modern
>>>>> + interface that OVMF needs.
>>>>> +**/
>>>>> +
>>>>> +#ifndef QEMU_CPU_HOTPLUG_H_
>>>>> +#define QEMU_CPU_HOTPLUG_H_
>>>>> +
>>>>> +#include <Base.h>
>>>>> +
>>>>> +//
>>>>> +// 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
>>>>> +// register,
>>>>> +// - followed by distinguished bitmasks or values in the register.
>>>>> +//
>>>>> +#define QEMU_CPUHP_R_CMD_DATA2 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):
>>>
>>> [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
>>> cpu hotplug MMIO interface
>>>
>>> http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.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).
>>
>> OK I read it, but it is still RFC, so this patch shouldn't get merged
>> 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).
>>>
>>> Therefore, we could call this register
>>>
>>> QEMU_CPUHP_R_RESERVED_0
>>>
>>> for now, but very soon we'd have to rename it to the above-proposed
>>>
>>> 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.)
>>
>> 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.
>
> 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.
Re-checked correctly. Thanks for waiting/tracking it get merged.
My Reviewed-by stands.
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>>>>
>>>>> +
>>>>> +#define QEMU_CPUHP_R_CPU_STAT 0x4
>>>>> +#define QEMU_CPUHP_STAT_ENABLED BIT0
>>>>
>>>> OK
>>>>
>>>>> +
>>>>> +#define QEMU_CPUHP_RW_CMD_DATA 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 0x0
>>>>
>>>> Another "DWORD access". OK.
>>>>
>>>>> +
>>>>> +#define QEMU_CPUHP_W_CMD 0x5
>>>>> +#define QEMU_CPUHP_CMD_GET_PENDING 0x0
>>>>
>>>> 'PENDING' is more meaningful than "CPU device with
>>>> inserting/removing events". OK
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>
>>>>> +
>>>>> +#endif // QEMU_CPU_HOTPLUG_H_
>>>>>
>>
>>
>>
>
next prev parent reply other threads:[~2020-01-29 14:43 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-22 22:15 [PATCH v2 0/3] OvmfPkg: distinguish boot CPU count from possible CPU count Laszlo Ersek
2019-10-22 22:15 ` [PATCH v2 1/3] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults Laszlo Ersek
2019-10-24 14:18 ` Anthony PERARD
2019-10-22 22:15 ` [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers Laszlo Ersek
2019-10-23 12:05 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-10-24 10:29 ` Laszlo Ersek
2019-10-24 15:12 ` Philippe Mathieu-Daudé
2019-10-25 8:21 ` Laszlo Ersek
2020-01-24 11:40 ` Laszlo Ersek
2020-01-29 14:43 ` Philippe Mathieu-Daudé [this message]
2020-01-29 17:30 ` Laszlo Ersek
2019-10-22 22:15 ` [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug Laszlo Ersek
2019-10-24 14:27 ` Anthony PERARD
2019-10-24 15:28 ` Laszlo Ersek
2019-10-24 15:33 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-10-25 8:29 ` Laszlo Ersek
2019-10-25 9:17 ` Philippe Mathieu-Daudé
2019-10-23 8:52 ` [PATCH v2 0/3] OvmfPkg: distinguish boot CPU count from possible CPU count Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2fbe95f0-1c8c-5342-691d-d76b0486f06d@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox