Skip to contentSkip to conversation

prep: improve Raven PCI host emulation

PATCH Superseded Hervé Poussineau 3 participants 10 messages Aug 23, 2013 – Sep 2, 2013

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover prep: improve Raven PCI host emulation

This patchset improves Raven PCI host emulation, found in some PPC platforms, like the QEMU 'prep' one, and for example the IBM RS/6000 40p.

Some features added to raven emulation were already present in prep board (non contiguous I/O, firmware loading), while some other are new (PCI bus mastering memory region).

This patchset has been tested against Linux 2.4 PPC and IBM RS/6000 40p firmware.

Hervé Poussineau (3):
pci: remove explicit check to 64K ioport size
prep: kill get_system_io() usage
prep: improve Raven PCI host emulation

 hw/pci-host/prep.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/pci/pci.c       |    3 +-
 hw/ppc/prep.c      |  152 +++++++--------------------------------------
 3 files changed, 182 insertions(+), 146 deletions(-)
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/3 pci: remove explicit check to 64K ioport size

This check is useless, as bigger addresses will be ignored when added to 'io' MemoryRegion, which has a size of 64K.

However, on architectures which have memory-mapped I/O, PCI I/O BARs can be mapped to an I/O address which is bigger than 64K.

---
 hw/pci/pci.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
hw/pci/pci.c +1 -2
index 4c004f5..54cd43d 100644
@@ -1033,8 +1033,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
         }
         new_addr = pci_get_long(d->config + bar) & ~(size - 1);
         last_addr = new_addr + size - 1;
-        /* NOTE: we have only 64K ioports on PC */
-        if (last_addr <= new_addr || new_addr == 0 || last_addr > UINT16_MAX) {
+        if (last_addr <= new_addr || new_addr == 0) {
             return PCI_BAR_UNMAPPED;
         }
         return new_addr;
Signature · 1 line
1.7.10.4
#2 lore raw

PATCH 2/3 prep: kill get_system_io() usage

In reply to Hervé Poussineau

While ISA address space in prep machine is currently the one returned by get_system_io(), this depends of the implementation of i82378/raven devices, and this may not be the case forever.

Use the right ISA address space when adding some more ports to it. We can use whatever ISA device on the right ISA bus, as all ISA devices on the same ISA bus share the same ISA address space.

---
 hw/ppc/prep.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ppc/prep.c +1 -1
index 7e04b1a..efc892d 100644
@@ -656,7 +656,7 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     sysctrl->reset_irq = cpu->env.irq_inputs[PPC6xx_INPUT_HRESET];
 
     portio_list_init(port_list, NULL, prep_portio_list, sysctrl, "prep");
-    portio_list_add(port_list, get_system_io(), 0x0);
+    portio_list_add(port_list, isa_address_space_io(isa), 0x0);
 
     /* PowerPC control and status register group */
 #if 0
Signature · 1 line
1.7.10.4
#3 lore raw

PATCH 3/3 prep: improve Raven PCI host emulation

In reply to Hervé Poussineau

- let it load a firmware (raw or elf image)
- add a GPIO to let it handle the non-contiguous I/O address space
- provide a bus master address space

Missing part is dynamic endianness change, which is required for IBM AIX and MS Windows NT/PPC.

Also move isa_mem_base from PCI host to machine board. The right value for PReP-compliant boards is 0. However, Open Hack'Ware relies on it to be 0xc0000000 to be able to display something (wrong BAR programming). We'll be able to change it later, once firmware is replaced by something else.

Simplify prep board code by relying on Raven PCI host to handle non-contiguous I/O, and to load BIOS (with a small hack required for Open Hack'Ware).

---
 hw/pci-host/prep.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/ppc/prep.c      |  150 +++++++--------------------------------------
 2 files changed, 180 insertions(+), 143 deletions(-)
hw/pci-host/prep.c +159 -14
index e120058..6a44e14 100644
@@ -28,7 +28,9 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/i386/pc.h"
+#include "hw/loader.h"
 #include "exec/address-spaces.h"
+#include "elf.h"
 
 #define TYPE_RAVEN_PCI_DEVICE "raven"
 #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
@@ -38,6 +40,9 @@
 
 typedef struct RavenPCIState {
     PCIDevice dev;
+    uint32_t elf_machine;
+    char *bios_name;
+    MemoryRegion bios;
 } RavenPCIState;
 
 #define RAVEN_PCI_HOST_BRIDGE(obj) \
@@ -46,12 +51,25 @@ typedef struct RavenPCIState {
 typedef struct PRePPCIState {
     PCIHostState parent_obj;
 
-    MemoryRegion intack;
-    qemu_irq irq[4];
+    qemu_irq irq[PCI_NUM_PINS];
     PCIBus pci_bus;
+    AddressSpace pci_io_as;
+    MemoryRegion pci_io;
+    MemoryRegion pci_io_non_contiguous;
+    MemoryRegion pci_memory;
+    MemoryRegion pci_intack;
+    MemoryRegion bm;
+    MemoryRegion bm_regions[4];
+    MemoryRegion bm_ram_alias;
+    MemoryRegion bm_pci_memory_alias;
+    AddressSpace bm_as;
     RavenPCIState pci_dev;
+
+    int contiguous_map;
 } PREPPCIState;
 
+#define BIOS_SIZE (1024 * 1024)
+
 static inline uint32_t PPC_PCIIO_config(hwaddr addr)
 {
     int i;
@@ -99,6 +117,52 @@ static const MemoryRegionOps PPC_intack_ops = {
     },
 };
 
+static uint64_t prep_io_read(void *opaque, hwaddr addr,
+                             unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;
+}
+
+static void prep_io_write(void *opaque, hwaddr addr,
+                          uint64_t val, unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    memcpy(buf, &val, size);
+    address_space_write(&s->pci_io_as, addr + 0x80000000, buf, size);
+}
+
+static const MemoryRegionOps prep_io_ops = {
+    .read = prep_io_read,
+    .write = prep_io_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.max_access_size = 4,
+    .valid.unaligned = true,
+};
+
 static int prep_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     return (irq_num + (pci_dev->devfn >> 3)) & 1;
@@ -111,6 +175,19 @@ static void prep_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num] , level);
 }
 
