Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Tiwei Bie]

On Wed, Jun 20, 2018 at 05:02:26PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 20, 2018 at 09:40:50PM +0800, Tiwei Bie wrote:
> > On Wed, Jun 20, 2018 at 06:31:16AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> > > > There will be hardware virtio devices in the future, which
> > > > require drivers to use the barriers suitable for I/O devices,
> > > > compared with software virtio devices which just require
> > > > drivers to use the barriers suitable for CPU cores.
> > > > 
> > > > To fix the ordering issue for hardware virtio devices, add
> > > > a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> > > > will use the barriers suitable for I/O devices.
> > > > 
> > > > Signed-off-by: Tiwei Bie 
> > > > Reviewed-by: Stefan Hajnoczi 
> > > 
> > > So some people noticed that some systems also
> > > require a cache flush around DMA to real devices.
> > > 
> > > Shouldn't we add that as part of VIRTIO_F_IO_BARRIER?
> > > 
> > > Also rename it?
> > > 
> > 
> > Do we just need to add cache flush? One thing
> > worried me is that I'm not sure how to cover
> > all the possible cases..
> 
> It's a good point. Someone suggested "_I_AM_A_REAL_PCI_DEVICE".  Joking
> aside, maybe say that it's not running on another CPU
> so must not apply optimizations that assume another CPU?

Good idea! I'll try to compose a RFC ASAP.

> 
> IOMMU is somewhat special as it's used for protection
> within guests.
> 
> 
> > > 
> > > 
> > > Others also noticed that there are systems where accesses
> > > include some kind of translation e.g. some high bits
> > > in the address are set to signal access properties.
> > > Should that be included in VIRTIO_F_IOMMU_PLATFORM?
> > > 
> > 
> > It's my first time to know such systems. I don't
> > know any details about it..
> > 
> > Best regards,
> > Tiwei Bie
> > 
> 
> See discussion around
>   virtio: Add platform specific DMA API translation for virito devices
> 
> you can try poking Christoph Hellwing and Benjamin Herren about
> details.

Got it. Thanks!

Best regards,
Tiwei Bie

> 
> 
> > 
> > > 
> > > 
> > > 
> > > > ---
> > > > v2 -> v3:
> > > > - Update the feature bits allocation (Stefan);
> > > > 
> > > > v1 -> v2:
> > > > - Rebase to the latest spec (MST);
> > > > - Use a smaller textwidth (according to _vimrc);
> > > > 
> > > > RFC -> v1:
> > > > - Use plural (Stefan);
> > > > - Add more details (Stefan);
> > > > 
> > > >  content.tex | 20 ++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index 7a92cb1..95c243f 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -95,10 +95,10 @@ Feature bits are allocated as follows:
> > > >  \begin{description}
> > > >  \item[0 to 23] Feature bits for the specific device type
> > > >  
> > > > -\item[24 to 33] Feature bits reserved for extensions to the queue and
> > > > +\item[24 to 36] Feature bits reserved for extensions to the queue and
> > > >feature negotiation mechanisms
> > > >  
> > > > -\item[34 and above] Feature bits reserved for future extensions.
> > > > +\item[37 and above] Feature bits reserved for future extensions.
> > > >  \end{description}
> > > >  
> > > >  \begin{note}
> > > > @@ -5348,6 +5348,15 @@ Descriptors} and \ref{sec:Packed Virtqueues / 
> > > > Indirect Flag: Scatter-Gather Supp
> > > >\item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> > > >that all buffers are used by the device in the same
> > > >order in which they have been made available.
> > > > +  \item[VIRTIO_F_IO_BARRIER(36)] This feature indicates
> > > > +  that the device needs the driver to use the barriers
> > > > +  suitable for hardware devices.  Some transports require
> > > > +  barriers to ensure devices have a consistent view of
> > > > +  memory.  When devices are implemented in software a
> > > > +  weaker form of barrier may be sufficient and yield
> > > > +  better performance.  This feature indicates whether
> > > > +  a stronger form of barrier suitable for hardware
> > > > +  devices is necessary.
> > > >  \end{description}
> > > >  
> > > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature 
> > > > Bits}
> > > > @@ -5363,6 +5372,10 @@ addresses to the device.
> > > >  
> > > >  A driver SHOULD accept VIRTIO_F_RING_PACKED if it is offered.
> > > >  
> > > > +A driver SHOULD accept VIRTIO_F_IO_BARRIER if it is offered.
> > > > +If VIRTIO_F_IO_BARRIER has been negotiated, a driver MUST use
> > > > +the barriers suitable for hardware devices.
> > > > +
> > > >  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature 
> > > > Bits}
> > > >  
> > > >  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate 
> > > > further
> > > > @@ -5376,6 +5389,9 @@ accepted.
> > > >  If VIRTIO_F_IN_ORDER has been negotiated, a device MUST use
> > > >  buffers in the same order in which they have been available.
> > > >  
> 

Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Michael S. Tsirkin]

