From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vitaly Kuznetsov Subject: Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset() Date: Tue, 14 Jul 2015 17:52:44 +0200 Message-ID: <87pp3uybw3.fsf@vitty.brq.redhat.com> References: <1435075913-335-1-git-send-email-vkuznets@redhat.com> <1435075913-335-9-git-send-email-vkuznets@redhat.com> <20150710165442.GD24518@l.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZF2Vp-00063w-KD for xen-devel@lists.xenproject.org; Tue, 14 Jul 2015 15:52:53 +0000 In-Reply-To: <20150710165442.GD24518@l.oracle.com> (Konrad Rzeszutek Wilk's message of "Fri, 10 Jul 2015 12:54:42 -0400") List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Wei Liu , Andrew Jones , Keir Fraser , Ian Campbell , Stefano Stabellini , Andrew Cooper , Julien Grall , Ian Jackson , Olaf Hering , Tim Deegan , David Vrabel , Jan Beulich , xen-devel@lists.xenproject.org, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org Konrad Rzeszutek Wilk writes: > On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote: >> ... >> >> +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 */ > > Missing full stop. But perhaps we could explain what would be needed > for an PV guest to make it work (not as something for you to do > of course but an victim^H^H^Hvolunteer!). > Oh, I seriously doubt we'll ever be doing soft reset for classic PV. I don't see an easy way to preserve existent memory and without this soft reset is pointless. PVH (or PVHVM-without-dm) looks much more promising. >> + if ( !is_hvm_domain(d) ) >> + return -EINVAL; I suggest we leave it here with the comment above and decide something later based on the chosen path for PVH. >> + >> + hvm_destroy_all_ioreq_servers(d); >> + >> + spin_lock(&d->event_lock); >> + for ( i = 1; i < d->nr_pirqs ; i ++ ) > > Is pirq 0 special? > No, for some reason I though it is not a valid number for pirq. Will fix in v9. >> + if ( owner != d ) >> + goto exit_put_page; >> + >> + mfn = page_to_mfn(page); >> + if ( !mfn_valid(mfn) ) >> + { >> + printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n", >> + d->domain_id); > > Would it be good to print out the PFN of the shared page to help narrow the cause? > I think this case is impossibe under normal circumstances and it's not clear to me how to get the PFN (did you mean GFN?) in such case. shared_info is always allocated in arch_domain_create() from Xen heap and page_to_mfn() will never return INVALID_MFN here. In case we'll ever see this printed we'll have examine why this is not true anymore. This piece of code will have to be updated. >> + if ( ret ) >> + printk(XENLOG_G_ERR "Failed to add a page to replace" >> + " Dom%d's shared_info frame %lx\n", d->domain_id, gfn); > > Should we free the new_page in this case? > The new page is already in domain's page_list so we won't lose it on domain destroy but there is also no point in keeping it there if we failed to add it to physmap. Will free it in v9. -- Vitaly