On Mon, 10 Oct 2022 15:29:44 -0700 ira.we...@intel.com wrote: > From: Ira Weiny <ira.we...@intel.com> > > Replace the stubbed out CXL Get/Set Event interrupt policy mailbox > commands. Enable those commands to control interrupts for each of the > event log types. > > Signed-off-by: Ira Weiny <ira.we...@intel.com> A few trivial comments inline.
Thanks, Jonathan > --- > hw/cxl/cxl-mailbox-utils.c | 129 ++++++++++++++++++++++++++++++------ > include/hw/cxl/cxl_events.h | 21 ++++++ > 2 files changed, 129 insertions(+), 21 deletions(-) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index df345f23a30c..52e8804c24ed 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -101,25 +101,6 @@ struct cxl_cmd { > uint8_t *payload; > }; > > -#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; \ > - memset(cmd->payload, 0, *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; \ > - } > - > -DEFINE_MAILBOX_HANDLER_ZEROED(events_get_interrupt_policy, 4); > -DEFINE_MAILBOX_HANDLER_NOP(events_set_interrupt_policy); > - > static ret_code cmd_events_get_records(struct cxl_cmd *cmd, > CXLDeviceState *cxlds, > uint16_t *len) > @@ -218,6 +199,110 @@ static ret_code cmd_events_clear_records(struct cxl_cmd > *cmd, > return CXL_MBOX_SUCCESS; > } > > +static ret_code cmd_events_get_interrupt_policy(struct cxl_cmd *cmd, > + CXLDeviceState *cxl_dstate, > + uint16_t *len) > +{ > + struct cxl_event_interrupt_policy *policy; > + struct cxl_event_log *log; > + > + policy = (struct cxl_event_interrupt_policy *)cmd->payload; > + memset(policy, 0, sizeof(*policy)); > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_INFO); Less obvious than below case, but again, perhaps a little utility function to cut down on duplication. > + if (log->irq_enabled) { > + policy->info_settings = CXL_EVENT_INT_SETTING(log->irq_vec); > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_WARN); > + if (log->irq_enabled) { > + policy->warn_settings = CXL_EVENT_INT_SETTING(log->irq_vec); > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_FAIL); > + if (log->irq_enabled) { > + policy->failure_settings = CXL_EVENT_INT_SETTING(log->irq_vec); > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_FATAL); > + if (log->irq_enabled) { > + policy->fatal_settings = CXL_EVENT_INT_SETTING(log->irq_vec); > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_DYNAMIC_CAP); > + if (log->irq_enabled) { > + /* Dynamic Capacity borrows the same vector as info */ > + policy->dyn_cap_settings = CXL_INT_MSI_MSIX; > + } > + > + *len = sizeof(*policy); > + return CXL_MBOX_SUCCESS; > +} > + > +static ret_code cmd_events_set_interrupt_policy(struct cxl_cmd *cmd, > + CXLDeviceState *cxl_dstate, > + uint16_t *len) > +{ > + struct cxl_event_interrupt_policy *policy; > + struct cxl_event_log *log; > + > + policy = (struct cxl_event_interrupt_policy *)cmd->payload; > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_INFO); Maybe a utility function? set_int_policy(cxl_dstate, CXL_EVENT_TYPE_INFO, policy->info_settings); set_int_policy(cxl_dstate, CXL_EVENT_TYPE_WARN, policy->warn_settings); etc > + if ((policy->info_settings & CXL_EVENT_INT_MODE_MASK) == > + CXL_INT_MSI_MSIX) { > + log->irq_enabled = true; > + log->irq_vec = cxl_dstate->event_vector[CXL_EVENT_TYPE_INFO]; > + } else { > + log->irq_enabled = false; > + log->irq_vec = 0; > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_WARN); > + if ((policy->warn_settings & CXL_EVENT_INT_MODE_MASK) == > + CXL_INT_MSI_MSIX) { > + log->irq_enabled = true; > + log->irq_vec = cxl_dstate->event_vector[CXL_EVENT_TYPE_WARN]; > + } else { > + log->irq_enabled = false; > + log->irq_vec = 0; > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_FAIL); > + if ((policy->failure_settings & CXL_EVENT_INT_MODE_MASK) == > + CXL_INT_MSI_MSIX) { > + log->irq_enabled = true; > + log->irq_vec = cxl_dstate->event_vector[CXL_EVENT_TYPE_FAIL]; > + } else { > + log->irq_enabled = false; > + log->irq_vec = 0; > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_FATAL); > + if ((policy->fatal_settings & CXL_EVENT_INT_MODE_MASK) == > + CXL_INT_MSI_MSIX) { > + log->irq_enabled = true; > + log->irq_vec = cxl_dstate->event_vector[CXL_EVENT_TYPE_FATAL]; > + } else { > + log->irq_enabled = false; > + log->irq_vec = 0; > + } > + > + log = find_event_log(cxl_dstate, CXL_EVENT_TYPE_DYNAMIC_CAP); > + if ((policy->dyn_cap_settings & CXL_EVENT_INT_MODE_MASK) == > + CXL_INT_MSI_MSIX) { > + log->irq_enabled = true; > + /* Dynamic Capacity borrows the same vector as info */ > + log->irq_vec = cxl_dstate->event_vector[CXL_EVENT_TYPE_INFO]; > + } else { > + log->irq_enabled = false; > + log->irq_vec = 0; > + } > + > + *len = sizeof(*policy); > + return CXL_MBOX_SUCCESS; > +} > + ... > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h > index 255111f3dcfb..c121e504a6db 100644 > --- a/include/hw/cxl/cxl_events.h > +++ b/include/hw/cxl/cxl_events.h > @@ -170,4 +170,25 @@ struct cxl_event_mem_module { > uint8_t reserved[CXL_EVENT_MEM_MOD_RES_SIZE]; > } QEMU_PACKED; > > +/** > + * Event Interrupt Policy > + * > + * CXL rev 3.0 section 8.2.9.2.4; Table 8-52 > + */ > +enum cxl_event_int_mode { > + CXL_INT_NONE = 0x00, > + CXL_INT_MSI_MSIX = 0x01, > + CXL_INT_FW = 0x02, I guess at somepoint we'll probably want to wire up the INT_FW path. Job for another day though! > + CXL_INT_RES = 0x03, Why define the reserved value here? By definition we won't use it. > +}; > +#define CXL_EVENT_INT_MODE_MASK 0x3 > +#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | > CXL_INT_MSI_MSIX) I probably haven't had enough caffeine yet today, but why the cast given you are masking to a smaller range? > +struct cxl_event_interrupt_policy { > + uint8_t info_settings; Can we shorten these to just info, warn, failure, fatal, dyn_cap? No real loss I think and will help with some of the long lines above. > + uint8_t warn_settings; > + uint8_t failure_settings; > + uint8_t fatal_settings; > + uint8_t dyn_cap_settings; > +} QEMU_PACKED; > + > #endif /* CXL_EVENTS_H */