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.5467.1580309001597330959 for ; Wed, 29 Jan 2020 06:43:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hmSSHEFi; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580309000; 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=sbvgxCuI78FFWd0bWode9sLV1wopJITVr+hNYWwlv7E=; b=hmSSHEFiGECJOMT9BDMeyo+cO5ZHmCICD7eTp0GuVCOEp8QeZwPOPQRzOMU8bTf1elHUzD cLQoqT+OoUFwmZEgwKC5micGtwaIpIR1fbN1Baw7IYePWVLvIOUizQJBjobY00gSq0KKCP o5OwSj6wzTtBOy8ApON7+nhRmYLMFsM= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-191-Dy7ZQrt4PbOe8BpTD8juvw-1; Wed, 29 Jan 2020 09:43:10 -0500 Received: by mail-wr1-f71.google.com with SMTP id i9so10226676wru.1 for ; Wed, 29 Jan 2020 06:43:10 -0800 (PST) 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=22MXBKr4DpQX8VlHt1AdxY++efmUMBJGPyy+axRfdAs=; b=LxXdS48knHWWY4OVaDX5R7UufTzg4dOpVwXO6oljdN9HL61LMJ9trG8FIjy0VuCn+8 osSLSqaoZxKgwu1GkljNhn1BvCyVzbeqrTEc07boPIMxef0HREZFBoybuZVx6i09jaNh ptGfJnnGsO16D7+3yGfg5/inCFxBdmYT9MNZxkpxhMTcMkO6JEVKItYfQ/uzEgj6KAjf 9UxONgvjuCAg7F7vQRAzdWPbIEzBgzDZ2BevwXkXy7DwGSXVCC2KXGNtLMWbH/D+ELkr 2m9CyUueXc+W3b2M8Pt8X2H/iL0u7mNJ7bovQwLxIdLxwnUkG5ZmFkHrecCgJn59dgZj WrtQ== X-Gm-Message-State: APjAAAUC8h44LnfrfmX4G6R7Z8cDSxyWNNETprc7h0UZX5Wg/6oTEaYs noT7GCcIk4MiOVsl2dQFzmI6f4li8ufUYu1wQHkVAdp4q/96vxRvT+KdAw7HBu1X+8oWH7VWJFM xXS4VguDFc3uo3g== X-Received: by 2002:adf:ef92:: with SMTP id d18mr34402762wro.234.1580308988944; Wed, 29 Jan 2020 06:43:08 -0800 (PST) X-Google-Smtp-Source: APXvYqyrY5TBW6f42WG1foiOoe8CbZBsKGCYCgPNQoRZGclUlY17DCq8Oyf+bDE6188PF7BmubgoNA== X-Received: by 2002:adf:ef92:: with SMTP id d18mr34402740wro.234.1580308988672; Wed, 29 Jan 2020 06:43:08 -0800 (PST) Return-Path: Received: from [192.168.1.35] (113.red-83-57-172.dynamicip.rima-tde.net. [83.57.172.113]) by smtp.gmail.com with ESMTPSA id y7sm926047wrr.56.2020.01.29.06.43.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Jan 2020 06:43:07 -0800 (PST) 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> <7eb17416-c10f-95ae-9e2e-2b9220180644@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <2fbe95f0-1c8c-5342-691d-d76b0486f06d@redhat.com> Date: Wed, 29 Jan 2020 15:43:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <7eb17416-c10f-95ae-9e2e-2b9220180644@redhat.com> X-MC-Unique: Dy7ZQrt4PbOe8BpTD8juvw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable Hi Laszlo, On 1/24/20 12:40 PM, Laszlo Ersek wrote: > 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 "= 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 i= n >>>>> 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, r= ather 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 | BIT= 7=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 appeare= d 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 registe= r >>>>> 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. Pleas= e >>> see the following QEMU patch set (warning: long discussion): >>> >>> =A0=A0 [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD com= mand 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@re= dhat.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 no= t >>> 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 >>> >>> =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.) >> >> 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. >=20 > Igor's QEMU work is now upstream: 3e08b2b9cb64..3a61c8db9d25 >=20 > Can you please re-check this patch? No changes should be necessary. >=20 > In particular, >=20 > - 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, >=20 > - 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 serie= s. Re-checked correctly. Thanks for waiting/tracking it get merged. My Reviewed-by stands. >> 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). >>> >>> 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