Re: [Qemu-devel] [PATCH v17 0/9] Virtual Machine Generation ID

[Laszlo Ersek]

On 01/20/16 10:18, Igor Mammedov wrote:
> On Tue, 19 Jan 2016 17:49:30 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 01/19/16 15:48, Igor Mammedov wrote:
>>> Here is SSDT ASL diff with vmgenid device present that
>>> Laszlo's asked for:
>>>
>>> @@ -135,6 +135,11 @@ DefinitionBlock ("tests/acpi-test-data/pc/SSDT.aml", 
>>> "SSDT", 1, "BOCHS ", "BXPCS
>>>          })
>>>      }
>>>  
>>> +    Method (\_GPE._E00, 0, NotSerialized)  // _Exx: Edge-Triggered GPE
>>> +    {
>>> +        Notify (\_SB.PCI0.VGEN, 0x80) // Status Change
>>> +    }
>>> +  
>>
>> Thanks a lot! I have one comment for this: I think _E00 cannot be used.
>> Please see the argument in my earlier (now obsolete) patch:
>>
>>   [PATCH FYI 02/13] hw/acpi: add i386 callbacks for injecting GPE 04
>>                     when the VMGENID changes
>>
>>   http://thread.gmane.org/gmane.comp.emulators.qemu/357940/focus=361705
>>
>> Thanks!
>> Laszlo
> It should be ok to use _E00 per spec since it's GPE event.
> 
> Here is quote from spec to what you were referring in your patch:
> 
> '(A query response of 0 from the embedded controller is
> reserved for “no outstanding events.”)'
> 
> That limits 0 bit handler limitation only to 'embedded controller'
> which is handled by _QXX methods.

Okay.

My other remark / question is then, do you think that having *both* _L00
and _E00 is valid?

The section under

    5.6.4.1.1 Queuing the Matching Control Method for Execution

seems to suggest that it's either-or; you can have at most one kind of
handler (either _Exx for edge triggered, or _Lxx for level triggered).

_L00 is already present in the DSDT; see build_dsdt():

        aml_append(scope, aml_method("_L00", 0, AML_NOTSERIALIZED));

and this series adds _E00.

... Hm, wait, it's okay -- your "[PATCH v17 3/9] pc: add a Virtual
Machine Generation ID device" removes the _L00 method. Cool.

(I just wonder why the ASL diff you posted didn't show this... I guess
because that diff was for the SSDT only, and the _L00 method was removed
from the DSDT.)

Thanks!
Laszlo

> 
>>
>>
>>>      Scope (\_SB)
>>>      {
>>>          Device (PCI0.PRES)
>>> @@ -703,6 +708,28 @@ DefinitionBlock ("tests/acpi-test-data/pc/SSDT.aml", 
>>> "SSDT", 1, "BOCHS ", "BXPCS
>>>                  DVNT (PCIU, One)
>>>                  DVNT (PCID, 0x03)
>>>              }
>>> +
>>> +            Device (VGEN)
>>> +            {
>>> +                Name (_HID, "QEMU0003")  // _HID: Hardware ID
>>> +                Name (_CID, "VM_Gen_Counter")  // _CID: Compatible ID
>>> +                Name (_DDN, "VM_Gen_Counter")  // _DDN: DOS Device Name
>>> +                Name (ADDR, Package (0x02)
>>> +                {
>>> +                    0xFEBF0000, 
>>> +                    Zero
>>> +                })
>>> +                Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource 
>>> Settings
>>> +                {
>>> +                    QWordMemory (ResourceProducer, PosDecode, MinFixed, 
>>> MaxFixed, Cacheable, ReadWrite,
>>> +                        0x0000000000000000, // Granularity
>>> +                        0x00000000FEBF0000, // Range Minimum
>>> +                        0x00000000FEBF0FFF, // Range Maximum
>>> +                        0x0000000000000000, // Translation Offset
>>> +                        0x0000000000001000, // Length
>>> +                        ,, , AddressRangeMemory, TypeStatic)
>>> +                })
>>> +            }
>>>          }
>>>      }
>>>  }
>>>
>>>
>>> 'make V=1 check' doesn't show it since I've forgot to extend
>>> bios-tables-test with vmgenid variant, but it should be
>>> a separate patch anyway and I'd prefer to do it after
>>> I merge DSDT with SSDT, which will reduce number of
>>> test blobs we keep in tree along with other benefits.
>>>
>>> On Tue, 19 Jan 2016 14:06:20 +0100
>>> Igor Mammedov <address@hidden> wrote:
>>>   
>>>> It's respin of v14* series which uses a PCI BAR to map
>>>> VGID page in guest AS.
>>>>
>>>> Changes since v14:
>>>>   - statically reserve used BAR resources in SSDT, so
>>>>     that Windows won't claim them during PCI rebalancing
>>>>   - support VGID page in high mem in addition to low mem
>>>>   - add QMP/HMP interfaces to get/set VM Generation ID
>>>>   - do not consume a PCI slot by default and attach
>>>>     vmgenid device as a function of multifuction
>>>>     ISA bridge.
>>>>   - allow only one vmgenid device instance
>>>>
>>>>
>>>> Tested with WS2012R2x64.
>>>> Git tree for testing:
>>>> https://github.com/imammedo/qemu.git vmgenid_v17
>>>>
>>>> * v14, https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg00530.html
>>>>
>>>> Gal Hammer (1):
>>>>   docs: vm generation id device's description
>>>>
>>>> Igor Mammedov (8):
>>>>   acpi: extend ACPI interface to provide access to ACPI registers and
>>>>     SCI irq
>>>>   pc: add a Virtual Machine Generation ID device
>>>>   tests: add a unit test for the vmgenid device.
>>>>   qmp/hmp: add query-vm-generation-id and 'info vm-generation-id'
>>>>     commands
>>>>   qmp/hmp: add set-vm-generation-id commands
>>>>   add MachineClass->default_props for setting default device properties
>>>>   pc: put PIIX3 in slot 1 explicitly and cleanup functions assignment
>>>>   pc/q53: by default put vmgenid device as an function of ISA bridge
>>>>
>>>>  default-configs/i386-softmmu.mak     |   1 +
>>>>  default-configs/x86_64-softmmu.mak   |   1 +
>>>>  docs/specs/pci-ids.txt               |   1 +
>>>>  docs/specs/vmgenid.txt               |  36 +++++++
>>>>  hmp-commands-info.hx                 |  13 +++
>>>>  hmp-commands.hx                      |  13 +++
>>>>  hmp.c                                |  21 ++++
>>>>  hmp.h                                |   2 +
>>>>  hw/acpi/piix4.c                      |  17 ++++
>>>>  hw/i386/acpi-build.c                 |  56 ++++++++++-
>>>>  hw/i386/pc_piix.c                    |  29 ++++--
>>>>  hw/i386/pc_q35.c                     |  12 +++
>>>>  hw/isa/lpc_ich9.c                    |  16 +++
>>>>  hw/isa/vt82c686.c                    |  19 ++++
>>>>  hw/misc/Makefile.objs                |   1 +
>>>>  hw/misc/vmgenid.c                    | 183 
>>>> +++++++++++++++++++++++++++++++++++
>>>>  hw/pci-host/piix.c                   |   9 +-
>>>>  include/hw/acpi/acpi.h               |   1 +
>>>>  include/hw/acpi/acpi_dev_interface.h |   9 ++
>>>>  include/hw/boards.h                  |   1 +
>>>>  include/hw/i386/ich9.h               |   3 +-
>>>>  include/hw/i386/pc.h                 |   9 +-
>>>>  include/hw/misc/vmgenid.h            |  27 ++++++
>>>>  include/hw/pci/pci.h                 |   1 +
>>>>  qapi-schema.json                     |  18 ++++
>>>>  qmp-commands.hx                      |  41 ++++++++
>>>>  stubs/Makefile.objs                  |   1 +
>>>>  stubs/vmgenid.c                      |  13 +++
>>>>  tests/Makefile                       |   2 +
>>>>  tests/vmgenid-test.c                 |  92 ++++++++++++++++++
>>>>  vl.c                                 |   4 +
>>>>  31 files changed, 636 insertions(+), 16 deletions(-)
>>>>  create mode 100644 docs/specs/vmgenid.txt
>>>>  create mode 100644 hw/misc/vmgenid.c
>>>>  create mode 100644 include/hw/misc/vmgenid.h
>>>>  create mode 100644 stubs/vmgenid.c
>>>>  create mode 100644 tests/vmgenid-test.c
>>>>  
>>>   
>>
>