OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-comment] Re: [PATCH v10 03/10] admin: introduce group administration commands



On 08/03/2023 12:55, Max Gurtovoy wrote:



On 08/03/2023 2:34, Michael S. Tsirkin wrote:

On Mon, Mar 06, 2023 at 01:04:42PM +0200, Max Gurtovoy wrote:



On 03/03/2023 15:19, Michael S. Tsirkin wrote:

On Fri, Mar 03, 2023 at 08:17:03AM -0500, Stefan Hajnoczi wrote:

On Thu, Mar 02, 2023 at 07:01:56PM -0500, Michael S. Tsirkin wrote:

On Thu, Mar 02, 2023 at 03:19:12PM -0500, Stefan Hajnoczi wrote:

On Thu, Mar 02, 2023 at 06:40:29PM +0000, Parav Pandit wrote:



From: Michael S. Tsirkin <mst@redhat.com>
Sent: Thursday, March 2, 2023 8:05 AM
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier} 
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table 
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier
Can you please add other useful error codes in addition to the EINVAL? 
Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.
Please define a unique constant for each error condition that can occur 
instead of sharing catch-all errno constants between multiple error
conditions. If a driver wants to squash them together into an errno,
that's fine, but I think doing this at the hardware interface level is 
just propagating the mistakes of errnos.

Only status_qualifier is needed and the vague status field can be
dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and
ENODEV is useful. They have no meaning to the driver, only the
status_qualifier really indicates what is going on.


At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to
be seen.







I'm sure you guys have discussed this previously, but please provide
rationale in the spec because it looks weird to someone with fresh eyes. 

Stefan


Really most drivers just want to propagate errno to userspace.
All the detailed reporting is for sure well intentional but
in the end it is at best printed into log - end to end
people just end up with a switch statement
converting these to errno codes.
So we are passing them from device and this way there will be
some uniformity.


Please clarify the rationale in the spec. I don't agree with it, as
explained in my earlier email, but as long as its documented, people can 
try to follow it.

Stefan


It's 2:2 for now, you are with Parav, me and Cornelia like it :)
Or I will try to document it better.
I don't understand this status_qualifier as well and it wasn't included in 
my original patch set.


Sounds like you feel I should drop your S.O.B - is this the complaint?
I wanted to give attribution since I started with that but sure, no
problem.
please use "Co-developed-by:" for this patch in case you stick to the status_qualifier field that I don't agree with. According to your comment that it was 2:2 between MST/Cornelia and Parav/Stefan, now it's 2:3 with Parav/Stefan/Max (if that can help deciding on this topic in a fair game way). 




I vote for "status" that describe generic status codes and
"command_specific_error" that should be inspected by the driver only if
"status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR".
We discussed this so many times before (and already agreed IIRC) and adding this new qualifiers mechanism sounds not right to me and not intuitive for 
device and driver developers.

I suggest:

1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the
command_specific_error field).


I don't think it's a good idea, we'll have to agree to disagree.


Ok.
can you explain why isn't this a good idea please ?
remember that this is very common logic in many specifications and many HW device implementation. 


The point is simple. We can have a detailed virtio specific error.
Nice for debugging most drivers won't know what to do with it.
This is the status_qualifier.
Very detailed but
generally drivers will just have a giant switch statement translating
it to a simple error code for userspace.


how giant is this switch statement ?
NVMe handles fine with this logic. Maybe you can take a look on that to get some more ideas of implementing this kind of things. 

Why do we need to add some qualifiers logic into our devices ?


So to save everyone work, we also added "status"
a generic kind of error class that is easy to pass to userspace
with a small switch statement.


if you want something that is easy just pass OK = 0 and not_OK = 1 or != 0.
Not need for qualifiers.
In you case the qualifiers is just an extension for the status.



COMMAND_SPECIFIC_ERR is just way too much detail - commands generally
just should not fail it's a quality of implementation issue.


it's the way many specifications work.
And this is the correct way for future extensions IMO.
Each new command may have it's own namespace of errors extensions.

why we defer such essential decisions for the future ?
We already introduces 2-3 admin features in the past (MSIX, feature bits config, num_queues config, etc..) - maybe we can learn from it ? 















This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists 
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]