Re: [PATCH v3 32/34] xen/rirscv: add minimal amount of stubs to build full Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.12.2023 16:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/early_printk.c
> +++ b/xen/arch/riscv/early_printk.c
> @@ -40,171 +40,3 @@ void early_printk(const char *str)
>          str++;
>      }
>  }
> -
> -/*
> - * The following #if 1 ... #endif should be removed after printk
> - * and related stuff are ready.
> - */
> -#if 1
> -
> -#include <xen/stdarg.h>
> -#include <xen/string.h>
> -
> -/**
> - * strlen - Find the length of a string
> - * @s: The string to be sized
> - */
> -size_t (strlen)(const char * s)
> -{
> -    const char *sc;
> -
> -    for (sc = s; *sc != '\0'; ++sc)
> -        /* nothing */;
> -    return sc - s;
> -}
> -
> -/**
> - * memcpy - Copy one area of memory to another
> - * @dest: Where to copy to
> - * @src: Where to copy from
> - * @count: The size of the area.
> - *
> - * You should not use this function to access IO space, use memcpy_toio()
> - * or memcpy_fromio() instead.
> - */
> -void *(memcpy)(void *dest, const void *src, size_t count)
> -{
> -    char *tmp = (char *) dest, *s = (char *) src;
> -
> -    while (count--)
> -        *tmp++ = *s++;
> -
> -    return dest;
> -}
> -
> -int vsnprintf(char* str, size_t size, const char* format, va_list args)
> -{
> -    size_t i = 0; /* Current position in the output string */
> -    size_t written = 0; /* Total number of characters written */
> -    char* dest = str;
> -
> -    while ( format[i] != '\0' && written < size - 1 )
> -    {
> -        if ( format[i] == '%' )
> -        {
> -            i++;
> -
> -            if ( format[i] == '\0' )
> -                break;
> -
> -            if ( format[i] == '%' )
> -            {
> -                if ( written < size - 1 )
> -                {
> -                    dest[written] = '%';
> -                    written++;
> -                }
> -                i++;
> -                continue;
> -            }
> -
> -            /*
> -             * Handle format specifiers.
> -             * For simplicity, only %s and %d are implemented here.
> -             */
> -
> -            if ( format[i] == 's' )
> -            {
> -                char* arg = va_arg(args, char*);
> -                size_t arglen = strlen(arg);
> -
> -                size_t remaining = size - written - 1;
> -
> -                if ( arglen > remaining )
> -                    arglen = remaining;
> -
> -                memcpy(dest + written, arg, arglen);
> -
> -                written += arglen;
> -                i++;
> -            }
> -            else if ( format[i] == 'd' )
> -            {
> -                int arg = va_arg(args, int);
> -
> -                /* Convert the integer to string representation */
> -                char numstr[32]; /* Assumes a maximum of 32 digits */
> -                int numlen = 0;
> -                int num = arg;
> -                size_t remaining;
> -
> -                if ( arg < 0 )
> -                {
> -                    if ( written < size - 1 )
> -                    {
> -                        dest[written] = '-';
> -                        written++;
> -                    }
> -
> -                    num = -arg;
> -                }
> -
> -                do
> -                {
> -                    numstr[numlen] = '0' + num % 10;
> -                    num = num / 10;
> -                    numlen++;
> -                } while ( num > 0 );
> -
> -                /* Reverse the string */
> -                for (int j = 0; j < numlen / 2; j++)
> -                {
> -                    char tmp = numstr[j];
> -                    numstr[j] = numstr[numlen - 1 - j];
> -                    numstr[numlen - 1 - j] = tmp;
> -                }
> -
> -                remaining = size - written - 1;
> -
> -                if ( numlen > remaining )
> -                    numlen = remaining;
> -
> -                memcpy(dest + written, numstr, numlen);
> -
> -                written += numlen;
> -                i++;
> -            }
> -        }
> -        else
> -        {
> -            if ( written < size - 1 )
> -            {
> -                dest[written] = format[i];
> -                written++;
> -            }
> -            i++;
> -        }
> -    }
> -
> -    if ( size > 0 )
> -        dest[written] = '\0';
> -
> -    return written;
> -}
> -
> -void printk(const char *format, ...)
> -{
> -    static char buf[1024];
> -
> -    va_list args;
> -    va_start(args, format);
> -
> -    (void)vsnprintf(buf, sizeof(buf), format, args);
> -
> -    early_printk(buf);
> -
> -    va_end(args);
> -}
> -
> -#endif
> -

Aren't you transiently breaking the build by removing these here, rather
than in the next patch?

> --- /dev/null
> +++ b/xen/arch/riscv/stubs.c
> @@ -0,0 +1,422 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#include <xen/cpumask.h>
> +#include <xen/domain.h>
> +#include <xen/irq.h>
> +#include <xen/nodemask.h>
> +#include <xen/time.h>
> +#include <public/domctl.h>
> +
> +#include <asm/current.h>
> +
> +/* smpboot.c */
> +
> +cpumask_t cpu_online_map;
> +cpumask_t cpu_present_map;
> +cpumask_t cpu_possible_map;
> +
> +/* ID of the PCPU we're running on */
> +DEFINE_PER_CPU(unsigned int, cpu_id);
> +/* XXX these seem awfully x86ish... */
> +/* representing HT siblings of each logical CPU */
> +DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
> +/* representing HT and core siblings of each logical CPU */
> +DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
> +
> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> +
> +/* time.c */
> +
> +unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */

__ro_after_init?

> +s_time_t get_s_time(void)
> +{
> +    BUG();
> +}
> +
> +int reprogram_timer(s_time_t timeout)
> +{
> +    BUG();
> +}
> +
> +void send_timer_event(struct vcpu *v)
> +{
> +    BUG();
> +}
> +
> +void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
> +{
> +    BUG();
> +}
> +
> +/* shutdown.c */
> +
> +void machine_restart(unsigned int delay_millisecs)
> +{
> +    BUG();
> +}
> +
> +void machine_halt(void)
> +{
> +    BUG();
> +}
> +
> +/* vm_event.c */
> +
> +struct vm_event_st;
> +
> +void vm_event_fill_regs(struct vm_event_st *req)
> +{
> +    BUG();
> +}
> +
> +void vm_event_set_registers(struct vcpu *v, struct vm_event_st *rsp)
> +{
> +    BUG();
> +}
> +
> +void vm_event_monitor_next_interrupt(struct vcpu *v)
> +{
> +    /* Not supported on RISCV. */
> +}
> +
> +void vm_event_reset_vmtrace(struct vcpu *v)
> +{
> +    /* Not supported on RISCV. */
> +}

Judging from the comments these last two are in their final shape.
Wouldn't it make sense to put them in vm_event.c right away then? And
then perhaps together with the two stubs?

Yet then - Arm gets away without vm_event_reset_vmtrace()? Can you
explain why the same isn't true for RISC-V?

> @@ -11,3 +15,24 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>  {
>      die();
>  }
> +
> +void vcpu_show_execution_state(struct vcpu *v)
> +{
> +    assert_failed("need to be implented");

Just to mention it again - the expectation is that all instances will
use the same "canonical" pattern for identifying yet-to-be-implemented
functions.

Jan