Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC sys

qemu-ppc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC sys

From:	BALATON Zoltan
Subject:	Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller
Date:	Wed, 17 Mar 2021 02:18:16 +0100 (CET)
User-agent:	Alpine 2.03 (LMD 1266 2009-07-14)

On Wed, 17 Mar 2021, Philippe Mathieu-Daudé wrote:

On 3/16/21 11:03 PM, BALATON Zoltan wrote:

The Marvell Discovery II aka. MV64361 is a PowerPC system controller
chip that is used on the pegasos2 PPC board. This adds emulation of it
that models the device enough to boot guests on this board. The
mv643xx.h header with register definitions is taken from Linux 4.15.10
only fixing white space errors, removing not needed parts and changing
formatting for QEMU coding style.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/Kconfig           |   4 +
 hw/pci-host/meson.build       |   2 +
 hw/pci-host/mv64361.c         | 966 ++++++++++++++++++++++++++++++++++
 hw/pci-host/mv643xx.h         | 918 ++++++++++++++++++++++++++++++++
 hw/pci-host/trace-events      |   6 +
 include/hw/pci-host/mv64361.h |   8 +
 include/hw/pci/pci_ids.h      |   1 +
 7 files changed, 1905 insertions(+)
 create mode 100644 hw/pci-host/mv64361.c
 create mode 100644 hw/pci-host/mv643xx.h
 create mode 100644 include/hw/pci-host/mv64361.h