+static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
+                                             int devfn)
+{
+    PREPPCIState *s = opaque;
+    return &s->bm_as;
+}
+
+static void raven_change_gpio(void *opaque, int n, int level)
+{
+    PREPPCIState *s = opaque;
+    s->contiguous_map = level;
+}
+
 static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 {
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
@@ -119,29 +196,28 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     MemoryRegion *address_space_mem = get_system_memory();
     int i;
 
-    isa_mem_base = 0xc0000000;
-
-    for (i = 0; i < 4; i++) {
+    for (i = 0; i < PCI_NUM_PINS; i++) {
         sysbus_init_irq(dev, &s->irq[i]);
     }
 
-    pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, 4);
+    qdev_init_gpio_in(d, raven_change_gpio, 1);
+
+    pci_bus_irqs(&s->pci_bus, prep_set_irq, prep_map_irq, s->irq, PCI_NUM_PINS);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, s,
                           "pci-conf-idx", 1);
-    sysbus_add_io(dev, 0xcf8, &h->conf_mem);
-    sysbus_init_ioports(&h->busdev, 0xcf8, 1);
+    memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
 
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_be_ops, s,
                           "pci-conf-data", 1);
-    sysbus_add_io(dev, 0xcfc, &h->data_mem);
-    sysbus_init_ioports(&h->busdev, 0xcfc, 1);
+    memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
 
     memory_region_init_io(&h->mmcfg, OBJECT(s), &PPC_PCIIO_ops, s, "pciio", 0x00400000);
     memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
 
-    memory_region_init_io(&s->intack, OBJECT(s), &PPC_intack_ops, s, "pci-intack", 1);
-    memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->intack);
+    memory_region_init_io(&s->pci_intack, OBJECT(s), &PPC_intack_ops, s,
+                          "pci-intack", 1);
+    memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
 
     /* TODO Remove once realize propagates to child devices. */
     object_property_set_bool(OBJECT(&s->pci_dev), true, "realized", errp);
