Re: [PATCH v9 10/15] tools/libxc: x86 HVM save code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/04/15 13:28, Vitaly Kuznetsov wrote:
> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> +/*
>> + * Query for a range of HVM parameters and write an HVM_PARAMS record into the
>> + * stream.
>> + */
>> +static int write_hvm_params(struct xc_sr_context *ctx)
>> +{
>> +    static const unsigned int params[] = {
>> +        HVM_PARAM_STORE_PFN,
>> +        HVM_PARAM_IOREQ_PFN,
>> +        HVM_PARAM_BUFIOREQ_PFN,
>> +        HVM_PARAM_PAGING_RING_PFN,
>> +        HVM_PARAM_MONITOR_RING_PFN,
>> +        HVM_PARAM_SHARING_RING_PFN,
>> +        HVM_PARAM_VM86_TSS,
>> +        HVM_PARAM_CONSOLE_PFN,
>> +        HVM_PARAM_ACPI_IOPORTS_LOCATION,
>> +        HVM_PARAM_VIRIDIAN,
>> +        HVM_PARAM_IDENT_PT,
>> +        HVM_PARAM_PAE_ENABLED,
>> +        HVM_PARAM_VM_GENERATION_ID_ADDR,
>> +        HVM_PARAM_IOREQ_SERVER_PFN,
>> +        HVM_PARAM_NR_IOREQ_SERVER_PAGES,
>> +    };
>> +
>> +    xc_interface *xch = ctx->xch;
>> +    struct xc_sr_rec_hvm_params_entry entries[ARRAY_SIZE(params)];
>> +    struct xc_sr_rec_hvm_params hdr = {
>> +        .count = 0,
>> +    };
>> +    struct xc_sr_record rec = {
>> +        .type   = REC_TYPE_HVM_PARAMS,
>> +        .length = sizeof(hdr),
>> +        .data   = &hdr,
>> +    };
>> +    unsigned int i;
>> +    int rc;
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(params); i++ )
>> +    {
>> +        uint32_t index = params[i];
>> +        uint64_t value;
>> +
>> +        rc = xc_hvm_param_get(xch, ctx->domid, index, &value);
>> +        if ( rc )
>> +        {
>> +            PERROR("Failed to get HVMPARAM at index %u", index);
>> +            return rc;
>> +        }
>> +
>> +        if ( value != 0 )
>> +        {
>> +            entries[hdr.count].index = index;
>> +            entries[hdr.count].value = value;
>> +            hdr.count++;
>> +        }
>> +    }
> While reviewing my 'soft reset' series Ian C raised a question about the
> unsafeness of sequential get/set of HVM params:
> http://lists.xen.org/archives/html/xen-devel/2015-01/msg01177.html
>
> In case the concern is valid I think the requirements for migration are
> even stricter as we can migrate live. In case it is not, would it be
> possible to move your params[] array to some common place so it can be
> reused?

This code currently mirrors what the old migration did.  This was one
area we deliberately didn't try to clean up (we were focusing on a
functional replacement).

I am not a fan of hvm params.  They are used as a dumping ground for
things which IMO should live elsewhere.  The result is this mess,
without any clear direction of who owns which parameter, which should
move on migration, which shouldn't etc.  There are many areas of the
toolstack which independently poke at the parameters.

I would be hesitant to blindly lift this code out, although it is
probably the best start you will find.

What is really needed is formal statement of who owns which hvm params,
and what their purpose is.  That would at least be a good starting point
for evaluating questions like this.

~Andrew