Re: [Qemu-devel] [PATCH v5 05/10] ACPI: Add Virtual Machine Generation ID support

[Ben Warren]

> On Feb 6, 2017, at 9:41 AM, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Mon, Feb 06, 2017 at 09:29:30AM -0800, Ben Warren wrote:
>> 
>>    On Feb 6, 2017, at 8:15 AM, Michael S. Tsirkin <address@hidden> wrote:
>> 
>>    On Sun, Feb 05, 2017 at 01:12:00AM -0800, address@hidden wrote:
>> 
>>        From: Ben Warren <address@hidden>
>> 
>>        This implements the VM Generation ID feature by passing a 128-bit
>>        GUID to the guest via a fw_cfg blob.
>>        Any time the GUID changes, an ACPI notify event is sent to the guest
>> 
>>        The user interface is a simple device with one parameter:
>>        - guid (string, must be "auto" or in UUID format
>>          xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)
>> 
>>        Signed-off-by: Ben Warren <address@hidden>
>> 
>> 
>> 
>>    Thanks!
>>    I think it's mostly in a good shape.
>>    Comments inside.
>> 
>> 
>> Thanks!  Hopefully the next rev has everything you want.
>> 
>> 
>> 
>>        ---
>>        default-configs/i386-softmmu.mak     |   1 +
>>        default-configs/x86_64-softmmu.mak   |   1 +
>>        hw/acpi/Makefile.objs                |   1 +
>>        hw/acpi/vmgenid.c                    | 206
>>        +++++++++++++++++++++++++++++++++++
>>        hw/i386/acpi-build.c                 |  10 ++
>>        include/hw/acpi/acpi_dev_interface.h |   1 +
>>        include/hw/acpi/vmgenid.h            |  37 +++++++
>>        7 files changed, 257 insertions(+)
>>        create mode 100644 hw/acpi/vmgenid.c
>>        create mode 100644 include/hw/acpi/vmgenid.h
>> 
>>        diff --git a/default-configs/i386-softmmu.mak b/default-configs/
>>        i386-softmmu.mak
>>        index 384cefb..1a43542 100644
>>        --- a/default-configs/i386-softmmu.mak
>>        +++ b/default-configs/i386-softmmu.mak
>>        @@ -58,3 +58,4 @@ CONFIG_I82801B11=y
>>        CONFIG_SMBIOS=y
>>        CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>>        CONFIG_PXB=y
>>        +CONFIG_ACPI_VMGENID=y
>>        diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/
>>        x86_64-softmmu.mak
>>        index 491a191..aee8b08 100644
>>        --- a/default-configs/x86_64-softmmu.mak
>>        +++ b/default-configs/x86_64-softmmu.mak
>>        @@ -58,3 +58,4 @@ CONFIG_I82801B11=y
>>        CONFIG_SMBIOS=y
>>        CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>>        CONFIG_PXB=y
>>        +CONFIG_ACPI_VMGENID=y
>>        diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>>        index 6acf798..11c35bc 100644
>>        --- a/hw/acpi/Makefile.objs
>>        +++ b/hw/acpi/Makefile.objs
>>        @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
>>        common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>>        common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>>        common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>>        +common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>>        common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>> 
>>        common-obj-y += acpi_interface.o
>>        diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>        new file mode 100644
>>        index 0000000..6c9ecfd
>>        --- /dev/null
>>        +++ b/hw/acpi/vmgenid.c
>>        @@ -0,0 +1,206 @@
>>        +/*
>>        + *  Virtual Machine Generation ID Device
>>        + *
>>        + *  Copyright (C) 2017 Skyport Systems.
>>        + *
>>        + *  Author: Ben Warren <address@hidden>
>>        + *
>>        + * This work is licensed under the terms of the GNU GPL, version 2 or
>>        later.
>>        + * See the COPYING file in the top-level directory.
>>        + *
>>        + */
>>        +
>>        +#include "qemu/osdep.h"
>>        +#include "qmp-commands.h"
>>        +#include "hw/acpi/acpi.h"
>>        +#include "hw/acpi/aml-build.h"
>>        +#include "hw/acpi/vmgenid.h"
>>        +#include "hw/nvram/fw_cfg.h"
>>        +#include "sysemu/sysemu.h"
>>        +
>>        +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker
>>        *linker)
>>        +{
>>        +    Object *obj;
>>        +    VmGenIdState *s;
>>        +    Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
>>        +    uint32_t vgia_offset;
>>        +
>>        +    obj = find_vmgenid_dev(NULL);
>>        +    assert(obj);
>>        +    s = VMGENID(obj);
>>        +
>>        +    /* Fill in the GUID values */
>>        +    if (guid->len != VMGENID_FW_CFG_SIZE) {
>>        +        g_array_set_size(guid, VMGENID_FW_CFG_SIZE);
>>        +    }
>>        +    g_array_insert_vals(guid, VMGENID_GUID_OFFSET, s->guid.data, 16);
>> 
>> 
>>    ARRAY_SIZE(s->guid.data);
>> 
>> 
>> OK
>> 
>>        +
>>        +    /* Put this in a separate SSDT table */
>>        +    ssdt = init_aml_allocator();
>>        +
>>        +    /* Reserve space for header */
>>        +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>>        +
>>        +    /* Storage for the GUID address */
>>        +    vgia_offset = table_data->len +
>>        +        build_append_named_dword(ssdt->buf, "VGIA");
>>        +    scope = aml_scope("\\_SB");
>>        +    dev = aml_device("VGEN");
>>        +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
>>        +    aml_append(dev, aml_name_decl("_CID", aml_string
>>        ("VM_Gen_Counter")));
>>        +    aml_append(dev, aml_name_decl("_DDN", aml_string
>>        ("VM_Gen_Counter")));
>>        +
>>        +    /* Simple status method to check that address is linked and
>>        non-zero */
>>        +    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>>        +    addr = aml_local(0);
>>        +    aml_append(method, aml_store(aml_int(0xf), addr));
>>        +    if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
>>        +    aml_append(if_ctx, aml_store(aml_int(0), addr));
>>        +    aml_append(method, if_ctx);
>>        +    aml_append(method, aml_return(addr));
>>        +    aml_append(dev, method);
>>        +
>>        +    /* the ADDR method returns two 32-bit words representing the 
>> lower
>>        and
>>        +     * upper halves * of the physical address of the fw_cfg blob
>>        +     * (holding the GUID) */
>> 
>> 
>>    /*
>>    * multiline comments
>>    * look like this
>>    */
>> 
>> 
>> OK
>> 
>>        +    method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
>>        +
>>        +    addr = aml_local(0);
>>        +    aml_append(method, aml_store(aml_package(2), addr));
>>        +
>>        +    aml_append(method, aml_store(aml_add(aml_name("VGIA"),
>>        +                                         
>> aml_int(VMGENID_GUID_OFFSET),
>>        NULL),
>>        +                                 aml_index(addr, aml_int(0))));
>>        +    aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int
>>        (1))));
>>        +    aml_append(method, aml_return(addr));
>>        +
>>        +    aml_append(dev, method);
>>        +    aml_append(scope, dev);
>>        +    aml_append(ssdt, scope);
>>        +
>>        +    /* attach an ACPI notify */
>>        +    method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED);
>>        +    aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int
>>        (0x80)));
>>        +    aml_append(ssdt, method);
>>        +
>>        +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
>>        +
>>        +    /* Allocate guest memory for the Data fw_cfg blob */
>>        +    bios_linker_loader_alloc(linker, VMGENID_GUID_FW_CFG_FILE, guid,
>>        4096,
>>        +                             false /* page boundary, high memory */);
>>        +
>>        +    /* Patch address of GUID fw_cfg blob into the ADDR fw_cfg blob */
>> 
>> 
>>    add ... so QEMU can write GUID there
>> 
>> 
>> OK
>> 
>>        +    bios_linker_loader_add_pointer(linker,
>>        +        VMGENID_ADDR_FW_CFG_FILE, 0, sizeof(uint32_t),
>>        +        VMGENID_GUID_FW_CFG_FILE, 0, true);
>>        +
>>        +    /* Patch address of GUID fw_cfg blob into the AML */
>> 
>> 
>>    add ... so OSPM can retrieve and read it
>> 
>> 
>> OK
>> 
>>        +    bios_linker_loader_add_pointer(linker,
>>        +        ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t),
>>        +        VMGENID_GUID_FW_CFG_FILE, 0, false);
>>        +
>>        +    build_header(linker, table_data,
>>        +        (void *)(table_data->data + table_data->len - 
>> ssdt->buf->len),
>>        +        "SSDT", ssdt->buf->len, 1, NULL, "VMGENID");
>>        +    free_aml_allocator();
>>        +}
>>        +
>>        +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid)
>>        +{
>>        +    Object *obj = find_vmgenid_dev(NULL);
>>        +    assert(obj);
>>        +    VmGenIdState *vms = VMGENID(obj);
>>        +
>>        +    /* Create a read-only fw_cfg file for GUID */
>>        +    fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data,
>>        +                    VMGENID_FW_CFG_SIZE);
>>        +    /* Create a read-write fw_cfg file for Address */
>>        +    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
>> 
>> 
>>    Seems wrong. What if guest updates the address after command line
>>    set it? You want a callback to copy guid there.
>> 
>> 
>> Sure, I can do that.  My understanding was that this is a read callback, but 
>> if
>> it also is called upon a write, we should do what you suggest.
> 
> Hmm you are right. But we really need to call something
> on write though - unlike read, it must be called after write.
> Otherwise I don't see how it can work if you set gen id before
> guest boots.
> 
> I guess this means we need yet another callback per file.
> FWCfgWriteCallback ?
> 
> Can you implement this in hw/nvram/fw_cfg.c?
> It's rather straight-forward to do.
> 
The reason it works is that we put the initial contents of the GUID (as 
supplied by command-line) into the GUID fw_cfg in the ‘vmgenid_build_acpi()’ 
function, which is guaranteed to happen before the guest boots.  The only time 
QEMU needs to know VGIA is on later updates to the GUID (via monitor) or when 
restoring.  If you really think this extra complexity is needed, I can do so, 
but it seems to work very well as-is.
> 
>> 
>> 
>>        +                             vms->vgia_le, sizeof(uint32_t), false);
>> 
>> 
>>    Should be ARRAY_SIZE(vms->vgia_le).
>> 
>>    Also, I thought GUID is at an offset from the beginning
>>    in order to trick OVMF, isn't it?
>> 
>> 
>> The offset is taken into account in the GUID fw_cfg file.  The writing to 
>> guest
>> memory, we add the offset to VGIA.
> 
> I keep forgetting it's the address. Really please rename it
> using full words.
> 
Will do.
> 
>> 
>>        +}
>>        +
>>        +static void vmgenid_update_guest(VmGenIdState *s)
>>        +{
>>        +    Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF,
>>        NULL);
>>        +    uint32_t vgia;
>>        +
>>        +    if (obj) {
>>        +        /* Write the GUID to guest memory */
>>        +        memcpy(&vgia, s->vgia_le, sizeof(vgia));
>>        +        vgia = le32_to_cpu(vgia);
>>        +        if (vgia) {
>> 
>> 
>>    add a comment saying 0 means bios did not run yet.
>> 
>> OK
>> 
>> 
>> 
>>        +            cpu_physical_memory_write(vgia + VMGENID_GUID_OFFSET,
>>        +                                      s->guid.data, sizeof(s->
>>        guid.data));
>> 
>> 
>>    Avoid this. You want dma.h APIs.
>> 
>> I’ll have a look at those.  This function was recommended to me in an earlier
>> patch version.
>> 
>> 
>> 
>>        +            /* Send _GPE.E05 event */
>>        +            acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);
>>        +        }
>>        +    }
>>        +}
>>        +
>>        +static void vmgenid_set_guid(Object *obj, const char *value, Error
>>        **errp)
>>        +{
>>        +    VmGenIdState *s = VMGENID(obj);
>>        +
>>        +    if (!strncmp(value, "auto", 4)) {
>> 
>> 
>>    I'd use strcmp here.
>> 
>> OK
>> 
>> 
>> 
>>        +        qemu_uuid_generate(&s->guid);
>>        +    } else if (qemu_uuid_parse(value, &s->guid) < 0) {
>>        +        error_setg(errp, "'%s. %s': Failed to parse GUID string: %s",
>>        +                   object_get_typename(OBJECT(s)), VMGENID_GUID,
>>        value);
>>        +        return;
>>        +    }
>>        +    /* QemuUUID has the first three words as big-endian, and expect
>>        that any
>>        +     * GUIDs passed in will always be BE.  The guest, however will
>>        expect
>>        +     * the fields to be little-endian, so store that way internally.
>> 
>> 
>>    This makes my head spin. It's the actual guid, right? Only place we need 
>> it
>>    is when
>>    we copy it into guest memory. So swap there -
>>    and do it to a copy so you won't need these comments.
>> 
>> I understand.  I wanted to only do the swap at one place and decided to do it
>> closest to the user interface, but can do it at the other end instead.
>> 
>> 
>> 
>>        Make
>>        +     * sure to swap back whenever reporting via monitor */
>> 
>> 
>>    And what code does this swap back?
>> 
>> A later patch where the QMP code is added.  But I’ll remove that necessity.
>> 
>> 
>> 
>>        +    qemu_uuid_bswap(&s->guid);
>>        +
>>        +    /* Send the ACPI notify */
>>        +    vmgenid_update_guest(s);
>>        +}
>>        +
>>        +/* After restoring an image, we need to update the guest memory and
>>        notify
>>        + * it of a potential change to VM Generation ID */
>>        +static int vmgenid_post_load(void *opaque, int version_id)
>>        +{
>>        +    VmGenIdState *s = opaque;
>>        +    vmgenid_update_guest(s);
>>        +    return 0;
>>        +}
>>        +
>>        +static const VMStateDescription vmstate_vmgenid = {
>>        +    .name = "vmgenid",
>>        +    .version_id = 1,
>>        +    .minimum_version_id = 1,
>>        +    .post_load = vmgenid_post_load,
>>        +    .fields = (VMStateField[]) {
>>        +        VMSTATE_UINT8_ARRAY(vgia_le, VmGenIdState, sizeof(uint32_t)),
>>        +        VMSTATE_END_OF_LIST()
>>        +    },
>>        +};
>>        +
>>        +static void vmgenid_initfn(Object *obj)
>>        +{
>>        +    object_property_add_str(obj, VMGENID_GUID, NULL, 
>> vmgenid_set_guid,
>>        NULL);
>>        +}
>>        +
>>        +static void vmgenid_device_class_init(ObjectClass *klass, void *data)
>>        +{
>>        +    DeviceClass *dc = DEVICE_CLASS(klass);
>>        +
>>        +    dc->vmsd = &vmstate_vmgenid;
>>        +}
>>        +
>>        +static const TypeInfo vmgenid_device_info = {
>>        +    .name          = VMGENID_DEVICE,
>>        +    .parent        = TYPE_SYS_BUS_DEVICE,
>>        +    .instance_size = sizeof(VmGenIdState),
>>        +    .instance_init = vmgenid_initfn,
>>        +    .class_init    = vmgenid_device_class_init,
>>        +};
>>        +
>>        +static void vmgenid_register_types(void)
>>        +{
>>        +    type_register_static(&vmgenid_device_info);
>>        +}
>>        +
>>        +type_init(vmgenid_register_types)
>>        diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>        index 78a1d84..4c40f76 100644
>>        --- a/hw/i386/acpi-build.c
>>        +++ b/hw/i386/acpi-build.c
>>        @@ -42,6 +42,7 @@
>>        #include "hw/acpi/memory_hotplug.h"
>>        #include "sysemu/tpm.h"
>>        #include "hw/acpi/tpm.h"
>>        +#include "hw/acpi/vmgenid.h"
>>        #include "sysemu/tpm_backend.h"
>>        #include "hw/timer/mc146818rtc_regs.h"
>>        #include "sysemu/numa.h"
>>        @@ -2653,6 +2654,11 @@ void acpi_build(AcpiBuildTables *tables,
>>        MachineState *machine)
>>            acpi_add_table(table_offsets, tables_blob);
>>            build_madt(tables_blob, tables->linker, pcms);
>> 
>>        +    if (find_vmgenid_dev(NULL)) {
>>        +        acpi_add_table(table_offsets, tables_blob);
>>        +        vmgenid_build_acpi(tables_blob, tables->vmgenid, tables->
>>        linker);
>>        +    }
>>        +
>>            if (misc.has_hpet) {
>>                acpi_add_table(table_offsets, tables_blob);
>>                build_hpet(tables_blob, tables->linker);
>>        @@ -2859,6 +2865,10 @@ void acpi_setup(void)
>>            fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>                            tables.tcpalog->data, acpi_data_len
>>        (tables.tcpalog));
>> 
>>        +    if (find_vmgenid_dev(NULL)) {
>>        +        vmgenid_add_fw_cfg(pcms->fw_cfg, tables.vmgenid);
>>        +    }
>>        +
>>            if (!pcmc->rsdp_in_ram) {
>>                /*
>>                 * Keep for compatibility with old machine types.
>>        diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/
>>        acpi_dev_interface.h
>>        index 71d3c48..3c2e4e9 100644
>>        --- a/include/hw/acpi/acpi_dev_interface.h
>>        +++ b/include/hw/acpi/acpi_dev_interface.h
>>        @@ -11,6 +11,7 @@ typedef enum {
>>            ACPI_CPU_HOTPLUG_STATUS = 4,
>>            ACPI_MEMORY_HOTPLUG_STATUS = 8,
>>            ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>>        +    ACPI_VMGENID_CHANGE_STATUS = 32,
>>        } AcpiEventStatusBits;
>> 
>>        #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
>>        diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
>>        new file mode 100644
>>        index 0000000..b60437a
>>        --- /dev/null
>>        +++ b/include/hw/acpi/vmgenid.h
>>        @@ -0,0 +1,37 @@
>>        +#ifndef ACPI_VMGENID_H
>>        +#define ACPI_VMGENID_H
>>        +
>>        +#include "hw/acpi/bios-linker-loader.h"
>>        +#include "hw/sysbus.h"
>>        +#include "qemu/uuid.h"
>>        +
>>        +#define VMGENID_DEVICE           "vmgenid"
>>        +#define VMGENID_GUID             "guid"
>>        +#define VMGENID_GUID_FW_CFG_FILE      "etc/vmgenid"
>> 
>> 
>>    Maybe vmgenid_guid ?
>> 
>> Sure.
>> 
>> 
>> 
>>        +#define VMGENID_ADDR_FW_CFG_FILE      "etc/vmgenid_addr"
>>        +
>>        +#define VMGENID_FW_CFG_SIZE      4096 /* Occupy a page of memory */
>>        +#define VMGENID_GUID_OFFSET      40   /* allow space for
>>        +                                       * OVMF SDT Header Probe
>>        Supressor */
>> 
>> 
>>    ACPI header size is 36 bytes. I'd expect just to use that.
>>    Where does 40 come from? I think it's an 8 bytes alignment requirement.
>> 
>> 
>> The number 40 was asked for by Laszlo for the OVMF SDT Header Probe 
>> Suppressor,
>> which I don’t know anything about.
> 
> I think I do though :)
> 
>> This GUID isn’t going in an ACPI table, so
>> I’m not sure what you mean here.  We allocate on a page boundary and 40 bytes
>> in should be 8-byte aligned.
>> 
>>    So do QEMU_ALIGN_UP(sizeof(AcpiTableHeader), 8)
>> 
>>    Add a comment explaining that 8.
> 
> Laszlo could you suggest a comment explaining a bit about what
> OVMF does here?
> 
>> 
>>        +
>>        +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid);
>>        +void vmgenid_build_acpi(GArray *table_data, GArray *guid, BIOSLinker
>>        *linker);
>>        +
>>        +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), 
>> VMGENID_DEVICE)
>>        +
>>        +typedef struct VmGenIdState {
>>        +    SysBusDevice parent_obj;
>>        +    QemuUUID guid;
>>        +    uint8_t vgia_le[4];
>> 
>> 
>>    Please give this fields a better name. Is this the address?
>>    Pls add comments, too.
>> 
>> 
>> This variable name was suggested by Laszlo.  the ‘a’ in ‘vgia’ refers to
>> address, but I’ll add some comments.
> 
> vmgenid_addr_le?
> 
> 
>>    How about we make it 8 byte so it's future proof?
>> 
>> I can do that, but a previous conversation we had made it clear that guests
>> would never allocate above 4GB so 64 bits wasn’t necessary.
> 
> Right, it's just very painful to change once we make it 32 bit.
> 
OK, make sense.  I’ll wait to hear from Laszlo before continuing.
>>    alternatively put it in a ram block instead.
>> 
>> 
>>        +} VmGenIdState;
>>        +
>>        +static Object *find_vmgenid_dev(Error **errp)
>>        +{
>>        +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
>>        +    if (!obj && errp) {
>>        +        error_setg(errp, "%s is not found", VMGENID_DEVICE);
>>        +    }
>>        +    return obj;
>>        +}
>>        +
>>        +#endif
>>        -- 
>>        2.7.4

smime.p7s
Description: S/MIME cryptographic signature