* [PATCH for-5.0 v2 1/9] hw: add compat machines for 5.0
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 2/9] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
` (9 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
From: Cornelia Huck <cohuck@redhat.com>
Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
For i440fx and q35, unversioned cpu models are still translated
to -v1; I'll leave changing this (if desired) to the respective
maintainers.
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/boards.h | 3 +++
include/hw/i386/pc.h | 3 +++
hw/arm/virt.c | 7 ++++++-
hw/core/machine.c | 3 +++
hw/i386/pc.c | 3 +++
hw/i386/pc_piix.c | 14 +++++++++++++-
hw/i386/pc_q35.c | 13 ++++++++++++-
hw/ppc/spapr.c | 15 +++++++++++++--
hw/s390x/s390-virtio-ccw.c | 14 +++++++++++++-
9 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087..24cbeec 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -329,6 +329,9 @@ struct MachineState {
} \
type_init(machine_initfn##_register_types)
+extern GlobalProperty hw_compat_4_2[];
+extern const size_t hw_compat_4_2_len;
+
extern GlobalProperty hw_compat_4_1[];
extern const size_t hw_compat_4_1_len;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1f86eba..61a998d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -237,6 +237,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
const CPUArchIdList *apic_ids, GArray *entry);
+extern GlobalProperty pc_compat_4_2[];
+extern const size_t pc_compat_4_2_len;
+
extern GlobalProperty pc_compat_4_1[];
extern const size_t pc_compat_4_1_len;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2..02f654b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2147,10 +2147,15 @@ static void machvirt_machine_init(void)
}
type_init(machvirt_machine_init);
+static void virt_machine_5_0_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
+
static void virt_machine_4_2_options(MachineClass *mc)
{
}
-DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
+DEFINE_VIRT_MACHINE(4, 2)
static void virt_machine_4_1_options(MachineClass *mc)
{
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3..21fe2d9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,9 @@
#include "hw/pci/pci.h"
#include "hw/mem/nvdimm.h"
+GlobalProperty hw_compat_4_2[] = {};
+const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
+
GlobalProperty hw_compat_4_1[] = {
{ "virtio-pci", "x-pcie-flr-init", "off" },
};
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ac08e63..58867f9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -103,6 +103,9 @@
struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
+GlobalProperty pc_compat_4_2[] = {};
+const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
+
GlobalProperty pc_compat_4_1[] = {};
const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1bd70d1..846e70b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -424,7 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
}
-static void pc_i440fx_4_2_machine_options(MachineClass *m)
+static void pc_i440fx_5_0_machine_options(MachineClass *m)
{
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_i440fx_machine_options(m);
@@ -433,6 +433,18 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
pcmc->default_cpu_version = 1;
}
+DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
+ pc_i440fx_5_0_machine_options);
+
+static void pc_i440fx_4_2_machine_options(MachineClass *m)
+{
+ pc_i440fx_5_0_machine_options(m);
+ m->alias = NULL;
+ m->is_default = 0;
+ compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+ compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
+}
+
DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
pc_i440fx_4_2_machine_options);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 385e5cf..ddd485d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -348,7 +348,7 @@ static void pc_q35_machine_options(MachineClass *m)
m->max_cpus = 288;
}
-static void pc_q35_4_2_machine_options(MachineClass *m)
+static void pc_q35_5_0_machine_options(MachineClass *m)
{
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_q35_machine_options(m);
@@ -356,6 +356,17 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
pcmc->default_cpu_version = 1;
}
+DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
+ pc_q35_5_0_machine_options);
+
+static void pc_q35_4_2_machine_options(MachineClass *m)
+{
+ pc_q35_5_0_machine_options(m);
+ m->alias = NULL;
+ compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+ compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
+}
+
DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
pc_q35_4_2_machine_options);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f60..3ae7db1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4492,14 +4492,25 @@ static const TypeInfo spapr_machine_info = {
type_init(spapr_machine_register_##suffix)
/*
+ * pseries-5.0
+ */
+static void spapr_machine_5_0_class_options(MachineClass *mc)
+{
+ /* Defaults for the latest behaviour inherited from the base class */
+}
+
+DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
+
+/*
* pseries-4.2
*/
static void spapr_machine_4_2_class_options(MachineClass *mc)
{
- /* Defaults for the latest behaviour inherited from the base class */
+ spapr_machine_5_0_class_options(mc);
+ compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
}
-DEFINE_SPAPR_MACHINE(4_2, "4.2", true);
+DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
/*
* pseries-4.1
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef..01e7e20 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -639,14 +639,26 @@ bool css_migration_enabled(void)
} \
type_init(ccw_machine_register_##suffix)
+static void ccw_machine_5_0_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_5_0_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(5_0, "5.0", true);
+
static void ccw_machine_4_2_instance_options(MachineState *machine)
{
+ ccw_machine_5_0_instance_options(machine);
}
static void ccw_machine_4_2_class_options(MachineClass *mc)
{
+ ccw_machine_5_0_class_options(mc);
+ compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
}
-DEFINE_CCW_MACHINE(4_2, "4.2", true);
+DEFINE_CCW_MACHINE(4_2, "4.2", false);
static void ccw_machine_4_1_instance_options(MachineState *machine)
{
--
2.7.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH for-5.0 v2 2/9] q35: implement 128K SMRAM at default SMBASE address
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 1/9] hw: add compat machines for 5.0 Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 20:11 ` Laszlo Ersek
2019-12-09 13:08 ` [PATCH for-5.0 v2 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
` (8 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
It's not what real HW does, implementing which would be overkill [**]
and would require complex cross stack changes (QEMU+firmware) to make
it work.
So considering that SMRAM is owned by MCH, for simplicity (ab)use
reserved Q35 register, which allows QEMU and firmware easily init
and make RAM at SMBASE available only from SMM context.
Patch uses commit (2f295167e0 q35/mch: implement extended TSEG sizes)
for inspiration and uses 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
**) https://www.mail-archive.com/qemu-devel@nongnu.org/msg646965.html
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
V2:
- add a note in commit message/coed that used approach is QEMU hack
to make impl. simple instead of going after VT-d approach
which real HW does.
(Paolo Bonzini <pbonzini@redhat.com>)
- rebase on top of (hw: add compat machines for 5.0), and move
compat property smbase-smram to pc_compat_4_2[]
("Laszlo Ersek" <lersek@redhat.com>)
---
include/hw/pci-host/q35.h | 10 ++++++
hw/i386/pc.c | 4 ++-
hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 90 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 58867f9..ff4d583 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_2[] = {};
+GlobalProperty pc_compat_4_2[] = {
+ { "mch", "smbase-smram", "off" },
+};
const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
GlobalProperty pc_compat_4_1[] = {};
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270..6342f73 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,27 @@ 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);
+
+ /*
+ * This is not what hardware does, so it's QEMU specific hack.
+ * See commit message for details.
+ */
+ 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 +670,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] 20+ messages in thread* Re: [PATCH for-5.0 v2 2/9] q35: implement 128K SMRAM at default SMBASE address
2019-12-09 13:08 ` [PATCH for-5.0 v2 2/9] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
@ 2019-12-09 20:11 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2019-12-09 20:11 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/09/19 14:08, Igor Mammedov wrote:
> It's not what real HW does, implementing which would be overkill [**]
> and would require complex cross stack changes (QEMU+firmware) to make
> it work.
> So considering that SMRAM is owned by MCH, for simplicity (ab)use
> reserved Q35 register, which allows QEMU and firmware easily init
> and make RAM at SMBASE available only from SMM context.
>
> Patch uses commit (2f295167e0 q35/mch: implement extended TSEG sizes)
> for inspiration and uses 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
> **) https://www.mail-archive.com/qemu-devel@nongnu.org/msg646965.html
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> V2:
> - add a note in commit message/coed that used approach is QEMU hack
> to make impl. simple instead of going after VT-d approach
> which real HW does.
> (Paolo Bonzini <pbonzini@redhat.com>)
> - rebase on top of (hw: add compat machines for 5.0), and move
> compat property smbase-smram to pc_compat_4_2[]
> ("Laszlo Ersek" <lersek@redhat.com>)
> ---
> include/hw/pci-host/q35.h | 10 ++++++
> hw/i386/pc.c | 4 ++-
> hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 90 insertions(+), 8 deletions(-)
Interdiff per git-range-diff, with the last version applied on
1bdc319ab5d2 ("Update version for v4.2.0-rc4 release", 2019-12-03; aka
"v4.2.0-rc4"), and the present version applied on 9b4efa2ede5d ("Merge
remote-tracking branch 'remotes/ericb/tags/pull-nbd-2019-12-09' into
staging", 2019-12-09):
> 1: 5aafb1228e86 ! 2: effa5c4ca588 q35: implement 128K SMRAM at default SMBASE address
> @@ -2,8 +2,15 @@
>
> q35: implement 128K SMRAM at default SMBASE address
>
> - Use commit (2f295167e0 q35/mch: implement extended TSEG sizes) for
> - inspiration and (ab)use reserved register in config space at 0x9c
> + It's not what real HW does, implementing which would be overkill [**]
> + and would require complex cross stack changes (QEMU+firmware) to make
> + it work.
> + So considering that SMRAM is owned by MCH, for simplicity (ab)use
> + reserved Q35 register, which allows QEMU and firmware easily init
> + and make RAM at SMBASE available only from SMM context.
> +
> + Patch uses commit (2f295167e0 q35/mch: implement extended TSEG sizes)
> + for inspiration and uses 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.
>
> @@ -18,9 +25,10 @@
> ignored until the system reset.
>
> *) https://www.mail-archive.com/qemu-devel@nongnu.org/msg455991.html
> + **) https://www.mail-archive.com/qemu-devel@nongnu.org/msg646965.html
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> - Message-Id: <1575479147-6641-2-git-send-email-imammedo@redhat.com>
> + Message-Id: <1575896942-331151-3-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
> @@ -64,13 +72,13 @@
>
> struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>
> --GlobalProperty pc_compat_4_1[] = {};
> -+GlobalProperty pc_compat_4_1[] = {
> +-GlobalProperty pc_compat_4_2[] = {};
> ++GlobalProperty pc_compat_4_2[] = {
> + { "mch", "smbase-smram", "off" },
> +};
> - const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
> + const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
>
> - GlobalProperty pc_compat_4_0[] = {};
> + GlobalProperty pc_compat_4_1[] = {};
>
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- a/hw/pci-host/q35.c
> @@ -192,6 +200,10 @@
> memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
> &mch->tseg_window);
> +
> ++ /*
> ++ * This is not what hardware does, so it's QEMU specific hack.
> ++ * See commit message for details.
> ++ */
> + memory_region_init_io(&mch->smbase_blackhole, OBJECT(mch), &blackhole_ops,
> + NULL, "smbase-blackhole",
> + MCH_HOST_BRIDGE_SMBASE_SIZE);
Thanks for the updates.
Tested as follows. Because the differences are minimal / superficial in
comparison to the last two postings:
(a) http://mid.mail-archive.com/c18f1ada-3eca-d5f1-da4f-e74181c5a862@redhat.com
(b) http://mid.mail-archive.com/e31fc667-0288-eaa5-f3a6-0b0acab59ea2@redhat.com
I only repeated a subset of the previous tests (a). Namely:
- patched OVMF
<http://mid.mail-archive.com/20190924113505.27272-1-lersek@redhat.com>,
feature disabled via "pc-q35-4.2" machine type, Fedora guest, normal
boot and S3,
- patched OVMF, feature enabled via "pc-q35-5.0" machine type, Fedora
guest, normal boot and S3.
Compared the following artifacts between the above two machine types:
- OVMF boot log and S3 resume log,
- "info mtree" ("smbase-blackhole" & "smbase-window" disabled vs.
enabled), after normal boot, and after S3 resume.
For this patch:
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
On 12/09/19 14:08, 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 58867f9..ff4d583 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_2[] = {};
> +GlobalProperty pc_compat_4_2[] = {
> + { "mch", "smbase-smram", "off" },
> +};
> const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
>
> GlobalProperty pc_compat_4_1[] = {};
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 158d270..6342f73 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,27 @@ 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);
> +
> + /*
> + * This is not what hardware does, so it's QEMU specific hack.
> + * See commit message for details.
> + */
> + 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 +670,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] 20+ messages in thread
* [PATCH for-5.0 v2 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 1/9] hw: add compat machines for 5.0 Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 2/9] q35: implement 128K SMRAM at default SMBASE address Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 13:46 ` [PATCH for-5.0 v3 " Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 4/9] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
` (7 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 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] 20+ messages in thread* [PATCH for-5.0 v3 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test
2019-12-09 13:08 ` [PATCH for-5.0 v2 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
@ 2019-12-09 13:46 ` Igor Mammedov
2019-12-09 20:16 ` Laszlo Ersek
0 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:46 UTC (permalink / raw)
To: qemu-devel; +Cc: dinechin, 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>
---
v3:
- a bunch of spelling fixes
(Christophe de Dinechin <dinechin@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..c922d81 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 writing junk to 0x9c before before negotiating is ignored */
+ 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 into 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 is 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] 20+ messages in thread* Re: [PATCH for-5.0 v3 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test
2019-12-09 13:46 ` [PATCH for-5.0 v3 " Igor Mammedov
@ 2019-12-09 20:16 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2019-12-09 20:16 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: dinechin, pbonzini, philmd, mst
On 12/09/19 14:46, Igor Mammedov wrote:
> 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>
> ---
> v3:
> - a bunch of spelling fixes
> (Christophe de Dinechin <dinechin@redhat.com>)
Since I've run git-range-diff for this series anyway, I guess I can help
other reviewers with the changes (relative to the previous, fully
separate posting):
> 2: f1a896f4dbc6 ! 3: 092ec4a30289 tests: q35: MCH: add default SMBASE SMRAM lock test
> @@ -6,7 +6,7 @@
> patch "q35: implement 128K SMRAM at default SMBASE address"
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> - Message-Id: <1575479147-6641-3-git-send-email-imammedo@redhat.com>
> + Message-Id: <1575899217-333105-1-git-send-email-imammedo@redhat.com>
>
> diff --git a/tests/q35-test.c b/tests/q35-test.c
> --- a/tests/q35-test.c
> @@ -40,7 +40,7 @@
> + 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 */
> ++ /* check that writing junk to 0x9c before before negotiating is ignored */
> + 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);
> @@ -60,7 +60,7 @@
> + 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 */
> ++ /* RAM access should go into black hole */
> + qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
> + g_assert_cmpint(qtest_readb(qts, SMBASE), ==, 0xff);
> + }
> @@ -98,7 +98,7 @@
> + pcidev = qpci_device_find(pcibus, 0);
> + g_assert(pcidev != NULL);
> +
> -+ /* check that RAM accessible */
> ++ /* check that RAM is accessible */
> + qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
> + g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
> +
Thanks,
Laszlo
On 12/09/19 14:46, Igor Mammedov wrote:
> ---
> tests/q35-test.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 insertions(+)
>
> diff --git a/tests/q35-test.c b/tests/q35-test.c
> index a68183d..c922d81 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 writing junk to 0x9c before before negotiating is ignored */
> + 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 into 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 is 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();
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH for-5.0 v2 4/9] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (2 preceding siblings ...)
2019-12-09 13:08 ` [PATCH for-5.0 v2 3/9] tests: q35: MCH: add default SMBASE SMRAM lock test Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 5/9] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
` (6 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 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>
Reviewed-by: Laszlo Ersek <lersek@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] 20+ messages in thread* [PATCH for-5.0 v2 5/9] acpi: cpuhp: spec: fix 'Command data' description
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (3 preceding siblings ...)
2019-12-09 13:08 ` [PATCH for-5.0 v2 4/9] acpi: cpuhp: spec: clarify 'CPU selector' register usage and endianness Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 13:08 ` [PATCH for-5.0 v2 6/9] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
` (5 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Correct returned value description in case 'Command field' == 0x0,
it's 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
v2:
* rephrase a docs little bit (Laszlo)
---
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..d510872 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 value last stored in 'Command field' 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 modified.
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] 20+ messages in thread* [PATCH for-5.0 v2 6/9] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (4 preceding siblings ...)
2019-12-09 13:08 ` [PATCH for-5.0 v2 5/9] acpi: cpuhp: spec: fix 'Command data' description Igor Mammedov
@ 2019-12-09 13:08 ` Igor Mammedov
2019-12-09 13:09 ` [PATCH for-5.0 v2 7/9] acpi: cpuhp: introduce 'Command data 2' field Igor Mammedov
` (4 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:08 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 is equal to 0, to reflect that currently it's not
supported.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@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 d510872..8fb9ad2 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] 20+ messages in thread* [PATCH for-5.0 v2 7/9] acpi: cpuhp: introduce 'Command data 2' field
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (5 preceding siblings ...)
2019-12-09 13:08 ` [PATCH for-5.0 v2 6/9] acpi: cpuhp: spec: clarify store into 'Command data' when 'Command field' == 0 Igor Mammedov
@ 2019-12-09 13:09 ` Igor Mammedov
2019-12-09 20:27 ` Laszlo Ersek
2019-12-09 13:09 ` [PATCH for-5.0 v2 8/9] acpi: cpuhp: spec: add typical usecases Igor Mammedov
` (3 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:09 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
No functional change in practice, patch only aims to properly
document (in spec and code) intended usage of the reserved space.
The new field is to be used for 2 purposes:
- detection of modern CPU hotplug interface using
CPHP_GET_NEXT_CPU_WITH_EVENT_CMD command.
procedure will be described in follow up patch:
"acpi: cpuhp: spec: add typical usecases"
- for returning upper 32 bits of architecture specific CPU ID,
for new CPHP_GET_CPU_ID_CMD command added by follow up patch:
"acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
Change is backward compatible with 4.2 and older machines, as field was
unconditionally reserved and always returned 0x0 if modern CPU hotplug
interface was enabled.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_cpu_hotplug.txt | 5 ++++-
hw/acpi/cpu.c | 11 +++++++++++
hw/acpi/trace-events | 1 +
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 8fb9ad2..9879f9e 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -44,7 +44,10 @@ keeps the current value.
read access:
offset:
- [0x0-0x3] reserved
+ [0x0-0x3] Command data 2: (DWORD access)
+ if value last stored in 'Command field':
+ 0: reads as 0x0
+ other values: reserved
[0x4] CPU device status fields: (1 byte access)
bits:
0: Device is enabled and may be used by guest
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 87f30a3..d475c06 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -12,6 +12,7 @@
#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,
@@ -79,6 +80,16 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
}
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_NEXT_CPU_WITH_EVENT_CMD:
+ val = 0;
+ 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] 20+ messages in thread* Re: [PATCH for-5.0 v2 7/9] acpi: cpuhp: introduce 'Command data 2' field
2019-12-09 13:09 ` [PATCH for-5.0 v2 7/9] acpi: cpuhp: introduce 'Command data 2' field Igor Mammedov
@ 2019-12-09 20:27 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2019-12-09 20:27 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/09/19 14:09, Igor Mammedov wrote:
> No functional change in practice, patch only aims to properly
> document (in spec and code) intended usage of the reserved space.
>
> The new field is to be used for 2 purposes:
> - detection of modern CPU hotplug interface using
> CPHP_GET_NEXT_CPU_WITH_EVENT_CMD command.
> procedure will be described in follow up patch:
> "acpi: cpuhp: spec: add typical usecases"
> - for returning upper 32 bits of architecture specific CPU ID,
> for new CPHP_GET_CPU_ID_CMD command added by follow up patch:
> "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
>
> Change is backward compatible with 4.2 and older machines, as field was
> unconditionally reserved and always returned 0x0 if modern CPU hotplug
> interface was enabled.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_cpu_hotplug.txt | 5 ++++-
> hw/acpi/cpu.c | 11 +++++++++++
> hw/acpi/trace-events | 1 +
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 8fb9ad2..9879f9e 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -44,7 +44,10 @@ keeps the current value.
>
> read access:
> offset:
> - [0x0-0x3] reserved
> + [0x0-0x3] Command data 2: (DWORD access)
> + if value last stored in 'Command field':
> + 0: reads as 0x0
> + other values: reserved
> [0x4] CPU device status fields: (1 byte access)
> bits:
> 0: Device is enabled and may be used by guest
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 87f30a3..d475c06 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -12,6 +12,7 @@
> #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,
> @@ -79,6 +80,16 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
> }
> 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_NEXT_CPU_WITH_EVENT_CMD:
> + val = 0;
> + 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"]"
>
Neat. :)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH for-5.0 v2 8/9] acpi: cpuhp: spec: add typical usecases
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (6 preceding siblings ...)
2019-12-09 13:09 ` [PATCH for-5.0 v2 7/9] acpi: cpuhp: introduce 'Command data 2' field Igor Mammedov
@ 2019-12-09 13:09 ` Igor Mammedov
2019-12-09 20:36 ` Laszlo Ersek
2019-12-09 13:09 ` [PATCH for-5.0 v2 9/9] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
` (2 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:09 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
Document work-flows for
* enabling/detecting modern CPU hotplug interface
* finding a CPU with pending 'insert/remove' event
* enumerating present and possible CPUs
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- fix indent of "other values" that's just above
being added "Typical usecases:" section
- unindent "Typical usecases" to put it into right scope
(Laszlo)
- squash in ammended (using CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)
"acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug"
(Laszlo)
---
docs/specs/acpi_cpu_hotplug.txt | 51 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index 9879f9e..cb99cf3 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
@@ -67,6 +67,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.
@@ -98,4 +99,48 @@ 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
+ 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 0x0 to the 'Command field' register,
+ 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.
+ 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] 20+ messages in thread* Re: [PATCH for-5.0 v2 8/9] acpi: cpuhp: spec: add typical usecases
2019-12-09 13:09 ` [PATCH for-5.0 v2 8/9] acpi: cpuhp: spec: add typical usecases Igor Mammedov
@ 2019-12-09 20:36 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2019-12-09 20:36 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/09/19 14:09, Igor Mammedov wrote:
> Document work-flows for
> * enabling/detecting modern CPU hotplug interface
> * finding a CPU with pending 'insert/remove' event
> * enumerating present and possible CPUs
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
> - fix indent of "other values" that's just above
> being added "Typical usecases:" section
> - unindent "Typical usecases" to put it into right scope
> (Laszlo)
> - squash in ammended (using CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)
> "acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug"
> (Laszlo)
> ---
> docs/specs/acpi_cpu_hotplug.txt | 51 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 9879f9e..cb99cf3 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:
> -------------------------------------
(1) If you need to post a v3 for other reasons, please consider
adjusting the "===" and "---" lines that surround the new heading.
Otherwise, don't bother.
> Register block base address:
> ICH9-LPC IO port 0x0cd8
> @@ -67,6 +67,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.
> @@ -98,4 +99,48 @@ 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
> + 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 0x0 to the 'Command field' register,
> + 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.
> + 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".
>
Awesome!
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH for-5.0 v2 9/9] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (7 preceding siblings ...)
2019-12-09 13:09 ` [PATCH for-5.0 v2 8/9] acpi: cpuhp: spec: add typical usecases Igor Mammedov
@ 2019-12-09 13:09 ` Igor Mammedov
2019-12-09 20:46 ` Laszlo Ersek
2019-12-19 13:30 ` [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
2020-01-06 10:10 ` Igor Mammedov
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-09 13:09 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, philmd, lersek, mst
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 environment 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.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
- s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/.
v2:
- ACPI_CPU_CMD_DATA2_OFFSET_R moved into separate patch
that adds 'Command data 2' field separately
- ammend commit message
---
docs/specs/acpi_cpu_hotplug.txt | 3 +++
hw/acpi/cpu.c | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
index cb99cf3..a8ce5e7 100644
--- a/docs/specs/acpi_cpu_hotplug.txt
+++ b/docs/specs/acpi_cpu_hotplug.txt
@@ -47,6 +47,7 @@ read access:
[0x0-0x3] Command data 2: (DWORD access)
if value last stored in 'Command field':
0: reads as 0x0
+ 3: upper 32 bits of architecture specific CPU ID value
other values: reserved
[0x4] CPU device status fields: (1 byte access)
bits:
@@ -61,6 +62,8 @@ read access:
[0x8] Command data: (DWORD access)
contains 0 unless value last stored in 'Command field' is one of:
0: contains 'CPU selector' value of a CPU with pending event[s]
+ 3: lower 32 bits of architecture specific CPU ID value
+ (in x86 case: APIC ID)
write access:
offset:
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index d475c06..e2c957c 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -18,6 +18,7 @@ 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
};
@@ -75,6 +76,9 @@ 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;
}
@@ -85,6 +89,9 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
val = 0;
break;
+ case CPHP_GET_CPU_ID_CMD:
+ val = cdev->arch_id >> 32;
+ break;
default:
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH for-5.0 v2 9/9] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
2019-12-09 13:09 ` [PATCH for-5.0 v2 9/9] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
@ 2019-12-09 20:46 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2019-12-09 20:46 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: pbonzini, philmd, mst
On 12/09/19 14:09, Igor Mammedov wrote:
> 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 environment 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.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v1:
> - s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/.
> v2:
> - ACPI_CPU_CMD_DATA2_OFFSET_R moved into separate patch
> that adds 'Command data 2' field separately
> - ammend commit message
> ---
> docs/specs/acpi_cpu_hotplug.txt | 3 +++
> hw/acpi/cpu.c | 7 +++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index cb99cf3..a8ce5e7 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -47,6 +47,7 @@ read access:
> [0x0-0x3] Command data 2: (DWORD access)
> if value last stored in 'Command field':
> 0: reads as 0x0
> + 3: upper 32 bits of architecture specific CPU ID value
> other values: reserved
> [0x4] CPU device status fields: (1 byte access)
> bits:
> @@ -61,6 +62,8 @@ read access:
> [0x8] Command data: (DWORD access)
> contains 0 unless value last stored in 'Command field' is one of:
> 0: contains 'CPU selector' value of a CPU with pending event[s]
> + 3: lower 32 bits of architecture specific CPU ID value
> + (in x86 case: APIC ID)
>
> write access:
> offset:
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index d475c06..e2c957c 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -18,6 +18,7 @@ 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
> };
>
> @@ -75,6 +76,9 @@ 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;
> }
> @@ -85,6 +89,9 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
> case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
> val = 0;
> break;
> + case CPHP_GET_CPU_ID_CMD:
> + val = cdev->arch_id >> 32;
> + break;
> default:
> break;
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (8 preceding siblings ...)
2019-12-09 13:09 ` [PATCH for-5.0 v2 9/9] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command Igor Mammedov
@ 2019-12-19 13:30 ` Igor Mammedov
2019-12-19 14:07 ` Igor Mammedov
2020-01-06 10:10 ` Igor Mammedov
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2019-12-19 13:30 UTC (permalink / raw)
To: qemu-devel, mst; +Cc: pbonzini, philmd, lersek
On Mon, 9 Dec 2019 14:08:53 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> ChangeLog:
> * since v1:
> - include "hw: add compat machines for 5.0" to provide
> compat context for 4.2 machine types
> - add comment that SMRAM at SMBASE is QEMU hack
> and why it was used
> - split command data 2 into a separate patch
> "acpi: cpuhp: introduce 'Command data 2' field"
> - rewrite enabling/detecting modern CPU hotplug interface
> to use existing CPHP_GET_NEXT_CPU_WITH_EVENT_CMD and
> squash it into "acpi: cpuhp: spec: add typical usecases" patch
> - "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
> modulo 'Command data 2' being moved out into separate patch,
> rewrite commit message to explain better why new command is needed.
>
>
> Series consists of 2 parts: 1st is lockable SMRAM at SMBASE
> and the 2nd better documents interface and adds means to
> enumerate APIC IDs for possible CPUs.
>
> 1st part [1-2/9]:
> 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.
Michael,
could you pick it up please?
>
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: lersek@redhat.com
> CC: philmd@redhat.com
>
>
> Cornelia Huck (1):
> hw: add compat machines for 5.0
>
> 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: introduce 'Command data 2' field
> acpi: cpuhp: spec: add typical usecases
> acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
>
> include/hw/boards.h | 3 ++
> include/hw/i386/pc.h | 3 ++
> include/hw/pci-host/q35.h | 10 ++++
> docs/specs/acpi_cpu_hotplug.txt | 89 +++++++++++++++++++++++++++-------
> hw/acpi/cpu.c | 18 +++++++
> hw/acpi/trace-events | 1 +
> hw/arm/virt.c | 7 ++-
> hw/core/machine.c | 3 ++
> hw/i386/pc.c | 5 ++
> hw/i386/pc_piix.c | 14 +++++-
> hw/i386/pc_q35.c | 13 ++++-
> hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++---
> hw/ppc/spapr.c | 15 +++++-
> hw/s390x/s390-virtio-ccw.c | 14 +++++-
> tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++++
> 15 files changed, 354 insertions(+), 30 deletions(-)
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2
2019-12-19 13:30 ` [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
@ 2019-12-19 14:07 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2019-12-19 14:07 UTC (permalink / raw)
To: qemu-devel, mst; +Cc: pbonzini, philmd, lersek
On Thu, 19 Dec 2019 14:30:51 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 9 Dec 2019 14:08:53 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> > ChangeLog:
> > * since v1:
> > - include "hw: add compat machines for 5.0" to provide
> > compat context for 4.2 machine types
> > - add comment that SMRAM at SMBASE is QEMU hack
> > and why it was used
> > - split command data 2 into a separate patch
> > "acpi: cpuhp: introduce 'Command data 2' field"
> > - rewrite enabling/detecting modern CPU hotplug interface
> > to use existing CPHP_GET_NEXT_CPU_WITH_EVENT_CMD and
> > squash it into "acpi: cpuhp: spec: add typical usecases" patch
> > - "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
> > modulo 'Command data 2' being moved out into separate patch,
> > rewrite commit message to explain better why new command is needed.
> >
> >
> > Series consists of 2 parts: 1st is lockable SMRAM at SMBASE
> > and the 2nd better documents interface and adds means to
> > enumerate APIC IDs for possible CPUs.
> >
> > 1st part [1-2/9]:
> > 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.
>
> Michael,
>
> could you pick it up please?
modulo 1/9 which has been merged via s390 tree,
the rest still applies fine current master
>
> >
> > CC: mst@redhat.com
> > CC: pbonzini@redhat.com
> > CC: lersek@redhat.com
> > CC: philmd@redhat.com
> >
> >
> > Cornelia Huck (1):
> > hw: add compat machines for 5.0
> >
> > 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: introduce 'Command data 2' field
> > acpi: cpuhp: spec: add typical usecases
> > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
> >
> > include/hw/boards.h | 3 ++
> > include/hw/i386/pc.h | 3 ++
> > include/hw/pci-host/q35.h | 10 ++++
> > docs/specs/acpi_cpu_hotplug.txt | 89 +++++++++++++++++++++++++++-------
> > hw/acpi/cpu.c | 18 +++++++
> > hw/acpi/trace-events | 1 +
> > hw/arm/virt.c | 7 ++-
> > hw/core/machine.c | 3 ++
> > hw/i386/pc.c | 5 ++
> > hw/i386/pc_piix.c | 14 +++++-
> > hw/i386/pc_q35.c | 13 ++++-
> > hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++---
> > hw/ppc/spapr.c | 15 +++++-
> > hw/s390x/s390-virtio-ccw.c | 14 +++++-
> > tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++++
> > 15 files changed, 354 insertions(+), 30 deletions(-)
> >
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2
2019-12-09 13:08 [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
` (9 preceding siblings ...)
2019-12-19 13:30 ` [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2 Igor Mammedov
@ 2020-01-06 10:10 ` Igor Mammedov
2020-01-06 10:22 ` Michael S. Tsirkin
10 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2020-01-06 10:10 UTC (permalink / raw)
To: qemu-devel, mst; +Cc: lersek
On Mon, 9 Dec 2019 14:08:53 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> ChangeLog:
> * since v1:
> - include "hw: add compat machines for 5.0" to provide
> compat context for 4.2 machine types
> - add comment that SMRAM at SMBASE is QEMU hack
> and why it was used
> - split command data 2 into a separate patch
> "acpi: cpuhp: introduce 'Command data 2' field"
> - rewrite enabling/detecting modern CPU hotplug interface
> to use existing CPHP_GET_NEXT_CPU_WITH_EVENT_CMD and
> squash it into "acpi: cpuhp: spec: add typical usecases" patch
> - "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
> modulo 'Command data 2' being moved out into separate patch,
> rewrite commit message to explain better why new command is needed.
>
>
> Series consists of 2 parts: 1st is lockable SMRAM at SMBASE
> and the 2nd better documents interface and adds means to
> enumerate APIC IDs for possible CPUs.
>
> 1st part [1-2/9]:
> 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.
ping,
Michael,
could you merge series via your tree?
(PS: series still applies fine to today's master)
>
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> CC: lersek@redhat.com
> CC: philmd@redhat.com
>
>
> Cornelia Huck (1):
> hw: add compat machines for 5.0
>
> 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: introduce 'Command data 2' field
> acpi: cpuhp: spec: add typical usecases
> acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
>
> include/hw/boards.h | 3 ++
> include/hw/i386/pc.h | 3 ++
> include/hw/pci-host/q35.h | 10 ++++
> docs/specs/acpi_cpu_hotplug.txt | 89 +++++++++++++++++++++++++++-------
> hw/acpi/cpu.c | 18 +++++++
> hw/acpi/trace-events | 1 +
> hw/arm/virt.c | 7 ++-
> hw/core/machine.c | 3 ++
> hw/i386/pc.c | 5 ++
> hw/i386/pc_piix.c | 14 +++++-
> hw/i386/pc_q35.c | 13 ++++-
> hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++---
> hw/ppc/spapr.c | 15 +++++-
> hw/s390x/s390-virtio-ccw.c | 14 +++++-
> tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++++
> 15 files changed, 354 insertions(+), 30 deletions(-)
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH for-5.0 v2 0/9] q35: CPU hotplug with secure boot, part 1+2
2020-01-06 10:10 ` Igor Mammedov
@ 2020-01-06 10:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2020-01-06 10:22 UTC (permalink / raw)
To: Igor Mammedov; +Cc: lersek, qemu-devel
On Mon, Jan 06, 2020 at 11:10:20AM +0100, Igor Mammedov wrote:
> On Mon, 9 Dec 2019 14:08:53 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> > ChangeLog:
> > * since v1:
> > - include "hw: add compat machines for 5.0" to provide
> > compat context for 4.2 machine types
> > - add comment that SMRAM at SMBASE is QEMU hack
> > and why it was used
> > - split command data 2 into a separate patch
> > "acpi: cpuhp: introduce 'Command data 2' field"
> > - rewrite enabling/detecting modern CPU hotplug interface
> > to use existing CPHP_GET_NEXT_CPU_WITH_EVENT_CMD and
> > squash it into "acpi: cpuhp: spec: add typical usecases" patch
> > - "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command"
> > modulo 'Command data 2' being moved out into separate patch,
> > rewrite commit message to explain better why new command is needed.
> >
> >
> > Series consists of 2 parts: 1st is lockable SMRAM at SMBASE
> > and the 2nd better documents interface and adds means to
> > enumerate APIC IDs for possible CPUs.
> >
> > 1st part [1-2/9]:
> > 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.
>
> ping,
>
> Michael,
> could you merge series via your tree?
>
> (PS: series still applies fine to today's master)
I'm still waiting for Peter to apply my previous pull.
Will queue after that, thanks!
> >
> > CC: mst@redhat.com
> > CC: pbonzini@redhat.com
> > CC: lersek@redhat.com
> > CC: philmd@redhat.com
> >
> >
> > Cornelia Huck (1):
> > hw: add compat machines for 5.0
> >
> > 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: introduce 'Command data 2' field
> > acpi: cpuhp: spec: add typical usecases
> > acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command
> >
> > include/hw/boards.h | 3 ++
> > include/hw/i386/pc.h | 3 ++
> > include/hw/pci-host/q35.h | 10 ++++
> > docs/specs/acpi_cpu_hotplug.txt | 89 +++++++++++++++++++++++++++-------
> > hw/acpi/cpu.c | 18 +++++++
> > hw/acpi/trace-events | 1 +
> > hw/arm/virt.c | 7 ++-
> > hw/core/machine.c | 3 ++
> > hw/i386/pc.c | 5 ++
> > hw/i386/pc_piix.c | 14 +++++-
> > hw/i386/pc_q35.c | 13 ++++-
> > hw/pci-host/q35.c | 84 +++++++++++++++++++++++++++++---
> > hw/ppc/spapr.c | 15 +++++-
> > hw/s390x/s390-virtio-ccw.c | 14 +++++-
> > tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++++
> > 15 files changed, 354 insertions(+), 30 deletions(-)
> >
^ permalink raw reply [flat|nested] 20+ messages in thread