[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device |
Date: |
Wed, 4 Mar 2015 13:11:48 +0100 |
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" <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.
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
- [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID, Igor Mammedov, 2015/03/03
- [Qemu-devel] [PATCH V14 3/3] tests: add a unit test for the vmgenid device., Igor Mammedov, 2015/03/03
- [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/03
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Michael S. Tsirkin, 2015/03/03
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/03
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Michael S. Tsirkin, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Andreas Färber, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Michael S. Tsirkin, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Michael S. Tsirkin, 2015/03/04
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Igor Mammedov, 2015/03/05
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, David Gibson, 2015/03/11
- Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device, Michael S. Tsirkin, 2015/03/19