On Wed, Jun 20, 2018 at 09:40:50PM +0800, Tiwei Bie wrote:
> On Wed, Jun 20, 2018 at 06:31:16AM +0300, Michael S. Tsirkin wrote:
> > On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> > > There will be hardware virtio devices in the future, which
> > > require drivers to use the barriers suitable for I/O devices,
> > > compared with software virtio devices which just require
> > > drivers to use the barriers suitable for CPU cores.
> > > 
> > > To fix the ordering issue for hardware virtio devices, add
> > > a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> > > will use the barriers suitable for I/O devices.
> > > 
> > > Signed-off-by: Tiwei Bie 
> > > Reviewed-by: Stefan Hajnoczi 
> > 
> > So some people noticed that some systems also
> > require a cache flush around DMA to real devices.
> > 
> > Shouldn't we add that as part of VIRTIO_F_IO_BARRIER?
> > 
> > Also rename it?
> > 
> 
> Do we just need to add cache flush? One thing
> worried me is that I'm not sure how to cover
> all the possible cases..

It's a good point. Someone suggested "_I_AM_A_REAL_PCI_DEVICE".  Joking
aside, maybe say that it's not running on another CPU
so must not apply optimizations that assume another CPU?

IOMMU is somewhat special as it's used for protection
within guests.


> > 
> > 
> > Others also noticed that there are systems where accesses
> > include some kind of translation e.g. some high bits
> > in the address are set to signal access properties.
> > Should that be included in VIRTIO_F_IOMMU_PLATFORM?
> > 
> 
> It's my first time to know such systems. I don't
> know any details about it..
> 
> Best regards,
> Tiwei Bie
> 

See discussion around
virtio: Add platform specific DMA API translation for virito devices

you can try poking Christoph Hellwing and Benjamin Herren about
details.


