[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 26 Jul 2022 16:22:46 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=w5FDg5yIlwKx00AS1KOPVi2ZDa7fNr/Sis7oi5947cg=; b=LgLsXCbqcdZroXZESvi4pM3sqFwrtWIWY4gltAE7YLYF7l0Xiuf0zcXV8OsGOpfDTJJ9Yfs85G/FoqE/bG5WE/ht2yfy61l6+b0/Fq6Asd2yAbmlE96PrISmJ6awD/tXU+yxxBjC1h7QWccm/x1Jo62/kIUbbu37InHcWZ83wKXFz9Q9amSbvyjWn3OCcoqlaqjSbhBGuY41pmbQ9bg0BVlBawAlqnGGPuanVlan/qmYa6nHHVGhBmrzx0M4FnWMAv78X0Iyo3ubcIpJeYOmCkhmCnwpRoDgHWUZGJAusAPdJczCA/s9z3NRRL1IvFyI6p2WNs0fDbG57bSdangxsw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=oJcvpiX0iNuMNtwFflw4zXcDrCKrEUP+EmjOX7nhpzew2Yx1RxAJLkegZRWkZCBDXPZiCkHBWj76vZK4X4h6UUCMR/P6hAXvDH6D4vtR4vKFRVNb2F+Li2hNN/1Oo5tbVXwHHhashS+nPA36Dzb2lpHmX23zMmD8n6UHyp/ofd9DDfU5wadh2O0xW4rDXv7v+/dovkO5d8EbGgCLeQ0ZUOgeAr9zUxHv9sq53fzyt1KQdG6yBbMx7Omt017+PI1vDiL3FJVYGr8bn1YxE8e8EkfFxyvHFEevuPoCfAPlzxJqFJZq6QpPefjI+AsOF1D3xlduwinnxbXkxrH7M1ChJA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: scott.davis@xxxxxxxxxx, christopher.clark@xxxxxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Tue, 26 Jul 2022 14:22:59 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.