[Qemu-devel] [PATCH 0/3] prep: improve Raven PCI host emulation

[Qemu-devel] [PATCH 0/3] prep: improve Raven PCI host emulation

[Hervé Poussineau]

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(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] pci: remove explicit check to 64K ioport size

[Hervé Poussineau]

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.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/pci/pci.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c004f5..54cd43d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -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;
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] prep: kill get_system_io() usage

[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.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/ppc/prep.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 7e04b1a..efc892d 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -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
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[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).

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/pci-host/prep.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/ppc/prep.c      |  150 +++++++--------------------------------------
 2 files changed, 180 insertions(+), 143 deletions(-)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index e120058..6a44e14 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -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;
 }
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index efc892d..64ab9f8 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -185,6 +185,7 @@ typedef struct sysctrl_t {
     uint8_t state;
     uint8_t syscontrol;
     int contiguous_map;
+    qemu_irq contiguous_map_irq;
     int endian;
 } sysctrl_t;
 
@@ -253,6 +254,7 @@ static void PREP_io_800_writeb (void *opaque, uint32_t addr, uint32_t val)
     case 0x0850:
         /* I/O map type register */
         sysctrl->contiguous_map = val & 0x01;
+        qemu_set_irq(sysctrl->contiguous_map_irq, sysctrl->contiguous_map);
         break;
     default:
         printf("ERROR: unaffected IO port write: %04" PRIx32
@@ -327,92 +329,6 @@ static uint32_t PREP_io_800_readb (void *opaque, uint32_t addr)
     return retval;
 }
 
-static inline hwaddr prep_IO_address(sysctrl_t *sysctrl,
-                                                 hwaddr addr)
-{
-    if (sysctrl->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);
-    }
-
-    return addr;
-}
-
-static void PPC_prep_io_writeb (void *opaque, hwaddr addr,
-                                uint32_t value)
-{
-    sysctrl_t *sysctrl = opaque;
-
-    addr = prep_IO_address(sysctrl, addr);
-    cpu_outb(addr, value);
-}
-
-static uint32_t PPC_prep_io_readb (void *opaque, hwaddr addr)
-{
-    sysctrl_t *sysctrl = opaque;
-    uint32_t ret;
-
-    addr = prep_IO_address(sysctrl, addr);
-    ret = cpu_inb(addr);
-
-    return ret;
-}
-
-static void PPC_prep_io_writew (void *opaque, hwaddr addr,
-                                uint32_t value)
-{
-    sysctrl_t *sysctrl = opaque;
-
-    addr = prep_IO_address(sysctrl, addr);
-    PPC_IO_DPRINTF("0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", addr, value);
-    cpu_outw(addr, value);
-}
-
-static uint32_t PPC_prep_io_readw (void *opaque, hwaddr addr)
-{
-    sysctrl_t *sysctrl = opaque;
-    uint32_t ret;
-
-    addr = prep_IO_address(sysctrl, addr);
-    ret = cpu_inw(addr);
-    PPC_IO_DPRINTF("0x" TARGET_FMT_plx " <= 0x%08" PRIx32 "\n", addr, ret);
-
-    return ret;
-}
-
-static void PPC_prep_io_writel (void *opaque, hwaddr addr,
-                                uint32_t value)
-{
-    sysctrl_t *sysctrl = opaque;
-
-    addr = prep_IO_address(sysctrl, addr);
-    PPC_IO_DPRINTF("0x" TARGET_FMT_plx " => 0x%08" PRIx32 "\n", addr, value);
-    cpu_outl(addr, value);
-}
-
-static uint32_t PPC_prep_io_readl (void *opaque, hwaddr addr)
-{
-    sysctrl_t *sysctrl = opaque;
-    uint32_t ret;
-
-    addr = prep_IO_address(sysctrl, addr);
-    ret = cpu_inl(addr);
-    PPC_IO_DPRINTF("0x" TARGET_FMT_plx " <= 0x%08" PRIx32 "\n", addr, ret);
-
-    return ret;
-}
-
-static const MemoryRegionOps PPC_prep_io_ops = {
-    .old_mmio = {
-        .read = { PPC_prep_io_readb, PPC_prep_io_readw, PPC_prep_io_readl },
-        .write = { PPC_prep_io_writeb, PPC_prep_io_writew, PPC_prep_io_writel },
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 #define NVRAM_SIZE        0x2000
 
 static void cpu_request_exit(void *opaque, int irq, int level)
@@ -456,15 +372,13 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     PowerPCCPU *cpu = NULL;
     CPUPPCState *env = NULL;
-    char *filename;
     nvram_t nvram;
     M48t59State *m48t59;
-    MemoryRegion *PPC_io_memory = g_new(MemoryRegion, 1);
     PortioList *port_list = g_new(PortioList, 1);
 #if 0
     MemoryRegion *xcsr = g_new(MemoryRegion, 1);
 #endif
-    int linux_boot, i, nb_nics1, bios_size;
+    int linux_boot, i, nb_nics1;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     uint32_t kernel_base, initrd_base;
@@ -510,41 +424,12 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     memory_region_add_subregion(sysmem, 0, ram);
 
     /* allocate and load BIOS */
-    memory_region_init_ram(bios, NULL, "ppc_prep.bios", BIOS_SIZE);
-    memory_region_set_readonly(bios, true);
-    memory_region_add_subregion(sysmem, (uint32_t)(-BIOS_SIZE), bios);
-    vmstate_register_ram_global(bios);
-    if (bios_name == NULL)
-        bios_name = BIOS_FILENAME;
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-    if (filename) {
-        bios_size = load_elf(filename, NULL, NULL, NULL,
-                             NULL, NULL, 1, 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 > BIOS_SIZE) {
-                fprintf(stderr, "qemu: PReP bios '%s' is too large (0x%x)\n",
-                        bios_name, bios_size);
-                exit(1);
-            }
-        }
-    } else {
-        bios_size = -1;
-    }
-    if (bios_size < 0 && !qtest_enabled()) {
-        fprintf(stderr, "qemu: could not load PPC PReP bios '%s'\n",
-                bios_name);
-        exit(1);
-    }
-    if (filename) {
-        g_free(filename);
-    }
+    /* Open Hack'Ware hack: bios size is 512K and is loaded at 0xfff00000.
+     * However, reset address is 0xfffffffc. Mirror the bios from
+     * 0xfff00000 to 0xfff80000. */
+    memory_region_init_alias(bios, NULL, "bios-alias", sysmem, 0xfff00000,
+                             0x00080000);
+    memory_region_add_subregion_overlap(sysmem, 0xfff80000, bios, 1);
 
     if (linux_boot) {
         kernel_base = KERNEL_LOAD_ADDR;
@@ -592,7 +477,18 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
         hw_error("Only 6xx bus is supported on PREP machine\n");
     }
 
+    /* Open Hack'Ware hack: on PReP compliant-machines, this should be 0.
+     * However, setting it to 0 fixes PCI bus master operations, but breaks
+     * Open Hack'Ware display.
+     */
+    isa_mem_base = 0xc0000000;
+
     dev = qdev_create(NULL, "raven-pcihost");
+    if (bios_name == NULL) {
+        bios_name = BIOS_FILENAME;
+    }
+    qdev_prop_set_string(dev, "bios-name", bios_name);
+    qdev_prop_set_uint32(dev, "elf-machine", ELF_MACHINE);
     pcihost = PCI_HOST_BRIDGE(dev);
     object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev), NULL);
     qdev_init_nofail(dev);
