Re: [PATCH v2 37/39] xen/rirscv: add minimal amount of stubs to build full Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.11.2023 11:30, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -1,19 +1,23 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bug.h>
>  #include <xen/cache.h>
>  #include <xen/compiler.h>
>  #include <xen/init.h>
>  #include <xen/kernel.h>
>  #include <xen/macros.h>
> +#include <xen/mm.h>
>  #include <xen/pfn.h>
>  
>  #include <asm/early_printk.h>
>  #include <asm/csr.h>
>  #include <asm/current.h>
> -#include <asm/mm.h>
>  #include <asm/page.h>
>  #include <asm/processor.h>
>  
> +unsigned long frametable_base_pdx __read_mostly;
> +unsigned long frametable_virt_end __read_mostly;

Nit (style):

unsigned long __read_mostly frametable_base_pdx;
unsigned long __read_mostly frametable_virt_end;

(i.e. attributes generally between type and identifier). Plus
__read_mostly or __ro_after_init?

> @@ -294,3 +298,49 @@ unsigned long __init calc_phys_offset(void)
>      phys_offset = load_start - XEN_VIRT_START;
>      return phys_offset;
>  }
> +
> +void put_page(struct page_info *page)
> +{
> +    assert_failed(__func__);
> +}
> +
> +unsigned long get_upper_mfn_bound(void)
> +{
> +    /* No memory hotplug yet, so current memory limit is the final one. */
> +    return max_page - 1;
> +}
> +
> +void arch_dump_shared_mem_info(void)
> +{
> +    WARN();
> +}
> +
> +int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
> +{
> +    assert_failed(__func__);
> +    return -1;
> +}

Whats the pattern between picking WARN(), assert_failed() (which I don't
think you should be using anyway; if an assertion, then ASSERT_UNREACHABLE())
and BUG() (as used earlier in stubs living in header files)?

> --- /dev/null
> +++ b/xen/arch/riscv/stubs.c
> @@ -0,0 +1,426 @@
> +/* 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 <public/vm_event.h>

I think I can see why you need the former of these last two, but do you
really need the latter?

> +#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. */
> +
> +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 */
> +
> +void vm_event_fill_regs(vm_event_request_t *req)
> +{
> +    BUG();
> +}
> +
> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *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. */
> +}
> +
> +/* domctl.c */
> +
> +long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    BUG();
> +}
> +
> +void arch_get_domain_info(const struct domain *d,
> +                          struct xen_domctl_getdomaininfo *info)
> +{
> +    BUG();
> +}
> +
> +void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> +{
> +    BUG();
> +}
> +
> +/* monitor.c */
> +
> +int arch_monitor_domctl_event(struct domain *d,
> +                              struct xen_domctl_monitor_op *mop)
> +{
> +    BUG();
> +}
> +
> +/* smp.c */
> +
> +void arch_flush_tlb_mask(const cpumask_t *mask)
> +{
> +    BUG();
> +}
> +
> +void smp_send_event_check_mask(const cpumask_t *mask)
> +{
> +    BUG();
> +}
> +
> +void smp_send_call_function_mask(const cpumask_t *mask)
> +{
> +    BUG();
> +}
> +
> +/* irq.c */
> +
> +struct pirq *alloc_pirq_struct(struct domain *d)
> +{
> +    BUG();
> +}
> +
> +int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share)
> +{
> +    BUG();
> +}
> +
> +void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
> +{
> +    BUG();
> +}
> +
> +void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
> +{
> +    BUG();
> +}
> +
> +static void ack_none(struct irq_desc *irq)
> +{
> +    BUG();
> +}
> +
> +static void end_none(struct irq_desc *irq)
> +{
> +    BUG();
> +}

Much like I said for PPC - I don't think you need the two, as ...

> +hw_irq_controller no_irq_type = {
> +    .typename = "none",
> +    .startup = irq_startup_none,
> +    .shutdown = irq_shutdown_none,
> +    .enable = irq_enable_none,
> +    .disable = irq_disable_none,
> +    .ack = ack_none,
> +    .end = end_none

... there's nothing right now to invoke these hooks.

Jan