diff mbox

Fix for qemu crash on assertion error when adding PCI passthru device.

Message ID 36AF4B62444F4B4FB04EDD1FDAE1CF1D48899822@G4W3209.americas.hpqcorp.net
State New
Headers show

Commit Message

Ma, Stephen B. June 17, 2012, 6:26 a.m. UTC 
Michael,

Thanks for the review.  I added the unparent to the qdev_free.


---
 hw/qdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

--
1.7.1

-----Original Message-----
From: Michael S. Tsirkin [mailto:mst@redhat.com] 
Sent: Tuesday, June 12, 2012 1:27 AM
To: Ma, Stephen B.
Cc: 'qemu-devel@nongnu.org'
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

On Tue, Jun 12, 2012 at 04:31:20AM +0000, Ma, Stephen B. wrote:
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6a8f6bd..d2dc28b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -139,7 +139,7 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>  /* Initialize a device.  Device properties should be set before calling
>     this function.  IRQs and MMIO regions should be connected/mapped after
>     calling this function.
> -   On failure, destroy the device and return negative value.
> +   On failure, return a negative value.
>     Return 0 on success.  */
>  int qdev_init(DeviceState *dev)
>  {


Yes, I agree. qdev_init did now allocate the device so
it should not free it.


> @@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
>  
>      rc = dc->init(dev);
>      if (rc < 0) {
> -        qdev_free(dev);
>          return rc;
>      }
>

Another thing we need to fix is unparent the device in
qdev_free.

Comments

Michael S. Tsirkin June 17, 2012, 8:34 a.m. UTC | #1 
On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
> 
> Michael,
> 
> Thanks for the review.  I added the unparent to the qdev_free.
> 
> 
> ---
>  hw/qdev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d2dc28b..ed1328d 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>  /* Unlink device from bus and free the structure.  */
>  void qdev_free(DeviceState *dev)
>  {
> +    object_unparent(OBJECT(dev));
>      object_delete(OBJECT(dev));
>  }
> 
> --
> 1.7.1

Anthony, any feedback?
Anthony Liguori June 17, 2012, 2:28 p.m. UTC | #2 
On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>
>> Michael,
>>
>> Thanks for the review.  I added the unparent to the qdev_free.
>>
>>
>> ---
>>   hw/qdev.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index d2dc28b..ed1328d 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>   /* Unlink device from bus and free the structure.  */
>>   void qdev_free(DeviceState *dev)
>>   {
>> +    object_unparent(OBJECT(dev));
>>       object_delete(OBJECT(dev));
>>   }
>>
>> --
>> 1.7.1
>
> Anthony, any feedback?

Yes, this is wrong.

PCI passthrough isn't in qemu.git so it's not clear to me where this is 
happening.  Why would qdev_free be called when adding a PCI passthru device?

Regards,

Anthony Liguori

>
Jan Kiszka June 18, 2012, 6:24 a.m. UTC | #3 
On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |    1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +    object_unparent(OBJECT(dev));
>>>       object_delete(OBJECT(dev));
>>>   }
>>>
>>> -- 
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this is
> happening.  Why would qdev_free be called when adding a PCI passthru
> device?

The bug is reproducible with any in-tree device (at least PCI) that
happens to return != 0 from its init handler.

Jan
Ma, Stephen B. July 18, 2012, 8:42 p.m. UTC | #4 
Sorry for taking so long to reply.  I am new to this.  Should this patch be committed or just dropped


-----Original Message-----
From: Jan Kiszka [mailto:jan.kiszka@web.de] 
Sent: Sunday, June 17, 2012 11:25 PM
To: Anthony Liguori
Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |    1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +    object_unparent(OBJECT(dev));
>>>       object_delete(OBJECT(dev));
>>>   }
>>>
>>> --
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this 
> is happening.  Why would qdev_free be called when adding a PCI 
> passthru device?

The bug is reproducible with any in-tree device (at least PCI) that happens to return != 0 from its init handler.

Jan
Jan Kiszka July 19, 2012, 6:27 a.m. UTC | #5 
On 2012-07-18 22:42, Ma, Stephen B. wrote:
> Sorry for taking so long to reply.  I am new to this.  Should this patch be committed or just dropped

This bug was fixed by 266ca11a0433643a3cc3146a9837d9f2b0bfbe3b in the
meantime.

Jan

> 
> 
> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@web.de] 
> Sent: Sunday, June 17, 2012 11:25 PM
> To: Anthony Liguori
> Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
> Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.
> 
> On 2012-06-17 16:28, Anthony Liguori wrote:
>> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>>
>>>> Michael,
>>>>
>>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>>
>>>>
>>>> ---
>>>>   hw/qdev.c |    1 +
>>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>>> index d2dc28b..ed1328d 100644
>>>> --- a/hw/qdev.c
>>>> +++ b/hw/qdev.c
>>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>>   /* Unlink device from bus and free the structure.  */
>>>>   void qdev_free(DeviceState *dev)
>>>>   {
>>>> +    object_unparent(OBJECT(dev));
>>>>       object_delete(OBJECT(dev));
>>>>   }
>>>>
>>>> --
>>>> 1.7.1
>>>
>>> Anthony, any feedback?
>>
>> Yes, this is wrong.
>>
>> PCI passthrough isn't in qemu.git so it's not clear to me where this 
>> is happening.  Why would qdev_free be called when adding a PCI 
>> passthru device?
> 
> The bug is reproducible with any in-tree device (at least PCI) that happens to return != 0 from its init handler.
> 
> Jan
> 
>
diff mbox

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index d2dc28b..ed1328d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -264,6 +264,7 @@  void qdev_init_nofail(DeviceState *dev)
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
+    object_unparent(OBJECT(dev));
     object_delete(OBJECT(dev));
 }