[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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Penny Zheng <Penny.Zheng@xxxxxxx>
  • Date: Fri, 15 Apr 2022 09:52:15 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=t+6wfYKYO48PvL1zb+hNMHa4fCRhRKrcUwf9p5hplCo=; b=CLbhzf2G0ybuAKZUGMfx6yC5k550UVpWC4OFD7GiwL+6CnL7XxHXN35fXF59P+Zuq3DrC4/EF7X0MW2a+QnYrOAcNQ8TXEoz4/YRg88kdBG4HCoNqNbMIEL3cWIU7wqkwy0vCAoRtj9vGTtwR66FtyXtGQORPrKdC2wyjWZrAlfM0qjFp3/OXos1lgVZHmFqj6bEJCr9T/2/lM34T7iqPIsJWYJKQgGmFU5ND4IMSCp/cFrnnbubRBT6xcxuWtkUtsKehdGOj/Gl1rAB7DIkwPCFaA7c7Tvt0RBVwgLpLeVw3qH97QmMYU/Vubfxy7dogDkcTVb8+/mOogplL6o9XQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jbyaJYQoM1U8Q5nLZ3mqEzkZTEBi0/diR4XOrZvPsjd3PTUq/wNGn7KRhQsG7/XnP88E1nUjkE5bWpoSx9VJWpCiw2PPWcYh+Shh2PC9iBur50JlgX2uxnDQJKBKrbvFQjJ/b6BvrBFKaFphd0rHy39YYm+7VG6hjiVVYK/H5IWnrvQxaqgSzmEVRm6zEBuiDQJ7CCcuMTGkRFgHeM70LWdtxa7UWuLEbd71kz2VPc4lHo0ahTfTOV3KP0wCicc8CXKNHYCl3J7kRgKNXn7bDxAeRtSE9SMvRvYDXCQS10SYcnOm9ZY9Bq/5JnCdIhVyxxRPcg35I+asncdw7Lq/Hg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • 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" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Chen <Wei.Chen@xxxxxxx>
  • Delivery-date: Fri, 15 Apr 2022 09:52:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYNQ8BtyFeP2p9kkWMo7TCkVRwI6zE4SIAgCv+qWA=
  • Thread-topic: [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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.