| Message ID | 20220311061123.1883189-14-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Static shared memory on dom0less system | expand |
Hi, On 11/03/2022 06:11, Penny Zheng wrote: > From: Penny Zheng <penny.zheng@arm.com> > > When destroyed domain is an owner domain of a static shared memory > region, then we need to ensure that all according borrower domains > shall not have the access to this static shared memory region too. As Stefano wrote, I don't think this is necessary. The page reference accounting will keep the page alive until everyone have released the page. So can you explain why you want to do that? > > This commit covers above scenario through unmapping all borrowers' > according foreign memory mapping when destroyed domain is a owner > domain of a static shared memory region. > > NOTE: It will best for users to destroy all borrowers before the owner > domain in case encountering data abort when accidentally accessing > the static shared memory region. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain.c | 88 ++++++++++++++++++++++++++++++++++--------- > 1 file changed, 71 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 73ffbfb918..8f4a8dcbfc 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -998,10 +998,39 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list) > } > > #ifdef CONFIG_STATIC_SHM > +static int destroy_shm(struct domain *d, gfn_t gfn, unsigned long nr_gfns) If you still plan to go ahead with this approach, then I would prefer if this function is created in patch #8. This will help to reduce the churn in this patch. [...] > - for ( j = 0; j < nr_gfns; j++ ) > + if ( test_bit(shm_id, shm_list_mask) ) > { > - mfn_t mfn; > - > - mfn = gfn_to_mfn(d, gfn_add(gfn, j)); > - if ( !mfn_valid(mfn) ) > + domid_t od = shm_list[shm_id].owner_dom; > + unsigned long j; > + /* > + * If it is a owner domain, then after it gets destroyed, > + * static shared memory region shall be unaccessible to all > + * borrower domains too. > + */ > + if ( d->domain_id == od ) > { > - dprintk(XENLOG_ERR, > - "Domain %pd page number %lx invalid.\n", > - d, gfn_x(gfn) + i); > - return -EINVAL; > + struct domain *bd; > + > + for ( j = 0; j < shm_list[shm_id].nr_borrower; j++ ) > + { > + bd = get_domain_by_id(shm_list[shm_id].borrower_dom[j]); > + /* > + * borrower domain could be dead already, in such case > + * no need to do the unmapping. The domain ID could have been re-used. So it is not enough to lookup for the ID. > + */ > + if ( bd != NULL ) > + { > + gfn_t b_gfn = gaddr_to_gfn( > + shm_list[shm_id].borrower_gbase[j]); > + ret = destroy_shm(bd, b_gfn, nr_gfns); > + if ( ret ) > + dprintk(XENLOG_ERR, > + "Domain %pd: failed to destroy static shared memory.\n", > + bd); In the commit message, you wrote you want to remove the pages from the borrower. But here, you will ignore a failure and continue to destroy the owner like nothing happened. If you are concerned that the borrower can still use the pages. Then we should make sure that they are removed in every cases. However, I think the code is going to quite complex. So I think we should consider to do nothing here and let the borrower use the pages until they die. Potentially, we could notify the borrowers that the owner died so they can decide to remove/shutdown themself. Of course, it would mean we are relying on the borrowers to be nice. An alternative would be to destroy them. Cheers,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 73ffbfb918..8f4a8dcbfc 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -998,10 +998,39 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list) } #ifdef CONFIG_STATIC_SHM +static int destroy_shm(struct domain *d, gfn_t gfn, unsigned long nr_gfns) +{ + unsigned long i = 0; + int ret = 0; + + for ( ; i < nr_gfns; i++ ) + { + mfn_t mfn; + + mfn = gfn_to_mfn(d, gfn_add(gfn, i)); + if ( !mfn_valid(mfn) ) + { + dprintk(XENLOG_ERR, + "Domain %pd page number %lx invalid.\n", + d, gfn_x(gfn) + i); + return -EINVAL; + } + + ret = guest_physmap_remove_page(d, gfn_add(gfn, i), mfn, 0); + if ( ret ) + return ret; + + /* Drop the reference. */ + put_page(mfn_to_page(mfn)); + } + + return ret; +} + static int domain_destroy_shm(struct domain *d) { int ret = 0; - unsigned long i = 0UL, j; + unsigned long i = 0UL; if ( d->arch.shm_mem == NULL ) return ret; @@ -1009,29 +1038,54 @@ static int domain_destroy_shm(struct domain *d) { for ( ; i < d->arch.shm_mem->nr_banks; i++ ) { + u32 shm_id = d->arch.shm_mem->bank[i].shm_id; unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem->bank[i].size); gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start); - for ( j = 0; j < nr_gfns; j++ ) + if ( test_bit(shm_id, shm_list_mask) ) { - mfn_t mfn; - - mfn = gfn_to_mfn(d, gfn_add(gfn, j)); - if ( !mfn_valid(mfn) ) + domid_t od = shm_list[shm_id].owner_dom; + unsigned long j; + /* + * If it is a owner domain, then after it gets destroyed, + * static shared memory region shall be unaccessible to all + * borrower domains too. + */ + if ( d->domain_id == od ) { - dprintk(XENLOG_ERR, - "Domain %pd page number %lx invalid.\n", - d, gfn_x(gfn) + i); - return -EINVAL; + struct domain *bd; + + for ( j = 0; j < shm_list[shm_id].nr_borrower; j++ ) + { + bd = get_domain_by_id(shm_list[shm_id].borrower_dom[j]); + /* + * borrower domain could be dead already, in such case + * no need to do the unmapping. + */ + if ( bd != NULL ) + { + gfn_t b_gfn = gaddr_to_gfn( + shm_list[shm_id].borrower_gbase[j]); + ret = destroy_shm(bd, b_gfn, nr_gfns); + if ( ret ) + dprintk(XENLOG_ERR, + "Domain %pd: failed to destroy static shared memory.\n", + bd); + } + } + + continue; } - - ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0); - if ( ret ) - return ret; - - /* Drop the reference. */ - put_page(mfn_to_page(mfn)); } + /* + * As borrower domain, remove foreign memory mapping and drop the + * reference count. + */ + ret = destroy_shm(d, gfn, nr_gfns); + if ( ret ) + dprintk(XENLOG_ERR, + "Domain %pd: failed to destroy static shared memory.\n", + d); } }