[Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Igor Mammedov]

emits event when ACPI OSPM evaluates _OST method
of ACPI device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 - use new QAPI event infrastructure
      from rebased PCI tree on top of today's QMP pull request
---
 hw/acpi/memory_hotplug.c |    7 ++++++-
 qapi-event.json          |   10 ++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index e7009bc..38ca415 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -3,6 +3,7 @@
 #include "hw/mem/pc-dimm.h"
 #include "hw/boards.h"
 #include "trace.h"
+#include "qapi-event.h"
 
 static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
 {
@@ -88,6 +89,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
 {
     MemHotplugState *mem_st = opaque;
     MemStatus *mdev;
+    ACPIOSTInfo *info;
 
     if (!mem_st->dev_count) {
         return;
@@ -119,8 +121,11 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
         mdev = &mem_st->devs[mem_st->selector];
         mdev->ost_status = data;
         trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
-        /* TODO: report async error */
         /* TODO: implement memory removal on guest signal */
+
+        info = acpi_memory_device_status(mem_st->selector, mdev);
+        qapi_event_send_acpi_device_ost(info, &error_abort);
+        qapi_free_ACPIOSTInfo(info);
         break;
     case 0x14:
         mdev = &mem_st->devs[mem_st->selector];
diff --git a/qapi-event.json b/qapi-event.json
index fbdda48..6876327 100644
--- a/qapi-event.json
+++ b/qapi-event.json
@@ -304,3 +304,13 @@
 { 'event': 'QUORUM_REPORT_BAD',
   'data': { '*error': 'str', 'node-name': 'str',
             'sector-num': 'int', 'sector-count': 'int' } }
+
+##
+# @ACPI_DEVICE_OST
+#
+# Emitted when guest executes ACPI _OST method.
+#
+# @info: ACPIOSTInfo type as described in qapi-schema.json
+##
+{ 'event': 'ACPI_DEVICE_OST',
+     'data': { 'info': 'ACPIOSTInfo' } }
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On 06/20/2014 02:33 AM, Igor Mammedov wrote:
> emits event when ACPI OSPM evaluates _OST method
> of ACPI device.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>  - use new QAPI event infrastructure
>       from rebased PCI tree on top of today's QMP pull request
> ---
>  hw/acpi/memory_hotplug.c |    7 ++++++-
>  qapi-event.json          |   10 ++++++++++
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 

> +++ b/qapi-event.json
> @@ -304,3 +304,13 @@
>  { 'event': 'QUORUM_REPORT_BAD',
>    'data': { '*error': 'str', 'node-name': 'str',
>              'sector-num': 'int', 'sector-count': 'int' } }
> +
> +##
> +# @ACPI_DEVICE_OST
> +#
> +# Emitted when guest executes ACPI _OST method.
> +#
> +# @info: ACPIOSTInfo type as described in qapi-schema.json

Needs 'Since: 2.1'.

> +##
> +{ 'event': 'ACPI_DEVICE_OST',
> +     'data': { 'info': 'ACPIOSTInfo' } }
> 

Not your fault, as the problem already exists, but it's a bit awkward
that qapi-event.json is not self-contained, and your patch is only
making it worse.  qapi-event.json only makes sense when included by
qapi-schema.json, when ideally it would be nice if it made sense if
compiled in isolation. I already pointed this fact out on Wenchao's
series that made events part of QAPI, but fixing it first requires
teaching the code generators to flag places where a type is used without
a pre-definition, so that we know which types have to be moved into a
common include.  So don't let this comment hold up your patch.

If you add the Since line,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Wenchao Xia]

于 2014/6/20 23:46, Eric Blake 写道:
> On 06/20/2014 02:33 AM, Igor Mammedov wrote:
>> emits event when ACPI OSPM evaluates _OST method
>> of ACPI device.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>> v2:
>>   - use new QAPI event infrastructure
>>        from rebased PCI tree on top of today's QMP pull request
>> ---
>>   hw/acpi/memory_hotplug.c |    7 ++++++-
>>   qapi-event.json          |   10 ++++++++++
>>   2 files changed, 16 insertions(+), 1 deletions(-)
>>
>
>> +++ b/qapi-event.json
>> @@ -304,3 +304,13 @@
>>   { 'event': 'QUORUM_REPORT_BAD',
>>     'data': { '*error': 'str', 'node-name': 'str',
>>               'sector-num': 'int', 'sector-count': 'int' } }
>> +
>> +##
>> +# @ACPI_DEVICE_OST
>> +#
>> +# Emitted when guest executes ACPI _OST method.
>> +#
>> +# @info: ACPIOSTInfo type as described in qapi-schema.json
>
> Needs 'Since: 2.1'.
>
   With above added,
Reviewed-by: Wenchao Xia <wenchaoqemu@gmail.com>

>> +##
>> +{ 'event': 'ACPI_DEVICE_OST',
>> +     'data': { 'info': 'ACPIOSTInfo' } }
>>
>
> Not your fault, as the problem already exists, but it's a bit awkward
> that qapi-event.json is not self-contained, and your patch is only
> making it worse.  qapi-event.json only makes sense when included by
> qapi-schema.json, when ideally it would be nice if it made sense if
> compiled in isolation. I already pointed this fact out on Wenchao's
> series that made events part of QAPI, but fixing it first requires
> teaching the code generators to flag places where a type is used without



> a pre-definition, so that we know which types have to be moved into a
> common include.  So don't let this comment hold up your patch.
>
   I think two issues should be addressed:
1 don't let generator guess the type if define is not found.
2 Support duplicated include, that is:

              qapi/types.json
                      |
       ---------------------------
       |                          |
qapi/qapi-event.json      qapi/block.json
              |                |
               qapi-schema-json

   Making sure above include doesn't generate two copy of types from
qapi/types.json.
   We can improve it later.

> If you add the Since line,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Markus Armbruster]

Wenchao Xia <wenchaoqemu@gmail.com> writes:

> 于 2014/6/20 23:46, Eric Blake 写道:
>> On 06/20/2014 02:33 AM, Igor Mammedov wrote:
>>> emits event when ACPI OSPM evaluates _OST method
>>> of ACPI device.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v2:
>>>   - use new QAPI event infrastructure
>>>        from rebased PCI tree on top of today's QMP pull request
>>> ---
>>>   hw/acpi/memory_hotplug.c |    7 ++++++-
>>>   qapi-event.json          |   10 ++++++++++
>>>   2 files changed, 16 insertions(+), 1 deletions(-)
>>>
>>
>>> +++ b/qapi-event.json
>>> @@ -304,3 +304,13 @@
>>>   { 'event': 'QUORUM_REPORT_BAD',
>>>     'data': { '*error': 'str', 'node-name': 'str',
>>>               'sector-num': 'int', 'sector-count': 'int' } }
>>> +
>>> +##
>>> +# @ACPI_DEVICE_OST
>>> +#
>>> +# Emitted when guest executes ACPI _OST method.
>>> +#
>>> +# @info: ACPIOSTInfo type as described in qapi-schema.json
>>
>> Needs 'Since: 2.1'.
>>
>   With above added,
> Reviewed-by: Wenchao Xia <wenchaoqemu@gmail.com>
>
>>> +##
>>> +{ 'event': 'ACPI_DEVICE_OST',
>>> +     'data': { 'info': 'ACPIOSTInfo' } }
>>>
>>
>> Not your fault, as the problem already exists, but it's a bit awkward
>> that qapi-event.json is not self-contained, and your patch is only
>> making it worse.  qapi-event.json only makes sense when included by
>> qapi-schema.json, when ideally it would be nice if it made sense if
>> compiled in isolation. I already pointed this fact out on Wenchao's
>> series that made events part of QAPI, but fixing it first requires
>> teaching the code generators to flag places where a type is used without
>
>
>
>> a pre-definition, so that we know which types have to be moved into a
>> common include.  So don't let this comment hold up your patch.
>>
>   I think two issues should be addressed:
> 1 don't let generator guess the type if define is not found.
> 2 Support duplicated include, that is:
>
>              qapi/types.json
>                      |
>       ---------------------------
>       |                          |
> qapi/qapi-event.json      qapi/block.json
>              |                |
>               qapi-schema-json
>
>   Making sure above include doesn't generate two copy of types from
> qapi/types.json.

See Benoît's commit 24fd848 "qapi: skip redundant includes".

>   We can improve it later.
>
>> If you add the Since line,
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On 06/20/2014 05:47 PM, Wenchao Xia wrote:

>> Not your fault, as the problem already exists, but it's a bit awkward
>> that qapi-event.json is not self-contained, and your patch is only
>> making it worse.  qapi-event.json only makes sense when included by
>> qapi-schema.json, when ideally it would be nice if it made sense if
>> compiled in isolation. I already pointed this fact out on Wenchao's
>> series that made events part of QAPI, but fixing it first requires
>> teaching the code generators to flag places where a type is used without
>> a pre-definition, so that we know which types have to be moved into a
>> common include.  So don't let this comment hold up your patch.
>>
>   I think two issues should be addressed:
> 1 don't let generator guess the type if define is not found.

But we WANT to allow use of a type so long as it gets defined later in
the same file.  So the trick is distinguishing between:

base.json - use of 'TypeFoo'
total.json: include base.json, define 'TypeFoo'

(broken, base.json was not self-contained)

vs.

base.json - define 'TypeFoo'
total.json - use of 'TypeFoo', then include base.json

(supported - even though 'TypeFoo' was used prior to being defined, the
include later in the file meant that total.json was self-contained)

> 2 Support duplicated include, that is:
> 
>              qapi/types.json
>                      |
>       ---------------------------
>       |                          |
> qapi/qapi-event.json      qapi/block.json
>              |                |
>               qapi-schema-json
> 
>   Making sure above include doesn't generate two copy of types from
> qapi/types.json.

We already support that.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Michael S. Tsirkin]

