diff mbox series

[v8,12/15] hw/vmapple/cfg: Introduce vmapple cfg region

Message ID 20241108144709.95498-13-phil@philjordan.eu (mailing list archive)
State New
Headers show
Series macOS PV Graphics and new vmapple machine type | expand

Commit Message

Phil Dennis-Jordan Nov. 8, 2024, 2:47 p.m. UTC 
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

v4:

 * Fixed initialisation of default values for properties
 * Dropped superfluous endianness conversions
 * Moved most header code to .c, device name #define goes in vmapple.h

v5:

 * Improved error reporting in case of string property buffer overflow.

v7:

 * Changed error messages for overrun of properties with
   fixed-length strings to be more useful to users than developers.

v8:

 * Consistent parenthesising of macro arguments for better safety.

 hw/vmapple/Kconfig           |   3 +
 hw/vmapple/cfg.c             | 196 +++++++++++++++++++++++++++++++++++
 hw/vmapple/meson.build       |   1 +
 include/hw/vmapple/vmapple.h |   2 +
 4 files changed, 202 insertions(+)
 create mode 100644 hw/vmapple/cfg.c

Comments

Akihiko Odaki Nov. 10, 2024, 7:14 a.m. UTC | #1 
On 2024/11/08 23:47, 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
> 
> v4:
> 
>   * Fixed initialisation of default values for properties
>   * Dropped superfluous endianness conversions
>   * Moved most header code to .c, device name #define goes in vmapple.h
> 
> v5:
> 
>   * Improved error reporting in case of string property buffer overflow.
> 
> v7:
> 
>   * Changed error messages for overrun of properties with
>     fixed-length strings to be more useful to users than developers.
> 
> v8:
> 
>   * Consistent parenthesising of macro arguments for better safety.
> 
>   hw/vmapple/Kconfig           |   3 +
>   hw/vmapple/cfg.c             | 196 +++++++++++++++++++++++++++++++++++
>   hw/vmapple/meson.build       |   1 +
>   include/hw/vmapple/vmapple.h |   2 +
>   4 files changed, 202 insertions(+)
>   create mode 100644 hw/vmapple/cfg.c
> 
> 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..787e2505d57
> --- /dev/null
> +++ b/hw/vmapple/cfg.c
> @@ -0,0 +1,196 @@
> +/*
> + * VMApple Configuration Region
> + *
> + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/vmapple.h"
> +#include "hw/sysbus.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "net/net.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
> +
> +#define VMAPPLE_CFG_SIZE 0x00010000
> +
> +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;
> +
> +struct VMAppleCfgState {
> +    SysBusDevice parent_obj;
> +    VMAppleCfg cfg;
> +
> +    MemoryRegion mem;
> +    char *serial;
> +    char *model;
> +    char *soc_name;
> +};
> +
> +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(cfg, 0, VMAPPLE_CFG_SIZE);
> +    *cfg = s->cfg;
> +}
> +
> +static bool set_fixlen_property_or_error(char *restrict dst,
> +                                         const char *restrict src,
> +                                         size_t dst_size, Error **errp,
> +                                         const char *property_name)
> +{
> +    size_t len;
> +
> +    len = g_strlcpy(dst, src, dst_size);
> +    if (len < dst_size) { /* len does not count nul terminator */
> +        return true;
> +    }
> +
> +    error_setg(errp,
> +               "Failed to set property '%s' on VMApple 'cfg' device: length "
> +               "(%zu) exceeds maximum of %zu",

Don't print the device name here as it will be automatically printed.

> +               property_name, len, dst_size - 1);
> +    return false;
> +}
> +
> +#define set_fixlen_property_or_return(dst_array, src, errp, property_name) \
> +    do { \
> +        if (!set_fixlen_property_or_error((dst_array), (src), \
> +                                          ARRAY_SIZE(dst_array), \
> +                                          (errp), (property_name))) { \
> +            return; \
> +        } \
> +    } while (0)
> +
> +static void vmapple_cfg_realize(DeviceState *dev, Error **errp)
> +{
> +    VMAppleCfgState *s = VMAPPLE_CFG(dev);
> +    uint32_t i;
> +
> +    if (!s->serial) {
> +        s->serial = g_strdup("1234");
> +    }
> +    if (!s->model) {
> +        s->model = g_strdup("VM0001");
> +    }
> +    if (!s->soc_name) {
> +        s->soc_name = g_strdup("Apple M1 (Virtual)");
> +    }
> +
> +    set_fixlen_property_or_return(s->cfg.serial, s->serial, errp, "serial");
> +    set_fixlen_property_or_return(s->cfg.model, s->model, errp, "model");
> +    set_fixlen_property_or_return(s->cfg.soc_name, s->soc_name, errp, "soc_name");
> +    set_fixlen_property_or_return(s->cfg.unk8, "D/A", errp, "unk8");
> +    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;
> +
> +    if (s->cfg.nr_cpus > ARRAY_SIZE(s->cfg.cpu_ids)) {
> +        error_setg(errp,
> +                   "Failed to create %u CPUs, vmapple machine supports %zu max",
> +                   s->cfg.nr_cpus, ARRAY_SIZE(s->cfg.cpu_ids));
> +        return;
> +    }
> +    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);
> +}
> +
> +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/vmapple.h b/include/hw/vmapple/vmapple.h
> index 9090e9c5ac8..3bba59f5ec7 100644
> --- a/include/hw/vmapple/vmapple.h
> +++ b/include/hw/vmapple/vmapple.h
> @@ -16,4 +16,6 @@
>   
>   #define TYPE_VMAPPLE_BDIF "vmapple-bdif"
>   
> +#define TYPE_VMAPPLE_CFG "vmapple-cfg"
> +
>   #endif /* HW_VMAPPLE_VMAPPLE_H */
Phil Dennis-Jordan Nov. 10, 2024, 3:01 p.m. UTC | #2 
On Sun, 10 Nov 2024 at 08:15, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/11/08 23:47, 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
> >
> > v4:
> >
> >   * Fixed initialisation of default values for properties
> >   * Dropped superfluous endianness conversions
> >   * Moved most header code to .c, device name #define goes in vmapple.h
> >
> > v5:
> >
> >   * Improved error reporting in case of string property buffer overflow.
> >
> > v7:
> >
> >   * Changed error messages for overrun of properties with
> >     fixed-length strings to be more useful to users than developers.
> >
> > v8:
> >
> >   * Consistent parenthesising of macro arguments for better safety.
> >
> >   hw/vmapple/Kconfig           |   3 +
> >   hw/vmapple/cfg.c             | 196 +++++++++++++++++++++++++++++++++++
> >   hw/vmapple/meson.build       |   1 +
> >   include/hw/vmapple/vmapple.h |   2 +
> >   4 files changed, 202 insertions(+)
> >   create mode 100644 hw/vmapple/cfg.c
> >
> > 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..787e2505d57
> > --- /dev/null
> > +++ b/hw/vmapple/cfg.c
> > @@ -0,0 +1,196 @@
> > +/*
> > + * VMApple Configuration Region
> > + *
> > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * 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/vmapple.h"
> > +#include "hw/sysbus.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "qapi/error.h"
> > +#include "net/net.h"
> > +
> > +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
> > +
> > +#define VMAPPLE_CFG_SIZE 0x00010000
> > +
> > +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;
> > +
> > +struct VMAppleCfgState {
> > +    SysBusDevice parent_obj;
> > +    VMAppleCfg cfg;
> > +
> > +    MemoryRegion mem;
> > +    char *serial;
> > +    char *model;
> > +    char *soc_name;
> > +};
> > +
> > +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(cfg, 0, VMAPPLE_CFG_SIZE);
> > +    *cfg = s->cfg;
> > +}
> > +
> > +static bool set_fixlen_property_or_error(char *restrict dst,
> > +                                         const char *restrict src,
> > +                                         size_t dst_size, Error **errp,
> > +                                         const char *property_name)
> > +{
> > +    size_t len;
> > +
> > +    len = g_strlcpy(dst, src, dst_size);
> > +    if (len < dst_size) { /* len does not count nul terminator */
> > +        return true;
> > +    }
> > +
> > +    error_setg(errp,
> > +               "Failed to set property '%s' on VMApple 'cfg' device: length "
> > +               "(%zu) exceeds maximum of %zu",
>
> Don't print the device name here as it will be automatically printed.

That's not what I'm seeing in tests. With code as-is, and setting an
overly long value from create_cfg() in the vmapple machine type
startup, I get the following output:

qemu-system-aarch64: Failed to set property 'soc_name' on VMApple
'cfg' device: length (50) exceeds maximum of 31

I don't see any redundant mention of an object or property name. Of
course, this error occurs during the cfg device's realize(), not
immediately when setting the property. I can't see a built-in way to
explicitly limit the length of a string property.

> > +               property_name, len, dst_size - 1);
> > +    return false;
> > +}
> > +
> > +#define set_fixlen_property_or_return(dst_array, src, errp, property_name) \
> > +    do { \
> > +        if (!set_fixlen_property_or_error((dst_array), (src), \
> > +                                          ARRAY_SIZE(dst_array), \
> > +                                          (errp), (property_name))) { \
> > +            return; \
> > +        } \
> > +    } while (0)
> > +
> > +static void vmapple_cfg_realize(DeviceState *dev, Error **errp)
> > +{
> > +    VMAppleCfgState *s = VMAPPLE_CFG(dev);
> > +    uint32_t i;
> > +
> > +    if (!s->serial) {
> > +        s->serial = g_strdup("1234");
> > +    }
> > +    if (!s->model) {
> > +        s->model = g_strdup("VM0001");
> > +    }
> > +    if (!s->soc_name) {
> > +        s->soc_name = g_strdup("Apple M1 (Virtual)");
> > +    }
> > +
> > +    set_fixlen_property_or_return(s->cfg.serial, s->serial, errp, "serial");
> > +    set_fixlen_property_or_return(s->cfg.model, s->model, errp, "model");
> > +    set_fixlen_property_or_return(s->cfg.soc_name, s->soc_name, errp, "soc_name");
> > +    set_fixlen_property_or_return(s->cfg.unk8, "D/A", errp, "unk8");
> > +    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;
> > +
> > +    if (s->cfg.nr_cpus > ARRAY_SIZE(s->cfg.cpu_ids)) {
> > +        error_setg(errp,
> > +                   "Failed to create %u CPUs, vmapple machine supports %zu max",
> > +                   s->cfg.nr_cpus, ARRAY_SIZE(s->cfg.cpu_ids));
> > +        return;
> > +    }
> > +    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);
> > +}
> > +
> > +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/vmapple.h b/include/hw/vmapple/vmapple.h
> > index 9090e9c5ac8..3bba59f5ec7 100644
> > --- a/include/hw/vmapple/vmapple.h
> > +++ b/include/hw/vmapple/vmapple.h
> > @@ -16,4 +16,6 @@
> >
> >   #define TYPE_VMAPPLE_BDIF "vmapple-bdif"
> >
> > +#define TYPE_VMAPPLE_CFG "vmapple-cfg"
> > +
> >   #endif /* HW_VMAPPLE_VMAPPLE_H */
>
>
Akihiko Odaki Nov. 11, 2024, 4:20 a.m. UTC | #3 
On 2024/11/11 0:01, Phil Dennis-Jordan wrote:
> On Sun, 10 Nov 2024 at 08:15, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/11/08 23:47, 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
>>>
>>> v4:
>>>
>>>    * Fixed initialisation of default values for properties
>>>    * Dropped superfluous endianness conversions
>>>    * Moved most header code to .c, device name #define goes in vmapple.h
>>>
>>> v5:
>>>
>>>    * Improved error reporting in case of string property buffer overflow.
>>>
>>> v7:
>>>
>>>    * Changed error messages for overrun of properties with
>>>      fixed-length strings to be more useful to users than developers.
>>>
>>> v8:
>>>
>>>    * Consistent parenthesising of macro arguments for better safety.
>>>
>>>    hw/vmapple/Kconfig           |   3 +
>>>    hw/vmapple/cfg.c             | 196 +++++++++++++++++++++++++++++++++++
>>>    hw/vmapple/meson.build       |   1 +
>>>    include/hw/vmapple/vmapple.h |   2 +
>>>    4 files changed, 202 insertions(+)
>>>    create mode 100644 hw/vmapple/cfg.c
>>>
>>> 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..787e2505d57
>>> --- /dev/null
>>> +++ b/hw/vmapple/cfg.c
>>> @@ -0,0 +1,196 @@
>>> +/*
>>> + * VMApple Configuration Region
>>> + *
>>> + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * 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/vmapple.h"
>>> +#include "hw/sysbus.h"
>>> +#include "qemu/log.h"
>>> +#include "qemu/module.h"
>>> +#include "qapi/error.h"
>>> +#include "net/net.h"
>>> +
>>> +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
>>> +
>>> +#define VMAPPLE_CFG_SIZE 0x00010000
>>> +
>>> +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;
>>> +
>>> +struct VMAppleCfgState {
>>> +    SysBusDevice parent_obj;
>>> +    VMAppleCfg cfg;
>>> +
>>> +    MemoryRegion mem;
>>> +    char *serial;
>>> +    char *model;
>>> +    char *soc_name;
>>> +};
>>> +
>>> +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(cfg, 0, VMAPPLE_CFG_SIZE);
>>> +    *cfg = s->cfg;
>>> +}
>>> +
>>> +static bool set_fixlen_property_or_error(char *restrict dst,
>>> +                                         const char *restrict src,
>>> +                                         size_t dst_size, Error **errp,
>>> +                                         const char *property_name)
>>> +{
>>> +    size_t len;
>>> +
>>> +    len = g_strlcpy(dst, src, dst_size);
>>> +    if (len < dst_size) { /* len does not count nul terminator */
>>> +        return true;
>>> +    }
>>> +
>>> +    error_setg(errp,
>>> +               "Failed to set property '%s' on VMApple 'cfg' device: length "
>>> +               "(%zu) exceeds maximum of %zu",
>>
>> Don't print the device name here as it will be automatically printed.
> 
> That's not what I'm seeing in tests. With code as-is, and setting an
> overly long value from create_cfg() in the vmapple machine type
> startup, I get the following output:
> 
> qemu-system-aarch64: Failed to set property 'soc_name' on VMApple
> 'cfg' device: length (50) exceeds maximum of 31
> 
> I don't see any redundant mention of an object or property name. Of
> course, this error occurs during the cfg device's realize(), not
> immediately when setting the property. I can't see a built-in way to
> explicitly limit the length of a string property.

I was thinking of the scenario where the device is added with the 
-device option. It seems the device name is not printed when it is 
realized by the machine, but in that case the user will not be aware of 
the 'cfg' device so it is not helpful. A proper way to add a context 
here is calling error_prepend() in the vmapple machine to tell that it 
is a failure in that machine type.

> 
>>> +               property_name, len, dst_size - 1);
>>> +    return false;
>>> +}
>>> +
>>> +#define set_fixlen_property_or_return(dst_array, src, errp, property_name) \
>>> +    do { \
>>> +        if (!set_fixlen_property_or_error((dst_array), (src), \
>>> +                                          ARRAY_SIZE(dst_array), \
>>> +                                          (errp), (property_name))) { \
>>> +            return; \
>>> +        } \
>>> +    } while (0)
>>> +
>>> +static void vmapple_cfg_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    VMAppleCfgState *s = VMAPPLE_CFG(dev);
>>> +    uint32_t i;
>>> +
>>> +    if (!s->serial) {
>>> +        s->serial = g_strdup("1234");
>>> +    }
>>> +    if (!s->model) {
>>> +        s->model = g_strdup("VM0001");
>>> +    }
>>> +    if (!s->soc_name) {
>>> +        s->soc_name = g_strdup("Apple M1 (Virtual)");
>>> +    }
>>> +
>>> +    set_fixlen_property_or_return(s->cfg.serial, s->serial, errp, "serial");
>>> +    set_fixlen_property_or_return(s->cfg.model, s->model, errp, "model");
>>> +    set_fixlen_property_or_return(s->cfg.soc_name, s->soc_name, errp, "soc_name");
>>> +    set_fixlen_property_or_return(s->cfg.unk8, "D/A", errp, "unk8");
>>> +    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;
>>> +
>>> +    if (s->cfg.nr_cpus > ARRAY_SIZE(s->cfg.cpu_ids)) {
>>> +        error_setg(errp,
>>> +                   "Failed to create %u CPUs, vmapple machine supports %zu max",
>>> +                   s->cfg.nr_cpus, ARRAY_SIZE(s->cfg.cpu_ids));
>>> +        return;
>>> +    }
>>> +    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);
>>> +}
>>> +
>>> +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/vmapple.h b/include/hw/vmapple/vmapple.h
>>> index 9090e9c5ac8..3bba59f5ec7 100644
>>> --- a/include/hw/vmapple/vmapple.h
>>> +++ b/include/hw/vmapple/vmapple.h
>>> @@ -16,4 +16,6 @@
>>>
>>>    #define TYPE_VMAPPLE_BDIF "vmapple-bdif"
>>>
>>> +#define TYPE_VMAPPLE_CFG "vmapple-cfg"
>>> +
>>>    #endif /* HW_VMAPPLE_VMAPPLE_H */
>>
>>
Phil Dennis-Jordan Nov. 13, 2024, 2:32 p.m. UTC | #4 
On Mon, 11 Nov 2024 at 05:21, Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2024/11/11 0:01, Phil Dennis-Jordan wrote:
> > On Sun, 10 Nov 2024 at 08:15, Akihiko Odaki <akihiko.odaki@daynix.com>
> wrote:
> >>
> >> On 2024/11/08 23:47, 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
> >>>
> >>> v4:
> >>>
> >>>    * Fixed initialisation of default values for properties
> >>>    * Dropped superfluous endianness conversions
> >>>    * Moved most header code to .c, device name #define goes in
> vmapple.h
> >>>
> >>> v5:
> >>>
> >>>    * Improved error reporting in case of string property buffer
> overflow.
> >>>
> >>> v7:
> >>>
> >>>    * Changed error messages for overrun of properties with
> >>>      fixed-length strings to be more useful to users than developers.
> >>>
> >>> v8:
> >>>
> >>>    * Consistent parenthesising of macro arguments for better safety.
> >>>
> >>>    hw/vmapple/Kconfig           |   3 +
> >>>    hw/vmapple/cfg.c             | 196
> +++++++++++++++++++++++++++++++++++
> >>>    hw/vmapple/meson.build       |   1 +
> >>>    include/hw/vmapple/vmapple.h |   2 +
> >>>    4 files changed, 202 insertions(+)
> >>>    create mode 100644 hw/vmapple/cfg.c
> >>>
> >>> 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..787e2505d57
> >>> --- /dev/null
> >>> +++ b/hw/vmapple/cfg.c
> >>> @@ -0,0 +1,196 @@
> >>> +/*
> >>> + * VMApple Configuration Region
> >>> + *
> >>> + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights
> Reserved.
> >>> + *
> >>> + * SPDX-License-Identifier: GPL-2.0-or-later
> >>> + *
> >>> + * 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/vmapple.h"
> >>> +#include "hw/sysbus.h"
> >>> +#include "qemu/log.h"
> >>> +#include "qemu/module.h"
> >>> +#include "qapi/error.h"
> >>> +#include "net/net.h"
> >>> +
> >>> +OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
> >>> +
> >>> +#define VMAPPLE_CFG_SIZE 0x00010000
> >>> +
> >>> +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;
> >>> +
> >>> +struct VMAppleCfgState {
> >>> +    SysBusDevice parent_obj;
> >>> +    VMAppleCfg cfg;
> >>> +
> >>> +    MemoryRegion mem;
> >>> +    char *serial;
> >>> +    char *model;
> >>> +    char *soc_name;
> >>> +};
> >>> +
> >>> +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(cfg, 0, VMAPPLE_CFG_SIZE);
> >>> +    *cfg = s->cfg;
> >>> +}
> >>> +
> >>> +static bool set_fixlen_property_or_error(char *restrict dst,
> >>> +                                         const char *restrict src,
> >>> +                                         size_t dst_size, Error
> **errp,
> >>> +                                         const char *property_name)
> >>> +{
> >>> +    size_t len;
> >>> +
> >>> +    len = g_strlcpy(dst, src, dst_size);
> >>> +    if (len < dst_size) { /* len does not count nul terminator */
> >>> +        return true;
> >>> +    }
> >>> +
> >>> +    error_setg(errp,
> >>> +               "Failed to set property '%s' on VMApple 'cfg' device:
> length "
> >>> +               "(%zu) exceeds maximum of %zu",
> >>
> >> Don't print the device name here as it will be automatically printed.
> >
> > That's not what I'm seeing in tests. With code as-is, and setting an
> > overly long value from create_cfg() in the vmapple machine type
> > startup, I get the following output:
> >
> > qemu-system-aarch64: Failed to set property 'soc_name' on VMApple
> > 'cfg' device: length (50) exceeds maximum of 31
> >
> > I don't see any redundant mention of an object or property name. Of
> > course, this error occurs during the cfg device's realize(), not
> > immediately when setting the property. I can't see a built-in way to
> > explicitly limit the length of a string property.
>
> I was thinking of the scenario where the device is added with the
> -device option. It seems the device name is not printed when it is
> realized by the machine, but in that case the user will not be aware of
> the 'cfg' device so it is not helpful. A proper way to add a context
> here is calling error_prepend() in the vmapple machine to tell that it
> is a failure in that machine type.
>

I couldn't find any error_prepend() uses in existing machine types to work
from as examples, but I hope I've got it right now. I've made create_cfg()
accept an errp (pass &error_fatal); inside create_cfg, check whether
sysbus_realize_and_unref() succeeded, and if not, apply error_prepend
before returning.

It's in v10 of the patch set, which I've just posted.

>
> >>> +               property_name, len, dst_size - 1);
> >>> +    return false;
> >>> +}
> >>> +
> >>> +#define set_fixlen_property_or_return(dst_array, src, errp,
> property_name) \
> >>> +    do { \
> >>> +        if (!set_fixlen_property_or_error((dst_array), (src), \
> >>> +                                          ARRAY_SIZE(dst_array), \
> >>> +                                          (errp), (property_name))) {
> \
> >>> +            return; \
> >>> +        } \
> >>> +    } while (0)
> >>> +
> >>> +static void vmapple_cfg_realize(DeviceState *dev, Error **errp)
> >>> +{
> >>> +    VMAppleCfgState *s = VMAPPLE_CFG(dev);
> >>> +    uint32_t i;
> >>> +
> >>> +    if (!s->serial) {
> >>> +        s->serial = g_strdup("1234");
> >>> +    }
> >>> +    if (!s->model) {
> >>> +        s->model = g_strdup("VM0001");
> >>> +    }
> >>> +    if (!s->soc_name) {
> >>> +        s->soc_name = g_strdup("Apple M1 (Virtual)");
> >>> +    }
> >>> +
> >>> +    set_fixlen_property_or_return(s->cfg.serial, s->serial, errp,
> "serial");
> >>> +    set_fixlen_property_or_return(s->cfg.model, s->model, errp,
> "model");
> >>> +    set_fixlen_property_or_return(s->cfg.soc_name, s->soc_name, errp,
> "soc_name");
> >>> +    set_fixlen_property_or_return(s->cfg.unk8, "D/A", errp, "unk8");
> >>> +    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;
> >>> +
> >>> +    if (s->cfg.nr_cpus > ARRAY_SIZE(s->cfg.cpu_ids)) {
> >>> +        error_setg(errp,
> >>> +                   "Failed to create %u CPUs, vmapple machine
> supports %zu max",
> >>> +                   s->cfg.nr_cpus, ARRAY_SIZE(s->cfg.cpu_ids));
> >>> +        return;
> >>> +    }
> >>> +    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);
> >>> +}
> >>> +
> >>> +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/vmapple.h
> b/include/hw/vmapple/vmapple.h
> >>> index 9090e9c5ac8..3bba59f5ec7 100644
> >>> --- a/include/hw/vmapple/vmapple.h
> >>> +++ b/include/hw/vmapple/vmapple.h
> >>> @@ -16,4 +16,6 @@
> >>>
> >>>    #define TYPE_VMAPPLE_BDIF "vmapple-bdif"
> >>>
> >>> +#define TYPE_VMAPPLE_CFG "vmapple-cfg"
> >>> +
> >>>    #endif /* HW_VMAPPLE_VMAPPLE_H */
> >>
> >>
>
>
diff mbox series

