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 <[email protected]>
> >> Reviewed-by: Parav Pandit <[email protected]>
> >> Reviewed-by: Xuan Zhuo <[email protected]>
> >>
> >> ---
> >> 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;
> >> +
