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
