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

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
>
>
>

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
>>
>>
>>