Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On Fri, Jul 06, 2018 at 07:06:36PM +0200, Paolo Bonzini wrote: > On 06/07/2018 18:25, Michael S. Tsirkin wrote: > > On Fri, Jul 06, 2018 at 05:55:23PM +0200, Paolo Bonzini wrote: > >> On 06/07/2018 03:51, Peter Xu wrote: > >>> > >>> A question about memory region auto destruction (which might not > >>> related to this patch): I see that we have object_property_add_child() > >>> in memory_region_do_init() to achieve the auto destruction but only if > >>> the "name" of memory region is specified. Could we just do that > >>> unconditionally (though we might of course need to generate some of > >>> the names), or is there a reason not to do so? > >> > >> I'm not sure actually if there are still regions without a name... > >> > >> Paolo > > > > Answer to Peter's question would be a yes then? > > > > With all the autodestruct I'm unsure when is calling vmstate_unregister_ram > > appropriate. > > Is it necessary to invoke that from pci any longer? > > > > I think vmstate_unregister_ram is not necessary at all. This patch, or > Alex's suggestion, are smaller changes in that direction---more suitable > as we're closer to the release. > > Paolo Oh absolutely. I was just wandering what am I missing. Cédric would you be interested in posting a patch removing vmstate_unregister_ram after release? You can do a series starting with this one. -- MST
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On 06/07/2018 18:25, Michael S. Tsirkin wrote: > On Fri, Jul 06, 2018 at 05:55:23PM +0200, Paolo Bonzini wrote: >> On 06/07/2018 03:51, Peter Xu wrote: >>> >>> A question about memory region auto destruction (which might not >>> related to this patch): I see that we have object_property_add_child() >>> in memory_region_do_init() to achieve the auto destruction but only if >>> the "name" of memory region is specified. Could we just do that >>> unconditionally (though we might of course need to generate some of >>> the names), or is there a reason not to do so? >> >> I'm not sure actually if there are still regions without a name... >> >> Paolo > > Answer to Peter's question would be a yes then? > > With all the autodestruct I'm unsure when is calling vmstate_unregister_ram > appropriate. > Is it necessary to invoke that from pci any longer? > I think vmstate_unregister_ram is not necessary at all. This patch, or Alex's suggestion, are smaller changes in that direction---more suitable as we're closer to the release. Paolo
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On 07/06/2018 06:25 PM, Michael S. Tsirkin wrote: > On Fri, Jul 06, 2018 at 05:55:23PM +0200, Paolo Bonzini wrote: >> On 06/07/2018 03:51, Peter Xu wrote: >>> >>> A question about memory region auto destruction (which might not >>> related to this patch): I see that we have object_property_add_child() >>> in memory_region_do_init() to achieve the auto destruction but only if >>> the "name" of memory region is specified. Could we just do that >>> unconditionally (though we might of course need to generate some of >>> the names), or is there a reason not to do so? >> >> I'm not sure actually if there are still regions without a name... >> >> Paolo > > Answer to Peter's question would be a yes then? > > With all the autodestruct I'm unsure when is calling vmstate_unregister_ram > appropriate. > Is it necessary to invoke that from pci any longer? That could be another patch though. This is trying to fix vfio-pci unplug. C.
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On Fri, Jul 06, 2018 at 05:55:23PM +0200, Paolo Bonzini wrote: > On 06/07/2018 03:51, Peter Xu wrote: > > > > A question about memory region auto destruction (which might not > > related to this patch): I see that we have object_property_add_child() > > in memory_region_do_init() to achieve the auto destruction but only if > > the "name" of memory region is specified. Could we just do that > > unconditionally (though we might of course need to generate some of > > the names), or is there a reason not to do so? > > I'm not sure actually if there are still regions without a name... > > Paolo Answer to Peter's question would be a yes then? With all the autodestruct I'm unsure when is calling vmstate_unregister_ram appropriate. Is it necessary to invoke that from pci any longer? -- MST
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On 06/07/2018 03:51, Peter Xu wrote: > > A question about memory region auto destruction (which might not > related to this patch): I see that we have object_property_add_child() > in memory_region_do_init() to achieve the auto destruction but only if > the "name" of memory region is specified. Could we just do that > unconditionally (though we might of course need to generate some of > the names), or is there a reason not to do so? I'm not sure actually if there are still regions without a name... Paolo
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On Thu, Jul 05, 2018 at 11:44:44PM +0200, Paolo Bonzini wrote:
> On 05/07/2018 21:30, Alex Williamson wrote:
> > On Thu, 5 Jul 2018 20:11:48 +0200
> > Cédric Le Goater wrote:
> >
> >> PCI devices needing a ROM allocate an optional MemoryRegion with
> >> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
> >> device is destroyed. The only action taken by this routine is to call
> >> vmstate_unregister_ram() which clears the id string of the optional
> >> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
> >> was recently added by commit b895de502717 ("migration: discard
> >> non-migratable RAMBlocks"), .
> >>
> >> VFIO devices do their own allocation of the PCI ROM region. It is
> >> initialized in vfio_pci_size_rom() in which the PCI attribute
> >> 'has_rom' is set to true but the RAMBlock of the ROM region is not
> >> allocated. When the associated PCI device is deleted,
> >> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
> >> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
> >>
> >> The use of vmstate_unregister_ram() in the PCI device was added in
> >> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
> >> when unregister MemoryRegion") and from the archive in
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
> >> seems that it was trying to fix a reference count issue.
> >>
> >> vmstate_unregister_ram() being a work around, let's remove it to fix
> >> the current SEGV issue and let's try to find a fix for the initial ref
> >> count issue if we can reproduce.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> hw/pci/pci.c | 11 ---
> >> 1 file changed, 11 deletions(-)
> >
> > Looking back through git history, I think vfio sets PCIDevice.has_rom
> > because we needed that to have memory_region_destroy() called, but that
> > changed with Paolo's:
> >
> > 469b046ead06 ("memory: remove memory_region_destroy")
> >
> > Now the MemoryRegion gets freed automagically, so maybe the better
> > option is that vfio-pci should not set has_rom to keep it out of this
> > path. I don't see that has_rom serves any other purpose. Thanks,
>
> That would work for me too!
Paolo,
A question about memory region auto destruction (which might not
related to this patch): I see that we have object_property_add_child()
in memory_region_do_init() to achieve the auto destruction but only if
the "name" of memory region is specified. Could we just do that
unconditionally (though we might of course need to generate some of
the names), or is there a reason not to do so?
Also, not sure whether it's good we add some more comments to
memory_region_do_init() to mention about its auto destruction
capability, and if the "name" check is needed, possibly we comment
that as well (I can do that after I understand it well, if you like)?
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On 05/07/2018 21:30, Alex Williamson wrote:
> On Thu, 5 Jul 2018 20:11:48 +0200
> Cédric Le Goater wrote:
>
>> PCI devices needing a ROM allocate an optional MemoryRegion with
>> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
>> device is destroyed. The only action taken by this routine is to call
>> vmstate_unregister_ram() which clears the id string of the optional
>> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
>> was recently added by commit b895de502717 ("migration: discard
>> non-migratable RAMBlocks"), .
>>
>> VFIO devices do their own allocation of the PCI ROM region. It is
>> initialized in vfio_pci_size_rom() in which the PCI attribute
>> 'has_rom' is set to true but the RAMBlock of the ROM region is not
>> allocated. When the associated PCI device is deleted,
>> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
>> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
>>
>> The use of vmstate_unregister_ram() in the PCI device was added in
>> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
>> when unregister MemoryRegion") and from the archive in
>> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
>> seems that it was trying to fix a reference count issue.
>>
>> vmstate_unregister_ram() being a work around, let's remove it to fix
>> the current SEGV issue and let's try to find a fix for the initial ref
>> count issue if we can reproduce.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> hw/pci/pci.c | 11 ---
>> 1 file changed, 11 deletions(-)
>
> Looking back through git history, I think vfio sets PCIDevice.has_rom
> because we needed that to have memory_region_destroy() called, but that
> changed with Paolo's:
>
> 469b046ead06 ("memory: remove memory_region_destroy")
>
> Now the MemoryRegion gets freed automagically, so maybe the better
> option is that vfio-pci should not set has_rom to keep it out of this
> path. I don't see that has_rom serves any other purpose. Thanks,
That would work for me too!
Paolo
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On 07/05/2018 09:11 PM, Michael S. Tsirkin wrote:
> On Thu, Jul 05, 2018 at 08:11:48PM +0200, Cédric Le Goater wrote:
>> PCI devices needing a ROM allocate an optional MemoryRegion with
>> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
>> device is destroyed. The only action taken by this routine is to call
>> vmstate_unregister_ram() which clears the id string of the optional
>> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
>> was recently added by commit b895de502717 ("migration: discard
>> non-migratable RAMBlocks"), .
>>
>> VFIO devices do their own allocation of the PCI ROM region. It is
>> initialized in vfio_pci_size_rom() in which the PCI attribute
>> 'has_rom' is set to true but the RAMBlock of the ROM region is not
>> allocated. When the associated PCI device is deleted,
>> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
>> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
>>
>> The use of vmstate_unregister_ram() in the PCI device was added in
>> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
>> when unregister MemoryRegion")
>
> I don't see it in that commit. I think it was part of the original
> split by Avi.
ok. That was pointed out to me by Paolo. It is hard to track all
the changes.
>> and from the archive in
>> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
>> seems that it was trying to fix a reference count issue.
>>
>> vmstate_unregister_ram() being a work around, let's remove it to fix
>> the current SEGV issue
>> and let's try to find a fix for the initial ref
>> count issue if we can reproduce.
>>
>> Signed-off-by: Cédric Le Goater
>
> What kind of testing did you do on this patch? Could you include
> that info in the commit log pls?
>
> I think you need to at least add/remove some devices, then migrate.
ok, for next round. I plugged/unplugged a :
0034:01:00.0 Ethernet controller: Mellanox Technologies MT27710 Family
[ConnectX-4 Lx]
and then migrated.
Here is the original segv backtrace:
I caught this bug while deleting a passthrough device from a pseries
machine. Here is the stack:
#0 qemu_ram_unset_migratable (rb=0x0) at
/home/legoater/work/qemu/qemu-xive-3.0.git/exec.c:1994
#1 0x00010072def0 in vmstate_unregister_ram (mr=0x101796af0,
dev=)
#2 0x000100694e5c in pci_del_option_rom (pdev=0x101796330)
#3 pci_qdev_unrealize (dev=, errp=)
#4 0x0001005ff910 in device_set_realized (obj=0x101796330,
value=, errp=0x0)
#5 0x0001007a487c in property_set_bool (obj=0x101796330, v=, name=,
#6 0x0001007a7878 in object_property_set (obj=0x101796330,
v=0x7fff70033110,
#7 0x0001007aaf1c in object_property_set_qobject (obj=0x101796330,
value=,
#8 0x0001007a7b90 in object_property_set_bool (obj=0x101796330,
value=,
#9 0x0001005fcdd8 in device_unparent (obj=0x101796330)
#10 0x0001007a6dd0 in object_finalize_child_property (obj=, name=,
#11 0x0001007a50c0 in object_property_del_child (obj=0x10111f800,
child=0x101796330,
#12 0x000100425cc0 in spapr_phb_remove_pci_device_cb (dev=0x101796330)
#13 0x000100427974 in spapr_drc_release (drc=0x1017e2df0)
#14 0x000100429098 in spapr_drc_detach (drc=0x1017e2df0)
#15 0x0001004294e0 in drc_isolate_physical (drc=0x1017e2df0)
#16 0x00010042a50c in rtas_set_isolation_state (state=0, idx=)
C.
>
>> ---
>> hw/pci/pci.c | 11 ---
>> 1 file changed, 11 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 80bc45930dee..78bf74e19f22 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -191,7 +191,6 @@ static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
>> static void pci_update_mappings(PCIDevice *d);
>> static void pci_irq_handler(void *opaque, int irq_num, int level);
>> static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error
>> **);
>> -static void pci_del_option_rom(PCIDevice *pdev);
>>
>> static uint16_t pci_default_sub_vendor_id =
>> PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
>> static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU;
>> @@ -1096,7 +1095,6 @@ static void pci_qdev_unrealize(DeviceState *dev, Error
>> **errp)
>> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>>
>> pci_unregister_io_regions(pci_dev);
>> -pci_del_option_rom(pci_dev);
>>
>> if (pc->exit) {
>> pc->exit(pci_dev);
>> @@ -2262,15 +2260,6 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
>> is_default_rom,
>> pci_register_bar(pdev, PCI_ROM_SLOT, 0, >rom);
>> }
>>
>> -static void pci_del_option_rom(PCIDevice *pdev)
>> -{
>> -if (!pdev->has_rom)
>> -return;
>> -
>> -vmstate_unregister_ram(>rom, >qdev);
>> -pdev->has_rom = false;
>> -}
>> -
>> /*
>> * On success, pci_add_capability() returns a positive value
>> * that the offset of the pci capability.
>> --
>> 2.13.6
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On Thu, 5 Jul 2018 20:11:48 +0200
Cédric Le Goater wrote:
> PCI devices needing a ROM allocate an optional MemoryRegion with
> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
> device is destroyed. The only action taken by this routine is to call
> vmstate_unregister_ram() which clears the id string of the optional
> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
> was recently added by commit b895de502717 ("migration: discard
> non-migratable RAMBlocks"), .
>
> VFIO devices do their own allocation of the PCI ROM region. It is
> initialized in vfio_pci_size_rom() in which the PCI attribute
> 'has_rom' is set to true but the RAMBlock of the ROM region is not
> allocated. When the associated PCI device is deleted,
> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
>
> The use of vmstate_unregister_ram() in the PCI device was added in
> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
> when unregister MemoryRegion") and from the archive in
> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
> seems that it was trying to fix a reference count issue.
>
> vmstate_unregister_ram() being a work around, let's remove it to fix
> the current SEGV issue and let's try to find a fix for the initial ref
> count issue if we can reproduce.
>
> Signed-off-by: Cédric Le Goater
> ---
> hw/pci/pci.c | 11 ---
> 1 file changed, 11 deletions(-)
Looking back through git history, I think vfio sets PCIDevice.has_rom
because we needed that to have memory_region_destroy() called, but that
changed with Paolo's:
469b046ead06 ("memory: remove memory_region_destroy")
Now the MemoryRegion gets freed automagically, so maybe the better
option is that vfio-pci should not set has_rom to keep it out of this
path. I don't see that has_rom serves any other purpose. Thanks,
Alex
Re: [Qemu-devel] [PATCH] pci: remove pci_del_option_rom()
On Thu, Jul 05, 2018 at 08:11:48PM +0200, Cédric Le Goater wrote:
> PCI devices needing a ROM allocate an optional MemoryRegion with
> pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
> device is destroyed. The only action taken by this routine is to call
> vmstate_unregister_ram() which clears the id string of the optional
> ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
> was recently added by commit b895de502717 ("migration: discard
> non-migratable RAMBlocks"), .
>
> VFIO devices do their own allocation of the PCI ROM region. It is
> initialized in vfio_pci_size_rom() in which the PCI attribute
> 'has_rom' is set to true but the RAMBlock of the ROM region is not
> allocated. When the associated PCI device is deleted,
> pci_del_option_rom() calls vmstate_unregister_ram() which tries to
> flag a NULL RAMBlock because 'has_rom' is set, leading to a SEGV .
>
> The use of vmstate_unregister_ram() in the PCI device was added in
> commit b0e56e0b63f350691b52d3e75e89bb64143fbeff ("unset RAMBlock idstr
> when unregister MemoryRegion")
I don't see it in that commit. I think it was part of the original
split by Avi.
> and from the archive in
> http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00282.html, it
> seems that it was trying to fix a reference count issue.
>
> vmstate_unregister_ram() being a work around, let's remove it to fix
> the current SEGV issue
> and let's try to find a fix for the initial ref
> count issue if we can reproduce.
>
> Signed-off-by: Cédric Le Goater
What kind of testing did you do on this patch? Could you include
that info in the commit log pls?
I think you need to at least add/remove some devices, then migrate.
> ---
> hw/pci/pci.c | 11 ---
> 1 file changed, 11 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 80bc45930dee..78bf74e19f22 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -191,7 +191,6 @@ static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
> static void pci_update_mappings(PCIDevice *d);
> static void pci_irq_handler(void *opaque, int irq_num, int level);
> static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error
> **);
> -static void pci_del_option_rom(PCIDevice *pdev);
>
> static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
> static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU;
> @@ -1096,7 +1095,6 @@ static void pci_qdev_unrealize(DeviceState *dev, Error
> **errp)
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>
> pci_unregister_io_regions(pci_dev);
> -pci_del_option_rom(pci_dev);
>
> if (pc->exit) {
> pc->exit(pci_dev);
> @@ -2262,15 +2260,6 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom,
> pci_register_bar(pdev, PCI_ROM_SLOT, 0, >rom);
> }
>
> -static void pci_del_option_rom(PCIDevice *pdev)
> -{
> -if (!pdev->has_rom)
> -return;
> -
> -vmstate_unregister_ram(>rom, >qdev);
> -pdev->has_rom = false;
> -}
> -
> /*
> * On success, pci_add_capability() returns a positive value
> * that the offset of the pci capability.
> --
> 2.13.6
