| Message ID | 20240114123911.4877-6-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions | expand |
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? > + 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);
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); >
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); >>
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
diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index 95351ba51f..30b05f0f74 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -452,10 +452,10 @@ data doesn't match the stored device data well; it allows an intermediate temporary structure to be populated with migration data and then transferred to the main structure. -If you use memory API functions that update memory layout outside +If you use memory or portio_list API functions that update memory layout outside initialization (i.e., in response to a guest action), this is a strong indication that you need to call these functions in a ``post_load`` callback. -Examples of such memory API functions are: +Examples of such API functions are: - memory_region_add_subregion() - memory_region_del_subregion() @@ -464,6 +464,7 @@ Examples of such memory API functions are: - memory_region_set_enabled() - memory_region_set_address() - memory_region_set_alias_offset() + - portio_list_set_address() Iterative device migration -------------------------- diff --git a/include/exec/ioport.h b/include/exec/ioport.h index 95f1dc30d0..96858e5ac3 100644 --- a/include/exec/ioport.h +++ b/include/exec/ioport.h @@ -54,6 +54,7 @@ typedef struct PortioList { const struct MemoryRegionPortio *ports; Object *owner; struct MemoryRegion *address_space; + uint32_t addr; unsigned nr; struct MemoryRegion **regions; void *opaque; @@ -70,5 +71,6 @@ void portio_list_add(PortioList *piolist, struct MemoryRegion *address_space, uint32_t addr); void portio_list_del(PortioList *piolist); +void portio_list_set_address(PortioList *piolist, uint32_t addr); #endif /* IOPORT_H */ diff --git a/system/ioport.c b/system/ioport.c index a59e58b716..000e0ee1af 100644 --- a/system/ioport.c +++ b/system/ioport.c @@ -133,6 +133,7 @@ void portio_list_init(PortioList *piolist, piolist->nr = 0; piolist->regions = g_new0(MemoryRegion *, n); piolist->address_space = NULL; + piolist->addr = 0; piolist->opaque = opaque; piolist->owner = owner; piolist->name = name; @@ -282,6 +283,7 @@ void portio_list_add(PortioList *piolist, unsigned int off_low, off_high, off_last, count; piolist->address_space = address_space; + piolist->addr = start; /* Handle the first entry specially. */ off_last = off_low = pio_start->offset; @@ -322,6 +324,23 @@ void portio_list_del(PortioList *piolist) } } +void portio_list_set_address(PortioList *piolist, uint32_t addr) +{ + MemoryRegionPortioList *mrpio; + unsigned i, j; + + for (i = 0; i < piolist->nr; ++i) { + mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr); + 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; + } + } + + piolist->addr = addr; +} + static void memory_region_portio_list_finalize(Object *obj) { MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
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(-)