Re: [PATCH v7 05/18] tools/libxc: support to resume uncooperative HVM guests

From: Wen Congyang <wency@cn.fujitsu.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen devel <xen-devel@lists.xen.org>,
	Dong Eddie <eddie.dong@intel.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v7 05/18] tools/libxc: support to resume uncooperative HVM guests
Date: Thu, 4 Feb 2016 13:30:44 +0800	[thread overview]
Message-ID: <56B2E204.9090206@cn.fujitsu.com> (raw)
In-Reply-To: <20160203194001.GU23178@citrix.com>

On 02/04/2016 03:40 AM, Wei Liu wrote:
> On Fri, Jan 29, 2016 at 01:27:21PM +0800, Wen Congyang wrote:
>> Before this patch:
>> 1. suspend
>> a. PVHVM and PV: we use the same way to suspend the guest (send the suspend
>>    request to the guest). If the guest doesn't support evtchn, the xenstore
>>    variant will be used, suspending the guest via XenBus control node.
>> b. pure HVM: we call xc_domain_shutdown(..., SHUTDOWN_suspend) to suspend
>>    the guest
>>
>> 2. Resume:
>> a. fast path(fast=1)
>>    Do not change the guest state. We call libxl__domain_resume(.., 1) which
>>    calls xc_domain_resume(..., 1 /* fast=1*/) to resume the guest.
>>    PV:       modify the return code to 1, and than call the domctl:
>>              XEN_DOMCTL_resumedomain
>>    PVHVM:    same with PV
>>    pure HVM: do nothing in modify_returncode, and than call the domctl:
> 
> "then"
> 
>>              XEN_DOMCTL_resumedomain
>> b. slow
>>    Used when the guest's state have been changed. Will call
>>    libxl__domain_resume(..., 0) to resume the guest.
>>    PV:       update start info, and reset all secondary CPU states. Than call
>>              the domctl: XEN_DOMCTL_resumedomain
>>    PVHVM:    can not be resumed. You will get the following error message:
>>                  "Cannot resume uncooperative HVM guests"
>>    purt HVM: same with PVHVM
> 
> "pure"
> 
>>
>> After this patch:
>> 1. suspend
>>    unchanged
>>
>> 2. Resume
>> a. fast path:
>>    unchanged
>> b. slow
>>    PV:       unchanged
>>    PVHVM:    call XEN_DOMCTL_resumedomain to resume the guest. Because we
>>              don't modify the return code, the PV driver will disconnect
>>              and reconnect.
>>              The guest ends up doing the XENMAPSPACE_shared_info
>>              XENMEM_add_to_physmap hypercall and resetting all of its CPU
>>              states to point to the shared_info(well except the ones past 32).
>>              That is the Linux kernel does that - regardless whether the
>>              SCHEDOP_shutdown:SHUTDOWN_suspend returns 1 or not.
>>    Pure HVM: call XEN_DOMCTL_resumedomain to resume the guest.
> 
> In summary, this patch only changes slow path resume. Further more, it
> only affects PVHVM and pure HVM variants.
> 
> With you patch, pure HVM is able to resume with effectively the same
> path via XEN_DOMCTL_resumedomain, albeit it is done in two functions
> (_cooperative and _any).
> 
> And according to the recently change in documentation, slow path is
> always safe.
> 
> I think the commit message can be simplified a bit. This is assuming
> using XEN_DOMCTL_resumedomain to resume (PV)HVM in slow path is safe.
> 
> ===
> 
> Use XEN_DOMCTL_resumedomain to resume (PV)HVM guest in slow path
> 
> Previously it was not possible to resume PVHVM or pure HVM guest in slow
> path because libxc didn't support that.
> 
> Using XEN_DOMCTL_resumedomain without modifying guest state  to resume a
> guest is considered to be always safe.  Introduce a function to do that
> for (PV)HVM guests in slow path resume.
> 
> This patch fixes a bug that denies (PV)HVM slow path resume.  This will
> enable COLO to work properly:  COLO requires HVM guest to start in the
> new context that has been set up by COLO, hence slow path resume is
> required.
> 
> ===
> 
> Does this sound right? Especially the wording about safety.

It sounds right.

Thanks
Wen Congyang

> 
> Ian and Ian, you seemed to have suggested Congyang to write the above
> commit message. What do you think about my updated one?
> 
>>
>> Under COLO, we will update the guest's state(modify memory, cpu's registers,
>> device status...). In this case, we cannot use the fast path to resume it.
>> Keep the return code 0, and use a slow path to resume the guest. While
>> resuming HVM using slow path is not supported currently, this patch is to
>> make the resume call to not fail.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>>  tools/libxc/xc_resume.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
>> index 87d4324..4a9b035 100644
>> --- a/tools/libxc/xc_resume.c
>> +++ b/tools/libxc/xc_resume.c
>> @@ -108,6 +108,26 @@ static int xc_domain_resume_cooperative(xc_interface *xch, uint32_t domid)
>>      return do_domctl(xch, &domctl);
>>  }
>>  
>> +static int xc_domain_resume_hvm(xc_interface *xch, uint32_t domid)
>> +{
>> +    DECLARE_DOMCTL;
>> +
>> +    /*
>> +     * The domctl XEN_DOMCTL_resumedomain unpause each vcpu. After
>> +     * the domctl, the guest will run.
>> +     *
>> +     * If it is PVHVM, the guest called the hypercall
>> +     *    SCHEDOP_shutdown:SHUTDOWN_suspend
>> +     * to suspend itself. We don't modify the return code, so the PV driver
>> +     * will disconnect and reconnect.
>> +     *
>> +     * If it is a HVM, the guest will continue running.
>> +     */
>> +    domctl.cmd = XEN_DOMCTL_resumedomain;
>> +    domctl.domain = domid;
>> +    return do_domctl(xch, &domctl);
>> +}
>> +
>>  static int xc_domain_resume_any(xc_interface *xch, uint32_t domid)
>>  {
>>      DECLARE_DOMCTL;
>> @@ -137,10 +157,7 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid)
>>       */
>>  #if defined(__i386__) || defined(__x86_64__)
>>      if ( info.hvm )
>> -    {
>> -        ERROR("Cannot resume uncooperative HVM guests");
>> -        return rc;
>> -    }
>> +        return xc_domain_resume_hvm(xch, domid);
>>  
>>      if ( xc_domain_get_guest_width(xch, domid, &dinfo->guest_width) != 0 )
>>      {
>> -- 
>> 2.5.0
>>
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> 
> .
> 