[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 09/18] x86: introduce abstractions for domain builder
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |