Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Igor Mammedov]

On Mon, 13 Feb 2017 15:00:22 +0200
"Michael S. Tsirkin"  wrote:

> On Mon, Feb 13, 2017 at 12:00:36PM +0100, Igor Mammedov wrote:
> > On Fri, 10 Feb 2017 19:27:21 +0100
> > Andreas Färber  wrote:
> >   
> > > Am 10.02.2017 um 19:18 schrieb Andrew Jones:  
> > > > On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:
> > > >> On Fri, 10 Feb 2017 17:31:33 +0200
> > > >> "Michael S. Tsirkin"  wrote:
> > > >>
> > > >>> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
> > >  On 02/05/17 10:11, b...@skyportsystems.com wrote:  
> > > > From: Ben Warren 
> > > >> [...]
> > > >>
> > > 
> > >  - or else, add another boolean property to vmgenid, one that 
> > >  parallels
> > >  "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> > >  realize() when this property is false.  
> > > >>>
> > > >>> That's probably the easiest way. x-fw-cfg-dma-enabled.
> > > >>> Won't delay merging because of this, can be done with
> > > >>> patch on top.
> > > >> (not related to this series)
> > > >>
> > > >> I've thought that there still were no consensus on x-foo prefix,
> > > >> not to mention that x- might be legitimate prefix for some properties.
*1

> > > >>
> > > >> Maybe we should add a flag to property like INTERNAL_PROPERTY
> > > >> and then set it explicitly on for internal stuff.
> > > >>
> > > >> That way we could cleanly exclude internal properties from
> > > >>  -device foo,help and make sure that user won't set them from CLI.
> > > >> I'd even volunteer to add this API to Object
> > > > 
> > > > Yes, please. I know of a property or two where it would be nice to
> > > > have that flag.
> > > 
> > > Apart from documentation, what effect would such a flag have?
> > > 
> > > With QOM I don't really see "internal" as being a thing: Besides -device
> > > and the likes, we expose qom-set operation and -global option, and I
> > > don't think it makes sense to restrict the latter two. For -device,
> > > "realized" is a property I would classify as non-user maybe.  
> > I'd propose to start with this flag hiding/forbiding usage of
> > property in following interfaces:
> > 
> >   * -device
> >   * -device foo,help
> >   * device_add
> > 
> > With docs mentioning that usage of hidden properties with
> >  -globals/qom-set isn't recommended.  
> 
> I'm not sure I agree. These properties can be useful e.g. for debugging,
> the only issue is we don't guarantee to support them across QEMU
> releases. If you won't want something exposed to users, don't make it a
> property at all.
Property approach is cleaner especially in cases where
it's used to tie together generic with target-specific code
+ automatically makes feature introspect-able via qom-get
(I've used it for debugging to poke into object properties
without need for gdb).
Replacing property with direct function call makes one
to create stubs (I've seen Paolo working on removing stubs)
for targets that do not implement it or uglifying headers with
target specific ifdefs.

> Producing a warning when a property starting with x- is set
> might not be a bad idea.
I don't like x- prefix for above [1] and
we already have a bunch of properties that shouldn't be
user settable that makes x- convention inconsistent.
so how about following:
   * -device foo,help:
   * hide non user flagged properties or
   *less preferable mark them as non stable/supportable
   * issue warning in case such property is used
 with -device/device_add
   * since it's warning we could probably put it in -globals as well
 so user would notice probable misuse
  

> > > 
> > > Regards,
> > > Andreas
> > >   
> 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Michael S. Tsirkin]

