[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |