Re: [PATCH v3 12/14] hw/vmapple/cfg: Introduce vmapple cfg region

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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 */
>