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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/9/26 2:58 PM, Jan Beulich wrote:
> > > > +{
> > > > +    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"
> Oh, indeed. Which makes clear that the BUG_ON() comes too late.
It makes this BUG_ON() unnessary at all if d->vcpu[0] wasn't created 
then construct_domain() won't be called for such domain.
I will just drop this BUG_ON().

> > > > +    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.
> I think you misunderstood. I wasn't referring to the building of Dom0
> failing. Was rather emphasizing that when there is a Dom0, failure to
> create a DomU likely should even less so be fatal, as Dom0 could later
> rectify the situation.
Oh, okay, then it is really less fatal if DomU creation will fail in the 
case of Dom0.
> > > > +    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.
> Hmm, now that I look there, sched_setup_dom0_vcpus() ignores errors
> and doesn't even emit a log message. Question is why neither Arm nor
> RISC-V use that function, when we have it.
I haven't seen this function, I will re-use it and in separate patch 
suggest to re-use it for Arm.
One thing I think we want to do then is to drop #ifdef x86 around 
sched_setup_dom0_vcpus() and rename it to sched_setup_dom_vcpus().
And maybe add dprintk()'s to provide some information about which vCPUs 
were created and which not.
~ Oleksii