On 8/26/21 4:13 AM, Jan Beulich wrote:
> 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.
So my understanding is to leave this as is.
>> +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.
Unless I am misunderstanding what you are calling for, I am afraid this
will trickle further that what intended to be addressed in this patch
set. If disagree and would like to provide me a suggest that stays
bounded, I would gladly incorporate.
>> +/* 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.
Ack.
>> +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 ...
Ack.
>> [...]
>> + 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, ...
Ack.
>> + int (*xen_version) (uint32_t cmd);
>> + int (*domain_resource_map) (struct domain *d);
>> +#ifdef CONFIG_ARGO
>
> ... and here would be nice.
Ack.
>> + 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.
Confirmed only used in xsm_core.c, so will move there.
>> +#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.
Ack.
> 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]>
I believe I have pretty much incorporated all your adjustments. Will
include your Acked-by unless you feel I have not.
v/r,
dps
