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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-01-23 at 14:20 +0100, Jan Beulich wrote:
> 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?
It should break. I'll double check. If it will, then I'll move these
changes 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?
Yes, it should be __ro_after_init. Thanks.

> 
> > +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?
Yes, it makes sense.

> 
> Yet then - Arm gets away without vm_event_reset_vmtrace()? Can you
> explain why the same isn't true for RISC-V?
It should true and for RISC-V. I'll look at when they gets away
vm_event_reset_vmtrace().

> 
> > @@ -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.
Sure, I'll update using BUG_ON("unimplemented");

Thanks for review.

~ Oleksii