On Wed, May 18 2022, Max Gurtovoy <mgurto...@nvidia.com> wrote: > On 5/15/2022 6:23 PM, Michael S. Tsirkin wrote: >> On Wed, Apr 27, 2022 at 01:58:19AM +0300, Max Gurtovoy wrote: >>> This command set is used for essential administrative and management >>> operations. >>> >>> Admin commands should be submitted to a well defined management >>> interface. >>> >>> Reviewed-by: Parav Pandit <pa...@nvidia.com> >>> Signed-off-by: Max Gurtovoy <mgurto...@nvidia.com> >>> --- >>> admin.tex | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> content.tex | 2 + >>> 2 files changed, 125 insertions(+) >>> create mode 100644 admin.tex >>> >>> +The VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT command is used by the driver to >>> acknowledge those admin capabilities it understands and wishes to use. >> >> ok so we have a protocol here, kind of like feature negotiation. Please >> write its description. >> e.g. is it ok to change accepted caps? when? can device change its caps >> etc etc etc. > > I don't understand what does this mean to change a cap ? > > Device can offer a cap and driver can accept it if it wishes to use it. > > That is it. > > I added this mechanism just for your request. > > I never saw a device that asks acceptance from driver but I did my best > to fulfill your request. > >> >> Avoiding this kind of spec work is exactly why me and jason keep telling >> you to consider just using features instead. Add a 64 bit admin features >> field to the PCI transport and be done with it. CCW and MMIO already >> have feature selector so it's trivial to add feature bits. > > It's not scalable for admin mechanism and I don't want to perform 100 > write/read from configuration space instead of doing all in 1 admin command.
Why use the config space for that; just use feature bits, there are enough of those, and we already have a defined protocol. > >> >> >>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT by the >>> driver. >>> + >>> +The command specific data set by the driver is of form: >>> +\begin{lstlisting} >>> +struct virtio_admin_device_caps_accept_data { >>> + /* Indicates which of the below fields were set >>> + * (1 means that field is set): >> >> yes we all know that 1 means set. >> >> do you really mean field is valid maybe? > yes valid == set. >> >> >>> + * Bit 0 - driver_admin_caps >>> + * Bits 1 - 63 - reserved for future fields >>> + */ >>> + le64 attrs_mask; >> looks like going overboard. just send 64 caps bits and be done with it. >> and rename accept_data to accept_caps. > this is the command specific data. >> >>> + /* This field indicates which of the below admin >>> + * capabilities are supported by the driver: >>> + * Bits 0 - 63 - reserved for future capabilities. >>> + */ >>> + le64 driver_admin_caps; >>> + u8 reserved[112]; >> >> I just noticed this. Please do not add this huge amount of padding >> everywhere. instead, explain that device must be ready to accept >> a smaller or larger buffer depending on feature bits. > > It's not huge. It's 128B command data. > > We will be sorry in the future for not doing extendable API. > > I prefer keep it 128B unless there is a concrete reason for not doing so. So just use a variable length structure, that should be extendable for all future use cases. --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org