* [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2
@ 2019-12-04 17:05 Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
` (7 more replies)
0 siblings, 8 replies; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Series consists of 2 parts: 1st is lockable SMRAM at SMBASE
and the 2nd adds means to enumerate APIC IDs for possible CPUs.
1st part [1-2/8]:
In order to support CPU hotplug in secure boot mode,
UEFI firmware needs to relocate SMI handler of hotplugged CPU,
in a way that won't allow ring 0 user to break in priveleged
SMM mode that firmware maintains during runtime.
Used approach allows to hide RAM at default SMBASE to make it
accessible only to SMM mode, which lets us to make sure that
SMI handler installed by firmware can not be hijacked by
unpriveleged user (similar to TSEG behavior).
2nd part:
mostly fixes and extra documentation on how to detect and use
modern CPU hotplug interface (MMIO block).
So firmware could reuse it for enumerating possible CPUs and
detecting hotplugged CPU(s). It also adds support for
CPHP_GET_CPU_ID_CMD command [7/8], which should allow firmware
to fetch APIC IDs for possible CPUs which is necessary for
initializing internal structures for possible CPUs on boot.
CC: mst@redhat.com
CC: pbonzini@redhat.com
CC: lersek@redhat.com
CC: philmd@redhat.com
Igor Mammedov (8):
q35: implement 128K SMRAM at default SMBASE address
tests: q35: MCH: add default SMBASE SMRAM lock test
acpi: cpuhp: spec: clarify 'CPU selector' register usage and
endianness
acpi: cpuhp: spec: fix 'Command data' description
acpi: cpuhp: spec: clarify store into 'Command data' when 'Command
field' == 0
acpi: cpuhp: spec: add typical usecases
acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
include/hw/pci-host/q35.h | 10 ++++
docs/specs/acpi_cpu_hotplug.txt | 91 +++++++++++++++++++++++++++-------
hw/acpi/cpu.c | 15 ++++++
hw/acpi/trace-events | 1 +
hw/i386/pc.c | 4 +-
hw/pci-host/q35.c | 80 +++++++++++++++++++++++++++---
tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 281 insertions(+), 25 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 10:43 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 2/8] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
` (6 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Use commit (2f295167e0 q35/mch: implement extended TSEG sizes) for
inspiration and (ab)use reserved register in config space at 0x9c
offset [*] to extend q35 pci-host with ability to use 128K at
0x30000 as SMRAM and hide it (like TSEG) from non-SMM context.
Usage:
1: write 0xff in the register
2: if the feature is supported, follow up read from the register
should return 0x01. At this point RAM at 0x30000 is still
available for SMI handler configuration from non-SMM context
3: writing 0x02 in the register, locks SMBASE area, making its contents
available only from SMM context. In non-SMM context, reads return
0xff and writes are ignored. Further writes into the register are
ignored until the system reset.
*) https://www.mail-archive.com/qemu-devel@nongnu.org/msg455991.html
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/pci-host/q35.h | 10 ++++++
hw/i386/pc.c | 4 ++-
hw/pci-host/q35.c | 80 ++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 86 insertions(+), 8 deletions(-)
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index b3bcf2e..976fbae 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -32,6 +32,7 @@
#include "hw/acpi/ich9.h"
#include "hw/pci-host/pam.h"
#include "hw/i386/intel_iommu.h"
+#include "qemu/units.h"
#define TYPE_Q35_HOST_DEVICE "q35-pcihost"
#define Q35_HOST_DEVICE(obj) \
@@ -54,6 +55,8 @@ typedef struct MCHPCIState {
MemoryRegion smram_region, open_high_smram;
MemoryRegion smram, low_smram, high_smram;
MemoryRegion tseg_blackhole, tseg_window;
+ MemoryRegion smbase_blackhole, smbase_window;
+ bool has_smram_at_smbase;
Range pci_hole;
uint64_t below_4g_mem_size;
uint64_t above_4g_mem_size;
@@ -97,6 +100,13 @@ typedef struct Q35PCIHost {
#define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY 0xffff
#define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX 0xfff
+#define MCH_HOST_BRIDGE_SMBASE_SIZE (128 * KiB)
+#define MCH_HOST_BRIDGE_SMBASE_ADDR 0x30000
+#define MCH_HOST_BRIDGE_F_SMBASE 0x9c
+#define MCH_HOST_BRIDGE_F_SMBASE_QUERY 0xff
+#define MCH_HOST_BRIDGE_F_SMBASE_IN_RAM 0x01
+#define MCH_HOST_BRIDGE_F_SMBASE_LCK 0x02
+
#define MCH_HOST_BRIDGE_PCIEXBAR 0x60 /* 64bit register */
#define MCH_HOST_BRIDGE_PCIEXBAR_SIZE 8 /* 64bit register */
#define MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT 0xb0000000
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ac08e63..9c4b4ac 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -103,7 +103,9 @@
struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
-GlobalProperty pc_compat_4_1[] = {};
+GlobalProperty pc_compat_4_1[] = {
+ { "mch", "smbase-smram", "off" },
+};
const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
GlobalProperty pc_compat_4_0[] = {};
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270..c1bd9f7 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -275,20 +275,20 @@ static const TypeInfo q35_host_info = {
* MCH D0:F0
*/
-static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
+static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
{
return 0xffffffff;
}
-static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
- unsigned width)
+static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned width)
{
/* nothing */
}
-static const MemoryRegionOps tseg_blackhole_ops = {
- .read = tseg_blackhole_read,
- .write = tseg_blackhole_write,
+static const MemoryRegionOps blackhole_ops = {
+ .read = blackhole_read,
+ .write = blackhole_write,
.endianness = DEVICE_NATIVE_ENDIAN,
.valid.min_access_size = 1,
.valid.max_access_size = 4,
@@ -430,6 +430,46 @@ static void mch_update_ext_tseg_mbytes(MCHPCIState *mch)
}
}
+static void mch_update_smbase_smram(MCHPCIState *mch)
+{
+ PCIDevice *pd = PCI_DEVICE(mch);
+ uint8_t *reg = pd->config + MCH_HOST_BRIDGE_F_SMBASE;
+ bool lck;
+
+ if (!mch->has_smram_at_smbase) {
+ return;
+ }
+
+ if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
+ MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
+ return;
+ }
+
+ /*
+ * default/reset state, discard written value
+ * which will disable SMRAM balackhole at SMBASE
+ */
+ if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
+ *reg = 0x00;
+ }
+
+ memory_region_transaction_begin();
+ if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
+ /* disable all writes */
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
+ ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ lck = true;
+ } else {
+ lck = false;
+ }
+ memory_region_set_enabled(&mch->smbase_blackhole, lck);
+ memory_region_set_enabled(&mch->smbase_window, lck);
+ memory_region_transaction_commit();
+}
+
static void mch_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len)
{
@@ -456,6 +496,10 @@ static void mch_write_config(PCIDevice *d,
MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_SIZE)) {
mch_update_ext_tseg_mbytes(mch);
}
+
+ if (ranges_overlap(address, len, MCH_HOST_BRIDGE_F_SMBASE, 1)) {
+ mch_update_smbase_smram(mch);
+ }
}
static void mch_update(MCHPCIState *mch)
@@ -464,6 +508,7 @@ static void mch_update(MCHPCIState *mch)
mch_update_pam(mch);
mch_update_smram(mch);
mch_update_ext_tseg_mbytes(mch);
+ mch_update_smbase_smram(mch);
/*
* pci hole goes from end-of-low-ram to io-apic.
@@ -514,6 +559,9 @@ static void mch_reset(DeviceState *qdev)
MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY);
}
+ d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
+ d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
+
mch_update(mch);
}
@@ -563,7 +611,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
memory_region_add_subregion(&mch->smram, 0xfeda0000, &mch->high_smram);
memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
- &tseg_blackhole_ops, NULL,
+ &blackhole_ops, NULL,
"tseg-blackhole", 0);
memory_region_set_enabled(&mch->tseg_blackhole, false);
memory_region_add_subregion_overlap(mch->system_memory,
@@ -575,6 +623,23 @@ static void mch_realize(PCIDevice *d, Error **errp)
memory_region_set_enabled(&mch->tseg_window, false);
memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
&mch->tseg_window);
+
+ memory_region_init_io(&mch->smbase_blackhole, OBJECT(mch), &blackhole_ops,
+ NULL, "smbase-blackhole",
+ MCH_HOST_BRIDGE_SMBASE_SIZE);
+ memory_region_set_enabled(&mch->smbase_blackhole, false);
+ memory_region_add_subregion_overlap(mch->system_memory,
+ MCH_HOST_BRIDGE_SMBASE_ADDR,
+ &mch->smbase_blackhole, 1);
+
+ memory_region_init_alias(&mch->smbase_window, OBJECT(mch),
+ "smbase-window", mch->ram_memory,
+ MCH_HOST_BRIDGE_SMBASE_ADDR,
+ MCH_HOST_BRIDGE_SMBASE_SIZE);
+ memory_region_set_enabled(&mch->smbase_window, false);
+ memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
+ &mch->smbase_window);
+
object_property_add_const_link(qdev_get_machine(), "smram",
OBJECT(&mch->smram), &error_abort);
@@ -601,6 +666,7 @@ uint64_t mch_mcfg_base(void)
static Property mch_props[] = {
DEFINE_PROP_UINT16("extended-tseg-mbytes", MCHPCIState, ext_tseg_mbytes,
16),
+ DEFINE_PROP_BOOL("smbase-smram", MCHPCIState, has_smram_at_smbase, true),
DEFINE_PROP_END_OF_LIST(),
};
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 2/8] tests: q35: MCH: add default SMBASE SMRAM lock test
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
` (5 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
test lockable SMRAM at default SMBASE feature, introduced by
patch "q35: implement 128K SMRAM at default SMBASE address"
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
tests/q35-test.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/tests/q35-test.c b/tests/q35-test.c
index a68183d..dd02660 100644
--- a/tests/q35-test.c
+++ b/tests/q35-test.c
@@ -186,6 +186,109 @@ static void test_tseg_size(const void *data)
qtest_quit(qts);
}
+#define SMBASE 0x30000
+#define SMRAM_TEST_PATTERN 0x32
+#define SMRAM_TEST_RESET_PATTERN 0x23
+
+static void test_smram_smbase_lock(void)
+{
+ QPCIBus *pcibus;
+ QPCIDevice *pcidev;
+ QDict *response;
+ QTestState *qts;
+ int i;
+
+ qts = qtest_init("-M q35");
+
+ pcibus = qpci_new_pc(qts, NULL);
+ g_assert(pcibus != NULL);
+
+ pcidev = qpci_device_find(pcibus, 0);
+ g_assert(pcidev != NULL);
+
+ /* check that SMRAM is not enabled by default */
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+
+ /* check that writinng junk to 0x9c before before negotiating is ignorred */
+ for (i = 0; i < 0xff; i++) {
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ }
+
+ /* enable SMRAM at SMBASE */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);
+ /* lock SMRAM at SMBASE */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0x02);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x02);
+
+ /* check that SMRAM at SMBASE is locked and can't be unlocked */
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, 0xff);
+ for (i = 0; i <= 0xff; i++) {
+ /* make sure register is immutable */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x02);
+
+ /* RAM access should go inot black hole */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, 0xff);
+ }
+
+ /* reset */
+ response = qtest_qmp(qts, "{'execute': 'system_reset', 'arguments': {} }");
+ g_assert(response);
+ g_assert(!qdict_haskey(response, "error"));
+ qobject_unref(response);
+
+ /* check RAM at SMBASE is available after reset */
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_RESET_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_RESET_PATTERN);
+
+ g_free(pcidev);
+ qpci_free_pc(pcibus);
+
+ qtest_quit(qts);
+}
+
+static void test_without_smram_base(void)
+{
+ QPCIBus *pcibus;
+ QPCIDevice *pcidev;
+ QTestState *qts;
+ int i;
+
+ qts = qtest_init("-M pc-q35-4.1");
+
+ pcibus = qpci_new_pc(qts, NULL);
+ g_assert(pcibus != NULL);
+
+ pcidev = qpci_device_find(pcibus, 0);
+ g_assert(pcidev != NULL);
+
+ /* check that RAM accessible */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+
+ /* check that writing to 0x9c succeeds */
+ for (i = 0; i <= 0xff; i++) {
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == i);
+ }
+
+ /* check that RAM is still accessible */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN + 1);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, (SMRAM_TEST_PATTERN + 1));
+
+ g_free(pcidev);
+ qpci_free_pc(pcibus);
+
+ qtest_quit(qts);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -197,5 +300,7 @@ int main(int argc, char **argv)
qtest_add_data_func("/q35/tseg-size/8mb", &tseg_8mb, test_tseg_size);
qtest_add_data_func("/q35/tseg-size/ext/16mb", &tseg_ext_16mb,
test_tseg_size);
+ qtest_add_func("/q35/smram/smbase_lock", test_smram_smbase_lock);
+ qtest_add_func("/q35/smram/legacy_smbase", test_without_smram_base);
return g_test_run();
}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 2/8] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 11:50 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
` (4 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
* Move reserved registers to the top of the section, so reader would be
aware of effects when reading registers description.
* State registers endianness explicitly at the beginning of the section
* Describe registers behavior in case of 'CPU selector' register contains
value that doesn't point to a possible CPU.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index ee219c8..4e65286 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -30,6 +30,18 @@ Register block base address:
Register block size:
ACPI_CPU_HOTPLUG_REG_LEN = 12
+All accesses to registers described below, imply little-endian byte order.
+
+Reserved resisters behavior:
+ - write accesses are ignored
+ - read accesses return all bits set to 0.
+
+The last stored value in 'CPU selector' must refer to a possible CPU, otherwise
+ - reads from any register return 0
+ - writes to any other register are ignored until valid value is stored into it
+On QEMU start, 'CPU selector' is initialized to a valid value, on reset it
+keeps the current value.
+
read access:
offset:
[0x0-0x3] reserved
@@ -86,9 +98,3 @@ write access:
ACPI_DEVICE_OST QMP event from QEMU to external applications
with current values of OST event and status registers.
other values: reserved
-
-Selecting CPU device beyond possible range has no effect on platform:
- - write accesses to CPU hot-plug registers not documented above are
- ignored
- - read accesses to CPU hot-plug registers not documented above return
- all bits set to 0.
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (2 preceding siblings ...)
2019-12-04 17:05 ` [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 12:17 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
` (3 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Correct returned value description in case 'Command field' == 0x0,
it's in not PXM but CPU selector value with pending event
In addition describe 0 blanket value in case of not supported
'Command field' value.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 4e65286..19c508f 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -56,9 +56,8 @@ read access:
3-7: reserved and should be ignored by OSPM
[0x5-0x7] reserved
[0x8] Command data: (DWORD access)
- in case of error or unsupported command reads is 0xFFFFFFFF
- current 'Command field' value:
- 0: returns PXM value corresponding to device
+ contains 0 unless last stored in 'Command field' value is one of:
+ 0: contains 'CPU selector' value of a CPU with pending event[s]
write access:
offset:
@@ -81,9 +80,9 @@ write access:
value:
0: selects a CPU device with inserting/removing events and
following reads from 'Command data' register return
- selected CPU (CPU selector value). If no CPU with events
- found, the current CPU selector doesn't change and
- corresponding insert/remove event flags are not set.
+ selected CPU ('CPU selector' value).
+ If no CPU with events found, the current 'CPU selector' doesn't
+ change and corresponding insert/remove event flags are not set.
1: following writes to 'Command data' register set OST event
register in QEMU
2: following writes to 'Command data' register set OST status
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (3 preceding siblings ...)
2019-12-04 17:05 ` [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 12:21 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases Igor Mammedov
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Write section of 'Command data' register should describe what happens
when it's written into. Correct description in case the last stored
'Command field' value equals to 0, to reflect that currently it's not
supported.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 19c508f..f3c552d 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -90,8 +90,7 @@ write access:
other values: reserved
[0x6-0x7] reserved
[0x8] Command data: (DWORD access)
- current 'Command field' value:
- 0: OSPM reads value of CPU selector
+ if last stored 'Command field' value:
1: stores value into OST event register
2: stores value into OST status register, triggers
ACPI_DEVICE_OST QMP event from QEMU to external applications
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (4 preceding siblings ...)
2019-12-04 17:05 ` [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 12:29 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug Igor Mammedov
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Document work-flows for
* finding a CPU with pending 'insert/remove' event
* enumerating present and possible CPUs
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index f3c552d..58c16c6 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -64,6 +64,7 @@ write access:
[0x0-0x3] CPU selector: (DWORD access)
selects active CPU device. All following accesses to other
registers will read/store data from/to selected CPU.
+ Valid values: [0 .. max_cpus)
[0x4] CPU device control fields: (1 byte access)
bits:
0: reserved, OSPM must clear it before writing to register.
@@ -96,3 +97,31 @@ write access:
ACPI_DEVICE_OST QMP event from QEMU to external applications
with current values of OST event and status registers.
other values: reserved
+
+ Typical usecases:
+ - Get a cpu with pending event
+ 1. Store 0x0 to the 'CPU selector' register.
+ 2. Store 0x0 to the 'Command field' register.
+ 3. Read the 'CPU device status fields' register.
+ 4. If both bit#1 and bit#2 are clear in the value read, there is no CPU
+ with a pending event and selected CPU remains unchanged.
+ 5. Otherwise, read the 'Command data' register. The value read is the
+ selector of the CPU with the pending event (which is already
+ selected).
+
+ - Enumerate CPUs present/non present CPUs
+ 01. Set the present CPU count to 0.
+ 02. Set the iterator to 0.
+ 03. Store 0x0 to the 'CPU selector' register, to ensure that it's in
+ a valid state and that access to other registers won't be ignored.
+ 04. Store 0x0 to the 'Command field' register to make 'Command data'
+ register return 'CPU selector' value of selected CPU
+ 05. Read the 'CPU device status fields' register.
+ 06. If bit#0 is set, increment the present CPU count.
+ 07. Increment the iterator.
+ 08. Store the iterator to the 'CPU selector' register.
+ 09. Read the 'Command data' register.
+ 10. If the value read is not zero, goto 05.
+ 11. Otherwise store 0x0 to the 'CPU selector' register, to put it
+ into a valid state and exit.
+ The iterator at this point equals "max_cpus".
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (5 preceding siblings ...)
2019-12-04 17:05 ` [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 13:03 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug Igor Mammedov
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Extend CPU hotplug interface to return architecture specific
identifier for current CPU in 2 registers:
- lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW
- upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at
offset 0.
Target user is UEFI firmware, which needs a way to enumerate
all CPUs (including possible CPUs) to allocate and initialize
CPU structures on boot.
(for x86: it needs APIC ID and later command will be used to
retrieve ARM's MPIDR which serves the similar to APIC ID purpose)
The new ACPI_CPU_CMD_DATA2_OFFSET_R register will also be used
to detect presence of modern CPU hotplug, which will be described
in follow up patch.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
- s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/.
---
docs/specs/acpi_cpu_hotplug.txt | 10 ++++++++--
hw/acpi/cpu.c | 15 +++++++++++++++
hw/acpi/trace-events | 1 +
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 58c16c6..bb33144 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -44,7 +44,11 @@ keeps the current value.
read access:
offset:
- [0x0-0x3] reserved
+ [0x0-0x3] Command data 2: (DWORD access)
+ if last stored 'Command field' value:
+ 3: upper 32 bits of architecture specific identifying CPU value
+ (n x86 case: 0x0)
+ other values: reserved
[0x4] CPU device status fields: (1 byte access)
bits:
0: Device is enabled and may be used by guest
@@ -96,7 +100,9 @@ write access:
2: stores value into OST status register, triggers
ACPI_DEVICE_OST QMP event from QEMU to external applications
with current values of OST event and status registers.
- other values: reserved
+ 3: lower 32 bit of architecture specific identifier
+ (in x86 case: APIC ID)
+ other values: reserved
Typical usecases:
- Get a cpu with pending event
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 87f30a3..87813ce 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -12,11 +12,13 @@
#define ACPI_CPU_FLAGS_OFFSET_RW 4
#define ACPI_CPU_CMD_OFFSET_WR 5
#define ACPI_CPU_CMD_DATA_OFFSET_RW 8
+#define ACPI_CPU_CMD_DATA2_OFFSET_R 0
enum {
CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
CPHP_OST_EVENT_CMD = 1,
CPHP_OST_STATUS_CMD = 2,
+ CPHP_GET_CPU_ID_CMD = 3,
CPHP_CMD_MAX
};
@@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
val = cpu_st->selector;
break;
+ case CPHP_GET_CPU_ID_CMD:
+ val = cdev->arch_id & 0xFFFFFFFF;
+ break;
default:
break;
}
trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val);
break;
+ case ACPI_CPU_CMD_DATA2_OFFSET_R:
+ switch (cpu_st->command) {
+ case CPHP_GET_CPU_ID_CMD:
+ val = cdev->arch_id >> 32;
+ break;
+ default:
+ break;
+ }
+ trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val);
+ break;
default:
break;
}
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 96b8273..afbc77d 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%"
cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32
cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8
cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32
+cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32
cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d"
cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]"
cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (6 preceding siblings ...)
2019-12-04 17:05 ` [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
@ 2019-12-04 17:05 ` Igor Mammedov
2019-12-05 14:07 ` Laszlo Ersek
7 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-04 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Describe how to enable and detect modern CPU hotplug interface.
Detection part is based on new CPHP_GET_CPU_ID_CMD command,
introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index bb33144..667b264 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -15,14 +15,14 @@ CPU present bitmap for:
PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
The first DWORD in bitmap is used in write mode to switch from legacy
- to new CPU hotplug interface, write 0 into it to do switch.
+ to modern CPU hotplug interface, write 0 into it to do switch.
---------------------------------------------------------------
QEMU sets corresponding CPU bit on hot-add event and issues SCI
with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
=====================================
-ACPI CPU hotplug interface registers:
+Modern ACPI CPU hotplug interface registers:
-------------------------------------
Register block base address:
ICH9-LPC IO port 0x0cd8
@@ -105,6 +105,24 @@ write access:
other values: reserved
Typical usecases:
+ - (x86) Detecting and enabling modern CPU hotplug interface.
+ QEMU starts with legacy CPU hotplug interface enabled. Detecting and
+ switching to modern interface is based on the 2 legacy CPU hotplug features:
+ 1. Writes into CPU bitmap are ignored.
+ 2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
+
+ Use following steps to detect and enable modern CPU hotplug interface:
+ 1. Store 0x0 to the 'CPU selector' register,
+ attempting to switch to modern mode
+ 2. Store 0x0 to the 'CPU selector' register,
+ to ensure valid selector value
+ 3. Store 0x3 to the 'Command field' register,
+ sets the 'Command data 2' register into architecture specific
+ CPU identifier mode
+ 4. Read the 'Command data 2' register.
+ If read value is 0x0, the modern interface is enabled.
+ Otherwise legacy or no CPU hotplug interface available
+
- Get a cpu with pending event
1. Store 0x0 to the 'CPU selector' register.
2. Store 0x0 to the 'Command field' register.
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address
2019-12-04 17:05 ` [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
@ 2019-12-05 10:43 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 10:43 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
Hi Igor,
On 12/04/19 18:05, Igor Mammedov wrote:
> Use commit (2f295167e0 q35/mch: implement extended TSEG sizes) for
> inspiration and (ab)use reserved register in config space at 0x9c
> offset [*] to extend q35 pci-host with ability to use 128K at
> 0x30000 as SMRAM and hide it (like TSEG) from non-SMM context.
>
> Usage:
> 1: write 0xff in the register
> 2: if the feature is supported, follow up read from the register
> should return 0x01. At this point RAM at 0x30000 is still
> available for SMI handler configuration from non-SMM context
> 3: writing 0x02 in the register, locks SMBASE area, making its contents
> available only from SMM context. In non-SMM context, reads return
> 0xff and writes are ignored. Further writes into the register are
> ignored until the system reset.
>
> *) https://www.mail-archive.com/qemu-devel@nongnu.org/msg455991.html
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/hw/pci-host/q35.h | 10 ++++++
> hw/i386/pc.c | 4 ++-
> hw/pci-host/q35.c | 80 ++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 86 insertions(+), 8 deletions(-)
I have now applied this series of yours on a local topic branch, on top
of v4.2.0-rc4.
Earlier, I applied your
[Qemu-devel] [PATCH 0/2]
q35: mch: allow to lock down 128K RAM at default SMBASE address
on top of commit f8c3db33a5e8 ("target/sparc: Switch to
do_transaction_failed() hook", 2019-09-17), on another local topic
branch. Those patches can be found at:
http://mid.mail-archive.com/20190917130708.10281-2-imammedo@redhat.com
http://mid.mail-archive.com/20190917130708.10281-3-imammedo@redhat.com
I have now run "git range-diff" to compare the first two patches
between both series. They are identical, except for:
- a small (harmless) context difference in patch #1,
- a slight commit message update in patch #2.
See below:
> 1: 09b22eeda732 ! 1: 5aafb1228e86 q35: implement 128K SMRAM at default SMBASE address
> @@ -20,7 +20,7 @@
> *) https://www.mail-archive.com/qemu-devel@nongnu.org/msg455991.html
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> - Message-Id: <20190917130708.10281-2-imammedo@redhat.com>
> + Message-Id: <1575479147-6641-2-git-send-email-imammedo@redhat.com>
>
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> --- a/include/hw/pci-host/q35.h
> @@ -61,8 +61,8 @@
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@
> - /* Physical Address of PVH entry point read from kernel ELF NOTE */
> - static size_t pvh_start_addr;
> +
> + struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> -GlobalProperty pc_compat_4_1[] = {};
> +GlobalProperty pc_compat_4_1[] = {
> 2: feeb6a5262d4 ! 2: f1a896f4dbc6 tests: q35: MCH: add default SMBASE SMRAM lock test
> @@ -2,11 +2,11 @@
>
> tests: q35: MCH: add default SMBASE SMRAM lock test
>
> - test lockable SMRAM at default SMBASE feature introduced by
> - commit "q35: implement 128K SMRAM at default SMBASE address"
> + test lockable SMRAM at default SMBASE feature, introduced by
> + patch "q35: implement 128K SMRAM at default SMBASE address"
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> - Message-Id: <20190917130708.10281-3-imammedo@redhat.com>
> + Message-Id: <1575479147-6641-3-git-send-email-imammedo@redhat.com>
>
> diff --git a/tests/q35-test.c b/tests/q35-test.c
> --- a/tests/q35-test.c
Therefore, my Tested-by that I gave in the earlier thread, applies
still:
http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-e74181c5a862@redhat.com
(I gave that T-b after writing and posting the interfacing OVMF patches,
which have been reviewed since, and waiting for the QEMU patches to go
in first.)
However, at this time, I'd like to propose two (easy) updates:
(1) In message
http://mid.mail-archive.com/6799e19d-1aa8-ee09-9ef4-2533a7241360@redhat.com
Paolo said he was OK with this approach, but he asked for comments
stating that "this is not what real hardware does".
Can you please add such comments to the code?
Then, please see below for another comment:
On 12/04/19 18:05, Igor Mammedov wrote:
>
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index b3bcf2e..976fbae 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -32,6 +32,7 @@
> #include "hw/acpi/ich9.h"
> #include "hw/pci-host/pam.h"
> #include "hw/i386/intel_iommu.h"
> +#include "qemu/units.h"
>
> #define TYPE_Q35_HOST_DEVICE "q35-pcihost"
> #define Q35_HOST_DEVICE(obj) \
> @@ -54,6 +55,8 @@ typedef struct MCHPCIState {
> MemoryRegion smram_region, open_high_smram;
> MemoryRegion smram, low_smram, high_smram;
> MemoryRegion tseg_blackhole, tseg_window;
> + MemoryRegion smbase_blackhole, smbase_window;
> + bool has_smram_at_smbase;
> Range pci_hole;
> uint64_t below_4g_mem_size;
> uint64_t above_4g_mem_size;
> @@ -97,6 +100,13 @@ typedef struct Q35PCIHost {
> #define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY 0xffff
> #define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX 0xfff
>
> +#define MCH_HOST_BRIDGE_SMBASE_SIZE (128 * KiB)
> +#define MCH_HOST_BRIDGE_SMBASE_ADDR 0x30000
> +#define MCH_HOST_BRIDGE_F_SMBASE 0x9c
> +#define MCH_HOST_BRIDGE_F_SMBASE_QUERY 0xff
> +#define MCH_HOST_BRIDGE_F_SMBASE_IN_RAM 0x01
> +#define MCH_HOST_BRIDGE_F_SMBASE_LCK 0x02
> +
> #define MCH_HOST_BRIDGE_PCIEXBAR 0x60 /* 64bit register */
> #define MCH_HOST_BRIDGE_PCIEXBAR_SIZE 8 /* 64bit register */
> #define MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT 0xb0000000
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ac08e63..9c4b4ac 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -103,7 +103,9 @@
>
> struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> -GlobalProperty pc_compat_4_1[] = {};
> +GlobalProperty pc_compat_4_1[] = {
> + { "mch", "smbase-smram", "off" },
> +};
> const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
>
> GlobalProperty pc_compat_4_0[] = {};
(2) Because this series is now being proposed for QEMU 5.0, I think this
compat property should be introduced for machine types up to and
including 4.2, not 4.1.
... Technically, this change will invalidate my Tested-by (I will not
have tested your *exact* (upcoming) v2 patch, i.e. the one including the
4.2 bump, so we can't carry the T-b forward). Thus, I will re-test your
v2 (with the extra comments and the 4.2 bump).
I have no other comments for the first two patches in this series.
Thanks!
Laszlo
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 158d270..c1bd9f7 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -275,20 +275,20 @@ static const TypeInfo q35_host_info = {
> * MCH D0:F0
> */
>
> -static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
> +static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
> {
> return 0xffffffff;
> }
>
> -static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> - unsigned width)
> +static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned width)
> {
> /* nothing */
> }
>
> -static const MemoryRegionOps tseg_blackhole_ops = {
> - .read = tseg_blackhole_read,
> - .write = tseg_blackhole_write,
> +static const MemoryRegionOps blackhole_ops = {
> + .read = blackhole_read,
> + .write = blackhole_write,
> .endianness = DEVICE_NATIVE_ENDIAN,
> .valid.min_access_size = 1,
> .valid.max_access_size = 4,
> @@ -430,6 +430,46 @@ static void mch_update_ext_tseg_mbytes(MCHPCIState *mch)
> }
> }
>
> +static void mch_update_smbase_smram(MCHPCIState *mch)
> +{
> + PCIDevice *pd = PCI_DEVICE(mch);
> + uint8_t *reg = pd->config + MCH_HOST_BRIDGE_F_SMBASE;
> + bool lck;
> +
> + if (!mch->has_smram_at_smbase) {
> + return;
> + }
> +
> + if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
> + pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> + MCH_HOST_BRIDGE_F_SMBASE_LCK;
> + *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
> + return;
> + }
> +
> + /*
> + * default/reset state, discard written value
> + * which will disable SMRAM balackhole at SMBASE
> + */
> + if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> + *reg = 0x00;
> + }
> +
> + memory_region_transaction_begin();
> + if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
> + /* disable all writes */
> + pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
> + ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
> + *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> + lck = true;
> + } else {
> + lck = false;
> + }
> + memory_region_set_enabled(&mch->smbase_blackhole, lck);
> + memory_region_set_enabled(&mch->smbase_window, lck);
> + memory_region_transaction_commit();
> +}
> +
> static void mch_write_config(PCIDevice *d,
> uint32_t address, uint32_t val, int len)
> {
> @@ -456,6 +496,10 @@ static void mch_write_config(PCIDevice *d,
> MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_SIZE)) {
> mch_update_ext_tseg_mbytes(mch);
> }
> +
> + if (ranges_overlap(address, len, MCH_HOST_BRIDGE_F_SMBASE, 1)) {
> + mch_update_smbase_smram(mch);
> + }
> }
>
> static void mch_update(MCHPCIState *mch)
> @@ -464,6 +508,7 @@ static void mch_update(MCHPCIState *mch)
> mch_update_pam(mch);
> mch_update_smram(mch);
> mch_update_ext_tseg_mbytes(mch);
> + mch_update_smbase_smram(mch);
>
> /*
> * pci hole goes from end-of-low-ram to io-apic.
> @@ -514,6 +559,9 @@ static void mch_reset(DeviceState *qdev)
> MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY);
> }
>
> + d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
> + d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
> +
> mch_update(mch);
> }
>
> @@ -563,7 +611,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
> memory_region_add_subregion(&mch->smram, 0xfeda0000, &mch->high_smram);
>
> memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
> - &tseg_blackhole_ops, NULL,
> + &blackhole_ops, NULL,
> "tseg-blackhole", 0);
> memory_region_set_enabled(&mch->tseg_blackhole, false);
> memory_region_add_subregion_overlap(mch->system_memory,
> @@ -575,6 +623,23 @@ static void mch_realize(PCIDevice *d, Error **errp)
> memory_region_set_enabled(&mch->tseg_window, false);
> memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
> &mch->tseg_window);
> +
> + memory_region_init_io(&mch->smbase_blackhole, OBJECT(mch), &blackhole_ops,
> + NULL, "smbase-blackhole",
> + MCH_HOST_BRIDGE_SMBASE_SIZE);
> + memory_region_set_enabled(&mch->smbase_blackhole, false);
> + memory_region_add_subregion_overlap(mch->system_memory,
> + MCH_HOST_BRIDGE_SMBASE_ADDR,
> + &mch->smbase_blackhole, 1);
> +
> + memory_region_init_alias(&mch->smbase_window, OBJECT(mch),
> + "smbase-window", mch->ram_memory,
> + MCH_HOST_BRIDGE_SMBASE_ADDR,
> + MCH_HOST_BRIDGE_SMBASE_SIZE);
> + memory_region_set_enabled(&mch->smbase_window, false);
> + memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
> + &mch->smbase_window);
> +
> object_property_add_const_link(qdev_get_machine(), "smram",
> OBJECT(&mch->smram), &error_abort);
>
> @@ -601,6 +666,7 @@ uint64_t mch_mcfg_base(void)
> static Property mch_props[] = {
> DEFINE_PROP_UINT16("extended-tseg-mbytes", MCHPCIState, ext_tseg_mbytes,
> 16),
> + DEFINE_PROP_BOOL("smbase-smram", MCHPCIState, has_smram_at_smbase, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness
2019-12-04 17:05 ` [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
@ 2019-12-05 11:50 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 11:50 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> * Move reserved registers to the top of the section, so reader would be
> aware of effects when reading registers description.
> * State registers endianness explicitly at the beginning of the section
> * Describe registers behavior in case of 'CPU selector' register contains
> value that doesn't point to a possible CPU.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index ee219c8..4e65286 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -30,6 +30,18 @@ Register block base address:
> Register block size:
> ACPI_CPU_HOTPLUG_REG_LEN = 12
>
> +All accesses to registers described below, imply little-endian byte order.
> +
> +Reserved resisters behavior:
> + - write accesses are ignored
> + - read accesses return all bits set to 0.
> +
> +The last stored value in 'CPU selector' must refer to a possible CPU, otherwise
> + - reads from any register return 0
> + - writes to any other register are ignored until valid value is stored into it
> +On QEMU start, 'CPU selector' is initialized to a valid value, on reset it
> +keeps the current value.
> +
> read access:
> offset:
> [0x0-0x3] reserved
> @@ -86,9 +98,3 @@ write access:
> ACPI_DEVICE_OST QMP event from QEMU to external applications
> with current values of OST event and status registers.
> other values: reserved
> -
> -Selecting CPU device beyond possible range has no effect on platform:
> - - write accesses to CPU hot-plug registers not documented above are
> - ignored
> - - read accesses to CPU hot-plug registers not documented above return
> - all bits set to 0.
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description
2019-12-04 17:05 ` [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
@ 2019-12-05 12:17 ` Laszlo Ersek
2019-12-06 11:09 ` Igor Mammedov
0 siblings, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 12:17 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> Correct returned value description in case 'Command field' == 0x0,
> it's in not PXM but CPU selector value with pending event
(1) s/in not/not/
>
> In addition describe 0 blanket value in case of not supported
> 'Command field' value.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 4e65286..19c508f 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -56,9 +56,8 @@ read access:
> 3-7: reserved and should be ignored by OSPM
> [0x5-0x7] reserved
> [0x8] Command data: (DWORD access)
> - in case of error or unsupported command reads is 0xFFFFFFFF
> - current 'Command field' value:
> - 0: returns PXM value corresponding to device
> + contains 0 unless last stored in 'Command field' value is one of:
> + 0: contains 'CPU selector' value of a CPU with pending event[s]
(2) I think we can improve the word order:
last stored in 'Command field' value
->
value last stored in 'Command field'
>
> write access:
> offset:
> @@ -81,9 +80,9 @@ write access:
> value:
> 0: selects a CPU device with inserting/removing events and
> following reads from 'Command data' register return
> - selected CPU (CPU selector value). If no CPU with events
> - found, the current CPU selector doesn't change and
> - corresponding insert/remove event flags are not set.
> + selected CPU ('CPU selector' value).
> + If no CPU with events found, the current 'CPU selector' doesn't
> + change and corresponding insert/remove event flags are not set.
(3) AFAICT this is only a -- useful! -- re-wrapping. But, since we are
modifying this section anyway, can we replace "flags are not set" with
"flags are left unchanged" or "flags are not modified"?
"set" is ambiguous with bit fields: it can mean "rewritten", and it can
mean "set to 1".
> 1: following writes to 'Command data' register set OST event
> register in QEMU
> 2: following writes to 'Command data' register set OST status
>
Anyway, these are all superficial comments. Pick up whatever you agree
with. Either way:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0
2019-12-04 17:05 ` [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
@ 2019-12-05 12:21 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 12:21 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> Write section of 'Command data' register should describe what happens
> when it's written into. Correct description in case the last stored
> 'Command field' value equals to 0, to reflect that currently it's not
s/equals to/equals/
or
s/equals to/is equal to/
With that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> supported.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 19c508f..f3c552d 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -90,8 +90,7 @@ write access:
> other values: reserved
> [0x6-0x7] reserved
> [0x8] Command data: (DWORD access)
> - current 'Command field' value:
> - 0: OSPM reads value of CPU selector
> + if last stored 'Command field' value:
> 1: stores value into OST event register
> 2: stores value into OST status register, triggers
> ACPI_DEVICE_OST QMP event from QEMU to external applications
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases
2019-12-04 17:05 ` [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases Igor Mammedov
@ 2019-12-05 12:29 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 12:29 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> Document work-flows for
> * finding a CPU with pending 'insert/remove' event
> * enumerating present and possible CPUs
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index f3c552d..58c16c6 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -64,6 +64,7 @@ write access:
> [0x0-0x3] CPU selector: (DWORD access)
> selects active CPU device. All following accesses to other
> registers will read/store data from/to selected CPU.
> + Valid values: [0 .. max_cpus)
> [0x4] CPU device control fields: (1 byte access)
> bits:
> 0: reserved, OSPM must clear it before writing to register.
> @@ -96,3 +97,31 @@ write access:
> ACPI_DEVICE_OST QMP event from QEMU to external applications
> with current values of OST event and status registers.
> other values: reserved
> +
> + Typical usecases:
> + - Get a cpu with pending event
> + 1. Store 0x0 to the 'CPU selector' register.
> + 2. Store 0x0 to the 'Command field' register.
> + 3. Read the 'CPU device status fields' register.
> + 4. If both bit#1 and bit#2 are clear in the value read, there is no CPU
> + with a pending event and selected CPU remains unchanged.
> + 5. Otherwise, read the 'Command data' register. The value read is the
> + selector of the CPU with the pending event (which is already
> + selected).
> +
> + - Enumerate CPUs present/non present CPUs
> + 01. Set the present CPU count to 0.
> + 02. Set the iterator to 0.
> + 03. Store 0x0 to the 'CPU selector' register, to ensure that it's in
> + a valid state and that access to other registers won't be ignored.
> + 04. Store 0x0 to the 'Command field' register to make 'Command data'
> + register return 'CPU selector' value of selected CPU
> + 05. Read the 'CPU device status fields' register.
> + 06. If bit#0 is set, increment the present CPU count.
> + 07. Increment the iterator.
> + 08. Store the iterator to the 'CPU selector' register.
> + 09. Read the 'Command data' register.
> + 10. If the value read is not zero, goto 05.
> + 11. Otherwise store 0x0 to the 'CPU selector' register, to put it
> + into a valid state and exit.
> + The iterator at this point equals "max_cpus".
>
Nice!
I suggest unindenting the entire new text, to the leftmost column
(column#0) in the text file. Otherwise "Typical usecases" falls under
the scope of "write access".
With that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-04 17:05 ` [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
@ 2019-12-05 13:03 ` Laszlo Ersek
2019-12-06 15:15 ` Igor Mammedov
0 siblings, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 13:03 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> Extend CPU hotplug interface to return architecture specific
> identifier for current CPU in 2 registers:
> - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW
> - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at
> offset 0.
OK.
> Target user is UEFI firmware, which needs a way to enumerate
> all CPUs (including possible CPUs) to allocate and initialize
> CPU structures on boot.
(1) This is correct in general, but if we want to keep this description,
then it should be moved to the commit message of the previous patch.
CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it
will be necessary for handling the hotplug SMI.
For the boot time allocation / initialization, the "enumerating present
and possible CPUs" workflow is necessary, and that is documented in the
previous patch in this series.
So if we want to keep this paragraph, we should move it to the previous
patch's commit message.
> (for x86: it needs APIC ID and later command will be used to
> retrieve ARM's MPIDR which serves the similar to APIC ID purpose)
(2) I would suggest some punctuation, to make this clearer. How about:
> On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching
> the APIC ID when handling the hotplug SMI.
>
> Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
> which serves a purpose similar to the x86 APIC ID.
Back to the patch:
On 12/04/19 18:05, Igor Mammedov wrote:
>
> The new ACPI_CPU_CMD_DATA2_OFFSET_R register will also be used
> to detect presence of modern CPU hotplug, which will be described
> in follow up patch.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v1:
> - s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/.
> ---
> docs/specs/acpi_cpu_hotplug.txt | 10 ++++++++--
> hw/acpi/cpu.c | 15 +++++++++++++++
> hw/acpi/trace-events | 1 +
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 58c16c6..bb33144 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -44,7 +44,11 @@ keeps the current value.
>
> read access:
> offset:
> - [0x0-0x3] reserved
> + [0x0-0x3] Command data 2: (DWORD access)
> + if last stored 'Command field' value:
> + 3: upper 32 bits of architecture specific identifying CPU value
> + (n x86 case: 0x0)
> + other values: reserved
> [0x4] CPU device status fields: (1 byte access)
> bits:
> 0: Device is enabled and may be used by guest
OK.
> @@ -96,7 +100,9 @@ write access:
> 2: stores value into OST status register, triggers
> ACPI_DEVICE_OST QMP event from QEMU to external applications
> with current values of OST event and status registers.
> - other values: reserved
> + 3: lower 32 bit of architecture specific identifier
> + (in x86 case: APIC ID)
> + other values: reserved
>
> Typical usecases:
> - Get a cpu with pending event
(3) This fragment has been pasted in the wrong spot. We should add this
under:
> read access:
> ...
> [0x8] Command data: (DWORD access)
> contains 0 unless last stored in 'Command field' value is one of:
> 0: contains 'CPU selector' value of a CPU with pending event[s]
But right now, the addition is in the context of:
> write access:
> ...
> [0x8] Command data: (DWORD access)
> if last stored 'Command field' value:
> 1: stores value into OST event register
> 2: stores value into OST status register, triggers
> ACPI_DEVICE_OST QMP event from QEMU to external applications
> with current values of OST event and status registers.
> 3: lower 32 bit of architecture specific identifier
> (in x86 case: APIC ID)
> other values: reserved
and it does not make sense there.
(4) Furthermore, for consistency, please write "32 bits" (plural).
Back to the patch:
On 12/04/19 18:05, Igor Mammedov wrote:
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 87f30a3..87813ce 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -12,11 +12,13 @@
> #define ACPI_CPU_FLAGS_OFFSET_RW 4
> #define ACPI_CPU_CMD_OFFSET_WR 5
> #define ACPI_CPU_CMD_DATA_OFFSET_RW 8
> +#define ACPI_CPU_CMD_DATA2_OFFSET_R 0
>
> enum {
> CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
> CPHP_OST_EVENT_CMD = 1,
> CPHP_OST_STATUS_CMD = 2,
> + CPHP_GET_CPU_ID_CMD = 3,
> CPHP_CMD_MAX
> };
>
> @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
> case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
> val = cpu_st->selector;
> break;
> + case CPHP_GET_CPU_ID_CMD:
> + val = cdev->arch_id & 0xFFFFFFFF;
> + break;
> default:
> break;
> }
> trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val);
> break;
> + case ACPI_CPU_CMD_DATA2_OFFSET_R:
> + switch (cpu_st->command) {
> + case CPHP_GET_CPU_ID_CMD:
> + val = cdev->arch_id >> 32;
> + break;
> + default:
> + break;
> + }
> + trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val);
> + break;
> default:
> break;
> }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index 96b8273..afbc77d 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags: 0x%"
> cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32
> cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 0x%"PRIx8
> cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32
> +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 0x%"PRIx32
> cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] inserting: %d, removing: %d"
> cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]"
> cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"
>
The code looks good to me.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-04 17:05 ` [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug Igor Mammedov
@ 2019-12-05 14:07 ` Laszlo Ersek
2019-12-06 10:40 ` Igor Mammedov
2019-12-06 13:49 ` Igor Mammedov
0 siblings, 2 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-05 14:07 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/04/19 18:05, Igor Mammedov wrote:
> Describe how to enable and detect modern CPU hotplug interface.
> Detection part is based on new CPHP_GET_CPU_ID_CMD command,
> introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
Could we make this usecase / workflow independent of the new
CPHP_GET_CPU_ID_CMD command please?
I'd like to suggest the following:
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index bb33144..667b264 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -15,14 +15,14 @@ CPU present bitmap for:
> PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
> One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
> The first DWORD in bitmap is used in write mode to switch from legacy
> - to new CPU hotplug interface, write 0 into it to do switch.
> + to modern CPU hotplug interface, write 0 into it to do switch.
> ---------------------------------------------------------------
> QEMU sets corresponding CPU bit on hot-add event and issues SCI
> with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
> to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
>
> =====================================
> -ACPI CPU hotplug interface registers:
> +Modern ACPI CPU hotplug interface registers:
> -------------------------------------
> Register block base address:
> ICH9-LPC IO port 0x0cd8
> @@ -105,6 +105,24 @@ write access:
> other values: reserved
>
> Typical usecases:
> + - (x86) Detecting and enabling modern CPU hotplug interface.
(1) I think we can drop the (x86) restriction. (Because, we don't need
to depend on APIC ID specifics; see below.)
> + QEMU starts with legacy CPU hotplug interface enabled. Detecting and
> + switching to modern interface is based on the 2 legacy CPU hotplug features:
> + 1. Writes into CPU bitmap are ignored.
> + 2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
> +
> + Use following steps to detect and enable modern CPU hotplug interface:
> + 1. Store 0x0 to the 'CPU selector' register,
> + attempting to switch to modern mode
> + 2. Store 0x0 to the 'CPU selector' register,
> + to ensure valid selector value
OK thus far.
> + 3. Store 0x3 to the 'Command field' register,
> + sets the 'Command data 2' register into architecture specific
> + CPU identifier mode
(2) Can we please store command 0 here
(CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)?
That might change the selector value, yes. (Even if that happens, the
new selector will be *valid*.)
But the main point is that, with 0 stored to the command register, one
of the following *four* cases will hold, subsequently:
(2a) if register block is totally missing:
--> Offset#0 will read as all-bits-one (unassigned read)
(2b) if register block is legacy-only:
--> Offset#0 will read as nonzero, due to CPU#0 being always present
(2c) if the modern register block is active, but the "Command data 2"
register is *not* yet described in the spec file:
--> Offset#0 will read as zero, because it is *reserved*:
> read access:
> offset:
> [0x0-0x3] reserved <---- HERE
(2d) if the modern register block is active, and the "Command data 2"
register *is* described in the spec file:
--> the "Command data 2" register (at offset#0) will read as zero,
because:
> read access:
> offset:
> [0x0-0x3] Command data 2: (DWORD access)
> if last stored 'Command field' value:
> 3: upper 32 bits of architecture specific identifying CPU value
> (n x86 case: 0x0)
> other values: reserved <------ HERE
and then step#4 applies just the same:
On 12/04/19 18:05, Igor Mammedov wrote:
> + 4. Read the 'Command data 2' register.
> + If read value is 0x0, the modern interface is enabled.
> + Otherwise legacy or no CPU hotplug interface available
> +
because "read value is 0x0" corresponds to the *union* of cases (2c) and
(2d) -- namely, "the modern register block is active".
My proposal above is what I implemented for OVMF in October:
[edk2-devel] [PATCH v2 3/3]
OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
http://mid.mail-archive.com/20191022221554.14963-4-lersek@redhat.com
and it works very well.
So the benefits would be:
- I wouldn't have to rewrite that (complex!) patch :) ,
- the logic would not store the new CPHP_GET_CPU_ID_CMD command, only
read offset#0,
- the logic would not be x86 specific (it would not have to rely on the
most significant 32 bits of the 64-bit arch-specific CPU identifier --
here: the APIC ID -- being zero).
Furthermore,
(3) In step#4, I suggest replacing 'Command data 2' with "offset 0",
(4) finally, I'd suggest squashing this patch (updated as requested
above) into patch "acpi: cpuhp: spec: add typical usecases".
Using my suggestion (2), you can define the "modern interface
enablement" workflow as well, without any dependency on
CPHP_GET_CPU_ID_CMD. The only thing that's necessary is the small update
from (3), and then you can describe all three important use cases in one
go, in patch#6.
And then you can introduce CPHP_GET_CPU_ID_CMD in the last patch
(patch#7), while staying compatible with the previously-documented
workflows.
Pretty please? :)
Thanks!
Laszlo
> - Get a cpu with pending event
> 1. Store 0x0 to the 'CPU selector' register.
> 2. Store 0x0 to the 'Command field' register.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-05 14:07 ` Laszlo Ersek
@ 2019-12-06 10:40 ` Igor Mammedov
2019-12-06 12:02 ` Laszlo Ersek
2019-12-06 13:49 ` Igor Mammedov
1 sibling, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-06 10:40 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, philmd, qemu-devel, mst
On Thu, 5 Dec 2019 15:07:53 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/04/19 18:05, Igor Mammedov wrote:
> > Describe how to enable and detect modern CPU hotplug interface.
> > Detection part is based on new CPHP_GET_CPU_ID_CMD command,
> > introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
>
> Could we make this usecase / workflow independent of the new
> CPHP_GET_CPU_ID_CMD command please?
>
> I'd like to suggest the following:
>
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> > index bb33144..667b264 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -15,14 +15,14 @@ CPU present bitmap for:
> > PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
> > One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
> > The first DWORD in bitmap is used in write mode to switch from legacy
> > - to new CPU hotplug interface, write 0 into it to do switch.
> > + to modern CPU hotplug interface, write 0 into it to do switch.
> > ---------------------------------------------------------------
> > QEMU sets corresponding CPU bit on hot-add event and issues SCI
> > with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
> > to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
> >
> > =====================================
> > -ACPI CPU hotplug interface registers:
> > +Modern ACPI CPU hotplug interface registers:
> > -------------------------------------
> > Register block base address:
> > ICH9-LPC IO port 0x0cd8
> > @@ -105,6 +105,24 @@ write access:
> > other values: reserved
> >
> > Typical usecases:
> > + - (x86) Detecting and enabling modern CPU hotplug interface.
>
> (1) I think we can drop the (x86) restriction. (Because, we don't need
> to depend on APIC ID specifics; see below.)
>
> > + QEMU starts with legacy CPU hotplug interface enabled. Detecting and
> > + switching to modern interface is based on the 2 legacy CPU hotplug features:
> > + 1. Writes into CPU bitmap are ignored.
> > + 2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
> > +
> > + Use following steps to detect and enable modern CPU hotplug interface:
> > + 1. Store 0x0 to the 'CPU selector' register,
> > + attempting to switch to modern mode
> > + 2. Store 0x0 to the 'CPU selector' register,
> > + to ensure valid selector value
>
> OK thus far.
>
> > + 3. Store 0x3 to the 'Command field' register,
> > + sets the 'Command data 2' register into architecture specific
> > + CPU identifier mode
>
> (2) Can we please store command 0 here
> (CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)?
that could work too, as far "Command data 2" is defined before
we use it here.
Point is to define "Command data 2" state with command 0 and leave our hands
free when it comes to reserved (so it won't get in a way in the future
if we need to 'unreserve' it in some context)
>
> That might change the selector value, yes. (Even if that happens, the
> new selector will be *valid*.)
>
> But the main point is that, with 0 stored to the command register, one
> of the following *four* cases will hold, subsequently:
>
> (2a) if register block is totally missing:
>
> --> Offset#0 will read as all-bits-one (unassigned read)
>
> (2b) if register block is legacy-only:
>
> --> Offset#0 will read as nonzero, due to CPU#0 being always present
>
> (2c) if the modern register block is active, but the "Command data 2"
> register is *not* yet described in the spec file:
>
> --> Offset#0 will read as zero, because it is *reserved*:
>
> > read access:
> > offset:
> > [0x0-0x3] reserved <---- HERE
>
> (2d) if the modern register block is active, and the "Command data 2"
> register *is* described in the spec file:
>
> --> the "Command data 2" register (at offset#0) will read as zero,
> because:
>
> > read access:
> > offset:
> > [0x0-0x3] Command data 2: (DWORD access)
> > if last stored 'Command field' value:
> > 3: upper 32 bits of architecture specific identifying CPU value
> > (n x86 case: 0x0)
> > other values: reserved <------ HERE
>
> and then step#4 applies just the same:
>
> On 12/04/19 18:05, Igor Mammedov wrote:
> > + 4. Read the 'Command data 2' register.
> > + If read value is 0x0, the modern interface is enabled.
> > + Otherwise legacy or no CPU hotplug interface available
> > +
>
> because "read value is 0x0" corresponds to the *union* of cases (2c) and
> (2d) -- namely, "the modern register block is active".
>
> My proposal above is what I implemented for OVMF in October:
>
> [edk2-devel] [PATCH v2 3/3]
> OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
>
> http://mid.mail-archive.com/20191022221554.14963-4-lersek@redhat.com
>
> and it works very well.
>
> So the benefits would be:
>
> - I wouldn't have to rewrite that (complex!) patch :) ,
>
> - the logic would not store the new CPHP_GET_CPU_ID_CMD command, only
> read offset#0,
>
> - the logic would not be x86 specific (it would not have to rely on the
> most significant 32 bits of the 64-bit arch-specific CPU identifier --
> here: the APIC ID -- being zero).
>
> Furthermore,
>
> (3) In step#4, I suggest replacing 'Command data 2' with "offset 0",
Spec uses field names so I'd rather use 'Command data 2' here instead of
direct offset to be consistent with the rest of the spec.
> (4) finally, I'd suggest squashing this patch (updated as requested
> above) into patch "acpi: cpuhp: spec: add typical usecases".
>
> Using my suggestion (2), you can define the "modern interface
> enablement" workflow as well, without any dependency on
> CPHP_GET_CPU_ID_CMD. The only thing that's necessary is the small update
> from (3), and then you can describe all three important use cases in one
> go, in patch#6.
I'd add extra patch that defines 'Command data 2' for command 0
and after that it should be possible to squash detection usecase
into 6/8.
> And then you can introduce CPHP_GET_CPU_ID_CMD in the last patch
> (patch#7), while staying compatible with the previously-documented
> workflows.
>
> Pretty please? :)
>
> Thanks!
> Laszlo
>
> > - Get a cpu with pending event
> > 1. Store 0x0 to the 'CPU selector' register.
> > 2. Store 0x0 to the 'Command field' register.
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description
2019-12-05 12:17 ` Laszlo Ersek
@ 2019-12-06 11:09 ` Igor Mammedov
2019-12-06 12:00 ` Laszlo Ersek
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-06 11:09 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, philmd, qemu-devel, mst
On Thu, 5 Dec 2019 13:17:22 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/04/19 18:05, Igor Mammedov wrote:
> > Correct returned value description in case 'Command field' == 0x0,
> > it's in not PXM but CPU selector value with pending event
>
> (1) s/in not/not/
>
> >
> > In addition describe 0 blanket value in case of not supported
> > 'Command field' value.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > docs/specs/acpi_cpu_hotplug.txt | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> > index 4e65286..19c508f 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -56,9 +56,8 @@ read access:
> > 3-7: reserved and should be ignored by OSPM
> > [0x5-0x7] reserved
> > [0x8] Command data: (DWORD access)
> > - in case of error or unsupported command reads is 0xFFFFFFFF
> > - current 'Command field' value:
> > - 0: returns PXM value corresponding to device
> > + contains 0 unless last stored in 'Command field' value is one of:
> > + 0: contains 'CPU selector' value of a CPU with pending event[s]
>
> (2) I think we can improve the word order:
>
> last stored in 'Command field' value
> ->
> value last stored in 'Command field'
>
> >
> > write access:
> > offset:
> > @@ -81,9 +80,9 @@ write access:
> > value:
> > 0: selects a CPU device with inserting/removing events and
> > following reads from 'Command data' register return
> > - selected CPU (CPU selector value). If no CPU with events
> > - found, the current CPU selector doesn't change and
> > - corresponding insert/remove event flags are not set.
> > + selected CPU ('CPU selector' value).
> > + If no CPU with events found, the current 'CPU selector' doesn't
> > + change and corresponding insert/remove event flags are not set.
>
> (3) AFAICT this is only a -- useful! -- re-wrapping.
Not sure what you are trying to say here ...
> But, since we are
> modifying this section anyway, can we replace "flags are not set" with
> "flags are left unchanged" or "flags are not modified"?
sure
> "set" is ambiguous with bit fields: it can mean "rewritten", and it can
> mean "set to 1".
>
> > 1: following writes to 'Command data' register set OST event
> > register in QEMU
> > 2: following writes to 'Command data' register set OST status
> >
>
> Anyway, these are all superficial comments. Pick up whatever you agree
> with. Either way:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description
2019-12-06 11:09 ` Igor Mammedov
@ 2019-12-06 12:00 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-06 12:00 UTC (permalink / raw)
To: Igor Mammedov; +Cc: pbonzini, philmd, qemu-devel, mst
On 12/06/19 12:09, Igor Mammedov wrote:
> On Thu, 5 Dec 2019 13:17:22 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 12/04/19 18:05, Igor Mammedov wrote:
>>> Correct returned value description in case 'Command field' == 0x0,
>>> it's in not PXM but CPU selector value with pending event
>>
>> (1) s/in not/not/
>>
>>>
>>> In addition describe 0 blanket value in case of not supported
>>> 'Command field' value.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> docs/specs/acpi_cpu_hotplug.txt | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
>>> index 4e65286..19c508f 100644
>>> --- a/docs/specs/acpi_cpu_hotplug.txt
>>> +++ b/docs/specs/acpi_cpu_hotplug.txt
>>> @@ -56,9 +56,8 @@ read access:
>>> 3-7: reserved and should be ignored by OSPM
>>> [0x5-0x7] reserved
>>> [0x8] Command data: (DWORD access)
>>> - in case of error or unsupported command reads is 0xFFFFFFFF
>>> - current 'Command field' value:
>>> - 0: returns PXM value corresponding to device
>>> + contains 0 unless last stored in 'Command field' value is one of:
>>> + 0: contains 'CPU selector' value of a CPU with pending event[s]
>>
>> (2) I think we can improve the word order:
>>
>> last stored in 'Command field' value
>> ->
>> value last stored in 'Command field'
>>
>>>
>>> write access:
>>> offset:
>>> @@ -81,9 +80,9 @@ write access:
>>> value:
>>> 0: selects a CPU device with inserting/removing events and
>>> following reads from 'Command data' register return
>>> - selected CPU (CPU selector value). If no CPU with events
>>> - found, the current CPU selector doesn't change and
>>> - corresponding insert/remove event flags are not set.
>>> + selected CPU ('CPU selector' value).
>>> + If no CPU with events found, the current 'CPU selector' doesn't
>>> + change and corresponding insert/remove event flags are not set.
>>
>> (3) AFAICT this is only a -- useful! -- re-wrapping.
> Not sure what you are trying to say here ...
I meant that you re-wrapped the last sentence, by breaking it off to a
separate line. And that I found it useful.
Thanks
laszlo
>
>> But, since we are
>> modifying this section anyway, can we replace "flags are not set" with
>> "flags are left unchanged" or "flags are not modified"?
> sure
>
>
>> "set" is ambiguous with bit fields: it can mean "rewritten", and it can
>> mean "set to 1".
>>
>>> 1: following writes to 'Command data' register set OST event
>>> register in QEMU
>>> 2: following writes to 'Command data' register set OST status
>>>
>>
>> Anyway, these are all superficial comments. Pick up whatever you agree
>> with. Either way:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks!
>> Laszlo
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-06 10:40 ` Igor Mammedov
@ 2019-12-06 12:02 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-06 12:02 UTC (permalink / raw)
To: Igor Mammedov; +Cc: pbonzini, philmd, qemu-devel, mst
On 12/06/19 11:40, Igor Mammedov wrote:
> On Thu, 5 Dec 2019 15:07:53 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 12/04/19 18:05, Igor Mammedov wrote:
>>> Describe how to enable and detect modern CPU hotplug interface.
>>> Detection part is based on new CPHP_GET_CPU_ID_CMD command,
>>> introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> Could we make this usecase / workflow independent of the new
>> CPHP_GET_CPU_ID_CMD command please?
>>
>> I'd like to suggest the following:
>>
>>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
>>> index bb33144..667b264 100644
>>> --- a/docs/specs/acpi_cpu_hotplug.txt
>>> +++ b/docs/specs/acpi_cpu_hotplug.txt
>>> @@ -15,14 +15,14 @@ CPU present bitmap for:
>>> PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
>>> One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
>>> The first DWORD in bitmap is used in write mode to switch from legacy
>>> - to new CPU hotplug interface, write 0 into it to do switch.
>>> + to modern CPU hotplug interface, write 0 into it to do switch.
>>> ---------------------------------------------------------------
>>> QEMU sets corresponding CPU bit on hot-add event and issues SCI
>>> with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
>>> to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
>>>
>>> =====================================
>>> -ACPI CPU hotplug interface registers:
>>> +Modern ACPI CPU hotplug interface registers:
>>> -------------------------------------
>>> Register block base address:
>>> ICH9-LPC IO port 0x0cd8
>>> @@ -105,6 +105,24 @@ write access:
>>> other values: reserved
>>>
>>> Typical usecases:
>>> + - (x86) Detecting and enabling modern CPU hotplug interface.
>>
>> (1) I think we can drop the (x86) restriction. (Because, we don't need
>> to depend on APIC ID specifics; see below.)
>>
>>> + QEMU starts with legacy CPU hotplug interface enabled. Detecting and
>>> + switching to modern interface is based on the 2 legacy CPU hotplug features:
>>> + 1. Writes into CPU bitmap are ignored.
>>> + 2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
>>> +
>>> + Use following steps to detect and enable modern CPU hotplug interface:
>>> + 1. Store 0x0 to the 'CPU selector' register,
>>> + attempting to switch to modern mode
>>> + 2. Store 0x0 to the 'CPU selector' register,
>>> + to ensure valid selector value
>>
>> OK thus far.
>>
>>> + 3. Store 0x3 to the 'Command field' register,
>>> + sets the 'Command data 2' register into architecture specific
>>> + CPU identifier mode
>>
>> (2) Can we please store command 0 here
>> (CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)?
>
> that could work too, as far "Command data 2" is defined before
> we use it here.
> Point is to define "Command data 2" state with command 0 and leave our hands
> free when it comes to reserved (so it won't get in a way in the future
> if we need to 'unreserve' it in some context)
>
>>
>> That might change the selector value, yes. (Even if that happens, the
>> new selector will be *valid*.)
>>
>> But the main point is that, with 0 stored to the command register, one
>> of the following *four* cases will hold, subsequently:
>>
>> (2a) if register block is totally missing:
>>
>> --> Offset#0 will read as all-bits-one (unassigned read)
>>
>> (2b) if register block is legacy-only:
>>
>> --> Offset#0 will read as nonzero, due to CPU#0 being always present
>>
>> (2c) if the modern register block is active, but the "Command data 2"
>> register is *not* yet described in the spec file:
>>
>> --> Offset#0 will read as zero, because it is *reserved*:
>>
>>> read access:
>>> offset:
>>> [0x0-0x3] reserved <---- HERE
>>
>> (2d) if the modern register block is active, and the "Command data 2"
>> register *is* described in the spec file:
>>
>> --> the "Command data 2" register (at offset#0) will read as zero,
>> because:
>>
>>> read access:
>>> offset:
>>> [0x0-0x3] Command data 2: (DWORD access)
>>> if last stored 'Command field' value:
>>> 3: upper 32 bits of architecture specific identifying CPU value
>>> (n x86 case: 0x0)
>>> other values: reserved <------ HERE
>>
>> and then step#4 applies just the same:
>>
>> On 12/04/19 18:05, Igor Mammedov wrote:
>>> + 4. Read the 'Command data 2' register.
>>> + If read value is 0x0, the modern interface is enabled.
>>> + Otherwise legacy or no CPU hotplug interface available
>>> +
>>
>> because "read value is 0x0" corresponds to the *union* of cases (2c) and
>> (2d) -- namely, "the modern register block is active".
>>
>> My proposal above is what I implemented for OVMF in October:
>>
>> [edk2-devel] [PATCH v2 3/3]
>> OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
>>
>> http://mid.mail-archive.com/20191022221554.14963-4-lersek@redhat.com
>>
>> and it works very well.
>>
>> So the benefits would be:
>>
>> - I wouldn't have to rewrite that (complex!) patch :) ,
>>
>> - the logic would not store the new CPHP_GET_CPU_ID_CMD command, only
>> read offset#0,
>>
>> - the logic would not be x86 specific (it would not have to rely on the
>> most significant 32 bits of the 64-bit arch-specific CPU identifier --
>> here: the APIC ID -- being zero).
>>
>> Furthermore,
>>
>> (3) In step#4, I suggest replacing 'Command data 2' with "offset 0",
> Spec uses field names so I'd rather use 'Command data 2' here instead of
> direct offset to be consistent with the rest of the spec.
>
>
>> (4) finally, I'd suggest squashing this patch (updated as requested
>> above) into patch "acpi: cpuhp: spec: add typical usecases".
>>
>> Using my suggestion (2), you can define the "modern interface
>> enablement" workflow as well, without any dependency on
>> CPHP_GET_CPU_ID_CMD. The only thing that's necessary is the small update
>> from (3), and then you can describe all three important use cases in one
>> go, in patch#6.
>
> I'd add extra patch that defines 'Command data 2' for command 0
> and after that it should be possible to squash detection usecase
> into 6/8.
Sounds good!
Thanks!
Laszlo
>
>> And then you can introduce CPHP_GET_CPU_ID_CMD in the last patch
>> (patch#7), while staying compatible with the previously-documented
>> workflows.
>>
>> Pretty please? :)
>>
>> Thanks!
>> Laszlo
>>
>>> - Get a cpu with pending event
>>> 1. Store 0x0 to the 'CPU selector' register.
>>> 2. Store 0x0 to the 'Command field' register.
>>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-05 14:07 ` Laszlo Ersek
2019-12-06 10:40 ` Igor Mammedov
@ 2019-12-06 13:49 ` Igor Mammedov
2019-12-06 15:06 ` Laszlo Ersek
1 sibling, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-06 13:49 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, philmd, qemu-devel, mst
On Thu, 5 Dec 2019 15:07:53 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/04/19 18:05, Igor Mammedov wrote:
> > Describe how to enable and detect modern CPU hotplug interface.
> > Detection part is based on new CPHP_GET_CPU_ID_CMD command,
> > introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
>
> Could we make this usecase / workflow independent of the new
> CPHP_GET_CPU_ID_CMD command please?
>
> I'd like to suggest the following:
>
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> > index bb33144..667b264 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -15,14 +15,14 @@ CPU present bitmap for:
> > PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
> > One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
> > The first DWORD in bitmap is used in write mode to switch from legacy
> > - to new CPU hotplug interface, write 0 into it to do switch.
> > + to modern CPU hotplug interface, write 0 into it to do switch.
> > ---------------------------------------------------------------
> > QEMU sets corresponding CPU bit on hot-add event and issues SCI
> > with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
> > to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
> >
> > =====================================
> > -ACPI CPU hotplug interface registers:
> > +Modern ACPI CPU hotplug interface registers:
> > -------------------------------------
> > Register block base address:
> > ICH9-LPC IO port 0x0cd8
> > @@ -105,6 +105,24 @@ write access:
> > other values: reserved
> >
> > Typical usecases:
> > + - (x86) Detecting and enabling modern CPU hotplug interface.
>
> (1) I think we can drop the (x86) restriction. (Because, we don't need
> to depend on APIC ID specifics; see below.)
I'd rather keep it x86 specific, as enabling interface and talking about
legacy bitmap applies only to x86 impl.
ARM one won't have any of it, it will just be enabled in a future QEMU
(probably even without version-ed machine type).
So could we just say usual "use firmware X.Y with QEMU Z to use CPU hotplug"
in that case?
Do we really need it to be arch agnostic?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug
2019-12-06 13:49 ` Igor Mammedov
@ 2019-12-06 15:06 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-06 15:06 UTC (permalink / raw)
To: Igor Mammedov; +Cc: pbonzini, philmd, qemu-devel, mst
On 12/06/19 14:49, Igor Mammedov wrote:
> On Thu, 5 Dec 2019 15:07:53 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 12/04/19 18:05, Igor Mammedov wrote:
>>> Describe how to enable and detect modern CPU hotplug interface.
>>> Detection part is based on new CPHP_GET_CPU_ID_CMD command,
>>> introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> Could we make this usecase / workflow independent of the new
>> CPHP_GET_CPU_ID_CMD command please?
>>
>> I'd like to suggest the following:
>>
>>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
>>> index bb33144..667b264 100644
>>> --- a/docs/specs/acpi_cpu_hotplug.txt
>>> +++ b/docs/specs/acpi_cpu_hotplug.txt
>>> @@ -15,14 +15,14 @@ CPU present bitmap for:
>>> PIIX-PM (IO port 0xaf00-0xaf1f, 1-byte access)
>>> One bit per CPU. Bit position reflects corresponding CPU APIC ID. Read-only.
>>> The first DWORD in bitmap is used in write mode to switch from legacy
>>> - to new CPU hotplug interface, write 0 into it to do switch.
>>> + to modern CPU hotplug interface, write 0 into it to do switch.
>>> ---------------------------------------------------------------
>>> QEMU sets corresponding CPU bit on hot-add event and issues SCI
>>> with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
>>> to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
>>>
>>> =====================================
>>> -ACPI CPU hotplug interface registers:
>>> +Modern ACPI CPU hotplug interface registers:
>>> -------------------------------------
>>> Register block base address:
>>> ICH9-LPC IO port 0x0cd8
>>> @@ -105,6 +105,24 @@ write access:
>>> other values: reserved
>>>
>>> Typical usecases:
>>> + - (x86) Detecting and enabling modern CPU hotplug interface.
>>
>> (1) I think we can drop the (x86) restriction. (Because, we don't need
>> to depend on APIC ID specifics; see below.)
> I'd rather keep it x86 specific, as enabling interface and talking about
> legacy bitmap applies only to x86 impl.
> ARM one won't have any of it, it will just be enabled in a future QEMU
> (probably even without version-ed machine type).
> So could we just say usual "use firmware X.Y with QEMU Z to use CPU hotplug"
> in that case?
> Do we really need it to be arch agnostic?
No, I only suggested removing the x86 reference as a possible "extra"
improvement, from not depending on CPHP_GET_CPU_ID_CMD / APIC ID.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-05 13:03 ` Laszlo Ersek
@ 2019-12-06 15:15 ` Igor Mammedov
2019-12-06 15:46 ` Laszlo Ersek
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2019-12-06 15:15 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, philmd, qemu-devel, mst
On Thu, 5 Dec 2019 14:03:01 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/04/19 18:05, Igor Mammedov wrote:
> > Extend CPU hotplug interface to return architecture specific
> > identifier for current CPU in 2 registers:
> > - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW
> > - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at
> > offset 0.
>
> OK.
>
> > Target user is UEFI firmware, which needs a way to enumerate
> > all CPUs (including possible CPUs) to allocate and initialize
> > CPU structures on boot.
>
> (1) This is correct in general, but if we want to keep this description,
> then it should be moved to the commit message of the previous patch.
> CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it
> will be necessary for handling the hotplug SMI.
>
> For the boot time allocation / initialization, the "enumerating present
> and possible CPUs" workflow is necessary, and that is documented in the
> previous patch in this series.
>
> So if we want to keep this paragraph, we should move it to the previous
> patch's commit message.
>
> > (for x86: it needs APIC ID and later command will be used to
> > retrieve ARM's MPIDR which serves the similar to APIC ID purpose)
>
> (2) I would suggest some punctuation, to make this clearer. How about:
>
> > On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching
> > the APIC ID when handling the hotplug SMI.
> >
> > Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
> > which serves a purpose similar to the x86 APIC ID.
How about following commit message:
Firmware can enumerate present at boot APs by broadcasting wakeup IPI,
so that woken up secondary CPUs could register them-selves.
However in CPU hotplug case, it would need to know architecture
specific CPU IDs for possible and hotplugged CPUs so it could
prepare enviroment for and wake hotplugged AP.
Reuse and extend existing CPU hotplug interface to return architecture
specific ID for currently selected CPU in 2 registers:
- lower 32 bits in ACPI_CPU_CMD_DATA_OFFSET_RW
- upper 32 bits in ACPI_CPU_CMD_DATA2_OFFSET_R
On x86, firmware will use CPHP_GET_CPU_ID_CMD for fetching the APIC ID
when handling hotplug SMI.
Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
which serves the similar to APIC ID purpose.
[...]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-06 15:15 ` Igor Mammedov
@ 2019-12-06 15:46 ` Laszlo Ersek
0 siblings, 0 replies; 24+ messages in thread
From: Laszlo Ersek @ 2019-12-06 15:46 UTC (permalink / raw)
To: Igor Mammedov; +Cc: pbonzini, philmd, qemu-devel, mst
On 12/06/19 16:15, Igor Mammedov wrote:
> On Thu, 5 Dec 2019 14:03:01 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 12/04/19 18:05, Igor Mammedov wrote:
>>> Extend CPU hotplug interface to return architecture specific
>>> identifier for current CPU in 2 registers:
>>> - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW
>>> - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at
>>> offset 0.
>>
>> OK.
>>
>>> Target user is UEFI firmware, which needs a way to enumerate
>>> all CPUs (including possible CPUs) to allocate and initialize
>>> CPU structures on boot.
>>
>> (1) This is correct in general, but if we want to keep this description,
>> then it should be moved to the commit message of the previous patch.
>> CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it
>> will be necessary for handling the hotplug SMI.
>>
>> For the boot time allocation / initialization, the "enumerating present
>> and possible CPUs" workflow is necessary, and that is documented in the
>> previous patch in this series.
>>
>> So if we want to keep this paragraph, we should move it to the previous
>> patch's commit message.
>>
>>> (for x86: it needs APIC ID and later command will be used to
>>> retrieve ARM's MPIDR which serves the similar to APIC ID purpose)
>>
>> (2) I would suggest some punctuation, to make this clearer. How about:
>>
>>> On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching
>>> the APIC ID when handling the hotplug SMI.
>>>
>>> Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
>>> which serves a purpose similar to the x86 APIC ID.
>
> How about following commit message:
>
> Firmware can enumerate present at boot APs by broadcasting wakeup IPI,
> so that woken up secondary CPUs could register them-selves.
> However in CPU hotplug case, it would need to know architecture
> specific CPU IDs for possible and hotplugged CPUs so it could
> prepare enviroment for and wake hotplugged AP.
>
> Reuse and extend existing CPU hotplug interface to return architecture
> specific ID for currently selected CPU in 2 registers:
> - lower 32 bits in ACPI_CPU_CMD_DATA_OFFSET_RW
> - upper 32 bits in ACPI_CPU_CMD_DATA2_OFFSET_R
>
> On x86, firmware will use CPHP_GET_CPU_ID_CMD for fetching the APIC ID
> when handling hotplug SMI.
>
> Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
> which serves the similar to APIC ID purpose.
>
> [...]
>
Looks fine to me, thank you!
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-12-06 17:00 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 17:05 [PATCH for-5.0 0/8] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 1/8] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
2019-12-05 10:43 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 2/8] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
2019-12-04 17:05 ` [PATCH for-5.0 3/8] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
2019-12-05 11:50 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
2019-12-05 12:17 ` Laszlo Ersek
2019-12-06 11:09 ` Igor Mammedov
2019-12-06 12:00 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 5/8] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
2019-12-05 12:21 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 6/8] acpi: cpuhp: spec: add typical usecases Igor Mammedov
2019-12-05 12:29 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
2019-12-05 13:03 ` Laszlo Ersek
2019-12-06 15:15 ` Igor Mammedov
2019-12-06 15:46 ` Laszlo Ersek
2019-12-04 17:05 ` [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug Igor Mammedov
2019-12-05 14:07 ` Laszlo Ersek
2019-12-06 10:40 ` Igor Mammedov
2019-12-06 12:02 ` Laszlo Ersek
2019-12-06 13:49 ` Igor Mammedov
2019-12-06 15:06 ` Laszlo Ersek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.