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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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, <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.

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 <jbeulich@xxxxxxxx>

I believe I have pretty much incorporated all your adjustments. Will
include your Acked-by unless you feel I have not.

v/r,
dps