Re: Subject: [PATCH] loader: Add register setting support via cli

[Alistair Francis]

On Mon, Jan 6, 2025 at 2:59 PM Sam Price <thesamprice@gmail.com> wrote:
>
> I didn't mean to change all of the submodules.
> I was somewhat porting from xilinx-qemu over to the main line, and
> messed up the commit on that.
> Ill get my gitlab branch fixed up on the next commit.
> I am horrible at this email part.
>
> I could malloc memory and push all of the register names/ values into
> a queue, and then call the backend gdb routines on the number of
> calls.

So GDB uses the XML files in `gdb-xml` for the register information.
Considering that each architecture uses a different register naming
structure I think you are going to have a hard time doing this
manually.

The best bet is probably just to use `cc->gdb_num_core_regs` to get
the number of regs and then use integer offsets (so no strings, just
0, 1, 2...) for the registers.

It's still not ideal, but might be good enough

> I will rely on the gdb routine for doing validation of both the name /
> value arguments.

If there is a way to do this with the GDB stub that might also work well

>
> I will make an implementation of this using the include/qemue/queue.h
> to store all passed in register arguments.
> https://gitlab.com/thesamprice/qemu/-/blob/master/include/qemu/queue.h
> If there is a better datastructure I should use for implementation let me 
> know.

I don't follow why you need a queue, but I might just be misunderstanding.

>
> Ill also update docs/system/generic-loader.rst

Yes please

>
> Are unit tests needed?

I think for now I wouldn't worry about it

Alistair

