Re: [RFC PATCH 3/3] hw/peci: add support for EndPointConfig reads

[Peter Delevoryas <peter@pjd.dev>]

On Tue, Sep 06, 2022 at 10:05:52PM +0000, Titus Rwantare wrote:
> Signed-off-by: Titus Rwantare <titusr@google.com>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> ---
>  hw/peci/peci-client.c  | 63 ++++++++++++++++++++++++++++++++++++++++++
>  hw/peci/peci-core.c    | 44 +++++++++++++++++++++++++++--
>  include/hw/peci/peci.h | 23 +++++++++++++++
>  3 files changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/peci/peci-client.c b/hw/peci/peci-client.c
> index 2aa797b5f6..8d93333248 100644
> --- a/hw/peci/peci-client.c
> +++ b/hw/peci/peci-client.c
> @@ -23,6 +23,64 @@
>  
>  #define PECI_CLIENT_DEFAULT_TEMP 30
>  
> +/* TODO: move this out into a config */
> +static const PECIEndPtConfig spr_config[] = {
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 0,
> +        .hdr.func = 2,
> +        .hdr.reg = 0xD4,
> +        .data = BIT(31)
> +    },
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 0,
> +        .hdr.func = 2,
> +        .hdr.reg = 0xD0,
> +        .data = BIT(31) | BIT(30)
> +    },
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 30,
> +        .hdr.func = 6,
> +        .hdr.reg = 0x84,
> +        .data = 0x03FFFFFF
> +    },
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 30,
> +        .hdr.func = 6,
> +        .hdr.reg = 0x80,
> +        .data = 0xFFFFFFFF
> +    },
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 30,
> +        .hdr.func = 6,
> +        .hdr.reg = 0x84,
> +        .data = 0x03FFFFFF
> +    },
> +    {
> +        .hdr.msg_type = LOCAL_PCI_CFG,
> +        .hdr.addr_type = 0x4,
> +        .hdr.bus = 31,
> +        .hdr.dev = 30,
> +        .hdr.func = 6,
> +        .hdr.reg = 0x80,
> +        .data = 0xFFFFFFFF
> +    },
> +};
> +
>  static void peci_client_update_temps(PECIClientDevice *client)
>  {
>      uint8_t temp_cpu = 0;
> @@ -115,7 +173,12 @@ PECIClientDevice *peci_add_client(PECIBus *bus,
>          break;
>  
>      case FAM6_ICELAKE_X:
> +        client->revision = 0x40;
> +        break;
> +
>      case FAM6_SAPPHIRE_RAPIDS_X:
> +        client->endpt_conf = spr_config;
> +        client->num_entries = sizeof(spr_config) / sizeof(spr_config[0]);
>          client->revision = 0x40;
>          client->ucode = 0x8c0004a0;
>          break;
> diff --git a/hw/peci/peci-core.c b/hw/peci/peci-core.c
> index 8210bfa198..a961ae51f3 100644
> --- a/hw/peci/peci-core.c
> +++ b/hw/peci/peci-core.c
> @@ -22,6 +22,47 @@
>  #define PECI_FCS_OK         0
>  #define PECI_FCS_ERR        1
>  
> +static PECIEndPtHeader peci_fmt_end_pt_header(PECICmd *pcmd)
> +{
> +    uint32_t val = pcmd->rx[7] | (pcmd->rx[8] << 8) | (pcmd->rx[9] << 16) |
> +                  (pcmd->rx[10] << 24);
> +
> +    PECIEndPtHeader header = {
> +        .msg_type = pcmd->rx[1],
> +        .addr_type = pcmd->rx[5],
> +        .bus = (val >> 20) & 0xFF,
> +        .dev = (val >> 15) & 0x1F,
> +        .func = (val >> 12) & 0x7,
> +        .reg = val & 0xFFF,
> +    };
> +
> +    return header;
> +}
> +
> +static void peci_rd_endpt_cfg(PECIClientDevice *client, PECICmd *pcmd)
> +{
> +    PECIPkgCfg *resp = (PECIPkgCfg *)pcmd->tx;
> +    PECIEndPtHeader req = peci_fmt_end_pt_header(pcmd);
> +    PECIEndPtConfig const *c;
> +
> +    if (client->endpt_conf) {
> +        for (size_t i = 0; i < client->num_entries; i++) {
> +            c = &client->endpt_conf[i];
> +
> +            if (!memcmp(&req, &c->hdr, sizeof(PECIEndPtHeader))) {
> +                    resp->data = c->data;
> +                    resp->cc = PECI_DEV_CC_SUCCESS;
> +                    return;
> +            }
> +        }
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: msg_type: 0x%x bus: %u, dev: %u, func: %u, reg: 0x%x\n",
> +                  __func__, req.msg_type, req.bus, req.dev, req.func, req.reg);
> +
> +}
> +
>  static void peci_rd_pkg_cfg(PECIClientDevice *client, PECICmd *pcmd)
>  {
>      PECIPkgCfg *resp = (PECIPkgCfg *)pcmd->tx;
> @@ -153,8 +194,7 @@ int peci_handle_cmd(PECIBus *bus, PECICmd *pcmd)
>          break;
>  
>      case PECI_CMD_RD_END_PT_CFG:
> -        qemu_log_mask(LOG_UNIMP, "%s: unimplemented CMD_RD_END_PT_CFG\n",
> -                      __func__);
> +        peci_rd_endpt_cfg(client, pcmd);
>          break;
>  
>      default:
> diff --git a/include/hw/peci/peci.h b/include/hw/peci/peci.h
> index 1a0abe65cd..4fb2fc236e 100644
> --- a/include/hw/peci/peci.h
> +++ b/include/hw/peci/peci.h
> @@ -112,6 +112,26 @@ typedef struct PECITempTarget {
>      uint8_t tjmax;
>  } PECITempTarget;
>  
> +typedef enum PECIEndPtType {
> +    LOCAL_PCI_CFG = 3,
> +    PCI_CFG,
> +    MMIO_BDF,
> +} PECIEndPtType;
> +
> +typedef struct __attribute__ ((__packed__)) {
> +    PECIEndPtType msg_type;
> +    uint8_t addr_type;
> +    uint8_t bus;
> +    uint8_t dev;
> +    uint8_t func;
> +    uint16_t reg;
> +} PECIEndPtHeader;
> +
> +typedef struct {
> +    PECIEndPtHeader hdr;
> +    uint32_t data;
> +} PECIEndPtConfig;

I noticed the summary is "hw/peci: add support for EndPointConfig reads"
but type definitions here use "EndPt", maybe they should be
"PECIEndPoint*"? I don't think extending to Point is too long.

> +
>  #define PECI_BASE_ADDR              0x30
>  #define PECI_BUFFER_SIZE            0x100
>  #define PECI_NUM_CPUS_MAX           56
> @@ -140,6 +160,9 @@ typedef struct PECIClientDevice {
>      uint8_t dimm_temp_max;
>      uint8_t dimm_temp[PECI_NUM_DIMMS_MAX];
>  
> +    /* EndPtConfig info */
> +    PECIEndPtConfig const *endpt_conf;
> +    size_t num_entries;
>  } PECIClientDevice;
>  
>  #define TYPE_PECI_CLIENT "peci-client"
> -- 
> 2.37.2.789.g6183377224-goog
>