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

[Marcel Apfelbaum <marcel@redhat.com>]

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.

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));
>      }
> }