> Any guidance on what you would want done for this would be appreciated.
>
> Thanks,
> Sam
>
> On Sun, Jan 5, 2025 at 11:41 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Dec 6, 2024 at 1:30 PM Sam Price <thesamprice@gmail.com> wrote:
> > >
> > > I needed to set the registers prior to boot up to mimic what uboot
> > > would do prior to loading a binary.  This adds a generic option of reg
> > > to the loader command, it uses the existing gcc commands for setting
> > > register values.
> > >
> > > I'm sorry I couldn't figure out how to work the git send-email
> > > properly.  Configuring it with my gmail became too cumbersome, and my
> > > work email was also challenging to configure.
> > >
> > > I have the file staged here.
> > > https://gitlab.com/thesamprice/qemu/-/tree/loader?ref_type=heads
> > > I am unsure of how to add tests for this.
> > > I could continue working too polish this off with instructions from
> > > others if it is desired for the main line.
> >
> > Thanks for the patch. This will need documentation added under
> > `docs/system/generic-loader.rst` so that people know how to use it and
> > what it does
> >
> > >
> > > Signed-off-by: Sam Price <thesamprice@gmail.com>
> > > ---
> > >
> > > ---
> > >  hw/core/generic-loader.c         | 28 ++++++++++++++++++++++++++++
> > >  include/hw/core/generic-loader.h |  6 +++++-
> > >  roms/SLOF                        |  2 +-
> > >  roms/edk2                        |  2 +-
> > >  roms/openbios                    |  2 +-
> > >  roms/opensbi                     |  2 +-
> > >  roms/seabios                     |  2 +-
> > >  roms/seabios-hppa                |  2 +-
> > >  tests/lcitool/libvirt-ci         |  2 +-
> > >  9 files changed, 40 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > > index ea8628b892..ebda8ac43f 100644
> > > --- a/hw/core/generic-loader.c
> > > +++ b/hw/core/generic-loader.c
> > > @@ -55,6 +55,19 @@ static void generic_loader_reset(void *opaque)
> > >          }
> > >      }
> > >
> > > +    for (int i = 0; i < 31; i++) {
> >
> > Why 31?
> >
> > I'm guessing you picked this because that's how many registers your
> > architecture supports, but that's not the same for all architectures
> >
> > > +        if (s->has_register_defaults[i]) {
> > > +            CPUClass *cc = CPU_GET_CLASS(s->cpu);
> > > +            uint8_t buf[sizeof(uint64_t)];
> > > +            memcpy(buf, &s->register_defaults[i], sizeof(uint64_t));
> > > +            if (cc && cc->gdb_write_register) {
> > > +                cc->gdb_write_register(s->cpu, buf, i);
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +
> > > +
> > >      if (s->data_len) {
> > >          assert(s->data_len <= sizeof(s->data));
> > >          dma_memory_write(s->cpu->as, s->addr, &s->data, s->data_len,
> > > @@ -172,6 +185,20 @@ static void generic_loader_realize(DeviceState
> > > *dev, Error **errp)
> > >      } else {
> > >          s->data = cpu_to_le64(s->data);
> > >      }
> > > +
> > > +    /* Store the CPU register default if specified */
> > > +    if (s->reg) {
> > > +        int reg_num;
> > > +        if (sscanf(s->reg, "r%d", &reg_num) == 1 &&
> > > +                    reg_num >= 0 && reg_num < 31) {
> >
> > I don't think all architectures call their registers r* and they don't
> > all have 31 registers
> >
> > > +            s->register_defaults[reg_num] = s->data;
> > > +            s->has_register_defaults[reg_num] = true;
> > > +        } else {
> > > +            error_setg(errp, "Unsupported register: %s", s->reg);
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > >  }
> > >
> > >  static void generic_loader_unrealize(DeviceState *dev)
> > > @@ -186,6 +213,7 @@ static Property generic_loader_props[] = {
> > >      DEFINE_PROP_BOOL("data-be", GenericLoaderState, data_be, false),
> > >      DEFINE_PROP_UINT32("cpu-num", GenericLoaderState, cpu_num, CPU_NONE),
> > >      DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
> > > +    DEFINE_PROP_STRING("reg", GenericLoaderState, reg),
> > >      DEFINE_PROP_STRING("file", GenericLoaderState, file),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > > diff --git a/include/hw/core/generic-loader.h 
> > > b/include/hw/core/generic-loader.h
> > > index 19d87b39c8..d81e1632fd 100644
> > > --- a/include/hw/core/generic-loader.h
> > > +++ b/include/hw/core/generic-loader.h
> > > @@ -35,10 +35,14 @@ struct GenericLoaderState {
> > >      uint32_t cpu_num;
> > >
> > >      char *file;
> > > -
> > > +    char *reg;
> > >      bool force_raw;
> > >      bool data_be;
> > >      bool set_pc;
> > > +
> > > +    /* Add an array for storing default register values */
> > > +    bool has_register_defaults[31];  /* Track if a default value is 
> > > provided */
> > > +    uint64_t register_defaults[31];  /* Default values for registers 
> > > r0-r30 */
> > >  };
> > >
> > >  #define TYPE_GENERIC_LOADER "loader"
> > > diff --git a/roms/SLOF b/roms/SLOF
> > > index 3a259df244..6b6c16b4b4 160000
> > > --- a/roms/SLOF
> > > +++ b/roms/SLOF
> > > @@ -1 +1 @@
> > > -Subproject commit 3a259df2449fc4a4e43ab5f33f0b2c66484b4bc3
> > > +Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d
> > > diff --git a/roms/edk2 b/roms/edk2
> > > index 4dfdca63a9..f80f052277 160000
> > > --- a/roms/edk2
> > > +++ b/roms/edk2
> > > @@ -1 +1 @@
> > > -Subproject commit 4dfdca63a93497203f197ec98ba20e2327e4afe4
> > > +Subproject commit f80f052277c88a67c55e107b550f504eeea947d3
> > > diff --git a/roms/openbios b/roms/openbios
> > > index c3a19c1e54..af97fd7af5 160000
> > > --- a/roms/openbios
> > > +++ b/roms/openbios
> > > @@ -1 +1 @@
> > > -Subproject commit c3a19c1e54977a53027d6232050e1e3e39a98a1b
> > > +Subproject commit af97fd7af5e7c18f591a7b987291d3db4ffb28b5
> > > diff --git a/roms/opensbi b/roms/opensbi
> > > index 43cace6c36..057eb10b6d 160000
> > > --- a/roms/opensbi
> > > +++ b/roms/opensbi
> > > @@ -1 +1 @@
> > > -Subproject commit 43cace6c3671e5172d0df0a8963e552bb04b7b20
> > > +Subproject commit 057eb10b6d523540012e6947d5c9f63e95244e94
> > > diff --git a/roms/seabios b/roms/seabios
> > > index a6ed6b701f..ea1b7a0733 160000
> > > --- a/roms/seabios
> > > +++ b/roms/seabios
> > > @@ -1 +1 @@
> > > -Subproject commit a6ed6b701f0a57db0569ab98b0661c12a6ec3ff8
> > > +Subproject commit ea1b7a0733906b8425d948ae94fba63c32b1d425
> > > diff --git a/roms/seabios-hppa b/roms/seabios-hppa
> > > index a528f01d7a..673d2595d4 160000
> > > --- a/roms/seabios-hppa
> > > +++ b/roms/seabios-hppa
> > > @@ -1 +1 @@
> > > -Subproject commit a528f01d7abd511d3cc71b7acaab6e036ee524bd
> > > +Subproject commit 673d2595d4f773cc266cbf8dbaf2f475a6adb949
> > > diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci
> > > index 9ad3f70bde..9bff3b763b 160000
> > > --- a/tests/lcitool/libvirt-ci
> > > +++ b/tests/lcitool/libvirt-ci
> > > @@ -1 +1 @@
> > > -Subproject commit 9ad3f70bde9865d5ad18f36d256d472e72b5cbf3
> > > +Subproject commit 9bff3b763b5531a1490e238bfbf77306dc3a6dbb
> >
> > Did you mean to change all of these submodules?
> >
> >
> > Alistair
> >
> > > --
> > > 2.45.2
> > >
>
>
>
> --
> Sincerely,
>
> Sam Price