[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [virtio-comment] [PATCH v10 02/10] admin: introduce device group and related concepts
On 2/13/2023 4:12 PM, Michael S. Tsirkin wrote:
How to detect whether VF_enable set to 0 or 1, pulling is not a good idea, and do we wantOn Mon, Feb 13, 2023 at 12:49:44AM +0200, Max Gurtovoy wrote:On 12/02/2023 22:19, Michael S. Tsirkin wrote:On Sun, Feb 12, 2023 at 04:34:09PM +0200, Max Gurtovoy wrote:On 12/02/2023 15:15, Michael S. Tsirkin wrote:On Sun, Feb 12, 2023 at 02:10:29PM +0200, Max Gurtovoy wrote:On 09/02/2023 21:58, Michael S. Tsirkin wrote:On Thu, Feb 09, 2023 at 07:47:56PM +0200, Max Gurtovoy wrote:On 09/02/2023 17:22, David Edmondson wrote: On Thursday, 2023-02-09 at 10:13:46 -05, Michael S. Tsirkin wrote: On Thu, Feb 09, 2023 at 03:00:58PM +0000, David Edmondson wrote: On Thursday, 2023-02-09 at 07:13:36 -05, Michael S. Tsirkin wrote: Each device group has a type. For now, define one initial group: SR-IOV type - PCI SR-IOV virtual functions (VFs) of a given PCI SR-IOV physical function (PF). This group may contain one or more virtio devices. The specification says zero or more generally and nothing to refine that for SR-IOV groups. Well it says: (SR-IOV) physical function (PF) device as the owner and includes all its SR-IOV virtual functions (VFs) as members (see \hyperref[intro:PCIe]{[PCIe]}). how can that be zero? If I remove all of the VFs of a PF, does the group implicitly disappear? Enable/Disable SR-IOV are dynamic operations controlled by NumVFs register.Are you sure? Not by VF Enable?Right. It's a combination of the VF_Enable and numVfs. ... "The NumVFs field defines the number of VFs that are enabled when VF Enable is Set in the associated PF."We can mention that a group include all virtual functions indicated by TotalVFs RO register.TotalVFs indicates the maximum number of VFs that could be associated with the PF but not all of these exist. If we use this we get into issues of discovering whether VFs can actually be used. I'd rather avoid that.This group exist as long as the owner PF supports SR-IOV and has admin capabilities.SRIOV spec says: NumVFs controls the number of VFs that are visible. And one can not play with NumVFs dynamically: NumVFs may only be written while VF Enable is Clear. If NumVFs is written when VF Enable is Set, the results are undefined. one has to kill all VFs then enable them all again. As long as we are doing that, we can just unload the driver.what driver to unload ? Not sure I'm following..Basically I am saying that if VFs are disabled this destroys all VFs so it is not clear that it makes sense to keep some of the configuration for previous VFs around. My idea is to say that reset is required to disable VFs.which reset ? VFs disabled/enabled using VF_enable PCI register. Lets follow that please. Lets define what is happening during VF_enable = 1 and VF_enable = 0. On VF_enable = 0 --> 1 transition the corresponding device_group, that is owned by the PF, grows and contains NumVFs members with initial features and resources. On VF_enable = 1 --> 0 transition the corresponding device_group, that is owned by the PF, shrinks and contains 0 members and all the dynamic resources are back to the owner.Maybe, it's a possible approach. As current patch set has no resources I feel we can leave this part to a patch on top just so that this one can move forward while we have a more specific example to work from.we can differ the resources part but lets add the part that the group becomes empty/non-empty according to the disable/enable of VF_enable register as mentioned above.Hmm. Another option is VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP when VF_enable is clear - since this disables the whole group not a specific member. This allows distinguishing between id > NumVFs and VF_enable == 0 so it seems like a nicer option to me.
SRIOV cap to notify the admin vq on changes? Thanks
So I would say we should just ask that VF Enable is set before poking at admin vq, and prohibit clearing VF Enable.Sending admin commands shouldn't be restricted and should be allowed always. Device should react to incoming command. We can say that an SR-IOV group is empty while the VF enable is unset and is not empty when it's set. The size of the non empty SR-IOV group is equal to the numVFs and the VF index is equal to the member identifier in this group. I had this definition in my patches. Sending admin command to a non existing member will result in an ERROR VIRTIO_ADMIN_STATUS_Q_INVALID_MEM.just an example: - assign resource X to VF 16 - set NumVFs to 8 Does VF 16 keep resource X? I note that resource X can not be taken back because VF 16 is not accessible.all dynamic resources should return to the owner when VF_enable moves 1 --> 0It's a possible approach but my question would be whether we need a transport independent way to do this through VQ. And if we do it's annoying to have multiple ways.We are defining the existence of the SR-IOV group. Also, the approach of sending commands to a non-existing member is valid for any group type and should return error VIRTIO_ADMIN_STATUS_Q_INVALID_MEM in that case. why do we need more ways ?Sorry I mean a transport independent way to destroy a group as a whole.For sure, all this is solvable but I'd rather just forbid changes to VF enable for now.how we can forbid it ? setting this register is not related to virtio.We can ask driver to write 0 to device reset before changing VF enable to 0. The advantage is that this kind of reset by definition frees up all resources.why do we need to reset the device ? It is fully functional and should be able to run perfectly fine without any need to reset. The PF device might have a very big role in the hypervisor, so restricting it in this way doesn't sounds mandatory. Controlling the SR-IOV group using SR-IOV register enable/disable and NumVFs used as the number of the group size sounds simple from Spec and implementation POV.Simple, however - only works for SRIOV - also limited, for example it's impossible to report an error I would rather not decide now until we have a use-case.There's also the concern of VF migration under MRIOV where VFs might not exit. I'm guessing trying to include that right now is overthinking. So maybe this should mention that VF Migration Capable should be clear. I will add all this to the spec.How about the converse? Each device within a group has a unique identifier. This identifier is the group member identifier. Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer to a new device type or a new transport type. However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter. I did my best to minimize transport-specific text. Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- admin.tex | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ content.tex | 2 ++ 2 files changed, 51 insertions(+) create mode 100644 admin.tex diff --git a/admin.tex b/admin.tex new file mode 100644 index 0000000..2bc7322 --- /dev/null +++ b/admin.tex @@ -0,0 +1,49 @@ +\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups} + +It is occasionally useful to have a device control a group of +other devices. Terminology used in such cases: + +\begin{description} +\item[Device group] + or just group, includes zero or more devices. +\item[Owner device] + or owner, the device controlling the group. +\item[Member device] + a device within a group. The owner device itself is not + a member of the group. +\item[Member identifier] + each member has this identifier, unique within the group + and used to address it through the owner device. +\item[Group type identifier] + specifies what kind of member devices there are in a + group, how is the member identifier is interpreted + and what kind of control the owner has. + A given owner can control a single group of a given type, + thus the type and the owner together identify the group. + \footnote{Even though some group types only support + specific transports, group type identifiers + are global rather than transport-specific - + we don't expect a flood of new group types.} +\end{description} + +The following group types, and their identifiers, are currently specified): +\begin{description} +\item[SR-IOV group type (0x1)] +This device group has a PCI Single Root I/O Virtualization +(SR-IOV) physical function (PF) device as the owner and includes +all its SR-IOV virtual functions (VFs) as members (see +\hyperref[intro:PCIe]{[PCIe]}). + +The PF device itself is not a member of the group. + +The group type identifier for this group is 0x1. + +A member identifier for this group can have a value 0x1 to 0xFFFF +and equals the SR-IOV VF number of the member device (see +\hyperref[intro:PCIe]{[PCIe]}). + +Both owner and member devices for this group type use the Virtio +PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}). +\end{description} + + diff --git a/content.tex b/content.tex index 0c2d917..ffe45c4 100644 --- a/content.tex +++ b/content.tex @@ -491,6 +491,8 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo types. It is RECOMMENDED that devices generate version 4 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}. +\input{admin.tex} + \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation} We start with an overview of device initialization, then expand on the -- Tell her I'll be waiting, in the usual place.--------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]