Re: [PATCH net-next v12 03/12] virtio: Expose generic device capability operations

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

On Mon, Nov 24, 2025 at 04:24:37PM -0600, Dan Jurgens wrote:
> On 11/24/25 2:30 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 19, 2025 at 01:15:14PM -0600, Daniel Jurgens wrote:
> >> Currently querying and setting capabilities is restricted to a single
> >> capability and contained within the virtio PCI driver. However, each
> >> device type has generic and device specific capabilities, that may be
> >> queried and set. In subsequent patches virtio_net will query and set
> >> flow filter capabilities.
> >>
> >> This changes the size of virtio_admin_cmd_query_cap_id_result. It's safe
> >> to do because this data is written by DMA, so a newer controller can't
> >> overrun the size on an older kernel.
> >>
> >> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> >> Reviewed-by: Parav Pandit <parav@nvidia.com>
> >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>
> >> ---
> >> v4: Moved this logic from virtio_pci_modern to new file
> >>     virtio_admin_commands.
> >>
> >> v12:
> >>   - Removed uapi virtio_pci include in virtio_admin.h. MST
> >>   - Added virtio_pci uapi include to virtio_admin_commands.c
> >>   - Put () around cap in macro. MST
> >>   - Removed nonsense comment above VIRTIO_ADMIN_MAX_CAP. MST
> >>   - +1 VIRTIO_ADMIN_MAX_CAP when calculating array size. MST
> >>   - Updated commit message
> >> ---
> >>  drivers/virtio/Makefile                |  2 +-
> >>  drivers/virtio/virtio_admin_commands.c | 91 ++++++++++++++++++++++++++
> >>  include/linux/virtio_admin.h           | 80 ++++++++++++++++++++++
> >>  include/uapi/linux/virtio_pci.h        |  6 +-
> >>  4 files changed, 176 insertions(+), 3 deletions(-)
> >>  create mode 100644 drivers/virtio/virtio_admin_commands.c
> >>  create mode 100644 include/linux/virtio_admin.h
> >>
> >> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> >> index eefcfe90d6b8..2b4a204dde33 100644
> >> --- a/drivers/virtio/Makefile
> >> +++ b/drivers/virtio/Makefile
> >> @@ -1,5 +1,5 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >> -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
> >> +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_admin_commands.o
> >>  obj-$(CONFIG_VIRTIO_ANCHOR) += virtio_anchor.o
> >>  obj-$(CONFIG_VIRTIO_PCI_LIB) += virtio_pci_modern_dev.o
> >>  obj-$(CONFIG_VIRTIO_PCI_LIB_LEGACY) += virtio_pci_legacy_dev.o
> >> diff --git a/drivers/virtio/virtio_admin_commands.c b/drivers/virtio/virtio_admin_commands.c
> >> new file mode 100644
> >> index 000000000000..a2254e71e8dc
> >> --- /dev/null
> >> +++ b/drivers/virtio/virtio_admin_commands.c
> >> @@ -0,0 +1,91 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +
> >> +#include <linux/virtio.h>
> >> +#include <linux/virtio_config.h>
> >> +#include <linux/virtio_admin.h>
> >> +#include <uapi/linux/virtio_pci.h>
> >> +
> >> +int virtio_admin_cap_id_list_query(struct virtio_device *vdev,
> >> +				   struct virtio_admin_cmd_query_cap_id_result *data)
> >> +{
> >> +	struct virtio_admin_cmd cmd = {};
> >> +	struct scatterlist result_sg;
> >> +
> >> +	if (!vdev->config->admin_cmd_exec)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	sg_init_one(&result_sg, data, sizeof(*data));
> >> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_CAP_ID_LIST_QUERY);
> >> +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
> >> +	cmd.result_sg = &result_sg;
> >> +
> >> +	return vdev->config->admin_cmd_exec(vdev, &cmd);
> >> +}
> >> +EXPORT_SYMBOL_GPL(virtio_admin_cap_id_list_query);
> >> +
> >> +int virtio_admin_cap_get(struct virtio_device *vdev,
> >> +			 u16 id,
> >> +			 void *caps,
> >> +			 size_t cap_size)
> > 
> > 
> > I still don't get why cap_size needs to be as large as size_t.
> > 
> > if you don't care what's it size is, just say "unsigned".
> > or u8 as a hint to users it's a small value.
> 
> The size is small for net flow filters, but this is supposed to be a
> generic interface for future uses as well. Why limit it?

because your implementation
makes assumptions - if you want it to be truly generic then you need to handle
weird corner cases such as integer overflow when you add these things.


> 
> > 
> >> +{
> >> +	struct virtio_admin_cmd_cap_get_data *data;
> >> +	struct virtio_admin_cmd cmd = {};
> >> +	struct scatterlist result_sg;
> >> +	struct scatterlist data_sg;
> >> +	int err;
> >> +
> >> +	if (!vdev->config->admin_cmd_exec)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > 
> > uses kzalloc without including linux/slab.h
> > 
> > 
> > 
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +