RE: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 3, 2023 3:19 PM

[..]
> > > just say all these must be supported.
> > > In fact what are you saying here? That all 3 are supported or none
> > > at all? What about just
> > > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > > Looks like a slower but working way to do notifications is through a
> > > common config write, no?
> > Notifications to be done using the notification region exposed and
> > queried using the 3rd QUERY command.
> > >
> > > > +
> > > > +The device MUST support legacy configuration registers to its defined
> width.
> > >
> > > what is this?
> > >
> > Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> > So device support the access to its defined width.
> > May be to rewrite it differently?
> 
> Again you are duplicating the existing text, no?
> And in this case, contradicting it?
> 
Yes, to both the questions.
I started with slightly sane version that contradicts below, but since below is written it what is written so it is the current version.
I will drop above normative.

> 	When using the legacy interface the driver MAY access
> 	the device-specific configuration region using any width accesses, and
> 	a transitional device MUST present driver with the same results as
> 	when accessed using the ``natural'' access method (i.e.
> 	32-bit accesses for 32-bit fields, etc).
> 
> 
> > > > +
> > > > +The device MAY fail legacy configuration registers access when
> > > > +either the access is for an incorrct register width or if the register offset
> is incorrect.
> > >
> > Spell checkers didnt capture the error of incorrct. Need to find
> > better tool.
> >
> > > with which error code?
> > EINVAL
> > but should we repeat the general section here?
> 
> this is a command specific case, isn't it?
> 
In this case the generic define error codes are covering it for command.
In this specific example, if the register offset provided is outside the register range, 
it returns VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD.
This is already defined in general section.

> 
> > >
> > > > +
> > > > +The device MUST allow access of one or multiple bytes of the
> > > > +registers when such register is defined as byte array, for
> > > > +example \field{mac} of \field{struct virtio_net_config} of the Network
> Device.
> > >
> > > so which accesses need to be supported then?
> > >
> > Not sure I follow the question.
> > Can you please explain?
> 
> Consider mac, do you allow access to any length at all, from 1 to 6 bytes?

Yes, maybe you missed the text.
I wrote it as " The device MUST allow access of one or multiple bytes of the registers when its byte array".

The other paragraph you wrote above about " When using the legacy interface, the driver", it eliminates most of the normative here.

I tried to keep it little sane, but its fine to keep it relaxed too. Sw will be able to make it strict as it finds it suitable.

So I will drop these normatives.