> 
> > 
> > 
> > 
> > > ---
> > > v2 -> v3:
> > > - Update the feature bits allocation (Stefan);
> > > 
> > > v1 -> v2:
> > > - Rebase to the latest spec (MST);
> > > - Use a smaller textwidth (according to _vimrc);
> > > 
> > > RFC -> v1:
> > > - Use plural (Stefan);
> > > - Add more details (Stefan);
> > > 
> > >  content.tex | 20 ++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 7a92cb1..95c243f 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -95,10 +95,10 @@ Feature bits are allocated as follows:
> > >  \begin{description}
> > >  \item[0 to 23] Feature bits for the specific device type
> > >  
> > > -\item[24 to 33] Feature bits reserved for extensions to the queue and
> > > +\item[24 to 36] Feature bits reserved for extensions to the queue and
> > >feature negotiation mechanisms
> > >  
> > > -\item[34 and above] Feature bits reserved for future extensions.
> > > +\item[37 and above] Feature bits reserved for future extensions.
> > >  \end{description}
> > >  
> > >  \begin{note}
> > > @@ -5348,6 +5348,15 @@ Descriptors} and \ref{sec:Packed Virtqueues / 
> > > Indirect Flag: Scatter-Gather Supp
> > >\item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> > >that all buffers are used by the device in the same
> > >order in which they have been made available.
> > > +  \item[VIRTIO_F_IO_BARRIER(36)] This feature indicates
> > > +  that the device needs the driver to use the barriers
> > > +  suitable for hardware devices.  Some transports require
> > > +  barriers to ensure devices have a consistent view of
> > > +  memory.  When devices are implemented in software a
> > > +  weaker form of barrier may be sufficient and yield
> > > +  better performance.  This feature indicates whether
> > > +  a stronger form of barrier suitable for hardware
> > > +  devices is necessary.
> > >  \end{description}
> > >  
> > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > @@ -5363,6 +5372,10 @@ addresses to the device.
> > >  
> > >  A driver SHOULD accept VIRTIO_F_RING_PACKED if it is offered.
> > >  
> > > +A driver SHOULD accept VIRTIO_F_IO_BARRIER if it is offered.
> > > +If VIRTIO_F_IO_BARRIER has been negotiated, a driver MUST use
> > > +the barriers suitable for hardware devices.
> > > +
> > >  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > >  
> > >  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate 
> > > further
> > > @@ -5376,6 +5389,9 @@ accepted.
> > >  If VIRTIO_F_IN_ORDER has been negotiated, a device MUST use
> > >  buffers in the same order in which they have been available.
> > >  
> > > +A device MAY fail to operate further if VIRTIO_F_IO_BARRIER
> > > +is not accepted.
> > > +
> > >  \section{Legacy Interface: Reserved Feature Bits}\label{sec:Reserved 
> > > Feature Bits / Legacy Interface: Reserved Feature Bits}
> > >  
> > >  Transitional devices MAY offer the following:
> > > -- 
> > > 2.11.0
> > > 
> > > 
> > > -
> > > To 

Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Tiwei Bie]

On Wed, Jun 20, 2018 at 06:31:16AM +0300, Michael S. Tsirkin wrote:
> On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> > There will be hardware virtio devices in the future, which
> > require drivers to use the barriers suitable for I/O devices,
> > compared with software virtio devices which just require
> > drivers to use the barriers suitable for CPU cores.
> > 
> > To fix the ordering issue for hardware virtio devices, add
> > a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> > will use the barriers suitable for I/O devices.
> > 
> > Signed-off-by: Tiwei Bie 
> > Reviewed-by: Stefan Hajnoczi 
> 
> So some people noticed that some systems also
> require a cache flush around DMA to real devices.
> 
> Shouldn't we add that as part of VIRTIO_F_IO_BARRIER?
> 
> Also rename it?
> 

Do we just need to add cache flush? One thing
worried me is that I'm not sure how to cover
all the possible cases..

> 
> 
> Others also noticed that there are systems where accesses
> include some kind of translation e.g. some high bits
> in the address are set to signal access properties.
> Should that be included in VIRTIO_F_IOMMU_PLATFORM?
> 

It's my first time to know such systems. I don't
know any details about it..

Best regards,
Tiwei Bie


