Re: [PATCH v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap

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

On 21/1/25 05:39, Nicholas Piggin wrote:
> On Mon Jan 20, 2025 at 3:29 PM AEST, Philippe Mathieu-Daudé wrote:
>> Hi Nick,
>>
>> Only nitpicking comments...
> 
> Hey, no they're good comments actually.
> 
>>
>> On 17/1/25 18:22, Nicholas Piggin wrote:
>>> Add assertions to ensure a BAR is not mapped twice, and only
>>> previously mapped BARs are unmapped. This can help catch some
>>> bugs.
>>>
>>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    tests/qtest/libqos/ahci.h       |  1 +
>>>    tests/qtest/libqos/pci.h        |  2 ++
>>>    tests/qtest/libqos/virtio-pci.h |  1 +
>>>    tests/qtest/ahci-test.c         |  2 ++
>>>    tests/qtest/libqos/ahci.c       |  6 ++++++
>>>    tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
>>>    tests/qtest/libqos/virtio-pci.c |  6 +++++-
>>>    7 files changed, 48 insertions(+), 2 deletions(-)
>>
>> Maybe put the AHCI fix in a preliminary patch?
> 
> Yeah, this was just laziness. I will fix.
> 
>>
>>> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
>>> index 83896145235..9dc82ea723a 100644
>>> --- a/tests/qtest/libqos/pci.h
>>> +++ b/tests/qtest/libqos/pci.h
>>
>> Consider using a definition rather than a magic value:
>>
>>     #define PCI_BAR_COUNT 6
> 
> Now I look again at PCI code and it has PCI_NUM_REGIONS 7
> where ROM slot is the last entry. qtests doesn't use it
> AFAIKS but maybe it could(?) so should I just use that
> existing define?

Even if qtests don't use it, using it makes the code clearer IMHO.

Thanks!

Phil.