Re: [PATCH v3 5/7] xsm: decouple xsm header inclusion selection

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2021 16:06, Daniel P. Smith wrote:
> --- /dev/null
> +++ b/xen/include/xsm/xsm-core.h
> @@ -0,0 +1,273 @@
> +/*
> + *  This file contains the XSM hook definitions for Xen.
> + *
> + *  This work is based on the LSM implementation in Linux 2.6.13.4.
> + *
> + *  Author:  George Coker, <gscoker@xxxxxxxxxxxxxx>
> + *
> + *  Contributors: Michael LeMay, <mdlemay@xxxxxxxxxxxxxx>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2,
> + *  as published by the Free Software Foundation.
> + */
> +
> +#ifndef __XSM_CORE_H__
> +#define __XSM_CORE_H__
> +
> +#include <xen/sched.h>
> +#include <xen/multiboot.h>

I was going to ask to invert the order (as we try to arrange #include-s
alphabetically), but it looks like multiboot.h isn't fit for this.

> +typedef void xsm_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t);

Just FTR - I consider this dubious. If void is meant, I don't see why
a void handle can't be used.

> +/* policy magic number (defined by XSM_MAGIC) */
> +typedef uint32_t xsm_magic_t;
> +
> +#ifdef CONFIG_XSM_FLASK
> +#define XSM_MAGIC 0xf97cff8c
> +#else
> +#define XSM_MAGIC 0x0
> +#endif
> +
> +/* These annotations are used by callers and in dummy.h to document the
> + * default actions of XSM hooks. They should be compiled out otherwise.
> + */

I realize you only move code, but like e.g. the u32 -> uint32_t change
in context above I'd like to encourage you to also address other style
issues in the newly introduced file. Here I'm talking about comment
style, requiring /* to be on its own line.

> +enum xsm_default {
> +    XSM_HOOK,     /* Guests can normally access the hypercall */
> +    XSM_DM_PRIV,  /* Device model can perform on its target domain */
> +    XSM_TARGET,   /* Can perform on self or your target domain */
> +    XSM_PRIV,     /* Privileged - normally restricted to dom0 */
> +    XSM_XS_PRIV,  /* Xenstore domain - can do some privileged operations */
> +    XSM_OTHER     /* Something more complex */
> +};
> +typedef enum xsm_default xsm_default_t;
> +
> +struct xsm_ops {
> +    void (*security_domaininfo) (struct domain *d,

Similarly here (and below) - we don't normally put a blank between
the closing and opening parentheses in function pointer declarations.
The majority does so here, but ...

>[...]
> +    int (*page_offline)(uint32_t cmd);
> +    int (*hypfs_op)(void);

... there are exceptions.

>[...]
> +    int (*platform_op) (uint32_t cmd);
> +
> +#ifdef CONFIG_X86
> +    int (*do_mca) (void);
> +    int (*shadow_control) (struct domain *d, uint32_t op);
> +    int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op);
> +    int (*apic) (struct domain *d, int cmd);
> +    int (*memtype) (uint32_t access);
> +    int (*machine_memory_map) (void);
> +    int (*domain_memory_map) (struct domain *d);
> +#define XSM_MMU_UPDATE_READ      1
> +#define XSM_MMU_UPDATE_WRITE     2
> +#define XSM_MMU_NORMAL_UPDATE    4
> +#define XSM_MMU_MACHPHYS_UPDATE  8
> +    int (*mmu_update) (struct domain *d, struct domain *t,
> +                       struct domain *f, uint32_t flags);
> +    int (*mmuext_op) (struct domain *d, struct domain *f);
> +    int (*update_va_mapping) (struct domain *d, struct domain *f,
> +                              l1_pgentry_t pte);
> +    int (*priv_mapping) (struct domain *d, struct domain *t);
> +    int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e,
> +                              uint8_t allow);
> +    int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e,
> +                           uint8_t allow);
> +    int (*pmu_op) (struct domain *d, unsigned int op);
> +#endif
> +    int (*dm_op) (struct domain *d);

To match grouping elsewhere, a blank line above here, ...

> +    int (*xen_version) (uint32_t cmd);
> +    int (*domain_resource_map) (struct domain *d);
> +#ifdef CONFIG_ARGO

... and here would be nice.

> +    int (*argo_enable) (const struct domain *d);
> +    int (*argo_register_single_source) (const struct domain *d,
> +                                        const struct domain *t);
> +    int (*argo_register_any_source) (const struct domain *d);
> +    int (*argo_send) (const struct domain *d, const struct domain *t);
> +#endif
> +};
> +
> +extern void xsm_fixup_ops(struct xsm_ops *ops);
> +
> +#ifdef CONFIG_XSM
> +
> +#ifdef CONFIG_MULTIBOOT
> +extern int xsm_multiboot_init(unsigned long *module_map,
> +                              const multiboot_info_t *mbi);
> +extern int xsm_multiboot_policy_init(unsigned long *module_map,
> +                                     const multiboot_info_t *mbi,
> +                                     void **policy_buffer,
> +                                     size_t *policy_size);
> +#endif
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +/*
> + * Initialize XSM
> + *
> + * On success, return 1 if using SILO mode else 0.
> + */
> +extern int xsm_dt_init(void);
> +extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size);
> +extern bool has_xsm_magic(paddr_t);
> +#endif
> +
> +#ifdef CONFIG_XSM_FLASK
> +extern const struct xsm_ops *flask_init(const void *policy_buffer,
> +                                        size_t policy_size);
> +#else
> +static inline const struct xsm_ops *flask_init(const void *policy_buffer,
> +                                               size_t policy_size)
> +{
> +    return NULL;
> +}
> +#endif
> +
> +#ifdef CONFIG_XSM_FLASK_POLICY
> +extern const unsigned char xsm_flask_init_policy[];
> +extern const unsigned int xsm_flask_init_policy_size;
> +#endif

To be honest, I don't think this belongs in any header. This interfaces
with a generated assembly file. In such a case I would always suggest
to limit visibility of the symbols as much as possible, i.e. put the
declarations in the sole file referencing them.

> +#ifdef CONFIG_XSM_SILO
> +extern const struct xsm_ops *silo_init(void);
> +#else
> +static const inline struct xsm_ops *silo_init(void)
> +{
> +    return NULL;
> +}
> +#endif
> +
> +#else /* CONFIG_XSM */
> +
> +#ifdef CONFIG_MULTIBOOT
> +static inline int xsm_multiboot_init (unsigned long *module_map,

Nit: Stray blank ahead of the opening parenthesis.

Looking back there's also the question of "extern" on function
declarations. In new code I don't think we put the redundant
storage class specifier there; they're only needed on data
declarations. I'm inclined to suggest to drop all of them while
moving the code.

Preferably with these adjustments
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan