Message-ID: <4fa7bf85-e935-45aa-bb2f-f37926397c31@nvidia.com>
Date: Thu, 25 Sep 2025 15:21:38 +0530
From: Parav Pandit <parav@...dia.com>
To: "Michael S. Tsirkin" <mst@...hat.com>, Dan Jurgens <danielj@...dia.com>
Cc: Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org,
 alex.williamson@...hat.com, pabeni@...hat.com,
 virtualization@...ts.linux.dev, 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 25-09-2025 11:46 am, Michael S. Tsirkin wrote:
> On Wed, Sep 24, 2025 at 02:02:34PM -0500, Dan Jurgens wrote:
>> 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.

It is very clear from the virtio_admin_ops definition that it is not 
specific to admin vq. It is a admin command interface.


>>>> 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.
> Right, but most stuff is not transport specific. If you are going to
> put in the work, what is transport specific is admin VQ access.
> Commands themselves are transport agnostic, we just did not need
> them in non-pci previously.
>
Should config_ops be extended to have admin cmd interface there?

No strong preference for putting function pointers in new admin_ops or 
config_ops.

I just find it better to have admin_ops clearly defined as it makes the 
code crystal clear of what the ops are about.

Today, one needs to be cautious when reading config_ops of below note.

"Note: Do not assume that a transport implements all of the operations".

Having well defined admin_ops appeared more clear. But config_ops 
extension (or overloading) seems more simpler, no strong preference to me.

Regarding Dan's comment on "net driver directly accessing admin_ops 
directly" seems a bad idea to me.

Function pointers are there for multiple transports to implement their 
own implementation.

it is not meant to develop open coded drivers. In fact some day 
config_ops struct definition can be restricted to only transport drivers.

Major part of the kernel does not follow open coding function pointer calls.

Powered by blists - more mailing lists