On Mon, 11 Nov 2024 at 05:21, Akihiko Odaki <akihiko.od...@daynix.com>
wrote:
> On 2024/11/11 0:01, Phil Dennis-Jordan wrote:
> > On Sun, 10 Nov 2024 at 08:15, Akihiko Odaki <akihiko.od...@daynix.com>
> wrote:
> >>
> >> On 2024/11/08 23:47, 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
> >>>
> >>> 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 */
> >>
> >>
>
>
