diff mbox series

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

Message ID 20220706210454.30096-10-dpsmith@apertussolutions.com (mailing list archive)
State New, archived
Headers show
Series Hyperlaunch | expand

Commit Message

Daniel P. Smith July 6, 2022, 9:04 p.m. UTC 
This commit expands the new boot info structs to provide the initial
abstractions for domain builder.  Additionally, it reuses the memory allocation
structures previously used for dom0, bring the structures and helper functions
under the domain builder.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Christopher Clark <christopher.clark@starlab.io>
---
 xen/arch/x86/boot/boot_info32.h       |  3 ++
 xen/arch/x86/dom0_build.c             | 21 +---------
 xen/arch/x86/include/asm/bootdomain.h | 30 +++++++++++++++
 xen/arch/x86/include/asm/bootinfo.h   |  2 +
 xen/include/xen/bootdomain.h          | 52 +++++++++++++++++++++++++
 xen/include/xen/bootinfo.h            |  4 ++
 xen/include/xen/domain_builder.h      | 55 +++++++++++++++++++++++++++
 7 files changed, 147 insertions(+), 20 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/bootdomain.h
 create mode 100644 xen/include/xen/bootdomain.h
 create mode 100644 xen/include/xen/domain_builder.h

Comments

Jan Beulich July 26, 2022, 2:22 p.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/xen/arch/x86/boot/boot_info32.h b/xen/arch/x86/boot/boot_info32.h
index 01af950efc..0e7821efb3 100644
--- a/xen/arch/x86/boot/boot_info32.h
+++ b/xen/arch/x86/boot/boot_info32.h
@@ -87,6 +87,9 @@  struct __packed boot_info {
     /* struct boot_module* */
     u64 mods;
 
+    /* struct domain_builder* */
+    u64 builder;
+
     /* struct arch_boot_info* */
     u64 arch;
 };
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 9ca5a99510..e44f7f3c43 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -4,6 +4,7 @@ 
  * Copyright (c) 2002-2005, K A Fraser
  */
 
+#include <xen/bootdomain.h>
 #include <xen/bootinfo.h>
 #include <xen/init.h>
 #include <xen/iocap.h>
@@ -22,31 +23,11 @@ 
 #include <asm/setup.h>
 #include <asm/spec_ctrl.h>
 
-struct memsize {
-    long nr_pages;
-    unsigned int percent;
-    bool minus;
-};
-
 static struct memsize __initdata dom0_size;
 static struct memsize __initdata dom0_min_size;
 static struct memsize __initdata dom0_max_size = { .nr_pages = LONG_MAX };
 static bool __initdata dom0_mem_set;
 
-static bool __init memsize_gt_zero(const struct memsize *sz)
-{
-    return !sz->minus && sz->nr_pages;
-}
-
-static unsigned long __init 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;
-}
-
 /*
  * dom0_mem=[min:<min_amt>,][max:<max_amt>,][<amt>]
  *
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
new file mode 100644
index 0000000000..6f37ac99dc
--- /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;
+}
+
+struct arch_domain_mem {
+    struct memsize mem_size;
+    struct memsize mem_min;
+    struct memsize mem_max;
+};
+
+#endif
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 2fcd576023..f02f4edcd7 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -45,6 +45,8 @@  struct __packed mb_memmap {
     uint32_t type;
 };
 
+struct arch_domain_builder { };
+
 static inline bool loader_is_grub2(const char *loader_name)
 {
     /* GRUB1="GNU GRUB 0.xx"; GRUB2="GRUB 1.xx" */
diff --git a/xen/include/xen/bootdomain.h b/xen/include/xen/bootdomain.h
new file mode 100644
index 0000000000..b172d16f4e
--- /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;
+
+struct boot_domain {
+#define BUILD_PERMISSION_NONE          (0)
+#define BUILD_PERMISSION_CONTROL       (1 << 0)
+#define BUILD_PERMISSION_HARDWARE      (1 << 1)
+    uint32_t permissions;
+
+#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  */
+    uint32_t mode;
+
+    domid_t domid;
+    uint8_t uuid[16];
+
+    uint32_t ncpus;
+    struct arch_domain_mem meminfo;
+
+#define BUILD_MAX_SECID_LEN 64
+    unsigned char secid[BUILD_MAX_SECID_LEN];
+
+    struct boot_module *kernel;
+    struct boot_module *ramdisk;
+#define BUILD_MAX_CONF_MODS 2
+#define BUILD_DTB_CONF_IDX 0
+#define BUILD_DOM_CONF_IDX 1
+    struct boot_module *configs[BUILD_MAX_CONF_MODS];
+
+    struct domain *domain;
+};
+
+#endif
diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
index 477294dc10..1d76d99a40 100644
--- a/xen/include/xen/bootinfo.h
+++ b/xen/include/xen/bootinfo.h
@@ -1,6 +1,7 @@ 
 #ifndef __XEN_BOOTINFO_H__
 #define __XEN_BOOTINFO_H__
 
+#include <xen/bootdomain.h>
 #include <xen/mm.h>
 #include <xen/types.h>
 
@@ -15,6 +16,7 @@  typedef enum {
     BOOTMOD_XSM,
     BOOTMOD_UCODE,
     BOOTMOD_GUEST_DTB,
+    BOOTMOD_GUEST_CONF,
 }  bootmodule_kind;
 
 typedef enum {
@@ -48,6 +50,8 @@  struct __packed boot_info {
     uint32_t nr_mods;
     struct boot_module *mods;
 
+    struct domain_builder *builder;
+
     struct arch_boot_info *arch;
 };
 
diff --git a/xen/include/xen/domain_builder.h b/xen/include/xen/domain_builder.h
new file mode 100644
index 0000000000..79785ef251
--- /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)
+{
+    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 );
+}
+
+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;
+
+    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);
+
+#endif /* XEN_DOMAIN_BUILDER_H */