+static void unmap_region(MemoryRegion *mr)
+{
+    if (memory_region_is_mapped(mr)) {
+        memory_region_del_subregion(get_system_memory(), mr);
+        object_unparent(OBJECT(mr));
+    }
+}
+
+static void map_pci_region(MemoryRegion *mr, MemoryRegion *parent,
+                           struct Object *owner, const char *name,
+                           hwaddr poffs, uint64_t size, hwaddr moffs)
+{
+    memory_region_init_alias(mr, owner, name, parent, poffs, size);
+    memory_region_add_subregion(get_system_memory(), moffs, mr);
+    trace_mv64361_region_map(name, poffs, size, moffs);
+}
+
+static void setup_mem_windows(MV64361State *s, uint32_t val)
+{
+    MV64361PCIState *p;
+    MemoryRegion *mr;
+    uint32_t mask;
+    int i;
+
+    val &= 0x1fffff;


magic value for PCI_WINDOW_SIZE_MASK?

That constant does not exist. I could define it but don't see how is itbetter then literal value if it's only needed once.

+    for (mask = 1, i = 0; i < 21; i++, mask <<= 1) {


magic 21.

Same here, 21 is number of memory windows as is clear from the belowcomments and this is only used here so hiding it behind a constant thatyou'd have to scroll up to figure out the value of would not make itclearer in my opinion.

+        if ((val & mask) != (s->base_addr_enable & mask)) {
+            trace_mv64361_region_enable(!(val & mask) ? "enable" : "disable", 
i);
+            switch (i) {
+            /*
+             * 0-3 are SDRAM chip selects but we map all RAM directly
+             * 4-7 are device chip selects (not sure what those are)
+             * 8 is Boot device (ROM) chip select but we map that directly too
+             */
+            case 9:
+                p = &s->pci[0];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->io, OBJECT(s), "pci0-io-win",
+                                   p->remap[4], (p->io_size + 1) << 16,
+                                   (p->io_base & 0xfffff) << 16);
+                }
+                break;
+            case 10:
+                p = &s->pci[0];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem0-win",
+                                   p->remap[0], (p->mem_size[0] + 1) << 16,
+                                   (p->mem_base[0] & 0xfffff) << 16);
+                }
+                break;
+            case 11:
+                p = &s->pci[0];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem1-win",
+                                   p->remap[1], (p->mem_size[1] + 1) << 16,
+                                   (p->mem_base[1] & 0xfffff) << 16);
+                }
+                break;
+            case 12:
+                p = &s->pci[0];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem2-win",
+                                   p->remap[2], (p->mem_size[2] + 1) << 16,
+                                   (p->mem_base[2] & 0xfffff) << 16);
+                }
+                break;
+            case 13:
+                p = &s->pci[0];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci0-mem3-win",
+                                   p->remap[3], (p->mem_size[3] + 1) << 16,
+                                   (p->mem_base[3] & 0xfffff) << 16);
+                }
+                break;
+            case 14:
+                p = &s->pci[1];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->io, OBJECT(s), "pci1-io-win",
+                                   p->remap[4], (p->io_size + 1) << 16,
+                                   (p->io_base & 0xfffff) << 16);
+                }
+                break;
+            case 15:
+                p = &s->pci[1];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci1-mem0-win",
+                                   p->remap[0], (p->mem_size[0] + 1) << 16,
+                                   (p->mem_base[0] & 0xfffff) << 16);
+                }
+                break;
+            case 16:
+                p = &s->pci[1];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci1-mem1-win",
+                                   p->remap[1], (p->mem_size[1] + 1) << 16,
+                                   (p->mem_base[1] & 0xfffff) << 16);
+                }
+                break;
+            case 17:
+                p = &s->pci[1];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci1-mem2-win",
+                                   p->remap[2], (p->mem_size[2] + 1) << 16,
+                                   (p->mem_base[2] & 0xfffff) << 16);
+                }
+                break;
+            case 18:
+                p = &s->pci[1];
+                mr = &s->cpu_win[i];
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    map_pci_region(mr, &p->mem, OBJECT(s), "pci1-mem3-win",
+                                   p->remap[3], (p->mem_size[3] + 1) << 16,
+                                   (p->mem_base[3] & 0xfffff) << 16);
+                }
+                break;
+            /* 19 is integrated SRAM */
+            case 20:
+                mr = &s->regs;
+                unmap_region(mr);
+                if (!(val & mask)) {
+                    memory_region_add_subregion(get_system_memory(),
+                        (s->regs_base & 0xfffff) << 16, mr);
+                }
+                break;
+            }
+        }
+    }
+}
+
+static void mv64361_update_irq(void *opaque, int n, int level)
+{
+    MV64361State *s = opaque;
+    uint64_t val = s->main_int_cr;
+
+    if (level) {
+        val |= BIT_ULL(n);
+    } else {
+        val &= ~BIT_ULL(n);
+    }
+    if ((s->main_int_cr & s->cpu0_int_mask) != (val & s->cpu0_int_mask)) {
+        qemu_set_irq(s->cpu_irq, level);
+    }
+    s->main_int_cr = val;
+}
+
+static uint64_t mv64361_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    MV64361State *s = MV64361(opaque);
+    uint32_t ret = 0;
+
+    switch (addr) {
+    case MV64340_CPU_CONFIG:
+        ret = s->cpu_conf;
+        break;
+    case MV64340_PCI_0_IO_BASE_ADDR:
+        ret = s->pci[0].io_base;
+        break;
+    case MV64340_PCI_0_IO_SIZE:
+        ret = s->pci[0].io_size;
+        break;
+    case MV64340_PCI_0_IO_ADDR_REMAP:
+        ret = s->pci[0].remap[4] >> 16;
+        break;
+    case MV64340_PCI_0_MEMORY0_BASE_ADDR:
+        ret = s->pci[0].mem_base[0];
+        break;
+    case MV64340_PCI_0_MEMORY0_SIZE:
+        ret = s->pci[0].mem_size[0];
+        break;
+    case MV64340_PCI_0_MEMORY0_LOW_ADDR_REMAP:
+        ret = (s->pci[0].remap[0] & 0xffff0000) >> 16;


extract32()

I don't like that because it has an unneded assert for every invocation soI usually avoid it unless it would make code clearer. This is pretty clearalready this way, in fact I think clrearer than having to check what theparams of extract32 are. I might consider extract* if it was a macrowithout additional unnecessary checks.

+        break;
+    case MV64340_PCI_0_MEMORY0_HIGH_ADDR_REMAP:
+        ret = s->pci[0].remap[0] >> 32;
+        break;
+    case MV64340_PCI_0_MEMORY1_BASE_ADDR:
+        ret = s->pci[0].mem_base[1];
+        break;
+    case MV64340_PCI_0_MEMORY1_SIZE:
+        ret = s->pci[0].mem_size[1];
+        break;
+    case MV64340_PCI_0_MEMORY1_LOW_ADDR_REMAP:
+        ret = (s->pci[0].remap[1] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_0_MEMORY1_HIGH_ADDR_REMAP:
+        ret = s->pci[0].remap[1] >> 32;
+        break;
+    case MV64340_PCI_0_MEMORY2_BASE_ADDR:
+        ret = s->pci[0].mem_base[2];
+        break;
+    case MV64340_PCI_0_MEMORY2_SIZE:
+        ret = s->pci[0].mem_size[2];
+        break;
+    case MV64340_PCI_0_MEMORY2_LOW_ADDR_REMAP:
+        ret = (s->pci[0].remap[2] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_0_MEMORY2_HIGH_ADDR_REMAP:
+        ret = s->pci[0].remap[2] >> 32;
+        break;
+    case MV64340_PCI_0_MEMORY3_BASE_ADDR:
+        ret = s->pci[0].mem_base[3];
+        break;
+    case MV64340_PCI_0_MEMORY3_SIZE:
+        ret = s->pci[0].mem_size[3];
+        break;
+    case MV64340_PCI_0_MEMORY3_LOW_ADDR_REMAP:
+        ret = (s->pci[0].remap[3] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_0_MEMORY3_HIGH_ADDR_REMAP:
+        ret = s->pci[0].remap[3] >> 32;
+        break;
+    case MV64340_PCI_1_IO_BASE_ADDR:
+        ret = s->pci[1].io_base;
+        break;
+    case MV64340_PCI_1_IO_SIZE:
+        ret = s->pci[1].io_size;
+        break;
+    case MV64340_PCI_1_IO_ADDR_REMAP:
+        ret = s->pci[1].remap[4] >> 16;
+        break;
+    case MV64340_PCI_1_MEMORY0_BASE_ADDR:
+        ret = s->pci[1].mem_base[0];
+        break;
+    case MV64340_PCI_1_MEMORY0_SIZE:
+        ret = s->pci[1].mem_size[0];
+        break;
+    case MV64340_PCI_1_MEMORY0_LOW_ADDR_REMAP:
+        ret = (s->pci[1].remap[0] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_1_MEMORY0_HIGH_ADDR_REMAP:
+        ret = s->pci[1].remap[0] >> 32;
+        break;
+    case MV64340_PCI_1_MEMORY1_BASE_ADDR:
+        ret = s->pci[1].mem_base[1];
+        break;
+    case MV64340_PCI_1_MEMORY1_SIZE:
+        ret = s->pci[1].mem_size[1];
+        break;
+    case MV64340_PCI_1_MEMORY1_LOW_ADDR_REMAP:
+        ret = (s->pci[1].remap[1] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_1_MEMORY1_HIGH_ADDR_REMAP:
+        ret = s->pci[1].remap[1] >> 32;
+        break;
+    case MV64340_PCI_1_MEMORY2_BASE_ADDR:
+        ret = s->pci[1].mem_base[2];
+        break;
+    case MV64340_PCI_1_MEMORY2_SIZE:
+        ret = s->pci[1].mem_size[2];
+        break;
+    case MV64340_PCI_1_MEMORY2_LOW_ADDR_REMAP:
+        ret = (s->pci[1].remap[2] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_1_MEMORY2_HIGH_ADDR_REMAP:
+        ret = s->pci[1].remap[2] >> 32;
+        break;
+    case MV64340_PCI_1_MEMORY3_BASE_ADDR:
+        ret = s->pci[1].mem_base[3];
+        break;
+    case MV64340_PCI_1_MEMORY3_SIZE:
+        ret = s->pci[1].mem_size[3];
+        break;
+    case MV64340_PCI_1_MEMORY3_LOW_ADDR_REMAP:
+        ret = (s->pci[1].remap[3] & 0xffff0000) >> 16;
+        break;
+    case MV64340_PCI_1_MEMORY3_HIGH_ADDR_REMAP:
+        ret = s->pci[1].remap[3] >> 32;
+        break;
+    case MV64340_INTERNAL_SPACE_BASE_ADDR:
+        ret = s->regs_base;
+        break;
+    case MV64340_BASE_ADDR_ENABLE:
+        ret = s->base_addr_enable;
+        break;
+    case MV64340_PCI_0_CONFIG_ADDR:
+        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(&s->pci[0]), 0, size);
+        break;
+    case MV64340_PCI_0_CONFIG_DATA_VIRTUAL_REG ...
+         MV64340_PCI_0_CONFIG_DATA_VIRTUAL_REG + 3:
+        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(&s->pci[0]),
+                  addr - MV64340_PCI_0_CONFIG_DATA_VIRTUAL_REG, size);


Code smell... You probably want memory_region_dispatch_read().

Never heard of that, no idea how to use that and how would that would helphere. Any examples? But at this point I really don't want to change any ofthis as that could break it while this was tested to work.

+        break;
+    case MV64340_PCI_1_CONFIG_ADDR:
+        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(&s->pci[1]), 0, size);
+        break;
+    case MV64340_PCI_1_CONFIG_DATA_VIRTUAL_REG ...
+         MV64340_PCI_1_CONFIG_DATA_VIRTUAL_REG + 3:
+        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(&s->pci[1]),
+                  addr - MV64340_PCI_1_CONFIG_DATA_VIRTUAL_REG, size);
+        break;
+    case MV64340_PCI_1_INTERRUPT_ACKNOWLEDGE_VIRTUAL_REG:
+        /* FIXME: Should this be sent via the PCI bus somehow? */
+        if (s->gpp_int_level && (s->gpp_value & BIT(31))) {
+            ret = pic_read_irq(isa_pic);
+        }
+        break;
+    case MV64340_MAIN_INTERRUPT_CAUSE_LOW:
+        ret = s->main_int_cr;
+        break;
+    case MV64340_MAIN_INTERRUPT_CAUSE_HIGH:
+        ret = s->main_int_cr >> 32;
+        break;
+    case MV64340_CPU_INTERRUPT0_MASK_LOW:
+        ret = s->cpu0_int_mask;
+        break;
+    case MV64340_CPU_INTERRUPT0_MASK_HIGH:
+        ret = s->cpu0_int_mask >> 32;
+        break;
+    case MV64340_CPU_INTERRUPT0_SELECT_CAUSE:
+        ret = s->main_int_cr;
+        if (s->main_int_cr & s->cpu0_int_mask) {
+            if (!(s->main_int_cr & s->cpu0_int_mask & 0xffffffff)) {
+                ret = s->main_int_cr >> 32 | BIT(30);
+            } else if ((s->main_int_cr & s->cpu0_int_mask) >> 32) {
+                ret |= BIT(31);
+            }
+        }
+        break;
+    case MV64340_CUNIT_ARBITER_CONTROL_REG:
+        ret = 0x11ff0000 | (s->gpp_int_level << 10);
+        break;
+    case MV64340_GPP_IO_CONTROL:
+        ret = s->gpp_io;
+        break;
+    case MV64340_GPP_LEVEL_CONTROL:
+        ret = s->gpp_level;
+        break;
+    case MV64340_GPP_VALUE:
+        ret = s->gpp_value;
+        break;
+    case MV64340_GPP_VALUE_SET:
+    case MV64340_GPP_VALUE_CLEAR:
+        ret = 0;
+        break;
+    case MV64340_GPP_INTERRUPT_CAUSE:
+        ret = s->gpp_int_cr;
+        break;
+    case MV64340_GPP_INTERRUPT_MASK0:
+    case MV64340_GPP_INTERRUPT_MASK1:
+        ret = s->gpp_int_mask;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
+                      HWADDR_PRIx "\n", __func__, addr);
+        break;
+    }
+    if (addr != MV64340_PCI_1_INTERRUPT_ACKNOWLEDGE_VIRTUAL_REG) {
+        trace_mv64361_reg_read(addr, ret);
+    }
+    return ret;
+}
+
+static void warn_swap_bit(uint64_t val)
+{
+    if ((val & 0x3000000ULL) >> 24 != 1) {


Code smell. Shouldn't this be a MemoryRegion?

Why is this a code smell? It is a memory region but can you flip itsendianness during runtime or only when declaring it? Anyway, fortunatelynothing seems to need this, I've only put the warning here to see if itwould be needed but never have seen it firing. (The chip has an endiannessbit that can enable swapping of memory region accesses but we don'temulate that. This function checks that guest does not try to enable it.)


Regards,
BALATON Zoltan

+        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
+    }
+}
+
+static void mv64361_set_pci_mem_remap(MV64361State *s, int bus, int idx,
+                                      uint64_t val, bool high)
+{
+    if (high) {
+        s->pci[bus].remap[idx] = val;
+    } else {
+        s->pci[bus].remap[idx] &= 0xffffffff00000000ULL;
+        s->pci[bus].remap[idx] |= (val & 0xffffULL) << 16;
+    }
+}

