Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device

[Igor Mammedov]

On Tue, 3 Mar 2015 18:35:39 +0100
"Michael S. Tsirkin" <address@hidden> 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.

> 
> 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.

> 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?

> > 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?

[...]
> >  
> >  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.

> 
> > +    }
> >      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.

> 
> 
> >  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?

[...]
> > +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?

> 
> > +        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
especially with buggy kernel as it was reported in
http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html


[...]
> > 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 ..."

[...]
> > +
> > +#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