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
