[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 08/11] xen: arch-specific hooks for domain_soft_reset()
>>> On 16.07.15 at 18:27, <vkuznets@xxxxxxxxxx> wrote: > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -704,6 +704,90 @@ void arch_domain_unpause(struct domain *d) > viridian_time_ref_count_thaw(d); > } > > +int arch_domain_soft_reset(struct domain *d) > +{ > + struct page_info *page = virt_to_page(d->shared_info), *new_page; > + int ret = 0; > + struct domain *owner; > + unsigned long mfn, mfn_new, gfn; > + p2m_type_t p2mt; > + unsigned int i; > + > + /* Soft reset is supported for HVM domains only. */ > + if ( !is_hvm_domain(d) ) > + return -EINVAL; This suggests that the whole function may better go somewhere in hvm/. And is this intentionally excluding PVH? > + hvm_destroy_all_ioreq_servers(d); Or if the function was to stay here, you'd probably better add a hvm_domain_soft_reset() instead of making this function non- static. > + spin_lock(&d->event_lock); > + for ( i = 0; i < d->nr_pirqs ; i++ ) > + { > + if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND ) > + { > + ret = unmap_domain_pirq_emuirq(d, i); > + if ( ret ) > + break; > + } > + } > + spin_unlock(&d->event_lock); > + > + if ( ret ) > + return ret; > + > + /* > + * shared_info page needs to be replaced with a new page, otherwise we "The shared info page ..." > + * will get a hole if the domain does XENMAPSPACE_shared_info. > + */ > + > + owner = page_get_owner_and_reference(page); > + /* If shared_info page wasn't used, we do not need to replace it. */ > + if ( owner != d ) owner may be NULL, in which case you mustn't put_page(). Or if you're sure owner can't be NULL here, then please ASSERT() so. (I'm about to post a patch adjusting a remotely similar case in the grant table code). > + goto exit_put_page; > + > + mfn = page_to_mfn(page); > + if ( !mfn_valid(mfn) ) This check makes sense at best ahead of calling page_get_owner_and_reference(), but I can't see how virt_to_page() could produce an invalid page unless there was a bug elsewhere, or d->shared_info was NULL. > + { > + printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid > MFN\n", > + d->domain_id); > + ret = -EINVAL; > + goto exit_put_page; > + } > + > + gfn = mfn_to_gmfn(d, mfn); > + if ( mfn_x(get_gfn_query(d, gfn, &p2mt)) == INVALID_MFN ) Perhaps even != mfn? > + { > + printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n", > + d->domain_id, gfn); > + ret = -EINVAL; > + goto exit_put_page; > + } > + > + new_page = alloc_domheap_page(d, 0); > + if ( !new_page ) > + { > + printk(XENLOG_G_ERR "Failed to alloc a page to replace" > + " Dom%d's shared_info frame %lx\n", d->domain_id, gfn); > + ret = -ENOMEM; > + goto exit_put_gfn; > + } > + mfn_new = page_to_mfn(new_page); mfn_new is used just once, and hence this variable can be easily eliminated (either be re-using mfn after switching this and the following lines, or by just not using an intermediate variable here at all. > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1035,6 +1035,10 @@ int domain_soft_reset(struct domain *d) > for_each_vcpu ( d, v ) > unmap_vcpu_info(v); > > + rc = arch_domain_soft_reset(d); > + if (rc) > + return rc; I don't think this can be done this late, or else you'd need to have a way to undo everything you've done earlier in the function. This of course also applies to what's being done inside the function. Or if undoing is - as it looks like - rather hard, perhaps the domain would better be crashed than left in an inconsistent state? Also - coding style. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |