Re: [Patch v6 11/13] tools/libxc: x86 HVM restore code

[Wen Congyang <ghostwcy@gmail.com>]

At 2014/7/18 23:42, Paul Durrant Wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 18 July 2014 16:09
>> To: Wen Congyang; Xen-devel
>> Subject: Re: [Xen-devel] [Patch v6 11/13] tools/libxc: x86 HVM restore code
>>
>> On 18/07/14 15:38, Wen Congyang wrote:
>>> At 2014/7/8 1:38, Andrew Cooper Wrote:
>>>> Restore the x86 HVM specific parts of a domain.  This is the
>>>> HVM_CONTEXT and
>>>> HVM_PARAMS records.
>>>>
>>>> There is no need for any page localisation.
>>>>
>>>> This also includes writing the trailing qemu save record to a file
>>>> because
>>>> this is what libxc currently does.  This is intended to be moved into
>>>> libxl
>>>> proper in the future.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>>    tools/libxc/saverestore/common.h          |    1 +
>>>>    tools/libxc/saverestore/restore_x86_hvm.c |  336
>>>> +++++++++++++++++++++++++++++
>>>>    2 files changed, 337 insertions(+)
>>>>    create mode 100644 tools/libxc/saverestore/restore_x86_hvm.c
>>>>
>>>> diff --git a/tools/libxc/saverestore/common.h
>>>> b/tools/libxc/saverestore/common.h
>>>> index 8d125f9..4894cac 100644
>>>> --- a/tools/libxc/saverestore/common.h
>>>> +++ b/tools/libxc/saverestore/common.h
>>>> @@ -241,6 +241,7 @@ extern struct xc_sr_save_ops save_ops_x86_pv;
>>>>    extern struct xc_sr_save_ops save_ops_x86_hvm;
>>>>
>>>>    extern struct xc_sr_restore_ops restore_ops_x86_pv;
>>>> +extern struct xc_sr_restore_ops restore_ops_x86_hvm;
>>>>
>>>>    struct xc_sr_record
>>>>    {
>>>> diff --git a/tools/libxc/saverestore/restore_x86_hvm.c
>>>> b/tools/libxc/saverestore/restore_x86_hvm.c
>>>> new file mode 100644
>>>> index 0000000..0004dee
>>>> --- /dev/null
>>>> +++ b/tools/libxc/saverestore/restore_x86_hvm.c
>>>> @@ -0,0 +1,336 @@
>>>> +#include <assert.h>
>>>> +#include <arpa/inet.h>
>>>> +
>>>> +#include "common_x86.h"
>>>> +
>>>> +/* TODO: remove */
>>>> +static int handle_toolstack(struct xc_sr_context *ctx, struct
>>>> xc_sr_record *rec)
>>>> +{
>>>> +    xc_interface *xch = ctx->xch;
>>>> +    int rc;
>>>> +
>>>> +    if ( !ctx->restore.callbacks ||
>>>> !ctx->restore.callbacks->toolstack_restore )
>>>> +        return 0;
>>>> +
>>>> +    rc = ctx->restore.callbacks->toolstack_restore(ctx->domid,
>>>> rec->data, rec->length,
>>>> +
>>>> ctx->restore.callbacks->data);
>>>> +    if ( rc < 0 )
>>>> +        PERROR("restoring toolstack");
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Process an HVM_CONTEXT record from the stream.
>>>> + */
>>>> +static int handle_hvm_context(struct xc_sr_context *ctx, struct
>>>> xc_sr_record *rec)
>>>> +{
>>>> +    xc_interface *xch = ctx->xch;
>>>> +    int rc;
>>>> +
>>>> +    rc = xc_domain_hvm_setcontext(xch, ctx->domid, rec->data,
>>>> rec->length);
>>>> +    if ( rc < 0 )
>>>> +        PERROR("Unable to restore HVM context");
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Process an HVM_PARAMS record from the stream.
>>>> + */
>>>> +static int handle_hvm_params(struct xc_sr_context *ctx, struct
>>>> xc_sr_record *rec)
>>>> +{
>>>> +    xc_interface *xch = ctx->xch;
>>>> +    struct xc_sr_rec_hvm_params *hdr = rec->data;
>>>> +    struct xc_sr_rec_hvm_params_entry *entry = hdr->param;
>>>> +    unsigned int i;
>>>> +    int rc;
>>>> +
>>>> +    if ( rec->length < sizeof(*hdr)
>>>> +         || rec->length < sizeof(*hdr) + hdr->count * sizeof(*entry) )
>>>> +    {
>>>> +        ERROR("hvm_params record is too short");
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    for ( i = 0; i < hdr->count; i++, entry++ )
>>>> +    {
>>>> +        switch ( entry->index )
>>>> +        {
>>>> +        case HVM_PARAM_CONSOLE_PFN:
>>>> +            ctx->restore.console_mfn = entry->value;
>>>> +            xc_clear_domain_page(xch, ctx->domid, entry->value);
>>>> +            break;
>>>> +        case HVM_PARAM_STORE_PFN:
>>>> +            ctx->restore.xenstore_mfn = entry->value;
>>>> +            xc_clear_domain_page(xch, ctx->domid, entry->value);
>>>> +            break;
>>>> +        case HVM_PARAM_IOREQ_PFN:
>>>> +        case HVM_PARAM_BUFIOREQ_PFN:
>>>> +            xc_clear_domain_page(xch, ctx->domid, entry->value);
>>>
>>> ioreq page may contain pending i/o request, so I think we should not
>>> clear
>>> them.
>>>
>>> Thanks
>>> Wen Congyang
>>
>> I noticed that in your other series.
>>
>> While I can see why it is causing problems for you, avoiding clearing
>> the page only hides the problem; it doesn't solve it.
>>
>> Amongst other things, avoiding clearing the page could result in qemu
>> binding to the wrong event channels.  Without sending the other IO
>> emulation state, leaving stale ioreqs in the rings is dangerous.
>>
>
> I don't think this is the case (providing vcpus are paused). Xen should have updated the event channels before the new QEMU instance starts at the receiving end (otherwise how would it work now?) and the ioreq state will either be complete (i.e. ready for a vcpu resume) or new (i.e. untouched by qemu), unless qemu died uncleanly at the sending end, which I assume is not a supported usecase. Gratuitously clearing these pages seems like a bad thing to do.

Here is the qemu codes:
     xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
     DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
     state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, 
XC_PAGE_SIZE,
                                               PROT_READ|PROT_WRITE, 
ioreq_pfn);
     if (state->shared_page == NULL) {
         hw_error("map shared IO page returned error %d handle=" 
XC_INTERFACE_FMT,
                  errno, xen_xc);
     }

     xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_PFN, 
&ioreq_pfn);
     DPRINTF("buffered io page at pfn %lx\n", ioreq_pfn);
     state->buffered_io_page = xc_map_foreign_range(xen_xc, xen_domid, 
XC_PAGE_SIZE,
 
PROT_READ|PROT_WRITE, ioreq_pfn);
     if (state->buffered_io_page == NULL) {
         hw_error("map buffered IO page returned error %d", errno);
     }

     state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));

In qemu, we need to get ioreq/buffeed req pfn before mapping the share 
page. We will
auto create default ioreq server when getting the pfn. So the event 
channel is valid
before we map the page.

Thanks
Wen Congyang

>
>    Paul
>
>
>> All of this comes as a result of attempting to performs actions (i.e.
>> migrating an essentially unpaused VM) outside of the original designed
>> usecase, and you are going to have to do more than just avoiding
>> cleaning these pages.
>>
>> ~Andrew
>>
>>>
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        rc = xc_set_hvm_param(xch, ctx->domid, entry->index,
>>>> entry->value);
>>>> +        if ( rc < 0 )
>>>> +        {
>>>> +            PERROR("set HVM param %"PRId64" = 0x%016"PRIx64,
>>>> +                   entry->index, entry->value);
>>>> +            return rc;
>>>> +        }
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>