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

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> 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() ?

The issue is you have two items to track here:
- addr - you stick that in the info struct
- full object address - you don't
an inconsistency that I dislike.


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

So it boils down to the fact that windows thinks it's RAM,
so it binds a generic driver to it, but then we get
lucky and it does not try to use it as RAM.
Is that a fair summary? If yes, to me, it looks exactly like the reverse
side of the pseries issue.


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

Right, less work for us :)
But I'm guessing *users* would rather have a debuggable guest.

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

That's still up to guest.  BIOS can abort boot if it wants to.


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

windows will detect that it does not have VGID.
whether it's worth crashing in that case is up to windows, not us.

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

Been there, done that.
Each time we try to steal memory, we get pain.
Guests should allocate memory.
Either via PCI, or linker like Gal's patches 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.
> > 
> > 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)
>  
> [...]

I agree, what worries me is the driver prompt if we set class to
something generic, and guest confusion if we set it to RAM.

-- 
MST