| Message ID | 20240928085727.56883-13-phil@philjordan.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | macOS PV Graphics and new vmapple machine type | expand |
On 2024/09/28 17:57, Phil Dennis-Jordan wrote: > From: Alexander Graf <graf@amazon.com> > > Instead of device tree or other more standardized means, VMApple passes > platform configuration to the first stage boot loader in a binary encoded > format that resides at a dedicated RAM region in physical address space. > > This patch models this configuration space as a qdev device which we can > then map at the fixed location in the address space. That way, we can > influence and annotate all configuration fields easily. > > Signed-off-by: Alexander Graf <graf@amazon.com> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > --- > v3: > > * Replaced legacy device reset method with Resettable method > > hw/vmapple/Kconfig | 3 ++ > hw/vmapple/cfg.c | 106 +++++++++++++++++++++++++++++++++++++++ > hw/vmapple/meson.build | 1 + > include/hw/vmapple/cfg.h | 68 +++++++++++++++++++++++++ > 4 files changed, 178 insertions(+) > create mode 100644 hw/vmapple/cfg.c > create mode 100644 include/hw/vmapple/cfg.h > > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig > index 68f88876eb9..8bbeb9a9237 100644 > --- a/hw/vmapple/Kconfig > +++ b/hw/vmapple/Kconfig > @@ -4,3 +4,6 @@ config VMAPPLE_AES > config VMAPPLE_BDIF > bool > > +config VMAPPLE_CFG > + bool > + > diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c > new file mode 100644 > index 00000000000..a5e5c62f59f > --- /dev/null > +++ b/hw/vmapple/cfg.c > @@ -0,0 +1,106 @@ > +/* > + * VMApple Configuration Region > + * > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. > + * > + * 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/vmapple/cfg.h" > +#include "qemu/log.h" > +#include "qemu/module.h" > +#include "qapi/error.h" > + > +static void vmapple_cfg_reset(Object *obj, ResetType type) > +{ > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > + VMAppleCfg *cfg; > + > + cfg = memory_region_get_ram_ptr(&s->mem); > + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); > + *cfg = s->cfg; > +}> + > +static void vmapple_cfg_realize(DeviceState *dev, Error **errp) > +{ > + VMAppleCfgState *s = VMAPPLE_CFG(dev); > + uint32_t i; > + > + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); > + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); > + strncpy(s->cfg.soc_name, s->soc_name, sizeof(s->cfg.soc_name)); > + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); Use qemu_strnlen() to report an error for too long strings. > + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); > + s->cfg.version = 2; > + s->cfg.unk1 = 1; > + s->cfg.unk2 = 1; > + s->cfg.unk3 = 0x20; > + s->cfg.unk4 = 0; > + s->cfg.unk5 = 1; > + s->cfg.unk6 = 1; > + s->cfg.unk7 = 0; > + s->cfg.unk10 = 1; > + > + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); Report an error instead of asserting. > + for (i = 0; i < s->cfg.nr_cpus; i++) { > + s->cfg.cpu_ids[i] = i; > + } > +}> + > +static void vmapple_cfg_init(Object *obj) > +{ > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > + > + memory_region_init_ram(&s->mem, obj, "VMApple Config", VMAPPLE_CFG_SIZE, > + &error_fatal); > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); > + > + s->serial = (char *)"1234"; > + s->model = (char *)"VM0001"; > + s->soc_name = (char *)"Apple M1 (Virtual)"; These casts are unsafe; these pointers will be freed when this device is freed. > +} > + > +static Property vmapple_cfg_properties[] = { > + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, cfg.nr_cpus, 1), > + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), > + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, cfg.ram_size, 0), > + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, cfg.run_installer1, 0), > + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, cfg.run_installer2, 0), > + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), > + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, cfg.mac_en0), > + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, cfg.mac_en1), > + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, cfg.mac_wifi0), > + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, cfg.mac_bt0), > + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), > + DEFINE_PROP_STRING("model", VMAppleCfgState, model), > + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, soc_name), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void vmapple_cfg_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + ResettableClass *rc = RESETTABLE_CLASS(klass); > + > + dc->realize = vmapple_cfg_realize; > + dc->desc = "VMApple Configuration Region"; > + device_class_set_props(dc, vmapple_cfg_properties); > + rc->phases.hold = vmapple_cfg_reset; > +} > + > +static const TypeInfo vmapple_cfg_info = { > + .name = TYPE_VMAPPLE_CFG, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(VMAppleCfgState), > + .instance_init = vmapple_cfg_init, > + .class_init = vmapple_cfg_class_init, > +}; > + > +static void vmapple_cfg_register_types(void) > +{ > + type_register_static(&vmapple_cfg_info); > +} > + > +type_init(vmapple_cfg_register_types) > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build > index d4624713deb..64b78693a31 100644 > --- a/hw/vmapple/meson.build > +++ b/hw/vmapple/meson.build > @@ -1,2 +1,3 @@ > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: files('aes.c')) > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: files('bdif.c')) > +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: files('cfg.c')) > diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h > new file mode 100644 > index 00000000000..3337064e447 > --- /dev/null > +++ b/include/hw/vmapple/cfg.h > @@ -0,0 +1,68 @@ > +/* > + * VMApple Configuration Region > + * > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. > + * > + * 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 HW_VMAPPLE_CFG_H > +#define HW_VMAPPLE_CFG_H > + > +#include "hw/sysbus.h" > +#include "qom/object.h" > +#include "net/net.h" > + > +typedef struct VMAppleCfg { > + uint32_t version; /* 0x000 */ > + uint32_t nr_cpus; /* 0x004 */ > + uint32_t unk1; /* 0x008 */ > + uint32_t unk2; /* 0x00c */ > + uint32_t unk3; /* 0x010 */ > + uint32_t unk4; /* 0x014 */ > + uint64_t ecid; /* 0x018 */ > + uint64_t ram_size; /* 0x020 */ > + uint32_t run_installer1; /* 0x028 */ > + uint32_t unk5; /* 0x02c */ > + uint32_t unk6; /* 0x030 */ > + uint32_t run_installer2; /* 0x034 */ > + uint32_t rnd; /* 0x038 */ > + uint32_t unk7; /* 0x03c */ > + MACAddr mac_en0; /* 0x040 */ > + uint8_t pad1[2]; > + MACAddr mac_en1; /* 0x048 */ > + uint8_t pad2[2]; > + MACAddr mac_wifi0; /* 0x050 */ > + uint8_t pad3[2]; > + MACAddr mac_bt0; /* 0x058 */ > + uint8_t pad4[2]; > + uint8_t reserved[0xa0]; /* 0x060 */ > + uint32_t cpu_ids[0x80]; /* 0x100 */ > + uint8_t scratch[0x200]; /* 0x180 */ > + char serial[32]; /* 0x380 */ > + char unk8[32]; /* 0x3a0 */ > + char model[32]; /* 0x3c0 */ > + uint8_t unk9[32]; /* 0x3e0 */ > + uint32_t unk10; /* 0x400 */ > + char soc_name[32]; /* 0x404 */ > +} VMAppleCfg; > + > +#define TYPE_VMAPPLE_CFG "vmapple-cfg" > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) > + > +struct VMAppleCfgState { > + /* <private> */ > + SysBusDevice parent_obj; > + VMAppleCfg cfg; > + > + /* <public> */ > + MemoryRegion mem; > + char *serial; > + char *model; > + char *soc_name; > +}; > + > +#define VMAPPLE_CFG_SIZE 0x00010000 > + > +#endif /* HW_VMAPPLE_CFG_H */
On Sat, 5 Oct 2024 at 07:35, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2024/09/28 17:57, Phil Dennis-Jordan wrote: > > From: Alexander Graf <graf@amazon.com> > > > > Instead of device tree or other more standardized means, VMApple passes > > platform configuration to the first stage boot loader in a binary encoded > > format that resides at a dedicated RAM region in physical address space. > > > > This patch models this configuration space as a qdev device which we can > > then map at the fixed location in the address space. That way, we can > > influence and annotate all configuration fields easily. > > > > Signed-off-by: Alexander Graf <graf@amazon.com> > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > > > --- > > v3: > > > > * Replaced legacy device reset method with Resettable method > > > > hw/vmapple/Kconfig | 3 ++ > > hw/vmapple/cfg.c | 106 +++++++++++++++++++++++++++++++++++++++ > > hw/vmapple/meson.build | 1 + > > include/hw/vmapple/cfg.h | 68 +++++++++++++++++++++++++ > > 4 files changed, 178 insertions(+) > > create mode 100644 hw/vmapple/cfg.c > > create mode 100644 include/hw/vmapple/cfg.h > > > > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig > > index 68f88876eb9..8bbeb9a9237 100644 > > --- a/hw/vmapple/Kconfig > > +++ b/hw/vmapple/Kconfig > > @@ -4,3 +4,6 @@ config VMAPPLE_AES > > config VMAPPLE_BDIF > > bool > > > > +config VMAPPLE_CFG > > + bool > > + > > diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c > > new file mode 100644 > > index 00000000000..a5e5c62f59f > > --- /dev/null > > +++ b/hw/vmapple/cfg.c > > @@ -0,0 +1,106 @@ > > +/* > > + * VMApple Configuration Region > > + * > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights > Reserved. > > + * > > + * 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/vmapple/cfg.h" > > +#include "qemu/log.h" > > +#include "qemu/module.h" > > +#include "qapi/error.h" > > + > > +static void vmapple_cfg_reset(Object *obj, ResetType type) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > + VMAppleCfg *cfg; > > + > > + cfg = memory_region_get_ram_ptr(&s->mem); > > + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); > > + *cfg = s->cfg; > > +}> + > > +static void vmapple_cfg_realize(DeviceState *dev, Error **errp) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(dev); > > + uint32_t i; > > + > > + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); > > + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); > > + strncpy(s->cfg.soc_name, s->soc_name, sizeof(s->cfg.soc_name)); > > + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); > > Use qemu_strnlen() to report an error for too long strings. > Hmm, I don't see any existing instances of such a pattern. I do however see a couple of uses of g_strlcpy in the Qemu codebase - that would be a better candidate for error checked string copying, though it still involves some awkward return value checks. I'm going to wrap that in a helper function and macro to replace all 4 strncpy instances here. If the same thing is useful elsewhere later, it can be promoted to cutils or similar. (Also, I notice that last strncpy actually uses the wrong destination size; my wrapper macro uses ARRAY_SIZE to avoid this mistake altogether.) > + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); > > + s->cfg.version = 2; > > + s->cfg.unk1 = 1; > > + s->cfg.unk2 = 1; > > + s->cfg.unk3 = 0x20; > > + s->cfg.unk4 = 0; > > + s->cfg.unk5 = 1; > > + s->cfg.unk6 = 1; > > + s->cfg.unk7 = 0; > > + s->cfg.unk10 = 1; > > + > > + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); > > Report an error instead of asserting. > > > + for (i = 0; i < s->cfg.nr_cpus; i++) { > > + s->cfg.cpu_ids[i] = i; > > + } > > +}> + > > +static void vmapple_cfg_init(Object *obj) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > + > > + memory_region_init_ram(&s->mem, obj, "VMApple Config", > VMAPPLE_CFG_SIZE, > > + &error_fatal); > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); > > + > > + s->serial = (char *)"1234"; > > + s->model = (char *)"VM0001"; > > + s->soc_name = (char *)"Apple M1 (Virtual)"; > > These casts are unsafe; these pointers will be freed when this device is > freed. > Good catch! The more usual pattern for default string property values seems to be to fill them in _realize() (using g_strdup()) if no other value was previously set, so I've applied that here for the next version of the patch. > > > +} > > + > > +static Property vmapple_cfg_properties[] = { > > + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, cfg.nr_cpus, 1), > > + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), > > + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, cfg.ram_size, 0), > > + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, > cfg.run_installer1, 0), > > + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, > cfg.run_installer2, 0), > > + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), > > + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, cfg.mac_en0), > > + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, cfg.mac_en1), > > + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, cfg.mac_wifi0), > > + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, cfg.mac_bt0), > > + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), > > + DEFINE_PROP_STRING("model", VMAppleCfgState, model), > > + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, soc_name), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void vmapple_cfg_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > + > > + dc->realize = vmapple_cfg_realize; > > + dc->desc = "VMApple Configuration Region"; > > + device_class_set_props(dc, vmapple_cfg_properties); > > + rc->phases.hold = vmapple_cfg_reset; > > +} > > + > > +static const TypeInfo vmapple_cfg_info = { > > + .name = TYPE_VMAPPLE_CFG, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(VMAppleCfgState), > > + .instance_init = vmapple_cfg_init, > > + .class_init = vmapple_cfg_class_init, > > +}; > > + > > +static void vmapple_cfg_register_types(void) > > +{ > > + type_register_static(&vmapple_cfg_info); > > +} > > + > > +type_init(vmapple_cfg_register_types) > > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build > > index d4624713deb..64b78693a31 100644 > > --- a/hw/vmapple/meson.build > > +++ b/hw/vmapple/meson.build > > @@ -1,2 +1,3 @@ > > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: files('aes.c')) > > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: files('bdif.c')) > > +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: files('cfg.c')) > > diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h > > new file mode 100644 > > index 00000000000..3337064e447 > > --- /dev/null > > +++ b/include/hw/vmapple/cfg.h > > @@ -0,0 +1,68 @@ > > +/* > > + * VMApple Configuration Region > > + * > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights > Reserved. > > + * > > + * 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 HW_VMAPPLE_CFG_H > > +#define HW_VMAPPLE_CFG_H > > + > > +#include "hw/sysbus.h" > > +#include "qom/object.h" > > +#include "net/net.h" > > + > > +typedef struct VMAppleCfg { > > + uint32_t version; /* 0x000 */ > > + uint32_t nr_cpus; /* 0x004 */ > > + uint32_t unk1; /* 0x008 */ > > + uint32_t unk2; /* 0x00c */ > > + uint32_t unk3; /* 0x010 */ > > + uint32_t unk4; /* 0x014 */ > > + uint64_t ecid; /* 0x018 */ > > + uint64_t ram_size; /* 0x020 */ > > + uint32_t run_installer1; /* 0x028 */ > > + uint32_t unk5; /* 0x02c */ > > + uint32_t unk6; /* 0x030 */ > > + uint32_t run_installer2; /* 0x034 */ > > + uint32_t rnd; /* 0x038 */ > > + uint32_t unk7; /* 0x03c */ > > + MACAddr mac_en0; /* 0x040 */ > > + uint8_t pad1[2]; > > + MACAddr mac_en1; /* 0x048 */ > > + uint8_t pad2[2]; > > + MACAddr mac_wifi0; /* 0x050 */ > > + uint8_t pad3[2]; > > + MACAddr mac_bt0; /* 0x058 */ > > + uint8_t pad4[2]; > > + uint8_t reserved[0xa0]; /* 0x060 */ > > + uint32_t cpu_ids[0x80]; /* 0x100 */ > > + uint8_t scratch[0x200]; /* 0x180 */ > > + char serial[32]; /* 0x380 */ > > + char unk8[32]; /* 0x3a0 */ > > + char model[32]; /* 0x3c0 */ > > + uint8_t unk9[32]; /* 0x3e0 */ > > + uint32_t unk10; /* 0x400 */ > > + char soc_name[32]; /* 0x404 */ > > +} VMAppleCfg; > > + > > +#define TYPE_VMAPPLE_CFG "vmapple-cfg" > > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) > > + > > +struct VMAppleCfgState { > > + /* <private> */ > > + SysBusDevice parent_obj; > > + VMAppleCfg cfg; > > + > > + /* <public> */ > > + MemoryRegion mem; > > + char *serial; > > + char *model; > > + char *soc_name; > > +}; > > + > > +#define VMAPPLE_CFG_SIZE 0x00010000 > > + > > +#endif /* HW_VMAPPLE_CFG_H */ > >
On 2024/10/07 23:10, Phil Dennis-Jordan wrote: > > > On Sat, 5 Oct 2024 at 07:35, Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2024/09/28 17:57, Phil Dennis-Jordan wrote: > > From: Alexander Graf <graf@amazon.com <mailto:graf@amazon.com>> > > > > Instead of device tree or other more standardized means, VMApple > passes > > platform configuration to the first stage boot loader in a binary > encoded > > format that resides at a dedicated RAM region in physical address > space. > > > > This patch models this configuration space as a qdev device which > we can > > then map at the fixed location in the address space. That way, we can > > influence and annotate all configuration fields easily. > > > > Signed-off-by: Alexander Graf <graf@amazon.com > <mailto:graf@amazon.com>> > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu > <mailto:phil@philjordan.eu>> > > > > --- > > v3: > > > > * Replaced legacy device reset method with Resettable method > > > > hw/vmapple/Kconfig | 3 ++ > > hw/vmapple/cfg.c | 106 ++++++++++++++++++++++++++++++++ > +++++++ > > hw/vmapple/meson.build | 1 + > > include/hw/vmapple/cfg.h | 68 +++++++++++++++++++++++++ > > 4 files changed, 178 insertions(+) > > create mode 100644 hw/vmapple/cfg.c > > create mode 100644 include/hw/vmapple/cfg.h > > > > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig > > index 68f88876eb9..8bbeb9a9237 100644 > > --- a/hw/vmapple/Kconfig > > +++ b/hw/vmapple/Kconfig > > @@ -4,3 +4,6 @@ config VMAPPLE_AES > > config VMAPPLE_BDIF > > bool > > > > +config VMAPPLE_CFG > > + bool > > + > > diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c > > new file mode 100644 > > index 00000000000..a5e5c62f59f > > --- /dev/null > > +++ b/hw/vmapple/cfg.c > > @@ -0,0 +1,106 @@ > > +/* > > + * VMApple Configuration Region > > + * > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > Rights Reserved. > > + * > > + * 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/vmapple/cfg.h" > > +#include "qemu/log.h" > > +#include "qemu/module.h" > > +#include "qapi/error.h" > > + > > +static void vmapple_cfg_reset(Object *obj, ResetType type) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > + VMAppleCfg *cfg; > > + > > + cfg = memory_region_get_ram_ptr(&s->mem); > > + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); > > + *cfg = s->cfg; > > +}> + > > +static void vmapple_cfg_realize(DeviceState *dev, Error **errp) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(dev); > > + uint32_t i; > > + > > + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); > > + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); > > + strncpy(s->cfg.soc_name, s->soc_name, sizeof(s->cfg.soc_name)); > > + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); > > Use qemu_strnlen() to report an error for too long strings. > > > Hmm, I don't see any existing instances of such a pattern. I do however > see a couple of uses of g_strlcpy in the Qemu codebase - that would be a > better candidate for error checked string copying, though it still > involves some awkward return value checks. I'm going to wrap that in a > helper function and macro to replace all 4 strncpy instances here. If > the same thing is useful elsewhere later, it can be promoted to cutils > or similar. g_strlcpy() internally performs strlen(), which is worse than qemu_strnlen(). It is nice to have a helper function. Linux also has something similar called strscpy(): https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.strscpy > > (Also, I notice that last strncpy actually uses the wrong destination > size; my wrapper macro uses ARRAY_SIZE to avoid this mistake altogether.) > > > + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); > > + s->cfg.version = 2; > > + s->cfg.unk1 = 1; > > + s->cfg.unk2 = 1; > > + s->cfg.unk3 = 0x20; > > + s->cfg.unk4 = 0; > > + s->cfg.unk5 = 1; > > + s->cfg.unk6 = 1; > > + s->cfg.unk7 = 0; > > + s->cfg.unk10 = 1; > > + > > + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); > > Report an error instead of asserting. > > > + for (i = 0; i < s->cfg.nr_cpus; i++) { > > + s->cfg.cpu_ids[i] = i; > > + } > > +}> + > > +static void vmapple_cfg_init(Object *obj) > > +{ > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > + > > + memory_region_init_ram(&s->mem, obj, "VMApple Config", > VMAPPLE_CFG_SIZE, > > + &error_fatal); > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); > > + > > + s->serial = (char *)"1234"; > > + s->model = (char *)"VM0001"; > > + s->soc_name = (char *)"Apple M1 (Virtual)"; > > These casts are unsafe; these pointers will be freed when this > device is > freed. > > > Good catch! The more usual pattern for default string property values > seems to be to fill them in _realize() (using g_strdup()) if no other > value was previously set, so I've applied that here for the next version > of the patch. > > > > +} > > + > > +static Property vmapple_cfg_properties[] = { > > + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, cfg.nr_cpus, 1), > > + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), > > + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, > cfg.ram_size, 0), > > + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, > cfg.run_installer1, 0), > > + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, > cfg.run_installer2, 0), > > + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), > > + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, cfg.mac_en0), > > + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, cfg.mac_en1), > > + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, > cfg.mac_wifi0), > > + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, cfg.mac_bt0), > > + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), > > + DEFINE_PROP_STRING("model", VMAppleCfgState, model), > > + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, soc_name), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void vmapple_cfg_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > + > > + dc->realize = vmapple_cfg_realize; > > + dc->desc = "VMApple Configuration Region"; > > + device_class_set_props(dc, vmapple_cfg_properties); > > + rc->phases.hold = vmapple_cfg_reset; > > +} > > + > > +static const TypeInfo vmapple_cfg_info = { > > + .name = TYPE_VMAPPLE_CFG, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(VMAppleCfgState), > > + .instance_init = vmapple_cfg_init, > > + .class_init = vmapple_cfg_class_init, > > +}; > > + > > +static void vmapple_cfg_register_types(void) > > +{ > > + type_register_static(&vmapple_cfg_info); > > +} > > + > > +type_init(vmapple_cfg_register_types) > > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build > > index d4624713deb..64b78693a31 100644 > > --- a/hw/vmapple/meson.build > > +++ b/hw/vmapple/meson.build > > @@ -1,2 +1,3 @@ > > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: files('aes.c')) > > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: > files('bdif.c')) > > +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: files('cfg.c')) > > diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h > > new file mode 100644 > > index 00000000000..3337064e447 > > --- /dev/null > > +++ b/include/hw/vmapple/cfg.h > > @@ -0,0 +1,68 @@ > > +/* > > + * VMApple Configuration Region > > + * > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > Rights Reserved. > > + * > > + * 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 HW_VMAPPLE_CFG_H > > +#define HW_VMAPPLE_CFG_H > > + > > +#include "hw/sysbus.h" > > +#include "qom/object.h" > > +#include "net/net.h" > > + > > +typedef struct VMAppleCfg { > > + uint32_t version; /* 0x000 */ > > + uint32_t nr_cpus; /* 0x004 */ > > + uint32_t unk1; /* 0x008 */ > > + uint32_t unk2; /* 0x00c */ > > + uint32_t unk3; /* 0x010 */ > > + uint32_t unk4; /* 0x014 */ > > + uint64_t ecid; /* 0x018 */ > > + uint64_t ram_size; /* 0x020 */ > > + uint32_t run_installer1; /* 0x028 */ > > + uint32_t unk5; /* 0x02c */ > > + uint32_t unk6; /* 0x030 */ > > + uint32_t run_installer2; /* 0x034 */ > > + uint32_t rnd; /* 0x038 */ > > + uint32_t unk7; /* 0x03c */ > > + MACAddr mac_en0; /* 0x040 */ > > + uint8_t pad1[2]; > > + MACAddr mac_en1; /* 0x048 */ > > + uint8_t pad2[2]; > > + MACAddr mac_wifi0; /* 0x050 */ > > + uint8_t pad3[2]; > > + MACAddr mac_bt0; /* 0x058 */ > > + uint8_t pad4[2]; > > + uint8_t reserved[0xa0]; /* 0x060 */ > > + uint32_t cpu_ids[0x80]; /* 0x100 */ > > + uint8_t scratch[0x200]; /* 0x180 */ > > + char serial[32]; /* 0x380 */ > > + char unk8[32]; /* 0x3a0 */ > > + char model[32]; /* 0x3c0 */ > > + uint8_t unk9[32]; /* 0x3e0 */ > > + uint32_t unk10; /* 0x400 */ > > + char soc_name[32]; /* 0x404 */ > > +} VMAppleCfg; > > + > > +#define TYPE_VMAPPLE_CFG "vmapple-cfg" > > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) > > + > > +struct VMAppleCfgState { > > + /* <private> */ > > + SysBusDevice parent_obj; > > + VMAppleCfg cfg; > > + > > + /* <public> */ > > + MemoryRegion mem; > > + char *serial; > > + char *model; > > + char *soc_name; > > +}; > > + > > +#define VMAPPLE_CFG_SIZE 0x00010000 > > + > > +#endif /* HW_VMAPPLE_CFG_H */ >
On Mon, 7 Oct 2024 at 20:04, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2024/10/07 23:10, Phil Dennis-Jordan wrote: > > > > > > On Sat, 5 Oct 2024 at 07:35, Akihiko Odaki <akihiko.odaki@daynix.com > > <mailto:akihiko.odaki@daynix.com>> wrote: > > > > On 2024/09/28 17:57, Phil Dennis-Jordan wrote: > > > From: Alexander Graf <graf@amazon.com <mailto:graf@amazon.com>> > > > > > > Instead of device tree or other more standardized means, VMApple > > passes > > > platform configuration to the first stage boot loader in a binary > > encoded > > > format that resides at a dedicated RAM region in physical address > > space. > > > > > > This patch models this configuration space as a qdev device which > > we can > > > then map at the fixed location in the address space. That way, we > can > > > influence and annotate all configuration fields easily. > > > > > > Signed-off-by: Alexander Graf <graf@amazon.com > > <mailto:graf@amazon.com>> > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu > > <mailto:phil@philjordan.eu>> > > > > > > --- > > > v3: > > > > > > * Replaced legacy device reset method with Resettable method > > > > > > hw/vmapple/Kconfig | 3 ++ > > > hw/vmapple/cfg.c | 106 ++++++++++++++++++++++++++++++++ > > +++++++ > > > hw/vmapple/meson.build | 1 + > > > include/hw/vmapple/cfg.h | 68 +++++++++++++++++++++++++ > > > 4 files changed, 178 insertions(+) > > > create mode 100644 hw/vmapple/cfg.c > > > create mode 100644 include/hw/vmapple/cfg.h > > > > > > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig > > > index 68f88876eb9..8bbeb9a9237 100644 > > > --- a/hw/vmapple/Kconfig > > > +++ b/hw/vmapple/Kconfig > > > @@ -4,3 +4,6 @@ config VMAPPLE_AES > > > config VMAPPLE_BDIF > > > bool > > > > > > +config VMAPPLE_CFG > > > + bool > > > + > > > diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c > > > new file mode 100644 > > > index 00000000000..a5e5c62f59f > > > --- /dev/null > > > +++ b/hw/vmapple/cfg.c > > > @@ -0,0 +1,106 @@ > > > +/* > > > + * VMApple Configuration Region > > > + * > > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > > Rights Reserved. > > > + * > > > + * 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/vmapple/cfg.h" > > > +#include "qemu/log.h" > > > +#include "qemu/module.h" > > > +#include "qapi/error.h" > > > + > > > +static void vmapple_cfg_reset(Object *obj, ResetType type) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > > + VMAppleCfg *cfg; > > > + > > > + cfg = memory_region_get_ram_ptr(&s->mem); > > > + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); > > > + *cfg = s->cfg; > > > +}> + > > > +static void vmapple_cfg_realize(DeviceState *dev, Error **errp) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(dev); > > > + uint32_t i; > > > + > > > + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); > > > + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); > > > + strncpy(s->cfg.soc_name, s->soc_name, > sizeof(s->cfg.soc_name)); > > > + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); > > > > Use qemu_strnlen() to report an error for too long strings. > > > > > > Hmm, I don't see any existing instances of such a pattern. I do however > > see a couple of uses of g_strlcpy in the Qemu codebase - that would be a > > better candidate for error checked string copying, though it still > > involves some awkward return value checks. I'm going to wrap that in a > > helper function and macro to replace all 4 strncpy instances here. If > > the same thing is useful elsewhere later, it can be promoted to cutils > > or similar. > > g_strlcpy() internally performs strlen(), which is worse than > qemu_strnlen(). > Worse in what sense? It really depends what you're defending against. Sure, strlcpy blows up if the source string isn't nul-terminated. All the source strings here are expected to be nul-terminated here though. > It is nice to have a helper function. Linux also has something similar > called strscpy(): > https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.strscpy > I'm not convinced this is the right patch set to be reinventing Qemu's string copying functions. Instances abound of the even less safe strcpy() (and sprintf, etc.) being used in Qemu. Some of those are likely even problematic, but it seems that should be subject to a more holistic investigation into what kinds of usage patterns there are, and then a handful of safe solutions can be found that would work for 99% of string copying in the code base. Not by adding yet another strcpy variant in a device backend and machine type patch set where one of the existing variants works just fine. > > > > (Also, I notice that last strncpy actually uses the wrong destination > > size; my wrapper macro uses ARRAY_SIZE to avoid this mistake altogether.) > > > > > + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); > > > + s->cfg.version = 2; > > > + s->cfg.unk1 = 1; > > > + s->cfg.unk2 = 1; > > > + s->cfg.unk3 = 0x20; > > > + s->cfg.unk4 = 0; > > > + s->cfg.unk5 = 1; > > > + s->cfg.unk6 = 1; > > > + s->cfg.unk7 = 0; > > > + s->cfg.unk10 = 1; > > > + > > > + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); > > > > Report an error instead of asserting. > > > > > + for (i = 0; i < s->cfg.nr_cpus; i++) { > > > + s->cfg.cpu_ids[i] = i; > > > + } > > > +}> + > > > +static void vmapple_cfg_init(Object *obj) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > > + > > > + memory_region_init_ram(&s->mem, obj, "VMApple Config", > > VMAPPLE_CFG_SIZE, > > > + &error_fatal); > > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); > > > + > > > + s->serial = (char *)"1234"; > > > + s->model = (char *)"VM0001"; > > > + s->soc_name = (char *)"Apple M1 (Virtual)"; > > > > These casts are unsafe; these pointers will be freed when this > > device is > > freed. > > > > > > Good catch! The more usual pattern for default string property values > > seems to be to fill them in _realize() (using g_strdup()) if no other > > value was previously set, so I've applied that here for the next version > > of the patch. > > > > > > > +} > > > + > > > +static Property vmapple_cfg_properties[] = { > > > + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, cfg.nr_cpus, > 1), > > > + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), > > > + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, > > cfg.ram_size, 0), > > > + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, > > cfg.run_installer1, 0), > > > + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, > > cfg.run_installer2, 0), > > > + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), > > > + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, cfg.mac_en0), > > > + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, cfg.mac_en1), > > > + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, > > cfg.mac_wifi0), > > > + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, cfg.mac_bt0), > > > + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), > > > + DEFINE_PROP_STRING("model", VMAppleCfgState, model), > > > + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, soc_name), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > +static void vmapple_cfg_class_init(ObjectClass *klass, void > *data) > > > +{ > > > + DeviceClass *dc = DEVICE_CLASS(klass); > > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > > + > > > + dc->realize = vmapple_cfg_realize; > > > + dc->desc = "VMApple Configuration Region"; > > > + device_class_set_props(dc, vmapple_cfg_properties); > > > + rc->phases.hold = vmapple_cfg_reset; > > > +} > > > + > > > +static const TypeInfo vmapple_cfg_info = { > > > + .name = TYPE_VMAPPLE_CFG, > > > + .parent = TYPE_SYS_BUS_DEVICE, > > > + .instance_size = sizeof(VMAppleCfgState), > > > + .instance_init = vmapple_cfg_init, > > > + .class_init = vmapple_cfg_class_init, > > > +}; > > > + > > > +static void vmapple_cfg_register_types(void) > > > +{ > > > + type_register_static(&vmapple_cfg_info); > > > +} > > > + > > > +type_init(vmapple_cfg_register_types) > > > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build > > > index d4624713deb..64b78693a31 100644 > > > --- a/hw/vmapple/meson.build > > > +++ b/hw/vmapple/meson.build > > > @@ -1,2 +1,3 @@ > > > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: > files('aes.c')) > > > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: > > files('bdif.c')) > > > +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: > files('cfg.c')) > > > diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h > > > new file mode 100644 > > > index 00000000000..3337064e447 > > > --- /dev/null > > > +++ b/include/hw/vmapple/cfg.h > > > @@ -0,0 +1,68 @@ > > > +/* > > > + * VMApple Configuration Region > > > + * > > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > > Rights Reserved. > > > + * > > > + * 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 HW_VMAPPLE_CFG_H > > > +#define HW_VMAPPLE_CFG_H > > > + > > > +#include "hw/sysbus.h" > > > +#include "qom/object.h" > > > +#include "net/net.h" > > > + > > > +typedef struct VMAppleCfg { > > > + uint32_t version; /* 0x000 */ > > > + uint32_t nr_cpus; /* 0x004 */ > > > + uint32_t unk1; /* 0x008 */ > > > + uint32_t unk2; /* 0x00c */ > > > + uint32_t unk3; /* 0x010 */ > > > + uint32_t unk4; /* 0x014 */ > > > + uint64_t ecid; /* 0x018 */ > > > + uint64_t ram_size; /* 0x020 */ > > > + uint32_t run_installer1; /* 0x028 */ > > > + uint32_t unk5; /* 0x02c */ > > > + uint32_t unk6; /* 0x030 */ > > > + uint32_t run_installer2; /* 0x034 */ > > > + uint32_t rnd; /* 0x038 */ > > > + uint32_t unk7; /* 0x03c */ > > > + MACAddr mac_en0; /* 0x040 */ > > > + uint8_t pad1[2]; > > > + MACAddr mac_en1; /* 0x048 */ > > > + uint8_t pad2[2]; > > > + MACAddr mac_wifi0; /* 0x050 */ > > > + uint8_t pad3[2]; > > > + MACAddr mac_bt0; /* 0x058 */ > > > + uint8_t pad4[2]; > > > + uint8_t reserved[0xa0]; /* 0x060 */ > > > + uint32_t cpu_ids[0x80]; /* 0x100 */ > > > + uint8_t scratch[0x200]; /* 0x180 */ > > > + char serial[32]; /* 0x380 */ > > > + char unk8[32]; /* 0x3a0 */ > > > + char model[32]; /* 0x3c0 */ > > > + uint8_t unk9[32]; /* 0x3e0 */ > > > + uint32_t unk10; /* 0x400 */ > > > + char soc_name[32]; /* 0x404 */ > > > +} VMAppleCfg; > > > + > > > +#define TYPE_VMAPPLE_CFG "vmapple-cfg" > > > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) > > > + > > > +struct VMAppleCfgState { > > > + /* <private> */ > > > + SysBusDevice parent_obj; > > > + VMAppleCfg cfg; > > > + > > > + /* <public> */ > > > + MemoryRegion mem; > > > + char *serial; > > > + char *model; > > > + char *soc_name; > > > +}; > > > + > > > +#define VMAPPLE_CFG_SIZE 0x00010000 > > > + > > > +#endif /* HW_VMAPPLE_CFG_H */ > > > >
On 2024/10/09 22:08, Phil Dennis-Jordan wrote: > > > On Mon, 7 Oct 2024 at 20:04, Akihiko Odaki <akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>> wrote: > > On 2024/10/07 23:10, Phil Dennis-Jordan wrote: > > > > > > On Sat, 5 Oct 2024 at 07:35, Akihiko Odaki > <akihiko.odaki@daynix.com <mailto:akihiko.odaki@daynix.com> > > <mailto:akihiko.odaki@daynix.com > <mailto:akihiko.odaki@daynix.com>>> wrote: > > > > On 2024/09/28 17:57, Phil Dennis-Jordan wrote: > > > From: Alexander Graf <graf@amazon.com > <mailto:graf@amazon.com> <mailto:graf@amazon.com > <mailto:graf@amazon.com>>> > > > > > > Instead of device tree or other more standardized means, > VMApple > > passes > > > platform configuration to the first stage boot loader in a > binary > > encoded > > > format that resides at a dedicated RAM region in physical > address > > space. > > > > > > This patch models this configuration space as a qdev > device which > > we can > > > then map at the fixed location in the address space. That > way, we can > > > influence and annotate all configuration fields easily. > > > > > > Signed-off-by: Alexander Graf <graf@amazon.com > <mailto:graf@amazon.com> > > <mailto:graf@amazon.com <mailto:graf@amazon.com>>> > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu > <mailto:phil@philjordan.eu> > > <mailto:phil@philjordan.eu <mailto:phil@philjordan.eu>>> > > > > > > --- > > > v3: > > > > > > * Replaced legacy device reset method with Resettable method > > > > > > hw/vmapple/Kconfig | 3 ++ > > > hw/vmapple/cfg.c | 106 +++++++++++++++++++++++++ > +++++++ > > +++++++ > > > hw/vmapple/meson.build | 1 + > > > include/hw/vmapple/cfg.h | 68 +++++++++++++++++++++++++ > > > 4 files changed, 178 insertions(+) > > > create mode 100644 hw/vmapple/cfg.c > > > create mode 100644 include/hw/vmapple/cfg.h > > > > > > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig > > > index 68f88876eb9..8bbeb9a9237 100644 > > > --- a/hw/vmapple/Kconfig > > > +++ b/hw/vmapple/Kconfig > > > @@ -4,3 +4,6 @@ config VMAPPLE_AES > > > config VMAPPLE_BDIF > > > bool > > > > > > +config VMAPPLE_CFG > > > + bool > > > + > > > diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c > > > new file mode 100644 > > > index 00000000000..a5e5c62f59f > > > --- /dev/null > > > +++ b/hw/vmapple/cfg.c > > > @@ -0,0 +1,106 @@ > > > +/* > > > + * VMApple Configuration Region > > > + * > > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > > Rights Reserved. > > > + * > > > + * 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/vmapple/cfg.h" > > > +#include "qemu/log.h" > > > +#include "qemu/module.h" > > > +#include "qapi/error.h" > > > + > > > +static void vmapple_cfg_reset(Object *obj, ResetType type) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > > + VMAppleCfg *cfg; > > > + > > > + cfg = memory_region_get_ram_ptr(&s->mem); > > > + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); > > > + *cfg = s->cfg; > > > +}> + > > > +static void vmapple_cfg_realize(DeviceState *dev, Error > **errp) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(dev); > > > + uint32_t i; > > > + > > > + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); > > > + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); > > > + strncpy(s->cfg.soc_name, s->soc_name, sizeof(s- > >cfg.soc_name)); > > > + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); > > > > Use qemu_strnlen() to report an error for too long strings. > > > > > > Hmm, I don't see any existing instances of such a pattern. I do > however > > see a couple of uses of g_strlcpy in the Qemu codebase - that > would be a > > better candidate for error checked string copying, though it still > > involves some awkward return value checks. I'm going to wrap that > in a > > helper function and macro to replace all 4 strncpy instances > here. If > > the same thing is useful elsewhere later, it can be promoted to > cutils > > or similar. > > g_strlcpy() internally performs strlen(), which is worse than > qemu_strnlen(). > > > Worse in what sense? It really depends what you're defending against. > Sure, strlcpy blows up if the source string isn't nul-terminated. All > the source strings here are expected to be nul-terminated here though. > > It is nice to have a helper function. Linux also has something similar > called strscpy(): > https://www.kernel.org/doc/html/latest/core-api/kernel- > api.html#c.strscpy <https://www.kernel.org/doc/html/latest/core-api/ > kernel-api.html#c.strscpy> > > > I'm not convinced this is the right patch set to be reinventing Qemu's > string copying functions. Instances abound of the even less safe > strcpy() (and sprintf, etc.) being used in Qemu. Some of those are > likely even problematic, but it seems that should be subject to a more > holistic investigation into what kinds of usage patterns there are, and > then a handful of safe solutions can be found that would work for 99% of > string copying in the code base. Not by adding yet another strcpy > variant in a device backend and machine type patch set where one of the > existing variants works just fine. My suggestion were for the case you were going to write one to avoid cumbersome return value checks as you told in the earlier email. It is fine if your helper function is to be specific to this device to add device-specific error reporting code, for example. > > > > > (Also, I notice that last strncpy actually uses the wrong > destination > > size; my wrapper macro uses ARRAY_SIZE to avoid this mistake > altogether.) > > > > > + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); > > > + s->cfg.version = 2; > > > + s->cfg.unk1 = 1; > > > + s->cfg.unk2 = 1; > > > + s->cfg.unk3 = 0x20; > > > + s->cfg.unk4 = 0; > > > + s->cfg.unk5 = 1; > > > + s->cfg.unk6 = 1; > > > + s->cfg.unk7 = 0; > > > + s->cfg.unk10 = 1; > > > + > > > + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); > > > > Report an error instead of asserting. > > > > > + for (i = 0; i < s->cfg.nr_cpus; i++) { > > > + s->cfg.cpu_ids[i] = i; > > > + } > > > +}> + > > > +static void vmapple_cfg_init(Object *obj) > > > +{ > > > + VMAppleCfgState *s = VMAPPLE_CFG(obj); > > > + > > > + memory_region_init_ram(&s->mem, obj, "VMApple Config", > > VMAPPLE_CFG_SIZE, > > > + &error_fatal); > > > + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); > > > + > > > + s->serial = (char *)"1234"; > > > + s->model = (char *)"VM0001"; > > > + s->soc_name = (char *)"Apple M1 (Virtual)"; > > > > These casts are unsafe; these pointers will be freed when this > > device is > > freed. > > > > > > Good catch! The more usual pattern for default string property > values > > seems to be to fill them in _realize() (using g_strdup()) if no > other > > value was previously set, so I've applied that here for the next > version > > of the patch. > > > > > > > +} > > > + > > > +static Property vmapple_cfg_properties[] = { > > > + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, > cfg.nr_cpus, 1), > > > + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), > > > + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, > > cfg.ram_size, 0), > > > + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, > > cfg.run_installer1, 0), > > > + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, > > cfg.run_installer2, 0), > > > + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), > > > + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, > cfg.mac_en0), > > > + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, > cfg.mac_en1), > > > + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, > > cfg.mac_wifi0), > > > + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, > cfg.mac_bt0), > > > + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), > > > + DEFINE_PROP_STRING("model", VMAppleCfgState, model), > > > + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, > soc_name), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > +static void vmapple_cfg_class_init(ObjectClass *klass, > void *data) > > > +{ > > > + DeviceClass *dc = DEVICE_CLASS(klass); > > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > > + > > > + dc->realize = vmapple_cfg_realize; > > > + dc->desc = "VMApple Configuration Region"; > > > + device_class_set_props(dc, vmapple_cfg_properties); > > > + rc->phases.hold = vmapple_cfg_reset; > > > +} > > > + > > > +static const TypeInfo vmapple_cfg_info = { > > > + .name = TYPE_VMAPPLE_CFG, > > > + .parent = TYPE_SYS_BUS_DEVICE, > > > + .instance_size = sizeof(VMAppleCfgState), > > > + .instance_init = vmapple_cfg_init, > > > + .class_init = vmapple_cfg_class_init, > > > +}; > > > + > > > +static void vmapple_cfg_register_types(void) > > > +{ > > > + type_register_static(&vmapple_cfg_info); > > > +} > > > + > > > +type_init(vmapple_cfg_register_types) > > > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build > > > index d4624713deb..64b78693a31 100644 > > > --- a/hw/vmapple/meson.build > > > +++ b/hw/vmapple/meson.build > > > @@ -1,2 +1,3 @@ > > > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: > files('aes.c')) > > > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: > > files('bdif.c')) > > > +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: > files('cfg.c')) > > > diff --git a/include/hw/vmapple/cfg.h b/include/hw/ > vmapple/cfg.h > > > new file mode 100644 > > > index 00000000000..3337064e447 > > > --- /dev/null > > > +++ b/include/hw/vmapple/cfg.h > > > @@ -0,0 +1,68 @@ > > > +/* > > > + * VMApple Configuration Region > > > + * > > > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All > > Rights Reserved. > > > + * > > > + * 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 HW_VMAPPLE_CFG_H > > > +#define HW_VMAPPLE_CFG_H > > > + > > > +#include "hw/sysbus.h" > > > +#include "qom/object.h" > > > +#include "net/net.h" > > > + > > > +typedef struct VMAppleCfg { > > > + uint32_t version; /* 0x000 */ > > > + uint32_t nr_cpus; /* 0x004 */ > > > + uint32_t unk1; /* 0x008 */ > > > + uint32_t unk2; /* 0x00c */ > > > + uint32_t unk3; /* 0x010 */ > > > + uint32_t unk4; /* 0x014 */ > > > + uint64_t ecid; /* 0x018 */ > > > + uint64_t ram_size; /* 0x020 */ > > > + uint32_t run_installer1; /* 0x028 */ > > > + uint32_t unk5; /* 0x02c */ > > > + uint32_t unk6; /* 0x030 */ > > > + uint32_t run_installer2; /* 0x034 */ > > > + uint32_t rnd; /* 0x038 */ > > > + uint32_t unk7; /* 0x03c */ > > > + MACAddr mac_en0; /* 0x040 */ > > > + uint8_t pad1[2]; > > > + MACAddr mac_en1; /* 0x048 */ > > > + uint8_t pad2[2]; > > > + MACAddr mac_wifi0; /* 0x050 */ > > > + uint8_t pad3[2]; > > > + MACAddr mac_bt0; /* 0x058 */ > > > + uint8_t pad4[2]; > > > + uint8_t reserved[0xa0]; /* 0x060 */ > > > + uint32_t cpu_ids[0x80]; /* 0x100 */ > > > + uint8_t scratch[0x200]; /* 0x180 */ > > > + char serial[32]; /* 0x380 */ > > > + char unk8[32]; /* 0x3a0 */ > > > + char model[32]; /* 0x3c0 */ > > > + uint8_t unk9[32]; /* 0x3e0 */ > > > + uint32_t unk10; /* 0x400 */ > > > + char soc_name[32]; /* 0x404 */ > > > +} VMAppleCfg; > > > + > > > +#define TYPE_VMAPPLE_CFG "vmapple-cfg" > > > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) > > > + > > > +struct VMAppleCfgState { > > > + /* <private> */ > > > + SysBusDevice parent_obj; > > > + VMAppleCfg cfg; > > > + > > > + /* <public> */ > > > + MemoryRegion mem; > > > + char *serial; > > > + char *model; > > > + char *soc_name; > > > +}; > > > + > > > +#define VMAPPLE_CFG_SIZE 0x00010000 > > > + > > > +#endif /* HW_VMAPPLE_CFG_H */ > > >
diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig index 68f88876eb9..8bbeb9a9237 100644 --- a/hw/vmapple/Kconfig +++ b/hw/vmapple/Kconfig @@ -4,3 +4,6 @@ config VMAPPLE_AES config VMAPPLE_BDIF bool +config VMAPPLE_CFG + bool + diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c new file mode 100644 index 00000000000..a5e5c62f59f --- /dev/null +++ b/hw/vmapple/cfg.c @@ -0,0 +1,106 @@ +/* + * VMApple Configuration Region + * + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * 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/vmapple/cfg.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qapi/error.h" + +static void vmapple_cfg_reset(Object *obj, ResetType type) +{ + VMAppleCfgState *s = VMAPPLE_CFG(obj); + VMAppleCfg *cfg; + + cfg = memory_region_get_ram_ptr(&s->mem); + memset((void *)cfg, 0, VMAPPLE_CFG_SIZE); + *cfg = s->cfg; +} + +static void vmapple_cfg_realize(DeviceState *dev, Error **errp) +{ + VMAppleCfgState *s = VMAPPLE_CFG(dev); + uint32_t i; + + strncpy(s->cfg.serial, s->serial, sizeof(s->cfg.serial)); + strncpy(s->cfg.model, s->model, sizeof(s->cfg.model)); + strncpy(s->cfg.soc_name, s->soc_name, sizeof(s->cfg.soc_name)); + strncpy(s->cfg.unk8, "D/A", sizeof(s->cfg.soc_name)); + s->cfg.ecid = cpu_to_be64(s->cfg.ecid); + s->cfg.version = 2; + s->cfg.unk1 = 1; + s->cfg.unk2 = 1; + s->cfg.unk3 = 0x20; + s->cfg.unk4 = 0; + s->cfg.unk5 = 1; + s->cfg.unk6 = 1; + s->cfg.unk7 = 0; + s->cfg.unk10 = 1; + + g_assert(s->cfg.nr_cpus < ARRAY_SIZE(s->cfg.cpu_ids)); + for (i = 0; i < s->cfg.nr_cpus; i++) { + s->cfg.cpu_ids[i] = i; + } +} + +static void vmapple_cfg_init(Object *obj) +{ + VMAppleCfgState *s = VMAPPLE_CFG(obj); + + memory_region_init_ram(&s->mem, obj, "VMApple Config", VMAPPLE_CFG_SIZE, + &error_fatal); + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mem); + + s->serial = (char *)"1234"; + s->model = (char *)"VM0001"; + s->soc_name = (char *)"Apple M1 (Virtual)"; +} + +static Property vmapple_cfg_properties[] = { + DEFINE_PROP_UINT32("nr-cpus", VMAppleCfgState, cfg.nr_cpus, 1), + DEFINE_PROP_UINT64("ecid", VMAppleCfgState, cfg.ecid, 0), + DEFINE_PROP_UINT64("ram-size", VMAppleCfgState, cfg.ram_size, 0), + DEFINE_PROP_UINT32("run_installer1", VMAppleCfgState, cfg.run_installer1, 0), + DEFINE_PROP_UINT32("run_installer2", VMAppleCfgState, cfg.run_installer2, 0), + DEFINE_PROP_UINT32("rnd", VMAppleCfgState, cfg.rnd, 0), + DEFINE_PROP_MACADDR("mac-en0", VMAppleCfgState, cfg.mac_en0), + DEFINE_PROP_MACADDR("mac-en1", VMAppleCfgState, cfg.mac_en1), + DEFINE_PROP_MACADDR("mac-wifi0", VMAppleCfgState, cfg.mac_wifi0), + DEFINE_PROP_MACADDR("mac-bt0", VMAppleCfgState, cfg.mac_bt0), + DEFINE_PROP_STRING("serial", VMAppleCfgState, serial), + DEFINE_PROP_STRING("model", VMAppleCfgState, model), + DEFINE_PROP_STRING("soc_name", VMAppleCfgState, soc_name), + DEFINE_PROP_END_OF_LIST(), +}; + +static void vmapple_cfg_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + ResettableClass *rc = RESETTABLE_CLASS(klass); + + dc->realize = vmapple_cfg_realize; + dc->desc = "VMApple Configuration Region"; + device_class_set_props(dc, vmapple_cfg_properties); + rc->phases.hold = vmapple_cfg_reset; +} + +static const TypeInfo vmapple_cfg_info = { + .name = TYPE_VMAPPLE_CFG, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(VMAppleCfgState), + .instance_init = vmapple_cfg_init, + .class_init = vmapple_cfg_class_init, +}; + +static void vmapple_cfg_register_types(void) +{ + type_register_static(&vmapple_cfg_info); +} + +type_init(vmapple_cfg_register_types) diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build index d4624713deb..64b78693a31 100644 --- a/hw/vmapple/meson.build +++ b/hw/vmapple/meson.build @@ -1,2 +1,3 @@ system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: files('aes.c')) system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: files('bdif.c')) +system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: files('cfg.c')) diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h new file mode 100644 index 00000000000..3337064e447 --- /dev/null +++ b/include/hw/vmapple/cfg.h @@ -0,0 +1,68 @@ +/* + * VMApple Configuration Region + * + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * 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 HW_VMAPPLE_CFG_H +#define HW_VMAPPLE_CFG_H + +#include "hw/sysbus.h" +#include "qom/object.h" +#include "net/net.h" + +typedef struct VMAppleCfg { + uint32_t version; /* 0x000 */ + uint32_t nr_cpus; /* 0x004 */ + uint32_t unk1; /* 0x008 */ + uint32_t unk2; /* 0x00c */ + uint32_t unk3; /* 0x010 */ + uint32_t unk4; /* 0x014 */ + uint64_t ecid; /* 0x018 */ + uint64_t ram_size; /* 0x020 */ + uint32_t run_installer1; /* 0x028 */ + uint32_t unk5; /* 0x02c */ + uint32_t unk6; /* 0x030 */ + uint32_t run_installer2; /* 0x034 */ + uint32_t rnd; /* 0x038 */ + uint32_t unk7; /* 0x03c */ + MACAddr mac_en0; /* 0x040 */ + uint8_t pad1[2]; + MACAddr mac_en1; /* 0x048 */ + uint8_t pad2[2]; + MACAddr mac_wifi0; /* 0x050 */ + uint8_t pad3[2]; + MACAddr mac_bt0; /* 0x058 */ + uint8_t pad4[2]; + uint8_t reserved[0xa0]; /* 0x060 */ + uint32_t cpu_ids[0x80]; /* 0x100 */ + uint8_t scratch[0x200]; /* 0x180 */ + char serial[32]; /* 0x380 */ + char unk8[32]; /* 0x3a0 */ + char model[32]; /* 0x3c0 */ + uint8_t unk9[32]; /* 0x3e0 */ + uint32_t unk10; /* 0x400 */ + char soc_name[32]; /* 0x404 */ +} VMAppleCfg; + +#define TYPE_VMAPPLE_CFG "vmapple-cfg" +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG) + +struct VMAppleCfgState { + /* <private> */ + SysBusDevice parent_obj; + VMAppleCfg cfg; + + /* <public> */ + MemoryRegion mem; + char *serial; + char *model; + char *soc_name; +}; + +#define VMAPPLE_CFG_SIZE 0x00010000 + +#endif /* HW_VMAPPLE_CFG_H */