From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55623) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Xsi-0006kE-Qx for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:23:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9XsK-0007G1-4Y for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:23:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3971) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9XsJ-0007FD-Sk for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:23:32 -0400 Date: Mon, 19 Mar 2012 10:23:44 +0200 From: "Michael S. Tsirkin" Message-ID: <20120319082343.GD17673@redhat.com> References: <20120316152026.6428.6983.stgit@jason-ThinkPad-T400> <20120318122252.GE29902@redhat.com> <4F66A364.2080600@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <4F66A364.2080600@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: linux-kernel@vger.kernel.org, rusty@rustcorp.com.au, qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org On Mon, Mar 19, 2012 at 11:09:24AM +0800, Jason Wang wrote: > On 03/18/2012 08:22 PM, Michael S. Tsirkin wrote: > >On Fri, Mar 16, 2012 at 11:20:26PM +0800, Jason Wang wrote: > >>This patch splits the device status field of virtio-net into ro and r= w > >>byte. This would simplify the implementation of both host and guest > >>and make the layout more clean. As VIRTIO_NET_S_ANNOUNCE is a rw bit, > >>it was moved to bit 8 (0x100). > >> > >>btw. looks like there's no implementation that depends on > >>VIRTIO_NET_S_ANNOUNCE, so the move is safe. > >> > >>Signed-off-by: Jason Wang > >Hmm, I know I proposed this myself, and I thought it will > >prevent problems if we will add more rw bits, > >but I missed the following race: > > > >host writes VIRTIO_NET_S_ANNOUNCE =3D 1, interrupt > > > > guest reads VIRTIO_NET_S_ANNOUNCE =3D 1 > > > >host writes VIRTIO_NET_S_SOME_NEW_FIELD =3D 1, interrupt > > > > guest writes VIRTIO_NET_S_ANNOUNCE =3D 0 > > VIRTIO_NET_S_SOME_NEW_FIELD is overwritten > > > > guest reads VIRTIO_NET_S_SOME_NEW_FIELD =3D 0 > > > >How about making the new bit write 1 to clear? > >If we do, we can keep it where it is currently ... > > >=20 > Didn't follow, do you mean to make VIRITO_NET_S_ANNOUNCE bit clear > on read? Looks like this can prevent the race and keep what > currently we have. Clear on read is evil, it makes debugging harder since you can't check the state without destroying it. Write 1 to clear means writing 0 to a bit does not change it, writing 1 clears it. This makes it possible to flip individual bits in config space without making Schr=F6dinger's cat experiments. > > > >>--- > >> virtio-0.9.4.lyx | 20 +++++++++++++++++++- > >> 1 files changed, 19 insertions(+), 1 deletions(-) > >> > >>diff --git a/virtio-0.9.4.lyx b/virtio-0.9.4.lyx > >>index 6c7bab1..ef3951c 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 > >>@@ -4012,8 +4013,19 @@ configuration > >> layout Two configuration fields are currently defined. > >> The mac address field always exists (though is only valid if VIRTI= O_NET_F_MAC > >> is set), and the status field only exists if VIRTIO_NET_F_STATUS i= s set. > >>+ > >>+\change_inserted 2090695081 1331907586 > >>+ The low byte of status field is read-only, guest write to this byte= would > >>+ be ignored. > >>+ Currently only one bit is defined for this byte: VIRTIO_NET_S_LINK_= UP. > >>+ The high byte of status field is read-writable. > >>+ Currently only one bit is defined for this byte: VIRTIO_NET_S_ANNOU= NCE. > >>+ > >>+\change_deleted 2090695081 1331907489 > >> Two bits are currently defined for the status field: VIRTIO_NET_S_= LINK_UP > >> and VIRTIO_NET_S_ANNOUNCE. > >>+ > >>+\change_unchanged > >> > >> \begin_inset listings > >> inline false > >>@@ -4026,7 +4038,13 @@ status open > >> > >> \begin_layout Plain Layout > >> > >>-#define VIRTIO_NET_S_ANNOUNCE 2 > >>+#define VIRTIO_NET_S_ANNOUNCE=09 > >>+\change_inserted 2090695081 1331907493 > >>+0x100 > >>+\change_deleted 2090695081 1331907491 > >>+2 > >>+\change_unchanged > >>+ > >> \end_layout > >> > >> \begin_layout Plain Layout > >-- > >To unsubscribe from this list: send the line "unsubscribe linux-kernel= " in > >the body of a message to majordomo@vger.kernel.org > >More majordomo info at http://vger.kernel.org/majordomo-info.html > >Please read the FAQ at http://www.tux.org/lkml/