On Mon, Feb 13, 2017 at 12:00:36PM +0100, Igor Mammedov wrote:
> On Fri, 10 Feb 2017 19:27:21 +0100
> Andreas Färber  wrote:
> 
> > Am 10.02.2017 um 19:18 schrieb Andrew Jones:
> > > On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:  
> > >> On Fri, 10 Feb 2017 17:31:33 +0200
> > >> "Michael S. Tsirkin"  wrote:
> > >>  
> > >>> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:  
> >  On 02/05/17 10:11, b...@skyportsystems.com wrote:
> > > From: Ben Warren   
> > >> [...]
> > >>  
> > 
> >  - or else, add another boolean property to vmgenid, one that parallels
> >  "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> >  realize() when this property is false.
> > >>>
> > >>> That's probably the easiest way. x-fw-cfg-dma-enabled.
> > >>> Won't delay merging because of this, can be done with
> > >>> patch on top.  
> > >> (not related to this series)
> > >>
> > >> I've thought that there still were no consensus on x-foo prefix,
> > >> not to mention that x- might be legitimate prefix for some properties.
> > >>
> > >> Maybe we should add a flag to property like INTERNAL_PROPERTY
> > >> and then set it explicitly on for internal stuff.
> > >>
> > >> That way we could cleanly exclude internal properties from
> > >>  -device foo,help and make sure that user won't set them from CLI.
> > >> I'd even volunteer to add this API to Object  
> > > 
> > > Yes, please. I know of a property or two where it would be nice to
> > > have that flag.  
> > 
> > Apart from documentation, what effect would such a flag have?
> > 
> > With QOM I don't really see "internal" as being a thing: Besides -device
> > and the likes, we expose qom-set operation and -global option, and I
> > don't think it makes sense to restrict the latter two. For -device,
> > "realized" is a property I would classify as non-user maybe.
> I'd propose to start with this flag hiding/forbiding usage of
> property in following interfaces:
> 
>   * -device
>   * -device foo,help
>   * device_add
> 
> With docs mentioning that usage of hidden properties with
>  -globals/qom-set isn't recommended.

I'm not sure I agree. These properties can be useful e.g. for debugging,
the only issue is we don't guarantee to support them across QEMU
releases. If you won't want something exposed to users, don't make it a
property at all.

Producing a warning when a property starting with x- is set
might not be a bad idea.

> > 
> > Regards,
> > Andreas
> > 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Igor Mammedov]

On Fri, 10 Feb 2017 19:27:21 +0100
Andreas Färber  wrote:

> Am 10.02.2017 um 19:18 schrieb Andrew Jones:
> > On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:  
> >> On Fri, 10 Feb 2017 17:31:33 +0200
> >> "Michael S. Tsirkin"  wrote:
> >>  
> >>> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:  
>  On 02/05/17 10:11, b...@skyportsystems.com wrote:
> > From: Ben Warren   
> >> [...]
> >>  
> 
>  - or else, add another boolean property to vmgenid, one that parallels
>  "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
>  realize() when this property is false.
> >>>
> >>> That's probably the easiest way. x-fw-cfg-dma-enabled.
> >>> Won't delay merging because of this, can be done with
> >>> patch on top.  
> >> (not related to this series)
> >>
> >> I've thought that there still were no consensus on x-foo prefix,
> >> not to mention that x- might be legitimate prefix for some properties.
> >>
> >> Maybe we should add a flag to property like INTERNAL_PROPERTY
> >> and then set it explicitly on for internal stuff.
> >>
> >> That way we could cleanly exclude internal properties from
> >>  -device foo,help and make sure that user won't set them from CLI.
> >> I'd even volunteer to add this API to Object  
> > 
> > Yes, please. I know of a property or two where it would be nice to
> > have that flag.  
> 
> Apart from documentation, what effect would such a flag have?
> 
> With QOM I don't really see "internal" as being a thing: Besides -device
> and the likes, we expose qom-set operation and -global option, and I
> don't think it makes sense to restrict the latter two. For -device,
> "realized" is a property I would classify as non-user maybe.
I'd propose to start with this flag hiding/forbiding usage of
property in following interfaces:

  * -device
  * -device foo,help
  * device_add

With docs mentioning that usage of hidden properties with
 -globals/qom-set isn't recommended.
  
> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Andreas Färber]

Am 10.02.2017 um 19:18 schrieb Andrew Jones:
> On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:
>> On Fri, 10 Feb 2017 17:31:33 +0200
>> "Michael S. Tsirkin"  wrote:
>>
>>> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
 On 02/05/17 10:11, b...@skyportsystems.com wrote:  
> From: Ben Warren 
>> [...]
>>

 - or else, add another boolean property to vmgenid, one that parallels
 "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
 realize() when this property is false.  
