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

[Alexander Graf <graf@amazon.com>]

On 16.06.23 12:47, Philippe Mathieu-Daudé wrote:
>
> On 15/6/23 00:57, Alexander Graf wrote:
>> 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>
>> ---
>>   hw/vmapple/Kconfig       |   3 ++
>>   hw/vmapple/cfg.c         | 105 +++++++++++++++++++++++++++++++++++++++
>>   hw/vmapple/meson.build   |   1 +
>>   include/hw/vmapple/cfg.h |  68 +++++++++++++++++++++++++
>>   4 files changed, 177 insertions(+)
>>   create mode 100644 hw/vmapple/cfg.c
>>   create mode 100644 include/hw/vmapple/cfg.h
>
>
>> diff --git a/hw/vmapple/cfg.c b/hw/vmapple/cfg.c
>> new file mode 100644
>> index 0000000000..d48e3c3afa
>> --- /dev/null
>> +++ b/hw/vmapple/cfg.c
>> @@ -0,0 +1,105 @@
>> +/*
>> + * 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(DeviceState *dev)
>> +{
>> +    VMAppleCfgState *s = VMAPPLE_CFG(dev);
>> +    VMAppleCfg *cfg;
>> +
>> +    cfg = memory_region_get_ram_ptr(&s->mem);
>> +    memset((void *)cfg, 0, VMAPPLE_CFG_SIZE);
>
> I'm a bit confused here: DeviceReset() handler is called _after_
> DeviceRealize().


Yes. In Realize we set up s->cfg (the template). In reset, we fetch a 
pointer to the guest exposed memory region (cfg), wipe it and then copy 
the template over it in the next line:


>
>> +    *cfg = s->cfg;


[...]


>
>> diff --git a/include/hw/vmapple/cfg.h b/include/hw/vmapple/cfg.h
>> new file mode 100644
>> index 0000000000..3337064e44
>> --- /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;
>
> Since you access this structure via qdev properties (which is
> good), then we can restrict its definition to cfg.c (no need to
> expose it).


This struct is part of VMAppleCfgState which (unless we go through 
pointers and allocate dynamically - bleks) means it needs to know the 
size of the struct which again means it needs to be part of the header :)


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879