Patchwork virtio-spec: clarify ro/rw bits and updating rule of virtio-net status field

login
register
mail settings
Submitter Jason Wang
Date March 20, 2012, 6:20 a.m.
Message ID <20120320062020.12815.61342.stgit@jason-ThinkPad-T400>
Download mbox | patch
Permalink /patch/147746/
State New
Headers show

Comments

Jason Wang - March 20, 2012, 6:20 a.m.
This patch clarifies VIRTIO_NET_S_LINK_UP as a read-only bit and
VIRTIO_NET_S_ANNOUNCE as a read-writable bit. Also introduce the write 1 to
clear semantics for all read-writable bits of config status field. This could
help to reduce the config status field updating race between host and guest and
also simplify the implementation.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 virtio-0.9.4.lyx |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)
Rusty Russell - March 20, 2012, 11:05 a.m.
On Tue, 20 Mar 2012 14:20:21 +0800, Jason Wang <jasowang@redhat.com> wrote:
> This patch clarifies VIRTIO_NET_S_LINK_UP as a read-only bit and
> VIRTIO_NET_S_ANNOUNCE as a read-writable bit. Also introduce the write 1 to
> clear semantics for all read-writable bits of config status field. This could
> help to reduce the config status field updating race between host and guest and
> also simplify the implementation.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

This approach assumes an active config field, which neither lguest nor
S/390 have.

Thanks,
Rusty.
Michael S. Tsirkin - March 20, 2012, 10:48 p.m.
On Tue, Mar 20, 2012 at 09:35:18PM +1030, Rusty Russell wrote:
> On Tue, 20 Mar 2012 14:20:21 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > This patch clarifies VIRTIO_NET_S_LINK_UP as a read-only bit and
> > VIRTIO_NET_S_ANNOUNCE as a read-writable bit. Also introduce the write 1 to
> > clear semantics for all read-writable bits of config status field. This could
> > help to reduce the config status field updating race between host and guest and
> > also simplify the implementation.
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> This approach assumes an active config field, which neither lguest nor
> S/390 have.
> 
> Thanks,
> Rusty.

What does 'active' mean in this context?

> -- 
>   How could I marry someone with more hair than me?  http://baldalex.org
Rusty Russell - March 20, 2012, 11:40 p.m.
On Wed, 21 Mar 2012 00:48:05 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 20, 2012 at 09:35:18PM +1030, Rusty Russell wrote:
> > On Tue, 20 Mar 2012 14:20:21 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > This patch clarifies VIRTIO_NET_S_LINK_UP as a read-only bit and
> > > VIRTIO_NET_S_ANNOUNCE as a read-writable bit. Also introduce the write 1 to
> > > clear semantics for all read-writable bits of config status field. This could
> > > help to reduce the config status field updating race between host and guest and
> > > also simplify the implementation.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > 
> > This approach assumes an active config field, which neither lguest nor
> > S/390 have.
> > 
> > Thanks,
> > Rusty.
> 
> What does 'active' mean in this context?

It means you are assuming that writes to the config field trap to the
host.

Rusty.
Michael S. Tsirkin - March 21, 2012, 6:37 a.m.
On Wed, Mar 21, 2012 at 10:10:52AM +1030, Rusty Russell wrote:
> On Wed, 21 Mar 2012 00:48:05 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Mar 20, 2012 at 09:35:18PM +1030, Rusty Russell wrote:
> > > On Tue, 20 Mar 2012 14:20:21 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > This patch clarifies VIRTIO_NET_S_LINK_UP as a read-only bit and
> > > > VIRTIO_NET_S_ANNOUNCE as a read-writable bit. Also introduce the write 1 to
> > > > clear semantics for all read-writable bits of config status field. This could
> > > > help to reduce the config status field updating race between host and guest and
> > > > also simplify the implementation.
> > > > 
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > 
> > > This approach assumes an active config field, which neither lguest nor
> > > S/390 have.
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > What does 'active' mean in this context?
> 
> It means you are assuming that writes to the config field trap to the
> host.
> 
> Rusty.

Ah. Right, we need to trap for host to clear the bit.
OK, so let's make the bit RO, and add
VIRTIO_NET_CTRL_ANNOUNCED to acknowledge that we've
seen VIRTIO_NET_S_ANNOUNCE using the control VQ?

If there's no ctrl VQ, we won't acknowledge the feature bit ...

> -- 
>   How could I marry someone with more hair than me?  http://baldalex.org
Rusty Russell - March 22, 2012, 4:30 a.m.
On Wed, 21 Mar 2012 08:37:46 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Ah. Right, we need to trap for host to clear the bit.
> OK, so let's make the bit RO, and add
> VIRTIO_NET_CTRL_ANNOUNCED to acknowledge that we've
> seen VIRTIO_NET_S_ANNOUNCE using the control VQ?

Thanks, that's nice.  Guest should send arp packets first, then send
VIRTIO_NET_CTRL_ANNOUNCED, and ignore the bit being set in the meantime.

Thanks,
Rusty.
Jason Wang - March 23, 2012, 2:57 a.m.
On 03/22/2012 12:30 PM, Rusty Russell wrote:
> On Wed, 21 Mar 2012 08:37:46 +0200, "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>> Ah. Right, we need to trap for host to clear the bit.
>> OK, so let's make the bit RO, and add
>> VIRTIO_NET_CTRL_ANNOUNCED to acknowledge that we've
>> seen VIRTIO_NET_S_ANNOUNCE using the control VQ?
> Thanks, that's nice.  Guest should send arp packets first, then send
> VIRTIO_NET_CTRL_ANNOUNCED, and ignore the bit being set in the meantime.
>
> Thanks,
> Rusty.
Sure, I would update and re-send the spec updating patch.

Thanks

Patch

diff --git a/virtio-0.9.4.lyx b/virtio-0.9.4.lyx
index 6c7bab1..614ab55 100644
--- a/virtio-0.9.4.lyx
+++ b/virtio-0.9.4.lyx
@@ -58,6 +58,7 @@ 
 \html_be_strict false
 \author -608949062 "Rusty Russell,,," 
 \author 1531152142 "pbonzini" 
+\author 2090695081 "jason" 
 \end_header
 
 \begin_body
@@ -4013,7 +4014,21 @@  layout Two configuration fields are currently defined.
  The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
  is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
  Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
- and VIRTIO_NET_S_ANNOUNCE.
+ 
+\change_inserted 2090695081 1332220873
+(read-only) 
+\change_unchanged
+and VIRTIO_NET_S_ANNOUNCE
+\change_inserted 2090695081 1332220883
+ (read-writable)
+\change_unchanged
+.
+
+\change_inserted 2090695081 1332220901
+ Writing 1 to any read-writable bit of status filed would cause the bit
+ to be cleared.
+
+\change_unchanged
  
 \begin_inset listings
 inline false
@@ -4915,7 +4930,11 @@  Processing this notification involves:
 \end_layout
 
 \begin_layout Enumerate
-Clearing VIRTIO_NET_S_ANNOUNCE bit in the status field.
+Clearing VIRTIO_NET_S_ANNOUNCE bit in the status field
+\change_inserted 2090695081 1332220849
+ (by writing 1 to VIRTIO_NET_S_ANNOUNCE bit)
+\change_unchanged
+.
 \end_layout
 
 \begin_layout Enumerate