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

[Igor Mammedov]

On Wed, 4 Mar 2015 13:11:48 +0100
"Michael S. Tsirkin" <address@hidden> wrote:

> 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.
As I understand mutifunction devices are usually composit
ones and created along with parent device.
I'm not sure what to do here, 
an example or some hints how to implement it are welcome.

> 
> 
> > > 
> > > 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.
Well then suggest something more concrete here I don't
quietly have an idea what 
  > > > 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?
means, maybe we need a canfcall with Gal to discuss idea?

> 
> 
> > > > 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?
I don't really know, it's approach used in original patches.
Any suggestions?
Do QOM properties have some 'help' connected to them? If yes we
could stick explanation there so that -device vmgenid,help
would show it at least.

> 
> 
> > > >  
> > > >  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?
I'd prefer to abort machine since otherwise guest OS wouldn't
get what its users expected to and silently would continue running.
Considering that MS intends to use this value for cryptography
purposes () it would be security risk.

> 
> 
> BTW, why do we need to stick vmgen_buf_paddr in the info?
Because according to MS spec device should have ADDR object
with physical buffer address packed in Package(2). So that
Windows could read value from there.

[...]
> > > > +    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?
it's possible but I'd prefer to return back to 2 ACPI devices as it was
in v13 since Windows sees 2 devices anyway, even if they merged into one
PCI device description (which probably wrong but windows handles it because
PCI Standard RAM controller is driver less) and get rid of
acpi_get_pci_dev_scope_name() thing.

It will also help if vmgenid will be a part of multifunction device,
which current build_append_pci_bus_devices() ignores for now (i.e. it
describes only function 0 devices on slot).

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

> 
> > [...]
> > > > +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?
Accessing it internally as property will also allow to
prevent guest starting if BIOS failed to initialize BAR
(not implemented but shouldn't be hard to do)

[...]
> > > > +    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.
maybe both, should we care about faulty guest pieces when they
don't use this device. If the pseries would need to use it then
they should fix guest size instead of poking soldering iron
in HW.

> 
> > 
> > [...]
> 
> 
> Some options to think about/try
> 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
I've already tried this and a number of others,
Windows asks for driver.

> 2. Name(_HID, "PNP0A06") (or some other id)
experiment on Windows shows that _HID doesn't influence PCI devices described
in ACPI in any way. In this version _HID = QEMU0003 and its required
by VMGID spec to have unique vendor specific HID for VMGID device.
It looks like PCI driver mostly ignores PCI slots described in ACPI
and as result there are devices in device manager "PCI standard RAM Ctrl"
and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
tables.

> 
> 
> 
> > > > 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.
ok, will fix.