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

[Igor Mammedov]

On Wed, 4 Mar 2015 14:49:00 +0100
"Michael S. Tsirkin" <address@hidden> wrote:

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

[...]

> > > > > 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?
Monday is fine for me.

> 
> > > 
> > > 
> > > > > > 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.
related comment is below.

> 
> 
> > > 
> > > 
> > > 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?
It's basically to fit the style used in acpi-build.c
where we collect info by reading properties in
 acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
and then just use pm, misc, pci in build_ssdt()
should we drop all above and just inline it in build_ssdt() ?

> 
> > > > > > +    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.
vmgenid device doesn't claim any resource if we use PCI for its
implementation since corresponding PCI device claims its BAR.
But I don't see any problem in putting VGID device into PCI0 scope.

> 
> > 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.
Experiments show that Windows ignores _STA for PCI devices,
it looks like it completely ignores SXX devices in ACPI for present at boot
devices except of _EJ().
BTW: I've already tried it, it doesn't hide anything.
 
[...]
> > > > > > +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.
I don't think it's just policy, BIOS and ACPI in QEMU are tightly coupled
and if BIOS is unable initialize devices as it's supposed to
then I'd rather abort machine with error message pointing to source
of problem then silently continue boot and allow guest OS or even
some guest application to guess what went wrong if they will be able
to do so.
In this case Windows will continue to work just without using VGID
possibly leading to duplicate keys on to 2 different VMs
or something else (it's used not only for crypto).

Alternatively lets map page directly in QEMU before PCI hole
without any PCI BARs and pass reservation via E820,
 - it would solve issue with selecting PCI CLASS ID, it would be just plain 
QEMU device
 - no consuming of slot and/or addrX.functionY
 - we would know immediately if device is correctly initialized
   even before BIOS runs. i.e. no guest involved and with
   clear end result.

> > > > > > +    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?
Nope, resources are claimed by respective PCI device regardless of its
presence in ACPI tables. VGID device just exposes ADDR so that Windows could
poke into that buffer

> Any other ideas? Mark it hidden?
Gal's already checked, Windows doesn't hide VGID device.
I think we should just leave 2 devices as is (i.e. shown), no harm in it.
(I'd hide PCI device but it seems to be impossible, and it's anyway just 
cosmetic)
 
[...]