1. Patchew
  2. QEMU
  3. View series

[PATCH] pcie: Release references of virtual functions

Akihiko Odaki posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230411090408.48366-1-akihiko.odaki@daynix.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/pci/pcie_sriov.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] pcie: Release references of virtual functions
Posted by Akihiko Odaki 1 year ago 
pci_new() automatically retains a reference to a virtual function when
registering it so we need to release the reference when unregistering.

Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/pci/pcie_sriov.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index aa5a757b11..76a3b6917e 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
             error_free(local_err);
         }
         object_unparent(OBJECT(vf));
+        object_unref(OBJECT(vf));
     }
     g_free(dev->exp.sriov_pf.vf);
     dev->exp.sriov_pf.vf = NULL;
-- 
2.40.0
Re: [PATCH] pcie: Release references of virtual functions
Posted by Philippe Mathieu-Daudé 1 year ago 
On 11/4/23 11:04, Akihiko Odaki wrote:
> pci_new() automatically retains a reference to a virtual function when
> registering it so we need to release the reference when unregistering.
> 
> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/pci/pcie_sriov.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index aa5a757b11..76a3b6917e 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>               error_free(local_err);
>           }
>           object_unparent(OBJECT(vf));
> +        object_unref(OBJECT(vf));
>       }
>       g_free(dev->exp.sriov_pf.vf);
>       dev->exp.sriov_pf.vf = NULL;

It feels the issue is at the device creation.

[/me looking at the code]

What about:

-- >8 --
diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index aa5a757b11..fca3bf6e72 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int 
devfn, const char *name,
      PCIBus *bus = pci_get_bus(pf);
      Error *local_err = NULL;

-    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
+    pci_realize_and_unref(dev, bus, &local_err);
      if (local_err) {
          error_report_err(local_err);
          return NULL;
---
Re: [PATCH] pcie: Release references of virtual functions
Posted by Michael S. Tsirkin 1 year ago 
On Tue, Apr 11, 2023 at 12:11:30PM +0200, Philippe Mathieu-Daudé wrote:
> On 11/4/23 11:04, Akihiko Odaki wrote:
> > pci_new() automatically retains a reference to a virtual function when
> > registering it so we need to release the reference when unregistering.
> > 
> > Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   hw/pci/pcie_sriov.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > index aa5a757b11..76a3b6917e 100644
> > --- a/hw/pci/pcie_sriov.c
> > +++ b/hw/pci/pcie_sriov.c
> > @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
> >               error_free(local_err);
> >           }
> >           object_unparent(OBJECT(vf));
> > +        object_unref(OBJECT(vf));
> >       }
> >       g_free(dev->exp.sriov_pf.vf);
> >       dev->exp.sriov_pf.vf = NULL;
> 
> It feels the issue is at the device creation.
> 
> [/me looking at the code]
> 
> What about:
> 
> -- >8 --
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index aa5a757b11..fca3bf6e72 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int devfn,
> const char *name,
>      PCIBus *bus = pci_get_bus(pf);
>      Error *local_err = NULL;
> 
> -    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
> +    pci_realize_and_unref(dev, bus, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>          return NULL;

ok you want to repost this or Akihiko convinced you?


> ---
Re: [PATCH] pcie: Release references of virtual functions
Posted by Akihiko Odaki 10 months ago 
On 2023/04/21 17:09, Michael S. Tsirkin wrote:
> On Tue, Apr 11, 2023 at 12:11:30PM +0200, Philippe Mathieu-Daudé wrote:
>> On 11/4/23 11:04, Akihiko Odaki wrote:
>>> pci_new() automatically retains a reference to a virtual function when
>>> registering it so we need to release the reference when unregistering.
>>>
>>> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>    hw/pci/pcie_sriov.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>> index aa5a757b11..76a3b6917e 100644
>>> --- a/hw/pci/pcie_sriov.c
>>> +++ b/hw/pci/pcie_sriov.c
>>> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>>>                error_free(local_err);
>>>            }
>>>            object_unparent(OBJECT(vf));
>>> +        object_unref(OBJECT(vf));
>>>        }
>>>        g_free(dev->exp.sriov_pf.vf);
>>>        dev->exp.sriov_pf.vf = NULL;
>>
>> It feels the issue is at the device creation.
>>
>> [/me looking at the code]
>>
>> What about:
>>
>> -- >8 --
>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>> index aa5a757b11..fca3bf6e72 100644
>> --- a/hw/pci/pcie_sriov.c
>> +++ b/hw/pci/pcie_sriov.c
>> @@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int devfn,
>> const char *name,
>>       PCIBus *bus = pci_get_bus(pf);
>>       Error *local_err = NULL;
>>
>> -    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
>> +    pci_realize_and_unref(dev, bus, &local_err);
>>       if (local_err) {
>>           error_report_err(local_err);
>>           return NULL;
> 
> ok you want to repost this or Akihiko convinced you?
> 
> 
>> ---
> 

Can you reply to this?

Regards,
Akihiko Odaki
Re: [PATCH] pcie: Release references of virtual functions
Posted by Philippe Mathieu-Daudé 10 months ago 
On 1/7/23 08:19, Akihiko Odaki wrote:
> On 2023/04/21 17:09, Michael S. Tsirkin wrote:
>> On Tue, Apr 11, 2023 at 12:11:30PM +0200, Philippe Mathieu-Daudé wrote:
>>> On 11/4/23 11:04, Akihiko Odaki wrote:
>>>> pci_new() automatically retains a reference to a virtual function when
>>>> registering it so we need to release the reference when unregistering.
>>>>
>>>> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O 
>>>> Virtualization (SR/IOV)")
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    hw/pci/pcie_sriov.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>>> index aa5a757b11..76a3b6917e 100644
>>>> --- a/hw/pci/pcie_sriov.c
>>>> +++ b/hw/pci/pcie_sriov.c
>>>> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>>>>                error_free(local_err);
>>>>            }
>>>>            object_unparent(OBJECT(vf));
>>>> +        object_unref(OBJECT(vf));
>>>>        }
>>>>        g_free(dev->exp.sriov_pf.vf);
>>>>        dev->exp.sriov_pf.vf = NULL;
>>>
>>> It feels the issue is at the device creation.
>>>
>>> [/me looking at the code]
>>>
>>> What about:
>>>
>>> -- >8 --
>>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>>> index aa5a757b11..fca3bf6e72 100644
>>> --- a/hw/pci/pcie_sriov.c
>>> +++ b/hw/pci/pcie_sriov.c
>>> @@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int 
>>> devfn,
>>> const char *name,
>>>       PCIBus *bus = pci_get_bus(pf);
>>>       Error *local_err = NULL;
>>>
>>> -    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
>>> +    pci_realize_and_unref(dev, bus, &local_err);
>>>       if (local_err) {
>>>           error_report_err(local_err);
>>>           return NULL;
>>
>> ok you want to repost this or Akihiko convinced you?
>>
>>
>>> ---
>>
> 
> Can you reply to this?

Sorry I didn't noticed MST question.

PCIDevice should use their class realize() handler, not the qdev
parent. Code smell somewhere, but can be cleaned later.
I'm fine with Akihiko's patch.

Regards,

Phil.
Re: [PATCH] pcie: Release references of virtual functions
Posted by Akihiko Odaki 1 year ago 
On 2023/04/11 19:11, Philippe Mathieu-Daudé wrote:
> On 11/4/23 11:04, Akihiko Odaki wrote:
>> pci_new() automatically retains a reference to a virtual function when
>> registering it so we need to release the reference when unregistering.
>>
>> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O 
>> Virtualization (SR/IOV)")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/pci/pcie_sriov.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
>> index aa5a757b11..76a3b6917e 100644
>> --- a/hw/pci/pcie_sriov.c
>> +++ b/hw/pci/pcie_sriov.c
>> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>>               error_free(local_err);
>>           }
>>           object_unparent(OBJECT(vf));
>> +        object_unref(OBJECT(vf));
>>       }
>>       g_free(dev->exp.sriov_pf.vf);
>>       dev->exp.sriov_pf.vf = NULL;
> 
> It feels the issue is at the device creation.

