Re: [Qemu-devel] [RFC 2/5] acpi: introduce TYPE_ACPI_DEVICE_IF interface

Re: [Qemu-devel] [RFC 2/5] acpi: introduce TYPE_ACPI_DEVICE_IF interface

[Igor Mammedov]

On Fri, 30 May 2014 09:44:20 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 05/30/2014 09:39 AM, Igor Mammedov wrote:
> 
> >>> +# @source_event: Arg0 - An Integer containing the source event
> >>> +#
> >>> +# @status_code: Arg1 – An Integer containing the status code
> >>
> 
> >>> +{ 'type': 'ACPIOSTInfo',
> >>> +  'data'  : { 'device': 'str',
> >>> +              'source': 'int',
> >>> +              'status': 'int',
> >>> +              'slot': 'int' } }
> >>
> >> ...this type declaration.  I have no idea which one of the two is wrong.
> > What do you mean under wrong?
> 
> Sorry for not being clear enough.  I'm not sure whether you meant to
> document four fields (device, source, status, and slot) or whether the
> command should have been just two fields ( 'data': { 'source_event':
> 'int', 'status_code': 'int' } ).
> 
> Although re-reading what I just wrote, it appears your 'source' field
> matches the 'source_event' documentation, the 'status' field matches the
> 'status_code' documentation, and you omitted the 'device' and 'slot'
> documentation.
Thanks, I'll fix it.

> 
> And my question in 4/5 remains - should 'source' and/or 'status' be
> defined as an enum rather than an open-coded int?
Using enum for 'source' event, might be possible if we restrict ourselves
to a limit set of supported values and ignore the rest of
unknown/not implemented values. It might be not a good idea to lose
events because QEMU doesn't know about them.
Also from maintainability PoV it would add a bunch of mapping code
from 'int' into our enum, which would essentially prevent
new source events be exposed to users until QEMU adds support for
them.

For 'status' code it'd add a lot more mapping code to translate its
known values into enum since it's changing depending on 'source'.
Especially if it comes to expanding range of known values in
table 6-160 of ACPI5.0 spec.

I think it would be better to expose raw values the guest reported
via _OST and let management to pick-up ones it's interested in and
allow it to handle not implemented ones as it wishes rather than
hiding them at QEMU level.

Re: [Qemu-devel] [RFC 2/5] acpi: introduce TYPE_ACPI_DEVICE_IF interface

[Eric Blake]

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

On 06/05/2014 04:10 AM, Igor Mammedov wrote:

>> And my question in 4/5 remains - should 'source' and/or 'status' be
>> defined as an enum rather than an open-coded int?
> Using enum for 'source' event, might be possible if we restrict ourselves
> to a limit set of supported values and ignore the rest of
> unknown/not implemented values. It might be not a good idea to lose
> events because QEMU doesn't know about them.
> Also from maintainability PoV it would add a bunch of mapping code
> from 'int' into our enum, which would essentially prevent
> new source events be exposed to users until QEMU adds support for
> them.
> 
> For 'status' code it'd add a lot more mapping code to translate its
> known values into enum since it's changing depending on 'source'.
> Especially if it comes to expanding range of known values in
> table 6-160 of ACPI5.0 spec.
> 
> I think it would be better to expose raw values the guest reported
> via _OST and let management to pick-up ones it's interested in and
> allow it to handle not implemented ones as it wishes rather than
> hiding them at QEMU level.

Fair enough - if qemu is just doing passthrough, and is not doing any
particular change in behavior based on particular values being passed
through, then exposing raw status codes is the only scalable approach
(new codes are equally uninteresting to qemu, and passthrough handles a
raw value better than qemu trying to interpret and name all values being
passed through).  But probably worth documenting this rationale in the
commit message, so we can remember why we chose to go this way.

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