Re: [Qemu-devel] [PATCH v8 4/8] ACPI: Add Virtual Machine Generation ID support

[Michael S. Tsirkin]

On Fri, Feb 17, 2017 at 10:34:29AM -0800, Ben Warren wrote:
> 
>     On Feb 17, 2017, at 8:03 AM, Laszlo Ersek <address@hidden> wrote:
> 
>     On 02/17/17 16:33, Ben Warren wrote:
> 
> 
> 
>             On Feb 17, 2017, at 2:43 AM, Igor Mammedov <address@hidden
>             <mailto:address@hidden>> wrote:
> 
>             On Thu, 16 Feb 2017 15:15:36 -0800
>             address@hidden <mailto:address@hidden> wrote:
> 
> 
>                 From: Ben Warren <address@hidden <
>                 mailto:address@hidden>>
> 
>                 This implements the VM Generation ID feature by passing a
>                 128-bit
>                 GUID to the guest via a fw_cfg blob.
>                 Any time the GUID changes, an ACPI notify event is sent to the
>                 guest
> 
>                 The user interface is a simple device with one parameter:
>                 - guid (string, must be "auto" or in UUID format
>                  xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)
> 
>             I've given it some testing with WS2012R2 and v4 patches for
>             Seabios,
> 
>             Windows is able to read initial GUID allocation and writeback
>             seems to work somehow:
> 
>             (qemu) info vm-generation-id
>             c109c09b-0e8b-42d5-9b33-8409c9dcd16c
> 
>             vmgenid client in Windows reads it as 2 following 64bit integers:
>             42d50e8bc109c09b:6cd1dcc90984339b
> 
>             However update path/restore from snapshot doesn't
>             here is as I've tested it:
> 
>             qemu-system-x86_64 -device vmgenid,id=testvgid,guid=auto -monitor
>             stdio
>             (qemu) info vm-generation-id
>             c109c09b-0e8b-42d5-9b33-8409c9dcd16c
>             (qemu) stop
>             (qemu) migrate "exec:gzip -c > STATEFILE.gz"
>             (qemu) quit
> 
>             qemu-system-x86_64 -device vmgenid,id=testvgid,guid=auto -monitor
>             stdio
>             -incoming "exec: gzip -c -d STATEFILE.gz"
>             (qemu) info vm-generation-id
>             28b587fa-991b-4267-80d7-9cf28b746fe9
> 
>             guest
>             1. doesn't get GPE notification that it must receive
>             2. vmgenid client in Windows reads the same value
>                 42d50e8bc109c09b:6cd1dcc90984339b
> 
> 
>         Strange, this was working for me, but with a slightly different test
>         method:
> 
>          * I use virsh save/restore
> 
> 
>     Awesome, this actually what I should try. All my guests are managed by
>     libvirt (with the occasional <qemu:arg>, for development), and direct
>     QEMU monitor commands such as
> 
>      virsh qemu-monitor-command ovmf.rhel7 --hmp 'info vm-generation-id'
> 
>     only work for me if they are reasonably non-intrusive.
> 
> 
>          * While I do later testing with Windows, during development I use a
>            Linux kernel module I wrote that keeps track of GUID and
>            notifications.  I’m happy to share this with you if interested.
> 
> 
>     Please do. If you have a public git repo somewhere, that would be
>     awesome. (Bonus points if the module builds out-of-tree, if the
>     kernel-devel package is installed.)
> 
> 
> Here you go:
> https://github.com/ben-skyportsystems/vmgenid-test
> 
> I don’t know if something like this would ever be accepted into the Linux
> kernel, but it has been invaluable to me, and I’d like to see it somewhere
> better.

I think the main issue here is that there's no blocking
interface to wait for change events.



Also ioremap_nocache is definitely the wrong thing to do
since the spec says
        It must not be in the same 4-kilobyte page as any memory that is
        expected to be mapped by a page table entry with caching disabled.

Finally, I think it makes sense to add an mmap call to this driver.
Basically add some kind of interface telling guest that gen id does not
share a 4K page with any other structure. Maybe just a special HID
value?  Or a special method we can test for.  Then it's safe for guest
to map this page read-only into userspace memory. It should have an
interface to report the offset to userspace. Userspace can then get the
ID without a system call by doing

ptr = mmap(...)
offset = ioctl(... GET_OFFSET ...)
guid = *(ptr + offset)

Windows does not seem to have this ability but it might be
a significant performance enhancement IMHO.

> 
>     NB: while the set-id monitor command was part of the series, I did test
>     it to the extent that I checked the SCI ("ACPI interrupt") count in the
>     guest, in /proc/interrupts. I did see it increase, so minimally the SCI
>     injection was fine.
> 
>     Thanks!
>     Laszlo
> 
> 
>         I’ll dig into this morning.
> 
>         —Ben
>         <snip>
> 
>