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

Stewart Smith stewart at linux.ibm.com
Thu May 3 19:52:43 AEST 2018


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?

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.

(this would mean there's less vesnin specific code which is always nice)

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.


> The synchronization session can be started from pci_probe_complete function to send PCI device list to the BMC.
>
> Signed-off-by: Artem Senichev <a.senichev at yadro.com>
> ---
>  hw/ipmi/Makefile.inc      |   2 +-
>  hw/ipmi/ipmi-pciinv.c     | 149 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/ipmi.h            |   7 +++
>  platforms/astbmc/vesnin.c |  11 +++-
>  4 files changed, 167 insertions(+), 2 deletions(-)
>  create mode 100644 hw/ipmi/ipmi-pciinv.c
>
> diff --git a/hw/ipmi/Makefile.inc b/hw/ipmi/Makefile.inc
> index 34d6bd31..78350907 100644
> --- a/hw/ipmi/Makefile.inc
> +++ b/hw/ipmi/Makefile.inc
> @@ -1,7 +1,7 @@
>  SUBDIRS += hw/ipmi
>  
>  IPMI_OBJS  = ipmi-rtc.o ipmi-power.o ipmi-fru.o ipmi-sel.o
> -IPMI_OBJS += ipmi-watchdog.o ipmi-sensor.o ipmi-attn.o
> +IPMI_OBJS += ipmi-watchdog.o ipmi-sensor.o ipmi-attn.o ipmi-pciinv.o
>  
>  IPMI = hw/ipmi/built-in.a
>  $(IPMI): $(IPMI_OBJS:%=hw/ipmi/%)
> diff --git a/hw/ipmi/ipmi-pciinv.c b/hw/ipmi/ipmi-pciinv.c
> new file mode 100644
> index 00000000..33f1f92b
> --- /dev/null
> +++ b/hw/ipmi/ipmi-pciinv.c
> @@ -0,0 +1,149 @@
> +/**
> + * ipmi-pciinv.c -- PCI device inventory support.
> + *
> + * Implements sending a list of installed PCI devices through IPMI protocol.
> + * Each PCI device description is send as standalone IPMI message.
> + * A list of devices can be collected from separated messages using the
> + * session identifier. The session Id is an incremental counter that updates at
> + * start of synchronization session.
> + *
> + * Copyright (c) 2018 YADRO (KNS Group LLC)
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + *
> + * Written by Artem Senichev <a.senichev at yadro.com>.
> + */
> +
> +#include <ipmi.h>
> +#include <pci.h>
> +#include <pci-cfg.h>
> +
> +/* Current version of the PCI inventory synchronization packet */
> +#define PCIINV_PACK_VERSION 1
> +
> +/**
> + * struct pciinv_device - PCI device inventory description.
> + * @domain_num: Domain number.
> + * @bus_num: Bus number.
> + * @device_num: Device number.
> + * @func_num: Function number.
> + * @vendor_id: Vendor Id.
> + * @device_id: Device Id.
> + * @class_code: Device class code.
> + * @revision: Revision number.
> + *
> + * All fields have Big Endian byte order.
> + */
> +struct pciinv_device {
> +	uint16_t	domain_num;
> +	uint8_t		bus_num;
> +	uint8_t		device_num;
> +	uint8_t		func_num;
> +	uint16_t	vendor_id;
> +	uint16_t	device_id;
> +	uint32_t	class_code;
> +	uint8_t		revision;
> +} __packed;
> +
> +/**
> + * struct pciinv_packet - IPMI message packet data.
> + * @version: Packet version, must be set to %PCIINV_PACK_VERSION.
> + * @session: Sync session Id.
> + * @device: PCI device description.
> + */
> +struct pciinv_packet {
> +	uint8_t		version;
> +	uint8_t		session;
> +	struct pciinv_device device;
> +} __packed;
> +
> +
> +/* Id of the current sync session. */
> +static uint8_t session_id;
> +
> +
> +/**
> + * pciinv_fill() - Fill the PCI device inventory description.
> + * @phb: PHB descriptor.
> + * @pd: PCE device descriptor.
> + * @dev: Inventory device description to fill.
> + */
> +static void pciinv_fill(struct phb *phb, struct pci_device *pd, struct pciinv_device* dev)
> +{
> +	dev->domain_num = cpu_to_be16(phb->opal_id & 0xffff);
> +	dev->bus_num = (pd->bdfn >> 8) & 0xff;
> +	dev->device_num = (pd->bdfn >> 3) & 0x1f;
> +	dev->func_num = pd->bdfn & 0x7;
> +
> +	pci_cfg_read16(phb, pd->bdfn, PCI_CFG_VENDOR_ID, &dev->vendor_id);
> +	dev->vendor_id = cpu_to_be16(dev->vendor_id);
> +
> +	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) ?

> +
> +	dev->class_code = cpu_to_be32(pd->class & 0xffffff);
> +
> +	pci_cfg_read8(phb, pd->bdfn, PCI_CFG_REV_ID, &dev->revision);
> +}
> +
> +
> +/**
> + * pci_walk_dev_cb() - Callback from PCI enumerator, see :c:func:`pci_walk_dev`.
> + */
> +static int pci_walk_dev_cb(struct phb *phb, struct pci_device *pd, void *filter)
> +{
> +	struct ipmi_msg *msg;
> +	struct pciinv_packet pack = {
> +		.version = PCIINV_PACK_VERSION,
> +		.session = session_id
> +	};
> +
> +	/* PCI device filter */
> +	if (filter) {
> +		/* Skip non-EP devices */
> +		if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) {
> +			if (pd->dev_type != PCIE_TYPE_ENDPOINT)
> +				return OPAL_SUCCESS;
> +		}
> +		else if (pd->is_bridge)
> +			return OPAL_SUCCESS;
> +	}
> +
> +	pciinv_fill(phb, pd, &pack.device);
> +
> +	msg = ipmi_mkmsg_simple(IPMI_PCI_INVENTORY, &pack, sizeof(pack));
> +	if (!msg)
> +		return OPAL_HARDWARE;
> +
> +	/* 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.

> diff --git a/include/ipmi.h b/include/ipmi.h
> index 3389eaf5..a9999a70 100644
> --- a/include/ipmi.h
> +++ b/include/ipmi.h
> @@ -101,6 +101,7 @@
>  #define IPMI_NETFN_SE			0x04
>  #define IPMI_NETFN_STORAGE		0x0a
>  #define IPMI_NETFN_APP			0x06
> +#define IPMI_NETFN_IBM			0x3a

Hrm.. this makes me wonder if we have the ipmi_oem_partial_add_esel
incorrect for OpenBMC.

Tom, is this right for current OpenBMC?

const struct bmc_platform astbmc_openbmc = {
        .name = "OpenBMC",
        .ipmi_oem_partial_add_esel   = IPMI_CODE(0x32, 0xf0),
};

(It's distinctly possible it isn't and this explains a bug report that
we have laying around internally)

>  #define IPMI_WRITE_FRU			IPMI_CODE(IPMI_NETFN_STORAGE, 0x12)
>  #define IPMI_GET_SEL_INFO		IPMI_CODE(IPMI_NETFN_STORAGE, 0x40)
> @@ -121,6 +122,7 @@
>  #define IPMI_READ_EVENT			IPMI_CODE(IPMI_NETFN_APP, 0x35)
>  #define IPMI_GET_BT_CAPS		IPMI_CODE(IPMI_NETFN_APP, 0x36)
>  #define IPMI_SET_SENSOR_READING		IPMI_CODE(IPMI_NETFN_SE, 0x30)
> +#define IPMI_PCI_INVENTORY 		IPMI_CODE(IPMI_NETFN_IBM, 0x2a)
>  
>  /*
>   * IPMI response codes.
> @@ -275,6 +277,11 @@ uint8_t ipmi_get_sensor_number(uint8_t sensor_type);
>  /* Set the boot count once the OS is up and running */
>  int ipmi_set_boot_count(void);
>  
> +/* 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.


-- 
Stewart Smith
OPAL Architect, IBM.



More information about the Skiboot mailing list