Quick QAPI schema sanity check, mostly.
Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
> SGX EPC is enumerated through CPUID, i.e. EPC "devices" need to be
> realized prior to realizing the vCPUs themselves, which occurs long
> before generic devices are parsed and realized. Because of this,
> do not allow 'sgx-epc' devices to be instantiated after vCPUS have
> been created.
>
> The 'sgx-epc' device is essentially a placholder at this time, it will
> be fully implemented in a future patch along with a dedicated command
> to create 'sgx-epc' devices.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> hw/i386/Makefile.objs | 1 +
> hw/i386/sgx-epc.c | 169 ++++++++++++++++++++++++++++++++++++++
> include/hw/i386/sgx-epc.h | 44 ++++++++++
> qapi/misc.json | 32 +++++++-
> 4 files changed, 244 insertions(+), 2 deletions(-)
> create mode 100644 hw/i386/sgx-epc.c
> create mode 100644 include/hw/i386/sgx-epc.h
>
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 5d9c9efd5f..18c9693d9d 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -13,3 +13,4 @@ obj-$(CONFIG_VMMOUSE) += vmmouse.o
>
> obj-y += kvmvapic.o
> obj-y += acpi-build.o
> +obj-y += sgx-epc.o
> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
> new file mode 100644
> index 0000000000..73221ba86b
> --- /dev/null
> +++ b/hw/i386/sgx-epc.c
> @@ -0,0 +1,169 @@
> +/*
> + * SGX EPC device
> + *
> + * Copyright (C) 2019 Intel Corporation
> + *
> + * Authors:
> + * Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "hw/i386/pc.h"
> +#include "hw/i386/sgx-epc.h"
> +#include "hw/mem/memory-device.h"
> +#include "monitor/qdev.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +#include "qemu/option.h"
> +#include "qemu/units.h"
> +#include "target/i386/cpu.h"
> +
> +static Property sgx_epc_properties[] = {
> + DEFINE_PROP_UINT64(SGX_EPC_ADDR_PROP, SGXEPCDevice, addr, 0),
> + DEFINE_PROP_LINK(SGX_EPC_MEMDEV_PROP, SGXEPCDevice, hostmem,
> + TYPE_MEMORY_BACKEND, HostMemoryBackend *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void sgx_epc_get_size(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + Error *local_err = NULL;
> + uint64_t value;
> +
> + value = memory_device_get_region_size(MEMORY_DEVICE(obj), &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + visit_type_uint64(v, name, &value, errp);
> +}
> +
> +static void sgx_epc_init(Object *obj)
> +{
> + object_property_add(obj, SGX_EPC_SIZE_PROP, "uint64", sgx_epc_get_size,
> + NULL, NULL, NULL, &error_abort);
> +}
> +
> +static void sgx_epc_realize(DeviceState *dev, Error **errp)
> +{
> + PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> + SGXEPCDevice *epc = SGX_EPC(dev);
> +
> + if (pcms->boot_cpus != 0) {
> + error_setg(errp,
> + "'" TYPE_SGX_EPC "' can't be created after vCPUs, e.g. via -device");
> + return;
> + }
> +
> + if (!epc->hostmem) {
> + error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property is not set");
> + return;
> + } else if (host_memory_backend_is_mapped(epc->hostmem)) {
> + char *path = object_get_canonical_path_component(OBJECT(epc->hostmem));
> + error_setg(errp, "can't use already busy memdev: %s", path);
> + g_free(path);
> + return;
> + }
Please avoid "return; else":
if (!epc->hostmem) {
error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property is not set");
return;
}
if (host_memory_backend_is_mapped(epc->hostmem)) {
char *path = object_get_canonical_path_component(OBJECT(epc->hostmem));
error_setg(errp, "can't use already busy memdev: %s", path);
g_free(path);
return;
}
> +
> + error_setg(errp, "'" TYPE_SGX_EPC "' not supported");
> +}
> +
> +static void sgx_epc_unrealize(DeviceState *dev, Error **errp)
> +{
> + SGXEPCDevice *epc = SGX_EPC(dev);
> +
> + host_memory_backend_set_mapped(epc->hostmem, false);
> +}
> +
> +static uint64_t sgx_epc_md_get_addr(const MemoryDeviceState *md)
> +{
> + const SGXEPCDevice *epc = SGX_EPC(md);
> +
> + return epc->addr;
> +}
> +
> +static void sgx_epc_md_set_addr(MemoryDeviceState *md, uint64_t addr,
> + Error **errp)
> +{
> + object_property_set_uint(OBJECT(md), addr, SGX_EPC_ADDR_PROP, errp);
> +}
> +
> +static uint64_t sgx_epc_md_get_plugged_size(const MemoryDeviceState *md,
> + Error **errp)
> +{
> + return 0;
> +}
> +
> +static MemoryRegion *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
> + Error **errp)
> +{
> + SGXEPCDevice *epc = SGX_EPC(md);
> +
> + if (!epc->hostmem) {
> + error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> + return NULL;
> + }
> +
> + return host_memory_backend_get_memory(epc->hostmem);
> +}
> +
> +static void sgx_epc_md_fill_device_info(const MemoryDeviceState *md,
> + MemoryDeviceInfo *info)
> +{
> + SGXEPCDeviceInfo *di = g_new0(SGXEPCDeviceInfo, 1);
> + const SGXEPCDevice *epc = SGX_EPC(md);
> + const DeviceState *dev = DEVICE(md);
> +
> + if (dev->id) {
> + di->has_id = true;
> + di->id = g_strdup(dev->id);
> + }
> + di->addr = epc->addr;
> + di->node = 0 /* TODO: EPC NUMA spec not yet defined */;
> + di->size = memory_device_get_region_size(MEMORY_DEVICE(epc), &error_fatal);
> + di->memdev = object_get_canonical_path(OBJECT(epc->hostmem));
> +}
> +
> +static void sgx_epc_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc);
> +
> + dc->hotpluggable = false;
> + dc->realize = sgx_epc_realize;
> + dc->unrealize = sgx_epc_unrealize;
> + dc->props = sgx_epc_properties;
> + dc->desc = "SGX EPC section";
> +
> + mdc->get_addr = sgx_epc_md_get_addr;
> + mdc->set_addr = sgx_epc_md_set_addr;
> + mdc->get_plugged_size = sgx_epc_md_get_plugged_size;
> + mdc->get_memory_region = sgx_epc_md_get_memory_region;
> + mdc->fill_device_info = sgx_epc_md_fill_device_info;
> +}
> +
> +static TypeInfo sgx_epc_info = {
> + .name = TYPE_SGX_EPC,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(SGXEPCDevice),
> + .instance_init = sgx_epc_init,
> + .class_init = sgx_epc_class_init,
> + .class_size = sizeof(DeviceClass),
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_MEMORY_DEVICE },
> + { }
> + },
> +};
> +
> +static void sgx_epc_register_types(void)
> +{
> + type_register_static(&sgx_epc_info);
> +}
> +
> +type_init(sgx_epc_register_types)
> diff --git a/include/hw/i386/sgx-epc.h b/include/hw/i386/sgx-epc.h
> new file mode 100644
> index 0000000000..5fd9ae2d0c
> --- /dev/null
> +++ b/include/hw/i386/sgx-epc.h
> @@ -0,0 +1,44 @@
> +/*
> + * SGX EPC device
> + *
> + * Copyright (C) 2019 Intel Corporation
> + *
> + * Authors:
> + * Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef QEMU_SGX_EPC_H
> +#define QEMU_SGX_EPC_H
> +
> +#include "sysemu/hostmem.h"
> +
> +#define TYPE_SGX_EPC "sgx-epc"
> +#define SGX_EPC(obj) \
> + OBJECT_CHECK(SGXEPCDevice, (obj), TYPE_SGX_EPC)
> +#define SGX_EPC_CLASS(oc) \
> + OBJECT_CLASS_CHECK(SGXEPCDeviceClass, (oc), TYPE_SGX_EPC)
> +#define SGX_EPC_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(SGXEPCDeviceClass, (obj), TYPE_SGX_EPC)
> +
> +#define SGX_EPC_ADDR_PROP "addr"
> +#define SGX_EPC_SIZE_PROP "size"
> +#define SGX_EPC_MEMDEV_PROP "memdev"
> +
> +/**
> + * SGXEPCDevice:
> + * @addr: starting guest physical address, where @SGXEPCDevice is mapped.
> + * Default value: 0, means that address is auto-allocated.
> + * @hostmem: host memory backend providing memory for @SGXEPCDevice
> + */
> +typedef struct SGXEPCDevice {
> + /* private */
> + DeviceState parent_obj;
> +
> + /* public */
> + uint64_t addr;
> + HostMemoryBackend *hostmem;
> +} SGXEPCDevice;
> +
> +#endif
> diff --git a/qapi/misc.json b/qapi/misc.json
> index a7fba7230c..965905c9e8 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1573,19 +1573,47 @@
> }
> }
>
> +##
> +# @SGXEPCDeviceInfo:
> +#
> +# SGX EPC state information
> +#
> +# @id: device's ID
> +#
> +# @addr: physical address, where device is mapped
> +#
> +# @size: size of memory that the device provides
> +#
> +# @node: NUMA node number where device is plugged in
> +#
> +# @memdev: memory backend linked with device
> +#
> +# Since: TBD
> +##
> +{ 'struct': 'SGXEPCDeviceInfo',
> + 'data': { '*id': 'str',
> + 'addr': 'int',
> + 'size': 'int',
> + 'node': 'int',
> + 'memdev': 'str'
> + }
> +}
> +
> ##
> # @MemoryDeviceInfo:
> #
> # Union containing information about a memory device
> #
> -# nvdimm is included since 2.12. virtio-pmem is included since 4.1.
> +# nvdimm is included since 2.12. virtio-pmem is included since 4.1,
> +# sgx-epc is included since TBD.
> #
> # Since: 2.1
> ##
> { 'union': 'MemoryDeviceInfo',
> 'data': { 'dimm': 'PCDIMMDeviceInfo',
> 'nvdimm': 'PCDIMMDeviceInfo',
> - 'virtio-pmem': 'VirtioPMEMDeviceInfo'
> + 'virtio-pmem': 'VirtioPMEMDeviceInfo',
> + 'sgx-epc': 'SGXEPCDeviceInfo'
> }
> }
This adds a fourth kind of MemoryDeviceInfo. Their doc comments all
neglect to tell us what a "DIMM Device" is, why it's a "PC DIMM Device",
how that differs from an "NVDIMM Device", what a "Virtio PMEM Device"
is, and now what an "SGX EPC Device" is.
I'd appreciate a brief explanation, possibly with a reference to
pertinent documentation elsewhere. I'm not demanding you do that for
the existing kinds, too. Igor, perhaps?