>>>
>>> That's probably the easiest way. x-fw-cfg-dma-enabled.
>>> Won't delay merging because of this, can be done with
>>> patch on top.
>> (not related to this series)
>>
>> I've thought that there still were no consensus on x-foo prefix,
>> not to mention that x- might be legitimate prefix for some properties.
>>
>> Maybe we should add a flag to property like INTERNAL_PROPERTY
>> and then set it explicitly on for internal stuff.
>>
>> That way we could cleanly exclude internal properties from
>>  -device foo,help and make sure that user won't set them from CLI.
>> I'd even volunteer to add this API to Object
> 
> Yes, please. I know of a property or two where it would be nice to
> have that flag.

Apart from documentation, what effect would such a flag have?

With QOM I don't really see "internal" as being a thing: Besides -device
and the likes, we expose qom-set operation and -global option, and I
don't think it makes sense to restrict the latter two. For -device,
"realized" is a property I would classify as non-user maybe.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Andrew Jones]

On Fri, Feb 10, 2017 at 05:16:59PM +0100, Igor Mammedov wrote:
> On Fri, 10 Feb 2017 17:31:33 +0200
> "Michael S. Tsirkin"  wrote:
> 
> > On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
> > > On 02/05/17 10:11, b...@skyportsystems.com wrote:  
> > > > From: Ben Warren 
> [...]
> 
> > > 
> > > - or else, add another boolean property to vmgenid, one that parallels
> > > "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> > > realize() when this property is false.  
> > 
> > That's probably the easiest way. x-fw-cfg-dma-enabled.
> > Won't delay merging because of this, can be done with
> > patch on top.
> (not related to this series)
> 
> I've thought that there still were no consensus on x-foo prefix,
> not to mention that x- might be legitimate prefix for some properties.
> 
> Maybe we should add a flag to property like INTERNAL_PROPERTY
> and then set it explicitly on for internal stuff.
> 
> That way we could cleanly exclude internal properties from
>  -device foo,help and make sure that user won't set them from CLI.
> I'd even volunteer to add this API to Object

Yes, please. I know of a property or two where it would be nice to
have that flag.

Thanks,
drew

> 
> 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Igor Mammedov]

On Fri, 10 Feb 2017 17:31:33 +0200
"Michael S. Tsirkin"  wrote:

> On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
> > On 02/05/17 10:11, b...@skyportsystems.com wrote:  
> > > From: Ben Warren 
[...]

> > 
> > - or else, add another boolean property to vmgenid, one that parallels
> > "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> > realize() when this property is false.  
> 
> That's probably the easiest way. x-fw-cfg-dma-enabled.
> Won't delay merging because of this, can be done with
> patch on top.
(not related to this series)

I've thought that there still were no consensus on x-foo prefix,
not to mention that x- might be legitimate prefix for some properties.

Maybe we should add a flag to property like INTERNAL_PROPERTY
and then set it explicitly on for internal stuff.

That way we could cleanly exclude internal properties from
 -device foo,help and make sure that user won't set them from CLI.
I'd even volunteer to add this API to Object

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Michael S. Tsirkin]

On Fri, Feb 10, 2017 at 11:12:13AM +0100, Laszlo Ersek wrote:
> On 02/05/17 10:11, b...@skyportsystems.com wrote:
> > From: Ben Warren 
> > 
> > This patch set adds support for passing a GUID to Windows guests.  It is a
> > re-implementation of previous patch sets written by Igor Mammedov et al, but
> > this time passing the GUID data as a fw_cfg blob.
> > 
> > This patch set has dependencies on new guest functionality, in particular 
> > the
> > support for a new linker-loader command and the ability to write back data
> > to QEMU over a DMA link.  Work is in flight in both SeaBIOS and OVMF to 
> > support this.
> > 
> > v4->v5:
> > - Added significantly more detail to the documentation.
> > - Replaced the previously-implemented linker-loader command with a new 
> > one:
> >   "write pointer".  This allows writing the guest address of a fw_cfg 
> > blob back
> >   to an arbitrary offset in a writeable fw_cfg file visible to QEMU.  
> > This will
> >   require support in SeaBIOS and OVMF (ongoing).
> > - Fixed endianness issues throughout.
> > - Several styling cleanups.
> 
> Low importance question, but I'll ask it anyway:
> 
> Can we somehow make the realization of "-device vmgenid" fail, if the PC
> machine type in use is 2.6 or earlier? Because, those machine types do
> not have DMA enabled for fw_cfg. Hence the WRITE_POINTER commands cannot
> be effectively executed by firmware (the underlying virtual hardware is
> not enabled).
> 
> For example, if a user adds VMGENID to a 2.5 machine type, and migrates
> the VM, then on the target host,
> 
>   vmgenid_post_load()
> vmgenid_update_guest()
> 
> will not be effective, because (back on the source host) the guest had
> no way to write the address of the downloaded "etc/vmgenid" blob to
> "etc/vmgenid_addr".
> 
> I don't think it's a big deal, but perhaps we can be user-friendly a
> bit.

