[PATCH 1/2] pci: Change error_report to assert(3)

[PATCH 1/2] pci: Change error_report to assert(3)

[Ben Widawsky]

Asserts are used for developer bugs. As registering a bar of the wrong
size is not something that should be possible for a user to achieve,
this is a developer bug.

While here, use the more obvious helper function.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 hw/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 100c9381c2..2c7d6dd352 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1144,11 +1144,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 
     assert(region_num >= 0);
     assert(region_num < PCI_NUM_REGIONS);
-    if (size & (size-1)) {
-        error_report("ERROR: PCI region size must be pow2 "
-                    "type=0x%x, size=0x%"FMT_PCIBUS"", type, size);
-        exit(1);
-    }
+    assert(is_power_of_2(size));
 
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
-- 
2.28.0

[PATCH v2 2/2] pci: Disallow improper BAR registration for type 1

[Ben Widawsky]

Prevent future developers working on root complexes, root ports, or
bridges that also wish to implement a BAR for those, from shooting
themselves in the foot. PCI type 1 headers only support 2 base address
registers. It is incorrect and difficult to figure out what is wrong
with the device when this mistake is made. With this, it is immediate
and obvious what has gone wrong.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 hw/pci/pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2c7d6dd352..14fce10132 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1141,11 +1141,17 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     uint32_t addr; /* offset in pci config space */
     uint64_t wmask;
     pcibus_t size = memory_region_size(memory);
+    uint8_t hdr_type;
 
     assert(region_num >= 0);
     assert(region_num < PCI_NUM_REGIONS);
     assert(is_power_of_2(size));
 
+    /* A PCI bridge device (with Type 1 header) may only have at most 2 BARs */
+    hdr_type =
+        pci_dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+    assert(hdr_type != PCI_HEADER_TYPE_BRIDGE || region_num < 2);
+
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
-- 
2.28.0

Re: [PATCH 1/2] pci: Change error_report to assert(3)

[Philippe Mathieu-Daudé]

On 10/15/20 8:14 PM, Ben Widawsky wrote:
> Asserts are used for developer bugs. As registering a bar of the wrong
> size is not something that should be possible for a user to achieve,
> this is a developer bug.
> 
> While here, use the more obvious helper function.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>   hw/pci/pci.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/2] pci: Change error_report to assert(3)

[Philippe Mathieu-Daudé]

On 10/15/20 9:55 PM, Philippe Mathieu-Daudé wrote:
> On 10/15/20 8:14 PM, Ben Widawsky wrote:
>> Asserts are used for developer bugs. As registering a bar of the wrong
>> size is not something that should be possible for a user to achieve,
>> this is a developer bug.
>>
>> While here, use the more obvious helper function.
>>
>> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>> ---
>>   hw/pci/pci.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Maybe 'assert(3)' -> 'assert()' in subject.