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

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

[Sam Price]

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.

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++) {
+        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) {
+            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
-- 
2.45.2

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

[Sam Price]

[-- Attachment #1: Type: text/plain, Size: 6040 bytes --]

Bump / ping

Sincerely,

Sam Price



On Thu, Dec 5, 2024 at 10:29 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.
>
> 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++) {
> +        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) {
> +            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
> --
> 2.45.2
>

[-- Attachment #2: Type: text/html, Size: 7459 bytes --]

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

[Sam Price]

Bump / Ping

On Thu, Dec 19, 2024 at 6:11 PM Sam Price <thesamprice@gmail.com> wrote:
>
> Bump / ping
>
> Sincerely,
>
> Sam Price
>
>
>
> On Thu, Dec 5, 2024 at 10:29 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.
>>
>> 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++) {
>> +        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) {
>> +            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
>> --
>> 2.45.2



-- 
Sincerely,

Sam Price

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

[Alistair Francis]

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
>

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

[Sam Price]

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.
I will rely on the gdb routine for doing validation of both the name /
value arguments.

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.

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

Are unit tests needed?
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

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

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

[Sam Price]

I made the changes, and added documentation.
https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699

I left it as [PREFIX]<RegNumber>

I can switch this to just RegNumber if desired.

I am still struggling with the email format sorry.
---
docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
hw/core/generic-loader.c | 46 +++++++++++----
include/hw/core/generic-loader.h | 7 +++
3 files changed, 139 insertions(+), 12 deletions(-)

diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst
index 4f9fb005f1..71d4aaa097 100644
--- a/docs/system/generic-loader.rst
+++ b/docs/system/generic-loader.rst
@@ -117,4 +117,102 @@ future the internal state 'set_pc' (which exists
in the generic loader
now) should be exposed to the user so that they can choose if the PC
is set or not.
+Loading Data into Registers
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The `loader` device allows the initialization of CPU registers from the command
+line. This feature is particularly useful for setting up the processor state
+before starting an executable. By configuring registers prior to execution, the
+`loader` can mimic the state that a bootloader would leave the processor in
+before transferring control to an ELF file or another executable.
+
+The syntax for loading data into registers is as follows::
+
+ -device loader,reg=<reg>,data=<data>,data-len=<data-len>
+
+**Parameters:**
+
+``<reg>``
+ The target register to set. Format must pass the following regex
+ ``[a-zA-Z]+[0-9]+``. The numeric part corresponds to the processor's GDB \
+ register index. For general-purpose registers, this is typically the
+ number in the register's name (e.g., ``r5`` translates to ``5``).
+ Special-purpose registers have specific IDs defined in their processor's
+ `gdbstub.c` file. Note that these IDs vary between processors.
+
+``<data>``
+ The value to load into the specified register. The data must not exceed 8
+ bytes in size.
+
+``<data-len>``
+ The length of the data in bytes. This parameter is mandatory when using
+ the ``data`` argument.
+
+**Examples:**
+
+Set a general-purpose register
+""""""""""""""""""""""""""""""
+
+To set register ``r5`` to ``0xc0001000`` (4 bytes) on CPU 0::
+
+ -device loader,reg=r5,data=0xc0001000,data-len=4
+
+Set a special register
+""""""""""""""""""""""
+
+To set the Program Counter (PC, register ``32``) to ``0x80000000`` on CPU 0::
+
+ -device loader,reg=pc32,data=0x80000000,data-len=4
+
+You must look in your processor's `gdbstub.c` file to special register to index
+mappings.
+
+**Special Registers:**
+
+Special registers are defined in the processor's ``gdbstub.c`` file
with numeric IDs.
+Examples from the MicroBlaze processor at one point looked like. include::
+
+ enum {
+ GDB_PC = 32 + 0,
+ GDB_MSR = 32 + 1,
+ GDB_EAR = 32 + 2,
+ GDB_ESR = 32 + 3,
+ GDB_FSR = 32 + 4,
+ GDB_BTR = 32 + 5,
+ GDB_PVR0 = 32 + 6,
+ GDB_PVR11 = 32 + 17,
+ GDB_EDR = 32 + 18,
+ GDB_SLR = 32 + 25,
+ GDB_SHR = 32 + 26,
+ };
+
+For example, to set the Machine State Register (``GDB_MSR``) on a MicroBlaze
+processor::
+
+ -device loader,reg=MSR33,data=0x00000001,data-len=4
+
+**Register Loading Notes:**
+
+1. **Processor-Specific IDs**:
+ The numeric IDs for registers vary between processors. Always refer to the
+ `gdbstub.c` file for the target processor to identify the correct register
+ mappings.
+
+2. **Pre-Execution State**:
+ This capability is ideal for initializing a simulated environment to match
+ the state expected by an ELF file. For example, you can configure stack
+ pointers, machine state registers, and program counters to prepare the
+ processor to run a bootstrapped application.
+
+3. **Validation**:
+ Register numbers are validated by the `gdb_write_register` function. Ensure
+ the specified register is supported by the target architecture.
+
+4. **Endianess**:
+ The `data` value is written using the processor's native endian format.
+
+By using the `loader` device to initialize registers, you can simulate
+realistic execution environments, enabling detailed testing and debugging
+of embedded software, including bootloaders interactions and operating
+system kernels.
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index ea8628b892..9408ecd150 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -55,6 +55,14 @@ static void generic_loader_reset(void *opaque)
}
}
+ if(s->reg.name) {
+ CPUClass *cc = CPU_GET_CLASS(s->cpu);
+ int bytes_written = cc->gdb_write_register(s->cpu, (uint8_t*)
&s->reg.value, s->reg.num);
+ if(bytes_written != s->reg.data_len) {
+ printf("Error setting register %d to value %lX expected to write %d,
but wrote %d\n", s->reg.num, s->reg.value, s->reg.data_len,
bytes_written);
+ }
+ }
+
if (s->data_len) {
assert(s->data_len <= sizeof(s->data));
dma_memory_write(s->cpu->as, s->addr, &s->data, s->data_len,
@@ -89,14 +97,12 @@ static void generic_loader_realize(DeviceState
*dev, Error **errp)
} else if (s->data_len > 8) {
error_setg(errp, "data-len cannot be greater then 8 bytes");
return;
- }
- } else if (s->file || s->force_raw) {
- /* User is loading an image */
- if (s->data || s->data_len || s->data_be) {
- error_setg(errp, "data can not be specified when loading an "
- "image");
+ }else if (s->addr) {
+ error_setg(errp, "data can not be specified when setting a "
+ "program counter");
return;
}
+ } else if (s->file || s->force_raw) {
/* The user specified a file, only set the PC if they also specified
* a CPU to use.
*/
@@ -105,17 +111,13 @@ static void generic_loader_realize(DeviceState
*dev, Error **errp)
}
} else if (s->addr) {
/* User is setting the PC */
- if (s->data || s->data_len || s->data_be) {
- error_setg(errp, "data can not be specified when setting a "
- "program counter");
- return;
- } else if (s->cpu_num == CPU_NONE) {
+ if (s->cpu_num == CPU_NONE) {
error_setg(errp, "cpu_num must be specified when setting a "
"program counter");
return;
}
s->set_pc = true;
- } else {
+ } else {
/* Did the user specify anything? */
error_setg(errp, "please include valid arguments");
return;
@@ -166,6 +168,25 @@ static void generic_loader_realize(DeviceState
*dev, Error **errp)
}
}
+ if (s->reg.name) {
+ int reg_num;
+ CPUClass *cc = CPU_GET_CLASS(s->cpu);
+ char prefix[32]; /* Read up to 32 characters prior to the register number*/
+ int scan_num = sscanf(s->reg.name, "%31[a-zA-Z]%d",prefix, &reg_num);
+ if ( scan_num != 2){
+ error_setg(errp, "Unsupported register: %s, Failed to deterimine
register number", s->reg.name);
+ s->reg.name = 0x0;
+ }else if( reg_num < 0 || cc->gdb_num_core_regs < reg_num){
+ error_setg(errp, "Unsupported register: %s, register number must be
less than %d, got %d", s->reg.name, cc->gdb_num_core_regs, reg_num);
+ s->reg.name = 0x0;
+ }else{
+ s->reg.value = s->data;
+ s->reg.num = reg_num;
+ s->reg.data_len = s->data_len;
+ s->data_len = 0;
+ }
+ }
+
/* Convert the data endianness */
if (s->data_be) {
s->data = cpu_to_be64(s->data);
@@ -187,6 +208,7 @@ static Property generic_loader_props[] = {
DEFINE_PROP_UINT32("cpu-num", GenericLoaderState, cpu_num, CPU_NONE),
DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
DEFINE_PROP_STRING("file", GenericLoaderState, file),
+ DEFINE_PROP_STRING("reg", GenericLoaderState, reg.name),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
index 19d87b39c8..ba826806d3 100644
--- a/include/hw/core/generic-loader.h
+++ b/include/hw/core/generic-loader.h
@@ -39,6 +39,13 @@ struct GenericLoaderState {
bool force_raw;
bool data_be;
bool set_pc;
+
+ struct {
+ char * name;
+ int num;
+ int data_len;
+ uint64_t value;
+ } reg;
};
#define TYPE_GENERIC_LOADER "loader"
-- 
2.45.2

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

[Alistair Francis]

On Wed, Jan 8, 2025 at 12:28 PM Sam Price <thesamprice@gmail.com> wrote:
>
> I made the changes, and added documentation.
> https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699
>
> I left it as [PREFIX]<RegNumber>
>
> I can switch this to just RegNumber if desired.
>
> I am still struggling with the email format sorry.
> ---
> docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
> hw/core/generic-loader.c | 46 +++++++++++----
> include/hw/core/generic-loader.h | 7 +++
> 3 files changed, 139 insertions(+), 12 deletions(-)
>
> diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst
> index 4f9fb005f1..71d4aaa097 100644
> --- a/docs/system/generic-loader.rst
> +++ b/docs/system/generic-loader.rst
> @@ -117,4 +117,102 @@ future the internal state 'set_pc' (which exists
> in the generic loader
> now) should be exposed to the user so that they can choose if the PC
> is set or not.
> +Loading Data into Registers
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The `loader` device allows the initialization of CPU registers from the command
> +line. This feature is particularly useful for setting up the processor state
> +before starting an executable. By configuring registers prior to execution, the
> +`loader` can mimic the state that a bootloader would leave the processor in
> +before transferring control to an ELF file or another executable.

This isn't really true though. A bootloader generally will set more
than the GP registers. A boot loader will configure devices and
perhaps initalise memory.

> +
> +The syntax for loading data into registers is as follows::
> +
> + -device loader,reg=<reg>,data=<data>,data-len=<data-len>
> +
> +**Parameters:**
> +
> +``<reg>``
> + The target register to set. Format must pass the following regex
> + ``[a-zA-Z]+[0-9]+``. The numeric part corresponds to the processor's GDB \
> + register index. For general-purpose registers, this is typically the
> + number in the register's name (e.g., ``r5`` translates to ``5``).
> + Special-purpose registers have specific IDs defined in their processor's
> + `gdbstub.c` file. Note that these IDs vary between processors.
> +
> +``<data>``
> + The value to load into the specified register. The data must not exceed 8
> + bytes in size.

Why 8 bytes?

> +
> +``<data-len>``
> + The length of the data in bytes. This parameter is mandatory when using
> + the ``data`` argument.

Do we need data-len? Why not just use the register size

> +
> +**Examples:**
> +
> +Set a general-purpose register
> +""""""""""""""""""""""""""""""
> +
> +To set register ``r5`` to ``0xc0001000`` (4 bytes) on CPU 0::
> +
> + -device loader,reg=r5,data=0xc0001000,data-len=4
> +
> +Set a special register
> +""""""""""""""""""""""
> +
> +To set the Program Counter (PC, register ``32``) to ``0x80000000`` on CPU 0::
> +
> + -device loader,reg=pc32,data=0x80000000,data-len=4
> +
> +You must look in your processor's `gdbstub.c` file to special register to index
> +mappings.

That isn't really helpful for users, but I don't have a better idea

> +
> +**Special Registers:**
> +
> +Special registers are defined in the processor's ``gdbstub.c`` file
> with numeric IDs.
> +Examples from the MicroBlaze processor at one point looked like. include::
> +
> + enum {
> + GDB_PC = 32 + 0,
> + GDB_MSR = 32 + 1,
> + GDB_EAR = 32 + 2,
> + GDB_ESR = 32 + 3,
> + GDB_FSR = 32 + 4,
> + GDB_BTR = 32 + 5,
> + GDB_PVR0 = 32 + 6,
> + GDB_PVR11 = 32 + 17,
> + GDB_EDR = 32 + 18,
> + GDB_SLR = 32 + 25,
> + GDB_SHR = 32 + 26,
> + };
> +
> +For example, to set the Machine State Register (``GDB_MSR``) on a MicroBlaze
> +processor::
> +
> + -device loader,reg=MSR33,data=0x00000001,data-len=4
> +
> +**Register Loading Notes:**
> +
> +1. **Processor-Specific IDs**:
> + The numeric IDs for registers vary between processors. Always refer to the
> + `gdbstub.c` file for the target processor to identify the correct register
> + mappings.
> +
> +2. **Pre-Execution State**:
> + This capability is ideal for initializing a simulated environment to match
> + the state expected by an ELF file. For example, you can configure stack
> + pointers, machine state registers, and program counters to prepare the
> + processor to run a bootstrapped application.
> +
> +3. **Validation**:
> + Register numbers are validated by the `gdb_write_register` function. Ensure
> + the specified register is supported by the target architecture.
> +
> +4. **Endianess**:
> + The `data` value is written using the processor's native endian format.
> +
> +By using the `loader` device to initialize registers, you can simulate
> +realistic execution environments, enabling detailed testing and debugging
> +of embedded software, including bootloaders interactions and operating
> +system kernels.
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index ea8628b892..9408ecd150 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -55,6 +55,14 @@ static void generic_loader_reset(void *opaque)
> }
> }
> + if(s->reg.name) {
> + CPUClass *cc = CPU_GET_CLASS(s->cpu);
> + int bytes_written = cc->gdb_write_register(s->cpu, (uint8_t*)
> &s->reg.value, s->reg.num);
> + if(bytes_written != s->reg.data_len) {
> + printf("Error setting register %d to value %lX expected to write %d,
> but wrote %d\n", s->reg.num, s->reg.value, s->reg.data_len,
> bytes_written);

The line wrapping is muddled up here. Can you please send it with git
send-email. Do some sends against yourself to make sure it works.

You mentioned gmail in an earlier thread I think, did you follow the
instructions: https://git-scm.com/docs/git-send-email#_use_gmail_as_the_smtp_server

Alistair

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

[Sam Price]

Yes that is true a boot loader will do more than just set registers.
Ill rework the text a bit on the next update.
In my case i need to set the r5 register that specifies the memory
location to the device tree.
I also use the device loader to load in a elf file to ram, and the
device-loader to load in the device tree to the location specified by
the r5 register

I could add a gdb call that would return an array of string mappings
to integers.
If the machine doesn't implement the function/  leaves it as null
pointer then you wouldn't get the cli support.
Not sure where you would document all the machine register names /
numbers at though.
This might be too much though?
I left the door somewhat open on this via the NAME_NUMBER format.

There was some checking logic where if data is supplied then it forces
a check for data-len.
I could relax that check if you supply the reg.name field.

I am unsure how to determine the machine register size.
I assumed the max register size on any machine would be 8 bytes, this
might be wrong.
the gdb call seems to just pass in the full 8 bytes, but I didn't dig
into it for all machines.

Ill look at this a bit more and try to configure the git email.
I also need to set up a docker container to build /test latest.
I have been building / testing on an old ubuntu machine.
(To test this I need to run it on qemu-xilinx).
My workplace has us on ubuntu 20.

So it might be a while before I have another version up.

Thanks,
Sam

On Thu, Jan 9, 2025 at 7:34 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jan 8, 2025 at 12:28 PM Sam Price <thesamprice@gmail.com> wrote:
> >
> > I made the changes, and added documentation.
> > https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699
> >
> > I left it as [PREFIX]<RegNumber>
> >
> > I can switch this to just RegNumber if desired.
> >
> > I am still struggling with the email format sorry.
> > ---
> > docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
> > hw/core/generic-loader.c | 46 +++++++++++----
> > include/hw/core/generic-loader.h | 7 +++
> > 3 files changed, 139 insertions(+), 12 deletions(-)
> >
> > diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst
> > index 4f9fb005f1..71d4aaa097 100644
> > --- a/docs/system/generic-loader.rst
> > +++ b/docs/system/generic-loader.rst
> > @@ -117,4 +117,102 @@ future the internal state 'set_pc' (which exists
> > in the generic loader
> > now) should be exposed to the user so that they can choose if the PC
> > is set or not.
> > +Loading Data into Registers
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The `loader` device allows the initialization of CPU registers from the command
> > +line. This feature is particularly useful for setting up the processor state
> > +before starting an executable. By configuring registers prior to execution, the
> > +`loader` can mimic the state that a bootloader would leave the processor in
> > +before transferring control to an ELF file or another executable.
>
> This isn't really true though. A bootloader generally will set more
> than the GP registers. A boot loader will configure devices and
> perhaps initalise memory.
>
> > +
> > +The syntax for loading data into registers is as follows::
> > +
> > + -device loader,reg=<reg>,data=<data>,data-len=<data-len>
> > +
> > +**Parameters:**
> > +
> > +``<reg>``
> > + The target register to set. Format must pass the following regex
> > + ``[a-zA-Z]+[0-9]+``. The numeric part corresponds to the processor's GDB \
> > + register index. For general-purpose registers, this is typically the
> > + number in the register's name (e.g., ``r5`` translates to ``5``).
> > + Special-purpose registers have specific IDs defined in their processor's
> > + `gdbstub.c` file. Note that these IDs vary between processors.
> > +
> > +``<data>``
> > + The value to load into the specified register. The data must not exceed 8
> > + bytes in size.
>
> Why 8 bytes?
>
> > +
> > +``<data-len>``
> > + The length of the data in bytes. This parameter is mandatory when using
> > + the ``data`` argument.
>
> Do we need data-len? Why not just use the register size
>
> > +
> > +**Examples:**
> > +
> > +Set a general-purpose register
> > +""""""""""""""""""""""""""""""
> > +
> > +To set register ``r5`` to ``0xc0001000`` (4 bytes) on CPU 0::
> > +
> > + -device loader,reg=r5,data=0xc0001000,data-len=4
> > +
> > +Set a special register
> > +""""""""""""""""""""""
> > +
> > +To set the Program Counter (PC, register ``32``) to ``0x80000000`` on CPU 0::
> > +
> > + -device loader,reg=pc32,data=0x80000000,data-len=4
> > +
> > +You must look in your processor's `gdbstub.c` file to special register to index
> > +mappings.
>
> That isn't really helpful for users, but I don't have a better idea
>
> > +
> > +**Special Registers:**
> > +
> > +Special registers are defined in the processor's ``gdbstub.c`` file
> > with numeric IDs.
> > +Examples from the MicroBlaze processor at one point looked like. include::
> > +
> > + enum {
> > + GDB_PC = 32 + 0,
> > + GDB_MSR = 32 + 1,
> > + GDB_EAR = 32 + 2,
> > + GDB_ESR = 32 + 3,
> > + GDB_FSR = 32 + 4,
> > + GDB_BTR = 32 + 5,
> > + GDB_PVR0 = 32 + 6,
> > + GDB_PVR11 = 32 + 17,
> > + GDB_EDR = 32 + 18,
> > + GDB_SLR = 32 + 25,
> > + GDB_SHR = 32 + 26,
> > + };
> > +
> > +For example, to set the Machine State Register (``GDB_MSR``) on a MicroBlaze
> > +processor::
> > +
> > + -device loader,reg=MSR33,data=0x00000001,data-len=4
> > +
> > +**Register Loading Notes:**
> > +
> > +1. **Processor-Specific IDs**:
> > + The numeric IDs for registers vary between processors. Always refer to the
> > + `gdbstub.c` file for the target processor to identify the correct register
> > + mappings.
> > +
> > +2. **Pre-Execution State**:
> > + This capability is ideal for initializing a simulated environment to match
> > + the state expected by an ELF file. For example, you can configure stack
> > + pointers, machine state registers, and program counters to prepare the
> > + processor to run a bootstrapped application.
> > +
> > +3. **Validation**:
> > + Register numbers are validated by the `gdb_write_register` function. Ensure
> > + the specified register is supported by the target architecture.
> > +
> > +4. **Endianess**:
> > + The `data` value is written using the processor's native endian format.
> > +
> > +By using the `loader` device to initialize registers, you can simulate
> > +realistic execution environments, enabling detailed testing and debugging
> > +of embedded software, including bootloaders interactions and operating
> > +system kernels.
> > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > index ea8628b892..9408ecd150 100644
> > --- a/hw/core/generic-loader.c
> > +++ b/hw/core/generic-loader.c
> > @@ -55,6 +55,14 @@ static void generic_loader_reset(void *opaque)
> > }
> > }
> > + if(s->reg.name) {
> > + CPUClass *cc = CPU_GET_CLASS(s->cpu);
> > + int bytes_written = cc->gdb_write_register(s->cpu, (uint8_t*)
> > &s->reg.value, s->reg.num);
> > + if(bytes_written != s->reg.data_len) {
> > + printf("Error setting register %d to value %lX expected to write %d,
> > but wrote %d\n", s->reg.num, s->reg.value, s->reg.data_len,
> > bytes_written);
>
> The line wrapping is muddled up here. Can you please send it with git
> send-email. Do some sends against yourself to make sure it works.
>
> You mentioned gmail in an earlier thread I think, did you follow the
> instructions: https://git-scm.com/docs/git-send-email#_use_gmail_as_the_smtp_server
>
> Alistair



-- 
Sincerely,

Sam Price

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

[Philippe Mathieu-Daudé]

Hi Sam,

On 8/1/25 03:28, Sam Price wrote:
> I made the changes, and added documentation.
> https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699
> 
> I left it as [PREFIX]<RegNumber>
> 
> I can switch this to just RegNumber if desired.
> 
> I am still struggling with the email format sorry.

Possibly SourceHut can help you, see:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-you-cannot-send-patch-emails

> ---
> docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
> hw/core/generic-loader.c | 46 +++++++++++----
> include/hw/core/generic-loader.h | 7 +++
> 3 files changed, 139 insertions(+), 12 deletions(-)

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

[Alistair Francis]

On Fri, Jan 10, 2025 at 3:33 PM Sam Price <thesamprice@gmail.com> wrote:
>
> Yes that is true a boot loader will do more than just set registers.
> Ill rework the text a bit on the next update.
> In my case i need to set the r5 register that specifies the memory
> location to the device tree.

Should that be done in the machine instead? It seems tricky to expect
users to set this register

> I also use the device loader to load in a elf file to ram, and the
> device-loader to load in the device tree to the location specified by
> the r5 register
>
> I could add a gdb call that would return an array of string mappings
> to integers.
> If the machine doesn't implement the function/  leaves it as null
> pointer then you wouldn't get the cli support.
> Not sure where you would document all the machine register names /
> numbers at though.
> This might be too much though?

We probably don't need to document the register names

Alistair

> I left the door somewhat open on this via the NAME_NUMBER format.
>
> There was some checking logic where if data is supplied then it forces
> a check for data-len.
> I could relax that check if you supply the reg.name field.
>
> I am unsure how to determine the machine register size.
> I assumed the max register size on any machine would be 8 bytes, this
> might be wrong.
> the gdb call seems to just pass in the full 8 bytes, but I didn't dig
> into it for all machines.
>
> Ill look at this a bit more and try to configure the git email.
> I also need to set up a docker container to build /test latest.
> I have been building / testing on an old ubuntu machine.
> (To test this I need to run it on qemu-xilinx).
> My workplace has us on ubuntu 20.
>
> So it might be a while before I have another version up.
>
> Thanks,
> Sam
>
> On Thu, Jan 9, 2025 at 7:34 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 8, 2025 at 12:28 PM Sam Price <thesamprice@gmail.com> wrote:
> > >
> > > I made the changes, and added documentation.
> > > https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699
> > >
> > > I left it as [PREFIX]<RegNumber>
> > >
> > > I can switch this to just RegNumber if desired.
> > >
> > > I am still struggling with the email format sorry.
> > > ---
> > > docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
> > > hw/core/generic-loader.c | 46 +++++++++++----
> > > include/hw/core/generic-loader.h | 7 +++
> > > 3 files changed, 139 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst
> > > index 4f9fb005f1..71d4aaa097 100644
> > > --- a/docs/system/generic-loader.rst
> > > +++ b/docs/system/generic-loader.rst
> > > @@ -117,4 +117,102 @@ future the internal state 'set_pc' (which exists
> > > in the generic loader
> > > now) should be exposed to the user so that they can choose if the PC
> > > is set or not.
> > > +Loading Data into Registers
> > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +The `loader` device allows the initialization of CPU registers from the command
> > > +line. This feature is particularly useful for setting up the processor state
> > > +before starting an executable. By configuring registers prior to execution, the
> > > +`loader` can mimic the state that a bootloader would leave the processor in
> > > +before transferring control to an ELF file or another executable.
> >
> > This isn't really true though. A bootloader generally will set more
> > than the GP registers. A boot loader will configure devices and
> > perhaps initalise memory.
> >
> > > +
> > > +The syntax for loading data into registers is as follows::
> > > +
> > > + -device loader,reg=<reg>,data=<data>,data-len=<data-len>
> > > +
> > > +**Parameters:**
> > > +
> > > +``<reg>``
> > > + The target register to set. Format must pass the following regex
> > > + ``[a-zA-Z]+[0-9]+``. The numeric part corresponds to the processor's GDB \
> > > + register index. For general-purpose registers, this is typically the
> > > + number in the register's name (e.g., ``r5`` translates to ``5``).
> > > + Special-purpose registers have specific IDs defined in their processor's
> > > + `gdbstub.c` file. Note that these IDs vary between processors.
> > > +
> > > +``<data>``
> > > + The value to load into the specified register. The data must not exceed 8
> > > + bytes in size.
> >
> > Why 8 bytes?
> >
> > > +
> > > +``<data-len>``
> > > + The length of the data in bytes. This parameter is mandatory when using
> > > + the ``data`` argument.
> >
> > Do we need data-len? Why not just use the register size
> >
> > > +
> > > +**Examples:**
> > > +
> > > +Set a general-purpose register
> > > +""""""""""""""""""""""""""""""
> > > +
> > > +To set register ``r5`` to ``0xc0001000`` (4 bytes) on CPU 0::
> > > +
> > > + -device loader,reg=r5,data=0xc0001000,data-len=4
> > > +
> > > +Set a special register
> > > +""""""""""""""""""""""
> > > +
> > > +To set the Program Counter (PC, register ``32``) to ``0x80000000`` on CPU 0::
> > > +
> > > + -device loader,reg=pc32,data=0x80000000,data-len=4
> > > +
> > > +You must look in your processor's `gdbstub.c` file to special register to index
> > > +mappings.
> >
> > That isn't really helpful for users, but I don't have a better idea
> >
> > > +
> > > +**Special Registers:**
> > > +
> > > +Special registers are defined in the processor's ``gdbstub.c`` file
> > > with numeric IDs.
> > > +Examples from the MicroBlaze processor at one point looked like. include::
> > > +
> > > + enum {
> > > + GDB_PC = 32 + 0,
> > > + GDB_MSR = 32 + 1,
> > > + GDB_EAR = 32 + 2,
> > > + GDB_ESR = 32 + 3,
> > > + GDB_FSR = 32 + 4,
> > > + GDB_BTR = 32 + 5,
> > > + GDB_PVR0 = 32 + 6,
> > > + GDB_PVR11 = 32 + 17,
> > > + GDB_EDR = 32 + 18,
> > > + GDB_SLR = 32 + 25,
> > > + GDB_SHR = 32 + 26,
> > > + };
> > > +
> > > +For example, to set the Machine State Register (``GDB_MSR``) on a MicroBlaze
> > > +processor::
> > > +
> > > + -device loader,reg=MSR33,data=0x00000001,data-len=4
> > > +
> > > +**Register Loading Notes:**
> > > +
> > > +1. **Processor-Specific IDs**:
> > > + The numeric IDs for registers vary between processors. Always refer to the
> > > + `gdbstub.c` file for the target processor to identify the correct register
> > > + mappings.
> > > +
> > > +2. **Pre-Execution State**:
> > > + This capability is ideal for initializing a simulated environment to match
> > > + the state expected by an ELF file. For example, you can configure stack
> > > + pointers, machine state registers, and program counters to prepare the
> > > + processor to run a bootstrapped application.
> > > +
> > > +3. **Validation**:
> > > + Register numbers are validated by the `gdb_write_register` function. Ensure
> > > + the specified register is supported by the target architecture.
> > > +
> > > +4. **Endianess**:
> > > + The `data` value is written using the processor's native endian format.
> > > +
> > > +By using the `loader` device to initialize registers, you can simulate
> > > +realistic execution environments, enabling detailed testing and debugging
> > > +of embedded software, including bootloaders interactions and operating
> > > +system kernels.
> > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > > index ea8628b892..9408ecd150 100644
> > > --- a/hw/core/generic-loader.c
> > > +++ b/hw/core/generic-loader.c
> > > @@ -55,6 +55,14 @@ static void generic_loader_reset(void *opaque)
> > > }
> > > }
> > > + if(s->reg.name) {
> > > + CPUClass *cc = CPU_GET_CLASS(s->cpu);
> > > + int bytes_written = cc->gdb_write_register(s->cpu, (uint8_t*)
> > > &s->reg.value, s->reg.num);
> > > + if(bytes_written != s->reg.data_len) {
> > > + printf("Error setting register %d to value %lX expected to write %d,
> > > but wrote %d\n", s->reg.num, s->reg.value, s->reg.data_len,
> > > bytes_written);
> >
> > The line wrapping is muddled up here. Can you please send it with git
> > send-email. Do some sends against yourself to make sure it works.
> >
> > You mentioned gmail in an earlier thread I think, did you follow the
> > instructions: https://git-scm.com/docs/git-send-email#_use_gmail_as_the_smtp_server
> >
> > Alistair
>
>
>
> --
> Sincerely,
>
> Sam Price

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

[Philippe Mathieu-Daudé]

Cc'ing Alex/Pierrick

On 31/1/25 01:27, Alistair Francis wrote:
> On Fri, Jan 10, 2025 at 3:33 PM Sam Price <thesamprice@gmail.com> wrote:
>>
>> Yes that is true a boot loader will do more than just set registers.
>> Ill rework the text a bit on the next update.
>> In my case i need to set the r5 register that specifies the memory
>> location to the device tree.
> 
> Should that be done in the machine instead? It seems tricky to expect
> users to set this register
> 
>> I also use the device loader to load in a elf file to ram, and the
>> device-loader to load in the device tree to the location specified by
>> the r5 register
>>
>> I could add a gdb call that would return an array of string mappings
>> to integers.
>> If the machine doesn't implement the function/  leaves it as null
>> pointer then you wouldn't get the cli support.
>> Not sure where you would document all the machine register names /
>> numbers at though.
>> This might be too much though?
> 
> We probably don't need to document the register names
> 
> Alistair
> 
>> I left the door somewhat open on this via the NAME_NUMBER format.
>>
>> There was some checking logic where if data is supplied then it forces
>> a check for data-len.
>> I could relax that check if you supply the reg.name field.
>>
>> I am unsure how to determine the machine register size.
>> I assumed the max register size on any machine would be 8 bytes, this
>> might be wrong.
>> the gdb call seems to just pass in the full 8 bytes, but I didn't dig
>> into it for all machines.
>>
>> Ill look at this a bit more and try to configure the git email.
>> I also need to set up a docker container to build /test latest.
>> I have been building / testing on an old ubuntu machine.
>> (To test this I need to run it on qemu-xilinx).
>> My workplace has us on ubuntu 20.
>>
>> So it might be a while before I have another version up.
>>
>> Thanks,
>> Sam
>>
>> On Thu, Jan 9, 2025 at 7:34 PM Alistair Francis <alistair23@gmail.com> wrote:
>>>
>>> On Wed, Jan 8, 2025 at 12:28 PM Sam Price <thesamprice@gmail.com> wrote:
>>>>
>>>> I made the changes, and added documentation.
>>>> https://gitlab.com/thesamprice/qemu/-/compare/master...loader?from_project_id=11167699
>>>>
>>>> I left it as [PREFIX]<RegNumber>
>>>>
>>>> I can switch this to just RegNumber if desired.
>>>>
>>>> I am still struggling with the email format sorry.
>>>> ---
>>>> docs/system/generic-loader.rst | 98 ++++++++++++++++++++++++++++++++
>>>> hw/core/generic-loader.c | 46 +++++++++++----
>>>> include/hw/core/generic-loader.h | 7 +++
>>>> 3 files changed, 139 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst
>>>> index 4f9fb005f1..71d4aaa097 100644
>>>> --- a/docs/system/generic-loader.rst
>>>> +++ b/docs/system/generic-loader.rst
>>>> @@ -117,4 +117,102 @@ future the internal state 'set_pc' (which exists
>>>> in the generic loader
>>>> now) should be exposed to the user so that they can choose if the PC
>>>> is set or not.
>>>> +Loading Data into Registers
>>>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> +
>>>> +The `loader` device allows the initialization of CPU registers from the command
>>>> +line. This feature is particularly useful for setting up the processor state
>>>> +before starting an executable. By configuring registers prior to execution, the
>>>> +`loader` can mimic the state that a bootloader would leave the processor in
>>>> +before transferring control to an ELF file or another executable.
>>>
>>> This isn't really true though. A bootloader generally will set more
>>> than the GP registers. A boot loader will configure devices and
>>> perhaps initalise memory.
>>>
>>>> +
>>>> +The syntax for loading data into registers is as follows::
>>>> +
>>>> + -device loader,reg=<reg>,data=<data>,data-len=<data-len>
>>>> +
>>>> +**Parameters:**
>>>> +
>>>> +``<reg>``
>>>> + The target register to set. Format must pass the following regex
>>>> + ``[a-zA-Z]+[0-9]+``. The numeric part corresponds to the processor's GDB \
>>>> + register index. For general-purpose registers, this is typically the
>>>> + number in the register's name (e.g., ``r5`` translates to ``5``).
>>>> + Special-purpose registers have specific IDs defined in their processor's
>>>> + `gdbstub.c` file. Note that these IDs vary between processors.
>>>> +
>>>> +``<data>``
>>>> + The value to load into the specified register. The data must not exceed 8
>>>> + bytes in size.
>>>
>>> Why 8 bytes?
>>>
>>>> +
>>>> +``<data-len>``
>>>> + The length of the data in bytes. This parameter is mandatory when using
>>>> + the ``data`` argument.
>>>
>>> Do we need data-len? Why not just use the register size
>>>
>>>> +
>>>> +**Examples:**
>>>> +
>>>> +Set a general-purpose register
>>>> +""""""""""""""""""""""""""""""
>>>> +
>>>> +To set register ``r5`` to ``0xc0001000`` (4 bytes) on CPU 0::
>>>> +
>>>> + -device loader,reg=r5,data=0xc0001000,data-len=4
>>>> +
>>>> +Set a special register
>>>> +""""""""""""""""""""""
>>>> +
>>>> +To set the Program Counter (PC, register ``32``) to ``0x80000000`` on CPU 0::
>>>> +
>>>> + -device loader,reg=pc32,data=0x80000000,data-len=4
>>>> +
>>>> +You must look in your processor's `gdbstub.c` file to special register to index
>>>> +mappings.
>>>
>>> That isn't really helpful for users, but I don't have a better idea
>>>
>>>> +
>>>> +**Special Registers:**
>>>> +
>>>> +Special registers are defined in the processor's ``gdbstub.c`` file
>>>> with numeric IDs.
>>>> +Examples from the MicroBlaze processor at one point looked like. include::
>>>> +
>>>> + enum {
>>>> + GDB_PC = 32 + 0,
>>>> + GDB_MSR = 32 + 1,
>>>> + GDB_EAR = 32 + 2,
>>>> + GDB_ESR = 32 + 3,
>>>> + GDB_FSR = 32 + 4,
>>>> + GDB_BTR = 32 + 5,
>>>> + GDB_PVR0 = 32 + 6,
>>>> + GDB_PVR11 = 32 + 17,
>>>> + GDB_EDR = 32 + 18,
>>>> + GDB_SLR = 32 + 25,
>>>> + GDB_SHR = 32 + 26,
>>>> + };
>>>> +
>>>> +For example, to set the Machine State Register (``GDB_MSR``) on a MicroBlaze
>>>> +processor::
>>>> +
>>>> + -device loader,reg=MSR33,data=0x00000001,data-len=4
>>>> +
>>>> +**Register Loading Notes:**
>>>> +
>>>> +1. **Processor-Specific IDs**:
>>>> + The numeric IDs for registers vary between processors. Always refer to the
>>>> + `gdbstub.c` file for the target processor to identify the correct register
>>>> + mappings.
>>>> +
>>>> +2. **Pre-Execution State**:
>>>> + This capability is ideal for initializing a simulated environment to match
>>>> + the state expected by an ELF file. For example, you can configure stack
>>>> + pointers, machine state registers, and program counters to prepare the
>>>> + processor to run a bootstrapped application.
>>>> +
>>>> +3. **Validation**:
>>>> + Register numbers are validated by the `gdb_write_register` function. Ensure
>>>> + the specified register is supported by the target architecture.
>>>> +
>>>> +4. **Endianess**:
>>>> + The `data` value is written using the processor's native endian format.
>>>> +
>>>> +By using the `loader` device to initialize registers, you can simulate
>>>> +realistic execution environments, enabling detailed testing and debugging
>>>> +of embedded software, including bootloaders interactions and operating
>>>> +system kernels.
>>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>>>> index ea8628b892..9408ecd150 100644
>>>> --- a/hw/core/generic-loader.c
>>>> +++ b/hw/core/generic-loader.c
>>>> @@ -55,6 +55,14 @@ static void generic_loader_reset(void *opaque)
>>>> }
>>>> }
>>>> + if(s->reg.name) {
>>>> + CPUClass *cc = CPU_GET_CLASS(s->cpu);
>>>> + int bytes_written = cc->gdb_write_register(s->cpu, (uint8_t*)
>>>> &s->reg.value, s->reg.num);
>>>> + if(bytes_written != s->reg.data_len) {
>>>> + printf("Error setting register %d to value %lX expected to write %d,
>>>> but wrote %d\n", s->reg.num, s->reg.value, s->reg.data_len,
>>>> bytes_written);
>>>
>>> The line wrapping is muddled up here. Can you please send it with git
>>> send-email. Do some sends against yourself to make sure it works.
>>>
>>> You mentioned gmail in an earlier thread I think, did you follow the
>>> instructions: https://git-scm.com/docs/git-send-email#_use_gmail_as_the_smtp_server
>>>
>>> Alistair
>>
>>
>>
>> --
>> Sincerely,
>>
>> Sam Price
>