On Fri, Jun 20, 2014 at 10:33:26AM +0200, Igor Mammedov wrote:
> emits event when ACPI OSPM evaluates _OST method
> of ACPI device.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

The subject and commit log are confusing here.
We already have
	Author: Igor Mammedov <imammedo@redhat.com>
	    qmp: add ACPI_DEVICE_OST event handling

So I am guessing this reworks existing functionality
to use the new QMP infrastructure.
Please rewrite both subject and commit log.


> ---
> v2:
>  - use new QAPI event infrastructure
>       from rebased PCI tree on top of today's QMP pull request
> ---
>  hw/acpi/memory_hotplug.c |    7 ++++++-
>  qapi-event.json          |   10 ++++++++++
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index e7009bc..38ca415 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -3,6 +3,7 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/boards.h"
>  #include "trace.h"
> +#include "qapi-event.h"
>  
>  static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
>  {
> @@ -88,6 +89,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>  {
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
> +    ACPIOSTInfo *info;
>  
>      if (!mem_st->dev_count) {
>          return;
> @@ -119,8 +121,11 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>          mdev = &mem_st->devs[mem_st->selector];
>          mdev->ost_status = data;
>          trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
> -        /* TODO: report async error */
>          /* TODO: implement memory removal on guest signal */
> +
> +        info = acpi_memory_device_status(mem_st->selector, mdev);
> +        qapi_event_send_acpi_device_ost(info, &error_abort);
> +        qapi_free_ACPIOSTInfo(info);
>          break;
>      case 0x14:
>          mdev = &mem_st->devs[mem_st->selector];
> diff --git a/qapi-event.json b/qapi-event.json
> index fbdda48..6876327 100644
> --- a/qapi-event.json
> +++ b/qapi-event.json
> @@ -304,3 +304,13 @@
>  { 'event': 'QUORUM_REPORT_BAD',
>    'data': { '*error': 'str', 'node-name': 'str',
>              'sector-num': 'int', 'sector-count': 'int' } }
> +
> +##
> +# @ACPI_DEVICE_OST
> +#
> +# Emitted when guest executes ACPI _OST method.
> +#
> +# @info: ACPIOSTInfo type as described in qapi-schema.json
> +##
> +{ 'event': 'ACPI_DEVICE_OST',
> +     'data': { 'info': 'ACPIOSTInfo' } }
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On 06/21/2014 11:58 PM, Michael S. Tsirkin wrote:
> On Fri, Jun 20, 2014 at 10:33:26AM +0200, Igor Mammedov wrote:
>> emits event when ACPI OSPM evaluates _OST method
>> of ACPI device.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> The subject and commit log are confusing here.
> We already have
> 	Author: Igor Mammedov <imammedo@redhat.com>
> 	    qmp: add ACPI_DEVICE_OST event handling
> 
> So I am guessing this reworks existing functionality
> to use the new QMP infrastructure.
> Please rewrite both subject and commit log.
> 

Based on IRC chatter, it looks like Igor has reworked the patch to be
applied as part of Luiz' QMP queue in a way that minimizes the conflicts
there.  Igor's rework is more-or-less a revert of the v2 patch that was
applied prematurely in mst's tree, combined with this v3 patch to apply
correctly in Wenchao's event-as-qapi series on Luiz' tree.

At this point, I'd wait for a PULL request from Luiz to determine if
anything further is needed on this front.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3] qmp: add ACPI_DEVICE_OST event handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On 06/23/2014 09:59 AM, Eric Blake wrote:
> On 06/21/2014 11:58 PM, Michael S. Tsirkin wrote:
>> On Fri, Jun 20, 2014 at 10:33:26AM +0200, Igor Mammedov wrote:
>>> emits event when ACPI OSPM evaluates _OST method
>>> of ACPI device.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>
>> The subject and commit log are confusing here.
>> We already have
>> 	Author: Igor Mammedov <imammedo@redhat.com>
>> 	    qmp: add ACPI_DEVICE_OST event handling
>>
>> So I am guessing this reworks existing functionality
>> to use the new QMP infrastructure.
>> Please rewrite both subject and commit log.
>>
> 
> Based on IRC chatter, it looks like Igor has reworked the patch to be
> applied as part of Luiz' QMP queue in a way that minimizes the conflicts
> there.  Igor's rework is more-or-less a revert of the v2 patch that was
> applied prematurely in mst's tree, combined with this v3 patch to apply
> correctly in Wenchao's event-as-qapi series on Luiz' tree.
> 
> At this point, I'd wait for a PULL request from Luiz to determine if
> anything further is needed on this front.

Indeed,
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05424.html (in
particular 33/43) solves the problems.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]