diff mbox

[6/7] ipmi: Fix SSIF ACPI handling to use the right CRS

Message ID 1462995966-1184-7-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard May 11, 2016, 7:46 p.m. UTC 
From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/ipmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Michael S. Tsirkin May 12, 2016, 7:33 a.m. UTC | #1 
On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/acpi/ipmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> index 731f4ad..c187fdd 100644
> --- a/hw/acpi/ipmi.c
> +++ b/hw/acpi/ipmi.c
> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>                              regspacing, info->register_length));
>          break;
>      case IPMI_MEMSPACE_SMBUS:
> -        aml_append(crs, aml_return(aml_int(info->base_address)));
> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
> +                                                  info->base_address,
> +                                                  info->acpi_parent));

Isn't this fairly new? If so using these opcodes
is likely to break some older guests. Maybe they already don't
work, but I'd like to see some explanation about that,
and what was tested.

>          break;
>      default:
>          abort();
> -- 
> 2.7.4
Corey Minyard May 12, 2016, 1:29 p.m. UTC | #2 
On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/acpi/ipmi.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>> index 731f4ad..c187fdd 100644
>> --- a/hw/acpi/ipmi.c
>> +++ b/hw/acpi/ipmi.c
>> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>                               regspacing, info->register_length));
>>           break;
>>       case IPMI_MEMSPACE_SMBUS:
>> -        aml_append(crs, aml_return(aml_int(info->base_address)));
>> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
>> +                                                  info->base_address,
>> +                                                  info->acpi_parent));
> Isn't this fairly new? If so using these opcodes
> is likely to break some older guests. Maybe they already don't
> work, but I'd like to see some explanation about that,
> and what was tested.

This is new with the 5.0 specification.

I haven't done extensive testing on anything but Linux 3.10 and later.  
Well, I might have run 2.6.32, but I can't remember.  I don't have the 
ability to test Windows.

But isn't the idea of these definitions that they are ignored if the OS 
doesn't understand them?  Otherwise you could never add anything.

-corey

>>           break;
>>       default:
>>           abort();
>> -- 
>> 2.7.4
Michael S. Tsirkin May 12, 2016, 1:39 p.m. UTC | #3 
On Thu, May 12, 2016 at 08:29:53AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
> >On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
> >>From: Corey Minyard <cminyard@mvista.com>
> >>
> >>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>---
> >>  hw/acpi/ipmi.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
> >>index 731f4ad..c187fdd 100644
> >>--- a/hw/acpi/ipmi.c
> >>+++ b/hw/acpi/ipmi.c
> >>@@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
> >>                              regspacing, info->register_length));
> >>          break;
> >>      case IPMI_MEMSPACE_SMBUS:
> >>-        aml_append(crs, aml_return(aml_int(info->base_address)));
> >>+        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
> >>+                                                  info->base_address,
> >>+                                                  info->acpi_parent));
> >Isn't this fairly new? If so using these opcodes
> >is likely to break some older guests. Maybe they already don't
> >work, but I'd like to see some explanation about that,
> >and what was tested.
> 
> This is new with the 5.0 specification.
> 
> I haven't done extensive testing on anything but Linux 3.10 and later.
> Well, I might have run 2.6.32, but I can't remember.  I don't have the
> ability to test Windows.
> 
> But isn't the idea of these definitions that they are ignored if the OS
> doesn't understand them?

Not always. It depends, spec does not require it.

You can check which revision does OSPM support but
you have to decide what to do for an old revision then.

>  Otherwise you could never add anything.
> 
> -corey

Question is, what happened before this change?


> >>          break;
> >>      default:
> >>          abort();
> >>-- 
> >>2.7.4
Corey Minyard May 13, 2016, 1:13 p.m. UTC | #4 
On 05/12/2016 08:39 AM, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 08:29:53AM -0500, Corey Minyard wrote:
>> On 05/12/2016 02:33 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 11, 2016 at 02:46:05PM -0500, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>   hw/acpi/ipmi.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>>>> index 731f4ad..c187fdd 100644
>>>> --- a/hw/acpi/ipmi.c
>>>> +++ b/hw/acpi/ipmi.c
>>>> @@ -49,7 +49,9 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>>>                               regspacing, info->register_length));
>>>>           break;
>>>>       case IPMI_MEMSPACE_SMBUS:
>>>> -        aml_append(crs, aml_return(aml_int(info->base_address)));
>>>> +        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
>>>> +                                                  info->base_address,
>>>> +                                                  info->acpi_parent));
>>> Isn't this fairly new? If so using these opcodes
>>> is likely to break some older guests. Maybe they already don't
>>> work, but I'd like to see some explanation about that,
>>> and what was tested.
>> This is new with the 5.0 specification.
>>
>> I haven't done extensive testing on anything but Linux 3.10 and later.
>> Well, I might have run 2.6.32, but I can't remember.  I don't have the
>> ability to test Windows.
>>
>> But isn't the idea of these definitions that they are ignored if the OS
>> doesn't understand them?
> Not always. It depends, spec does not require it.
>
> You can check which revision does OSPM support but
> you have to decide what to do for an old revision then.
>
>>   Otherwise you could never add anything.
>>
>> -corey
> Question is, what happened before this change?

I could add a "noacpi" option for the device, that way the user could
accommodate an OS that couldn't handle the option.

I also realized that the addition of the SMBus ACPI device required
backwards compatibility so it wouldn't be there for previous machine
versions.  So you could specify a 2.6 machine and not get the ACPI
entry.

-corey
diff mbox

Patch

diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
index 731f4ad..c187fdd 100644
--- a/hw/acpi/ipmi.c
+++ b/hw/acpi/ipmi.c
@@ -49,7 +49,9 @@  static Aml *aml_ipmi_crs(IPMIFwInfo *info)
                             regspacing, info->register_length));
         break;
     case IPMI_MEMSPACE_SMBUS:
-        aml_append(crs, aml_return(aml_int(info->base_address)));
+        aml_append(crs, aml_i2c_serial_bus_device(0, 100000,
+                                                  info->base_address,
+                                                  info->acpi_parent));
         break;
     default:
         abort();