Re: [RFC PATCH 2/4] hw/pci/pcie_doe: Introduce utility functions for PCIe DOE

[Ben Widawsky]

This was a bit more complicated than I was anticipating :-)

On 21-02-01 23:16:27, Jonathan Cameron wrote:
> This implements the ECN to the PCI 5.0 specification available at
> https://members.pcisig.com/wg/PCI-SIG/document/14143
> 
> Does not currently support interrupts.
> 
> Note that currently no attempt is made to clean up allocated memory.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/pci/meson.build       |   2 +-
>  hw/pci/pcie_doe.c        | 257 +++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/doe.h     |  40 ++++++
>  include/hw/pci/pci_ids.h |   2 +
>  4 files changed, 300 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/meson.build b/hw/pci/meson.build
> index 5c4bbac817..7336620ee3 100644
> --- a/hw/pci/meson.build
> +++ b/hw/pci/meson.build
> @@ -11,7 +11,7 @@ pci_ss.add(files(
>  # The functions in these modules can be used by devices too.  Since we
>  # allow plugging PCIe devices into PCI buses, include them even if
>  # CONFIG_PCI_EXPRESS=n.
> -pci_ss.add(files('pcie.c', 'pcie_aer.c'))
> +pci_ss.add(files('pcie.c', 'pcie_aer.c',  'pcie_doe.c'))
>  softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: files('pcie_port.c', 
> 'pcie_host.c'))
>  softmmu_ss.add_all(when: 'CONFIG_PCI', if_true: pci_ss)
>  
> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
> new file mode 100644
> index 0000000000..8739c41280
> --- /dev/null
> +++ b/hw/pci/pcie_doe.c
> @@ -0,0 +1,257 @@
> +/*
> + * pcie_doe.c
> + * utility functions for pci express data object exchange introduced
> + * in PCI 5.0 Data Object Exchange (DOE) ECN
> + *
> + * Copyright (c) 2021 Jonathan Cameron <Jonathan.Cameron@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/error-report.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/doe.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/range.h"
> +#include "qemu/rcu.h"
> +#include "sysemu/hostmem.h"
> +

I know it's RFC and quickly thrown together, but:

/* VID and Type */
#define DOE_DATA_OBJECT_HDR1 0
/* Length */
#define DOE_DATA_OBJECT_HDR2 1
/* Index */
#define DOE_DATA_OBJECT_REQUEST_DATA 2
#define DOE_DATA_OBJECT_RESPONSE_DATA 3

Then use that throughout below, please?

> +struct doe_handler {
> +    uint16_t vendor_id;
> +    uint8_t object_type;
> +    doe_msg_handler_t handler;
> +    void *priv;
> +};
> +
> +static void doe_set_ctl(PCIEDOE *doe, uint32_t val)
> +{
> +    /* Abort */
> +    if (val & PCI_DOE_CTRL_DOE_ABORT) {
> +        doe->req_index = 0;
> +        doe->rsp_index = 0;
> +        doe->req_length = 0;
> +        doe->error = false;
> +        doe->data_object_ready = false;
> +    }
> +
> +    if (val & PCI_DOE_CTRL_DOE_GO) {
> +        GList *l;
> +        uint16_t vendor_id = doe->store[0] & PCI_DATA_OBJ_DW0_VID;
> +        uint8_t object_type = (doe->store[0] & PCI_DATA_OBJ_DW0_TYPE) >>
> +            ctz32(PCI_DATA_OBJ_DW0_TYPE);

I think it'd be much nicer to read this as REG32/FIELD_EX32.

> +        if ((doe->req_index != 3) || (doe->req_length != 3)) {
> +            /*
> +             * Not entirely clear what should happen if req_length is correct
> +             * buf insufficient data has been received.

s/buf/but

Also, maybe for more resiliency and readability, with a comment about why '3':
if ((doe->req_index < 3) ...

I think it'd be much more readable if you pulled the index directly out of the
register store here instead of when you set.

len = doe->store[DOE_DATA_OBJECT_HDR2] & PCI_DATA_OBJ_DW1_LEN;
if (!len)
        len = 1<<18;

> +             */
> +            doe->error = true;
> +            return;

I don't think this should be an error:
"If the Length is shorter than expected for a specific data object, then the 
data
object must be silently discarded.

If the Length is greater than expected for a specific data object, then the
portion of the data object up to the expected length must be processednormally
and the remainder of the data object must be silently discarded."

> +        }
> +        /* Discovery protocol - DOE ECN */
> +        if (vendor_id == PCI_VENDOR_ID_PCI_SIG &&
> +            object_type == PCI_DOE_DIS_OBJ_TYPE) {
> +            uint8_t index = doe->store[2] & PCI_DOE_DIS_REQ_D0_DW0_INDEX;
> +            doe->store[1] = 3;
> +            if (index == 0) {
> +                /* First entry is this one, the discovery protocol itself */
> +                uint8_t next;
> +
> +                if (doe->cb_list) {
> +                    next = index + 1;
> +                } else {
> +                    next = 0;
> +                }

I think a comment here that you're terminating the list if no callbacks are
registered would be good.

> +                doe->store[2] =
> +                    (next << ctz32(PCI_DOE_DIS_RSP_D0_DW0_NEXT_INDEX)) |
> +                    (0 << ctz32(PCI_DOE_DIS_RSP_D0_DW0_PROT)) |
> +                    0x0001;

Same comment about FIELD_DP|EX32. I'd try it and see how it looks.

Would probably be nice to #define some vendor ID, or use fields so it's obvious
why you have a 0x1.

> +            } else {
> +                /* Other entries based on register callbacks */
> +                uint8_t next;
> +                struct doe_handler *h;
> +
> +                h = g_list_nth_data(doe->cb_list, index - 1);

It wasn't immediately obvious to me why index - 1. Maybe it'd be a little nicer
if you had a helper:

static inline struct doe_handler *doe_get_handler(PCIEDOE *doe, int doe_index)
{
        struct doe_handler *ret = NULL;

        // assert(doe->cb_list);
        if (doe->cb_list)
                ret = g_list_nth_data(doe->cb_list, doe_index - 1);

        return ret;
}

> +                /*
> +                 * Off end of list, Table 7-x4 in DOE ECN -
> +                 * Vendor ID 0xFFFF if no more indices
> +                 */
> +                if (h == NULL) {
> +                    doe->store[2] = 0xFFFF;
> +                } else {
> +                    if (g_list_nth(doe->cb_list, index)) {
> +                        next = index + 1;
> +                    } else {
> +                        next = 0;
> +                    }

A bit confusing here as well IMO, but I don't know a better way to write it
other than adding comments about index effectively being off by 1.

> +                    doe->store[2] =
> +                        (next << ctz32(PCI_DOE_DIS_RSP_D0_DW0_NEXT_INDEX)) |
> +                        (h->object_type << 
> ctz32(PCI_DOE_DIS_RSP_D0_DW0_PROT)) |
> +                        h->vendor_id;
> +                }
> +            }
> +            doe->data_object_ready = true;
> +            doe->rsp_index = 0;
> +        } else {
> +            for (l = doe->cb_list; l != NULL; l = l->next) {
> +                struct doe_handler *h = l->data;
> +                if (h->vendor_id == vendor_id &&
> +                    h->object_type == object_type) {
> +                    int ret = h->handler(doe, h->vendor_id, h->object_type,
> +                                         h->priv);
> +                    if (ret) {
> +                        /*
> +                         * No response so as per 6.xx.1 in DOE ECN
> +                         * "... within 1 second after the DOE Go bit was Set
> +                         *  in the DOE Control register, otherwise the DOE
> +                         *  instance must Set the DOE Error bit in the DOE
> +                         *  Status register.."
> +                         */
> +                         doe->error = true;

extra whitespace

> +                        break;
> +                    }
> +                    doe->data_object_ready = true;
> +                    doe->rsp_index = 0;
> +                    break;
> +                }
> +            }
> +            /* Comamnd not handled */
> +            if (l == NULL) {
> +                doe->error = true;
> +            }
> +        }
> +        /* Reset input index to allow for a new message */
> +        doe->req_index = 0;
> +    }
> +}
> +
> +static void doe_set_write_mailbox(PCIEDOE *doe, uint32_t val)
> +{
> +    if (doe->req_index == 1) {
> +        if (val & 0x3FFFF) {

val & PCI_DATA_OBJ_DW1_LEN

> +            doe->req_length = val & PCI_DATA_OBJ_DW1_LEN;
> +        } else {
> +            doe->req_length = 1 << 18;
> +        }
> +    }
> +    if (doe->req_length && doe->req_index == doe->req_length) {
> +        /*
> +         * 6.xx.1 Data Objects
> +         * If the DW transferred does not match the indicated Length
> +         * for a data object, then the data object must be
> +         * silently discarded
> +         */
> +        return;
> +    }
> +    doe->store[doe->req_index] = val;
> +    doe->req_index++;
> +}
> +
> +static uint32_t doe_get_read_mailbox(PCIEDOE *doe)
> +{
> +    uint32_t val;
> +
> +    if (doe->rsp_index == 0) {
> +        doe->rsp_length = doe->store[1] & PCI_DATA_OBJ_DW1_LEN;
> +    }
> +    if (!doe->data_object_ready) {
> +        /* Underflow of the Read Data Mailbox Mechanism */
> +        doe->error = true;
> +        return 0;
> +    }

I don't think this should be an error condition. Could you please explain?

> +
> +    val = doe->store[doe->rsp_index];
> +    doe->rsp_index++;
> +    if (doe->rsp_index == doe->rsp_length) {
> +        doe->rsp_index = -1;
> +        doe->data_object_ready = false;
> +    }
> +
> +    return val;
> +}
> +
> +static uint32_t doe_get_status(PCIEDOE *doe)
> +{
> +    uint32_t val = 0;
> +
> +    if (doe->busy) {
> +        val |= PCI_DOE_STATUS_DOE_BUSY;
> +    }

Do you actually intend to model busy?

> +    /* bit 1: interrupt not yet supported */
> +    if (doe->error) {
> +        val |= PCI_DOE_STATUS_DOE_ERROR;
> +    }
> +    if (doe->data_object_ready) {
> +        val |= PCI_DOE_STATUS_DATA_OBJECT_READY;
> +    }
> +
> +    return val;
> +}
> +
> +void doe_add_message_handler(PCIEDOE *doe, uint16_t vendor_id,
> +                             uint8_t object_type,
> +                             const doe_msg_handler_t handler, void *priv)
> +{
> +    struct doe_handler *h = g_malloc0(sizeof(*handler));
> +
> +    h->vendor_id = vendor_id;
> +    h->object_type = object_type;
> +    h->handler = handler;
> +    h->priv = priv;
> +    doe->cb_list = g_list_append(doe->cb_list, h);
> +}
> +
> +uint32_t pcie_doe_ecap(PCIEDOE *doe, PCIDevice *d, uint16_t offset)
> +{
> +    doe->doe_base = offset;
> +    /* Length field is 18 bits and is in dwords */
> +    doe->store = g_malloc0((1 << 18) * sizeof(uint32_t));

I think it's fine if you're instantiating DOE to just have this be statically
allocated.

> +
> +    pcie_add_capability(d, PCI_EXT_CAP_ID_DOE, 1, offset, 0x18);
> +    offset += 0x18;
> +
> +    return offset;
> +}
> +
> +void pcie_doe_write(PCIEDOE *doe, uint32_t addr, uint32_t val, int len)
> +{
> +    if (len != 4) {
> +        return;
> +    } 

Do you want to check alignment also?

> +
> +    switch (addr - doe->doe_base) {
> +    case PCI_DOE_CTRL:
> +        doe_set_ctl(doe, val);
> +        break;
> +    case PCI_DOE_WRITE_MAILBOX:
> +        doe_set_write_mailbox(doe, val);
> +        break;
> +    }
> +}
> +
> +uint32_t pcie_doe_read(PCIEDOE *doe, uint32_t addr, int len, int *found)
> +{
> +    if (len != 4) {
> +        *found = 0;
> +        return 0;
> +    }

Same comment as _write

> +
> +    *found = 1;
> +    switch (addr - doe->doe_base) {
> +    case PCI_DOE_CAP:
> +        return 0; /* No interrupt support */
> +    case PCI_DOE_STATUS:
> +        return doe_get_status(doe);
> +    case PCI_DOE_READ_MAILBOX:
> +        return doe_get_read_mailbox(doe);
> +    default:
> +        *found = 0;
> +        return 0;
> +    }
> +}

I'm guessing you don't love 'found'. Perhaps you can have this return an int64_t
and negative values are errors?

I'm not yet convinced you even need that level of error handling though. I
suppose next patch will tell me.

> +
> diff --git a/include/hw/pci/doe.h b/include/hw/pci/doe.h
> new file mode 100644
> index 0000000000..364c866c53
> --- /dev/null
> +++ b/include/hw/pci/doe.h
> @@ -0,0 +1,40 @@
> +/*
> + * PCIE DOE emulation.
> + *
> + * Copyright (c) 2021 Jonathan Cameron <Jonathan.Cameron@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_PCIE_DOE_H_
> +#define QEMU_PCIE_DOE_H_
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +
> +typedef struct pcie_doe {
> +    uint32_t doe_base;
> +    GList *cb_list;
> +    int req_index;
> +    int req_length;
> +    int rsp_index;
> +    int rsp_length;
> +    bool data_object_ready;
> +    bool error;
> +    bool busy;
> +    uint32_t *store;
> +} PCIEDOE;
> +
> +typedef int (*doe_msg_handler_t)(PCIEDOE *doe, uint16_t vendor_id,
> +                                 uint8_t object_type, void *priv);
> +
> +uint32_t pcie_doe_ecap(PCIEDOE *doe, PCIDevice *d, uint16_t offset);
> +void doe_add_message_handler(PCIEDOE *doe, uint16_t vendor_id,
> +                             uint8_t object_type,
> +                             const doe_msg_handler_t handler, void *priv);
> +uint32_t pcie_doe_read(PCIEDOE *doe, uint32_t addr, int len, int *found);
> +void pcie_doe_write(PCIEDOE *doe, uint32_t addr, uint32_t val, int len);
> +#endif
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 76bf3ed590..636b2e8017 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -157,6 +157,8 @@
>  
>  /* Vendors and devices.  Sort key: vendor first, device next. */
>  
> +#define PCI_VENDOR_ID_PCI_SIG            0x0001
> +
>  #define PCI_VENDOR_ID_LSI_LOGIC          0x1000
>  #define PCI_DEVICE_ID_LSI_53C810         0x0001
>  #define PCI_DEVICE_ID_LSI_53C895A        0x0012
> -- 
> 2.19.1
> 
>