[PATCH 0/2] hw/pci: catch a few error cases

[PATCH 0/2] hw/pci: catch a few error cases

[Nicholas Piggin]

Hi,

These would have helped to catch a few bugs I ran into / created
recently, so they might be worth upstreaming.

Thanks,
Nick

Nicholas Piggin (2):
  hw/pci/msix: Warn on PBA writes
  hw/pci: Assert a bar is not registered multiple times

 hw/pci/msix.c | 9 +++++++++
 hw/pci/pci.c  | 1 +
 2 files changed, 10 insertions(+)

-- 
2.45.2

[PATCH 1/2] hw/pci/msix: Warn on PBA writes

[Nicholas Piggin]

Of the MSI-X PBA pending bits, the PCI Local Bus Specification says:

  Software should never write, and should only read
  Pending Bits. If software writes to Pending Bits, the
  result is undefined.

Log a GUEST_ERROR message if the PBA is written to by software.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/pci/msix.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 57ec7084a47..66f27b9d712 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -15,6 +15,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "hw/pci/pci.h"
@@ -260,6 +261,14 @@ static uint64_t msix_pba_mmio_read(void *opaque, hwaddr addr,
 static void msix_pba_mmio_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
+    PCIDevice *dev = opaque;
+
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "PCI [%s:%02x:%02x.%x] attempt to write to MSI-X "
+                  "PBA at 0x%" FMT_PCIBUS ", ignoring.\n",
+                  pci_root_bus_path(dev), pci_dev_bus_num(dev),
+                  PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn),
+                  addr);
 }
 
 static const MemoryRegionOps msix_pba_mmio_ops = {
-- 
2.45.2

Re: [PATCH 1/2] hw/pci/msix: Warn on PBA writes

[Akihiko Odaki]

On 2025/01/18 2:28, Nicholas Piggin wrote:
> Of the MSI-X PBA pending bits, the PCI Local Bus Specification says:
> 
>    Software should never write, and should only read
>    Pending Bits. If software writes to Pending Bits, the
>    result is undefined.
> 
> Log a GUEST_ERROR message if the PBA is written to by software.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH 1/2] hw/pci/msix: Warn on PBA writes

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Fri, 17 Jan 2025 at 18:29, Nicholas Piggin <npiggin@gmail.com> wrote:

> Of the MSI-X PBA pending bits, the PCI Local Bus Specification says:
>
>   Software should never write, and should only read
>   Pending Bits. If software writes to Pending Bits, the
>   result is undefined.
>
> Log a GUEST_ERROR message if the PBA is written to by software.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>

Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>


> ---
>  hw/pci/msix.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 57ec7084a47..66f27b9d712 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -15,6 +15,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "hw/pci/msi.h"
>  #include "hw/pci/msix.h"
>  #include "hw/pci/pci.h"
> @@ -260,6 +261,14 @@ static uint64_t msix_pba_mmio_read(void *opaque,
> hwaddr addr,
>  static void msix_pba_mmio_write(void *opaque, hwaddr addr,
>                                  uint64_t val, unsigned size)
>  {
> +    PCIDevice *dev = opaque;
> +
> +    qemu_log_mask(LOG_GUEST_ERROR,
> +                  "PCI [%s:%02x:%02x.%x] attempt to write to MSI-X "
> +                  "PBA at 0x%" FMT_PCIBUS ", ignoring.\n",
> +                  pci_root_bus_path(dev), pci_dev_bus_num(dev),
> +                  PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn),
> +                  addr);
>  }
>
>  static const MemoryRegionOps msix_pba_mmio_ops = {
> --
> 2.45.2
>
>
>

[-- Attachment #2: Type: text/html, Size: 3007 bytes --]

Re: [PATCH 1/2] hw/pci/msix: Warn on PBA writes

[Philippe Mathieu-Daudé]

On 17/1/25 18:28, Nicholas Piggin wrote:
> Of the MSI-X PBA pending bits, the PCI Local Bus Specification says:
> 
>    Software should never write, and should only read
>    Pending Bits. If software writes to Pending Bits, the
>    result is undefined.
> 
> Log a GUEST_ERROR message if the PBA is written to by software.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/pci/msix.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/2] hw/pci: Assert a bar is not registered multiple times

[Nicholas Piggin]

Nothing should be doing this, but it doesn't get caught by
pci_register_bar(). Add an assertion to prevent misuse.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/pci/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2afa423925c..b067a55c5bc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1391,6 +1391,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     assert(hdr_type != PCI_HEADER_TYPE_BRIDGE || region_num < 2);
 
     r = &pci_dev->io_regions[region_num];
+    assert(!r->size);
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
     r->type = type;
-- 
2.45.2

Re: [PATCH 2/2] hw/pci: Assert a bar is not registered multiple times

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

Looks good to me. There is a risk here that the assertion will fail on
existing code. (Unless you've rigorously audited all callers, which would
be quite the task.) However, I agree that this would constitute a bug in
the calling code, not an issue with this change. Since we've still got a
few months left in the 10.0 release cycle, I say go for it - hopefully such
bugs, if there are any, will be shaken out over the next few weeks.

On Fri, 17 Jan 2025 at 18:29, Nicholas Piggin <npiggin@gmail.com> wrote:

> Nothing should be doing this, but it doesn't get caught by
> pci_register_bar(). Add an assertion to prevent misuse.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>


> ---
>  hw/pci/pci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2afa423925c..b067a55c5bc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1391,6 +1391,7 @@ void pci_register_bar(PCIDevice *pci_dev, int
> region_num,
>      assert(hdr_type != PCI_HEADER_TYPE_BRIDGE || region_num < 2);
>
>      r = &pci_dev->io_regions[region_num];
> +    assert(!r->size);
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
>      r->type = type;
> --
> 2.45.2
>
>
>

[-- Attachment #2: Type: text/html, Size: 1957 bytes --]

Re: [PATCH 2/2] hw/pci: Assert a bar is not registered multiple times

[Nicholas Piggin]

On Sun Jan 19, 2025 at 8:38 PM AEST, Phil Dennis-Jordan wrote:
> Looks good to me. There is a risk here that the assertion will fail on
> existing code. (Unless you've rigorously audited all callers, which would
> be quite the task.) However, I agree that this would constitute a bug in
> the calling code, not an issue with this change. Since we've still got a
> few months left in the 10.0 release cycle, I say go for it - hopefully such
> bugs, if there are any, will be shaken out over the next few weeks.

You're right I didn't do an exhaustive audit or test beyond CI and some
browsing. I think it would be quite buggy already if this happens so we
should just catch and fix it quickly, but happy to change to a warning
first if anybody is concerned.

Thanks,
Nick

>
> On Fri, 17 Jan 2025 at 18:29, Nicholas Piggin <npiggin@gmail.com> wrote:
>
>> Nothing should be doing this, but it doesn't get caught by
>> pci_register_bar(). Add an assertion to prevent misuse.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>
>
>> ---
>>  hw/pci/pci.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 2afa423925c..b067a55c5bc 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1391,6 +1391,7 @@ void pci_register_bar(PCIDevice *pci_dev, int
>> region_num,
>>      assert(hdr_type != PCI_HEADER_TYPE_BRIDGE || region_num < 2);
>>
>>      r = &pci_dev->io_regions[region_num];
>> +    assert(!r->size);
>>      r->addr = PCI_BAR_UNMAPPED;
>>      r->size = size;
>>      r->type = type;
>> --
>> 2.45.2
>>
>>
>>