On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote: > On Tue, 3 Mar 2015 18:35:39 +0100 > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, 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" > > > > So this is not a great example since we are burning up > > a pci slot. Management can work around this by making this > > a multifunction device and making this part of chipset, > > but how about we make this a default, by specifying > > appropriate addr and multifunction properties > > as part of machine type. > Why make it default? It's some Windows specific thing that > should be used when guest is Windows to be of any use > and not present when it's not needed.
yes, but when it's used, I'd like to avoid using up a whole slot. > > > > Also, how are we going to extend this device? > > looks like we've burned it all just for vmid? > I don't like the way MS uses yet another side-channel > to communicate something (UUID) instead of using ACPI > method for getting it. > I'd rather avoid extending it beyond of what it's now > and use channels that we already have. Famous last words :) > > How about we have a slightly more generic container > > where we'll be able to stick all kind of stuff > > in the future, and make vmgenid a child of > > this device? > What other possible uses do you have in mind? I don't know for sure - some other value that applications want to map. > > > To change uuid in runtime use: > > > qom-set "/machine/peripheral/FOO.uuid" > > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87" > > > > Looking just at this, how does user discover this functionality? > what do you mean? > > [...] Just this. You are a user. You want to change the vm gen id. Adding devices is partially documented in -help and -device help. commands are documented in hmp help. But how do you find out that qom-set should be used to update vm gen id, and how do you find out how to do this? > > > > > > 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); > > > > confused. So what happens if BAR is not mapped by guest? > it will get 0 address on acpi_setup() stage but later > when ACPI tables are read by BIOS (which happens after PCI is > initialized) it will be updated and get mapped address. Yes but it's up to guest. What if guest does not map BAR later, either? BTW, why do we need to stick vmgen_buf_paddr in the info? > > > > > + } > > > info->has_hpet = hpet_find(); > > > info->has_tpm = tpm_find(); > > > info->pvpanic_port = pvpanic_port(); > > > @@ -521,8 +531,27 @@ static void > > > build_append_pcihp_notify_entry(Aml *method, int slot) > > > aml_append(method, if_ctx); } > > > > > > +static char *acpi_get_pci_dev_scope_name(PCIDevice *pdev) > > > +{ > > > + char *name = NULL; > > > + char *last = name; > > > + PCIBus *bus = pdev->bus; > > > + > > > + while (bus->parent_dev) { > > > + last = name; > > > + name = g_strdup_printf("%s.S%.02X_", name ? name : "", > > > + bus->parent_dev->devfn); > > > + g_free(last); > > > + bus = bus->parent_dev->bus; > > > + } > > > + last = name; > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "", > > > pdev->devfn); > > > + g_free(last); > > > + return name; > > > +} > > > > Looks tricky, and duplicates logic for device naming. > > All this won't be necessary if you just add this as child > > of the correct device, without playing with scope. > > Why not do it? > since vmgenid PCI device is located somewhere on PCI bus we don't have > fixed PATH to it and we need full path to it to send Notivy from > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below. I see. Still - can't this function return the full aml_name? > > > > > > > static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus > > > *bus, > > > - bool pcihp_bridge_en) > > > + bool pcihp_bridge_en, > > > + AcpiMiscInfo *misc) > > > { > > > Aml *dev, *notify_method, *method; > > > QObject *bsel; > > > @@ -583,7 +612,21 @@ static void build_append_pci_bus_devices(Aml > > > *parent_scope, PCIBus *bus, dev = aml_device("S%.02X", > > > PCI_DEVFN(slot, 0)); aml_append(dev, aml_name_decl("_ADR", > > > aml_int(slot << 16))); > > > - if (pc->class_id == PCI_CLASS_DISPLAY_VGA) { > > > + if (pc->device_id == PCI_DEVICE_ID_REDHAT_VMGENID) { > > > > clearly not enough, one must also check the vendor. > Yep, I've noticed that after sending, will fix. > > [...] > > > diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c > > > new file mode 100644 > > > index 0000000..c47fb06 > > > --- /dev/null > > > +++ b/hw/misc/vmgenid.c > > > @@ -0,0 +1,134 @@ > > > +/* > > > + * Virtual Machine Generation ID Device > > > + * > > > + * Copyright (C) 2014 Red Hat Inc. > > > > It's 2015 isn't it? > sure, I'll fix it. > > [...] > > > +typedef struct VmGenIdState { > > > + PCIDevice parent_obj; > > > + MemoryRegion iomem; > > > + union { > > > + uint8_t guid[16]; > > > + uint8_t guid_page[TARGET_PAGE_SIZE]; > > > > Please just make it 4K. > > We don't want more target-specific code if we can help it. > ok > > [...] > > > +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.", > > > > s/Fail/Failed/ > sure > > > Also - print the string itself? > What do you mean? the uuid string which we failed to parse? > [...] > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void > > > *opaque, > > > + const char *name, Error **errp) > > > +{ > > > + VmGenIdState *s = VMGENID(obj); > > > > Why cast to VMGENID here? > Yep, there is no need to do it, I'll clean it up. > > > > > > + 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))); > > > > This is guest error. Pls don't print these to monitor by default. > Then how test case querying this property via QOM could get to know > that property is in wrong state yet? Maybe leave this around for tests (with a comment) but use plain pci_get_bar_addr internally? > > > > > + return; > > > + } > > > + visit_type_int(v, &value, name, errp); > > > +} > > > + > > > > Useful for testing, but looks like of generic. > > Add methods to access BAR from QOM for all pci > > devices? > That's what it's used for in following test case. > > But I'm not sure how to make it generic provided it > would be sort of array property (which Peter introduced) > but later they got some critique, I haven't kept on that issue though. > I'd leave this as it is for now. > > > > +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; > > > > Still looks scary. > Can do nothing about it, > it's closest class id to what this device is > (i.e. it exposes page of RAM) that works with Windows > without asking for drivers. > If that class id is not acceptable then let's drop PCI > approach altogether. > > More over it's limited to target-i386 only and possibly > could apply to ARM in the future when Windows comes there, > so in this case I'm not very concerned about pseries guests I don't think we should treat this as a windows only device, the function seems generally useful. > especially with buggy kernel as it was reported in > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html I think it's firmware that's confused, not the guest kernel. > > [...] Some options to think about/try 1. PCI_CLASS_MEMORY_OTHER (or some other class?) 2. Name(_HID, "PNP0A06") (or some other id) > > > 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 > > > > I asked about this already I think - why is it here? > do you mean comment "current device naming scheme ..." > > [...] no - the extra include. > > > + > > > +#ifndef HW_MISC_VMGENID_H > > > +#define HW_MISC_VMGENID_H > > > + > > > +#define VMGENID_DEVICE "vmgenid" > > > +#define VMGENID_UUID "uuid" > > > > uuid looks kind of too generic. vmgenid-uuid? > sure