[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [PATCH v6 4/4] transport-pci: Introduce group legacy group member config region access
> From: Michael S. Tsirkin <mst@redhat.com> > Sent: Monday, June 19, 2023 12:17 PM > I'd love to just put this stuff in a separate tex document and just link there from > the generic thing. > Or if it's too hard, please create new sections for this stuff. But in this case each > of these sections has to start with repeating the description when things apply. > E.g. "when using legacy interface" > when using commands "A B C to access" etc etc. > > I wrote a bunch of comments below. > I will stick to them as most things are taken out to generic section anyway in patch 3. Only PCI transport part stays here. I will move some more text to generic section. > > +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]} > > +PCI VFs do not support the I/O space BAR. Due to this limitation, a > > +PCI VF hardware device that supports legacy interfaces cannot be > > +attached as passthrough device to the guest virtual machine. > > That's not the only reason. Let's keep things generic and start by saying > something like "Some configurations with a need to support legacy drivers can > not implement a transitional device. For example .... ". > I will avoid For example part and move this to generic section. > > + > > +To have a transitional or legacy virtio device in the guest virtual > > +machine, a PCI PF device as a group owner may > > may -> can outside conformance statements. > Ack. > > support accessing its group member > > Pls keep it generic. A group owner .... > Ack > > > +device's legacy configuration region using the group administration > > +commands listed at \ref{sec:Basic Facilities of a Virtio Device / > > +Device groups / Group administration commands / Legacy Interfaces: legacy > configuration access commands}. > > +These legacy configuration access commands are transported via the > > +administration virtqueue. > > are these called legacy configuration access commands then? > why do you introduce this term? it does not look like it's used anywhere else. > It is used and referred PCI section " Legacy Interfaces: Group member device Configuration Region Access". > > I would prefer it if you just use the term "legacy interface" > it is well defined and widely used in the spec. > It is "legacy interface", with suffix for group member config access. In future, there may be a section for other things than config access. > again leave virtqueue out of it pls we'll have other ways down the road. what > matters is that they are transferred to a group member by sending legacy > configuration access commands to the group owner. > Ack. It is theory of operation, not a normative. Tomorrow if other than aq exists, one will write as "AQ or xyz".. But fine to me to reduce changes from 216 to less lines. :) > > > This mechanism enables minimal involvement of the > > +hypervisor software only for the purpose of the legacy configuration region > access. > > Leave marketing out of it pls, kill last sentence. > Huh, I don't see theory of operation and scope of the hypervisor as marketing at all. I will remove as it makes this patch shorter. > > > > + > > +When a guest virtual machine requests legacy configuration and device > > +specific regions > > wait so now it's legacy configuration and device specific regions and these two > are distinct? > > > of group member device, a hypervisor software accesses > > +it using an administration virtqueue > > please talk of administration commands. virtqueue does not matter at all. > Ack. > > on behalf of the guest virtual machine. > > > > To put it in our terms, something like this: > when a legacy driver accesses a member device of > a group using the > legacy interface, a modern driver can intercept > the access and forward it to the owner device. > I will not mention "modern driver" as it has zero reference in spec and don't want to bring Linux terms here. "the driver can intercept" is enough. > > > > + > > +Even though virtqueue driver notifications can be communicated > > +through administration virtqueue, if the group member device support > > +such > > supports? > Ack. > > +notifications using a memory-mapped operation, such driver > > +notifications are sent using a group member device defined notification > region. > > what does "are sent" imply here? is there a matching conformance statement? > who sends them? > It has the same meaning as "Driver notifications" section first line has. Isnt it implicit that driver notifications are sent by driver based on "Driver notifications" section? > > + > > +The group owner device > > this is the 1st you mention any group or its owner. > > > should not expose PCI BAR0 > > Not PCI BAR0. That is in header. You mean VF BAR0. > I will rewrite it as, The group owner device should not expose PCI BAR0 in the PCI SR-IOV extended capability for the group member PCI VF devices when it prefers to support legacy interface for legacy configuration access using this group owner. > > for the group member > > +devices when it prefers to support legacy interface for legacy > > +configuration access using its group owner. > > don't use should outside conformance > What should be used instead of "should"? > > I think this specific case actually should be more specific. > Something like: > > - For PCI SRIOV groups, when group owner device implements commands > A,B,C the group owner's VF BAR 0 is hardwired to 0 > in its PCI SRIOV capability. > I wrote it slightly differently above. > > > +This facilitates hypervisor software to operate with least amount of > > +complexities > > complexity is its own plural > > > to emulate the legacy interface I/O space BAR and passthrough > > +other PCI BARs and PCI device capabilities to the guest virtual > > +machine without any translation. > > + > > +The group member device should not expose PCI BAR0 in various PCI > capabilities. > > + > > +\devicenormative{\subsubsection}{Legacy Interfaces Requirements: > > +Group Member Device Legacy Configuration Access}{Virtio Transport > > +Options / Virtio Over PCI Bus / Legacy Interfaces Requirements: Group > > +Member Device Legacy Configuration Access} > > + > > +When a PCI SR-IOV group owner device supports > > +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ, > > +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, > > +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ, > > +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE commands, the group > owner > > +device SHOULD NOT expose PCI BAR0 in the SR-IOV Extended capability. > > +This is to facilitate the software to emulate I/O region BAR0 for supporting > the legacy interface. > > not PCI BAR0 - VF BAR0. Check the PCI spec you can not "not expose it". if you > want to register can be "unimplemented". > Base Address registers are hardwired to zero > > But it is better to be specific on what should happen. hardwire VF BAR0 to 0, > right? > Hardwire to zero and not expose is same thing to me. > > > + > > +\drivernormative{\subsubsection}{Legacy Interfaces Requirements: > > +Group Member Device Legacy Configuration Access}{Virtio Transport > > +Options / Virtio Over PCI Bus / Legacy Interfaces Requirements: Group > > +Member Device Legacy Configuration Access} > > + > > +The driver SHOULD NOT emulate I/O region BAR0 if a device group > > +member exposes a PCI BAR0. > > what does "emulate" mean here? which driver it is? > I will add the line to theory of operation, but I recollect "emulate" line came from you. > > diff --git a/transport-pci.tex b/transport-pci.tex index > > a5c6719..3647485 100644 > > --- a/transport-pci.tex > > +++ b/transport-pci.tex > > @@ -541,6 +541,8 @@ \subsubsection{Notification structure > > layout}\label{sec:Virtio Transport Options struct virtio_pci_notify_cap { > > struct virtio_pci_cap cap; > > le32 notify_off_multiplier; /* Multiplier for > > queue_notify_off. */ > > + u8 legacy_q_notify_supported; > > I am still mulling whether VIRTIO_PCI_CAP_NOTIFY_CFG would be better. > > > > + u8 reserved[3]; > > > align with spaces pls. > Ack. > > > }; > > \end{lstlisting} > > > > @@ -560,6 +562,14 @@ \subsubsection{Notification structure > > layout}\label{sec:Virtio Transport Options the same Queue Notify address for > all queues. > > \end{note} > > > > +\field{legacy_q_notify_supported} when set to 1, > > > Do you mean something like: > When \field{legacy_q_notify_supported} this indicates that ... > Yes will fix. > > indicates that the device > > +supports legacy queue notifications at this notification location. > > And what location? this refers to what? the location described by this capability > maybe? > Ack. > > More specifically, assume a transitional device (with an IO BAR as normal). Does > presence of this flag in the capability imply we can use a legacy queue but use > this notification with this capability? I don't know how this combination can be even possible by the device other than a buggy software who will do both. > I worry that if we allow that, it opens floodgates of people who are too lazy to > upgrade to 1.0 but just hack this notification in there. > Don't understand the concern. If a device has transitional device, it is 1.0 device supporting legacy with IOBAR and newer interface. So... > > > > > > Legacy Queue > > +Notification address is derived within a BAR for a virtqueue: > > + > > +\begin{lstlisting} > > + cap.offset > > +\end{lstlisting} > > + > > \devicenormative{\paragraph}{Notification capability}{Virtio > > Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification > capability} The device MUST present at least one notification capability. > > > > I don't get what this is saying. I *think* you really mean is that > queue_notify_off is 0. > Please refer to the current notification address definition. Above is written in similar way where it doesnt need to describe queue_notify_off. > > > @@ -596,6 +606,14 @@ \subsubsection{Notification structure > > layout}\label{sec:Virtio Transport Options cap.length >= > > queue_notify_off * notify_off_multiplier + 4 \end{lstlisting} > > > > +\paragraph{Legacy Interfaces: Notification > > +capability}\label{par:Virtio Transport Options / Virtio Over PCI Bus > > +/ PCI Device Layout / Legacy Interfaces: Notification capability} > > + > > +The device SHOULD set \field{legacy_q_notify_supported} > > why SHOULD and not MUST here - if driver checks for it then it won't work > without no? > If device doesn't want to support queue notifications, it has ability to set to 0. > > when the device > > +notification location supports legacy driver notifications. > > legacy driver notifications is ambigous. > what you want here and elsewhere is something like "when used through a > legacy interface, device supports driver > Ack.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]