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

[Marcel Apfelbaum]

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
> > > > address@hidden 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));
>      }
> }