Re: [PATCH v3 COLOPre 08/26] tools/libxc: support to resume uncooperative HVM guests

[Wen Congyang <wency@cn.fujitsu.com>]

On 06/30/2015 12:27 AM, Ian Campbell wrote:
> On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> 1. suspend
>> a. PVHVM and PV: we use the same way to suspend the guest(send the suspend
> 
> space between "guest" and the open parenthesis please.
> 
>>    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
>>    In this case, we don't change the guest's state.
>>    PV: modify the return code to 1, and than call the domctl:
>>        XEN_DOMCTL_resumedomain
>>    PVHVM: same with PV
>>    HVM: do nothing in modify_returncode, and than call the domctl:
>>         XEN_DOMCTL_resumedomain
>> b. slow
>>    In this case, we have changed the guest's state.
> 
> "have" or "will"? AIUI the latter would be more accurate.
> 
>>    PV: update start info, and reset all secondary CPU states. Than call the
>>    domctl: XEN_DOMCTL_resumedomain
>>    PVHVM and HVM can not be resumed.
> 
> I'm confused -- isn't the purpose of this patch to make PVHM support
> resume?

Without this patch, HVM(both PVHVM and pure HVM) can not be resumed in slow
path if its state is changed.

> 
>> For PVHVM, in my test, only call the domctl: XEN_DOMCTL_resumedomain
>> can work. I am not sure if we should update start info and reset all
>> secondary CPU states.
>>
>> For pure HVM guest, in my test, only call the domctl:
>> XEN_DOMCTL_resumedomain can work.
>>
>> So we can call libxl__domain_resume(..., 1) if we don't change the guest
>> state, otherwise call libxl__domain_resume(..., 0).
> 
> Hrm, so it sounds here like the correctness of this new functionality
> requires the caller to have not messed with the domain's state? What
> sort of changes are to the guest state are we talking about here?

Any state can be changed: memory, device state. If the caller messes
with the domain's state, the guest may be crashed.

> 
> Isn't that a new requirement for this call? If so then it should be
> documented somewhere, specifically what sorts of changes are and are not
> allowed and the types of guests which are affected.

The state should be the one saved by xen.

> 
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>  tools/libxc/xc_resume.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
>> index e67bebd..bd82334 100644
>> --- a/tools/libxc/xc_resume.c
>> +++ b/tools/libxc/xc_resume.c
>> @@ -109,6 +109,23 @@ 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;
>> +
>> +    /*
>> +     * If it is PVHVM, the hypercall return code is 0, because this
>> +     * is not a fast path resume, we do not modify_returncode as in
>> +     * xc_domain_resume_cooperative.
>> +     * (resuming it in a new domain context)
>> +     *
>> +     * If it is a HVM, the hypercall is a NOP.
>> +     */
>> +    domctl.cmd = XEN_DOMCTL_resumedomain;
>> +    domctl.domain = domid;
>> +    return do_domctl(xch, &domctl);
> 
> There are already several open coded instances of this
> XEN_DOMCTL_resumedomain, and I think putting this particular one into a
> helper is actually more confusing than just inlining this at the caller.
> 
> In particular when reading this function my first question was "how do
> we know this is not a fast path resume", the answer being that the only
> caller is the slow path resume case, but that's not evident from the
> context (what if someone adds a second call?)

Only the caller know it. See xc_domain_resume()'s third parameter.

Thanks
Wen Congyang

> 
> So I think at least the comment ought to go at the callsite, at which
> point this function doesn't add much.
> 
> (Ideally all the open coded do_domctl would go into a single
> do_domainresume or something, but I don't think you need to do that
> unless you really want to).
> 
> 
> 
> .
>