1. Patchew
  2. Xen
  3. View series

[PATCH] include/public: add command result definitions to vscsiif.h

Juergen Gross posted 1 patch 2 years, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220228112224.18942-1-jgross@suse.com
There is a newer version of this series
xen/include/public/io/vscsiif.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
[PATCH] include/public: add command result definitions to vscsiif.h
Posted by Juergen Gross 2 years, 2 months ago 
The result field of struct vscsiif_response is lacking a detailed
definition. Today the Linux kernel internal scsi definitions are being
used, which is not a sane interface for a PV device driver.

Add macros to change that by using today's values in the XEN namespace.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/io/vscsiif.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
index c9ceb1884d..17a9033b43 100644
--- a/xen/include/public/io/vscsiif.h
+++ b/xen/include/public/io/vscsiif.h
@@ -315,6 +315,33 @@ struct vscsiif_response {
 };
 typedef struct vscsiif_response vscsiif_response_t;
 
+/* SCSI I/O status from vscsiif_response->rslt */
+#define XEN_VSCSIIF_RSLT_STATUS(x)  (x & 0x00ff)
+
+/* Host I/O status from vscsiif_response->rslt */
+#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)
+#define XEN_VSCSIIF_RSLT_HOST_OK         0
+#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
+#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
+#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
+#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
+#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
+#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
+#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
+#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
+#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
+#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
+#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
+#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
+#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
+#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
+#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
+#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
+#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
+#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
+#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
+#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
+
 DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response);
 
 
-- 
2.34.1
Re: [PATCH] include/public: add command result definitions to vscsiif.h
Posted by Jan Beulich 2 years, 1 month ago 
On 28.02.2022 12:22, Juergen Gross wrote:
> --- a/xen/include/public/io/vscsiif.h
> +++ b/xen/include/public/io/vscsiif.h
> @@ -315,6 +315,33 @@ struct vscsiif_response {
>  };
>  typedef struct vscsiif_response vscsiif_response_t;
>  
> +/* SCSI I/O status from vscsiif_response->rslt */
> +#define XEN_VSCSIIF_RSLT_STATUS(x)  (x & 0x00ff)

No #define-s for individual values for this? I see the backend use
e.g. SUCCESS and FAILED, wherever these come from ...

Also please parenthesize x here and ...

> +/* Host I/O status from vscsiif_response->rslt */
> +#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)

... here.

You cast to unsigned here, but rslt is a signed field. Is it really
the entire upper 16 bits that are the host I/O status?

> +#define XEN_VSCSIIF_RSLT_HOST_OK         0
> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */

Are the all-upper-case words really in need of mirroring this
aspect from Linux? To me it gives the impression of this being
acronyms of some sort at the first glance.

> +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
> +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
> +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
> +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */

Some of the name shortening that you did, comparing with the
Linux names, has gone a little too far for my taste. But you're
the maintainer ...

Jan
Re: [PATCH] include/public: add command result definitions to vscsiif.h
Posted by Juergen Gross 2 years, 1 month ago 
On 14.03.22 10:55, Jan Beulich wrote:
> On 28.02.2022 12:22, Juergen Gross wrote:
>> --- a/xen/include/public/io/vscsiif.h
>> +++ b/xen/include/public/io/vscsiif.h
>> @@ -315,6 +315,33 @@ struct vscsiif_response {
>>   };
>>   typedef struct vscsiif_response vscsiif_response_t;
>>   
>> +/* SCSI I/O status from vscsiif_response->rslt */
>> +#define XEN_VSCSIIF_RSLT_STATUS(x)  (x & 0x00ff)
> 
> No #define-s for individual values for this? I see the backend use
> e.g. SUCCESS and FAILED, wherever these come from ...

Oh, right, those are being used for the reset actions. Thanks for
spotting.

The "normal" request result values are defined at the SCSI layer.

> Also please parenthesize x here and ...
> 
>> +/* Host I/O status from vscsiif_response->rslt */
>> +#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)
> 
> ... here.

Okay.

> You cast to unsigned here, but rslt is a signed field. Is it really
> the entire upper 16 bits that are the host I/O status?

I thought I have seen it being used this way, but now I've found the
definition of "host_byte()" indicating it is only 8 bits wide. Will
change that.

> 
>> +#define XEN_VSCSIIF_RSLT_HOST_OK         0
>> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
>> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
>> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
> 
> Are the all-upper-case words really in need of mirroring this
> aspect from Linux? To me it gives the impression of this being
> acronyms of some sort at the first glance.

The backend can return all these values, so I think I need to define
them here.

> 
>> +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
>> +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
>> +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
>> +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
>> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
>> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
>> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
>> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
>> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
>> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
>> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
>> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
>> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
>> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
>> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
> 
> Some of the name shortening that you did, comparing with the
> Linux names, has gone a little too far for my taste. But you're
> the maintainer ...

