Message-ID: <16019785-ca9e-4d63-8a0f-c2f3fdcd32b8@nvidia.com>
Date: Wed, 24 Sep 2025 14:02:34 -0500
From: Dan Jurgens <danielj@...dia.com>
To: "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org, alex.williamson@...hat.com, pabeni@...hat.com,
 virtualization@...ts.linux.dev, parav@...dia.com, shshitrit@...dia.com,
 yohadt@...dia.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
 shameerali.kolothum.thodi@...wei.com, jgg@...pe.ca, kevin.tian@...el.com,
 kuba@...nel.org, andrew+netdev@...n.ch, edumazet@...gle.com,
 Yishai Hadas <yishaih@...dia.com>
Subject: Re: [PATCH net-next v3 01/11] virtio-pci: Expose generic device
 capability operations

On 9/24/25 1:22 AM, Michael S. Tsirkin wrote:
> On Wed, Sep 24, 2025 at 09:16:32AM +0800, Jason Wang wrote:
>> On Tue, Sep 23, 2025 at 10:20 PM Daniel Jurgens <danielj@...dia.com> 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.
>>>
>>> Move the admin related definitions to a new header file. It needs to be
>>> abstracted away from the PCI specifics to be used by upper layer
>>> drivers.
>>>
>>> Signed-off-by: Daniel Jurgens <danielj@...dia.com>
>>> Reviewed-by: Parav Pandit <parav@...dia.com>
>>> Reviewed-by: Shahar Shitrit <shshitrit@...dia.com>
>>> Reviewed-by: Yishai Hadas <yishaih@...dia.com>
>>> ---
>>
>> [...]
>>
>>>
>>>  size_t virtio_max_dma_size(const struct virtio_device *vdev);
>>>
>>> diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
>>> new file mode 100644
>>> index 000000000000..bbf543d20be4
>>> --- /dev/null
>>> +++ b/include/linux/virtio_admin.h
>>> @@ -0,0 +1,68 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only
>>> + *
>>> + * Header file for virtio admin operations
>>> + */
>>> +#include <uapi/linux/virtio_pci.h>
>>> +
>>> +#ifndef _LINUX_VIRTIO_ADMIN_H
>>> +#define _LINUX_VIRTIO_ADMIN_H
>>> +
>>> +struct virtio_device;
>>> +
>>> +/**
>>> + * VIRTIO_CAP_IN_LIST - Check if a capability is supported in the capability list
>>> + * @cap_list: Pointer to capability list structure containing supported_caps array
>>> + * @cap: Capability ID to check
>>> + *
>>> + * The cap_list contains a supported_caps array of little-endian 64-bit integers
>>> + * where each bit represents a capability. Bit 0 of the first element represents
>>> + * capability ID 0, bit 1 represents capability ID 1, and so on.
>>> + *
>>> + * Return: 1 if capability is supported, 0 otherwise
>>> + */
>>> +#define VIRTIO_CAP_IN_LIST(cap_list, cap) \
>>> +       (!!(1 & (le64_to_cpu(cap_list->supported_caps[cap / 64]) >> cap % 64)))
>>> +
>>> +/**
>>> + * struct virtio_admin_ops - Operations for virtio admin functionality
>>> + *
>>> + * This structure contains function pointers for performing administrative
>>> + * operations on virtio devices. All data and caps pointers must be allocated
>>> + * on the heap by the caller.
>>> + */
>>> +struct virtio_admin_ops {
>>> +       /**
>>> +        * @cap_id_list_query: Query the list of supported capability IDs
>>> +        * @vdev: The virtio device to query
>>> +        * @data: Pointer to result structure (must be heap allocated)
>>> +        * Return: 0 on success, negative error code on failure
>>> +        */
>>> +       int (*cap_id_list_query)(struct virtio_device *vdev,
>>> +                                struct virtio_admin_cmd_query_cap_id_result *data);
>>> +       /**
>>> +        * @cap_get: Get capability data for a specific capability ID
>>> +        * @vdev: The virtio device
>>> +        * @id: Capability ID to retrieve
>>> +        * @caps: Pointer to capability data structure (must be heap allocated)
>>> +        * @cap_size: Size of the capability data structure
>>> +        * Return: 0 on success, negative error code on failure
>>> +        */
>>> +       int (*cap_get)(struct virtio_device *vdev,
>>> +                      u16 id,
>>> +                      void *caps,
>>> +                      size_t cap_size);
>>> +       /**
>>> +        * @cap_set: Set capability data for a specific capability ID
>>> +        * @vdev: The virtio device
>>> +        * @id: Capability ID to set
>>> +        * @caps: Pointer to capability data structure (must be heap allocated)
>>> +        * @cap_size: Size of the capability data structure
>>> +        * Return: 0 on success, negative error code on failure
>>> +        */
>>> +       int (*cap_set)(struct virtio_device *vdev,
>>> +                      u16 id,
>>> +                      const void *caps,
>>> +                      size_t cap_size);
>>> +};
>>
>> Looking at this, it's nothing admin virtqueue specific, I wonder why
>> it is not part of virtio_config_ops.
>>
>> Thanks
> 
> cap things are admin commands. But what I do not get is why they
> need to be callbacks.
> 
> The only thing about admin commands that is pci specific is finding
> the admin vq.
> 
> I'd expect an API for that in config then, and the rest of code can
> be completely transport independent.
> 
> 

The idea was that each transport would implement the callbacks, and we
have indirection at the virtio_device level. Similar to the config_ops.
So the drivers stay transport agnostic. I know these are PCI specific
now, but thought it should be implemented generically.

These could go in config ops. But I thought it was better to isolate
them in a new _ops structure.

An earlier implementation had the net driver accessing the admin_ops
directly. But Parav thought this was better.

Powered by blists - more mailing lists