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, <[email protected]>
> + *
> + * Contributors: Michael LeMay, <[email protected]>
> + *
> + * 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 <[email protected]>
Jan
