|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 05/10] xsm: switch XSM init to boot info structures
On Sat, 8 Jul 2023, Stefano Stabellini wrote:
> On Sat, 1 Jul 2023, Christopher Clark wrote:
> > Change the XSM implementation to use the boot info structure instead of
> > the multiboot module_map.
> >
> > Drops a dependency on CONFIG_MULTIBOOT, so boot module logic is now
> > used whenever the DEVICE_TREE specific logic (for Arm) is not, with the
> > bootinfo header conditionally included to ensure no change on Arm.
>
> Given that device tree is used for ARM but also for the Hyperlaunch
> configuration, I think this is very confusing.
>
> Ideally I think we should use struct bootinfo (ideally the same version
> currently under xen/arch/arm/include/asm/setup.h) for both ARM and x86,
> getting rid of any need of any #ifdefs. We should be able to get
> xsm_dt_policy_init to work with Hyperlaunch x86 pretty easily?
>
> If that is too much work, then please use #ifdef CONFIG_ARM so at least
> it is clear what the difference is.
Wait, is it the case that xsm_bootmodule_init is for the old grub
multiboot protocol? And xsm_dt_init will be used by both Dom0less ARM
and Hyperlaunch x86?
Wouldn't it be better to have a "detect" or "pre-init" function per
module-type (one for xsm, one for microcode, etc.) that configure the
bootinfo and bootmodules correctly and then we can reuse a single xsm
init function in all cases:
- dom0less arm
- hyperlaunch x86
- legacy grub multiboot x86
> > Adds a multiboot header inclusion into guest/xen/pvh-boot.c
> > since it is no longer provided via transitive inclusion and the source
> > in that file uses multiboot structures.
> >
> > Signed-off-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > Changes since v1: patch is a subset of v1 series patches 2 and 3.
> > - Added the <xen/multiboot.h> inclusion to pvh-boot.c
> > - Use conditional inclusion for <xen/bootinfo.h>
> >
> > xen/arch/x86/guest/xen/pvh-boot.c | 1 +
> > xen/arch/x86/setup.c | 2 +-
> > xen/include/xsm/xsm.h | 28 +++++++--------
> > xen/xsm/xsm_core.c | 47 +++++++++++++++---------
> > xen/xsm/xsm_policy.c | 59 ++++++++++++++++---------------
> > 5 files changed, 77 insertions(+), 60 deletions(-)
> >
> > diff --git a/xen/arch/x86/guest/xen/pvh-boot.c
> > b/xen/arch/x86/guest/xen/pvh-boot.c
> > index 9cbe87b61b..1ed04d035c 100644
> > --- a/xen/arch/x86/guest/xen/pvh-boot.c
> > +++ b/xen/arch/x86/guest/xen/pvh-boot.c
> > @@ -9,6 +9,7 @@
> > #include <xen/init.h>
> > #include <xen/lib.h>
> > #include <xen/mm.h>
> > +#include <xen/multiboot.h>
> >
> > #include <asm/e820.h>
> > #include <asm/guest.h>
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index f9b04daebd..a616ccc0a8 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1835,7 +1835,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
> > RANGESETF_prettyprint_hex);
> >
> > - xsm_multiboot_init(module_map, mbi);
> > + xsm_bootmodule_init(boot_info);
> >
> > /*
> > * IOMMU-related ACPI table parsing may require some of the system
> > domains
> > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> > index 8dad03fd3d..d409c30be6 100644
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -16,8 +16,10 @@
> > #define __XSM_H__
> >
> > #include <xen/alternative-call.h>
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +# include <xen/bootinfo.h>
> > +#endif
> > #include <xen/sched.h>
> > -#include <xen/multiboot.h>
> >
> > /* policy magic number (defined by XSM_MAGIC) */
> > typedef uint32_t xsm_magic_t;
> > @@ -776,15 +778,14 @@ static inline int xsm_argo_send(const struct domain
> > *d, const struct domain *t)
> >
> > #endif /* XSM_NO_WRAPPERS */
> >
> > -#ifdef CONFIG_MULTIBOOT
> > -int xsm_multiboot_init(
> > - unsigned long *module_map, const multiboot_info_t *mbi);
> > -int xsm_multiboot_policy_init(
> > - unsigned long *module_map, const multiboot_info_t *mbi,
> > - void **policy_buffer, size_t *policy_size);
> > -#endif
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +int xsm_bootmodule_init(const struct boot_info *info);
> > +int xsm_bootmodule_policy_init(
> > + const struct boot_info *info, const unsigned char **policy_buffer,
> > + size_t *policy_size);
> > +
> > +#else
> >
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > /*
> > * Initialize XSM
> > *
> > @@ -826,15 +827,14 @@ static const inline struct xsm_ops *silo_init(void)
> >
> > #include <xsm/dummy.h>
> >
> > -#ifdef CONFIG_MULTIBOOT
> > -static inline int xsm_multiboot_init (
> > - unsigned long *module_map, const multiboot_info_t *mbi)
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +static inline int xsm_bootmodule_init(const struct boot_info *info)
> > {
> > return 0;
> > }
> > -#endif
> >
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > +#else
> > +
> > static inline int xsm_dt_init(void)
> > {
> > return 0;
> > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> > index eaa028109b..301ae4dc8b 100644
> > --- a/xen/xsm/xsm_core.c
> > +++ b/xen/xsm/xsm_core.c
> > @@ -10,8 +10,12 @@
> > * as published by the Free Software Foundation.
> > */
> >
> > -#include <xen/init.h>
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +# include <xen/bootinfo.h>
> > +#endif
> > #include <xen/errno.h>
> > +#include <xen/hypercall.h>
> > +#include <xen/init.h>
> > #include <xen/lib.h>
> > #include <xen/param.h>
> >
> > @@ -138,26 +142,35 @@ static int __init xsm_core_init(const void
> > *policy_buffer, size_t policy_size)
> > return 0;
> > }
> >
> > -#ifdef CONFIG_MULTIBOOT
> > -int __init xsm_multiboot_init(
> > - unsigned long *module_map, const multiboot_info_t *mbi)
> > +/*
> > + * ifdef'ing this against multiboot is no longer valid as the boot module
> > + * is agnostic and it will be possible to dropped the ifndef should Arm
> > + * adopt boot info
> > + */
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +int __init xsm_bootmodule_init(const struct boot_info *bootinfo)
> > {
> > int ret = 0;
> > - void *policy_buffer = NULL;
> > + const unsigned char *policy_buffer = NULL;
> > size_t policy_size = 0;
> >
> > printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
> >
> > if ( XSM_MAGIC )
> > {
> > - ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer,
> > - &policy_size);
> > - if ( ret )
> > - {
> > - bootstrap_map(NULL);
> > - printk(XENLOG_ERR "Error %d initializing XSM policy\n", ret);
> > - return -EINVAL;
> > - }
> > + int ret = xsm_bootmodule_policy_init(bootinfo, &policy_buffer,
> > + &policy_size);
> > + bootstrap_map(NULL);
> > +
> > + if ( ret == -ENOENT )
> > + /*
> > + * The XSM module needs a policy file but one was not located.
> > + * Report as a warning and continue as the XSM module may late
> > + * load a policy file.
> > + */
> > + printk(XENLOG_WARNING "xsm: starting without a policy
> > loaded!\n");
> > + else if ( ret )
> > + panic("Error %d initializing XSM policy\n", ret);
> > }
> >
> > ret = xsm_core_init(policy_buffer, policy_size);
> > @@ -165,9 +178,9 @@ int __init xsm_multiboot_init(
> >
> > return 0;
> > }
> > -#endif
> >
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > +#else
> > +
> > int __init xsm_dt_init(void)
> > {
> > int ret = 0;
> > @@ -215,9 +228,9 @@ bool __init has_xsm_magic(paddr_t start)
> >
> > return false;
> > }
> > -#endif
> > +#endif /* CONFIG_HAS_DEVICE_TREE */
> >
> > -#endif
> > +#endif /* CONFIG_XSM */
> >
> > long do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op)
> > {
> > diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
> > index c6df8c6e06..f1970c8964 100644
> > --- a/xen/xsm/xsm_policy.c
> > +++ b/xen/xsm/xsm_policy.c
> > @@ -18,62 +18,65 @@
> > *
> > */
> >
> > -#include <xsm/xsm.h>
> > -#ifdef CONFIG_MULTIBOOT
> > -#include <asm/boot.h>
> > -#include <xen/multiboot.h>
> > -#include <asm/setup.h>
> > -#endif
> > #include <xen/bitops.h>
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +# include <xen/bootinfo.h>
> > +#endif
> > +#include <xsm/xsm.h>
> > #ifdef CONFIG_HAS_DEVICE_TREE
> > -# include <asm/setup.h>
> > # include <xen/device_tree.h>
> > +#else
> > +#include <asm/boot.h>
> > #endif
> >
> > -#ifdef CONFIG_MULTIBOOT
> > -int __init xsm_multiboot_policy_init(
> > - unsigned long *module_map, const multiboot_info_t *mbi,
> > - void **policy_buffer, size_t *policy_size)
> > +#include <asm/setup.h>
> > +
> > +#ifndef CONFIG_HAS_DEVICE_TREE
> > +int __init xsm_bootmodule_policy_init(
> > + const struct boot_info *bootinfo, const unsigned char **policy_buffer,
> > + size_t *policy_size)
> > {
> > - int i;
> > - module_t *mod = (module_t *)__va(mbi->mods_addr);
> > - int rc = 0;
> > + unsigned long i;
> > + int rc = -ENOENT;
> > u32 *_policy_start;
> > unsigned long _policy_len;
> >
> > - /*
> > - * Try all modules and see whichever could be the binary policy.
> > - * Adjust module_map for the module that is the binary policy.
> > - */
> > - for ( i = mbi->mods_count-1; i >= 1; i-- )
> > - {
> > - if ( !test_bit(i, module_map) )
> > - continue;
> > +#ifdef CONFIG_XSM_FLASK_POLICY
> > + /* Initially set to builtin policy, overriden if boot module is found.
> > */
> > + *policy_buffer = xsm_flask_init_policy;
> > + *policy_size = xsm_flask_init_policy_size;
> > + rc = 0;
> > +#endif
> >
> > - _policy_start = bootstrap_map_multiboot(mod + i);
> > - _policy_len = mod[i].mod_end;
> > + i = bootmodule_index(bootinfo, BOOTMOD_UNKNOWN, 0);
> > + while ( i < bootinfo->nr_mods )
> > + {
> > + _policy_start = bootstrap_map(&bootinfo->mods[i]);
> > + _policy_len = bootinfo->mods[i].size;
> >
> > if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
> > {
> > - *policy_buffer = _policy_start;
> > + *policy_buffer = (unsigned char *)_policy_start;
> > *policy_size = _policy_len;
> >
> > printk("Policy len %#lx, start at %p.\n",
> > _policy_len,_policy_start);
> >
> > - __clear_bit(i, module_map);
> > + bootinfo->mods[i].bootmod_type = BOOTMOD_XSM;
> > + rc = 0;
> > break;
> >
> > }
> >
> > bootstrap_map(NULL);
> > + i = bootmodule_index(bootinfo, BOOTMOD_UNKNOWN, ++i);
> > }
> >
> > return rc;
> > }
> > -#endif
> >
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > +#else
> > +
> > int __init xsm_dt_policy_init(void **policy_buffer, size_t *policy_size)
> > {
> > struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_XSM);
> > --
> > 2.25.1
> >
> >
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |