Re: [PATCH 4/4] hw/pci: Ensure pci_add_capability() is called before device is realized

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 22/3/23 09:52, Philippe Mathieu-Daudé wrote:
> On 22/3/23 03:18, Michael S. Tsirkin wrote:
>> On Tue, Mar 14, 2023 at 12:14:35PM +0100, Philippe Mathieu-Daudé wrote:
>>> PCI capabilities can't appear magically at runtime.
>>> Guests aren't expecting that. Assert all capabilities
>>> are added _before_ a device instance is realized.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/pci/pci.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index ac41fcbf6a..ed60b352e4 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2397,7 +2397,7 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>>    * On success, pci_add_capability() returns a positive value
>>>    * that the offset of the pci capability.
>>>    * On failure, it sets an error and returns a negative error
>>> - * code.
>>> + * code. @pdev must be unrealized.
>>>    */
>>>   int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>>                          uint8_t offset, uint8_t size,
>>> @@ -2406,6 +2406,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t 
>>> cap_id,
>>>       uint8_t *config;
>>>       int i, overlapping_cap;
>>> +    assert(!DEVICE(pdev)->realized);
>>> +
>>>       if (!offset) {
>>>           offset = pci_find_space(pdev, size);
>>>           /* out of PCI config space is programming error */
>>
>> Fails in CI:
>>
>> https://gitlab.com/mstredhat/qemu/-/jobs/3976974199
>>
>> qemu-system-i386: ../hw/pci/pci.c:2409: pci_add_capability: Assertion 
>> `!DEVICE(pdev)->realized' failed.
>> Broken pipe
>> ../tests/qtest/libqtest.c:193: kill_qemu() detected QEMU death from 
>> signal 6 (Aborted) (core dumped)
>> TAP parsing error: Too few tests run (expected 49, got 40)
>> (test program exited with status code -6)
> 
> Thanks for testing!
> 
> Likely the AMD-Vi device, see on the cover this series is
> Based-on: <20230313153031.86107-1-philmd@linaro.org>
>            "hw/i386/amd_iommu: Orphanize & QDev cleanups"
> https://lore.kernel.org/qemu-devel/20230313153031.86107-1-philmd@linaro.org/

I confirm this is the AMD-Vi device, so you are missing the
previous (based-on) series:

     #1 0x102d4e5b0 in pci_add_capability pci.c:2354
     #2 0x102d2ff28 in msi_init msi.c:227
     #3 0x10371a340 in amdvi_sysbus_realize amd_iommu.c:1553
     #4 0x1037194e8 in x86_iommu_realize x86-iommu.c:124
     #5 0x10409db88 in device_set_realized+0x788 
(qemu-system-i386:arm64+0x101d91b88)
     #6 0x1040cb248 in property_set_bool+0x2a0 
(qemu-system-i386:arm64+0x101dbf248)
     #7 0x1040c31f4 in object_property_set+0x4bc 
(qemu-system-i386:arm64+0x101db71f4)
     #8 0x1040d9990 in object_property_set_qobject+0x38 
(qemu-system-i386:arm64+0x101dcd990)
     #9 0x1040c40f8 in object_property_set_bool+0xfc 
(qemu-system-i386:arm64+0x101db80f8)
     #10 0x10409639c in qdev_realize+0x3bc 
(qemu-system-i386:arm64+0x101d8a39c)
     #11 0x10334f8e8 in qdev_device_add_from_qdict qdev-monitor.c:714
     #12 0x103352114 in qdev_device_add qdev-monitor.c:733
     #13 0x10337906c in device_init_func vl.c:1140
     #14 0x104a84200 in qemu_opts_foreach qemu-option.c:1135
     #15 0x103364fcc in qemu_create_cli_devices vl.c:2541
     #16 0x103364ab8 in qmp_x_exit_preconfig vl.c:2609
     #17 0x10336c0dc in qemu_init vl.c:3611
     #18 0x1040812e4 in main main.c:47
     #19 0x1a025be4c  (<unknown module>)

Due to the required base series, this is 8.1 material.