@@ -152,11 +228,36 @@ static void raven_pcihost_initfn(Object *obj)
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
     MemoryRegion *address_space_mem = get_system_memory();
-    MemoryRegion *address_space_io = get_system_io();
     DeviceState *pci_dev;
 
+    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
+    memory_region_init_io(&s->pci_io_non_contiguous, obj, &prep_io_ops, s,
+                          "pci-io-non-contiguous", 0x00800000);
+    memory_region_init(&s->pci_memory, obj, "pci-memory",
+                       0x3f000000 + isa_mem_base);
+    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
+
+    /* CPU address space */
+    memory_region_add_subregion(address_space_mem, 0x80000000, &s->pci_io);
+    memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
+                                        &s->pci_io_non_contiguous, 1);
+    memory_region_add_subregion(address_space_mem, 0xc0000000 - isa_mem_base,
+                                &s->pci_memory);
     pci_bus_new_inplace(&s->pci_bus, DEVICE(obj), NULL,
-                        address_space_mem, address_space_io, 0, TYPE_PCI_BUS);
+                        &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
+
+    /* Bus master address space */
+    memory_region_init(&s->bm, obj, "bm-raven", UINT32_MAX);
+    memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
+                             &s->pci_memory, isa_mem_base,
+                             memory_region_size(&s->pci_memory) - isa_mem_base);
+    memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
+                             get_system_memory(), 0, 0x80000000);
+    memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
+    memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
+    address_space_init(&s->bm_as, &s->bm, "raven-bm");
+    pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
+
     h->bus = &s->pci_bus;
 
     object_initialize(&s->pci_dev, TYPE_RAVEN_PCI_DEVICE);
@@ -169,10 +270,46 @@ static void raven_pcihost_initfn(Object *obj)
 
 static int raven_init(PCIDevice *d)
 {
+    Object *obj = OBJECT(d);
+    RavenPCIState *s = RAVEN_PCI_DEVICE(d);
+    char *filename;
+    int bios_size = -1;
+
     d->config[0x0C] = 0x08; // cache_line_size
     d->config[0x0D] = 0x10; // latency_timer
     d->config[0x34] = 0x00; // capabilities_pointer
 
+    memory_region_init_ram(&s->bios, obj, "bios", BIOS_SIZE);
+    memory_region_set_readonly(&s->bios, true);
+    memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
+                                &s->bios);
+    vmstate_register_ram_global(&s->bios);
+    if (s->bios_name) {
+        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, s->bios_name);
+        if (filename) {
+            if (s->elf_machine != EM_NONE) {
+                bios_size = load_elf(filename, NULL, NULL, NULL,
+                                     NULL, NULL, 1, s->elf_machine, 0);
+            }
+            if (bios_size < 0) {
+                bios_size = get_image_size(filename);
+                if (bios_size > 0 && bios_size <= BIOS_SIZE) {
+                    hwaddr bios_addr;
+                    bios_size = (bios_size + 0xfff) & ~0xfff;
+                    bios_addr = (uint32_t)(-BIOS_SIZE);
+                    bios_size = load_image_targphys(filename, bios_addr,
+                                                    bios_size);
+                }
+            }
+        }
+        if (bios_size < 0 || bios_size > BIOS_SIZE) {
+            hw_error("qemu: could not load bios image '%s'\n", s->bios_name);
+        }
+        if (filename) {
+            g_free(filename);
+        }
+    }
+
     return 0;
 }
 
@@ -208,12 +345,20 @@ static const TypeInfo raven_info = {
     .class_init = raven_class_init,
 };
 