@@ -601,6 +497,7 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
         fprintf(stderr, "Couldn't create PCI host controller.\n");
         exit(1);
     }
+    sysctrl->contiguous_map_irq = qdev_get_gpio_in(dev, 0);
 
     /* PCI -> ISA bridge */
     pci = pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "i82378");
@@ -621,11 +518,6 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     qdev_prop_set_uint8(dev, "config", 13); /* fdc, ser0, ser1, par0 */
     qdev_init_nofail(dev);
 
-    /* Register 8 MB of ISA IO space (needed for non-contiguous map) */
-    memory_region_init_io(PPC_io_memory, NULL, &PPC_prep_io_ops, sysctrl,
-                          "ppc-io", 0x00800000);
-    memory_region_add_subregion(sysmem, 0x80000000, PPC_io_memory);
-
     /* init basic PC hardware */
     pci_vga_init(pci_bus);
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Richard Henderson]

On 08/23/2013 11:52 AM, Hervé Poussineau wrote:
> +    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;

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


r~

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Hervé Poussineau]

Richard Henderson a écrit :
> On 08/23/2013 11:52 AM, Hervé Poussineau wrote:
>> +    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;
> 
> 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é

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Peter Maydell]

On 2 September 2013 21:26, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Richard Henderson a écrit :
>
>> On 08/23/2013 11:52 AM, Hervé Poussineau wrote:
>>>
>>> +    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;
>>
>>
>> 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.

> 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

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Peter Maydell]

On 23 August 2013 19:52, Hervé Poussineau <hpoussin@reactos.org> wrote:

> - 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?

> +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;
> +}

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.

> +
> +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);
> +}

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

> +
> +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;
> +}

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

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Hervé Poussineau]

Peter Maydell a écrit :
> On 23 August 2013 19:52, Hervé Poussineau <hpoussin@reactos.org> wrote:
> 
>> - 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?

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

> 
>> +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;
>> +}
> 
> 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...

> 
>> +
>> +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);
>> +}
> 
> ...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é

Re: [Qemu-devel] [PATCH 3/3] prep: improve Raven PCI host emulation

[Peter Maydell]

On 2 September 2013 22:18, Hervé Poussineau <hpoussin@reactos.org> wrote:
> 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