Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Igor Mammedov]

On Mon, 27 Apr 2015 14:19:50 +0300
Gal Hammer <address@hidden> wrote:

> Based on Microsoft's sepecifications (paper can be dowloaded from
> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
> description to the SSDT ACPI table and its implementation.
> 
> The GUID is set using a global "vmgenid.uuid" parameter.
> 
> Signed-off-by: Gal Hammer <address@hidden>
> ---
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  hw/i386/acpi-build.c               |  41 +++++++++++++
>  hw/i386/pc.c                       |   8 +++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 116 
> +++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h               |   3 +
>  include/hw/misc/vmgenid.h          |  21 +++++++
>  8 files changed, 192 insertions(+)
>  create mode 100644 hw/misc/vmgenid.c
>  create mode 100644 include/hw/misc/vmgenid.h
> 
> diff --git a/default-configs/i386-softmmu.mak 
> b/default-configs/i386-softmmu.mak
> index 6a74e00..9fc0a3c 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak 
> b/default-configs/x86_64-softmmu.mak
> index 46b87dd..f66fd3c 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e761005..8d1a761 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/misc/vmgenid.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    bool vm_generation_id_set;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->has_tpm = tpm_find();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->vm_generation_id_set = vm_generation_id_set();
>  }
>  
>  static void acpi_get_pci_info(PcPciInfo *info)
> @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(ssdt, scope);
>      }
>  
> +    if (misc->vm_generation_id_set) {
would be better to make it conditional depending on if vmgenid device is present

> +        scope = aml_scope("\\_SB");
> +
> +        dev = aml_device("VMGI");

[...]

> +
> +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
> +    if (vmgenid) {
> +        qdev_init_nofail(vmgenid);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
> +    }
it should be possible not to create device if user haven't asked for it,
my guess is that you are using sysbus device as a convenience for mapping.

I'd suggest to use plain Device type instead, with it
vmgenid would be created when management asks for it using CLI:
  -device vmgenid,uuid=XXXX
and from mgmt side it would treat device as any other device using the same 
APIs.

Mapping device to board is also simple, for example see:

pc_machine_device_plug_cb()->pc_dimm_plug()->memory_region_add_subregion()

for PC machine pc_machine_device_plug_cb() callback is called for
every bus-less device during device_realize() time
   hw/core/qdev.c:hotplug_handler_plug(hotplug_ctrl, dev, &local_err);

you just need to add something like:
  pc_vmgenid_plug()
        addr = object_property_get_int(dev, VMGENID_BASE_ADDR_PROP, &local_err);
        VMGenIdDeviceClass vmgidc = CAST(dev)
        memory_region_add_subregion(&system_memory, addr, 
vmgidc->get_memory_region());

[...]


> +
> +#define VMGENID_BASE_ADDRESS    0xfedf0000
I'd make it a property, so that every target would be able to set
its own value when times come and ACPI part would automatically pick it up,
it also helps with test cases.

> +#define VMGENID_BASE_ADDR_LEN   16
> +
> +#endif