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

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

On 29/7/24 23:07, Bernhard Beschow wrote:
> 
> 
> 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?

What says the datasheet? At least we should log a GUEST_ERROR here.

> 
>>
>>> +        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);
>>