[PATCH 0/4] hw/pci: Ensure capabilities are added before calling pci_qdev_realize()

[PATCH 0/4] hw/pci: Ensure capabilities are added before calling pci_qdev_realize()

[Philippe Mathieu-Daudé]

Per MST in [*]: "Calling pci_add_capability when VM is running is
likely to confuse guests".
Ensure this can't happen by asserting pci_add_capability() is never
called after a PCI device is realized.

[*] https://lore.kernel.org/qemu-devel/20230308071628-mutt-send-email-mst@kernel.org/
Based-on: <20230313153031.86107-1-philmd@linaro.org>
          "hw/i386/amd_iommu: Orphanize & QDev cleanups"

Philippe Mathieu-Daudé (4):
  hw/pci/msi: Fix debug format string
  hw/pci/msi: Ensure msi_init() is called before device is realized
  hw/pci: Add sanity check in pci_find_space()
  hw/pci: Ensure pci_add_capability() is called before device is
    realized

 hw/pci/msi.c | 4 +++-
 hw/pci/pci.c | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.38.1

[PATCH 1/4] hw/pci/msi: Fix debug format string

[Philippe Mathieu-Daudé]

Fix this format string warning when defining MSI_DEBUG:

  hw/pci/msi.c:209:28: warning: format specifies type 'char' but the argument has type 'unsigned int' [-Wformat]
             offset, nr_vectors, msi64bit, msi_per_vector_mask);
             ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  hw/pci/msi.c:83:61: note: expanded from macro 'MSI_DEV_PRINTF'
      MSI_DPRINTF("%s:%x " fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__)
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
  hw/pci/msi.c:78:58: note: expanded from macro 'MSI_DPRINTF'
      fprintf(stderr, "%s:%d " fmt, __func__, __LINE__, ## __VA_ARGS__)
                               ~~~                         ^~~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 041b0bdbec..5de6df8154 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -204,7 +204,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     }
 
     MSI_DEV_PRINTF(dev,
-                   "init offset: 0x%"PRIx8" vector: %"PRId8
+                   "init offset: 0x%"PRIx8" vector: %u"
                    " 64bit %d mask %d\n",
                    offset, nr_vectors, msi64bit, msi_per_vector_mask);
 
-- 
2.38.1

[PATCH 2/4] hw/pci/msi: Ensure msi_init() is called before device is realized

[Philippe Mathieu-Daudé]

A PCI device can't magically become MSI-capable at runtime.
Guests aren't expecting that. Assert MSI is initialized
_before_ a device instance is realized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 5de6df8154..dfa257cc22 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -182,6 +182,7 @@ bool msi_enabled(const PCIDevice *dev)
  * address.
  * If @msi_per_vector_mask, make the device support per-vector masking.
  * @errp is for returning errors.
+ * @dev must be unrealized.
  * Return 0 on success; set @errp and return -errno on error.
  *
  * -ENOTSUP means lacking msi support for a msi-capable platform.
@@ -208,6 +209,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
                    " 64bit %d mask %d\n",
                    offset, nr_vectors, msi64bit, msi_per_vector_mask);
 
+    assert(!DEVICE(dev)->realized);
     assert(!(nr_vectors & (nr_vectors - 1)));   /* power of 2 */
     assert(nr_vectors > 0);
     assert(nr_vectors <= PCI_MSI_VECTORS_MAX);
-- 
2.38.1

[PATCH 3/4] hw/pci: Add sanity check in pci_find_space()

[Philippe Mathieu-Daudé]

This 'used' array is allocated via:

 pci_qdev_realize() -> do_pci_register_device() -> pci_config_alloc()

In a perfect world where all device models are correctly QOM'ified
this can't happen. Still it occured to me while refactoring QDev and
it was not obvious to figure out. This assert helped, so keep it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..ac41fcbf6a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2185,6 +2185,7 @@ static uint8_t pci_find_space(PCIDevice *pdev, uint8_t size)
 {
     int offset = PCI_CONFIG_HEADER_SIZE;
     int i;
+    assert(pdev->used);
     for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i) {
         if (pdev->used[i])
             offset = i + 1;
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

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 */
-- 
2.38.1

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

[Michael S. Tsirkin]

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)



> -- 
> 2.38.1

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

[Philippe Mathieu-Daudé]

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/

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

[Philippe Mathieu-Daudé]

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.