[Skiboot] [PATCH] ipmi: Sending a list of PCI devices via IPMI OEM messages

[Artem Senichev]

On Thu, May 03, 2018 at 07:52:43PM +1000, Stewart Smith wrote:
> Artem Senichev <a.senichev at yadro.com> writes:
> > Implements sending a list of installed PCI devices through IPMI
> > protocol - OEM message (NetFn 0x3a (IBM), Command 0x2a).
> Is the handling of this something you're looking at upstreaming on
> OpenBMC?
It will be, actually we have a patch to add PCI device handling:
https://github.com/YADRO-KNS/openpower-host-ipmi-oem/pull/1/files
I will send it to OpenBMC's gerrit as soon as we have PCI list support from OPAL.

> A minor change, I'd suggest adding an ipmi_oem_pci_device_list to struct
> bmc_platform (like we have today for ipmi_oem_partial_add_esel and
> ipmi_oem_pnor_access_status) and have the code to send the detected PCI
> devices in common PCI code and the OEM command in the common
> astbmc_openbmc struct in platforms/astbmc/common.c.
Perhaps I misunderstand something, but I don't have any specific code for PCI
common code, there is just a for_each_phb() call that generates an OEM IPMI
message for each device. Currently I have defined Vesnin's own
pci_probe_complete() callback that initiates sending the PCI device list through
IPMI.
Something like this:
OPAL's PCI subsystem -> platform.pci_probe_complete() -> ipmi_inventory_pci() -> for_each_phb -> ipmi -> BMC

What do you mean by "have the code to send the detected PCI devices in
common PCI code"?

I didn't put the OEM command to astbmc because there is no any AST specific,
the command can be handled with any other BMC implementation that knows about
the message format (for example, qemu with IPMI emulator).
Do you really want to move the IPMI-OEM code to platforms/astbmc/common.c?

> If the change isn't going to go upstream to openbmc, I wonder if the
> vendor code here is correct? It may be worthwhile to constrain that to a
> struct bmc_platform for vesnin until it's upstream.
I don't know which code to use :)
I chose 0x3a (IBM) as vendor code because IBM is a founder of OpenPOWER.
The command id 0x2a it is just 42 - "The Ultimate Question of Life, the Universe, and Everything".
I will change them if you specify the correct identifiers.

> > +	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_DEVICE_ID, &dev->device_id);
> > +	dev->device_id = cpu_to_be16(dev->device_id);
> I think these two could be replaced by PCI_VENDOR_ID(pd->vdid) and
> PCI_DEVICE_ID(pd->vdid) ?
Yes, thanks!

> > +	/* Synchronously send the IPMI message, the queue is too small */
> > +	ipmi_queue_msg_sync(msg);
> I guess in an ideal world we'd walk the hierarchy in the callback when
> the message is done, but seeing as where we are in boot, I think this
> ends up being okay.... Or, at least, I'm not going to worry about it
> taking too long.
In an ideal world we have ipmi_queue_msg_sync() function with additional
parameter that specified the timeout.
I can do it later, but not as part of this patch.

> > +#define IPMI_NETFN_IBM			0x3a
> Hrm.. this makes me wonder if we have the ipmi_oem_partial_add_esel
> incorrect for OpenBMC.
Yea, there is something strange with this id's. We have a patch for hostboot
that makes partial_add_esel() function to use NETFUN_IBM instead of NETFUN_AMI,
in skiboot our platform uses astbmc_ami (not astbmc_openbmc).

> > +/* Report an inventory of PCI devices.
> > + * @ep_only: Device filter, true to handle End Point devices only.
> > + */
> > +void ipmi_inventory_pci(bool ep_only);
> I'd prefer just naming the parameter endpoint_only to make it obvious.
Ok, I'll change it.

--
Regards,
Artem Senichev
Software Engineer, YADRO.