[Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Amit Shah]

This can happen if a port gets unplugged before guest has chance to
initialise vqs.

Reported-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 09e22aa..e05ab5e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -117,6 +117,9 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
 {
     VirtQueueElement elem;
 
+    if (!virtio_queue_ready(vq)) {
+        return;
+    }
     while (virtqueue_pop(vq, &elem)) {
         virtqueue_push(vq, &elem, 0);
     }
-- 
1.7.3.5

[Qemu-devel] Re: [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> This can happen if a port gets unplugged before guest has chance to
> initialise vqs.
>
> Reported-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-serial-bus.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 09e22aa..e05ab5e 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -117,6 +117,9 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
>  {
>      VirtQueueElement elem;
>  
> +    if (!virtio_queue_ready(vq)) {
> +        return;
> +    }
>      while (virtqueue_pop(vq, &elem)) {
>          virtqueue_push(vq, &elem, 0);
>      }

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Anthony Liguori]

On 02/04/2011 02:54 AM, Amit Shah wrote:
> This can happen if a port gets unplugged before guest has chance to
> initialise vqs.
>
> Reported-by: Juan Quintela<quintela@redhat.com>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>    

Applied to master, Thanks.

Regards,

Anthony Liguori
> ---
>   hw/virtio-serial-bus.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 09e22aa..e05ab5e 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -117,6 +117,9 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
>   {
>       VirtQueueElement elem;
>
> +    if (!virtio_queue_ready(vq)) {
> +        return;
> +    }
>       while (virtqueue_pop(vq,&elem)) {
>           virtqueue_push(vq,&elem, 0);
>       }
>    

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Amit Shah]

On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> On 02/04/2011 02:54 AM, Amit Shah wrote:
> >This can happen if a port gets unplugged before guest has chance to
> >initialise vqs.
> >
> >Reported-by: Juan Quintela<quintela@redhat.com>
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> 
> Applied to master, Thanks.

Hi,

What's the strategy to commit to 0.14?

		Amit

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Amit Shah]

On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> > On 02/04/2011 02:54 AM, Amit Shah wrote:
> > >This can happen if a port gets unplugged before guest has chance to
> > >initialise vqs.
> > >
> > >Reported-by: Juan Quintela<quintela@redhat.com>
> > >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> > 
> > Applied to master, Thanks.
> 
> What's the strategy to commit to 0.14?

Nevermind; saw it in the last pull.

		Amit

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Anthony Liguori]

On 02/07/2011 08:14 PM, Amit Shah wrote:
> On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
>    
>> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
>>      
>>> On 02/04/2011 02:54 AM, Amit Shah wrote:
>>>        
>>>> This can happen if a port gets unplugged before guest has chance to
>>>> initialise vqs.
>>>>
>>>> Reported-by: Juan Quintela<quintela@redhat.com>
>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>>          
>>> Applied to master, Thanks.
>>>        
>> What's the strategy to commit to 0.14?
>>      
> Nevermind; saw it in the last pull.
>    

Yes, but if we decide to make this a subsection, we may need to revert 
this before the 0.14.0 release because once 0.14.0 goes out, we can't go 
back and change it.

If that's how you want to approach it, then please send the revert 
patches.  The flow_control=0 patches you sent out before definitely 
aren't the right approach for migration compatibility.

Regards,

Anthony Liguori

> 		Amit
>
>    

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Amit Shah]

On (Mon) 07 Feb 2011 [20:25:59], Anthony Liguori wrote:
> On 02/07/2011 08:14 PM, Amit Shah wrote:
> >On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
> >>On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> >>>On 02/04/2011 02:54 AM, Amit Shah wrote:
> >>>>This can happen if a port gets unplugged before guest has chance to
> >>>>initialise vqs.
> >>>>
> >>>>Reported-by: Juan Quintela<quintela@redhat.com>
> >>>>Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >>>Applied to master, Thanks.
> >>What's the strategy to commit to 0.14?
> >Nevermind; saw it in the last pull.
> 
> Yes, but if we decide to make this a subsection, we may need to
> revert this before the 0.14.0 release because once 0.14.0 goes out,
> we can't go back and change it.

Right.

However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
in 0.14 to introduce subsections?  I doubt that.

> If that's how you want to approach it, then please send the revert
> patches.  The flow_control=0 patches you sent out before definitely
> aren't the right approach for migration compatibility.

I understand, I've not looked at that thread yet, but if we want to do
that for 0.14 w/o vmstate, that might be the only way.

		Amit

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Anthony Liguori]

On 02/07/2011 08:55 PM, Amit Shah wrote:
> On (Mon) 07 Feb 2011 [20:25:59], Anthony Liguori wrote:
>    
>> On 02/07/2011 08:14 PM, Amit Shah wrote:
>>      
>>> On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
>>>        
>>>> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
>>>>          
>>>>> On 02/04/2011 02:54 AM, Amit Shah wrote:
>>>>>            
>>>>>> This can happen if a port gets unplugged before guest has chance to
>>>>>> initialise vqs.
>>>>>>
>>>>>> Reported-by: Juan Quintela<quintela@redhat.com>
>>>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>>>>              
>>>>> Applied to master, Thanks.
>>>>>            
>>>> What's the strategy to commit to 0.14?
>>>>          
>>> Nevermind; saw it in the last pull.
>>>        
>> Yes, but if we decide to make this a subsection, we may need to
>> revert this before the 0.14.0 release because once 0.14.0 goes out,
>> we can't go back and change it.
>>      
> Right.
>
> However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
> in 0.14 to introduce subsections?  I doubt that.
>    

Not, that's why I suggested reverting this change and pushing it to 
0.15.  We're not going to fix it properly in 0.14.

Regards,

Anthony Liguori

>> If that's how you want to approach it, then please send the revert
>> patches.  The flow_control=0 patches you sent out before definitely
>> aren't the right approach for migration compatibility.
>>      
> I understand, I've not looked at that thread yet, but if we want to do
> that for 0.14 w/o vmstate, that might be the only way.
>
> 		Amit
>
>    

Re: [Qemu-devel] [PATCH master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

[Amit Shah]

On (Tue) 08 Feb 2011 [01:40:09], Anthony Liguori wrote:
> >
> >However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
> >in 0.14 to introduce subsections?  I doubt that.
> 
> Not, that's why I suggested reverting this change and pushing it to
> 0.15.  We're not going to fix it properly in 0.14.

You mean reverting all of the flow control work?  But then it exposes
another bug: a malicious guest doing huge writes will cause an OOM on
the host; it's better to have this fix rather than not have it.

		Amit