On Thu, 27 Jan 2022 11:55:47 +0000
Alex Bennée <alex.ben...@linaro.org> wrote:
> Jonathan Cameron <jonathan.came...@huawei.com> writes:
>
> > From: Ben Widawsky <ben.widaw...@intel.com>
> >
> > CXL specification provides for the ability to obtain logs from the
> > device. Logs are either spec defined, like the "Command Effects Log"
> > (CEL), or vendor specific. UUIDs are defined for all log types.
> >
> > The CEL is a mechanism to provide information to the host about which
> > commands are supported. It is useful both to determine which spec'd
> > optional commands are supported, as well as provide a list of vendor
> > specified commands that might be used. The CEL is already created as
> > part of mailbox initialization, but here it is now exported to hosts
> > that use these log commands.
> >
> > Signed-off-by: Ben Widawsky <ben.widaw...@intel.com>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 67 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index cea4b2a59c..0ab0592e6c 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -46,6 +46,9 @@ enum {
> > TIMESTAMP = 0x03,
> > #define GET 0x0
> > #define SET 0x1
> > + LOGS = 0x04,
> > + #define GET_SUPPORTED 0x0
> > + #define GET_LOG 0x1
> > };
> >
> > /* 8.2.8.4.5.1 Command Return Codes */
> > @@ -122,6 +125,8 @@
> > define_mailbox_handler_zeroed(EVENTS_GET_INTERRUPT_POLICY, 4);
> > define_mailbox_handler_nop(EVENTS_SET_INTERRUPT_POLICY);
> > declare_mailbox_handler(TIMESTAMP_GET);
> > declare_mailbox_handler(TIMESTAMP_SET);
> > +declare_mailbox_handler(LOGS_GET_SUPPORTED);
> > +declare_mailbox_handler(LOGS_GET_LOG);
> >
> > #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> > #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> > @@ -137,6 +142,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> > CXL_CMD(EVENTS, SET_INTERRUPT_POLICY, 4, IMMEDIATE_CONFIG_CHANGE),
> > CXL_CMD(TIMESTAMP, GET, 0, 0),
> > CXL_CMD(TIMESTAMP, SET, 8, IMMEDIATE_POLICY_CHANGE),
> > + CXL_CMD(LOGS, GET_SUPPORTED, 0, 0),
> > + CXL_CMD(LOGS, GET_LOG, 0x18, 0),
> > };
> >
> > #undef CXL_CMD
> > @@ -188,6 +195,66 @@ define_mailbox_handler(TIMESTAMP_SET)
> >
> > QemuUUID cel_uuid;
> >
> > +/* 8.2.9.4.1 */
> > +define_mailbox_handler(LOGS_GET_SUPPORTED)
> > +{
>
> Here is where I get a bit wary of the define_mailbox_handler define
> which from what I can tell just hides the declarations. This makes the
> handling of things like *cmd rather opaque. There is an argument for the
> boilerplate definitions (_nop and _zeroed) but perhaps not these.
Agreed. I think these macros got a bit too clever.
I debated keeping the CXL_CMD one but that then forces us to have
ugly mixed lower case and upper case function names, so I've dropped that
as well.
I'm debating whether to go with
[EVENTS][GET] = ...
[EVENTS][SET] = ...
vs
[EVENTS] = {
[GET] = { ..
[SET] = { ..
},
For now I'll go with the [][] variant. The other one may make more
sense as we add more commands.
Reorganizing the code a little gets rid of the need for the forward
declarations as well so end result is less code than with the macros
even if a few corners are a little repetitive.
Thanks,
Jonathan
>
> > + struct {
> > + uint16_t entries;
> > + uint8_t rsvd[6];
> > + struct {
> > + QemuUUID uuid;
> > + uint32_t size;
> > + } log_entries[1];
> > + } __attribute__((packed)) *supported_logs = (void *)cmd->payload;
> > + _Static_assert(sizeof(*supported_logs) == 0x1c, "Bad supported log
> > size");
> > +
> > + supported_logs->entries = 1;
> > + supported_logs->log_entries[0].uuid = cel_uuid;
> > + supported_logs->log_entries[0].size = 4 * cxl_dstate->cel_size;
> > +
> > + *len = sizeof(*supported_logs);
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > +/* 8.2.9.4.2 */
> > +define_mailbox_handler(LOGS_GET_LOG)
> > +{
> > + struct {
> > + QemuUUID uuid;
> > + uint32_t offset;
> > + uint32_t length;
> > + } __attribute__((packed, __aligned__(16))) *get_log = (void
> > *)cmd->payload;
> > +
> > + /*
> > + * 8.2.9.4.2
> > + * The device shall return Invalid Parameter if the Offset or Length
> > + * fields attempt to access beyond the size of the log as reported
> > by Get
> > + * Supported Logs.
> > + *
> > + * XXX: Spec is wrong, "Invalid Parameter" isn't a thing.
> > + * XXX: Spec doesn't address incorrect UUID incorrectness.
> > + *
> > + * The CEL buffer is large enough to fit all commands in the
> > emulation, so
> > + * the only possible failure would be if the mailbox itself isn't big
> > + * enough.
> > + */
> > + if (get_log->offset + get_log->length > cxl_dstate->payload_size) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
> > + return CXL_MBOX_UNSUPPORTED;
> > + }
> > +
> > + /* Store off everything to local variables so we can wipe out the
> > payload */
> > + *len = get_log->length;
> > +
> > + memmove(cmd->payload, cxl_dstate->cel_log + get_log->offset,
> > + get_log->length);
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> > {
> > uint16_t ret = CXL_MBOX_SUCCESS;
>
>
