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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-12-18 at 18:00 +0100, Jan Beulich wrote:
> 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?
I'll update the style.
I looked at code where this variables are used and they can be
__ro_after_init.
Thanks.
> 
> > @@ -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)?
There is no specific pattern; initially, I used WARN() everywhere.
However, when the time came to implement this function, it became
challenging to identify the location of some WARN() occurrences.
Consequently, I started changing them to assert_failed(__func__) to
pinpoint the source.

I'll be switching to BUG()
> 
> > --- /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?
It is needed for vm_event_request_t and vm_event_response_t, but if use
a forward declaration that it won't be needed:

typedef struct vm_event_st vm_event_request_t;
typedef struct vm_event_st vm_event_response_t;
> 
> > +#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.
They really can be dropped.
I'll take into account that.

Thanks.


~ Oleksii