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

[Jonathan Cameron]

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;                                          \
> +    }
> +