[virtio-dev] Re: [virtio-comment] RE: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access
On Fri, Jul 07, 2023 at 10:12:35AM +0200, Cornelia Huck wrote: > [Also, I notice that the discussion seems to get bogged down in tiny > details, fundamental statements, or a mixture of both. It might > sometimes be helpful to just step back, re-read the original proposal, > and only then answer. Otherwise, this leads to frustration for everyone > involved.] I feel in this instance we are actually good. All that's left is small details that's why we are discussing them. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access
On Thu, Jul 06 2023, Parav Pandit wrote: >> From: Michael S. Tsirkin >> Sent: Thursday, July 6, 2023 4:42 PM > > [..] >> > Can we please avoid this over engineering? >> > Interface has the doors open for driver to make wise decision depending on >> its environment. >> >> what if driver can access both with the same ease? this is the case that >> bothers >> me and I think it's practical since it will be common on linux. > > Then Linux can say my preference is order, so it picks member. Let's step back and consider what our goal is here: to provide a spec that someone wanting to implement a driver can follow and end up with a driver that works with devices that adhere to this spec. I don't think expecting the driver to make "wise descisions" is helpful for the person wanting to implement a driver. "Do this, and in case this does not work in your environment, do that" seems much more helpful, and still gives enough flexibility to cover different environments. In short, make things simple for the consumer of the spec. [Also, I notice that the discussion seems to get bogged down in tiny details, fundamental statements, or a mixture of both. It might sometimes be helpful to just step back, re-read the original proposal, and only then answer. Otherwise, this leads to frustration for everyone involved.] - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access
> From: Michael S. Tsirkin
> Sent: Thursday, July 6, 2023 3:59 PM
> > Hardwaring BAR0 is in BAR area of the VF.
> > But virtio pci capabilities without this can still report bar 0 and virtio
> capabilities.
>
> How do they do this?
>
Device must not do it, but a broken device can report pci capabilities as
garbage.
I will skip writing about it.
> > Ofcourse, it is a broken device.
> > But skipping this line imply that "because VF BAR0 is hardwired", this
> metadata in pci capability must not expose it.
> >
> > Why not write it extra thing instead of implying it?
> > We already wrote few duplicate things to make reader life easier.
>
> If you really want to go ahead, but prefix it with "In consequence" and do not
> start a new paragraph, so reader knows it's not an extra requirement.
>
> So I started writing it using correct grammar and spec terminology:
>
> In consequence, non of the group member devices has BAR0, and
> in particular none of the virtio structure capabilities
> of a member device has \field{bar} with the value of 0.
>
>
> However, this actually is kind of wrong (and so is your text). Not all caps
> have a
> RO bar value. virtio_pci_cfg_cap has bar that is RW by driver. So if we are
> going
> this route we also need to explain that it's true for all caps except
> virtio_pci_cfg_cap. And for virtio_pci_cfg_cap driver is not allowed to
> write 0
> there.
>
> Frankly too much trouble but if you want to, keep trying.
What I wrote still holds true because device wont have BAR0 and driver writing
there is ignored.
But it is some weird broken device who expose PCI capabilities, so I am going
to skip writing this.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access
On Thu, Jul 06, 2023 at 07:07:42PM +, Parav Pandit wrote:
> > From: virtio-comm...@lists.oasis-open.org > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Thursday, July 6, 2023 3:01 PM
>
> > > +\begin{lstlisting}
> > > +struct virtio_pci_legacy_notify_region {
> > > +u8 owner; /* When set to 1, notification region is of the
> > > +owner device */
> >
> > I propose we rename this to flags:
> > 0x0 - end of list (driver should ignore following values until end of
> > list)
> > 0x1 - owner
> > 0x2 - member
> > other values - reserved
> >
>
> > and prescribe that driver ignores any reserved values.
> >
> > basically we are following how the virtio capabilities work.
> >
> Looks good. Will change it.
>
> > > +The group owner device hardwires VF BAR0 to zero in the SR-IOV
> > > +Extended capability.
> > > +
> > > +The group member device does not use PCI BAR0 in all the Virtio PCI
> > > +capabilities listed in section \ref{sec:Virtio Transport Options /
> > > Virtio Over
> > PCI Bus / Virtio Structure PCI Capabilities}.
> >
> > Just drop this last one. How can they use it if there's no VF BAR0?
> >
> Hardwaring BAR0 is in BAR area of the VF.
> But virtio pci capabilities without this can still report bar 0 and virtio
> capabilities.
How do they do this?
> Ofcourse, it is a broken device.
> But skipping this line imply that "because VF BAR0 is hardwired", this
> metadata in pci capability must not expose it.
>
> Why not write it extra thing instead of implying it?
> We already wrote few duplicate things to make reader life easier.
If you really want to go ahead, but prefix it with "In consequence" and
do not start a new paragraph, so reader knows it's not an extra
requirement.
So I started writing it using correct grammar and spec terminology:
In consequence, non of the group member devices has BAR0, and
in particular none of the virtio structure capabilities
of a member device has \field{bar} with the value of 0.
However, this actually is kind of wrong (and so is your text). Not all
caps have a RO bar value. virtio_pci_cfg_cap has bar that is RW by
driver. So if we are going this route we also need to explain that it's
true for all caps except virtio_pci_cfg_cap. And for virtio_pci_cfg_cap
driver is not allowed to write 0 there.
Frankly too much trouble but if you want to, keep trying.
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access
> From: virtio-comm...@lists.oasis-open.org open.org> On Behalf Of Michael S. Tsirkin
> Sent: Thursday, July 6, 2023 3:01 PM
> > +\begin{lstlisting}
> > +struct virtio_pci_legacy_notify_region {
> > +u8 owner; /* When set to 1, notification region is of the
> > +owner device */
>
> I propose we rename this to flags:
> 0x0 - end of list (driver should ignore following values until end of
> list)
> 0x1 - owner
> 0x2 - member
> other values - reserved
>
> and prescribe that driver ignores any reserved values.
>
> basically we are following how the virtio capabilities work.
>
Looks good. Will change it.
> > +The group owner device hardwires VF BAR0 to zero in the SR-IOV
> > +Extended capability.
> > +
> > +The group member device does not use PCI BAR0 in all the Virtio PCI
> > +capabilities listed in section \ref{sec:Virtio Transport Options / Virtio
> > Over
> PCI Bus / Virtio Structure PCI Capabilities}.
>
> Just drop this last one. How can they use it if there's no VF BAR0?
>
Hardwaring BAR0 is in BAR area of the VF.
But virtio pci capabilities without this can still report bar 0 and virtio
capabilities.
Ofcourse, it is a broken device.
But skipping this line imply that "because VF BAR0 is hardwired", this metadata
in pci capability must not expose it.
Why not write it extra thing instead of implying it?
We already wrote few duplicate things to make reader life easier.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
