On Sat, 2 Jan 2021, Peter Maydell wrote:
On Sat, 2 Jan 2021 at 11:22, BALATON Zoltan <bala...@eik.bme.hu> wrote:
I have similar code in the series I've just posted where I'm mapping
regions of serial devices. I did consider using set_enabled and
set_address but ended up with removing and adding regions because I'm not
sure what happens if guest tries to move one region over another like
having one region at a default location while guest tries to map the other
one there (the pegasos2 maps serial at 0x2f8 which is normally COM2 on a
PC). This should not happen in theory but when removing disabled regions
it cannot happen so that looks safer therefore I chose to do that. Not
sure if this could be a problem here just shared my thughts about this.
I'm not sure what you have in mind -- could you explain further?
There should be no difference as far as the MemoryRegion handling
code is concerned between "this memory region is marked disabled" and
"the memory region was deleted and will be created from fresh and added
back later" -- an MR that's in the hierarchy but not enabled is
entirely ignored, as if it wasn't there at all, when creating the
flat-view.
The device I was implementing has two registers one to set base address of
io region and another with bits to enable/disable the regions so I could
do set_address for base regs and set_enabled for control reg bits but I've
seen guests first flipping the enable bits on then setting the base
address so I thought it might cause problems with regions added to their
parent but thinking about it more it's probably the same if we remove
regions and add them instead of just set_enabled because they should be
readded when control reg bits are set so they'll end up at the same
default address.
That said, doing memory_region_del_subregion()/memory_region_add_subregion()
I think is also OK -- what's definitely not required is actually
deleting and recreating the MRs the way this code is doing.
Anyway that's what I ended up doing and did not notice that this patch was
also deleting and recreating the memory regions which I did not do just
removing from parent when they are disabled but using set_address if they
are enabled and new base is set. Removing inactive regions maybe better
for debugging because they show up in info mtree so one can see which one
is enabled/disabled not sure how disabled regions show up though.
All in all I probably have nothing to add to this so just disregard my
comment.
Regards,
BALATON Zoltan