> 
> 
> 
> > ---
> > v2 -> v3:
> > - Update the feature bits allocation (Stefan);
> > 
> > v1 -> v2:
> > - Rebase to the latest spec (MST);
> > - Use a smaller textwidth (according to _vimrc);
> > 
> > RFC -> v1:
> > - Use plural (Stefan);
> > - Add more details (Stefan);
> > 
> >  content.tex | 20 ++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/content.tex b/content.tex
> > index 7a92cb1..95c243f 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -95,10 +95,10 @@ Feature bits are allocated as follows:
> >  \begin{description}
> >  \item[0 to 23] Feature bits for the specific device type
> >  
> > -\item[24 to 33] Feature bits reserved for extensions to the queue and
> > +\item[24 to 36] Feature bits reserved for extensions to the queue and
> >feature negotiation mechanisms
> >  
> > -\item[34 and above] Feature bits reserved for future extensions.
> > +\item[37 and above] Feature bits reserved for future extensions.
> >  \end{description}
> >  
> >  \begin{note}
> > @@ -5348,6 +5348,15 @@ Descriptors} and \ref{sec:Packed Virtqueues / 
> > Indirect Flag: Scatter-Gather Supp
> >\item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> >that all buffers are used by the device in the same
> >order in which they have been made available.
> > +  \item[VIRTIO_F_IO_BARRIER(36)] This feature indicates
> > +  that the device needs the driver to use the barriers
> > +  suitable for hardware devices.  Some transports require
> > +  barriers to ensure devices have a consistent view of
> > +  memory.  When devices are implemented in software a
> > +  weaker form of barrier may be sufficient and yield
> > +  better performance.  This feature indicates whether
> > +  a stronger form of barrier suitable for hardware
> > +  devices is necessary.
> >  \end{description}
> >  
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > @@ -5363,6 +5372,10 @@ addresses to the device.
> >  
> >  A driver SHOULD accept VIRTIO_F_RING_PACKED if it is offered.
> >  
> > +A driver SHOULD accept VIRTIO_F_IO_BARRIER if it is offered.
> > +If VIRTIO_F_IO_BARRIER has been negotiated, a driver MUST use
> > +the barriers suitable for hardware devices.
> > +
> >  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >  
> >  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate 
> > further
> > @@ -5376,6 +5389,9 @@ accepted.
> >  If VIRTIO_F_IN_ORDER has been negotiated, a device MUST use
> >  buffers in the same order in which they have been available.
> >  
> > +A device MAY fail to operate further if VIRTIO_F_IO_BARRIER
> > +is not accepted.
> > +
> >  \section{Legacy Interface: Reserved Feature Bits}\label{sec:Reserved 
> > Feature Bits / Legacy Interface: Reserved Feature Bits}
> >  
> >  Transitional devices MAY offer the following:
> > -- 
> > 2.11.0
> > 
> > 
> > -
> > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Michael S. Tsirkin]

On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> There will be hardware virtio devices in the future, which
> require drivers to use the barriers suitable for I/O devices,
> compared with software virtio devices which just require
> drivers to use the barriers suitable for CPU cores.
> 
> To fix the ordering issue for hardware virtio devices, add
> a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> will use the barriers suitable for I/O devices.
> 
> Signed-off-by: Tiwei Bie 
> Reviewed-by: Stefan Hajnoczi 

So some people noticed that some systems also
require a cache flush around DMA to real devices.

Shouldn't we add that as part of VIRTIO_F_IO_BARRIER?

Also rename it?



Others also noticed that there are systems where accesses
include some kind of translation e.g. some high bits
in the address are set to signal access properties.
Should that be included in VIRTIO_F_IOMMU_PLATFORM?




