Re: [PATCH 0/3] cxl/mbox: support background operation abort requests

[Ravis OpenSrc]

>On Wed, 23 Oct 2024, Davidlohr Bueso wrote:
>>The one major functionality in our original proposal apart from implementing abort is
>>
>>Allowing background commands to be invoked from user space through the primary mailbox
>>by ensuring only those background commands are enabled which also support request abort operation
>>by checking their individual CEL details.
>
>Is vendor-specific logs not something that can be done OoB?
>
>If we are strictly talking about CEL details, yes this series could use that, and was
>planning on adding it for an eventual v2 -- ie: why send the abort cmd at all if we
>know the current one doesn't support it. But that is minutiae, for kernel bg commands.
>
>But yeah, for your needs, the enabled cmds would need that filter.

Ok. we can merge changes related to CEL checking and filtering of enabled commands.

>
>Then with that, would adding something like the below address your needs and below
>questions? Basically, if userspace is running a command, then the kernel comes in
>and wants to run its own bg command, it will simply abort *anything* ongoing as a
>last resort. And since we have no kernel-context about whatever is running at that
>point, is *think* it is safe to assume it was user-initiated.

Yes, this seems a reasonable assumption.

Currently user space doesn't have a way to initiate background commands
through primary mailbox as there is no provision to provide timeout value.

By filtering out user-initiated background commands which do not support abort,
we can potentially have a default timeout or allow user space to provide a custom timeout value. 
 
Overall, the approach to cancel current background operation when new bg operation
is initiated seems elegant. 
>
>diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>index 7b0fad7f6c4d..bf0742546ea8 100644
>--- a/drivers/cxl/pci.c
>+++ b/drivers/cxl/pci.c
>@@ -374,7 +374,7 @@ static bool cxl_try_to_cancel_background(struct cxl_mailbox *cxl_mbox)
>		mds->security.sanitize_active = false;
>
>		dev_dbg(cxlds->dev, "Sanitization operation aborted\n");
>-	} else {
>+	} else if (atomic_read_acquire(&cxl_mbox->poll_bgop)){
>		/*
>		 * Kick the poller and wait for it to be done - no one else
>		 * can touch mbox regs. rcuwait_wake_up() provides full
>@@ -403,7 +403,9 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>	 */
>	if (cxl_is_background_cmd(cmd->opcode)) {
>		if (mds->security.sanitize_active ||
>-		    atomic_read_acquire(&cxl_mbox->poll_bgop)) {
>+		    atomic_read_acquire(&cxl_mbox->poll_bgop) ||
>+		    /* userspace-initiated ? */
>+		    !cxl_mbox_background_done(cxlds)) {
>			if (!cxl_try_to_cancel_background(cxl_mbox)) {
>				mutex_unlock(&cxl_mbox->mbox_mutex);
>
--Ravi.