Per the datasheet (Chapter 5.7.1. "PCI address regions"),
the PCIMAP register:

  Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
  each of which can be assigned to any 64 Mbyte-aligned region of
  PCI memory. The address appearing on the PCI bus consists of the
  low 26 bits of the CPU physical address, with the high 6 bits
  coming from the appropriate base6 field. Each of the three regions
  is an independent window onto PCI memory, and can be positioned on
  any 64Mbyte boundary in PCI space.

Remap the 3 regions on reset and when PCIMAP is updated.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index a99eced0657..c58eeaf504c 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
 
 /* 4. PCI address map control */
 #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
+FIELD(PCIMAP, LO0,               0, 6)
+FIELD(PCIMAP, LO1,               6, 6)
+FIELD(PCIMAP, LO2,              12, 6)
+FIELD(PCIMAP, 2G,               18, 1)
 #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
 #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
 
@@ -237,6 +241,7 @@ struct BonitoState {
     qemu_irq *pic;
     PCIBonitoState *pci_dev;
     MemoryRegion pci_mem;
+    MemoryRegion pcimem_lo_alias[3];
 };
 
 #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
@@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
 #define TYPE_PCI_BONITO "Bonito"
 OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
 
+static void bonito_remap(PCIBonitoState *s)
+{
+    static const char *const region_name[3] = {
+        "pci.lomem0", "pci.lomem1", "pci.lomem2"
+    };
+    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
+
+    for (size_t i = 0; i < 3; i++) {
+        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
+
+        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
+            memory_region_del_subregion(get_system_memory(),
+                                        &bs->pcimem_lo_alias[i]);
+            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
+        }
+
+        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
+                                 region_name[i], &bs->pci_mem,
+                                 offset, 64 * MiB);
+        memory_region_add_subregion(get_system_memory(),
+                                    BONITO_PCILO_BASE + i * 64 * MiB,
+                                    &bs->pcimem_lo_alias[i]);
+    }
+}
+
 static void bonito_writel(void *opaque, hwaddr addr,
                           uint64_t val, unsigned size)
 {
@@ -260,7 +290,6 @@ static void bonito_writel(void *opaque, hwaddr addr,
     case BONITO_BONPONCFG:
     case BONITO_IODEVCFG:
     case BONITO_SDCFG:
-    case BONITO_PCIMAP:
     case BONITO_PCIMEMBASECFG:
     case BONITO_PCIMAP_CFG:
     case BONITO_GPIODATA:
@@ -282,6 +311,10 @@ static void bonito_writel(void *opaque, hwaddr addr,
     case BONITO_MEMSIZE:
         s->regs[saddr] = val;
         break;
+    case BONITO_PCIMAP:
+        s->regs[saddr] = val;
+        bonito_remap(s);
+        break;
     case BONITO_BONGENCFG:
         if (!(s->regs[saddr] & 0x04) && (val & 0x04)) {
             reset = 1; /* bit 2 jump from 0 to 1 cause reset */
@@ -610,6 +643,8 @@ static void bonito_reset(void *opaque)
     s->regs[BONITO_DQCFG] = 0x8;
     s->regs[BONITO_MEMSIZE] = 0x10000000;
     s->regs[BONITO_PCIMAP] = 0x6140;
+
+    bonito_remap(s);
 }
 
 static const VMStateDescription vmstate_bonito = {
@@ -626,7 +661,6 @@ static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
 {
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     BonitoState *bs = BONITO_PCI_HOST_BRIDGE(dev);
-    MemoryRegion *pcimem_lo_alias = g_new(MemoryRegion, 3);
 
     memory_region_init(&bs->pci_mem, OBJECT(dev), "pci.mem", BONITO_PCIHI_SIZE);
     phb->bus = pci_register_root_bus(dev, "pci",
@@ -634,17 +668,6 @@ static void bonito_pcihost_realize(DeviceState *dev, Error **errp)
                                      dev, &bs->pci_mem, get_system_io(),
                                      0x28, 32, TYPE_PCI_BUS);
 
-    for (size_t i = 0; i < 3; i++) {
-        char *name = g_strdup_printf("pci.lomem%zu", i);
-
-        memory_region_init_alias(&pcimem_lo_alias[i], NULL, name,
-                                 &bs->pci_mem, i * 64 * MiB, 64 * MiB);
-        memory_region_add_subregion(get_system_memory(),
-                                    BONITO_PCILO_BASE + i * 64 * MiB,
-                                    &pcimem_lo_alias[i]);
-        g_free(name);
-    }
-
     create_unimplemented_device("pci.io", BONITO_PCIIO_BASE, 1 * MiB);
 }
 
-- 
2.26.2

On Fri, 1 Jan 2021 at 23:12, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Per the datasheet (Chapter 5.7.1. "PCI address regions"),
> the PCIMAP register:
>
>   Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
>   each of which can be assigned to any 64 Mbyte-aligned region of
>   PCI memory. The address appearing on the PCI bus consists of the
>   low 26 bits of the CPU physical address, with the high 6 bits
>   coming from the appropriate base6 field. Each of the three regions
>   is an independent window onto PCI memory, and can be positioned on
>   any 64Mbyte boundary in PCI space.
>
> Remap the 3 regions on reset and when PCIMAP is updated.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index a99eced0657..c58eeaf504c 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>
>  /* 4. PCI address map control */
>  #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
> +FIELD(PCIMAP, LO0,               0, 6)
> +FIELD(PCIMAP, LO1,               6, 6)
> +FIELD(PCIMAP, LO2,              12, 6)
> +FIELD(PCIMAP, 2G,               18, 1)
>  #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
>  #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
>
> @@ -237,6 +241,7 @@ struct BonitoState {
>      qemu_irq *pic;
>      PCIBonitoState *pci_dev;
>      MemoryRegion pci_mem;
> +    MemoryRegion pcimem_lo_alias[3];
>  };
>
>  #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
> @@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
>  #define TYPE_PCI_BONITO "Bonito"
>  OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
>
> +static void bonito_remap(PCIBonitoState *s)
> +{
> +    static const char *const region_name[3] = {
> +        "pci.lomem0", "pci.lomem1", "pci.lomem2"
> +    };
> +    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
> +
> +    for (size_t i = 0; i < 3; i++) {
> +        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
> +
> +        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
> +            memory_region_del_subregion(get_system_memory(),
> +                                        &bs->pcimem_lo_alias[i]);
> +            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
> +        }
> +
> +        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
> +                                 region_name[i], &bs->pci_mem,
> +                                 offset, 64 * MiB);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
> +                                    &bs->pcimem_lo_alias[i]);
> +    }

Rather than delete-and-reinit-and-add, it's probably better to
just create the subregions once at device startup, and then use
memory_region_set_enabled() and memory_region_set_address()
to manipulate whether the subregion is visible and what address
in the system memory it is mapped at.

thanks
-- PMM

On 1/2/21 12:19 AM, Peter Maydell wrote:
> On Fri, 1 Jan 2021 at 23:12, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Per the datasheet (Chapter 5.7.1. "PCI address regions"),
>> the PCIMAP register:
>>
>>   Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
>>   each of which can be assigned to any 64 Mbyte-aligned region of
>>   PCI memory. The address appearing on the PCI bus consists of the
>>   low 26 bits of the CPU physical address, with the high 6 bits
>>   coming from the appropriate base6 field. Each of the three regions
>>   is an independent window onto PCI memory, and can be positioned on
>>   any 64Mbyte boundary in PCI space.
>>
>> Remap the 3 regions on reset and when PCIMAP is updated.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index a99eced0657..c58eeaf504c 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>>
>>  /* 4. PCI address map control */
>>  #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
>> +FIELD(PCIMAP, LO0,               0, 6)
>> +FIELD(PCIMAP, LO1,               6, 6)
>> +FIELD(PCIMAP, LO2,              12, 6)
>> +FIELD(PCIMAP, 2G,               18, 1)
>>  #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
>>  #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
>>
>> @@ -237,6 +241,7 @@ struct BonitoState {
>>      qemu_irq *pic;
>>      PCIBonitoState *pci_dev;
>>      MemoryRegion pci_mem;
>> +    MemoryRegion pcimem_lo_alias[3];
>>  };
>>
>>  #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
>> @@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
>>  #define TYPE_PCI_BONITO "Bonito"
>>  OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
>>
>> +static void bonito_remap(PCIBonitoState *s)
>> +{
>> +    static const char *const region_name[3] = {
>> +        "pci.lomem0", "pci.lomem1", "pci.lomem2"
>> +    };
>> +    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>> +
>> +    for (size_t i = 0; i < 3; i++) {
>> +        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
>> +
>> +        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
>> +            memory_region_del_subregion(get_system_memory(),
>> +                                        &bs->pcimem_lo_alias[i]);
>> +            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
>> +        }
>> +
>> +        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
>> +                                 region_name[i], &bs->pci_mem,
>> +                                 offset, 64 * MiB);
>> +        memory_region_add_subregion(get_system_memory(),
>> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
>> +                                    &bs->pcimem_lo_alias[i]);
>> +    }
> 
> Rather than delete-and-reinit-and-add, it's probably better to
> just create the subregions once at device startup, and then use
> memory_region_set_enabled() and memory_region_set_address()
> to manipulate whether the subregion is visible and what address
> in the system memory it is mapped at.

Great! Thanks Peter :) TIL these functions.
From what I understand from memory_region_readd_subregion (called
from memory_region_set_address) using memory_region_set_enabled()
directly is enough.

