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

[BALATON Zoltan]

On Tue, 30 Jul 2024, Philippe Mathieu-Daudé wrote:
> 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.

It does not say what if it's not enabled but only says that parallel port 
tegs should be located at LPTBase (0xf6 of superio config). So I think 
this should move the memory region independent of if it's enabled but then 
mayube the enable bit should check the address and set it when enabling 
the region? But shouldn't portio take care of that remembering the base? I 
don't know how all this works in QEMU and don't have time to check now.

Reagrds,
BALATON Zoltan