Re: [PATCH v3 8/8] xen/common: dom0less: introduce common dom0less-build.c

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Oleksii,

FYI I know you might not be able to disable HTML in your email client
replies, but just as a heads up, my email client doesn't support HTML at
all so my replies will have your text and my older text mixed up.


On Mon, 5 May 2025, Oleksii Kurochko wrote:
> On 5/2/25 10:53 PM, Stefano Stabellini wrote:
> 
> On Fri, 2 May 2025, Oleksii Kurochko wrote:
> 
> Part of Arm's dom0less-build.c could be common between architectures which are
> using device tree files to create guest domains. Thereby move some parts of
> Arm's dom0less-build.c to common code with minor changes.
> 
> As a part of theses changes the following changes are introduced:
> - Introduce make_arch_nodes() to cover arch-specific nodes. For example, in
>   case of Arm, it is PSCI and vpl011 nodes.
> - Introduce set_domain_type() to abstract a way how setting of domain type
>   happens. For example, RISC-V won't have this member of arch_domain structure
>   as vCPUs will always have the same bitness as hypervisor. In case of Arm, it
>   is possible that Arm64 could create 32-bit and 64-bit domains.
> - Introduce init_vuart() to cover details of virtual uart initialization.
> - Introduce init_intc_phandle() to cover some details of interrupt controller
>   phandle initialization. As an example, RISC-V could have different name for
>   interrupt controller node ( APLIC, PLIC, IMSIC, etc ) but the code in
>   domain_handle_dtb_bootmodule() could handle only one interrupt controller
>   node name.
> - s/make_gic_domU_node/make_intc_domU_node as GIC is Arm specific naming and
>   add prototype of make_intc_domU_node() to dom0less-build.h
> 
> The following functions are moved to xen/common/device-tree:
> - Functions which are moved as is:
>   - domain_p2m_pages().
>   - handle_passthrough_prop().
>   - handle_prop_pfdt().
>   - scan_pfdt_node().
>   - check_partial_fdt().
> - Functions which are moved with some minor changes:
>   - alloc_xenstore_evtchn():
>     - ifdef-ing by CONFIG_HVM accesses to hvm.params.
>   - prepare_dtb_domU():
>     - ifdef-ing access to gnttab_{start,size} by CONFIG_GRANT_TABLE.
>     - s/make_gic_domU_node/make_intc_domU_node.
>     - Add call of make_arch_nodes().
> - domain_handle_dtb_bootmodule():
>   - hide details of interrupt controller phandle initialization by calling
>     init_intc_phandle().
>   - Update the comment above init_intc_phandle(): s/gic/interrupt controller.
> - construct_domU():
>   - ifdef-ing by CONFIG_HVM accesses to hvm.params.
>   - Call init_vuart() to hide Arm's vpl011_init() details there.
>   - Add call of set_domain_type() instead of setting kinfo->arch.type 
> explicitly.
> 
> Some parts of dom0less-build.c are wraped by #ifdef 
> CONFIG_STATIC_{SHMEM,MEMORY}
> as not all archs support these configs.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

[...]


> +    ret = make_memory_node(kinfo, addrcells, sizecells,
> +                           kernel_info_get_mem(kinfo));
> +    if ( ret )
> +        goto err;
> +
> +#ifdef CONFIG_STATIC_SHM
> 
> This should not be necessary because there is a stub implementation of
> make_resv_memory_node available in static-shmem.h for the
> !CONFIG_STATIC_SHM case.
> 
> But static-shmem.h isn't available on all architectures. Until static-shmem.h 
> isn't moved to
> asm-generic or xen folders and then re-used by an architecture we have to 
> have #ifdef.
 
OK let's keep it as is so that we don't need to move static-shmem.h

 
> +    ret = make_resv_memory_node(kinfo, addrcells, sizecells);
> +    if ( ret )
> +        goto err;
> +#endif

[...]


> +    if ( !dt_find_property(node, "xen,static-mem", NULL) )
> +        allocate_memory(d, &kinfo);
> +#ifdef CONFIG_STATIC_MEMORY
> 
> Also this should not be needed thanks to the stub implementation of
> allocate_static_memory and assign_static_memory_11
> 
> 
> +    else if ( !is_domain_direct_mapped(d) )
> +        allocate_static_memory(d, &kinfo, node);
> +    else
> +        assign_static_memory_11(d, &kinfo, node);
> +#endif
> +
> +#ifdef CONFIG_STATIC_SHM
> 
> There is a stub for process_shm too
> 
> The same as with make_resv_memory_node(), static-shmem.h header isn't 
> available for
> all archs.
> I can return my patches which move static-shmem.h to asm-generic and then 
> drop all the ifdef-s connect to it:
> https://lore.kernel.org/xen-devel/0203b98aa6a42aa69e22e7c973320add3ff4bb5d.1736334615.git.oleksii.kurochko@xxxxxxxxx/
> https://lore.kernel.org/xen-devel/0203b98aa6a42aa69e22e7c973320add3ff4bb5d.1736334615.git.oleksii.kurochko@xxxxxxxxx/
> 
> Let me know if it is better to do now or should it be better to drop 
> #ifdef-ing when an architrecture will require
> static-shmem or static-mem features?

I see Jan's point that they are advanced features probably not needed
initially. So maybe it is better to start with something simpler. I
think it is OK to keep the patch as is.