On Sat, 2 Jan 2021, Philippe Mathieu-Daudé wrote:
> On 1/2/21 12:19 AM, Peter Maydell wrote:
>> On Fri, 1 Jan 2021 at 23:12, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> Per the datasheet (Chapter 5.7.1. "PCI address regions"),
>>> the PCIMAP register:
>>>
>>>   Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
>>>   each of which can be assigned to any 64 Mbyte-aligned region of
>>>   PCI memory. The address appearing on the PCI bus consists of the
>>>   low 26 bits of the CPU physical address, with the high 6 bits
>>>   coming from the appropriate base6 field. Each of the three regions
>>>   is an independent window onto PCI memory, and can be positioned on
>>>   any 64Mbyte boundary in PCI space.
>>>
>>> Remap the 3 regions on reset and when PCIMAP is updated.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
>>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>>> index a99eced0657..c58eeaf504c 100644
>>> --- a/hw/pci-host/bonito.c
>>> +++ b/hw/pci-host/bonito.c
>>> @@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>>>
>>>  /* 4. PCI address map control */
>>>  #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
>>> +FIELD(PCIMAP, LO0,               0, 6)
>>> +FIELD(PCIMAP, LO1,               6, 6)
>>> +FIELD(PCIMAP, LO2,              12, 6)
>>> +FIELD(PCIMAP, 2G,               18, 1)
>>>  #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
>>>  #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
>>>
>>> @@ -237,6 +241,7 @@ struct BonitoState {
>>>      qemu_irq *pic;
>>>      PCIBonitoState *pci_dev;
>>>      MemoryRegion pci_mem;
>>> +    MemoryRegion pcimem_lo_alias[3];
>>>  };
>>>
>>>  #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
>>> @@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
>>>  #define TYPE_PCI_BONITO "Bonito"
>>>  OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
>>>
>>> +static void bonito_remap(PCIBonitoState *s)
>>> +{
>>> +    static const char *const region_name[3] = {
>>> +        "pci.lomem0", "pci.lomem1", "pci.lomem2"
>>> +    };
>>> +    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>>> +
>>> +    for (size_t i = 0; i < 3; i++) {
>>> +        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
>>> +
>>> +        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
>>> +            memory_region_del_subregion(get_system_memory(),
>>> +                                        &bs->pcimem_lo_alias[i]);
>>> +            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
>>> +        }
>>> +
>>> +        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
>>> +                                 region_name[i], &bs->pci_mem,
>>> +                                 offset, 64 * MiB);
>>> +        memory_region_add_subregion(get_system_memory(),
>>> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
>>> +                                    &bs->pcimem_lo_alias[i]);
>>> +    }
>>
>> Rather than delete-and-reinit-and-add, it's probably better to
>> just create the subregions once at device startup, and then use
>> memory_region_set_enabled() and memory_region_set_address()
>> to manipulate whether the subregion is visible and what address
>> in the system memory it is mapped at.
>
> Great! Thanks Peter :) TIL these functions.
> From what I understand from memory_region_readd_subregion (called
> from memory_region_set_address) using memory_region_set_enabled()
> directly is enough.

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.

Regards,
BALATON Zoltan

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.

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.

thanks
-- PMM

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

On 1/2/21 11:44 AM, Philippe Mathieu-Daudé wrote:
> On 1/2/21 12:19 AM, Peter Maydell wrote:
>> On Fri, 1 Jan 2021 at 23:12, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> Per the datasheet (Chapter 5.7.1. "PCI address regions"),
>>> the PCIMAP register:
>>>
>>>   Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
>>>   each of which can be assigned to any 64 Mbyte-aligned region of
>>>   PCI memory. The address appearing on the PCI bus consists of the
>>>   low 26 bits of the CPU physical address, with the high 6 bits
>>>   coming from the appropriate base6 field. Each of the three regions
>>>   is an independent window onto PCI memory, and can be positioned on
>>>   any 64Mbyte boundary in PCI space.
>>>
>>> Remap the 3 regions on reset and when PCIMAP is updated.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
>>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>>> index a99eced0657..c58eeaf504c 100644
>>> --- a/hw/pci-host/bonito.c
>>> +++ b/hw/pci-host/bonito.c
>>> @@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>>>
>>>  /* 4. PCI address map control */
>>>  #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
>>> +FIELD(PCIMAP, LO0,               0, 6)
>>> +FIELD(PCIMAP, LO1,               6, 6)
>>> +FIELD(PCIMAP, LO2,              12, 6)
>>> +FIELD(PCIMAP, 2G,               18, 1)
>>>  #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
>>>  #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
>>>
>>> @@ -237,6 +241,7 @@ struct BonitoState {
>>>      qemu_irq *pic;
>>>      PCIBonitoState *pci_dev;
>>>      MemoryRegion pci_mem;
>>> +    MemoryRegion pcimem_lo_alias[3];
>>>  };
>>>
>>>  #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
>>> @@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
>>>  #define TYPE_PCI_BONITO "Bonito"
>>>  OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
>>>
>>> +static void bonito_remap(PCIBonitoState *s)
>>> +{
>>> +    static const char *const region_name[3] = {
>>> +        "pci.lomem0", "pci.lomem1", "pci.lomem2"
>>> +    };
>>> +    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>>> +
>>> +    for (size_t i = 0; i < 3; i++) {
>>> +        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
>>> +
>>> +        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
>>> +            memory_region_del_subregion(get_system_memory(),
>>> +                                        &bs->pcimem_lo_alias[i]);
>>> +            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
>>> +        }
>>> +
>>> +        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
>>> +                                 region_name[i], &bs->pci_mem,
>>> +                                 offset, 64 * MiB);
>>> +        memory_region_add_subregion(get_system_memory(),
>>> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
>>> +                                    &bs->pcimem_lo_alias[i]);
>>> +    }
>>
>> Rather than delete-and-reinit-and-add, it's probably better to
>> just create the subregions once at device startup, and then use
>> memory_region_set_enabled() and memory_region_set_address()
>> to manipulate whether the subregion is visible and what address
>> in the system memory it is mapped at.
> 
> Great! Thanks Peter :) TIL these functions.
> From what I understand from memory_region_readd_subregion (called
> from memory_region_set_address) using memory_region_set_enabled()
> directly is enough.

Actually since this is a alias, we don't want to modify the base
address but the alias offset, so I need to use
memory_region_set_alias_offset().