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

[BALATON Zoltan via]

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é <f4...@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é <f4...@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