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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.04.2026 13:26, Oleksii Kurochko wrote:
> 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.

Ah, I see.

>>> +{
>>> +    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.

>>> +    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.

>>> +    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.

Jan