The most user-friendly thing would be to DWIM and enable DMA.
But that would be hard to implement so probably not
worth the trouble.

> Things that crossed my mind (different levels of ugliness):
> 
> - in the realize function for vmgenid (which I've proposed anyway, for
> registering the reset handler), we could look up the fw_cfg object, and
> see if its "dma-enabled" property is on or off. Possible problem: not
> sure if such cross-device checks are allowed in realize methods, and if
> there are any ordering issues for this. (I.e., when vmgenid's realize
> checks, has the property been set for fw_cfg?)
> 
> - or else, add another boolean property to vmgenid, one that parallels
> "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> realize() when this property is false.

That's probably the easiest way. x-fw-cfg-dma-enabled.
Won't delay merging because of this, can be done with
patch on top.

> There are probably better ways...
> 
> Anyway, it's not a big deal, I think this can be addressed incrementally
> (or not at all, perhaps). I don't want to delay the series with it, just
> throwing it out there.

Yes, pls do not delay because of this.

> BTW, I tested the OVMF patches against v5, and it looks good. I
> cross-referenced the firmware log with acpidump in the guest, plus I
> used dump-guest-memory and the "crash" utility to check the memory
> contents for the guest, before/after setting the GUID with the monitor
> command.
> 
> I'd like to verify S3 too, but for that I'd like to use v6 (which I hope
> clears "vgia_le" on reset).
> 
> Thanks,
> Laszlo
> 
> > 
> > v3->v4:
> > - Rebased to top of tree.
> > - Re-added document patch that was accidentally dropped from the last 
> > revision.
> > - Added VMState functionality so that VGIA is restored properly.
> > - Added Unit tests
> > v2->v3:
> > - Added second writeable fw_cfg for storing the VM Generaiton ID
> >   address.  This uses a new linker-loader command for instructing the
> >   guest to write back the allocated address.  A patch for SeaBIOS has 
> > been
> >   submitted 
> > (https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
> >   and the resulting binary will need to be pulled into QEMU once 
> > accepted.
> > - Setting VM Generation ID by command line or qmp/hmp now accepts an 
> > "auto"
> >   value, whereby QEMU generates a random GUID.
> > - Incorporated review comments from v2 mainly around code styling and 
> > AML syntax
> > - Changed to use the E05 ACPI event instead of E00
> > v1->v2:
> > - Removed "changed" boolean parameter as it is unneeded
> > - Added ACPI Notify logic
> > - Style changes to pass checkpatch.pl
> > - Added support for dynamic sysbus to pc_piix boards
> > 
> > 
> > Ben Warren (8):
> >   ACPI: Add a function for building named qword entries
> >   linker-loader: Add new 'write pointer' command
> >   docs: VM Generation ID device description
> >   ACPI: Add vmgenid storage entries to the build tables
> >   ACPI: Add Virtual 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Igor Mammedov]

On Fri, 10 Feb 2017 11:12:13 +0100
Laszlo Ersek  wrote:

