[vioscsi] SendSRB minor refactor

[benyamin-codez]

The virtqueue_add_buf() routine will return 0 on SUCCESS or otherwise a negative number, usually -28 (ENOSPC), i.e no space for buffer. An inline comment is added for edification.

Here we refactor virtqueue_add_buf() handling to only process the SRB if virtqueue_add_buf() returns SUCCESS. Presently, other positive return codes would be processed.

Here we define VQ_ADD_BUFFER_SUCCESS in vioscsi\helper.h.

To ensure valid data is reported we also move the trace event on failure to above the call to CompleteRequest() and wrap the line for improved readability.

[benyamin-codez]

I'll drop another commit here in coming hours but thought it worth mentioning that I'm expecting the following gets clobbered by PR #1150, so won't do anything with that...

kvm-guest-drivers-windows/vioscsi/helper.c

Lines 116 to 121 in 281bb7b

	}
	VioScsiVQUnlock(DeviceExtension, MessageID, &LockHandle, FALSE);
	if ( res >= 0){
	if (virtqueue_kick_prepare(adaptExt->vq[QueueNumber])) {
	virtqueue_notify(adaptExt->vq[QueueNumber]);
	}

[benyamin-codez]

Have pushed the additional commit...
Switching this from draft to review...

[benyamin-codez]

Check will fail per:

I'll drop another commit here in coming hours but thought it worth mentioning that I'm expecting the following gets clobbered by PR #1150, so won't do anything with that...

kvm-guest-drivers-windows/vioscsi/helper.c

Lines 116 to 121 in 281bb7b

	}
	VioScsiVQUnlock(DeviceExtension, MessageID, &LockHandle, FALSE);
	if ( res >= 0){
	if (virtqueue_kick_prepare(adaptExt->vq[QueueNumber])) {
	virtqueue_notify(adaptExt->vq[QueueNumber]);
	}

...for undeclared identifier res (as expected).

Let me know if that's ok or you need something else from me.

[JonKohler]

LGTM at first glance, thanks for the cleanup

[kostyanf14]

Hi @benyamin-codez, please fix build errors.

[benyamin-codez]

@kostyanf14

Thanks I will revert the offending changes and put them in a new PR to review after other merges...

[benyamin-codez]

@JonKohler

Jon, I thought I'd better check the modded trace works properly...

I found we do actually set -ENOSPC in virtqueue_add_buf_split() and virtqueue_add_buf_packed() per, e.g. :

kvm-guest-drivers-windows/VirtIO/VirtIORing.c

Line 263 in 22d0908

return -ENOSPC;

However, it returns -1 per:

kvm-guest-drivers-windows/VirtIO/osdep.h

Line 20 in 22d0908

#define ENOSPC 1

I'm still of the view that ENOSPC is appropriate here.

@YanVugenfirer

Per the above, do you know if there was any reason we define ENOSPC as 1 in osdep.h rather than 28...?
I see it was implemented in 2014 per 264f1b6.

In Windows, 0x1 is ERROR_INVALID_FUNCTION and 0x1C (28) is ERROR_OUT_OF_PAPER...
Given why and how we are using it, alternatives might include one of the following:

ERROR_ACCESS_DENIED     0x5
ERROR_NOT_ENOUGH_MEMORY 0x8
ERROR_NOT_READY         0x15 (21)
ERROR_BUSY              0xAA (170)

...but these would require single line changes to VirtIO ( both VirtIORing.c and VirtIORing-Packed.c).

AFAICT, these are the only files in the tree which reference ENOSPC, but deleting the definition from osdep.h will result in undeclared identifier compilation errors. Perhaps I should include a commit to change it to 28 in this PR (or maybe a separate PR would be best)?

An alternative might be for me to use:

RhelDbgPrint(TRACE_LEVEL_WARNING, 
             " Could not put an SRB into a VQ due to error %s. To be completed with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", 
             (res == -1) ? "ENOSPC" : "UNKNOWN", QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue);

Any advice on how to proceed would be appreciated.

cc: @vrozenfe

[YanVugenfirer]

@benyamin-codez I will take a look. But I am travelling for the KVM Forum now and it might take me some time.

[JonKohler]

@benyamin-codez I will take a look. But I am travelling for the KVM Forum now and it might take me some time.

Glad you could make it. Travel logistics to Brno are brutal from where I live, I wanted to go this year but yikes, it's tough!

[benyamin-codez]

@vrozenfe
@YanVugenfirer

Yan & Vadim,

I've set this back to draft with a view to replacing it with a single commit PR.

I will rebase against master and add the changes I had reverted back in.

On the ENOSPC / SCSISTAT_QUEUE_FULL trace, I think it best to use:

        RhelDbgPrint(TRACE_LEVEL_WARNING, 
                     " Could not put an SRB into a VQ due to error %s (%i). To be completed with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", 
                     (res == -ENOSPC) ? "ENOSPC" : "UNKNOWN", res, QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue);

Let me know if you want a PR to change the definition of ENOSPC in \VirtIO\osdep.h from 1 to 28 (or xyz) ...

cc: @JonKohler

[vrozenfe]

Let me know if you want a PR to change the definition of ENOSPC in \VirtIO\osdep.h from 1 to 28 (or xyz) ...

As for me, I think it would be nice to keep the same definitions as in Linux.

Best,
Vadim.

[YanVugenfirer]

Let me know if you want a PR to change the definition of ENOSPC in \VirtIO\osdep.h from 1 to 28 (or xyz) ...

As for me, I think it would be nice to keep the same definitions as in Linux.

Best, Vadim.

@benyamin-codez I agree with Vadim. Better be consistence with Linux.

[benyamin-codez]

Single commit PR is #1162.

PR for ENOSPC change is #1163.

Closing this one...