Re: [PATCH 1/1] hw/riscv: Add signature dump function for spike to run ACT tests

[liweiwei]

On 2023/3/6 19:00, LIU Zhiwei wrote:
> On 2023/3/6 17:03, Weiwei Li wrote:
> > Add signature and signature-granularity properties in spike to 
> > specify the target
> > signatrue file and the line size for signature data.
> >
> > Recgonize the signature section between begin_signature and 
> > end_signature symbols
> > when loading elf of ACT tests. Then dump signature data in signature 
> > section just
> > before the ACT tests exit.
> >
> > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > ---
> >   hw/char/riscv_htif.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >   hw/riscv/spike.c     | 16 ++++++++++++++++
> >   2 files changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> > index 098de50e35..2a82ed8500 100644
> > --- a/hw/char/riscv_htif.c
> > +++ b/hw/char/riscv_htif.c
> > @@ -29,6 +29,8 @@
> >   #include "chardev/char-fe.h"
> >   #include "qemu/timer.h"
> >   #include "qemu/error-report.h"
> > +#include "exec/address-spaces.h"
> > +#include "sysemu/dma.h"
> >     #define RISCV_DEBUG_HTIF 0
> >   #define HTIF_DEBUG(fmt, 
> > ...)                                                   \
> > @@ -51,7 +53,10 @@
> >   /* PK system call number */
> >   #define PK_SYS_WRITE            64
> >   -static uint64_t fromhost_addr, tohost_addr;
> > +extern const char *sig_file;
> > +extern uint8_t line_size;
> > +
> Why not declare them in riscv_htif.h and include them in 
> hw/riscv/spike.c?

Do you mean the above "extern ..." declaration? It's OK to move them to 
riscv_hitf.h.

However, we can not move the definition in spike.c to riscv_htif.h. 
Otherwise, it'll trigger

multiple definition error.

> > +static uint64_t fromhost_addr, tohost_addr, sig_addr, sig_len;
> >     void htif_symbol_callback(const char *st_name, int st_info, 
> > uint64_t st_value,
> >                             uint64_t st_size)
> > @@ -68,6 +73,10 @@ void htif_symbol_callback(const char *st_name, int 
> > st_info, uint64_t st_value,
> >               error_report("HTIF tohost must be 8 bytes");
> >               exit(1);
> >           }
> > +    } else if (strcmp("begin_signature", st_name) == 0) {
> > +        sig_addr = st_value;
> > +    } else if (strcmp("end_signature", st_name) == 0) {
> > +        sig_len = st_value - sig_addr;
> >       }
> >   }
> >   @@ -161,6 +170,34 @@ static void htif_handle_tohost_write(HTIFState 
> > *s, uint64_t val_written)
> >           /* frontend syscall handler, shutdown and exit code support */
> >           if (cmd == HTIF_SYSTEM_CMD_SYSCALL) {
> >               if (payload & 0x1) {
> > +                /* Dump signature data to sig_file if specified */
> > +                if (sig_file) {
> > +                    char *sig_data = g_malloc(sig_len);
> > +                    dma_memory_read(&address_space_memory, sig_addr, 
> > sig_data,
> > +                                    sig_len, MEMTXATTRS_UNSPECIFIED);
> > +                    FILE *signature = fopen(sig_file, "w");
> > +                    if (signature == NULL) {
> > +                        error_report("open %s: %s", sig_file,
> > +                                     strerror(errno));
> > +                        exit(1);
> > +                    }
> > +
> > +                    for (int i = 0; i < sig_len; i += line_size) {
> > +                        for (int j = line_size; j > 0; j--) {
> > +                            if (i + j <= sig_len) {
> > +                                fprintf(signature, "%02x",
> > +                                        sig_data[i + j - 1] & 0xff);
>
> Not sure about the order. Otherwise, 

It will put the higher data(at higher address) before the lower data in 
the same line,

just as the htif logic in riscv-isa-sim(spike).

Regards,

Weiwei Li

> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>
> Zhiwei
>
> > +                            } else {
> > +                                fprintf(signature, "%02x", 0);
> > +                            }
> > +                        }
> > +                        fprintf(signature, "\n");
> > +                    }
> > +
> > +                    fclose(signature);
> > +                    g_free(sig_data);
> > +                }
> > +
> >                   /* exit code */
> >                   int exit_code = payload >> 1;
> >                   exit(exit_code);
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index a584d5b3a2..eaa7f54fd6 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -41,6 +41,9 @@
> >     #include <libfdt.h>
> >   +const char *sig_file;
> > +uint8_t line_size = 16;
> > +
> >   static const MemMapEntry spike_memmap[] = {
> >       [SPIKE_MROM] =     {     0x1000,     0xf000 },
> >       [SPIKE_HTIF] =     {  0x1000000,     0x1000 },
> > @@ -332,6 +335,11 @@ static void spike_board_init(MachineState *machine)
> >                    htif_custom_base);
> >   }
> >   +static void spike_set_signature(Object *obj, const char *val, 
> > Error **errp)
> > +{
> > +    sig_file = g_strdup(val);
> > +}
> > +
> >   static void spike_machine_instance_init(Object *obj)
> >   {
> >   }
> > @@ -350,6 +358,14 @@ static void spike_machine_class_init(ObjectClass 
> > *oc, void *data)
> >       mc->get_default_cpu_node_id = riscv_numa_get_default_cpu_node_id;
> >       mc->numa_mem_supported = true;
> >       mc->default_ram_id = "riscv.spike.ram";
> > +    object_class_property_add_str(oc, "signature", NULL, 
> > spike_set_signature);
> > +    object_class_property_set_description(oc, "signature",
> > +                                          "File to write ACT test 
> > signature");
> > +    object_class_property_add_uint8_ptr(oc, "signature-granularity",
> > +                                        &line_size, 
> > OBJ_PROP_FLAG_WRITE);
> > +    object_class_property_set_description(oc, "signature-granularity",
> > +                                          "Size of each line in ACT 
> > signature "
> > +                                          "file");
> >   }
> >     static const TypeInfo spike_machine_typeinfo = {