There are basically the following alternatives:

- use longer names (using the Linux names would end up in e.g.
   XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer
- drop some part of the common prefix, e.g. the "RSLT_HOST_" part
- keep it as is

I'm basically fine with any of those.


Juergen
Re: [PATCH] include/public: add command result definitions to vscsiif.h
Posted by Jan Beulich 2 years, 1 month ago 
On 16.03.2022 10:03, Juergen Gross wrote:
> On 14.03.22 10:55, Jan Beulich wrote:
>> On 28.02.2022 12:22, Juergen Gross wrote:
>>> +#define XEN_VSCSIIF_RSLT_HOST_OK         0
>>> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
>>> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
>>> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
>>
>> Are the all-upper-case words really in need of mirroring this
>> aspect from Linux? To me it gives the impression of this being
>> acronyms of some sort at the first glance.
> 
> The backend can return all these values, so I think I need to define
> them here.

Oh, I realize I didn't say so explicitly and hence what I said
ended up being ambiguous: The remark was only about the all-
upper-case words in the comments. I would think they can be
spelled normally.

>>> +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
>>> +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
>>> +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
>>> +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
>>> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
>>> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
>>> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
>>> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
>>> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
>>> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
>>> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
>>> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
>>> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
>>> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
>>> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
>>
>> Some of the name shortening that you did, comparing with the
>> Linux names, has gone a little too far for my taste. But you're
>> the maintainer ...
> 
> There are basically the following alternatives:
> 
> - use longer names (using the Linux names would end up in e.g.
>    XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer
> - drop some part of the common prefix, e.g. the "RSLT_HOST_" part
> - keep it as is
> 
> I'm basically fine with any of those.

My personal preference would be in the order you named the
alternatives, perhaps with prepending them by "use longer names,
but in extreme cases not quite as long as Linux'es".

Jan
Re: [PATCH] include/public: add command result definitions to vscsiif.h
Posted by Juergen Gross 2 years, 1 month ago 
On 16.03.22 10:09, Jan Beulich wrote:
> On 16.03.2022 10:03, Juergen Gross wrote:
>> On 14.03.22 10:55, Jan Beulich wrote:
>>> On 28.02.2022 12:22, Juergen Gross wrote:
>>>> +#define XEN_VSCSIIF_RSLT_HOST_OK         0
>>>> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
>>>
>>> Are the all-upper-case words really in need of mirroring this
>>> aspect from Linux? To me it gives the impression of this being
>>> acronyms of some sort at the first glance.
>>
>> The backend can return all these values, so I think I need to define
>> them here.
> 
> Oh, I realize I didn't say so explicitly and hence what I said
> ended up being ambiguous: The remark was only about the all-
> upper-case words in the comments. I would think they can be
> spelled normally.

Ah, okay. :-)

Will change that.

> 
>>>> +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
>>>> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
>>>
>>> Some of the name shortening that you did, comparing with the
>>> Linux names, has gone a little too far for my taste. But you're
>>> the maintainer ...
>>
>> There are basically the following alternatives:
>>
>> - use longer names (using the Linux names would end up in e.g.
>>     XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer
>> - drop some part of the common prefix, e.g. the "RSLT_HOST_" part
>> - keep it as is
>>
>> I'm basically fine with any of those.
> 
> My personal preference would be in the order you named the
> alternatives, perhaps with prepending them by "use longer names,
> but in extreme cases not quite as long as Linux'es".

Okay, fine with me.


Juergen
Re: [PATCH] include/public: add command result definitions to vscsiif.h
Posted by Juergen Gross 2 years, 1 month ago 
Ping?

On 28.02.22 12:22, Juergen Gross wrote:
> The result field of struct vscsiif_response is lacking a detailed
> definition. Today the Linux kernel internal scsi definitions are being
> used, which is not a sane interface for a PV device driver.
> 
> Add macros to change that by using today's values in the XEN namespace.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   xen/include/public/io/vscsiif.h | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
> index c9ceb1884d..17a9033b43 100644
> --- a/xen/include/public/io/vscsiif.h
> +++ b/xen/include/public/io/vscsiif.h
> @@ -315,6 +315,33 @@ struct vscsiif_response {
>   };
>   typedef struct vscsiif_response vscsiif_response_t;
>   
> +/* SCSI I/O status from vscsiif_response->rslt */
> +#define XEN_VSCSIIF_RSLT_STATUS(x)  (x & 0x00ff)
> +
> +/* Host I/O status from vscsiif_response->rslt */
> +#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)
> +#define XEN_VSCSIIF_RSLT_HOST_OK         0
> +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout */
> +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
> +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
> +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
> +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
> +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
> +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
> +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
> +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
> +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
> +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
> +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
> +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
> +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
> +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
> +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
> +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path */
> +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
> +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
> +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
> +
>   DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response);
>   
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.