On Tue, 13 Aug 2024 15:12:55 -0700 Davidlohr Bueso <d...@stgolabs.net> wrote:
> As of 3.1 spec, background commands can be canceled with a new > abort command. Implement the support, which is advertised in > the CEL. No ad-hoc context undoing is necessary as all the > command logic of the running bg command is done upon completion. > Arbitrarily, the on-going background cmd will not be aborted if > already at least 85% done; > > A mutex is introduced to stabilize mbox request cancel command vs > the timer callback being fired scenarios (as well as reading the > mbox registers). While some operations under critical regions > may be unnecessary (irq notifying, cmd callbacks), this is not > a path where performance is important, so simplicity is preferred. > > Tested-by: Ajay Joshi <ajay.open...@micron.com> > Reviewed-by: Ajay Joshi <ajay.open...@micron.com> > Signed-off-by: Davidlohr Bueso <d...@stgolabs.net> +CC qemu-devel No comments inline and LGTM. I'll queue it on my tree and push that out on gitlab sometime soonish. J > --- > > Changes based on the following thread: > https://lore.kernel.org/linux-cxl/20240729102010.20996-1-ajay.open...@micron.com > > - Added a mutex (and expanded CCI to have a destroy counterpart). > An 'initialized' flag is added for correctly handling the reset() > case. > - Added the case where cancel is not done. > - Picked up Ajay's tags but it would be good to re-review/test if > possible. > > hw/cxl/cxl-device-utils.c | 5 ++- > hw/cxl/cxl-mailbox-utils.c | 63 +++++++++++++++++++++++++++++++++--- > hw/mem/cxl_type3.c | 8 ++++- > include/hw/cxl/cxl_device.h | 4 +++ > include/hw/cxl/cxl_mailbox.h | 1 + > 5 files changed, 74 insertions(+), 7 deletions(-) > > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c > index 035d034f6dd8..1a9779ed8201 100644 > --- a/hw/cxl/cxl-device-utils.c > +++ b/hw/cxl/cxl-device-utils.c > @@ -95,11 +95,14 @@ static uint64_t mailbox_reg_read(void *opaque, hwaddr > offset, unsigned size) > } > if (offset == A_CXL_DEV_MAILBOX_STS) { > uint64_t status_reg = cxl_dstate->mbox_reg_state64[offset / > size]; > - if (cci->bg.complete_pct) { > + > + qemu_mutex_lock(&cci->bg.lock); > + if (cci->bg.complete_pct == 100 || cci->bg.aborted) { > status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, > BG_OP, > 0); > cxl_dstate->mbox_reg_state64[offset / size] = status_reg; > } > + qemu_mutex_unlock(&cci->bg.lock); > } > return cxl_dstate->mbox_reg_state64[offset / size]; > default: > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index b752920ec88a..ff12dfc3dcc4 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -56,6 +56,7 @@ enum { > INFOSTAT = 0x00, > #define IS_IDENTIFY 0x1 > #define BACKGROUND_OPERATION_STATUS 0x2 > + #define BACKGROUND_OPERATION_ABORT 0x5 > EVENTS = 0x01, > #define GET_RECORDS 0x0 > #define CLEAR_RECORDS 0x1 > @@ -624,6 +625,38 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct > cxl_cmd *cmd, > return CXL_MBOX_SUCCESS; > } > > +/* CXL r3.1 Section 8.2.9.1.5: Request Abort Background Operation (Opcode > 0005h) */ > +static CXLRetCode cmd_infostat_bg_op_abort(const struct cxl_cmd *cmd, > + uint8_t *payload_in, > + size_t len_in, > + uint8_t *payload_out, > + size_t *len_out, > + CXLCCI *cci) > +{ > + int bg_set = cci->bg.opcode >> 8; > + int bg_cmd = cci->bg.opcode & 0xff; > + const struct cxl_cmd *bg_c = &cci->cxl_cmd_set[bg_set][bg_cmd]; > + > + if (!(bg_c->effect & CXL_MBOX_BACKGROUND_OPERATION_ABORT)) { > + return CXL_MBOX_REQUEST_ABORT_NOTSUP; > + } > + > + qemu_mutex_lock(&cci->bg.lock); > + if (cci->bg.runtime) { > + /* operation is near complete, let it finish */ > + if (cci->bg.complete_pct < 85) { > + timer_del(cci->bg.timer); > + cci->bg.ret_code = CXL_MBOX_ABORTED; > + cci->bg.starttime = 0; > + cci->bg.runtime = 0; > + cci->bg.aborted = true; > + } > + } > + qemu_mutex_unlock(&cci->bg.lock); > + > + return CXL_MBOX_SUCCESS; > +} > + > #define CXL_FW_SLOTS 2 > #define CXL_FW_SIZE 0x02000000 /* 32 mb */ > > @@ -2665,6 +2698,8 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct > cxl_cmd *cmd, > } > > static const struct cxl_cmd cxl_cmd_set[256][256] = { > + [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT", > + cmd_infostat_bg_op_abort, 0, 0 }, > [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS", > cmd_events_get_records, 1, 0 }, > [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS", > @@ -2708,7 +2743,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { > [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, > 0, > (CXL_MBOX_IMMEDIATE_DATA_CHANGE | > CXL_MBOX_SECURITY_STATE_CHANGE | > - CXL_MBOX_BACKGROUND_OPERATION)}, > + CXL_MBOX_BACKGROUND_OPERATION | > + CXL_MBOX_BACKGROUND_OPERATION_ABORT)}, > [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE", > cmd_get_security_state, 0, 0 }, > [MEDIA_AND_POISON][GET_POISON_LIST] = { > "MEDIA_AND_POISON_GET_POISON_LIST", > @@ -2721,7 +2757,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { > "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES", > cmd_media_get_scan_media_capabilities, 16, 0 }, > [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA", > - cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION }, > + cmd_media_scan_media, 17, > + (CXL_MBOX_BACKGROUND_OPERATION | > CXL_MBOX_BACKGROUND_OPERATION_ABORT)}, > [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { > "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS", > cmd_media_get_scan_media_results, 0, 0 }, > @@ -2745,6 +2782,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = { > [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 }, > [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { > "BACKGROUND_OPERATION_STATUS", > cmd_infostat_bg_op_sts, 0, 0 }, > + [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT", > + cmd_infostat_bg_op_abort, 0, 0 }, > [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 }, > [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, > CXL_MBOX_IMMEDIATE_POLICY_CHANGE }, > @@ -2831,6 +2870,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, > uint8_t cmd, > cci->bg.opcode = (set << 8) | cmd; > > cci->bg.complete_pct = 0; > + cci->bg.aborted = false; > cci->bg.ret_code = 0; > > now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > @@ -2844,10 +2884,12 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, > uint8_t cmd, > static void bg_timercb(void *opaque) > { > CXLCCI *cci = opaque; > - uint64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > - uint64_t total_time = cci->bg.starttime + cci->bg.runtime; > + uint64_t now, total_time; > + > + qemu_mutex_lock(&cci->bg.lock); > > - assert(cci->bg.runtime > 0); > + now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > + total_time = cci->bg.starttime + cci->bg.runtime; > > if (now >= total_time) { /* we are done */ > uint16_t ret = CXL_MBOX_SUCCESS; > @@ -2899,6 +2941,8 @@ static void bg_timercb(void *opaque) > msi_notify(pdev, cxl_dstate->mbox_msi_n); > } > } > + > + qemu_mutex_unlock(&cci->bg.lock); > } > > static void cxl_rebuild_cel(CXLCCI *cci) > @@ -2927,12 +2971,21 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max) > cci->bg.complete_pct = 0; > cci->bg.starttime = 0; > cci->bg.runtime = 0; > + cci->bg.aborted = false; > cci->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, > bg_timercb, cci); > + qemu_mutex_init(&cci->bg.lock); > > memset(&cci->fw, 0, sizeof(cci->fw)); > cci->fw.active_slot = 1; > cci->fw.slot[cci->fw.active_slot - 1] = true; > + cci->initialized = true; > +} > + > +void cxl_destroy_cci(CXLCCI *cci) > +{ > + qemu_mutex_destroy(&cci->bg.lock); > + cci->initialized = false; > } > > static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd > (*cxl_cmds)[256]) > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 4114163324bd..f04aa58ea85d 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -961,6 +961,7 @@ static void ct3_exit(PCIDevice *pci_dev) > pcie_aer_exit(pci_dev); > cxl_doe_cdat_release(cxl_cstate); > g_free(regs->special_ops); > + cxl_destroy_cci(&ct3d->cci); > if (ct3d->dc.host_dc) { > cxl_destroy_dc_regions(ct3d); > address_space_destroy(&ct3d->dc.host_dc_as); > @@ -1209,12 +1210,17 @@ static void ct3d_reset(DeviceState *dev) > * Bring up an endpoint to target with MCTP over VDM. > * This device is emulating an MLD with single LD for now. > */ > + if (ct3d->vdm_fm_owned_ld_mctp_cci.initialized) { > + cxl_destroy_cci(&ct3d->vdm_fm_owned_ld_mctp_cci); > + } > cxl_initialize_t3_fm_owned_ld_mctpcci(&ct3d->vdm_fm_owned_ld_mctp_cci, > DEVICE(ct3d), DEVICE(ct3d), > 512); /* Max payload made up */ > + if (ct3d->ld0_cci.initialized) { > + cxl_destroy_cci(&ct3d->ld0_cci); > + } > cxl_initialize_t3_ld_cci(&ct3d->ld0_cci, DEVICE(ct3d), DEVICE(ct3d), > 512); /* Max payload made up */ > - > } > > static Property ct3_props[] = { > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > index e14e56ae4bc2..c0e8d40053db 100644 > --- a/include/hw/cxl/cxl_device.h > +++ b/include/hw/cxl/cxl_device.h > @@ -176,10 +176,12 @@ typedef struct CXLCCI { > uint16_t opcode; > uint16_t complete_pct; > uint16_t ret_code; /* Current value of retcode */ > + bool aborted; > uint64_t starttime; > /* set by each bg cmd, cleared by the bg_timer when complete */ > uint64_t runtime; > QEMUTimer *timer; > + QemuMutex lock; /* serializes mbox abort vs timer cb */ > } bg; > > /* firmware update */ > @@ -201,6 +203,7 @@ typedef struct CXLCCI { > DeviceState *d; > /* Pointer to the device hosting the protocol conversion */ > DeviceState *intf; > + bool initialized; > } CXLCCI; > > typedef struct cxl_device_state { > @@ -316,6 +319,7 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState > *d, size_t payload_max); > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, > DeviceState *d, size_t payload_max); > void cxl_init_cci(CXLCCI *cci, size_t payload_max); > +void cxl_destroy_cci(CXLCCI *cci); > void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd > (*cxl_cmd_set)[256], > size_t payload_max); > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd, > diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h > index beb048052e1b..9008402d1c46 100644 > --- a/include/hw/cxl/cxl_mailbox.h > +++ b/include/hw/cxl/cxl_mailbox.h > @@ -14,5 +14,6 @@ > #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4) > #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5) > #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6) > +#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7) > > #endif