> On 02/05/17 10:11, b...@skyportsystems.com wrote:
> > From: Ben Warren 
> > 
> > This patch set adds support for passing a GUID to Windows guests.  It is a
> > re-implementation of previous patch sets written by Igor Mammedov et al, but
> > this time passing the GUID data as a fw_cfg blob.
> > 
> > This patch set has dependencies on new guest functionality, in particular 
> > the
> > support for a new linker-loader command and the ability to write back data
> > to QEMU over a DMA link.  Work is in flight in both SeaBIOS and OVMF to 
> > support this.
> > 
> > v4->v5:
> > - Added significantly more detail to the documentation.
> > - Replaced the previously-implemented linker-loader command with a new 
> > one:
> >   "write pointer".  This allows writing the guest address of a fw_cfg 
> > blob back
> >   to an arbitrary offset in a writeable fw_cfg file visible to QEMU.  
> > This will
> >   require support in SeaBIOS and OVMF (ongoing).
> > - Fixed endianness issues throughout.
> > - Several styling cleanups.  
> 
> Low importance question, but I'll ask it anyway:
> 
> Can we somehow make the realization of "-device vmgenid" fail, if the PC
> machine type in use is 2.6 or earlier? Because, those machine types do
> not have DMA enabled for fw_cfg. Hence the WRITE_POINTER commands cannot
> be effectively executed by firmware (the underlying virtual hardware is
> not enabled).
> 
> For example, if a user adds VMGENID to a 2.5 machine type, and migrates
> the VM, then on the target host,
> 
>   vmgenid_post_load()
> vmgenid_update_guest()
> 
> will not be effective, because (back on the source host) the guest had
> no way to write the address of the downloaded "etc/vmgenid" blob to
> "etc/vmgenid_addr".
> 
> I don't think it's a big deal, but perhaps we can be user-friendly a
> bit. Things that crossed my mind (different levels of ugliness):
> 
> - in the realize function for vmgenid (which I've proposed anyway, for
> registering the reset handler), we could look up the fw_cfg object, and
> see if its "dma-enabled" property is on or off. Possible problem: not
> sure if such cross-device checks are allowed in realize methods, and if
> there are any ordering issues for this. (I.e., when vmgenid's realize
> checks, has the property been set for fw_cfg?)
> 
> - or else, add another boolean property to vmgenid, one that parallels
> "dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
> realize() when this property is false.
we usually shouldn't poke from realize to outside world so
I'd go compat property way.


