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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi jan 

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, March 18, 2022 4:53 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Penny Zheng
> <penzhe01@xxxxxxxxxxxxxxxxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 02/13] xen/arm: introduce a special domain
> DOMID_SHARED
> 
> 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.
> 

I'll add the security aspects what Stefano explains in the cover letter next 
serie
for better understanding.

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

The reason why I place the setting up here is that DOMID_SHARED needs to map
pre-configured static shared memory in its p2m table, so it must be set up
after system P2M initialization(setup_virt_paging()). setup_system_domains()
is called before system P2M initialization on xen/arch/arm/setup.c, which
can't meet the requirement.

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

Agree. Shared domain needs the p2m initialization(p2m_init), which is
in arch_domain_create. So I will introduce a new helper 
needs_arch_domain_creation()
to include these system domains which need to call arch_domain_create to
be sufficiently constructed.

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

I'll take stefano's suggestion to reuse DOMID_IO.

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