On Sat, 5 Oct 2024 at 07:35, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
> On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> > From: Alexander Graf <g...@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 <g...@amazon.com>
> > Signed-off-by: Phil Dennis-Jordan <p...@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 */
>
>
