[Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte

[Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte

[Jason Wang]

This patch splits the device status field of virtio-net into ro and rw
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 <jasowang@redhat.com>
---
 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 VIRTIO_NET_F_MAC
  is set), and the status field only exists if VIRTIO_NET_F_STATUS is 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_ANNOUNCE.
+
+\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	
+\change_inserted 2090695081 1331907493
+0x100
+\change_deleted 2090695081 1331907491
+2
+\change_unchanged
+
 \end_layout
 
 \begin_layout Plain Layout

Re: [Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte

[Michael S. Tsirkin]

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 rw
> 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 <jasowang@redhat.com>

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 = 1, interrupt

				guest reads VIRTIO_NET_S_ANNOUNCE = 1

host writes VIRTIO_NET_S_SOME_NEW_FIELD = 1, interrupt

				guest writes VIRTIO_NET_S_ANNOUNCE = 0
				VIRTIO_NET_S_SOME_NEW_FIELD is overwritten

				guest reads VIRTIO_NET_S_SOME_NEW_FIELD = 0

How about making the new bit write 1 to clear?
If we do, we can keep it where it is currently ...



> ---
>  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 VIRTIO_NET_F_MAC
>   is set), and the status field only exists if VIRTIO_NET_F_STATUS is 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_ANNOUNCE.
> +
> +\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	
> +\change_inserted 2090695081 1331907493
> +0x100
> +\change_deleted 2090695081 1331907491
> +2
> +\change_unchanged
> +
>  \end_layout
>  
>  \begin_layout Plain Layout

Re: [Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte

[Jason Wang]

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 rw
>> 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<jasowang@redhat.com>
> 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 = 1, interrupt
>
> 				guest reads VIRTIO_NET_S_ANNOUNCE = 1
>
> host writes VIRTIO_NET_S_SOME_NEW_FIELD = 1, interrupt
>
> 				guest writes VIRTIO_NET_S_ANNOUNCE = 0
> 				VIRTIO_NET_S_SOME_NEW_FIELD is overwritten
>
> 				guest reads VIRTIO_NET_S_SOME_NEW_FIELD = 0
>
> How about making the new bit write 1 to clear?
> If we do, we can keep it where it is currently ...
>

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.

>
>> ---
>>   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 VIRTIO_NET_F_MAC
>>    is set), and the status field only exists if VIRTIO_NET_F_STATUS is 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_ANNOUNCE.
>> +
>> +\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	
>> +\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/

Re: [Qemu-devel] [PATCH] virtio-spec: split virtio-net device status filed into ro and rw byte

[Michael S. Tsirkin]

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 rw
> >>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<jasowang@redhat.com>
> >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 = 1, interrupt
> >
> >				guest reads VIRTIO_NET_S_ANNOUNCE = 1
> >
> >host writes VIRTIO_NET_S_SOME_NEW_FIELD = 1, interrupt
> >
> >				guest writes VIRTIO_NET_S_ANNOUNCE = 0
> >				VIRTIO_NET_S_SOME_NEW_FIELD is overwritten
> >
> >				guest reads VIRTIO_NET_S_SOME_NEW_FIELD = 0
> >
> >How about making the new bit write 1 to clear?
> >If we do, we can keep it where it is currently ...
> >
> 
> 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ödinger'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 VIRTIO_NET_F_MAC
> >>   is set), and the status field only exists if VIRTIO_NET_F_STATUS is 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_ANNOUNCE.
> >>+
> >>+\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	
> >>+\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/