Hi jan
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Friday, March 18, 2022 4:53 PM
> To: Penny Zheng <penny.zh...@arm.com>
> Cc: nd <n...@arm.com>; Penny Zheng
> <penzh...@a011292.shanghai.arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Julien Grall <jul...@xen.org>; Bertrand Marquis
> <bertrand.marq...@arm.com>; Volodymyr Babchuk
> <volodymyr_babc...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Wei Liu <w...@xen.org>; xen-devel@lists.xenproject.org
> 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