> There are probably better ways...
> 
> Anyway, it's not a big deal, I think this can be addressed incrementally
> (or not at all, perhaps). I don't want to delay the series with it, just
> throwing it out there.
> 
> BTW, I tested the OVMF patches against v5, and it looks good. I
> cross-referenced the firmware log with acpidump in the guest, plus I
> used dump-guest-memory and the "crash" utility to check the memory
> contents for the guest, before/after setting the GUID with the monitor
> command.
> 
> I'd like to verify S3 too, but for that I'd like to use v6 (which I hope
> clears "vgia_le" on reset).
> 
> Thanks,
> Laszlo
> 
> > 
> > v3->v4:
> > - Rebased to top of tree.
> > - Re-added document patch that was accidentally dropped from the last 
> > revision.
> > - Added VMState functionality so that VGIA is restored properly.
> > - Added Unit tests
> > v2->v3:
> > - Added second writeable fw_cfg for storing the VM Generaiton ID
> >   address.  This uses a new linker-loader command for instructing the
> >   guest to write back the allocated address.  A patch for SeaBIOS has 
> > been
> >   submitted 
> > (https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
> >   and the resulting binary will need to be pulled into QEMU once 
> > accepted.
> > - Setting VM Generation ID by command line or qmp/hmp now accepts an 
> > "auto"
> >   value, whereby QEMU generates a random GUID.
> > - Incorporated review comments from v2 mainly around code styling and 
> > AML syntax
> > - Changed to use the E05 ACPI event instead of E00
> > v1->v2:
> > - Removed "changed" boolean parameter as it is unneeded
> > - Added ACPI Notify logic
> > - Style changes to pass checkpatch.pl
> > - Added support for dynamic sysbus to pc_piix boards
> > 
> > 
> > Ben Warren (8):
> >   ACPI: Add a function for building named qword entries
> >   linker-loader: Add new 'write pointer' command
> >   docs: VM Generation ID device description
> >   ACPI: Add vmgenid storage entries to the build tables
> >   ACPI: Add Virtual Machine Generation ID support
> >   PC: Support dynamic sysbus on pc_i440fx
> >   tests: Move reusable ACPI macros into a new header file
> >   tests: Add unit tests for the VM Generation ID 

Re: [Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[Laszlo Ersek]

On 02/05/17 10:11, b...@skyportsystems.com wrote:
> From: Ben Warren 
> 
> This patch set adds support for passing a GUID to Windows guests.  It is a
> re-implementation of previous patch sets written by Igor Mammedov et al, but
> this time passing the GUID data as a fw_cfg blob.
> 
> This patch set has dependencies on new guest functionality, in particular the
> support for a new linker-loader command and the ability to write back data
> to QEMU over a DMA link.  Work is in flight in both SeaBIOS and OVMF to 
> support this.
> 
> v4->v5:
> - Added significantly more detail to the documentation.
> - Replaced the previously-implemented linker-loader command with a new 
> one:
>   "write pointer".  This allows writing the guest address of a fw_cfg 
> blob back
>   to an arbitrary offset in a writeable fw_cfg file visible to QEMU.  
> This will
>   require support in SeaBIOS and OVMF (ongoing).
> - Fixed endianness issues throughout.
> - Several styling cleanups.

Low importance question, but I'll ask it anyway:

Can we somehow make the realization of "-device vmgenid" fail, if the PC
machine type in use is 2.6 or earlier? Because, those machine types do
not have DMA enabled for fw_cfg. Hence the WRITE_POINTER commands cannot
be effectively executed by firmware (the underlying virtual hardware is
not enabled).

For example, if a user adds VMGENID to a 2.5 machine type, and migrates
the VM, then on the target host,

  vmgenid_post_load()
vmgenid_update_guest()

will not be effective, because (back on the source host) the guest had
no way to write the address of the downloaded "etc/vmgenid" blob to
"etc/vmgenid_addr".

I don't think it's a big deal, but perhaps we can be user-friendly a
bit. Things that crossed my mind (different levels of ugliness):

- in the realize function for vmgenid (which I've proposed anyway, for
registering the reset handler), we could look up the fw_cfg object, and
see if its "dma-enabled" property is on or off. Possible problem: not
sure if such cross-device checks are allowed in realize methods, and if
there are any ordering issues for this. (I.e., when vmgenid's realize
checks, has the property been set for fw_cfg?)

- or else, add another boolean property to vmgenid, one that parallels
"dma-enabled" of "fw-cfg" precisely, in HW_COMPAT*. Then simply fail
realize() when this property is false.

There are probably better ways...

Anyway, it's not a big deal, I think this can be addressed incrementally
(or not at all, perhaps). I don't want to delay the series with it, just
throwing it out there.

BTW, I tested the OVMF patches against v5, and it looks good. I
cross-referenced the firmware log with acpidump in the guest, plus I
used dump-guest-memory and the "crash" utility to check the memory
contents for the guest, before/after setting the GUID with the monitor
command.

I'd like to verify S3 too, but for that I'd like to use v6 (which I hope
clears "vgia_le" on reset).

Thanks,
Laszlo

> 
> v3->v4:
> - Rebased to top of tree.
> - Re-added document patch that was accidentally dropped from the last 
> revision.
> - Added VMState functionality so that VGIA is restored properly.
> - Added Unit tests
> v2->v3:
> - Added second writeable fw_cfg for storing the VM Generaiton ID
>   address.  This uses a new linker-loader command for instructing the
>   guest to write back the allocated address.  A patch for SeaBIOS has been
>   submitted 
> (https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
>   and the resulting binary will need to be pulled into QEMU once accepted.
> - Setting VM Generation ID by command line or qmp/hmp now accepts an 
> "auto"
>   value, whereby QEMU generates a random GUID.
> - Incorporated review comments from v2 mainly around code styling and AML 
> syntax
> - Changed to use the E05 ACPI event instead of E00
> v1->v2:
> - Removed "changed" boolean parameter as it is unneeded
> - Added ACPI Notify logic
> - Style changes to pass checkpatch.pl
> - Added support for dynamic sysbus to pc_piix boards
> 
> 
> Ben Warren (8):
>   ACPI: Add a function for building named qword entries
>   linker-loader: Add new 'write pointer' command
>   docs: VM Generation ID device description
>   ACPI: Add vmgenid storage entries to the build tables
>   ACPI: Add Virtual Machine Generation ID support
>   PC: Support dynamic sysbus on pc_i440fx
>   tests: Move reusable ACPI macros into a new header file
>   tests: Add unit tests for the VM Generation ID feature
> 
> Igor Mammedov (2):
>   qmp/hmp: add query-vm-generation-id and 'info vm-generation-id'
> commands
>   qmp/hmp: add set-vm-generation-id commands
> 
>  default-configs/i386-softmmu.mak |   1 +
>  default-configs/x86_64-softmmu.mak   |   1 +
>  docs/specs/vmgenid.txt   | 239 
> +++
>  hmp-commands-info.hx |  13 ++
>  

[Qemu-devel] [PATCH v5 00/10] Add support for VM Generation ID

[ben]

From: Ben Warren 

This patch set adds support for passing a GUID to Windows guests.  It is a
re-implementation of previous patch sets written by Igor Mammedov et al, but
this time passing the GUID data as a fw_cfg blob.

This patch set has dependencies on new guest functionality, in particular the
support for a new linker-loader command and the ability to write back data
to QEMU over a DMA link.  Work is in flight in both SeaBIOS and OVMF to support 
this.

v4->v5:
- Added significantly more detail to the documentation.
- Replaced the previously-implemented linker-loader command with a new one:
  "write pointer".  This allows writing the guest address of a fw_cfg blob 
back
  to an arbitrary offset in a writeable fw_cfg file visible to QEMU.  This 
will
  require support in SeaBIOS and OVMF (ongoing).
- Fixed endianness issues throughout.
- Several styling cleanups.

v3->v4:
- Rebased to top of tree.
- Re-added document patch that was accidentally dropped from the last 
revision.
- Added VMState functionality so that VGIA is restored properly.
- Added Unit tests
v2->v3:
- Added second writeable fw_cfg for storing the VM Generaiton ID
  address.  This uses a new linker-loader command for instructing the
  guest to write back the allocated address.  A patch for SeaBIOS has been
  submitted 
(https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
  and the resulting binary will need to be pulled into QEMU once accepted.
- Setting VM Generation ID by command line or qmp/hmp now accepts an "auto"
  value, whereby QEMU generates a random GUID.
- Incorporated review comments from v2 mainly around code styling and AML 
syntax
- Changed to use the E05 ACPI event instead of E00
v1->v2:
- Removed "changed" boolean parameter as it is unneeded
- Added ACPI Notify logic
- Style changes to pass checkpatch.pl
- Added support for dynamic sysbus to pc_piix boards


Ben Warren (8):
  ACPI: Add a function for building named qword entries
  linker-loader: Add new 'write pointer' command
  docs: VM Generation ID device description
  ACPI: Add vmgenid storage entries to the build tables
  ACPI: Add Virtual Machine Generation ID support
  PC: Support dynamic sysbus on pc_i440fx
  tests: Move reusable ACPI macros into a new header file
  tests: Add unit tests for the VM Generation ID feature

Igor Mammedov (2):
  qmp/hmp: add query-vm-generation-id and 'info vm-generation-id'
commands
  qmp/hmp: add set-vm-generation-id commands

 default-configs/i386-softmmu.mak |   1 +
 default-configs/x86_64-softmmu.mak   |   1 +
 docs/specs/vmgenid.txt   | 239 +++
 hmp-commands-info.hx |  13 ++
 hmp-commands.hx  |  13 ++
 hmp.c|  21 +++
 hmp.h|   2 +
 hw/acpi/Makefile.objs|   1 +
 hw/acpi/aml-build.c  |  39 +-
 hw/acpi/bios-linker-loader.c |  35 +++--
 hw/acpi/nvdimm.c |   2 +-
 hw/acpi/vmgenid.c| 238 ++
 hw/arm/virt-acpi-build.c |   4 +-
 hw/i386/acpi-build.c |  18 ++-
 hw/i386/pc_piix.c|   1 +
 include/hw/acpi/acpi_dev_interface.h |   1 +
 include/hw/acpi/aml-build.h  |   5 +
 include/hw/acpi/bios-linker-loader.h |   3 +-
 include/hw/acpi/vmgenid.h|  37 ++
 qapi-schema.json |  31 +
 stubs/Makefile.objs  |   1 +
 stubs/vmgenid.c  |  14 ++
 tests/Makefile.include   |   2 +
 tests/acpi-utils.h   |  75 +++
 tests/bios-tables-test.c |  72 +--
 tests/vmgenid-test.c | 184 +++
 26 files changed, 962 insertions(+), 91 deletions(-)
 create mode 100644 docs/specs/vmgenid.txt
 create mode 100644 hw/acpi/vmgenid.c
 create mode 100644 include/hw/acpi/vmgenid.h
 create mode 100644 stubs/vmgenid.c
 create mode 100644 tests/acpi-utils.h
 create mode 100644 tests/vmgenid-test.c

-- 
2.7.4