From: BALATON Zoltan <balaton@eik.bme.hu>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: Peter Maydell <peter.maydell@linaro.org>,
David Gibson <david@gibson.dropbear.id.au>,
qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
Paolo Bonzini <pbonzini@redhat.com>
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) [thread overview]
Message-ID: <alpine.LMD.2.03.2103170202420.5187@eik.bme.hu> (raw)
In-Reply-To: <ae1821e4-efae-d97a-ca55-079bb0399fe8@amsat.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 18212 bytes --]
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 it
better 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 below
comments and this is only used here so hiding it behind a constant that
you'd have to scroll up to figure out the value of would not make it
clearer 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 so
I usually avoid it unless it would make code clearer. This is pretty clear
already this way, in fact I think clrearer than having to check what the
params of extract32 are. I might consider extract* if it was a macro
without 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 help
here. Any examples? But at this point I really don't want to change any of
this 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 its
endianness during runtime or only when declaring it? Anyway, fortunately
nothing seems to need this, I've only put the warning here to see if it
would be needed but never have seen it firing. (The chip has an endianness
bit that can enable swapping of memory region accesses but we don't
emulate 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;
>> + }
>> +}
>
>
next prev parent reply other threads:[~2021-03-17 1:27 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 22:03 [PATCH v9 0/7] Pegasos2 emulation BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 2/7] vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 4/7] vt82c686: Add emulation of VT8231 south bridge BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 3/7] vt82c686: Introduce abstract TYPE_VIA_ISA and base vt82c686b_isa on it BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 6/7] hw/pci-host: Add emulation of Marvell MV64361 PPC system controller BALATON Zoltan
2021-03-17 0:55 ` Philippe Mathieu-Daudé
2021-03-17 1:18 ` BALATON Zoltan [this message]
2021-03-16 22:03 ` [PATCH v9 5/7] hw/isa/Kconfig: Add missing dependency VIA VT82C686 -> APM BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 7/7] hw/ppc: Add emulation of Genesi/bPlan Pegasos II BALATON Zoltan
2021-03-16 22:03 ` [PATCH v9 1/7] vt82c686: QOM-ify superio related functionality BALATON Zoltan
2021-03-17 0:04 ` Mark Cave-Ayland
2021-03-17 0:26 ` [PATCH v9 0/7] Pegasos2 emulation Mark Cave-Ayland
2021-03-17 1:36 ` BALATON Zoltan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LMD.2.03.2103170202420.5187@eik.bme.hu \
--to=balaton@eik.bme.hu \
--cc=david@gibson.dropbear.id.au \
--cc=f4bug@amsat.org \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).