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

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

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