[Prev in Thread]

Current Thread

[Next in Thread]

[PATCH v9 0/7] Pegasos2 emulation, BALATON Zoltan, 2021/03/16
- [PATCH v9 4/7] vt82c686: Add emulation of VT8231 south bridge, BALATON Zoltan, 2021/03/16
- [PATCH v9 5/7] hw/isa/Kconfig: Add missing dependency VIA VT82C686 -> APM, BALATON Zoltan, 2021/03/16
- [PATCH v9 2/7] vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO, BALATON Zoltan, 2021/03/16
- [PATCH v9 1/7] vt82c686: QOM-ify superio related functionality, BALATON Zoltan, 2021/03/16
  - Re: [PATCH v9 1/7] vt82c686: QOM-ify superio related functionality, Mark Cave-Ayland, 2021/03/16
- [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller, BALATON Zoltan, 2021/03/16
  - Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller, Philippe Mathieu-Daudé, 2021/03/16
    - Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller, BALATON Zoltan <=
- [PATCH v9 3/7] vt82c686: Introduce abstract TYPE_VIA_ISA and base vt82c686b_isa on it, BALATON Zoltan, 2021/03/16
- [PATCH v9 7/7] hw/ppc: Add emulation of Genesi/bPlan Pegasos II, BALATON Zoltan, 2021/03/16
- Re: [PATCH v9 0/7] Pegasos2 emulation, Mark Cave-Ayland, 2021/03/16
  - Re: [PATCH v9 0/7] Pegasos2 emulation, BALATON Zoltan, 2021/03/16

Prev by Date: Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller
Next by Date: [PATCH v10 2/7] vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO
Previous by thread: Re: [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller
Next by thread: [PATCH v9 3/7] vt82c686: Introduce abstract TYPE_VIA_ISA and base vt82c686b_isa on it
Index(es):
- Date
- Thread