Re: [PATCH v1 02/27] xen/riscv: Implement construct_domain()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/24/26 10:37 AM, Jan Beulich wrote:
> On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/domain-build.c
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/fdt-domain-build.h>
> > +#include <xen/fdt-kernel.h>
> > +#include <xen/init.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/current.h>
> > +#include <asm/guest_access.h>
> > +
> > +int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
> Are you actually altering what kinfo points to?
Not directly in this function, but it could be altered, for example, by 
kernel_image_load() where "info->entry = load_addr" is happening.
> > +{
> > +    struct vcpu *v = d->vcpu[0];
> > +    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(v);
> > +
> > +    BUG_ON(d->vcpu[0] == NULL);
> Why not simply "!v"?
It could work. I'll apply that.

> Also, while in the cover letter you state a dependency on another series,
> this is somewhat unwieldy here. From the titles there I can't deduce which
> of the patches would introduce vcpu_guest_cpu_user_regs(). Yet I would
> have wanted to double check that it doesn't de-reference v already.
It was already merged. It was part of:
 xen/riscv: implement vcpu_csr_init() "02b3a1b0e53c"

> > +    BUG_ON(v->is_initialised);
> > +
> > +    kernel_load(kinfo);
> > +    initrd_load(kinfo, copy_to_guest_phys);
> > +    dtb_load(kinfo, copy_to_guest_phys);
> These all return void, despite this also being used for non-Dom0. Is it
> really fatal to a dom0less system if one out of many domains fail to be
> built?
For a dom0less system, my opinion is that it should not be fatal, it 
should simply ignore a domain that fails to build and continue with the 
rest. However, with the current common dom0less code it will just 
panic(). This is a behavior I would like to change and it is on my TODO 
list.
Regarding the functions returning void, this is because all of them 
currently call panic() on failure, which I expect will need to change in 
order to ignore a domain that fails to build in dom0less mode.
For the current implementation of the common dom0less code this is fine, 
but I agree it should be addressed in a separate patch series.
 Especially when, despite the name, there is a Dom0?

For this case, a failure there should indeed be fatal, so panic() is 
appropriate.
> > +    regs->sepc = kinfo->entry;
> > +
> > +    /* Guest boot cpuid = 0 */
> > +    regs->a0 = 0;
> > +    regs->a1 = kinfo->dtb_paddr;
> > +
> > +    for ( unsigned int i = 1; i < d->max_vcpus; i++ )
> > +    {
> > +        if ( vcpu_create(d, i) == NULL )
> > +        {
> > +            printk("Failed to allocate %pd v%d\n", d, i);
> > +            break;
> And no error is indicated to the caller?
No, as generally it is enough to have only one vCPU0 to run domain, so 
we have to print that something went wrong with allocation of vCPU1...n 
but it is okay to me to continue domain construction.
Thanks.

~ Oleksii