Re: [RFC PATCH v3 05/31] hw/cxl/device: Implement basic mailbox (8.2.8.4)

Jonathan Cameron
2021-02-11T18:24:33Z

On Mon, 1 Feb 2021 16:59:22 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> This is the beginning of implementing mailbox support for CXL 2.0
> devices. The implementation recognizes when the doorbell is rung,
> handles the command/payload, clears the doorbell while returning error
> codes and data.
>
> Generally the mailbox mechanism is designed to permit communication
> between the host OS and the firmware running on the device. For our
> purposes, we emulate both the firmware, implemented primarily in
> cxl-mailbox-utils.c, and the hardware.
>
> No commands are implemented yet.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Sorry review is a little incoherent. It's a lot of patches
so I've ended up looking at your tree then trying to figure out
which patch a given comment belongs alongside.

> ---
> hw/cxl/cxl-device-utils.c | 125 ++++++++++++++++++++++-
> hw/cxl/cxl-mailbox-utils.c | 197 ++++++++++++++++++++++++++++++++++++
> hw/cxl/meson.build | 1 +
> include/hw/cxl/cxl.h | 3 +
> include/hw/cxl/cxl_device.h | 28 ++++-
> 5 files changed, 349 insertions(+), 5 deletions(-)
> create mode 100644 hw/cxl/cxl-mailbox-utils.c
>
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index bb15ad9a0f..6602606f3d 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -40,6 +40,111 @@ static uint64_t dev_reg_read(void *opaque, hwaddr offset, unsigned size)
> return 0;
> }
>

> +
> +#define define_mailbox_handler_zeroed(name, size) \
> + uint16_t __zero##name = size; \
> + static ret_code cmd_##name(struct cxl_cmd *cmd, \
> + CXLDeviceState *cxl_dstate, uint16_t *len) \
> + { \
> + *len = __zero##name; \

Why not just use the value of size here?

__zero##name isn't used anywhere else.

> + memset(cmd->payload, 0, *len); \
> + return CXL_MBOX_SUCCESS; \
> + }
> +#define define_mailbox_handler_const(name, data) \
> + static ret_code cmd_##name(struct cxl_cmd *cmd, \
> + CXLDeviceState *cxl_dstate, uint16_t *len) \
> + { \
> + *len = sizeof(data); \
> + memcpy(cmd->payload, data, *len); \
> + return CXL_MBOX_SUCCESS; \
> + }
> +#define define_mailbox_handler_nop(name) \
> + static ret_code cmd_##name(struct cxl_cmd *cmd, \
> + CXLDeviceState *cxl_dstate, uint16_t *len) \
> + { \
> + return CXL_MBOX_SUCCESS; \
> + }
> +