[virtio-dev] [PATCH] content: document console vq detach buffer

[virtio-dev] [PATCH] content: document console vq detach buffer

[Pankaj Gupta]

This patch documents console special case where vq buffers
are deleted at port hotunplug time. This behavior is different in
other devices where vq buffers are deleted at device unplug time.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 content.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/content.tex b/content.tex
index ee0d7c9..33e8ccc 100644
--- a/content.tex
+++ b/content.tex
@@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
 of a live virtqueue.
 
 Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
+Console has a special property that when port is detached virtqueue is considered stopped, device
+must not use any buffers, and it is legal to take buffers out of the device.
 
 \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
 
-- 
2.21.0


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

Re: [virtio-dev] [PATCH] content: document console vq detach buffer

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Tue, Aug 13, 2019 at 06:21:33PM +0530, Pankaj Gupta wrote:
> This patch documents console special case where vq buffers
> are deleted at port hotunplug time. This behavior is different in
> other devices where vq buffers are deleted at device unplug time.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  content.tex | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index ee0d7c9..33e8ccc 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
>  of a live virtqueue.
>  
>  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
> +Console has a special property that when port is detached virtqueue is considered stopped, device
> +must not use any buffers, and it is legal to take buffers out of the device.

The word "detach" is not defined or used in the spec, so it's unclear
what this is supposed to mean.  Is this related to a control message
(VIRTIO_CONSOLE_PORT_OPEN)?

How about:

  The device MUST NOT use buffers after it has sent
  VIRTIO_CONSOLE_PORT_OPEN with value=0 for a port.

I'm not sure what you mean by "take buffers out of the device"?  I guess
you mean the driver can modify the vring because the device isn't
looking.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [virtio-dev] [PATCH] content: document console vq detach buffer

[Pankaj Gupta]

> 
> On Tue, Aug 13, 2019 at 06:21:33PM +0530, Pankaj Gupta wrote:
> > This patch documents console special case where vq buffers
> > are deleted at port hotunplug time. This behavior is different in
> > other devices where vq buffers are deleted at device unplug time.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  content.tex | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/content.tex b/content.tex
> > index ee0d7c9..33e8ccc 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General
> > Initialization And Device Operation /
> >  of a live virtqueue.
> >  
> >  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before
> >  removing exposed buffers.
> > +Console has a special property that when port is detached virtqueue is
> > considered stopped, device
> > +must not use any buffers, and it is legal to take buffers out of the
> > device.
> 
> The word "detach" is not defined or used in the spec, so it's unclear
> what this is supposed to mean.  Is this related to a control message
> (VIRTIO_CONSOLE_PORT_OPEN)?

By detach here we mean "unplug".  

> 
> How about:
> 
>   The device MUST NOT use buffers after it has sent
>   VIRTIO_CONSOLE_PORT_OPEN with value=0 for a port.
> 
> I'm not sure what you mean by "take buffers out of the device"?  I guess
> you mean the driver can modify the vring because the device isn't
> looking.

"take buffers out of the device" means "remove the unused buffers from the virt queue".

How about the below text:

Console has a special property that when port is unplugged virtqueue is
considered stopped, device must not use any buffers, and it is legal to
remove the unused buffers from the device.

Thanks,
Pankaj
> 

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

Re: [virtio-dev] [PATCH] content: document console vq detach buffer

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

On Wed, Aug 14, 2019 at 02:33:15AM -0400, Pankaj Gupta wrote:
> > On Tue, Aug 13, 2019 at 06:21:33PM +0530, Pankaj Gupta wrote:
> > > This patch documents console special case where vq buffers
> > > are deleted at port hotunplug time. This behavior is different in
> > > other devices where vq buffers are deleted at device unplug time.
> > > 
> > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > > ---
> > >  content.tex | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index ee0d7c9..33e8ccc 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General
> > > Initialization And Device Operation /
> > >  of a live virtqueue.
> > >  
> > >  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before
> > >  removing exposed buffers.
> > > +Console has a special property that when port is detached virtqueue is
> > > considered stopped, device
> > > +must not use any buffers, and it is legal to take buffers out of the
> > > device.
> > 
> > The word "detach" is not defined or used in the spec, so it's unclear
> > what this is supposed to mean.  Is this related to a control message
> > (VIRTIO_CONSOLE_PORT_OPEN)?
> 
> By detach here we mean "unplug".  

"unplug" also has no meaning defined in the spec.  Please explain things
in terms of the actual device interface (control messages, config space
fields, etc) so that the spec is clear.  Which event or operation are
you talking about?

> > 
> > How about:
> > 
> >   The device MUST NOT use buffers after it has sent
> >   VIRTIO_CONSOLE_PORT_OPEN with value=0 for a port.
> > 
> > I'm not sure what you mean by "take buffers out of the device"?  I guess
> > you mean the driver can modify the vring because the device isn't
> > looking.
> 
> "take buffers out of the device" means "remove the unused buffers from the virt queue".

What does "remove the unused buffers from the virt queue" mean?
Decreasing the avail index (for split-ring virtqueue layouts) so that
previously available buffers are no longer available?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[virtio-dev] Re: [PATCH] content: document console vq detach buffer

[Michael S. Tsirkin]

