Re: [PATCH v1 03/27] xen/riscv: implement prerequisites for domain_create()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 4/1/26 2:57 PM, Jan Beulich wrote:
> On 10.03.2026 18:08, Oleksii Kurochko wrote:
>> arch_domain_create() and arch_sanitise_domain_config() are prerequisites for
>> domain_create().
>>
>> arch_sanitise_domain_config() currently returns 0, as there is no specific
>> work required at this stage.
>>
>> arch_domain_create() performs basic initialization, such as setting up the P2M
>> and initializing the domain's virtual timer.
> 
> Does it? I can spot only the former; instead there is ...

Stale comment, I will drop that as vtimer init is called per vCPU.

> 
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -288,6 +288,33 @@ void sync_vcpu_execstate(struct vcpu *v)
>>       /* Nothing to do -- no lazy switching */
>>   }
>>   
>> +int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> +{
>> +    return 0;
>> +}
>> +
>> +int arch_domain_create(struct domain *d,
>> +                       struct xen_domctl_createdomain *config,
>> +                       unsigned int flags)
>> +{
>> +    int rc = 0;
>> +
>> +    if ( is_idle_domain(d) )
>> +        return 0;
>> +
>> +    if ( (rc = p2m_init(d)) != 0)
>> +        goto fail;
>> +
>> +    d->arch.next_phandle = GUEST_PHANDLE_LAST + 1;
> 
> ... this, which I can't make any sense of. I can't find matching Arm code
> either, which might otherwise have helped.

There are some cases when we are creating a node for guest DTB we should 
know which phandles aren't busy (to avoid the case when some nodes have 
the same phandle) so to track that this struct field was introduced.

I will check Arm code as I expect that phandle should be generated 
somehow on their side too.

> 
>> +    return rc;
>> +
>> + fail:
>> +    d->is_dying = DOMDYING_dead;
>> +    arch_domain_destroy(d);
> 
> (At least) for the use here, that other function would better also move out
> of stubs.c at the same time (and no longer have unconditional BUG_ON() in it).
> 
>> --- a/xen/arch/riscv/stubs.c
>> +++ b/xen/arch/riscv/stubs.c
>> @@ -101,14 +101,7 @@ void dump_pageframe_info(struct domain *d)
>>       BUG_ON("unimplemented");
>>   }
>>   
>> -int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> -{
>> -    BUG_ON("unimplemented");
>> -}
>> -
>> -int arch_domain_create(struct domain *d,
>> -                       struct xen_domctl_createdomain *config,
>> -                       unsigned int flags)
>> +void vcpu_switch_to_aarch64_mode(struct vcpu *v)
> 
> What is this? Surely nothing with this name should exist under riscv/.

Good question. I don't know how it appeared here. I will drop it.

> 
>> --- a/xen/include/public/device_tree_defs.h
>> +++ b/xen/include/public/device_tree_defs.h
>> @@ -14,6 +14,7 @@
>>    */
>>   #define GUEST_PHANDLE_GIC (65000)
>>   #define GUEST_PHANDLE_IOMMU (GUEST_PHANDLE_GIC + 1)
>> +#define GUEST_PHANDLE_LAST GUEST_PHANDLE_IOMMU
> 
> This, to me, looks like a questionable addition to the public interface.
> Yet I'm not a DT person, so I may simply be missing why something like
> this might be wanted (and how stable it then would be, long term).

I will double check if I have to generate phandle for the node I'm 
creating for guest DTB. It looks like I am confused something and it 
could be just dropped.

Considering that only RISC-V needs that at the moment (if need it all?) 
I will put somewhere in riscv/ this definition.

Thanks.

~ Oleksii.