Patch

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..787e2505d57
--- /dev/null
+++ b/hw/vmapple/cfg.c
@@ -0,0 +1,196 @@ 
+/*
+ * VMApple Configuration Region
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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/vmapple.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "net/net.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(VMAppleCfgState, VMAPPLE_CFG)
+
+#define VMAPPLE_CFG_SIZE 0x00010000
+
+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;
+
+struct VMAppleCfgState {
+    SysBusDevice parent_obj;
+    VMAppleCfg cfg;
+
+    MemoryRegion mem;
+    char *serial;
+    char *model;
+    char *soc_name;
+};
+
+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(cfg, 0, VMAPPLE_CFG_SIZE);
+    *cfg = s->cfg;
+}
+
+static bool set_fixlen_property_or_error(char *restrict dst,
+                                         const char *restrict src,
+                                         size_t dst_size, Error **errp,
+                                         const char *property_name)
+{
+    size_t len;
+
+    len = g_strlcpy(dst, src, dst_size);
+    if (len < dst_size) { /* len does not count nul terminator */
+        return true;
+    }
+
+    error_setg(errp,
+               "Failed to set property '%s' on VMApple 'cfg' device: length "
+               "(%zu) exceeds maximum of %zu",
+               property_name, len, dst_size - 1);
+    return false;
+}
+
+#define set_fixlen_property_or_return(dst_array, src, errp, property_name) \
+    do { \
+        if (!set_fixlen_property_or_error((dst_array), (src), \
+                                          ARRAY_SIZE(dst_array), \
+                                          (errp), (property_name))) { \
+            return; \
+        } \
+    } while (0)
+
+static void vmapple_cfg_realize(DeviceState *dev, Error **errp)
+{
+    VMAppleCfgState *s = VMAPPLE_CFG(dev);
+    uint32_t i;
+
+    if (!s->serial) {
+        s->serial = g_strdup("1234");
+    }
+    if (!s->model) {
+        s->model = g_strdup("VM0001");
+    }
+    if (!s->soc_name) {
+        s->soc_name = g_strdup("Apple M1 (Virtual)");
+    }
+
+    set_fixlen_property_or_return(s->cfg.serial, s->serial, errp, "serial");
+    set_fixlen_property_or_return(s->cfg.model, s->model, errp, "model");
+    set_fixlen_property_or_return(s->cfg.soc_name, s->soc_name, errp, "soc_name");
+    set_fixlen_property_or_return(s->cfg.unk8, "D/A", errp, "unk8");
+    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;
+
+    if (s->cfg.nr_cpus > ARRAY_SIZE(s->cfg.cpu_ids)) {
+        error_setg(errp,
+                   "Failed to create %u CPUs, vmapple machine supports %zu max",
+                   s->cfg.nr_cpus, ARRAY_SIZE(s->cfg.cpu_ids));
+        return;
+    }
+    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);
+}
+
+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/vmapple.h b/include/hw/vmapple/vmapple.h
index 9090e9c5ac8..3bba59f5ec7 100644
--- a/include/hw/vmapple/vmapple.h
+++ b/include/hw/vmapple/vmapple.h
@@ -16,4 +16,6 @@ 
 
 #define TYPE_VMAPPLE_BDIF "vmapple-bdif"
 
+#define TYPE_VMAPPLE_CFG "vmapple-cfg"
+
 #endif /* HW_VMAPPLE_VMAPPLE_H */