+static Property raven_pcihost_properties[] = {
+    DEFINE_PROP_UINT32("elf-machine", PREPPCIState, pci_dev.elf_machine,
+                       EM_NONE),
+    DEFINE_PROP_STRING("bios-name", PREPPCIState, pci_dev.bios_name),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void raven_pcihost_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->realize = raven_pcihost_realizefn;
+    dc->props = raven_pcihost_properties;
     dc->fw_name = "pci";
     dc->no_user = 1;
 }
hw/ppc/prep.c +21 -129

205 lines not on this page · raw message

Signature · 1 line
1.7.10.4
#4 lore raw
Quoted 4 lines · Hervé Poussineau
hw/pci-host/prep.c +4 -0
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 137

Quoted 10 lines
hw/pci-host/prep.c +10 -0
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;

This memcpy can't be right, especially for big-endian host.

r~

#5 lore raw
Quoted 17 lines · Richard Henderson
Quoted 4 lines · Hervé Poussineau
hw/pci-host/prep.c +4 -0
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 137

Quoted 10 lines
hw/pci-host/prep.c +10 -0
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;

This memcpy can't be right, especially for big-endian host.

pci_io_as is supposed to contain words/longs in little-endian mode (PCI is always little-endian). prep_io_ops endianness is DEVICE_LITTLE_ENDIAN, so, AFAIK, this means that returned val should be LE. Why should it break on big-endian hosts?

Regards,

Hervé

#6 lore raw
Quoted 13 lines · Hervé Poussineau
Quoted 11 lines · Richard Henderson
Quoted 10 lines · Hervé Poussineau
hw/pci-host/prep.c +9 -0
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
Quoted 8 lines
Quoted 8 lines

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 137

Quoted 5 lines
hw/pci-host/prep.c +5 -0
+    }
+
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;

This memcpy can't be right, especially for big-endian host.

The most immediately obvious problem is that it's not initialized and you're only copying size bytes of data into an 8 byte wide type. Worse, as Richard suggests, on big-endian CPUs you're writing into the upper half of the variable, so when the caller truncates it to 32 bits for a guest CPU word access it will throw away the data completely.

Quoted 5 lines

pci_io_as is supposed to contain words/longs in little-endian mode (PCI is always little-endian).
prep_io_ops endianness is DEVICE_LITTLE_ENDIAN, so, AFAIK, this means that returned val should be LE.
Why should it break on big-endian hosts?

This isn't how the DEVICE_*_ENDIAN work. Memory region read/write callbacks always return their values in host-CPU byte order (ie "return 0x87654321;" returns a value whose least significant byte is 0x21). The DEVICE_*_ENDIAN fields are then used to correctly convert that host-endian value into whatever the CPU needs to see (ie if your CPU is TARGET_WORDS_BIGENDIAN and the device is DEVICE_LITTLE_ENDIAN the memory codepath
will insert one bswap operation of the appropriate size).

-- PMM

#7 lore raw
In reply to Hervé Poussineau
Quoted 9 lines · Hervé Poussineau

- let it load a firmware (raw or elf image)
- add a GPIO to let it handle the non-contiguous I/O address space
- provide a bus master address space

Also move isa_mem_base from PCI host to machine board.

Simplify prep board code by relying on Raven PCI host to handle non-contiguous I/O, and to load BIOS (with a small hack required for Open Hack'Ware).

That seems like quite a lot to be doing in a single patch. Does any of it split out for easier code review?

Quoted 9 lines
hw/pci-host/prep.c +9 -0
+static uint64_t prep_io_read(void *opaque, hwaddr addr,
+                             unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 138

Quoted 10 lines
hw/pci-host/prep.c +10 -0
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;
+}

Since this is just forwarding accesses to another address space, I'm fairly sure you could do it with a suitable collection of alias and container memory regions.

Quoted 8 lines
hw/pci-host/prep.c +8 -0
+
+static void prep_io_write(void *opaque, hwaddr addr,
+                          uint64_t val, unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+
+    if (s->contiguous_map == 0) {

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 156

Quoted 10 lines
hw/pci-host/prep.c +10 -0
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
+    memcpy(buf, &val, size);
+    address_space_write(&s->pci_io_as, addr + 0x80000000, buf, size);
+}

...if you don't do it that way, please at least factor out the address conversion code from the read and write routines.

Quoted 13 lines
hw/pci-host/prep.c +9 -0
+
+static const MemoryRegionOps prep_io_ops = {
+    .read = prep_io_read,
+    .write = prep_io_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.max_access_size = 4,
+    .valid.unaligned = true,
+};
+
 static int prep_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     return (irq_num + (pci_dev->devfn >> 3)) & 1;

hw/pci-host/prep.c static void prep_set_irq(void *opaque, int irq_num, int level) · line 183

Quoted 9 lines
hw/pci-host/prep.c +6 -0
@@ -111,6 +175,19 @@ static void prep_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num] , level);
 }
 
