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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-06-30 at 18:08 +0800, Wen Congyang wrote:
> 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@xxxxxxxxxxxxxx>
> >>
> >> 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.

So your commit message is confusingly and without saying so talking
about what the previous case was, not what this patch itself makes work.

> 
> > 
> >> 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.

I don't know what that refers to.

I'm afraid I think the commit message for this patch (and the associated
doc comments) need revisiting almost from scratch, to clearly explain
what this patch is doing and why and what the constraints on the new
functionality will be.

At the moment it mostly talks in a confusing way about the old behaviour
and adds very specific assumptions to the new function which are not
made clear.

> 
> > 
> >>
> >> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> >> Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
> >> ---
> >>  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.

Right, which is the problem I'm describing, this function is not in
itself self contained, it makes assumptions about its caller. Hence I
suggested:

> > So I think at least the comment ought to go at the callsite, at which
> > point this function doesn't add much.

The implication being the function should be removed and the actions it
takes just inlined at the caller
 
> > (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).

With this as a potential cleanup to the whole file.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel