Re: [Qemu-devel] [PATCH v5 5/6] acpi: Add IPMI table entries

[Corey Minyard <minyard@acm.org>]

On 05/23/2016 08:51 AM, Marcel Apfelbaum wrote:
> On 05/23/2016 04:39 PM, Corey Minyard wrote:
>> On 05/23/2016 04:05 AM, Marcel Apfelbaum wrote:
>>> On 05/22/2016 03:28 AM, Corey Minyard wrote:
>>>> Thanks for all the comments.  I didn't know about stubs, as
>>>> there's nothing that currently uses it in hw directory, but
>>>> it's easy enough to add.  I did have two comment below:
>>>>
>>>> On 05/20/2016 04:53 AM, Igor Mammedov wrote:
>>>>> On Thu, 19 May 2016 10:24:01 -0500
>>>>> minyard@acm.org wrote:
>>>> .
>>>> .
>>>> .
>>>>>
>>>>> +    aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s",
>>>>> + info->interface_name)));
>>>>> +    aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid)));
>>>>> +    aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, 
>>>>> resource)));
>>>>> +
>>>>> +    /*
>>>>> +     * The spec seems to require these to be methods. All the 
>>>>> examples
>>>>> +     * show them this way and it doesn't seem to work if they are 
>>>>> not.
>>>>> +     */
>>>>> +    method = aml_method("_IFT", 0, AML_NOTSERIALIZED);
>>>>> +    aml_append(method, aml_return(aml_int(info->interface_type)));
>>>>> +    aml_append(dev, method);
>>>>> +    method = aml_method("_SRV", 0, AML_NOTSERIALIZED);
>>>>> +    aml_append(method, aml_return(aml_int(version)));
>>>>> +    aml_append(dev, method);
>>>>> replace these methods with aml_name_decl() as they do not contain 
>>>>> any logic
>>>>> except of returning static value.
>>>>
>>>> I'm not sure why, but what you ask doesn't work.  These have to be
>>>> methods, and that is show by the IPMI spec, as the comment above
>>>> these says.
>>>>
>>>> .
>>>> .
>>>> .
>>>>
>>>>> @@ -2011,7 +2015,7 @@ build_dsdt(GArray *table_data, GArray *linker,
>>>>>           build_hpet_aml(dsdt);
>>>>>           build_piix4_pm(dsdt);one
>>>>>           build_piix4_isa_bridge(dsdt);
>>>>> -        build_isa_devices_aml(dsdt);
>>>>> +        build_isa_devices_aml(dsdt, pcms->isa_bus);
>>>>> I'm not sure about adding 'isa_bus' field to PCMachineState,
>>>>> it might be better to find a ISA bus object internally in
>>>>> build_isa_devices_aml() and assert if found more than one,
>>>>> since code assumes that there is only one anyway.
>>>>
>>>
>>> I agree.
>>>
>>>> I don't see a clean way to find the ISA bus object.  Well, I guess
>>>> you can scan the PCI bus for an ISA bus object, is that what you
>>>> mean?
>>>>
>>>
>>> You can run a QOM query. Please look for object_resolve_path function.
>>> Please let me know if you have difficulties with it.
>>>
>>> Thanks,
>>> Marcel
>>>
>>
>> Thanks for that info.
>>
>> I looked at that, and it works for a single object, but I don't see a 
>> way to
>> make this work if there is more than one IPMI device defined.
>
>
> I was under impression that you are looking for a way to find the 
> isa_bus.
> Since we have only a single isa_bus you can use object_resolve_path 
> instead
> of adding the isa_bus to the pc machine.
> I didn't refer to the IPMI device, sorry if I was not clear enough.
>

Got it, thanks.  I was over thinking it.

-corey

> Thanks,
> Marcel
>
>>
>> I changed to the following code, which seems to work ok.  You pass in 
>> the
>> ISA bus (or the SMBus in the I2C case).
>>
>> void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
>> {
>>      BusChild *kid;
>>
>>      QTAILQ_FOREACH(kid, &bus->children,  sibling) {
>>          DeviceState *dev = kid->child;
>>          Object *obj = object_dynamic_cast(OBJECT(dev), 
>> TYPE_IPMI_INTERFACE);
>>          IPMIInterface *ii;
>>          IPMIInterfaceClass *iic;
>>          IPMIFwInfo info;
>>
>>          if (!obj) {
>>              continue;
>>          }
>>
>>          ii = IPMI_INTERFACE(obj);
>>          iic = IPMI_INTERFACE_GET_CLASS(obj);
>>          memset(&info, 0, sizeof(info));
>>          iic->get_fwinfo(ii, &info);
>>          aml_append(scope, acpi_ipmi_device(&info));
>>      }
>> }
>