I added object_unref() to unregister_vfs() because dev->exp.sriov_pf.vf 
actually holds the reference. Practically there should be no difference 
as the parent bus also keeps the reference until unregister_vfs() calls 
object_unparent(), but I think it is semantically more correct to 
object_unref() in unregister_vfs().

> 
> [/me looking at the code]
> 
> What about:
> 
> -- >8 --
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index aa5a757b11..fca3bf6e72 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -150,7 +150,7 @@ static PCIDevice *register_vf(PCIDevice *pf, int 
> devfn, const char *name,
>       PCIBus *bus = pci_get_bus(pf);
>       Error *local_err = NULL;
> 
> -    qdev_realize(&dev->qdev, &bus->qbus, &local_err);
> +    pci_realize_and_unref(dev, bus, &local_err);
>       if (local_err) {
>           error_report_err(local_err);
>           return NULL;
> ---
Re: [PATCH] pcie: Release references of virtual functions
Posted by Cédric Le Goater 1 year ago 
On 4/11/23 11:04, Akihiko Odaki wrote:
> pci_new() automatically retains a reference to a virtual function when
> registering it so we need to release the reference when unregistering.
> 
> Fixes: 7c0fa8dff8 ("pcie: Add support for Single Root I/O Virtualization (SR/IOV)")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>   hw/pci/pcie_sriov.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index aa5a757b11..76a3b6917e 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -211,6 +211,7 @@ static void unregister_vfs(PCIDevice *dev)
>               error_free(local_err);
>           }
>           object_unparent(OBJECT(vf));
> +        object_unref(OBJECT(vf));
>       }
>       g_free(dev->exp.sriov_pf.vf);
>       dev->exp.sriov_pf.vf = NULL;

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.