From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Message-ID: <25a07ee5-3e2a-62fa-159b-ee7510d2d644@nvidia.com> Date: Sun, 30 Jan 2022 20:23:30 +0200 MIME-Version: 1.0 References: <87wnikys4p.fsf@redhat.com> <20220128074613-mutt-send-email-mst@kernel.org> <87tudnzwq9.fsf@redhat.com> <20220128105012-mutt-send-email-mst@kernel.org> <20220130043917-mutt-send-email-mst@kernel.org> <20220130093740-mutt-send-email-mst@kernel.org> <20220130102940-mutt-send-email-mst@kernel.org> From: Max Gurtovoy In-Reply-To: <20220130102940-mutt-send-email-mst@kernel.org> Subject: [virtio-comment] Re: [PATCH v2 1/4] Add virtio Admin Virtqueue Content-Language: en-US Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable To: "Michael S. Tsirkin" Cc: Jason Wang , Cornelia Huck , virtio-comment@lists.oasis-open.org, Virtio-Dev , Parav Pandit , Shahaf Shuler , Oren Duer , Stefan Hajnoczi List-ID: On 1/30/2022 5:30 PM, Michael S. Tsirkin wrote: > On Sun, Jan 30, 2022 at 05:12:46PM +0200, Max Gurtovoy wrote: >> On 1/30/2022 4:41 PM, Michael S. Tsirkin wrote: >>> On Sun, Jan 30, 2022 at 11:56:30AM +0200, Max Gurtovoy wrote: >>>> On 1/30/2022 11:40 AM, Michael S. Tsirkin wrote: >>>>> On Sun, Jan 30, 2022 at 11:13:38AM +0200, Max Gurtovoy wrote: >>>>>> On 1/29/2022 5:53 AM, Jason Wang wrote: >>>>>>> On Fri, Jan 28, 2022 at 11:52 PM Michael S. Tsirkin wrote: >>>>>>>> On Fri, Jan 28, 2022 at 04:49:34PM +0100, Cornelia Huck wrote: >>>>>>>>> On Fri, Jan 28 2022, "Michael S. Tsirkin" wrote: >>>>>>>>> >>>>>>>>>> On Fri, Jan 28, 2022 at 01:14:14PM +0100, Cornelia Huck wrote: >>>>>>>>>>> On Mon, Jan 24 2022, Max Gurtovoy wrote: >>>>>>>>>>>> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Vi= rtio Device / Admin Virtqueues} >>>>>>>>>>>> + >>>>>>>>>>>> +Admin virtqueue is used to send administrative commands to ma= nipulate >>>>>>>>>>>> +various features of the device and/or to manipulate various f= eatures, >>>>>>>>>>>> +if possible, of another device within the same group (e.g. PC= I VFs of >>>>>>>>>>>> +a parent PCI PF device are grouped together. These devices ca= n be >>>>>>>>>>>> +optionally managed by its parent PCI PF using its admin virtq= ueue.). >>>>>>>>>>>> + >>>>>>>>>>>> +Use of Admin virtqueue is negotiated by the VIRTIO_F_ADMIN_VQ >>>>>>>>>>>> +feature bit. >>>>>>>>>>>> + >>>>>>>>>>>> +Admin virtqueue index may vary among different device types. >>>>>>>>>>> So, my understanding is: >>>>>>>>>>> - any device type may or may not support the admin vq >>>>>>>>>>> - if the device type wants to be able to accommodate the admin = vq, it >>>>>>>>>>> also needs to specify where it shows up when the feature = is negotiated >>>>>>>>>>> >>>>>>>>>>> Do we expect that eventually all device types will need to supp= ort the >>>>>>>>>>> admin vq (if some use case comes along that will require all de= vices to >>>>>>>>>>> participate, for example?) >>>>>>>>>> I suspect yes. And that's one of the reasons why I'd rather we h= ad a >>>>>>>>>> device independent way to locate the admin queue. There are less >>>>>>>>>> transports than device types. >>>>>>>>> So, do we want to bite the bullet now and simply say that every d= evice >>>>>>>>> type has the admin vq as the last vq if the feature is negotiated= ? >>>>>>>>> Should be straightforward for the device types that have a fixed = number >>>>>>>>> of vqs, and doable for those that have a variable amount (two dev= ice >>>>>>>>> types are covered by this series anyway.) I think we need to put = it with >>>>>>>>> the device types, as otherwise the numbering of virtqueues could = change >>>>>>>>> in unpredictable ways with the admin vq off/on. >>>>>>>> Well that only works once. The next thing we'll need we won't be a= ble to >>>>>>>> make the last one ;) So I am inclined to add a per-transport field= that >>>>>>>> gives the admin queue number. >>>>>>> Technically, there's no need to use the same namespace for admin >>>>>>> virtqueue if it has a dedicated notification area. If we go this wa= y, >>>>>>> we can simply use 0 as queue index for admin virtqueue. >>>>>> Or we can use index 0xFFFF for admin virtqueue for compatibility. >>>>> I think I'd prefer a register with the #. For example we might want >>>>> to limit the # of VQs in order to pass extra data with the kick write= . >>>> So you are suggesting adding a new cfg_type (#define >>>> VIRTIO_PCI_CAP_ADMIN_CFG 10) ? >>>> >>>> that will look something like: >>>> >>>> struct virtio_pci_admin_cfg { >>>> >>>> =C2=A0=C2=A0=C2=A0 le32 queue_index; /* read only for the driver */ >>>> >>>> =C2=A0=C2=A0=C2=A0 le16 queue_size; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le16 queue_msix_vector; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le16 queue_enable; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le16 queue_notify_off; /* read-only for driver */ >>>> =C2=A0=C2=A0=C2=A0 le64 queue_desc; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le64 queue_driver; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le64 queue_device; /* read-write */ >>>> =C2=A0=C2=A0=C2=A0 le16 queue_notify_data; /* read-only for driver *= / >>>> =C2=A0=C2=A0=C2=A0 le16 queue_reset; /* read-write */ >>>> >>>> }; >>>> >>>> instead of re-using the struct virtio_pci_common_cfg ? >>>> >>>> >>>> or do you prefer extending the struct virtio_pci_common_cfg with "le16 >>>> admin_queue_index; /* read only for the driver */ ? >>> The later. Other transports will need this too. >>> >>> >>> Cornelia has another idea which is that instead of >>> adding just the admin queue register to all transports, >>> we instead add a misc_config structure to all >>> transports. Working basically like device specific config, >>> but being device independent. For now it will only have >>> a single le16 admin_queue_index register. >>> >>> For PCI we would thus add it with VIRTIO_PCI_CAP_MISC_CFG >>> >>> The point here is that we are making it easier to add >>> more fields just like admin queue index in the future. >> OK. >> >> #define VIRTIO_PCI_CAP_MISC_CFG 10 >> >> and >> >> struct virtio_pci_misc_cfg { >> le16 admin_queue_index; /* read-only for driver */ >> }; >> >> Is agreed by all for V3 ? instead of the net and blk AQ index definition= s. > We need to add it to MMIO and CCW I guess too. > > This is Cornelia's idea, we'll need her response. Ok, Cornelia please review the above. For V3 I'll prepare the PCI transport and we can work off list on the=20 MMIO and CCW patches for V4. sounds reasonable ? > > >>> >>>>>>> Thanks >>>>>>> >>>>>>>> Another advantage to this approach is that >>>>>>>> we can make sure admin queue gets a page by itself (which can be g= ood if >>>>>>>> we want to allow access to regular vqs but not to the admin queue = to >>>>>>>> guest) even if regular vqs share a page. Will help devices use les= s >>>>>>>> memory space. >>>>>>>> >>>>>>>> -- >>>>>>>> MST >>>>>>>> This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscribe@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org List help: virtio-comment-help@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lis= ts Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/