Re: [PATCH v8 8/9] admin: command list discovery

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Using 0x instead of h suffix? Sure.
> >
> > We should probably document the syntax in introduction.tex
> > though this seems low priority.
> 
> Nod.
> >> > +When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
> >> > +the following table describes possible \field{status_qialifier} values:
> >> > +\begin{tabular}{|l|l|l|}
> >> > +\hline
> >> > +Status & Name & Description \\
> >> > +\hline \hline
> >> > +00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> >> 
> >> Either 0x00, or decimal (which one is better?)
> >
> > I think I prefer 0x here. And maybe I will add status values in both hex
> > and decimal (I used decimal to be consistent with linux headers but
> > fundamentally what we use most of the time is hex).
> 
> Ok.
> 
> >
> >> > +\hline
> >> > +01h   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> >> > +\hline
> >> > +02h   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> >> > +\hline
> >> > +03h   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} was set  \\
> >> 
> >> s/was set//
> >> 
> >> > +\hline
> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> >> 
> >> s/was set//
> >> 
> >> > +\hline
> >> > +other   & -    & group administration command error  \\
> >> 
> >> Again the question whether this is something that can be defined per
> >> group type.
> >
> > probably - above ones are generic, let's see if we need specific ones.
> > if yes will be easy to add.
> 
> I think we want to distinguish between "reserved" (not defined yet, may
> get a meaning later on) and "group type specific" (a group type may use
> it, don't reuse for generic stuff). If we need group type specific
> errors (and don't want a free-for-all), we could go with eg.
> 
> 0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\
> 
> or so? Do we see any need for that yet?

Not yet.

> >> > +The driver then enables use of some of the opcodes by sending to
> >> > +the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
> >> > +of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
> >> > +both understood and used by the driver.
> >> > +
> >> > +If the device supports the command list used by the driver, the
> >> > +device completes the command with status VIRTIO_ADMIN_STATUS_OK.
> >> > +If the device does not support the command list, the device
> >> > +completes the command with status
> >> > +VIRTIO_ADMIN_STATUS_INVALID_FIELD.
> >> > +
> >> > +Note: drivers are assumed not to set bits in device_admin_cmds
> >> > +if they are not familiar with how the command opcode
> >> > +is used, since devices could have dependencies between
> >> > +command opcodes.
> >> > +
> >> > +It is assumed that all members in a group support and are used
> >> > +with the same list of commands.
> >> 
> >> Can the driver later change its mind, i.e. use VIRTIO_ADMIN_CMD_LIST_USE
> >> a second time with a different set of commands? If yes, can it add
> >> commands, or only withdraw them?
> >
> > I think it's ok to allow changing them arbitrarily at any time.
> > If driver wants to "lock down" the list then all it has to do
> > is send a list with VIRTIO_ADMIN_CMD_LIST_USE cleared.
> >
> > It seemed clear along the lines of since it's not prohibited it's
> > allowed but since the question arose I will add a conformance clause for
> > this.
> 
> So if the driver sends LIST_USE with an additional command, and the
> device rejects that list (because of dependencies or whatever), is the
> list of commands supposed to stay whatever it was prior to that?

I think so, yes. Good point I'll document that.


> >
> >
> >> What happens if a driver tries to send a command that it had not
> >> included in the list?
> >
> >
> > This is covered in conformance statements in the next patch.
> >
> > +
> > +Device MUST validate commands against the list used by
> > +driver and MUST fail any commands not in the list with
> > +\field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
> > +and \field{status_qualifier} set to
> > +VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE.
> 
> Ok.