Re: [Xen-devel] [PATCH 5/6] libxl: allow a generation ID to be specified at domain creation

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-06-03 at 14:15 +0100, David Vrabel wrote:
> Toolstacks may specify a VM generation ID using the
> u.hvm.generation_id field into libxl_domain_build_info structure, when
> creating a domain.
> 
> The toolstack is responsible for providing the correct generation ID
> according to the Microsoft specification (e.g., generating new random
> ones as appropriate when restoring).

Any reason not to make the libxl interface a (def)bool and have libxl
generate a suitable ID internally? Does anything ever care about the
specific UUID?

Should we consider calling the API field name something more specific,
like "ms_vgid"? I'm thinking of the case where some other OS vendor
reinvents the wheel. (I don't care about the internals, just the API).

> Although the specification requires that a ACPI Notify event is raised
> if the generation ID is changed, the generation ID is never changed
> when the domain is in a state to receive such an event (it's either
> newly created or suspended).
> 
> diff --git a/tools/libxl/libxl_vm_genid.c b/tools/libxl/libxl_vm_genid.c
> new file mode 100644
> index 0000000..378dcf2
> --- /dev/null
> +++ b/tools/libxl/libxl_vm_genid.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (C) 2014 Citrix Systems R&D Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +#include <xenctrl.h>
> +#include <xen/hvm/params.h>
> +
> +/*
> + * Set a HVM domain's VM Generation ID.
> + *
> + * For further details, refer to the "Virtual Machine Generation ID"
> + * document from Microsoft.
> + * 
> + *   http://www.microsoft.com/en-us/download/details.aspx?id=30707
> + */
> +int libxl__vm_generation_id_set(libxl_ctx *ctx, uint32_t domid,
> +                                const libxl_uuid *uuid)

Internal (libxl__) functions should take a libxl__gc *gc rather than a
libxl_ctx, which means you can then drop all the GC_INIT stuff here and
rely on the callers gc..

> +{
> +    GC_INIT(ctx);
> +    char *dom_path;
> +    char *key;

Both of these can be const I think.

> +    uint64_t genid[2];
> +    uint64_t paddr = 0;
> +    int rc;
> +
> +    memcpy(genid, libxl_uuid_bytearray((libxl_uuid *)uuid), 16);

Please can you make libxl_uuid_bytearray const correct instead of
casting away the const here.

Do you not need to worry about endianess when memcpy'ing out of a uuid?

> +
> +    /*
> +     * Set the "platform/generation-id" XenStore key to pass the ID to
> +     * hvmloader.
> +     */
> +    dom_path = libxl__xs_get_dompath(gc, domid);
> +    if (!dom_path) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    key = libxl__sprintf(gc, "%s/platform/generation-id", dom_path);

Please use GCSPRINTF(). That might even shorten things enough that you
can inline it into the xs_write call.

> +    rc = libxl__xs_write(gc, XBT_NULL, key, "%"PRIu64 ":%" PRIu64,
> +                         genid[0], genid[1]);
> +    if (rc < 0)
> +        goto out;
> +
> +    /*
> +     * Update the ID in guest memory (if available).
> +     */
> +    xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_VM_GENERATION_ID_ADDR, 
> &paddr);
> +    if (paddr) {
> +        void *vaddr;
> +
> +        vaddr = xc_map_foreign_range(ctx->xch, domid, XC_PAGE_SIZE,
> +                                     PROT_READ | PROT_WRITE,
> +                                     paddr >> XC_PAGE_SHIFT);
> +        if (vaddr == NULL) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +        memcpy(vaddr + (paddr & ~XC_PAGE_MASK), genid, 2 * sizeof(*genid));
> +        munmap(vaddr, XC_PAGE_SIZE);
> +
> +        /*
> +         * The spec requries an ACPI Notify event is injected into the

"requires"

> +         * guest when the generation ID is changed.
> +         *
> +         * This is only called for domains that are suspended or newly
> +         * created and they won't be in a state to receive such an
> +         * event.
> +         */
> +    }
> +
> +    rc = 0;
> +
> +  out:
> +    GC_FREE;
> +    return rc;
> +}



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