> ---
> v2 -> v3:
> - Update the feature bits allocation (Stefan);
> 
> v1 -> v2:
> - Rebase to the latest spec (MST);
> - Use a smaller textwidth (according to _vimrc);
> 
> RFC -> v1:
> - Use plural (Stefan);
> - Add more details (Stefan);
> 
>  content.tex | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 7a92cb1..95c243f 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -95,10 +95,10 @@ Feature bits are allocated as follows:
>  \begin{description}
>  \item[0 to 23] Feature bits for the specific device type
>  
> -\item[24 to 33] Feature bits reserved for extensions to the queue and
> +\item[24 to 36] Feature bits reserved for extensions to the queue and
>feature negotiation mechanisms
>  
> -\item[34 and above] Feature bits reserved for future extensions.
> +\item[37 and above] Feature bits reserved for future extensions.
>  \end{description}
>  
>  \begin{note}
> @@ -5348,6 +5348,15 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect 
> Flag: Scatter-Gather Supp
>\item[VIRTIO_F_IN_ORDER(35)] This feature indicates
>that all buffers are used by the device in the same
>order in which they have been made available.
> +  \item[VIRTIO_F_IO_BARRIER(36)] This feature indicates
> +  that the device needs the driver to use the barriers
> +  suitable for hardware devices.  Some transports require
> +  barriers to ensure devices have a consistent view of
> +  memory.  When devices are implemented in software a
> +  weaker form of barrier may be sufficient and yield
> +  better performance.  This feature indicates whether
> +  a stronger form of barrier suitable for hardware
> +  devices is necessary.
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> @@ -5363,6 +5372,10 @@ addresses to the device.
>  
>  A driver SHOULD accept VIRTIO_F_RING_PACKED if it is offered.
>  
> +A driver SHOULD accept VIRTIO_F_IO_BARRIER if it is offered.
> +If VIRTIO_F_IO_BARRIER has been negotiated, a driver MUST use
> +the barriers suitable for hardware devices.
> +
>  \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>  
>  A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further
> @@ -5376,6 +5389,9 @@ accepted.
>  If VIRTIO_F_IN_ORDER has been negotiated, a device MUST use
>  buffers in the same order in which they have been available.
>  
> +A device MAY fail to operate further if VIRTIO_F_IO_BARRIER
> +is not accepted.
> +
>  \section{Legacy Interface: Reserved Feature Bits}\label{sec:Reserved Feature 
> Bits / Legacy Interface: Reserved Feature Bits}
>  
>  Transitional devices MAY offer the following:
> -- 
> 2.11.0
> 
> 
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Tiwei Bie]

On Tue, May 15, 2018 at 10:40:44AM +0100, Stefan Hajnoczi wrote:
> On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> > There will be hardware virtio devices in the future, which
> > require drivers to use the barriers suitable for I/O devices,
> > compared with software virtio devices which just require
> > drivers to use the barriers suitable for CPU cores.
> > 
> > To fix the ordering issue for hardware virtio devices, add
> > a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> > will use the barriers suitable for I/O devices.
> > 
> > Signed-off-by: Tiwei Bie 
> > Reviewed-by: Stefan Hajnoczi 
> > ---
> > v2 -> v3:
> > - Update the feature bits allocation (Stefan);
> > 
> > v1 -> v2:
> > - Rebase to the latest spec (MST);
> > - Use a smaller textwidth (according to _vimrc);
> > 
> > RFC -> v1:
> > - Use plural (Stefan);
> > - Add more details (Stefan);
> > 
> >  content.tex | 20 ++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> Thanks for your patience!
> 
> Reviewed-by: Stefan Hajnoczi 

It's my pleasure. Thanks for your review! :)

Best regards,
Tiwei Bie

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

Re: [virtio-dev] [PATCH v3] VIRTIO_F_IO_BARRIER: use I/O barriers in driver

[Stefan Hajnoczi]

On Thu, May 10, 2018 at 11:41:26PM +0800, Tiwei Bie wrote:
> There will be hardware virtio devices in the future, which
> require drivers to use the barriers suitable for I/O devices,
> compared with software virtio devices which just require
> drivers to use the barriers suitable for CPU cores.
> 
> To fix the ordering issue for hardware virtio devices, add
> a new feature: VIRTIO_F_IO_BARRIER. When negotiated, driver
> will use the barriers suitable for I/O devices.
> 
> Signed-off-by: Tiwei Bie 
> Reviewed-by: Stefan Hajnoczi 
> ---
> v2 -> v3:
> - Update the feature bits allocation (Stefan);
> 
> v1 -> v2:
> - Rebase to the latest spec (MST);
> - Use a smaller textwidth (according to _vimrc);
> 
> RFC -> v1:
> - Use plural (Stefan);
> - Add more details (Stefan);
> 
>  content.tex | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)

Thanks for your patience!

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature