Re: [RFC PATCH 3/5] hw/pci-host/bonito: Remap PCI "lo" regions when PCIMAP reg is modified

[BALATON Zoltan]

On Sat, 2 Jan 2021, Peter Maydell wrote:
> On Sat, 2 Jan 2021 at 11:22, BALATON Zoltan <balaton@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