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 <address@hidden> 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.