Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2022 07:11, Penny Zheng wrote:
> In case to own statically shared pages when owner domain is not
> explicitly defined, this commits propose a special domain DOMID_SHARED,
> and we assign it 0x7FF5, as one of the system domains.
> 
> Statically shared memory reuses the same way of initialization with static
> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
> 
> We intends to do shared domain creation after setup_virt_paging so shared
> domain could successfully do p2m initialization.

There's nothing said here, in the earlier patch, or in the cover letter
about the security aspects of this. There is a reason we haven't been
allowing arbitrary, un-supervised sharing of memory between domains. It
wants clarifying why e.g. grants aren't an option to achieve what you
need, and how you mean to establish which domains are / aren't permitted
to access any individual page owned by this domain.

> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -106,6 +106,13 @@ config TEE
>  
>  source "arch/arm/tee/Kconfig"
>  
> +config STATIC_SHM
> +       bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> +       depends on STATIC_MEMORY
> +       default n

Nit: "default n" is redundant and hence would imo better be omitted.

> @@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
>      d->arch.directmap = flags & CDF_directmap;
>  
>      /* p2m_init relies on some value initialized by the IOMMU subsystem */
> -    if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> +    if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 : 
> config->iommu_opts)) != 0 )

Nit: Overlong line.

> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
>      return ( !dom0found && domUfound );
>  }
>  
> +#ifdef CONFIG_STATIC_SHM
> +static void __init setup_shared_domain(void)
> +{
> +    /*
> +     * Initialise our DOMID_SHARED domain.
> +     * This domain owns statically shared pages when owner domain is not
> +     * explicitly defined.
> +     */
> +    dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> +    if ( IS_ERR(dom_shared) )
> +        panic("Failed to create d[SHARED]: %ld\n", PTR_ERR(dom_shared));

I don't think this should be a panic - the system ought to be able to
come up fine, just without actually using this domain. After all this
is an optional feature which may not actually be used.

Also, along the lines of what Stefano has said, this setting up of
the domain would also better live next to where the other special
domains are set up. And even if it was to remain here, ...

> @@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long boot_phys_offset,
>      apply_alternatives_all();
>      enable_errata_workarounds();
>  
> +#ifdef CONFIG_STATIC_SHM
> +    /*
> +     * This needs to be called **after** setup_virt_paging so shared
> +     * domains could successfully do p2m initialization.
> +     */
> +    setup_shared_domain();
> +#endif

... the #ifdef-ary here should be avoided by moving the other
#ifdef inside the function body.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
>  
>      rangeset_domain_initialise(d);
>  
> -    /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> -    if ( is_system_domain(d) && !is_idle_domain(d) )
> +    /*
> +     * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
> +     * sufficiently constructed.
> +     */
> +    if ( is_system_domain(d) && !is_idle_domain(d) && !is_shared_domain(d) )
>          return d;
>  
> -    if ( !is_idle_domain(d) )
> +    if ( !is_idle_domain(d) && !is_shared_domain(d) )
>      {
>          if ( !is_hardware_domain(d) )
>              d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> @@ -663,7 +666,7 @@ struct domain *domain_create(domid_t domid,
>          goto fail;
>      init_status |= INIT_arch;
>  
> -    if ( !is_idle_domain(d) )
> +    if ( !is_idle_domain(d) && !is_shared_domain(d) )
>      {
>          watchdog_domain_init(d);
>          init_status |= INIT_watchdog;

All of these extra is_shared_domain() are quite ugly to see added.
First and foremost going this route doesn't scale very well - consider
how the code will look like when two more special domains with special
needs would be added. I think you want to abstract this some by
introducing one (or a small set of) new is_...() or e.g. needs_...()
predicates.

Further (there's no particularly good place to mention this) I'm
afraid I don't view "shared" as a good name: It's not the domain
which is shared, but it's the domain to hold shared memory. For this
my first consideration would be to see whether an existing special
domain can be re-used; after all the set of reserved domain IDs is
a very limited one, and hence each value taken from there should come
with a very good reason. We did such re-use e.g. when introducing
quarantining for PCI devices, by associating them with DOM_IO rather
than inventing a new DOM_QUARANTINE. If there are good reasons
speaking against such re-use, then I'd like to ask to consider e.g.
DOMID_SHM / DOMID_SHMEM plus associated predicate.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
>  
>      switch ( domid )
>      {
> +#ifdef CONFIG_STATIC_SHM
> +    case DOMID_SHARED:
> +        pg_owner = rcu_lock_domain(dom_shared);
> +        break;
> +#endif

Please can you avoid #ifdef in cases like this one, by instead using

    case DOMID_SHMEM:
        pg_owner = dom_shared ? rcu_lock_domain(dom_shared) : NULL;
        break;

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct domain 
> *d)
>      return d->domain_id >= DOMID_FIRST_RESERVED;
>  }
>  
> +#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)

Would this better evaluate to "false" when !STATIC_SHM, such that
the compiler can eliminate respective conditionals and/or code?

Jan