Re: [Qemu-devel] [PATCH 7/7] ipmi: Add ACPI to the SMBus IPMI device

[Corey Minyard <cminyard@mvista.com>]

On 05/12/2016 08:35 AM, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
>> On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>   hw/ipmi/smbus_ipmi.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
>>>> index 4e7203b..3a34aaf 100644
>>>> --- a/hw/ipmi/smbus_ipmi.c
>>>> +++ b/hw/ipmi/smbus_ipmi.c
>>>> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>>>>       sid->fwinfo.base_address = sid->parent.i2c.address;
>>>>       sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>>>>       sid->fwinfo.register_spacing = 1;
>>>> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
>>> I don't think it's a good idea to spread things like PCI0
>>> outside acpi-build.c. Why do you want to pass in the path
>>> at all?
>> You have to define the namespace for the ASL definition,
>> and you have to give the name in the serial bus definition.
>> However, thinking it through some more, this name needs
>> to come from the I2C device, not just hard-coded here.
>>
>> -corey
> For now you can put it as PCI0 within aml-build.c.
>
> Also do we need \\_SB.PCI0? Why not just create the
> device within PCI0 scope?
I'm not sure I follow here. \SB.PCI0.SMB0 is the proper
scope to identify which SMBus the device is on.  This
is how the ISA devices are added, for instance.

What I've done now to fix this is added an ACPI namespace
to the I2C bus structure and stored the value for the I2C
bus there in the i386 code.

Maybe it should be in BusState?  That way the ISA IPMI code
could pull it from the bus, too.

Putting PCI0 into aml-build.c (or hw/acpi/ipmi.c, really) seems
like a violation of scope.

-corey

>>>>       ipmi_add_fwinfo(&sid->fwinfo, errp);
>>>>   }
>>>> -- 
>>>> 2.7.4