+static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
+                                             int devfn)
+{
+    PREPPCIState *s = opaque;
+    return &s->bm_as;
+}

My versatilepb PCI DMA patches have a very similar set_iommu callback. I wonder if we're going to end up with one PCI host controller that actually uses the IOMMU facilities and a lot which use it as a rather boilerplate-heavy way to pass an AddressSpace to the core PCI code...

-- PMM

#8 lore raw
Quoted 6 lines · Peter Maydell
Quoted 3 lines · Hervé Poussineau

- let it load a firmware (raw or elf image)
- add a GPIO to let it handle the non-contiguous I/O address space
- provide a bus master address space

Quoted 8 lines
Quoted 5 lines

Also move isa_mem_base from PCI host to machine board.

Simplify prep board code by relying on Raven PCI host to handle non-contiguous I/O, and to load BIOS (with a small hack required for Open Hack'Ware).

That seems like quite a lot to be doing in a single patch. Does any of it split out for easier code review?

Yes, v2 is in preparation. This patch will be split in multiple patches.

Quoted 16 lines
Quoted 16 lines
hw/pci-host/prep.c +15 -0
+static uint64_t prep_io_read(void *opaque, hwaddr addr,
+                             unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+    uint64_t val;
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
+    }
+
Quoted 8 lines

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 138

Quoted 4 lines
hw/pci-host/prep.c +4 -0
+    address_space_read(&s->pci_io_as, addr + 0x80000000, buf, size);
+    memcpy(&val, buf, size);
+    return val;
+}

Since this is just forwarding accesses to another address space, I'm fairly sure you could do it with a suitable collection of alias and container memory regions.

Yes, aliases should probably work, but it won't be handy to create lots of them. Moreover, this function needs to be expanded later to handle an additional endianness switch, which will change both addresses and values...

Quoted 14 lines
Quoted 14 lines
hw/pci-host/prep.c +13 -0
+
+static void prep_io_write(void *opaque, hwaddr addr,
+                          uint64_t val, unsigned int size)
+{
+    PREPPCIState *s = opaque;
+    uint8_t buf[4];
+
+    if (s->contiguous_map == 0) {
+        /* 64 KB contiguous space for IOs */
+        addr &= 0xFFFF;
+    } else {
+        /* 8 MB non-contiguous space for IOs */
+        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
Quoted 8 lines

hw/pci-host/prep.c static const MemoryRegionOps PPC_intack_ops = { · line 156

Quoted 5 lines
hw/pci-host/prep.c +5 -0
+    }
+
+    memcpy(buf, &val, size);
+    address_space_write(&s->pci_io_as, addr + 0x80000000, buf, size);
+}

...if you don't do it that way, please at least factor out the address conversion code from the read and write routines.

... So, yes, I'll put conversion code in a common routine.

Hervé

#9 lore raw
Quoted 3 lines · Hervé Poussineau

Yes, aliases should probably work, but it won't be handy to create lots of them. Moreover, this function needs to be expanded later to handle an additional endianness switch, which will change both addresses and values...

FWIW, I think the right way to handle an endianness switch like that is that we should have a 'byteswap' property on MemoryRegion containers, so that the whole thing collapses down to 'swap y/n' when we flatten regions. However we don't have that at the moment, so I guess we're stuck with these functions.

-- PMM