hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)
A difference between sbsa platform and the virt platform is PSCI is
handled by ARM-TF in the sbsa platform. This means that the PSCI code
there needs to communicate some of the platform power changes down
to the qemu code for things like shutdown/reset control.
Space has been left to extend the EC if we find other use cases in
future where ARM-TF and qemu need to communicate.
Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
---
hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f030a416fd..c8743fc1d0 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -41,6 +41,7 @@
#include "hw/usb.h"
#include "hw/char/pl011.h"
#include "net/net.h"
+#include "migration/vmstate.h"
#define RAMLIMIT_GB 8192
#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
@@ -62,6 +63,7 @@ enum {
SBSA_CPUPERIPHS,
SBSA_GIC_DIST,
SBSA_GIC_REDIST,
+ SBSA_SECURE_EC,
SBSA_SMMU,
SBSA_UART,
SBSA_RTC,
@@ -98,6 +100,14 @@ typedef struct {
#define SBSA_MACHINE(obj) \
OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
+typedef struct {
+ SysBusDevice parent_obj;
+ MemoryRegion iomem;
+} SECUREECState;
+
+#define TYPE_SECURE_EC "sbsa-secure-ec"
+#define SECURE_EC(obj) OBJECT_CHECK(SECUREECState, (obj), TYPE_SECURE_EC)
+
static const MemMapEntry sbsa_ref_memmap[] = {
/* 512M boot ROM */
[SBSA_FLASH] = { 0, 0x20000000 },
@@ -107,6 +117,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
[SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
[SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
[SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
+ [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
[SBSA_UART] = { 0x60000000, 0x00001000 },
[SBSA_RTC] = { 0x60010000, 0x00001000 },
[SBSA_GPIO] = { 0x60020000, 0x00001000 },
@@ -585,6 +596,65 @@ static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
return board->fdt;
}
+enum sbsa_secure_ec_powerstates {
+ SBSA_SECURE_EC_CMD_NULL,
+ SBSA_SECURE_EC_CMD_POWEROFF,
+ SBSA_SECURE_EC_CMD_REBOOT,
+};
+
+static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
+{
+ /* No use for this currently */
+ return 0;
+}
+
+static void secure_ec_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+ if (offset == 0) { /* PSCI machine power command register */
+ switch (value) {
+ case SBSA_SECURE_EC_CMD_NULL:
+ break;
+ case SBSA_SECURE_EC_CMD_POWEROFF:
+ qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+ break;
+ case SBSA_SECURE_EC_CMD_REBOOT:
+ qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+ break;
+ default:
+ error_report("sbsa-ref: ERROR Unkown power command");
+ }
+ } else {
+ error_report("sbsa-ref: unknown EC register");
+ }
+}
+
+static const MemoryRegionOps secure_ec_ops = {
+ .read = secure_ec_read,
+ .write = secure_ec_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void secure_ec_init(Object *obj)
+{
+ SECUREECState *s = SECURE_EC(obj);
+ SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+
+ memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
+ 0x1000);
+ sysbus_init_mmio(dev, &s->iomem);
+}
+
+static void create_secure_ec(MemoryRegion *mem)
+{
+ hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
+ DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
+ SysBusDevice *s = SYS_BUS_DEVICE(dev);
+
+ memory_region_add_subregion(mem, base,
+ sysbus_mmio_get_region(s, 0));
+}
+
static void sbsa_ref_init(MachineState *machine)
{
unsigned int smp_cpus = machine->smp.cpus;
@@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
create_pcie(sms);
+ create_secure_ec(secure_sysmem);
+
sms->bootinfo.ram_size = machine->ram_size;
sms->bootinfo.nb_cpus = smp_cpus;
sms->bootinfo.board_id = -1;
@@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
.instance_size = sizeof(SBSAMachineState),
};
+static const VMStateDescription vmstate_secure_ec_info = {
+ .name = "sbsa-secure-ec",
+ .version_id = 0,
+ .minimum_version_id = 0,
+};
+
+static void secure_ec_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &vmstate_secure_ec_info;
+ dc->user_creatable = false;
+}
+
+static const TypeInfo secure_ec_info = {
+ .name = TYPE_SECURE_EC,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(SECUREECState),
+ .instance_init = secure_ec_init,
+ .class_init = secure_ec_class_init,
+};
+
static void sbsa_ref_machine_init(void)
{
+ type_register_static(&secure_ec_info);
type_register_static(&sbsa_ref_info);
}
--
2.25.1
On Thu, Aug 20, 2020 at 02:32:01PM +0100, Graeme Gregory wrote:
> A difference between sbsa platform and the virt platform is PSCI is
> handled by ARM-TF in the sbsa platform. This means that the PSCI code
> there needs to communicate some of the platform power changes down
> to the qemu code for things like shutdown/reset control.
>
> Space has been left to extend the EC if we find other use cases in
> future where ARM-TF and qemu need to communicate.
>
> Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> ---
> hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f030a416fd..c8743fc1d0 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -41,6 +41,7 @@
> #include "hw/usb.h"
> #include "hw/char/pl011.h"
> #include "net/net.h"
> +#include "migration/vmstate.h"
>
> #define RAMLIMIT_GB 8192
> #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> @@ -62,6 +63,7 @@ enum {
> SBSA_CPUPERIPHS,
> SBSA_GIC_DIST,
> SBSA_GIC_REDIST,
> + SBSA_SECURE_EC,
> SBSA_SMMU,
> SBSA_UART,
> SBSA_RTC,
> @@ -98,6 +100,14 @@ typedef struct {
> #define SBSA_MACHINE(obj) \
> OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
>
> +typedef struct {
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> +} SECUREECState;
> +
> +#define TYPE_SECURE_EC "sbsa-secure-ec"
> +#define SECURE_EC(obj) OBJECT_CHECK(SECUREECState, (obj), TYPE_SECURE_EC)
> +
> static const MemMapEntry sbsa_ref_memmap[] = {
> /* 512M boot ROM */
> [SBSA_FLASH] = { 0, 0x20000000 },
> @@ -107,6 +117,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
> [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
> [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
> [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
> + [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
> [SBSA_UART] = { 0x60000000, 0x00001000 },
> [SBSA_RTC] = { 0x60010000, 0x00001000 },
> [SBSA_GPIO] = { 0x60020000, 0x00001000 },
> @@ -585,6 +596,65 @@ static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> return board->fdt;
> }
>
> +enum sbsa_secure_ec_powerstates {
> + SBSA_SECURE_EC_CMD_NULL,
> + SBSA_SECURE_EC_CMD_POWEROFF,
> + SBSA_SECURE_EC_CMD_REBOOT,
> +};
> +
> +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + /* No use for this currently */
> + return 0;
> +}
> +
> +static void secure_ec_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + if (offset == 0) { /* PSCI machine power command register */
> + switch (value) {
> + case SBSA_SECURE_EC_CMD_NULL:
> + break;
> + case SBSA_SECURE_EC_CMD_POWEROFF:
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> + break;
> + case SBSA_SECURE_EC_CMD_REBOOT:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + default:
> + error_report("sbsa-ref: ERROR Unkown power command");
> + }
> + } else {
> + error_report("sbsa-ref: unknown EC register");
> + }
> +}
> +
> +static const MemoryRegionOps secure_ec_ops = {
> + .read = secure_ec_read,
> + .write = secure_ec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void secure_ec_init(Object *obj)
> +{
> + SECUREECState *s = SECURE_EC(obj);
> + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> + 0x1000);
> + sysbus_init_mmio(dev, &s->iomem);
> +}
> +
> +static void create_secure_ec(MemoryRegion *mem)
> +{
> + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
I've just discovered the API change here, I tested in an older tree, but
Ill wait for other reviews before re-issuing.
Graeme
> + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +
> + memory_region_add_subregion(mem, base,
> + sysbus_mmio_get_region(s, 0));
> +}
> +
> static void sbsa_ref_init(MachineState *machine)
> {
> unsigned int smp_cpus = machine->smp.cpus;
> @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
>
> create_pcie(sms);
>
> + create_secure_ec(secure_sysmem);
> +
> sms->bootinfo.ram_size = machine->ram_size;
> sms->bootinfo.nb_cpus = smp_cpus;
> sms->bootinfo.board_id = -1;
> @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> .instance_size = sizeof(SBSAMachineState),
> };
>
> +static const VMStateDescription vmstate_secure_ec_info = {
> + .name = "sbsa-secure-ec",
> + .version_id = 0,
> + .minimum_version_id = 0,
> +};
> +
> +static void secure_ec_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &vmstate_secure_ec_info;
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo secure_ec_info = {
> + .name = TYPE_SECURE_EC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(SECUREECState),
> + .instance_init = secure_ec_init,
> + .class_init = secure_ec_class_init,
> +};
> +
> static void sbsa_ref_machine_init(void)
> {
> + type_register_static(&secure_ec_info);
> type_register_static(&sbsa_ref_info);
> }
>
> --
> 2.25.1
>
Patchew URL: https://patchew.org/QEMU/20200820133201.80577-1-graeme@nuviainc.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/target/arm/translate-sve.o
CC aarch64-softmmu/trace/generated-helpers.o
/tmp/qemu-test/src/hw/arm/sbsa-ref.c: In function 'create_secure_ec':
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:24: error: implicit declaration of function 'qdev_create'; did you mean 'bdrv_create'? [-Werror=implicit-function-declaration]
651 | DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
| ^~~~~~~~~~~
| bdrv_create
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:24: error: nested extern declaration of 'qdev_create' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:24: error: initialization of 'DeviceState *' {aka 'struct DeviceState *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/arm/sbsa-ref.o] Error 1
make: *** [Makefile:527: aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
GEN x86_64-softmmu/qemu-system-x86_64.exe
Traceback (most recent call last):
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=b006c9f93b0348aaa2e50e953c64f576', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-wh_4hi2k/src/docker-src.2020-08-20-11.58.09.30151:/var/tmp/qemu:z,ro', 'qemu/fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=b006c9f93b0348aaa2e50e953c64f576
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-wh_4hi2k/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 4m15.953s
user 0m8.310s
The full log is available at
http://patchew.org/logs/20200820133201.80577-1-graeme@nuviainc.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200820133201.80577-1-graeme@nuviainc.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC x86_64-softmmu/target/i386/misc_helper.o
CC aarch64-softmmu/hw/arm/mps2-tz.o
/tmp/qemu-test/src/hw/arm/sbsa-ref.c: In function 'create_secure_ec':
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:5: error: implicit declaration of function 'qdev_create' [-Werror=implicit-function-declaration]
DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
^
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:5: error: nested extern declaration of 'qdev_create' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/arm/sbsa-ref.c:651:24: error: initialization makes pointer from integer without a cast [-Werror]
DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
^
cc1: all warnings being treated as errors
make[1]: *** [hw/arm/sbsa-ref.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC x86_64-softmmu/target/i386/mpx_helper.o
CC x86_64-softmmu/target/i386/seg_helper.o
---
CC x86_64-softmmu/target/i386/sev.o
GEN trace/generated-helpers.c
CC x86_64-softmmu/trace/control-target.o
make: *** [aarch64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
CC x86_64-softmmu/softmmu/main.o
CC x86_64-softmmu/gdbstub-xml.o
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=8c6b159dff054131a5378bedb5c35f58', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-1ttvfwsg/src/docker-src.2020-08-20-12.01.32.8367:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=8c6b159dff054131a5378bedb5c35f58
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-1ttvfwsg/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m46.746s
user 0m9.220s
The full log is available at
http://patchew.org/logs/20200820133201.80577-1-graeme@nuviainc.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, Aug 20, 2020 at 14:32:01 +0100, Graeme Gregory wrote:
> A difference between sbsa platform and the virt platform is PSCI is
> handled by ARM-TF in the sbsa platform. This means that the PSCI code
> there needs to communicate some of the platform power changes down
> to the qemu code for things like shutdown/reset control.
>
> Space has been left to extend the EC if we find other use cases in
> future where ARM-TF and qemu need to communicate.
>
> Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
I can confirm that with the qdev_create->qdev_new fixup, the model can
now reset or shut down through software control. Verified from UEFI
Shell with upstream edk2-platforms and upstream + 1 patch
trusted-firmware-a.
So with that fix:
Tested-by: Leif Lindholm <leif@nuviainc.com>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
Thanks!
/
Leif
> ---
> hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f030a416fd..c8743fc1d0 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -41,6 +41,7 @@
> #include "hw/usb.h"
> #include "hw/char/pl011.h"
> #include "net/net.h"
> +#include "migration/vmstate.h"
>
> #define RAMLIMIT_GB 8192
> #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> @@ -62,6 +63,7 @@ enum {
> SBSA_CPUPERIPHS,
> SBSA_GIC_DIST,
> SBSA_GIC_REDIST,
> + SBSA_SECURE_EC,
> SBSA_SMMU,
> SBSA_UART,
> SBSA_RTC,
> @@ -98,6 +100,14 @@ typedef struct {
> #define SBSA_MACHINE(obj) \
> OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
>
> +typedef struct {
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> +} SECUREECState;
> +
> +#define TYPE_SECURE_EC "sbsa-secure-ec"
> +#define SECURE_EC(obj) OBJECT_CHECK(SECUREECState, (obj), TYPE_SECURE_EC)
> +
> static const MemMapEntry sbsa_ref_memmap[] = {
> /* 512M boot ROM */
> [SBSA_FLASH] = { 0, 0x20000000 },
> @@ -107,6 +117,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
> [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
> [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
> [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
> + [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
> [SBSA_UART] = { 0x60000000, 0x00001000 },
> [SBSA_RTC] = { 0x60010000, 0x00001000 },
> [SBSA_GPIO] = { 0x60020000, 0x00001000 },
> @@ -585,6 +596,65 @@ static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> return board->fdt;
> }
>
> +enum sbsa_secure_ec_powerstates {
> + SBSA_SECURE_EC_CMD_NULL,
> + SBSA_SECURE_EC_CMD_POWEROFF,
> + SBSA_SECURE_EC_CMD_REBOOT,
> +};
> +
> +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + /* No use for this currently */
> + return 0;
> +}
> +
> +static void secure_ec_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + if (offset == 0) { /* PSCI machine power command register */
> + switch (value) {
> + case SBSA_SECURE_EC_CMD_NULL:
> + break;
> + case SBSA_SECURE_EC_CMD_POWEROFF:
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> + break;
> + case SBSA_SECURE_EC_CMD_REBOOT:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + default:
> + error_report("sbsa-ref: ERROR Unkown power command");
> + }
> + } else {
> + error_report("sbsa-ref: unknown EC register");
> + }
> +}
> +
> +static const MemoryRegionOps secure_ec_ops = {
> + .read = secure_ec_read,
> + .write = secure_ec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void secure_ec_init(Object *obj)
> +{
> + SECUREECState *s = SECURE_EC(obj);
> + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> + 0x1000);
> + sysbus_init_mmio(dev, &s->iomem);
> +}
> +
> +static void create_secure_ec(MemoryRegion *mem)
> +{
> + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
> + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +
> + memory_region_add_subregion(mem, base,
> + sysbus_mmio_get_region(s, 0));
> +}
> +
> static void sbsa_ref_init(MachineState *machine)
> {
> unsigned int smp_cpus = machine->smp.cpus;
> @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
>
> create_pcie(sms);
>
> + create_secure_ec(secure_sysmem);
> +
> sms->bootinfo.ram_size = machine->ram_size;
> sms->bootinfo.nb_cpus = smp_cpus;
> sms->bootinfo.board_id = -1;
> @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> .instance_size = sizeof(SBSAMachineState),
> };
>
> +static const VMStateDescription vmstate_secure_ec_info = {
> + .name = "sbsa-secure-ec",
> + .version_id = 0,
> + .minimum_version_id = 0,
> +};
> +
> +static void secure_ec_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &vmstate_secure_ec_info;
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo secure_ec_info = {
> + .name = TYPE_SECURE_EC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(SECUREECState),
> + .instance_init = secure_ec_init,
> + .class_init = secure_ec_class_init,
> +};
> +
> static void sbsa_ref_machine_init(void)
> {
> + type_register_static(&secure_ec_info);
> type_register_static(&sbsa_ref_info);
> }
>
> --
> 2.25.1
>
On 8/20/20 3:32 PM, Graeme Gregory wrote:
> A difference between sbsa platform and the virt platform is PSCI is
> handled by ARM-TF in the sbsa platform. This means that the PSCI code
> there needs to communicate some of the platform power changes down
> to the qemu code for things like shutdown/reset control.
No need to explicit 'fake' in patch subject, almost all emulated
devices are fake.
>
> Space has been left to extend the EC if we find other use cases in
> future where ARM-TF and qemu need to communicate.
>
> Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> ---
> hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f030a416fd..c8743fc1d0 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -41,6 +41,7 @@
> #include "hw/usb.h"
> #include "hw/char/pl011.h"
> #include "net/net.h"
> +#include "migration/vmstate.h"
>
> #define RAMLIMIT_GB 8192
> #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> @@ -62,6 +63,7 @@ enum {
> SBSA_CPUPERIPHS,
> SBSA_GIC_DIST,
> SBSA_GIC_REDIST,
> + SBSA_SECURE_EC,
> SBSA_SMMU,
> SBSA_UART,
> SBSA_RTC,
> @@ -98,6 +100,14 @@ typedef struct {
> #define SBSA_MACHINE(obj) \
> OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
>
> +typedef struct {
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> +} SECUREECState;
> +
> +#define TYPE_SECURE_EC "sbsa-secure-ec"
> +#define SECURE_EC(obj) OBJECT_CHECK(SECUREECState, (obj), TYPE_SECURE_EC)
> +
> static const MemMapEntry sbsa_ref_memmap[] = {
> /* 512M boot ROM */
> [SBSA_FLASH] = { 0, 0x20000000 },
> @@ -107,6 +117,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
> [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
> [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
> [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
> + [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
> [SBSA_UART] = { 0x60000000, 0x00001000 },
> [SBSA_RTC] = { 0x60010000, 0x00001000 },
> [SBSA_GPIO] = { 0x60020000, 0x00001000 },
> @@ -585,6 +596,65 @@ static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> return board->fdt;
> }
>
> +enum sbsa_secure_ec_powerstates {
> + SBSA_SECURE_EC_CMD_NULL,
> + SBSA_SECURE_EC_CMD_POWEROFF,
> + SBSA_SECURE_EC_CMD_REBOOT,
> +};
> +
> +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + /* No use for this currently */
> + return 0;
> +}
> +
> +static void secure_ec_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + if (offset == 0) { /* PSCI machine power command register */
> + switch (value) {
> + case SBSA_SECURE_EC_CMD_NULL:
> + break;
> + case SBSA_SECURE_EC_CMD_POWEROFF:
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> + break;
> + case SBSA_SECURE_EC_CMD_REBOOT:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + default:
> + error_report("sbsa-ref: ERROR Unkown power command");
> + }
> + } else {
> + error_report("sbsa-ref: unknown EC register");
> + }
> +}
> +
> +static const MemoryRegionOps secure_ec_ops = {
> + .read = secure_ec_read,
> + .write = secure_ec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void secure_ec_init(Object *obj)
> +{
> + SECUREECState *s = SECURE_EC(obj);
> + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> + 0x1000);
> + sysbus_init_mmio(dev, &s->iomem);
> +}
> +
> +static void create_secure_ec(MemoryRegion *mem)
> +{
> + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
This API has been removed in 2194abd6231 ("qdev: qdev_create(),
qdev_try_create() are now unused, drop"), can you rebase please?
> + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +
> + memory_region_add_subregion(mem, base,
> + sysbus_mmio_get_region(s, 0));
> +}
> +
> static void sbsa_ref_init(MachineState *machine)
> {
> unsigned int smp_cpus = machine->smp.cpus;
> @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
>
> create_pcie(sms);
>
> + create_secure_ec(secure_sysmem);
> +
> sms->bootinfo.ram_size = machine->ram_size;
> sms->bootinfo.nb_cpus = smp_cpus;
> sms->bootinfo.board_id = -1;
> @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> .instance_size = sizeof(SBSAMachineState),
> };
>
> +static const VMStateDescription vmstate_secure_ec_info = {
> + .name = "sbsa-secure-ec",
> + .version_id = 0,
> + .minimum_version_id = 0,
I'm think you need to initialize that, but VMStateDescription
is not my cup of tea, so better wait for confirmation by someone
more confident with it.
.fields = (VMStateField[]) {
VMSTATE_END_OF_LIST()
}
> +};
> +
> +static void secure_ec_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->vmsd = &vmstate_secure_ec_info;
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo secure_ec_info = {
> + .name = TYPE_SECURE_EC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(SECUREECState),
> + .instance_init = secure_ec_init,
> + .class_init = secure_ec_class_init,
> +};
> +
> static void sbsa_ref_machine_init(void)
> {
> + type_register_static(&secure_ec_info);
> type_register_static(&sbsa_ref_info);
> }
>
>
On Fri, Aug 21, 2020 at 03:49:11PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/20/20 3:32 PM, Graeme Gregory wrote:
> > A difference between sbsa platform and the virt platform is PSCI is
> > handled by ARM-TF in the sbsa platform. This means that the PSCI code
> > there needs to communicate some of the platform power changes down
> > to the qemu code for things like shutdown/reset control.
>
> No need to explicit 'fake' in patch subject, almost all emulated
> devices are fake.
>
Noted, will change.
> >
> > Space has been left to extend the EC if we find other use cases in
> > future where ARM-TF and qemu need to communicate.
> >
> > Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> > ---
> > hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> >
> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > index f030a416fd..c8743fc1d0 100644
> > --- a/hw/arm/sbsa-ref.c
> > +++ b/hw/arm/sbsa-ref.c
> > @@ -41,6 +41,7 @@
> > #include "hw/usb.h"
> > #include "hw/char/pl011.h"
> > #include "net/net.h"
> > +#include "migration/vmstate.h"
> >
> > #define RAMLIMIT_GB 8192
> > #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> > @@ -62,6 +63,7 @@ enum {
> > SBSA_CPUPERIPHS,
> > SBSA_GIC_DIST,
> > SBSA_GIC_REDIST,
> > + SBSA_SECURE_EC,
> > SBSA_SMMU,
> > SBSA_UART,
> > SBSA_RTC,
> > @@ -98,6 +100,14 @@ typedef struct {
> > #define SBSA_MACHINE(obj) \
> > OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
> >
> > +typedef struct {
> > + SysBusDevice parent_obj;
> > + MemoryRegion iomem;
> > +} SECUREECState;
> > +
> > +#define TYPE_SECURE_EC "sbsa-secure-ec"
> > +#define SECURE_EC(obj) OBJECT_CHECK(SECUREECState, (obj), TYPE_SECURE_EC)
> > +
> > static const MemMapEntry sbsa_ref_memmap[] = {
> > /* 512M boot ROM */
> > [SBSA_FLASH] = { 0, 0x20000000 },
> > @@ -107,6 +117,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
> > [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
> > [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
> > [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
> > + [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
> > [SBSA_UART] = { 0x60000000, 0x00001000 },
> > [SBSA_RTC] = { 0x60010000, 0x00001000 },
> > [SBSA_GPIO] = { 0x60020000, 0x00001000 },
> > @@ -585,6 +596,65 @@ static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> > return board->fdt;
> > }
> >
> > +enum sbsa_secure_ec_powerstates {
> > + SBSA_SECURE_EC_CMD_NULL,
> > + SBSA_SECURE_EC_CMD_POWEROFF,
> > + SBSA_SECURE_EC_CMD_REBOOT,
> > +};
> > +
> > +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > + /* No use for this currently */
> > + return 0;
> > +}
> > +
> > +static void secure_ec_write(void *opaque, hwaddr offset,
> > + uint64_t value, unsigned size)
> > +{
> > + if (offset == 0) { /* PSCI machine power command register */
> > + switch (value) {
> > + case SBSA_SECURE_EC_CMD_NULL:
> > + break;
> > + case SBSA_SECURE_EC_CMD_POWEROFF:
> > + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > + break;
> > + case SBSA_SECURE_EC_CMD_REBOOT:
> > + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > + break;
> > + default:
> > + error_report("sbsa-ref: ERROR Unkown power command");
> > + }
> > + } else {
> > + error_report("sbsa-ref: unknown EC register");
> > + }
> > +}
> > +
> > +static const MemoryRegionOps secure_ec_ops = {
> > + .read = secure_ec_read,
> > + .write = secure_ec_write,
> > + .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +static void secure_ec_init(Object *obj)
> > +{
> > + SECUREECState *s = SECURE_EC(obj);
> > + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +
> > + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> > + 0x1000);
> > + sysbus_init_mmio(dev, &s->iomem);
> > +}
> > +
> > +static void create_secure_ec(MemoryRegion *mem)
> > +{
> > + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> > + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
>
> This API has been removed in 2194abd6231 ("qdev: qdev_create(),
> qdev_try_create() are now unused, drop"), can you rebase please?
>
Yes, I accidently sent from a work branch not top of tree, noticed
1ms after sending!
> > + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > +
> > + memory_region_add_subregion(mem, base,
> > + sysbus_mmio_get_region(s, 0));
> > +}
> > +
> > static void sbsa_ref_init(MachineState *machine)
> > {
> > unsigned int smp_cpus = machine->smp.cpus;
> > @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
> >
> > create_pcie(sms);
> >
> > + create_secure_ec(secure_sysmem);
> > +
> > sms->bootinfo.ram_size = machine->ram_size;
> > sms->bootinfo.nb_cpus = smp_cpus;
> > sms->bootinfo.board_id = -1;
> > @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> > .instance_size = sizeof(SBSAMachineState),
> > };
> >
> > +static const VMStateDescription vmstate_secure_ec_info = {
> > + .name = "sbsa-secure-ec",
> > + .version_id = 0,
> > + .minimum_version_id = 0,
>
> I'm think you need to initialize that, but VMStateDescription
> is not my cup of tea, so better wait for confirmation by someone
> more confident with it.
>
> .fields = (VMStateField[]) {
> VMSTATE_END_OF_LIST()
> }
>
Peter answered this so Ill implement how he suggested without vmstate.
> > +};
> > +
> > +static void secure_ec_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + dc->vmsd = &vmstate_secure_ec_info;
> > + dc->user_creatable = false;
> > +}
> > +
> > +static const TypeInfo secure_ec_info = {
> > + .name = TYPE_SECURE_EC,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(SECUREECState),
> > + .instance_init = secure_ec_init,
> > + .class_init = secure_ec_class_init,
> > +};
> > +
> > static void sbsa_ref_machine_init(void)
> > {
> > + type_register_static(&secure_ec_info);
> > type_register_static(&sbsa_ref_info);
> > }
> >
> >
>
Thanks for the review.
Graeme
On Thu, 20 Aug 2020 at 14:32, Graeme Gregory <graeme@nuviainc.com> wrote:
>
> A difference between sbsa platform and the virt platform is PSCI is
> handled by ARM-TF in the sbsa platform. This means that the PSCI code
> there needs to communicate some of the platform power changes down
> to the qemu code for things like shutdown/reset control.
>
> Space has been left to extend the EC if we find other use cases in
> future where ARM-TF and qemu need to communicate.
>
> Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> ---
> hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f030a416fd..c8743fc1d0 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -41,6 +41,7 @@
> #include "hw/usb.h"
> #include "hw/char/pl011.h"
> #include "net/net.h"
> +#include "migration/vmstate.h"
>
> #define RAMLIMIT_GB 8192
> #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> @@ -62,6 +63,7 @@ enum {
> SBSA_CPUPERIPHS,
> SBSA_GIC_DIST,
> SBSA_GIC_REDIST,
> + SBSA_SECURE_EC,
> SBSA_SMMU,
> SBSA_UART,
> SBSA_RTC,
> @@ -98,6 +100,14 @@ typedef struct {
> #define SBSA_MACHINE(obj) \
> OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
>
> +typedef struct {
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> +} SECUREECState;
Could you put the definition of the device in its own .c file,
please (hw/misc/ seems like the right place). We generally
prefer to keep the board and individual device models
separate. (Some older code in the tree still has devices
lurking in source files, but new stuff generally follows
the rules.)
> +enum sbsa_secure_ec_powerstates {
> + SBSA_SECURE_EC_CMD_NULL,
> + SBSA_SECURE_EC_CMD_POWEROFF,
> + SBSA_SECURE_EC_CMD_REBOOT,
The last two are clear enough, but what's a null power state for?
> +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + /* No use for this currently */
> + return 0;
> +}
> +
> +static void secure_ec_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + if (offset == 0) { /* PSCI machine power command register */
> + switch (value) {
> + case SBSA_SECURE_EC_CMD_NULL:
> + break;
> + case SBSA_SECURE_EC_CMD_POWEROFF:
> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> + break;
> + case SBSA_SECURE_EC_CMD_REBOOT:
> + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> + break;
> + default:
> + error_report("sbsa-ref: ERROR Unkown power command");
"unknown".
We prefer qemu_log_mask(LOG_GUEST_ERROR, ...) for logging this
kind of "guest code did something wrong" situation.
(you could also do that for attempting to read this w/o register
if you liked).
> + }
> + } else {
> + error_report("sbsa-ref: unknown EC register");
similarly here
> + }
> +}
> +
> +static const MemoryRegionOps secure_ec_ops = {
> + .read = secure_ec_read,
> + .write = secure_ec_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
Consider explicitly specifying the permitted access size
by setting .valid.min_access_size and .valid.max_access_size
(restricting guests to only doing 32-bit writes will make
life easier when you come to add registers later...)
> +};
> +
> +static void secure_ec_init(Object *obj)
> +{
> + SECUREECState *s = SECURE_EC(obj);
> + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> + 0x1000);
Minor indent error here.
> + sysbus_init_mmio(dev, &s->iomem);
> +}
> +
> +static void create_secure_ec(MemoryRegion *mem)
> +{
> + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
> + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +
> + memory_region_add_subregion(mem, base,
> + sysbus_mmio_get_region(s, 0));
> +}
> +
> static void sbsa_ref_init(MachineState *machine)
> {
> unsigned int smp_cpus = machine->smp.cpus;
> @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
>
> create_pcie(sms);
>
> + create_secure_ec(secure_sysmem);
> +
> sms->bootinfo.ram_size = machine->ram_size;
> sms->bootinfo.nb_cpus = smp_cpus;
> sms->bootinfo.board_id = -1;
> @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> .instance_size = sizeof(SBSAMachineState),
> };
>
> +static const VMStateDescription vmstate_secure_ec_info = {
> + .name = "sbsa-secure-ec",
> + .version_id = 0,
> + .minimum_version_id = 0,
> +};
If you don't have any internal state in a device (as in
this case), then you don't need to set dc->vmsd at all.
Just have a comment in the class init saying
/* No vmstate or reset required: device has no internal state */
thanks
-- PMM
On Mon, Aug 24, 2020 at 03:59:38PM +0100, Peter Maydell wrote:
> On Thu, 20 Aug 2020 at 14:32, Graeme Gregory <graeme@nuviainc.com> wrote:
> >
> > A difference between sbsa platform and the virt platform is PSCI is
> > handled by ARM-TF in the sbsa platform. This means that the PSCI code
> > there needs to communicate some of the platform power changes down
> > to the qemu code for things like shutdown/reset control.
> >
> > Space has been left to extend the EC if we find other use cases in
> > future where ARM-TF and qemu need to communicate.
> >
> > Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> > ---
> > hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> >
> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > index f030a416fd..c8743fc1d0 100644
> > --- a/hw/arm/sbsa-ref.c
> > +++ b/hw/arm/sbsa-ref.c
> > @@ -41,6 +41,7 @@
> > #include "hw/usb.h"
> > #include "hw/char/pl011.h"
> > #include "net/net.h"
> > +#include "migration/vmstate.h"
> >
> > #define RAMLIMIT_GB 8192
> > #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> > @@ -62,6 +63,7 @@ enum {
> > SBSA_CPUPERIPHS,
> > SBSA_GIC_DIST,
> > SBSA_GIC_REDIST,
> > + SBSA_SECURE_EC,
> > SBSA_SMMU,
> > SBSA_UART,
> > SBSA_RTC,
> > @@ -98,6 +100,14 @@ typedef struct {
> > #define SBSA_MACHINE(obj) \
> > OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
> >
> > +typedef struct {
> > + SysBusDevice parent_obj;
> > + MemoryRegion iomem;
> > +} SECUREECState;
>
> Could you put the definition of the device in its own .c file,
> please (hw/misc/ seems like the right place). We generally
> prefer to keep the board and individual device models
> separate. (Some older code in the tree still has devices
> lurking in source files, but new stuff generally follows
> the rules.)
>
Yes, certainly!
> > +enum sbsa_secure_ec_powerstates {
> > + SBSA_SECURE_EC_CMD_NULL,
> > + SBSA_SECURE_EC_CMD_POWEROFF,
> > + SBSA_SECURE_EC_CMD_REBOOT,
>
> The last two are clear enough, but what's a null power state for?
>
My personal dislike of sending 0 to hardware as its harder to spot
when using scopes/logic analyzers. I can remove it if you wish and
explicitly number the states.
> > +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > + /* No use for this currently */
> > + return 0;
> > +}
> > +
> > +static void secure_ec_write(void *opaque, hwaddr offset,
> > + uint64_t value, unsigned size)
> > +{
> > + if (offset == 0) { /* PSCI machine power command register */
> > + switch (value) {
> > + case SBSA_SECURE_EC_CMD_NULL:
> > + break;
> > + case SBSA_SECURE_EC_CMD_POWEROFF:
> > + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > + break;
> > + case SBSA_SECURE_EC_CMD_REBOOT:
> > + qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > + break;
> > + default:
> > + error_report("sbsa-ref: ERROR Unkown power command");
>
> "unknown".
>
> We prefer qemu_log_mask(LOG_GUEST_ERROR, ...) for logging this
> kind of "guest code did something wrong" situation.
> (you could also do that for attempting to read this w/o register
> if you liked).
>
Excellent that is much better, Ill make that change.
> > + }
> > + } else {
> > + error_report("sbsa-ref: unknown EC register");
>
> similarly here
>
> > + }
> > +}
> > +
> > +static const MemoryRegionOps secure_ec_ops = {
> > + .read = secure_ec_read,
> > + .write = secure_ec_write,
> > + .endianness = DEVICE_NATIVE_ENDIAN,
>
> Consider explicitly specifying the permitted access size
> by setting .valid.min_access_size and .valid.max_access_size
> (restricting guests to only doing 32-bit writes will make
> life easier when you come to add registers later...)
>
That was my original intent so i will do this.
> > +};
> > +
> > +static void secure_ec_init(Object *obj)
> > +{
> > + SECUREECState *s = SECURE_EC(obj);
> > + SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +
> > + memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> > + 0x1000);
>
> Minor indent error here.
>
Will fix, just FYI checkpatch does not pick this up currently.
> > + sysbus_init_mmio(dev, &s->iomem);
> > +}
> > +
> > +static void create_secure_ec(MemoryRegion *mem)
> > +{
> > + hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> > + DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
> > + SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > +
> > + memory_region_add_subregion(mem, base,
> > + sysbus_mmio_get_region(s, 0));
> > +}
> > +
> > static void sbsa_ref_init(MachineState *machine)
> > {
> > unsigned int smp_cpus = machine->smp.cpus;
> > @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
> >
> > create_pcie(sms);
> >
> > + create_secure_ec(secure_sysmem);
> > +
> > sms->bootinfo.ram_size = machine->ram_size;
> > sms->bootinfo.nb_cpus = smp_cpus;
> > sms->bootinfo.board_id = -1;
> > @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> > .instance_size = sizeof(SBSAMachineState),
> > };
> >
> > +static const VMStateDescription vmstate_secure_ec_info = {
> > + .name = "sbsa-secure-ec",
> > + .version_id = 0,
> > + .minimum_version_id = 0,
> > +};
>
> If you don't have any internal state in a device (as in
> this case), then you don't need to set dc->vmsd at all.
> Just have a comment in the class init saying
> /* No vmstate or reset required: device has no internal state */
>
Thanks, that is cleaner, Ill do this.
Thanks for the review, Ill get right on v2.
Graeme
On Tue, 25 Aug 2020 at 11:08, Graeme Gregory <graeme@nuviainc.com> wrote:
>
> On Mon, Aug 24, 2020 at 03:59:38PM +0100, Peter Maydell wrote:
> > > +enum sbsa_secure_ec_powerstates {
> > > + SBSA_SECURE_EC_CMD_NULL,
> > > + SBSA_SECURE_EC_CMD_POWEROFF,
> > > + SBSA_SECURE_EC_CMD_REBOOT,
> >
> > The last two are clear enough, but what's a null power state for?
> >
> My personal dislike of sending 0 to hardware as its harder to spot
> when using scopes/logic analyzers. I can remove it if you wish and
> explicitly number the states.
Yeah, just number the states 1 and 2 rather than having an
unused 0 named state, I think.
PS: when you respin make sure you're on top-of-git -- we just
landed the meson build system conversion, so the way you add
new source files is different (no more Makefile.objs, it goes
in the meson.build file in the relevant directory instead).
thanks
-- PMM
© 2016 - 2024 Red Hat, Inc.