Re: [PATCH v1 17/18] builder: introduce domain builder hypfs tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -134,6 +134,9 @@ uint32_t __init builder_create_domains(struct boot_info 
> *info)
>          /* Free temporary buffers. */
>          discard_initial_images();
>  
> +    if ( IS_ENABLED(CONFIG_BUILDER_HYPFS) )
> +        builder_hypfs(info);

No need for the if() here when you provide a stub in the header file.
Just that of course the stub vs prototype there needs to depend on
CONFIG_BUILDER_HYPFS, not CONFIG_HYPFS.

> +static int __init alloc_hypfs(struct boot_info *info)
> +{
> +    if ( !(builder_dir = (struct hypfs_entry_dir *)xmalloc_bytes(
> +                        sizeof(struct hypfs_entry_dir))) )

Why not xmalloc() (or xzalloc()), which specifically exists to avoid
open-coded casts like the one here?

> +    {
> +        printk(XENLOG_WARNING "%s: unable to allocate hypfs dir\n", 
> __func__);
> +        return -ENOMEM;
> +    }
> +
> +    builder_dir->e.type = XEN_HYPFS_TYPE_DIR;
> +    builder_dir->e.encoding = XEN_HYPFS_ENC_PLAIN;
> +    builder_dir->e.name = "builder";
> +    builder_dir->e.size = 0;
> +    builder_dir->e.max_size = 0;
> +    INIT_LIST_HEAD(&builder_dir->e.list);
> +    builder_dir->e.funcs = &hypfs_dir_funcs;
> +    INIT_LIST_HEAD(&builder_dir->dirlist);
> +
> +    if ( !(entries = (struct domain_node *)xmalloc_bytes(
> +                        sizeof(struct domain_node) * 
> info->builder->nr_doms)) )

xmalloc_array()?

> +    {
> +        printk(XENLOG_WARNING "%s: unable to allocate hypfs nodes\n", 
> __func__);
> +        return -ENOMEM;
> +    }
> +
> +    return 0;
> +}
> +
> +void __init builder_hypfs(struct boot_info *info)
> +{
> +    int i;
> +
> +    printk("Domain Builder: creating hypfs nodes\n");

If at all, then dprintk().

> +    if ( alloc_hypfs(info) != 0 )
> +        return;
> +
> +    for ( i = 0; i < info->builder->nr_doms; i++ )
> +    {
> +        struct domain_node *e = &entries[i];
> +        struct boot_domain *bd = &info->builder->domains[i];
> +        uint8_t *uuid = bd->uuid;
> +
> +        snprintf(e->dir_name, sizeof(e->dir_name), "%d", bd->domid);
> +
> +        snprintf(e->uuid, sizeof(e->uuid), "%08x-%04x-%04x-%04x-%04x%08x",
> +                 *(uint32_t *)uuid, *(uint16_t *)(uuid+4),
> +                 *(uint16_t *)(uuid+6), *(uint16_t *)(uuid+8),
> +                 *(uint16_t *)(uuid+10), *(uint32_t *)(uuid+12));

Perhaps better introduce a properly typed structure? Endian-ness-wise
I'm also unsure about the last 12 nibbles: Isn't this an array of bytes
really? Actually the second-to-last 16-bit item is an array of two
bytes as well, if Linux'es %pU vsprintf() formatting is to be trusted.

> +        e->functions = bd->functions;
> +        e->constructed = bd->constructed;
> +
> +        e->ncpus = bd->ncpus;
> +        e->mem_size = (bd->meminfo.mem_size.nr_pages * PAGE_SIZE)/1024;
> +        e->mem_max = (bd->meminfo.mem_max.nr_pages * PAGE_SIZE)/1024;

Nit: Blanks around / please.

> +        e->xs.evtchn = bd->store.evtchn;
> +        e->xs.mfn = bd->store.mfn;
> +
> +        e->con_dev.evtchn = bd->console.evtchn;
> +        e->con_dev.mfn = bd->console.mfn;
> +
> +        /* Initialize and construct builder hypfs tree */
> +        INIT_HYPFS_DIR(e->dir, e->dir_name);
> +        INIT_HYPFS_DIR(e->xs.dir, "xenstore");
> +        INIT_HYPFS_DIR(e->dev_dir, "devices");
> +        INIT_HYPFS_DIR(e->con_dev.dir, "console");
> +
> +        INIT_HYPFS_STRING(e->uuid_leaf, "uuid");
> +        hypfs_string_set_reference(&e->uuid_leaf, e->uuid);
> +        INIT_HYPFS_UINT(e->func_leaf, "functions", e->functions);
> +        INIT_HYPFS_UINT(e->ncpus_leaf, "ncpus", e->ncpus);
> +        INIT_HYPFS_UINT(e->mem_sz_leaf, "mem_size", e->mem_size);
> +        INIT_HYPFS_UINT(e->mem_mx_leaf, "mem_max", e->mem_max);

May I suggest to prefer - over _ in node names?

> --- a/xen/include/xen/domain_builder.h
> +++ b/xen/include/xen/domain_builder.h
> @@ -72,4 +72,17 @@ int alloc_system_evtchn(
>      const struct boot_info *info, struct boot_domain *bd);
>  void arch_create_dom(const struct boot_info *bi, struct boot_domain *bd);
>  
> +#ifdef CONFIG_HYPFS
> +
> +void builder_hypfs(struct boot_info *info);
> +
> +#else
> +
> +static inline void builder_hypfs(struct boot_info *info)
> +{
> +    return;
> +}
> +
> +#endif

This would better go in a private header in xen/common/domain-builder/.

Jan