Hi Phil,
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.biesheu...@linaro.org>
>>>> Cc: Igor Mammedov <imamm...@redhat.com>
>>>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>>>> Signed-off-by: Laszlo Ersek <ler...@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.
Thanks!
Laszlo
>
> Reviewed-by: Philippe Mathieu-Daude <phi...@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_
>>>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53473): https://edk2.groups.io/g/devel/message/53473
Mute This Topic: https://groups.io/mt/36465006/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
