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

[liweiwei <liweiwei@iscas.ac.cn>]

On 2023/4/6 08:36, Alistair Francis wrote:
> On Wed, Apr 5, 2023 at 7:58 PM Weiwei Li <liweiwei@iscas.ac.cn> 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>
>> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   hw/char/riscv_htif.c         | 44 +++++++++++++++++++++++++++++++++++-
>>   hw/riscv/spike.c             | 13 +++++++++++
>>   include/hw/char/riscv_htif.h |  3 +++
>>   3 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
>> index 098de50e35..37d3ccc76b 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;
>> +const char *sig_file;
>> +uint8_t line_size = 16;
>> +
>> +static uint64_t fromhost_addr, tohost_addr, begin_sig_addr, end_sig_addr;
>>
>>   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) {
>> +        begin_sig_addr = st_value;
>> +    } else if (strcmp("end_signature", st_name) == 0) {
>> +        end_sig_addr = st_value;
>>       }
>>   }
>>
>> @@ -163,6 +172,39 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>>               if (payload & 0x1) {
>>                   /* exit code */
>>                   int exit_code = payload >> 1;
>> +
>> +                /*
>> +                 * Dump signature data if sig_file is specified and
>> +                 * begin/end_signature symbols exist.
>> +                 */
>> +                if (sig_file && begin_sig_addr && end_sig_addr) {
> There is no guarantee that these are initalised to zero, so this isn't
> really checking anything is it?

I think the static global variable will be  initialized to zero by default.

If not,  fromhost_addr and tohost_addr may have the same problem.

Regards,

Weiwei Li

>
> Alistair
>
>> +                    uint64_t sig_len = end_sig_addr - begin_sig_addr;
>> +                    char *sig_data = g_malloc(sig_len);
>> +                    dma_memory_read(&address_space_memory, begin_sig_addr,
>> +                                    sig_data, sig_len, MEMTXATTRS_UNSPECIFIED);
>> +                    FILE *signature = fopen(sig_file, "w");
>> +                    if (signature == NULL) {
>> +                        error_report("Unable to open %s with error %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);
>> +                            } else {
>> +                                fprintf(signature, "%02x", 0);
>> +                            }
>> +                        }
>> +                        fprintf(signature, "\n");
>> +                    }
>> +
>> +                    fclose(signature);
>> +                    g_free(sig_data);
>> +                }
>> +
>>                   exit(exit_code);
>>               } else {
>>                   uint64_t syscall[8];
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index a584d5b3a2..2c5546560a 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -332,6 +332,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 +355,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 = {
>> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
>> index 5958c5b986..df493fdf6b 100644
>> --- a/include/hw/char/riscv_htif.h
>> +++ b/include/hw/char/riscv_htif.h
>> @@ -40,6 +40,9 @@ typedef struct HTIFState {
>>       uint64_t pending_read;
>>   } HTIFState;
>>
>> +extern const char *sig_file;
>> +extern uint8_t line_size;
>> +
>>   /* HTIF symbol callback */
>>   void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>>       uint64_t st_size);
>> --
>> 2.25.1
>>
>>