Re: [PATCH v1 09/18] x86: introduce abstractions for domain builder

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -0,0 +1,30 @@
> +#ifndef __ARCH_X86_BOOTDOMAIN_H__
> +#define __ARCH_X86_BOOTDOMAIN_H__
> +
> +struct memsize {
> +    long nr_pages;
> +    unsigned int percent;
> +    bool minus;
> +};
> +
> +static inline bool memsize_gt_zero(const struct memsize *sz)
> +{
> +    return !sz->minus && sz->nr_pages;
> +}
> +
> +static inline unsigned long get_memsize(
> +    const struct memsize *sz, unsigned long avail)
> +{
> +    unsigned long pages;
> +
> +    pages = sz->nr_pages + sz->percent * avail / 100;
> +    return sz->minus ? avail - pages : pages;
> +}

For both functions I think you should retain the __init, just in case
the compiler decides against actually inlining them (according to my
observations Clang frequently won't).

> +struct arch_domain_mem {
> +    struct memsize mem_size;
> +    struct memsize mem_min;
> +    struct memsize mem_max;
> +};

How come this is introduced here without the three respective Dom0
variables being replaced by an instance of this struct? At which
point a further question would be: What about dom0_mem_set?

> --- /dev/null
> +++ b/xen/include/xen/bootdomain.h
> @@ -0,0 +1,52 @@
> +#ifndef __XEN_BOOTDOMAIN_H__
> +#define __XEN_BOOTDOMAIN_H__
> +
> +#include <xen/bootinfo.h>
> +#include <xen/types.h>
> +
> +#include <public/xen.h>
> +#include <asm/bootdomain.h>
> +
> +struct domain;

Why the forward decl? There's no function being declared here, and
this is not C++.

> +struct boot_domain {
> +#define BUILD_PERMISSION_NONE          (0)
> +#define BUILD_PERMISSION_CONTROL       (1 << 0)
> +#define BUILD_PERMISSION_HARDWARE      (1 << 1)
> +    uint32_t permissions;

Why a fixed width type? And why no 'u' suffixes on the 1s being left
shifted above? (Same further down from here.)

> +#define BUILD_FUNCTION_NONE            (0)
> +#define BUILD_FUNCTION_BOOT            (1 << 0)
> +#define BUILD_FUNCTION_CRASH           (1 << 1)
> +#define BUILD_FUNCTION_CONSOLE         (1 << 2)
> +#define BUILD_FUNCTION_STUBDOM         (1 << 3)
> +#define BUILD_FUNCTION_XENSTORE        (1 << 30)
> +#define BUILD_FUNCTION_INITIAL_DOM     (1 << 31)
> +    uint32_t functions;
> +                                                /* On     | Off    */
> +#define BUILD_MODE_PARAVIRTUALIZED     (1 << 0) /* PV     | PVH/HVM */
> +#define BUILD_MODE_ENABLE_DEVICE_MODEL (1 << 1) /* HVM    | PVH     */
> +#define BUILD_MODE_LONG                (1 << 2) /* 64 BIT | 32 BIT  */

I guess bitness would better not be a boolean-like value (and "LONG"
is kind of odd anyway) - see RISC-V having provisions right away for
128-bit mode.

> --- /dev/null
> +++ b/xen/include/xen/domain_builder.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef XEN_DOMAIN_BUILDER_H
> +#define XEN_DOMAIN_BUILDER_H
> +
> +#include <xen/bootdomain.h>
> +#include <xen/bootinfo.h>
> +
> +#include <asm/setup.h>
> +
> +struct domain_builder {
> +    bool fdt_enabled;
> +#define BUILD_MAX_BOOT_DOMAINS 64
> +    uint16_t nr_doms;
> +    struct boot_domain domains[BUILD_MAX_BOOT_DOMAINS];
> +
> +    struct arch_domain_builder *arch;
> +};
> +
> +static inline bool builder_is_initdom(struct boot_domain *bd)

const wherever possible, please.

> +{
> +    return bd->functions & BUILD_FUNCTION_INITIAL_DOM;
> +}
> +
> +static inline bool builder_is_ctldom(struct boot_domain *bd)
> +{
> +    return (bd->functions & BUILD_FUNCTION_INITIAL_DOM ||
> +            bd->permissions & BUILD_PERMISSION_CONTROL );

Please parenthesize the operands of &, |, or ^ inside && or ||.

> +}
> +
> +static inline bool builder_is_hwdom(struct boot_domain *bd)
> +{
> +    return (bd->functions & BUILD_FUNCTION_INITIAL_DOM ||
> +            bd->permissions & BUILD_PERMISSION_HARDWARE );
> +}
> +
> +static inline struct domain *builder_get_hwdom(struct boot_info *info)
> +{
> +    int i;

unsigned int please when the value can't go negative.

> +    for ( i = 0; i < info->builder->nr_doms; i++ )
> +    {
> +        struct boot_domain *d = &info->builder->domains[i];
> +
> +        if ( builder_is_hwdom(d) )
> +            return d->domain;
> +    }
> +
> +    return NULL;
> +}
> +
> +void builder_init(struct boot_info *info);
> +uint32_t builder_create_domains(struct boot_info *info);

Both for these and for the inline functions - how is one to judge they
are (a) needed and (b) fit their purpose without seeing even a single
caller. And for the prototypes not even the implementation is there:
What's wrong with adding those at the time they're actually implemented
(and hopefully also used)?

Jan