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 <clg@kaod.org>
---
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, &pdev->rom);
}
-static void pci_del_option_rom(PCIDevice *pdev)
-{
- if (!pdev->has_rom)
- return;
-
- vmstate_unregister_ram(&pdev->rom, &pdev->qdev);
- pdev->has_rom = false;
-}
-
/*
* On success, pci_add_capability() returns a positive value
* that the offset of the pci capability.
--
2.13.6
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 <clg@kaod.org>
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, &pdev->rom);
> }
>
> -static void pci_del_option_rom(PCIDevice *pdev)
> -{
> - if (!pdev->has_rom)
> - return;
> -
> - vmstate_unregister_ram(&pdev->rom, &pdev->qdev);
> - pdev->has_rom = false;
> -}
> -
> /*
> * On success, pci_add_capability() returns a positive value
> * that the offset of the pci capability.
> --
> 2.13.6
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 <clg@kaod.org>
>
> 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 0x000000010072def0 in vmstate_unregister_ram (mr=0x101796af0, dev=<optimized out>)
#2 0x0000000100694e5c in pci_del_option_rom (pdev=0x101796330)
#3 pci_qdev_unrealize (dev=<optimized out>, errp=<optimized out>)
#4 0x00000001005ff910 in device_set_realized (obj=0x101796330, value=<optimized out>, errp=0x0)
#5 0x00000001007a487c in property_set_bool (obj=0x101796330, v=<optimized out>, name=<optimized out>,
#6 0x00000001007a7878 in object_property_set (obj=0x101796330, v=0x7fff70033110,
#7 0x00000001007aaf1c in object_property_set_qobject (obj=0x101796330, value=<optimized out>,
#8 0x00000001007a7b90 in object_property_set_bool (obj=0x101796330, value=<optimized out>,
#9 0x00000001005fcdd8 in device_unparent (obj=0x101796330)
#10 0x00000001007a6dd0 in object_finalize_child_property (obj=<optimized out>, name=<optimized out>,
#11 0x00000001007a50c0 in object_property_del_child (obj=0x10111f800, child=0x101796330,
#12 0x0000000100425cc0 in spapr_phb_remove_pci_device_cb (dev=0x101796330)
#13 0x0000000100427974 in spapr_drc_release (drc=0x1017e2df0)
#14 0x0000000100429098 in spapr_drc_detach (drc=0x1017e2df0)
#15 0x00000001004294e0 in drc_isolate_physical (drc=0x1017e2df0)
#16 0x000000010042a50c in rtas_set_isolation_state (state=0, idx=<optimized out>)
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, &pdev->rom);
>> }
>>
>> -static void pci_del_option_rom(PCIDevice *pdev)
>> -{
>> - if (!pdev->has_rom)
>> - return;
>> -
>> - vmstate_unregister_ram(&pdev->rom, &pdev->qdev);
>> - pdev->has_rom = false;
>> -}
>> -
>> /*
>> * On success, pci_add_capability() returns a positive value
>> * that the offset of the pci capability.
>> --
>> 2.13.6
On Thu, 5 Jul 2018 20:11:48 +0200
Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> ---
> 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
On 05/07/2018 21:30, Alex Williamson wrote:
> On Thu, 5 Jul 2018 20:11:48 +0200
> Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
>> ---
>> 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
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 <clg@kaod.org> 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 <clg@kaod.org>
> >> ---
> >> 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
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
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
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.
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
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
© 2016 - 2025 Red Hat, Inc.