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

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 02:20:25PM -0500, Corey Minyard wrote:
> 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, address@hidden wrote:
> >>>>From: Corey Minyard <address@hidden>
> >>>>
> >>>>Signed-off-by: Corey Minyard <address@hidden>
> >>>>---
> >>>>  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.

We add most of them within PCI0 scope instead.
E.g.

Device (PCI0) {
        Device (SMB0) {
        }
}

this way device does not need to know where it is.


> 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

Generally we scan each bus and list devices found there.


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