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

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> 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.

I think you can just set addr and multifunction properties.



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

No problem. It's unfortunate we missed the developer conf call this
tuesday.  If you like, I'll try to setup something for Monday, I'd like
to attend too. Anyone else interested?

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

Eric, Andreas, any comments on this part?

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

An aborted guest is very secure but this is going way overboard I think.
It's easy for guest to detect that the ACPI device is not there,
whether crashing is the best solution in this case
is a policy question. In particular something like virtio
rng looks like an adequate replacement.


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

Yes but why not read the property when and where we
need it?

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

OK but I think it should be under PCI0 at least,
since that one claims the relevant resource in its CRS.

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

OK, though we might need to add the description for the pci device anyway
e.g. in order to mark it hidden.




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

I don't think it's a good idea. It's just a device,
it's not the most important thing for guests,
it's a policy question whether to initialize it.


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

It's better if we can avoid the issue altogether.
Assuming that we can't,
I'd like some confirmation from David Gibson on this.


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

I see. Interesting.
And VM Gen ID isn't using the resources of the pci
device?
Any other ideas? Mark it hidden?

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