On Wed, Jun 21, 2023 at 08:22:58PM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <[email protected]>
> > Sent: Wednesday, June 21, 2023 4:06 PM
>
> [..]
> > > > 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.
Maybe a (non legacy) driver can intercept? Would that be acceptable?
Just to clarify the confusion above.
> >
> > Good point but since you say "a legacy driver" then "the driver" seems to
> > refer
> > to exactly that. How about:
> >
> > when a legacy member device driver accesses a member device of
> > a group using the
> > legacy interface, an owner device driver can intercept
> > the access and forward it to the owner device.
> >
> Above is not correct.
>
> We have 3 drivers in picture.
> 1. guest driver
this is legacy driver, so easy
> 2. hypervisor level member device driver
this is just for notifications, optionally, yes?
> 3. group owner device driver
>
> Trying to write without introducing guest and hypervisor term,
>
> A group owner device driver provides the service to access member device's
> configuration region.
> A member device driver avail this service when it wants to access the member
> device's configuration region.
I agree, it's getting complicated.
>
> > > 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.
> >
> >
> > This seems to ignore all my comments.
> >
> I replied after that, probably missed in exchanges.
>
> How about:
> The group owner device MUST hardwire PCI BAR0 in the PCI SR-IOV extended
> capability for the group member PCI VF devices when it supports legacy
> configuration access commands.
>
better but it's not a PCI BAR0. let's add link to sriov spec,
and name it VF BAR0 same as in that spec.
> >
> >
> > > > > 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"?
> >
> > Depends on context, generally we just say what it does. E.g. "VF BAR0 is
> > hardwired to zero".
> >
> Ok.
>
> >
> >
> > > >
> > > > 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.
> >
> > I don't see why what you wrote is any better than what you had to be frank.
> > You
> > are coming up with our own terminology instead of just using what's in the
> > SR
> > IOV spec. Please don't.
> >
> > > >
> > > > > +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.
> >
> > Maybe, though previously you said it just means not put any capabilities
> > there.
> > More importantly I don't see expose or not expose anywhere in the PCI or
> > SRIOV spec. When spec wants to say how not to have a given BAR it says
> > exactly
> > hardwired to zero.
> >
> Hardwired to zero is fine, done above now.
>
> > > >
> > > > > +
> > > > > +\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.
> >
> > I generally am fine with using this term but we need to then define it
> > before
> > use.
> >
> > This specific thing I would just drop.
> >
> > Instead of wasting time just say device MUST hardwire VF BAR0 to zero as
> > opposed to SHOULD, and we do not need to worry about how drivers recover. If
> > they want to they will validate, if they don't then they won't.
> >
> This limitation seems fine to me.
> Will drop.
>
> >
> > > > > 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...
> >
> >
> > if we are switching to admin commands for this, then let's not argue about
> > it.
> >
> Ok.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