On Tue, Aug 13, 2019 at 06:21:33PM +0530, Pankaj Gupta wrote:
> This patch documents console special case where vq buffers
> are deleted at port hotunplug time. This behavior is different in
> other devices where vq buffers are deleted at device unplug time.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  content.tex | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index ee0d7c9..33e8ccc 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
>  of a live virtqueue.
>  
>  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
> +Console has a special property that when port is detached virtqueue is considered stopped, device
> +must not use any buffers, and it is legal to take buffers out of the device.
>  
>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}

I don't think that's enough, or a good way to do it.

The assumption that once exposed buffers stay exposed until used
is spread out in lots of places in the spec.
	E.g.
		2.6.6.1
		Driver Requirements: The Virtqueue Available Ring
		A driver MUST NOT decrement the available idx on a virtqueue (ie. there is no way to “unexpose” buffers).

	And in the packed ring part, e.g.

	A device MUST NOT use a descriptor unless it observes the VIRTQ_DESC_F_AVAIL bit in its flags
	being changed (e.g. as compared to the initial zero value). A device MUST NOT change a descriptor after
	changing it’s the VIRTQ_DESC_F_USED bit in its flags.
	2.7.16
	Driver Requirements: The Virtqueue Descriptor Table
	A driver MUST NOT change a descriptor unless it observes the VIRTQ_DESC_F_USED bit in its flags being
	changed. A driver MUST NOT change a descriptor after changing the VIRTQ_DESC_F_AVAIL bit in its flags.

So we need to document how exactly to revert added buffers in all
these places. I propose adding special sections, explaining
that some devices allow taking back available buffers.

We need to see how this interacts with other places in the spec:
e.g. if we do allow taking back buffers, what happens with notifications
such as when using event index? Are they resent when buffer is
re-added?


I also worry that since the spec said this can't happen, some
hypervisors might implement a policy that will crash if the guest
violates this rule.  Seems unlikely since Linux violated the rule for a
while, but I'd suggest that you take a look at least at some open-source
hypervisors to see what is going on. An alternative is a feature flag -
if we do that I would actually advocate for a generic feature that
allows stopping queues at any time, then restarting it. I think it would
be handy generally for things like enabling/disabling XDP.



> -- 
> 2.21.0

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

[virtio-dev] Re: [PATCH] content: document console vq detach buffer

[Pankaj Gupta]

> 
> On Tue, Aug 13, 2019 at 06:21:33PM +0530, Pankaj Gupta wrote:
> > This patch documents console special case where vq buffers
> > are deleted at port hotunplug time. This behavior is different in
> > other devices where vq buffers are deleted at device unplug time.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  content.tex | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/content.tex b/content.tex
> > index ee0d7c9..33e8ccc 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -497,6 +497,8 @@ \section{Device Cleanup}\label{sec:General
> > Initialization And Device Operation /
> >  of a live virtqueue.
> >  
> >  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before
> >  removing exposed buffers.
> > +Console has a special property that when port is detached virtqueue is
> > considered stopped, device
> > +must not use any buffers, and it is legal to take buffers out of the
> > device.
> >  
> >  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
> 
> I don't think that's enough, or a good way to do it.
> 
> The assumption that once exposed buffers stay exposed until used
> is spread out in lots of places in the spec.

oh... I did not notice the other places.

> 	E.g.
> 		2.6.6.1
> 		Driver Requirements: The Virtqueue Available Ring
> 		A driver MUST NOT decrement the available idx on a virtqueue (ie. there is
> 		no way to “unexpose” buffers).
> 
> 	And in the packed ring part, e.g.
> 
> 	A device MUST NOT use a descriptor unless it observes the VIRTQ_DESC_F_AVAIL
> 	bit in its flags
> 	being changed (e.g. as compared to the initial zero value). A device MUST
> 	NOT change a descriptor after
> 	changing it’s the VIRTQ_DESC_F_USED bit in its flags.
> 	2.7.16
> 	Driver Requirements: The Virtqueue Descriptor Table
> 	A driver MUST NOT change a descriptor unless it observes the
> 	VIRTQ_DESC_F_USED bit in its flags being
> 	changed. A driver MUST NOT change a descriptor after changing the
> 	VIRTQ_DESC_F_AVAIL bit in its flags.
> 
> So we need to document how exactly to revert added buffers in all
> these places. I propose adding special sections, explaining
> that some devices allow taking back available buffers.

Sure.

> 
> We need to see how this interacts with other places in the spec:
> e.g. if we do allow taking back buffers, what happens with notifications
> such as when using event index? Are they resent when buffer is
> re-added?

o.k. Will check this.

> 
> 
> I also worry that since the spec said this can't happen, some
> hypervisors might implement a policy that will crash if the guest
> violates this rule.  Seems unlikely since Linux violated the rule for a
> while, but I'd suggest that you take a look at least at some open-source
> hypervisors to see what is going on. 

Good point. I will check.

An alternative is a feature flag -
> if we do that I would actually advocate for a generic feature that
> allows stopping queues at any time, then restarting it. I think it would
> be handy generally for things like enabling/disabling XDP.

o.k. Good idea. Generic queue stopping will require additional work
e.g draining the queue and might require certain device specific operation.
Will look at this.
  

Thank you for the suggestions.

Best regards,
Pankaj


> 
> 
> 
> > --
> > 2.21.0
> 

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