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

[Philippe Mathieu-Daudé]

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