Re: [PATCH v5 05/11] exec/ioport: Add portio_list_set_address()

[Bernhard Beschow]

Am 29. Juli 2024 09:26:19 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 14/1/24 13:39, Bernhard Beschow wrote:
>> Some SuperI/O devices such as the VIA south bridges or the PC87312 controller
>> are able to relocate their SuperI/O functions. Add a convenience function for
>> implementing this in the VIA south bridges.
>> 
>> This convenience function relies on previous simplifications in exec/ioport
>> which avoids some duplicate synchronization of I/O port base addresses. The
>> naming of the function is inspired by its memory_region_set_address() 
>> pendant.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   docs/devel/migration.rst |  5 +++--
>>   include/exec/ioport.h    |  2 ++
>>   system/ioport.c          | 19 +++++++++++++++++++
>>   3 files changed, 24 insertions(+), 2 deletions(-)
>
>
>> +void portio_list_set_address(PortioList *piolist, uint32_t addr)
>> +{
>> +    MemoryRegionPortioList *mrpio;
>> +    unsigned i, j;
>> +
>
>       memory_region_transaction_begin();
>
>> +    for (i = 0; i < piolist->nr; ++i) {
>> +        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, 
>> mr);
>
>Should we check mrpio->mr is disabled before changing its base address?

Isn't that the responsibility of the guest? What should we do if the check 
fails?

>
>> +        memory_region_set_address(&mrpio->mr,
>> +                                  mrpio->mr.addr - piolist->addr + addr);
>> +        for (j = 0; mrpio->ports[j].size; ++j) {
>> +            mrpio->ports[j].offset += addr - piolist->addr;
>> +        }
>
>          memory_region_transaction_commit();
>
>> +    }
>> +
>> +    piolist->addr = addr;
>> +}
>> +
>>   static void memory_region_portio_list_finalize(Object *obj)
>>   {
>>       MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
>