Re: [PATCH] hw/cxl: Support aborting background commands

Jonathan Cameron
2024-08-27T15:34:00Z

On Tue, 13 Aug 2024 15:12:55 -0700
Davidlohr Bueso <dave@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.opensrc@micron.com>
> Reviewed-by: Ajay Joshi <ajay.opensrc@micron.com>
> Signed-off-by: Davidlohr Bueso <dave@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.opensrc@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