Re: [PATCH] hw/riscv: microchip_pfsoc: specify XIP image

[Alistair Francis]

On Tue, Jan 12, 2021 at 4:11 AM Vitaly Wool <vitaly.wool@konsulko.com> wrote:
>
> Hi Bin,
>
> On Tue, Jan 5, 2021 at 7:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > +Alistair Francis
> >
> > On Sat, Dec 19, 2020 at 8:24 AM Vitaly Wool <vitaly.wool@konsulko.com> 
> > wrote:
> > >
> > > Add command line parameter to microchip_pfsoc machine to be able
> > > to specify XIP kernel image file. To pass over XIP image file, it
> > > will be enough to run
> > >
> > > $ qemu-system-riscv64 -M microchip-icicle-kit,xipImage=<image> ...
> > >
> > > Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.com>
> > > ---
> > >  hw/riscv/microchip_pfsoc.c         | 42 +++++++++++++++++++++++++++---
> > >  include/hw/riscv/microchip_pfsoc.h |  1 +
> > >  2 files changed, 39 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > > index e952b49e8c..04d81d52fe 100644
> > > --- a/hw/riscv/microchip_pfsoc.c
> > > +++ b/hw/riscv/microchip_pfsoc.c
> > > @@ -181,6 +181,7 @@ static void microchip_pfsoc_soc_instance_init(Object 
> > > *obj)
> > >  static void microchip_pfsoc_soc_realize(DeviceState *dev, Error **errp)
> > >  {
> > >      MachineState *ms = MACHINE(qdev_get_machine());
> > > +    MicrochipIcicleKitState *mks = MICROCHIP_ICICLE_KIT_MACHINE(ms);
> > >      MicrochipPFSoCState *s = MICROCHIP_PFSOC(dev);
> > >      const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
> > >      MemoryRegion *system_memory = get_system_memory();
> > > @@ -415,10 +416,19 @@ static void microchip_pfsoc_soc_realize(DeviceState 
> > > *dev, Error **errp)
> > >                      memmap[MICROCHIP_PFSOC_IOSCB].base);
> > >
> > >      /* QSPI Flash */
> > > -    memory_region_init_rom(qspi_xip_mem, OBJECT(dev),
> >
> > I believe we only need to change this to memory_region_init_ram(),
> > then use the device loader the load the XIP image into this space. The
> > remove the need to introduce the XIP image property as you did.
>
> I think it should be possible to use the device loader with rom as
> well, and in this case we can skip this patch altogether. However, my
> idea was that specifying explicitly it's not just a random data being
> loaded but a kernel image in the XIP format would make things clearer.
> OTOH, I would then introduce a property that isn't strictly speaking
> necessary. I'll let Alistair decide what is the best way to go :)

In general I would say let's avoid machine properties. The more
aligned we can be to general QEMU arguments the better. That way
someone swapping from some other platform shouldn't have too much
trouble getting started.

Ideally we want to use either the device loader or the `-drive`
argument for loading these images. The RISC-V virt machine has -drive
support, maybe that is what you want here instead?

Alistair

>
> ~Vitaly
>
> >
> > > -                           "microchip.pfsoc.qspi_xip",
> > > -                           memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > > -                           &error_fatal);
> > > +    if (mks->xip_image) {
> > > +        memory_region_init_ram_from_file(qspi_xip_mem, OBJECT(dev),
> > > +                                         "microchip.pfsoc.qspi_xip",
> > > +                                         
> > > memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > > +                                         0x10000 /* align */, 0 /* 
> > > ram_flags */,
> > > +                                         mks->xip_image, &error_fatal);
> > > +        qspi_xip_mem->readonly = true;
> > > +    } else {
> > > +        memory_region_init_rom(qspi_xip_mem, OBJECT(dev),
> > > +                               "microchip.pfsoc.qspi_xip",
> > > +                               memmap[MICROCHIP_PFSOC_QSPI_XIP].size,
> > > +                               &error_fatal);
> > > +    }
> > >      memory_region_add_subregion(system_memory,
> > >                                  memmap[MICROCHIP_PFSOC_QSPI_XIP].base,
> > >                                  qspi_xip_mem);
> > > @@ -517,6 +527,24 @@ static void 
> > > microchip_icicle_kit_machine_init(MachineState *machine)
> > >      }
> > >  }
> > >
> >
> > Regards,
> > Bin
>