Re: [PATCH 08/13] hw/arm/mps3r: Initial skeleton for mps3-an536 board

[Peter Maydell]

On Wed, 7 Feb 2024 at 09:02, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 6/2/24 14:29, Peter Maydell wrote:
> > The AN536 is another FPGA image for the MPS3 development board. Unlike
> > the existing FPGA images we already model, this board uses a Cortex-R
> > family CPU, and it does not use any equivalent to the M-profile
> > "Subsystem for Embedded" SoC-equivalent that we model in hw/arm/armsse.c.
> > It's therefore more convenient for us to model it as a completely
> > separate C file.
> >
> > This commit adds the basic skeleton of the board model, and the
> > code to create all the RAM and ROM. We assume that we're probably
> > going to want to add more images in future, so use the same
> > base class/subclass setup that mps2-tz.c uses, even though at
> > the moment there's only a single subclass.
> >
> > Following commits will add the CPUs and the peripherals.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   MAINTAINERS                             |   3 +-
> >   configs/devices/arm-softmmu/default.mak |   1 +
> >   hw/arm/mps3r.c                          | 239 ++++++++++++++++++++++++
> >   hw/arm/Kconfig                          |   5 +
> >   hw/arm/meson.build                      |   1 +
> >   5 files changed, 248 insertions(+), 1 deletion(-)
> >   create mode 100644 hw/arm/mps3r.c
>
>
> > +/*
> > + * The MPS3 DDR is 3GiB, but on a 32-bit host QEMU doesn't permit
> > + * emulation of that much guest RAM, so artificially make it smaller.
> > + */
> > +#if HOST_LONG_BITS == 32
> > +#define MPS3_DDR_SIZE (1 * GiB)
> > +#else
> > +#define MPS3_DDR_SIZE (3 * GiB)
> > +#endif
>
> Generically, can we migrate a VM started on a 32-bit host to a 64-bit
> one?

I think it's one of those things that in theory is supposed
to be possible and in practice nobody tests so it might well
not work. At any rate, this is the same thing we do already
in mps2-tz.c for the 2GB DRAM those boards have.

> > +static void mps3r_set_default_ram_info(MPS3RMachineClass *mmc)
> > +{
> > +    /*
> > +     * Set mc->default_ram_size and default_ram_id from the
> > +     * information in mmc->raminfo.
> > +     */
> > +    MachineClass *mc = MACHINE_CLASS(mmc);
> > +    const RAMInfo *p;
> > +
> > +    for (p = mmc->raminfo; p->name; p++) {
> > +        if (p->mrindex < 0) {
> > +            /* Found the entry for "system memory" */
> > +            mc->default_ram_size = p->size;
> > +            mc->default_ram_id = p->name;
>
> Nice.
>
> > +            return;
> > +        }
> > +    }
> > +    g_assert_not_reached();
> > +}
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks.

I have remembered why we don't pass the MachineState as the
owner pointer for memory_region_init_ram(): in that function
we do a cast of the owner pointer to a DeviceState, so we
can hand it to vmstate_register_ram(). And a MachineState
inherits directly from Object, not from DeviceState.
There's a paragraph in the doc-comment for
memory_region_init_ram() explaining this: currently we
require the owner pointer to be either NULL or a DeviceState.

thanks
-- PMM