On 03/02/15 18:05, Igor Mammedov wrote:
> On Sun, 1 Mar 2015 16:09:33 +0100
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
>> On Wed, Feb 25, 2015 at 05:08:52PM +0000, Igor Mammedov 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 "vmgenid.uuid" property.
>>>
>>> Example of using vmgenid device:
>>> -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>>
>> If you do this, doesn't windows then prompt for a driver?
> it doesn't since PCI_CLASS_MEMORY_RAM is displayed as driver less
> "PCI standard RAM Controller" binding in device manager.
>
> There was an issue with
> virtio balloon device + pseries firmware + kernel bug
> http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> but it shouldn't be an issue for x86 targets with which device is
> supposed to be used.
> CCing David and Laszlo in case UEFI might do some crazy stuff like
> pseries firmware.
UEFI doing crazy stuff? Never. :)
But, I need more context. Is this a new PCI device with class code
PCI_CLASS_MEMORY_RAM? I grepped edk2 for it, and for
PCI_CLASS_MEMORY_CONTROLLER too. Only the macro definitions are there, I
don't see them used anywhere.
If I wanted to add different kinds of RAM to the system, I'd have to
produce new memory resource descriptor HOBs (hand-off blocks) during
PEI, or call gDS->AddMemorySpace() during DXE. Since I won't do that, I
expect the addition of just another PCI device to QEMU will simply go
unnoticed in OVMF (with there being no particular driver for it in OVMF).
So, unless I misunderstood something, this should be safe to do in qemu.
Thanks
Laszlo
>
>>
>>
>>> To change uuid in runtime use:
>>> qom-set "/machine/peripheral/FOO.uuid"
>>> "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>>>
>>> 'vmgenid' device initialization flow is as following:
>>> 1. vmgenid has RAM BAR resistered with size of UUID buffer
>>> 2. BIOS initializes PCI devices and it maps BAR in PCI hole
>>> 3. BIOS reads ACPI tables from QEMU, at that moment tables
>>> are generated with \_SB.VMGI.ADDR constant pointing to
>>> HPA where BIOS's mapped vmgenid's BAR earlier
>>>
>>> Signed-off-by: Gal Hammer <gham...@redhat.com>
>>> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
>>> ---
>>> v2:
>>> * rewrite to use PCIDevice so that we don't have to mess
>>> with complex fwcfg/linker and patch ACPI tables then
>>> read VMGENID buffer adddress in guest OSPM and communicate
>>> it to QEMU via reserved MMIO region.
>>> Which also allows us to write a more complete unit test
>>> that wouldn't require to run OSPM so that it could update
>>> HPA in QEMU.
>>> * make 'vmgenid' optional, users who want to use it
>>> should add -device vmgenid,.... to QEMU CLI
>>> it also saves us some space in SSDT if device is not used
>>> * mark UUID buffer as dirty when it's updated via QMP in runtime
>>> * make 'uiid' property mandatory at -device
>>> ---
>>> default-configs/i386-softmmu.mak | 1 +
>>> default-configs/x86_64-softmmu.mak | 1 +
>>> docs/specs/pci-ids.txt | 1 +
>>> hw/i386/acpi-build.c | 33 ++++++++++
>>> hw/i386/acpi-dsdt.dsl | 2 -
>>> hw/i386/q35-acpi-dsdt.dsl | 2 -
>>> hw/misc/Makefile.objs | 1 +
>>> hw/misc/vmgenid.c | 130
>>> +++++++++++++++++++++++++++++++++++++
>>> include/hw/acpi/acpi.h | 1 +
>>> include/hw/misc/vmgenid.h | 21 ++++++
>>> include/hw/pci/pci.h | 1 +
>>> 11 files changed, 190 insertions(+), 4 deletions(-)
>>> 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 bd99af9..0b913a8 100644
>>> --- a/default-configs/i386-softmmu.mak
>>> +++ b/default-configs/i386-softmmu.mak
>>> @@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
>>> CONFIG_ICC_BUS=y
>>> CONFIG_PVPANIC=y
>>> CONFIG_MEM_HOTPLUG=y
>>> +CONFIG_VMGENID=y
>>> diff --git a/default-configs/x86_64-softmmu.mak
>>> b/default-configs/x86_64-softmmu.mak
>>> index e7c2734..de5e6af 100644
>>> --- a/default-configs/x86_64-softmmu.mak
>>> +++ b/default-configs/x86_64-softmmu.mak
>>> @@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
>>> CONFIG_ICC_BUS=y
>>> CONFIG_PVPANIC=y
>>> CONFIG_MEM_HOTPLUG=y
>>> +CONFIG_VMGENID=y
>>> diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
>>> index c6732fe..b398c5d 100644
>>> --- a/docs/specs/pci-ids.txt
>>> +++ b/docs/specs/pci-ids.txt
>>> @@ -46,6 +46,7 @@ PCI devices (other than virtio):
>>> 1b36:0004 PCI Quad-port 16550A adapter (docs/specs/pci-serial.txt)
>>> 1b36:0005 PCI test device (docs/specs/pci-testdev.txt)
>>> 1b36:0007 PCI SD Card Host Controller Interface (SDHCI)
>>> +1b36:0009 PCI VM-Generation device
>>>
>>> All these devices are documented in docs/specs.
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 4d5d7e3..9290ae3 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -50,6 +50,7 @@
>>> #include "hw/pci/pci_bus.h"
>>> #include "hw/pci-host/q35.h"
>>> #include "hw/i386/intel_iommu.h"
>>> +#include "hw/misc/vmgenid.h"
>>>
>>> #include "hw/i386/q35-acpi-dsdt.hex"
>>> #include "hw/i386/acpi-dsdt.hex"
>>> @@ -110,6 +111,7 @@ typedef struct AcpiPmInfo {
>>> } AcpiPmInfo;
>>>
>>> typedef struct AcpiMiscInfo {
>>> + uint32_t vmgen_buf_paddr;
>>> bool has_hpet;
>>> bool has_tpm;
>>> DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
>>> @@ -245,6 +247,14 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>>>
>>> static void acpi_get_misc_info(AcpiMiscInfo *info)
>>> {
>>> + Object *obj;
>>> +
>>> + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
>>> + info->vmgen_buf_paddr = 0;
>>> + if (obj) {
>>> + info->vmgen_buf_paddr =
>>> + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
>>> + }
>>> info->has_hpet = hpet_find();
>>> info->has_tpm = tpm_find();
>>> info->pvpanic_port = pvpanic_port();
>>> @@ -981,6 +991,29 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>
>>> sb_scope = aml_scope("_SB");
>>> {
>>> + if (misc->vmgen_buf_paddr) {
>>> + dev = aml_device("VMGI");
>>> +
>>> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0003")));
>>> + aml_append(dev, aml_name_decl("_CID",
>>> aml_string("VM_Gen_Counter")));
>>> + aml_append(dev, aml_name_decl("_DDN",
>>> aml_string("VM_Gen_Counter")));
>>> +
>>> + pkg = aml_package(2);
>>> + /* low 32 bits of UUID buffer addr*/
>>> + aml_append(pkg, aml_int(misc->vmgen_buf_paddr));
>>> + aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer
>>> addr */
>>
>> Really should be full 64 bit, and use a 64 bit BAR.
> for compatibility with 32 windows guest, it probably should stay below
> 4Gb address space.
>
>>
>>
>>> + aml_append(dev, aml_name_decl("ADDR", pkg));
>>> +
>>> + aml_append(sb_scope, dev);
>>> +
>>> + scope = aml_scope("\\_GPE");
>>> + method = aml_method("_E00", 0);
>>> + aml_append(method,
>>> + aml_notify(aml_name("\\_SB.VMGI"), aml_int(0x80)));
>>> + aml_append(scope, method);
>>> + aml_append(ssdt, scope);
>>> + }
>>> +
>>
>> This confuses me.
>> Shouldn't VMGI device be on PCI bus, where it was
>> added?
> I'll try to merge it with PCI slot description that associated with this
> device,
> Though I'm not sure that windows would handle it correctly.
>
>>
>>
>>
>>> /* create PCI0.PRES device and its _CRS to reserve CPU hotplug
>>> MMIO */
>>> dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
>>> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
>>> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
>>> index a611e07..884038b 100644
>>> --- a/hw/i386/acpi-dsdt.dsl
>>> +++ b/hw/i386/acpi-dsdt.dsl
>>> @@ -306,8 +306,6 @@ DefinitionBlock (
>>> Scope(\_GPE) {
>>> Name(_HID, "ACPI0006")
>>>
>>> - Method(_L00) {
>>> - }
>>> Method(_E01) {
>>> // PCI hotplug event
>>> Acquire(\_SB.PCI0.BLCK, 0xFFFF)
>>> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
>>> index e1cee5d..9eb794c 100644
>>> --- a/hw/i386/q35-acpi-dsdt.dsl
>>> +++ b/hw/i386/q35-acpi-dsdt.dsl
>>> @@ -414,8 +414,6 @@ DefinitionBlock (
>>> Scope(\_GPE) {
>>> Name(_HID, "ACPI0006")
>>>
>>> - Method(_L00) {
>>> - }
>>> Method(_L01) {
>>> }
>>> Method(_E02) {
>>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>>> index 029a56f..e047aea 100644
>>> --- a/hw/misc/Makefile.objs
>>> +++ b/hw/misc/Makefile.objs
>>> @@ -41,3 +41,4 @@ obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>>>
>>> obj-$(CONFIG_PVPANIC) += pvpanic.o
>>> obj-$(CONFIG_EDU) += edu.o
>>> +obj-$(CONFIG_VMGENID) += vmgenid.o
>>> diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
>>> new file mode 100644
>>> index 0000000..631c9a3
>>> --- /dev/null
>>> +++ b/hw/misc/vmgenid.c
>>> @@ -0,0 +1,130 @@
>>> +/*
>>> + * Virtual Machine Generation ID Device
>>> + *
>>> + * Copyright (C) 2014 Red Hat Inc.
>>> + *
>>> + * Authors: Gal Hammer <gham...@redhat.com>
>>> + * Igor Mammedov <imamm...@redhat.com>
>>> + *
>>> + * 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 "hw/i386/pc.h"
>>> +#include "hw/pci/pci.h"
>>> +#include "hw/misc/vmgenid.h"
>>> +#include "hw/acpi/acpi.h"
>>> +#include "qapi/visitor.h"
>>> +
>>> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
>>> +
>>> +typedef struct VmGenIdState {
>>> + PCIDevice parent_obj;
>>> + MemoryRegion iomem;
>>> + uint8_t guid[16];
>>> + bool guid_set;
>>> +} VmGenIdState;
>>> +
>>> +static void vmgenid_update_guest(VmGenIdState *s)
>>> +{
>>> + Object *acpi_obj;
>>> + void *ptr = memory_region_get_ram_ptr(&s->iomem);
>>> +
>>> + memcpy(ptr, &s->guid, sizeof(s->guid));
>>> + memory_region_set_dirty(&s->iomem, 0, sizeof(s->guid));
>>> +
>>> + acpi_obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
>>> + if (acpi_obj) {
>>> + AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(acpi_obj);
>>> + AcpiDeviceIf *adev = ACPI_DEVICE_IF(acpi_obj);
>>> + ACPIREGS *acpi_regs = adevc->regs(adev);
>>> +
>>> + acpi_regs->gpe.sts[0] |= 1; /* _GPE.E00 handler */
>>> + acpi_update_sci(acpi_regs, adevc->sci(adev));
>>> + }
>>> +}
>>> +
>>> +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
>>> +{
>>> + VmGenIdState *s = VMGENID(obj);
>>> +
>>> + if (qemu_uuid_parse(value, s->guid) < 0) {
>>> + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse UUID
>>> string.",
>>> + object_get_typename(OBJECT(s)));
>>> + return;
>>> + }
>>> +
>>> + s->guid_set = true;
>>> + vmgenid_update_guest(s);
>>> +}
>>> +
>>> +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void *opaque,
>>> + const char *name, Error **errp)
>>> +{
>>> + VmGenIdState *s = VMGENID(obj);
>>> + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
>>> +
>>> + if (value == PCI_BAR_UNMAPPED) {
>>> + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not initialized",
>>> + object_get_typename(OBJECT(s)));
>>
>> Looks wrong - this is guest configuration.
>> I don't think we want to print errors and exit in this way just
>> because guest did not map the device.
>> Better to mask it in acpi.
> that would mean that user asked to run guest with VMGID but BIOS failed
> to do so for some reason and error was just ignored.
> I think it's better to abort guest at early start-up stage
> i.e. when it loads ACPI tables, than proceed with invalid state
> silently.
>
>>
>>
>>> + return;
>>> + }
>>> + visit_type_int(v, &value, name, errp);
>>> +}
>>> +
>>> +static void vmgenid_initfn(Object *obj)
>>> +{
>>> + VmGenIdState *s = VMGENID(obj);
>>> +
>>> + memory_region_init_ram(&s->iomem, obj, "vgid.bar", sizeof(s->guid),
>>> + &error_abort);
>>> +
>>> + object_property_add_str(obj, VMGENID_UUID, NULL, vmgenid_set_uuid,
>>> NULL);
>>> + object_property_add(obj, VMGENID_VMGID_ADDR, "int",
>>> vmgenid_get_vmgid_addr,
>>> + NULL, NULL, NULL, NULL);
>>> +}
>>> +
>>> +
>>> +static void vmgenid_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> + VmGenIdState *s = VMGENID(dev);
>>> +
>>> + if (!s->guid_set) {
>>> + error_setg(errp, "'%s." VMGENID_UUID "' property is not set",
>>> + object_get_typename(OBJECT(s)));
>>> + return;
>>> + }
>>> +
>>> + vmstate_register_ram(&s->iomem, DEVICE(s));
>>> + pci_register_bar(PCI_DEVICE(s), 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>> &s->iomem);
>>
>> This means this BAR is 16 bytes in size?
>>
>> Not good, should be full 4K.
> yep, it should be page
>
>>
>> You also make the BAR non prefetcheable, which makes
>> some OS-es (e.g. old linux) map it non-cacheable.
> we don't care about old linux here because it doesn't have
> vmgenid driver neither a modern linux but at least it could
> be added to it.
> But there shouldn't be a issue with tagging it as prefetchable.
>
>
>>
>>
>>
>>
>>
>>
>>> + return;
>>> +}
>>> +
>>> +static void vmgenid_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> +
>>> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>> + dc->hotpluggable = false;
>>> + k->realize = vmgenid_realize;
>>> + k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>> + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
>>> + k->class_id = PCI_CLASS_MEMORY_RAM;
>>> +}
>>> +
>>> +static const TypeInfo vmgenid_device_info = {
>>> + .name = VMGENID_DEVICE,
>>> + .parent = TYPE_PCI_DEVICE,
>>> + .instance_size = sizeof(VmGenIdState),
>>> + .instance_init = vmgenid_initfn,
>>> + .class_init = vmgenid_class_init,
>>> +};
>>> +
>>> +static void vmgenid_register_types(void)
>>> +{
>>> + type_register_static(&vmgenid_device_info);
>>> +}
>>> +
>>> +type_init(vmgenid_register_types)
>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>> index 1f678b4..a09cb3f 100644
>>> --- a/include/hw/acpi/acpi.h
>>> +++ b/include/hw/acpi/acpi.h
>>> @@ -25,6 +25,7 @@
>>> #include "qemu/option.h"
>>> #include "exec/memory.h"
>>> #include "hw/irq.h"
>>> +#include "hw/acpi/acpi_dev_interface.h"
>>>
>>> /*
>>> * current device naming scheme supports up to 256 memory devices
>>> diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
>>> new file mode 100644
>>> index 0000000..325f095
>>> --- /dev/null
>>> +++ b/include/hw/misc/vmgenid.h
>>> @@ -0,0 +1,21 @@
>>> +/*
>>> + * Virtual Machine Generation ID Device
>>> + *
>>> + * Copyright (C) 2014 Red Hat Inc.
>>> + *
>>> + * Authors: Gal Hammer <gham...@redhat.com>
>>> + * Igor Mammedov <imamm...@redhat.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#ifndef HW_MISC_VMGENID_H
>>> +#define HW_MISC_VMGENID_H
>>> +
>>> +#define VMGENID_DEVICE "vmgenid"
>>> +#define VMGENID_UUID "uuid"
>>> +#define VMGENID_VMGID_ADDR "vmgid-addr"
>>> +
>>> +#endif
>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>> index 3164fc3..245171b 100644
>>> --- a/include/hw/pci/pci.h
>>> +++ b/include/hw/pci/pci.h
>>> @@ -90,6 +90,7 @@
>>> #define PCI_DEVICE_ID_REDHAT_TEST 0x0005
>>> #define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
>>> #define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
>>> +#define PCI_DEVICE_ID_REDHAT_VMGENID 0x0009
>>> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
>>>
>>> #define FMT_PCIBUS PRIx64
>>> --
>>> 1.8.3.1
>>
>
