[Qemu-devel] [PATCH v3 00/14] Refactor portio dispatching

[Qemu-devel] [PATCH v3 00/14] Refactor portio dispatching

[Jan Kiszka]

Changes in v3:
 - decouple vmport from portio types
 - removed portio traces from memory.h, consolidating it in ioport.h

CC: Jan Kiszka <jan.kiszka@siemens.com>
CC: malc <av1474@comtv.ru>

Jan Kiszka (14):
  adlib: replace register_ioport*
  applesmc: replace register_ioport*
  wdt_ib700: replace register_ioport*
  i82374: replace register_ioport*
  prep: replace register_ioport*
  vt82c686: replace register_ioport*
  Privatize register_ioport_read/write
  isa: implement isa_is_ioport_assigned via memory_region_find
  vmware-vga: Accept unaligned I/O accesses
  xen: Mark fixed platform I/O as unaligned
  ioport: Switch dispatching to memory core layer
  ioport: Remove unused old dispatching services
  vmport: Disentangle read handler type from portio
  ioport: Move portio types to ioport.h

 exec.c                         |   27 ---
 hw/acpi/piix4.c                |    9 +-
 hw/audio/adlib.c               |   20 ++-
 hw/display/vmware_vga.c        |    4 +
 hw/dma/i82374.c                |   18 ++-
 hw/isa/lpc_ich9.c              |    9 +-
 hw/isa/vt82c686.c              |   40 +++--
 hw/misc/applesmc.c             |   50 ++++--
 hw/misc/vmport.c               |    4 +-
 hw/ppc/prep.c                  |   23 ++-
 hw/watchdog/wdt_ib700.c        |   12 +-
 hw/xen/xen_platform.c          |    4 +
 include/exec/ioport.h          |   26 +--
 include/exec/iorange.h         |   31 ----
 include/exec/memory-internal.h |    2 -
 include/exec/memory.h          |   25 ---
 include/hw/i386/pc.h           |    6 +-
 ioport.c                       |  380 +++++++++++-----------------------------
 memory.c                       |   88 ---------
 19 files changed, 247 insertions(+), 531 deletions(-)
 delete mode 100644 include/exec/iorange.h

-- 
1.7.3.4

[Qemu-devel] [PATCH v3 01/14] adlib: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

CC: malc <av1474@comtv.ru>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/audio/adlib.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 6a7d377..8b9b81e 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -283,9 +283,17 @@ static void Adlib_fini (AdlibState *s)
     AUD_remove_card (&s->card);
 }
 
+static MemoryRegionPortio adlib_portio_list[] = {
+    { 0x388, 4, 1, .read = adlib_read, .write = adlib_write, },
+    { 0, 4, 1, .read = adlib_read, .write = adlib_write, },
+    { 0, 2, 1, .read = adlib_read, .write = adlib_write, },
+    PORTIO_END_OF_LIST(),
+};
+
 static void adlib_realizefn (DeviceState *dev, Error **errp)
 {
     AdlibState *s = ADLIB(dev);
+    PortioList *port_list = g_new(PortioList, 1);
     struct audsettings as;
 
     if (glob_adlib) {
@@ -339,14 +347,10 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
     s->samples = AUD_get_buffer_size_out (s->voice) >> SHIFT;
     s->mixbuf = g_malloc0 (s->samples << SHIFT);
 
-    register_ioport_read (0x388, 4, 1, adlib_read, s);
-    register_ioport_write (0x388, 4, 1, adlib_write, s);
-
-    register_ioport_read (s->port, 4, 1, adlib_read, s);
-    register_ioport_write (s->port, 4, 1, adlib_write, s);
-
-    register_ioport_read (s->port + 8, 2, 1, adlib_read, s);
-    register_ioport_write (s->port + 8, 2, 1, adlib_write, s);
+    adlib_portio_list[1].offset = s->port;
+    adlib_portio_list[2].offset = s->port + 8;
+    portio_list_init (port_list, adlib_portio_list, s, "adlib");
+    portio_list_add (port_list, isa_address_space_io(&s->parent_obj), 0);
 }
 
 static Property adlib_properties[] = {
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 02/14] applesmc: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/misc/applesmc.c |   50 ++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 46f4fbd..83468dc 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -73,6 +73,8 @@ typedef struct AppleSMCState AppleSMCState;
 struct AppleSMCState {
     ISADevice parent_obj;
 
+    MemoryRegion io_data;
+    MemoryRegion io_cmd;
     uint32_t iobase;
     uint8_t cmd;
     uint8_t status;
@@ -86,7 +88,8 @@ struct AppleSMCState {
     QLIST_HEAD(, AppleSMCData) data_def;
 };
 
-static void applesmc_io_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void applesmc_io_cmd_write(void *opaque, hwaddr addr, uint64_t val,
+                                  unsigned size)
 {
     AppleSMCState *s = opaque;
 
@@ -115,7 +118,8 @@ static void applesmc_fill_data(AppleSMCState *s)
     }
 }
 
-static void applesmc_io_data_writeb(void *opaque, uint32_t addr, uint32_t val)
+static void applesmc_io_data_write(void *opaque, hwaddr addr, uint64_t val,
+                                   unsigned size)
 {
     AppleSMCState *s = opaque;
 
@@ -138,7 +142,8 @@ static void applesmc_io_data_writeb(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
-static uint32_t applesmc_io_data_readb(void *opaque, uint32_t addr1)
+static uint64_t applesmc_io_data_read(void *opaque, hwaddr addr1,
+                                      unsigned size)
 {
     AppleSMCState *s = opaque;
     uint8_t retval = 0;
@@ -162,7 +167,7 @@ static uint32_t applesmc_io_data_readb(void *opaque, uint32_t addr1)
     return retval;
 }
 
-static uint32_t applesmc_io_cmd_readb(void *opaque, uint32_t addr1)
+static uint64_t applesmc_io_cmd_read(void *opaque, hwaddr addr1, unsigned size)
 {
     AppleSMCState *s = opaque;
 
@@ -201,18 +206,39 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)
     applesmc_add_key(s, "MSSD", 1, "\0x3");
 }
 
+static const MemoryRegionOps applesmc_data_io_ops = {
+    .write = applesmc_io_data_write,
+    .read = applesmc_io_data_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static const MemoryRegionOps applesmc_cmd_io_ops = {
+    .write = applesmc_io_cmd_write,
+    .read = applesmc_io_cmd_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void applesmc_isa_realize(DeviceState *dev, Error **errp)
 {
     AppleSMCState *s = APPLE_SMC(dev);
 
-    register_ioport_read(s->iobase + APPLESMC_DATA_PORT, 4, 1,
-                         applesmc_io_data_readb, s);
-    register_ioport_read(s->iobase + APPLESMC_CMD_PORT, 4, 1,
-                         applesmc_io_cmd_readb, s);
-    register_ioport_write(s->iobase + APPLESMC_DATA_PORT, 4, 1,
-                          applesmc_io_data_writeb, s);
-    register_ioport_write(s->iobase + APPLESMC_CMD_PORT, 4, 1,
-                          applesmc_io_cmd_writeb, s);
+    memory_region_init_io(&s->io_data, &applesmc_data_io_ops, s,
+                          "applesmc-data", 4);
+    isa_register_ioport(&s->parent_obj, &s->io_data,
+                        s->iobase + APPLESMC_DATA_PORT);
+
+    memory_region_init_io(&s->io_cmd, &applesmc_cmd_io_ops, s,
+                          "applesmc-cmd", 4);
+    isa_register_ioport(&s->parent_obj, &s->io_cmd,
+                        s->iobase + APPLESMC_CMD_PORT);
 
     if (!s->osk || (strlen(s->osk) != 64)) {
         fprintf(stderr, "WARNING: Using AppleSMC with invalid key\n");
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 03/14] wdt_ib700: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/watchdog/wdt_ib700.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index d85c894..597a923 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -97,15 +97,23 @@ static const VMStateDescription vmstate_ib700 = {
     }
 };
 
+static const MemoryRegionPortio wdt_portio_list[] = {
+    { 0x441, 2, 1, .write = ib700_write_disable_reg, },
+    { 0x443, 2, 1, .write = ib700_write_enable_reg, },
+    PORTIO_END_OF_LIST(),
+};
+
 static void wdt_ib700_realize(DeviceState *dev, Error **errp)
 {
     IB700State *s = IB700(dev);
+    PortioList *port_list = g_new(PortioList, 1);
 
     ib700_debug("watchdog init\n");
 
     s->timer = qemu_new_timer_ns(vm_clock, ib700_timer_expired, s);
-    register_ioport_write(0x441, 2, 1, ib700_write_disable_reg, s);
-    register_ioport_write(0x443, 2, 1, ib700_write_enable_reg, s);
+
+    portio_list_init(port_list, wdt_portio_list, s, "ib700");
+    portio_list_add(port_list, isa_address_space_io(&s->parent_obj), 0);
 }
 
 static void wdt_ib700_reset(DeviceState *dev)
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 04/14] i82374: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/dma/i82374.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 6192780..ecda5cb 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -124,16 +124,24 @@ static const VMStateDescription vmstate_isa_i82374 = {
     },
 };
 
+static const MemoryRegionPortio i82374_portio_list[] = {
+    { 0x0A, 1, 1, .read = i82374_read_isr, },
+    { 0x10, 8, 1, .write = i82374_write_command, },
+    { 0x18, 8, 1, .read = i82374_read_status, },
+    { 0x20, 0x20, 1,
+      .write = i82374_write_descriptor, .read = i82374_read_descriptor, },
+    PORTIO_END_OF_LIST(),
+};
+
 static void i82374_isa_realize(DeviceState *dev, Error **errp)
 {
     ISAi82374State *isa = I82374(dev);
     I82374State *s = &isa->state;
+    PortioList *port_list = g_new(PortioList, 1);
 
-    register_ioport_read(isa->iobase + 0x0A, 1, 1, i82374_read_isr, s);
-    register_ioport_write(isa->iobase + 0x10, 8, 1, i82374_write_command, s);
-    register_ioport_read(isa->iobase + 0x18, 8, 1, i82374_read_status, s);
-    register_ioport_write(isa->iobase + 0x20, 0x20, 1, i82374_write_descriptor, s);
-    register_ioport_read(isa->iobase + 0x20, 0x20, 1, i82374_read_descriptor, s);
+    portio_list_init(port_list, i82374_portio_list, s, "i82374");
+    portio_list_add(port_list, isa_address_space_io(&isa->parent_obj),
+                    isa->iobase);
 
     i82374_realize(s, errp);
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 05/14] prep: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ppc/prep.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 4fdc164..e7689ad 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -434,6 +434,16 @@ static void ppc_prep_reset(void *opaque)
     cpu->env.nip = 0xfffffffc;
 }
 
+static const MemoryRegionPortio prep_portio_list[] = {
+    /* System control ports */
+    { 0x0092, 1, 1, .read = PREP_io_800_readb, .write = PREP_io_800_writeb, },
+    { 0x0800, 0x52, 1,
+      .read = PREP_io_800_readb, .write = PREP_io_800_writeb, },
+    /* Special port to get debug messages from Open-Firmware */
+    { 0x0F00, 4, 1, .write = PPC_debug_write, },
+    PORTIO_END_OF_LIST(),
+};
+
 /* PowerPC PREP hardware initialisation */
 static void ppc_prep_init(QEMUMachineInitArgs *args)
 {
@@ -450,6 +460,7 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     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
@@ -641,11 +652,10 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
     isa_create_simple(isa_bus, "i8042");
 
     sysctrl->reset_irq = first_cpu->irq_inputs[PPC6xx_INPUT_HRESET];
-    /* System control ports */
-    register_ioport_read(0x0092, 0x01, 1, &PREP_io_800_readb, sysctrl);
-    register_ioport_write(0x0092, 0x01, 1, &PREP_io_800_writeb, sysctrl);
-    register_ioport_read(0x0800, 0x52, 1, &PREP_io_800_readb, sysctrl);
-    register_ioport_write(0x0800, 0x52, 1, &PREP_io_800_writeb, sysctrl);
+
+    portio_list_init(port_list, prep_portio_list, sysctrl, "prep");
+    portio_list_add(port_list, get_system_io(), 0x0);
+
     /* PowerPC control and status register group */
 #if 0
     memory_region_init_io(xcsr, &PPC_XCSR_ops, NULL, "ppc-xcsr", 0x1000);
@@ -672,9 +682,6 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)
                          /* XXX: need an option to load a NVRAM image */
                          0,
                          graphic_width, graphic_height, graphic_depth);
-
-    /* Special port to get debug messages from Open-Firmware */
-    register_ioport_write(0x0F00, 4, 1, &PPC_debug_write, NULL);
 }
 
 static QEMUMachine prep_machine = {
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 06/14] vt82c686: replace register_ioport*

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Convert over to memory regions to obsolete register_ioport*.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/isa/vt82c686.c |   40 ++++++++++++++++++++++++++--------------
 1 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 391d90d..e5cd4cd 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -43,10 +43,12 @@ typedef struct SuperIOConfig
 
 typedef struct VT82C686BState {
     PCIDevice dev;
+    MemoryRegion superio;
     SuperIOConfig superio_conf;
 } VT82C686BState;
 
-static void superio_ioport_writeb(void *opaque, uint32_t addr, uint32_t data)
+static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
+                                  unsigned size)
 {
     int can_write;
     SuperIOConfig *superio_conf = opaque;
@@ -93,7 +95,7 @@ static void superio_ioport_writeb(void *opaque, uint32_t addr, uint32_t data)
     }
 }
 
-static uint32_t superio_ioport_readb(void *opaque, uint32_t addr)
+static uint64_t superio_ioport_readb(void *opaque, hwaddr addr, unsigned size)
 {
     SuperIOConfig *superio_conf = opaque;
 
@@ -101,6 +103,16 @@ static uint32_t superio_ioport_readb(void *opaque, uint32_t addr)
     return (superio_conf->config[superio_conf->index]);
 }
 
+static const MemoryRegionOps superio_ops = {
+    .read = superio_ioport_readb,
+    .write = superio_ioport_writeb,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void vt82c686b_reset(void * opaque)
 {
     PCIDevice *d = opaque;
@@ -140,17 +152,7 @@ static void vt82c686b_write_config(PCIDevice * d, uint32_t address,
 
     pci_default_write_config(d, address, val, len);
     if (address == 0x85) {  /* enable or disable super IO configure */
-        if (val & 0x2) {
-            /* floppy also uses 0x3f0 and 0x3f1.
-             * But we do not emulate flopy,so just set it here. */
-            isa_unassign_ioport(0x3f0, 2);
-            register_ioport_read(0x3f0, 2, 1, superio_ioport_readb,
-                                 &vt686->superio_conf);
-            register_ioport_write(0x3f0, 2, 1, superio_ioport_writeb,
-                                  &vt686->superio_conf);
-        } else {
-            isa_unassign_ioport(0x3f0, 2);
-        }
+        memory_region_set_enabled(&vt686->superio, val & 0x2);
     }
 }
 
@@ -423,11 +425,13 @@ static const VMStateDescription vmstate_via = {
 /* init the PCI-to-ISA bridge */
 static int vt82c686b_initfn(PCIDevice *d)
 {
+    VT82C686BState *vt82c = DO_UPCAST(VT82C686BState, dev, d);
     uint8_t *pci_conf;
+    ISABus *isa_bus;
     uint8_t *wmask;
     int i;
 
-    isa_bus_new(&d->qdev, pci_address_space_io(d));
+    isa_bus = isa_bus_new(&d->qdev, pci_address_space_io(d));
 
     pci_conf = d->config;
     pci_config_set_prog_interface(pci_conf, 0x0);
@@ -439,6 +443,14 @@ static int vt82c686b_initfn(PCIDevice *d)
        }
     }
 
+    memory_region_init_io(&vt82c->superio, &superio_ops, &vt82c->superio_conf,
+                          "superio", 2);
+    memory_region_set_enabled(&vt82c->superio, false);
+    /* The floppy also uses 0x3f0 and 0x3f1.
+     * But we do not emulate a floppy, so just set it here. */
+    memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
+                                &vt82c->superio);
+
     qemu_register_reset(vt82c686b_reset, d);
 
     return 0;
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 07/14] Privatize register_ioport_read/write

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

No more users outside of ioport.c.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/exec/ioport.h |    4 ----
 ioport.c              |    8 ++++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index fc28350..4953892 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -39,10 +39,6 @@ typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
 typedef void (IOPortDestructor)(void *opaque);
 
 void ioport_register(IORange *iorange);
-int register_ioport_read(pio_addr_t start, int length, int size,
-                         IOPortReadFunc *func, void *opaque);
-int register_ioport_write(pio_addr_t start, int length, int size,
-                          IOPortWriteFunc *func, void *opaque);
 void isa_unassign_ioport(pio_addr_t start, int length);
 bool isa_is_ioport_assigned(pio_addr_t start);
 
diff --git a/ioport.c b/ioport.c
index a0ac2a0..d5b7fbd 100644
--- a/ioport.c
+++ b/ioport.c
@@ -139,8 +139,8 @@ static int ioport_bsize(int size, int *bsize)
 }
 
 /* size is the word size in byte */
-int register_ioport_read(pio_addr_t start, int length, int size,
-                         IOPortReadFunc *func, void *opaque)
+static int register_ioport_read(pio_addr_t start, int length, int size,
+                                IOPortReadFunc *func, void *opaque)
 {
     int i, bsize;
 
@@ -159,8 +159,8 @@ int register_ioport_read(pio_addr_t start, int length, int size,
 }
 
 /* size is the word size in byte */
-int register_ioport_write(pio_addr_t start, int length, int size,
-                          IOPortWriteFunc *func, void *opaque)
+static int register_ioport_write(pio_addr_t start, int length, int size,
+                                 IOPortWriteFunc *func, void *opaque)
 {
     int i, bsize;
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 08/14] isa: implement isa_is_ioport_assigned via memory_region_find

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Open-code isa_is_ioport_assigned via a memory region lookup. As all IO
ports are now directly or indirectly registered via the memory API, this
becomes possible and will finally allow us to drop the ioport tables.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/acpi/piix4.c       |    9 +++++----
 hw/isa/lpc_ich9.c     |    9 +++++----
 include/exec/ioport.h |    1 -
 ioport.c              |    7 -------
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 756df3b..ff559c0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -383,14 +383,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
 static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 {
     PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
+    MemoryRegion *io_as = pci_address_space_io(&s->dev);
     uint8_t *pci_conf;
 
     pci_conf = s->dev.config;
-    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
+    pci_conf[0x5f] = 0x10 |
+        (memory_region_find(io_as, 0x378, 1).mr ? 0x80 : 0);
     pci_conf[0x63] = 0x60;
-    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
-	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
-
+    pci_conf[0x67] = (memory_region_find(io_as, 0x3f8, 1).mr ? 0x08 : 0) |
+        (memory_region_find(io_as, 0x2f8, 1).mr ? 0x90 : 0);
 }
 
 static int piix4_pm_initfn(PCIDevice *dev)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 667e882..461ab7c 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -477,22 +477,23 @@ static const MemoryRegionOps rbca_mmio_ops = {
 static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
 {
     ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
+    MemoryRegion *io_as = pci_address_space_io(&s->d);
     uint8_t *pci_conf;
 
     pci_conf = s->d.config;
-    if (isa_is_ioport_assigned(0x3f8)) {
+    if (memory_region_find(io_as, 0x3f8, 1).mr) {
         /* com1 */
         pci_conf[0x82] |= 0x01;
     }
-    if (isa_is_ioport_assigned(0x2f8)) {
+    if (memory_region_find(io_as, 0x2f8, 1).mr) {
         /* com2 */
         pci_conf[0x82] |= 0x02;
     }
-    if (isa_is_ioport_assigned(0x378)) {
+    if (memory_region_find(io_as, 0x378, 1).mr) {
         /* lpt */
         pci_conf[0x82] |= 0x04;
     }
-    if (isa_is_ioport_assigned(0x3f0)) {
+    if (memory_region_find(io_as, 0x3f0, 1).mr) {
         /* floppy */
         pci_conf[0x82] |= 0x08;
     }
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index 4953892..eb99ffe 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -40,7 +40,6 @@ typedef void (IOPortDestructor)(void *opaque);
 
 void ioport_register(IORange *iorange);
 void isa_unassign_ioport(pio_addr_t start, int length);
-bool isa_is_ioport_assigned(pio_addr_t start);
 
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
diff --git a/ioport.c b/ioport.c
index d5b7fbd..56470c5 100644
--- a/ioport.c
+++ b/ioport.c
@@ -273,13 +273,6 @@ void isa_unassign_ioport(pio_addr_t start, int length)
     }
 }
 
-bool isa_is_ioport_assigned(pio_addr_t start)
-{
-    return (ioport_read_table[0][start] || ioport_write_table[0][start] ||
-	    ioport_read_table[1][start] || ioport_write_table[1][start] ||
-	    ioport_read_table[2][start] || ioport_write_table[2][start]);
-}
-
 /***********************************************************/
 
 void cpu_outb(pio_addr_t addr, uint8_t val)
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 09/14] vmware-vga: Accept unaligned I/O accesses

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Before switching to the memory core dispatcher, we need to make sure
that this pv-device will continue to receive unaligned portio accesses.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/display/vmware_vga.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index fd3569d..ec41681 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1241,6 +1241,10 @@ static const MemoryRegionOps vmsvga_io_ops = {
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
+        .unaligned = true,
+    },
+    .impl = {
+        .unaligned = true,
     },
 };
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 10/14] xen: Mark fixed platform I/O as unaligned

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Before switching to the memory core dispatcher, we need to make sure
that this pv-device will continue to receive unaligned portio accesses.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/xen/xen_platform.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
index b6c6793..f8f5dd5 100644
--- a/hw/xen/xen_platform.c
+++ b/hw/xen/xen_platform.c
@@ -262,9 +262,13 @@ static void platform_fixed_ioport_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps platform_fixed_io_ops = {
     .read = platform_fixed_ioport_read,
     .write = platform_fixed_ioport_write,
+    .valid = {
+        .unaligned = true,
+    },
     .impl = {
         .min_access_size = 1,
         .max_access_size = 4,
+        .unaligned = true,
     },
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The current ioport dispatcher is a complex beast, mostly due to the
need to deal with old portio interface users. But we can overcome it
without converting all portio users by embedding the required base
address of a MemoryRegionPortio access into that data structure. That
removes the need to have the additional MemoryRegionIORange structure
in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers
for portio memory regions when registering them with the memory core.
This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the
special address space listener. cpu_in and cpu_out now simply call
address_space_read/write. And we can concentrate portio handling in a
single source file.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                         |   27 --------
 include/exec/ioport.h          |    1 -
 include/exec/memory-internal.h |    2 -
 include/exec/memory.h          |    5 +-
 ioport.c                       |  137 +++++++++++++++++++++++++++++-----------
 memory.c                       |   88 -------------------------
 6 files changed, 101 insertions(+), 159 deletions(-)

diff --git a/exec.c b/exec.c
index 0b0118b..87c7ef3 100644
--- a/exec.c
+++ b/exec.c
@@ -1760,26 +1760,6 @@ static void core_log_global_stop(MemoryListener *listener)
     cpu_physical_memory_set_dirty_tracking(0);
 }
 
-static void io_region_add(MemoryListener *listener,
-                          MemoryRegionSection *section)
-{
-    MemoryRegionIORange *mrio = g_new(MemoryRegionIORange, 1);
-
-    mrio->mr = section->mr;
-    mrio->offset = section->offset_within_region;
-    iorange_init(&mrio->iorange, &memory_region_iorange_ops,
-                 section->offset_within_address_space,
-                 int128_get64(section->size));
-    ioport_register(&mrio->iorange);
-}
-
-static void io_region_del(MemoryListener *listener,
-                          MemoryRegionSection *section)
-{
-    isa_unassign_ioport(section->offset_within_address_space,
-                        int128_get64(section->size));
-}
-
 static MemoryListener core_memory_listener = {
     .begin = core_begin,
     .log_global_start = core_log_global_start,
@@ -1787,12 +1767,6 @@ static MemoryListener core_memory_listener = {
     .priority = 1,
 };
 
-static MemoryListener io_memory_listener = {
-    .region_add = io_region_add,
-    .region_del = io_region_del,
-    .priority = 0,
-};
-
 static MemoryListener tcg_memory_listener = {
     .commit = tcg_commit,
 };
@@ -1834,7 +1808,6 @@ static void memory_map_init(void)
     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
-    memory_listener_register(&io_memory_listener, &address_space_io);
     memory_listener_register(&tcg_memory_listener, &address_space_memory);
 }
 
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index eb99ffe..b476857 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -56,7 +56,6 @@ typedef struct PortioList {
     struct MemoryRegion *address_space;
     unsigned nr;
     struct MemoryRegion **regions;
-    struct MemoryRegion **aliases;
     void *opaque;
     const char *name;
 } PortioList;
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 26689fe..d0e0633 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -119,8 +119,6 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
                                      int dirty_flags);
 
-extern const IORangeOps memory_region_iorange_ops;
-
 #endif
 
 #endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3598c4f..7c1e6da 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -124,10 +124,6 @@ struct MemoryRegionOps {
          bool unaligned;
     } impl;
 
-    /* If .read and .write are not present, old_portio may be used for
-     * backwards compatibility with old portio registration
-     */
-    const MemoryRegionPortio *old_portio;
     /* If .read and .write are not present, old_mmio may be used for
      * backwards compatibility with old mmio registration
      */
@@ -183,6 +179,7 @@ struct MemoryRegionPortio {
     unsigned size;
     IOPortReadFunc *read;
     IOPortWriteFunc *write;
+    uint32_t base; /* private field */
 };
 
 #define PORTIO_END_OF_LIST() { }
diff --git a/ioport.c b/ioport.c
index 56470c5..e34f6b2 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
 #include "exec/ioport.h"
 #include "trace.h"
 #include "exec/memory.h"
+#include "exec/address-spaces.h"
 
 /***********************************************************/
 /* IO Port */
@@ -47,6 +48,12 @@
 #  define LOG_IOPORT(...) do { } while (0)
 #endif
 
+typedef struct MemoryRegionPortioList {
+    MemoryRegion mr;
+    void *portio_opaque;
+    MemoryRegionPortio ports[];
+} MemoryRegionPortioList;
+
 /* XXX: use a two level table to limit memory usage */
 
 static void *ioport_opaque[MAX_IOPORTS];
@@ -279,27 +286,34 @@ void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(0, addr, val);
+    address_space_write(&address_space_io, addr, &val, 1);
 }
 
 void cpu_outw(pio_addr_t addr, uint16_t val)
 {
+    uint8_t buf[2];
+
     LOG_IOPORT("outw: %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(1, addr, val);
+    stw_p(buf, val);
+    address_space_write(&address_space_io, addr, buf, 2);
 }
 
 void cpu_outl(pio_addr_t addr, uint32_t val)
 {
+    uint8_t buf[4];
+
     LOG_IOPORT("outl: %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(2, addr, val);
+    stl_p(buf, val);
+    address_space_write(&address_space_io, addr, buf, 4);
 }
 
 uint8_t cpu_inb(pio_addr_t addr)
 {
     uint8_t val;
-    val = ioport_read(0, addr);
+
+    address_space_read(&address_space_io, addr, &val, 1);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inb : %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
     return val;
@@ -307,8 +321,11 @@ uint8_t cpu_inb(pio_addr_t addr)
 
 uint16_t cpu_inw(pio_addr_t addr)
 {
+    uint8_t buf[2];
     uint16_t val;
-    val = ioport_read(1, addr);
+
+    address_space_read(&address_space_io, addr, buf, 2);
+    val = lduw_p(buf);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inw : %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
     return val;
@@ -316,8 +333,11 @@ uint16_t cpu_inw(pio_addr_t addr)
 
 uint32_t cpu_inl(pio_addr_t addr)
 {
+    uint8_t buf[4];
     uint32_t val;
-    val = ioport_read(2, addr);
+
+    address_space_read(&address_space_io, addr, buf, 4);
+    val = ldl_p(buf);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inl : %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
     return val;
@@ -336,7 +356,6 @@ void portio_list_init(PortioList *piolist,
     piolist->ports = callbacks;
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
-    piolist->aliases = g_new0(MemoryRegion *, n);
     piolist->address_space = NULL;
     piolist->opaque = opaque;
     piolist->name = name;
@@ -345,46 +364,95 @@ void portio_list_init(PortioList *piolist,
 void portio_list_destroy(PortioList *piolist)
 {
     g_free(piolist->regions);
-    g_free(piolist->aliases);
 }
 
+static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
+                                             uint64_t offset, unsigned size,
+                                             bool write)
+{
+    const MemoryRegionPortio *mrp;
+
+    for (mrp = mrpio->ports; mrp->size; ++mrp) {
+        if (offset >= mrp->offset && offset < mrp->offset + mrp->len &&
+            size == mrp->size &&
+            (write ? (bool)mrp->write : (bool)mrp->read)) {
+            return mrp;
+        }
+    }
+    return NULL;
+}
+
+static uint64_t portio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, false);
+    uint64_t data;
+
+    data = ((uint64_t)1 << (size * 8)) - 1;
+    if (mrp) {
+        data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, false);
+        assert(mrp);
+        data = mrp->read(mrpio->portio_opaque, mrp->base + addr) |
+                (mrp->read(mrpio->portio_opaque, mrp->base + addr + 1) << 8);
+    }
+    return data;
+}
+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+
 static void portio_list_add_1(PortioList *piolist,
                               const MemoryRegionPortio *pio_init,
                               unsigned count, unsigned start,
                               unsigned off_low, unsigned off_high)
 {
-    MemoryRegionPortio *pio;
-    MemoryRegionOps *ops;
-    MemoryRegion *region, *alias;
+    MemoryRegionPortioList *mrpio;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    pio = g_new(MemoryRegionPortio, count + 1);
-    memcpy(pio, pio_init, sizeof(MemoryRegionPortio) * count);
-    memset(pio + count, 0, sizeof(MemoryRegionPortio));
+    mrpio = g_malloc0(sizeof(MemoryRegionPortioList) +
+                      sizeof(MemoryRegionPortio) * (count + 1));
+    mrpio->portio_opaque = piolist->opaque;
+    memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
+    memset(mrpio->ports + count, 0, sizeof(MemoryRegionPortio));
 
     /* Adjust the offsets to all be zero-based for the region.  */
     for (i = 0; i < count; ++i) {
-        pio[i].offset -= off_low;
+        mrpio->ports[i].offset -= off_low;
+        mrpio->ports[i].base = start + off_low;
     }
 
-    ops = g_new0(MemoryRegionOps, 1);
-    ops->old_portio = pio;
-
-    region = g_new(MemoryRegion, 1);
-    alias = g_new(MemoryRegion, 1);
     /*
      * Use an alias so that the callback is called with an absolute address,
      * rather than an offset relative to to start + off_low.
      */
-    memory_region_init_io(region, ops, piolist->opaque, piolist->name,
-                          INT64_MAX);
-    memory_region_init_alias(alias, piolist->name,
-                             region, start + off_low, off_high - off_low);
+    memory_region_init_io(&mrpio->mr, &portio_ops, mrpio, piolist->name,
+                          off_high - off_low);
     memory_region_add_subregion(piolist->address_space,
-                                start + off_low, alias);
-    piolist->regions[piolist->nr] = region;
-    piolist->aliases[piolist->nr] = alias;
+                                start + off_low, &mrpio->mr);
+    piolist->regions[piolist->nr] = &mrpio->mr;
     ++piolist->nr;
 }
 
@@ -427,19 +495,14 @@ void portio_list_add(PortioList *piolist,
 
 void portio_list_del(PortioList *piolist)
 {
-    MemoryRegion *mr, *alias;
+    MemoryRegionPortioList *mrpio;
     unsigned i;
 
     for (i = 0; i < piolist->nr; ++i) {
-        mr = piolist->regions[i];
-        alias = piolist->aliases[i];
-        memory_region_del_subregion(piolist->address_space, alias);
-        memory_region_destroy(alias);
-        memory_region_destroy(mr);
-        g_free((MemoryRegionOps *)mr->ops);
-        g_free(mr);
-        g_free(alias);
+        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
+        memory_region_del_subregion(piolist->address_space, &mrpio->mr);
+        memory_region_destroy(&mrpio->mr);
+        g_free(mrpio);
         piolist->regions[i] = NULL;
-        piolist->aliases[i] = NULL;
     }
 }
diff --git a/memory.c b/memory.c
index 47b005a..df07b24 100644
--- a/memory.c
+++ b/memory.c
@@ -401,94 +401,6 @@ static void access_with_adjusted_size(hwaddr addr,
     }
 }
 
-static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset,
-                                             unsigned width, bool write)
-{
-    const MemoryRegionPortio *mrp;
-
-    for (mrp = mr->ops->old_portio; mrp->size; ++mrp) {
-        if (offset >= mrp->offset && offset < mrp->offset + mrp->len
-            && width == mrp->size
-            && (write ? (bool)mrp->write : (bool)mrp->read)) {
-            return mrp;
-        }
-    }
-    return NULL;
-}
-
-static void memory_region_iorange_read(IORange *iorange,
-                                       uint64_t offset,
-                                       unsigned width,
-                                       uint64_t *data)
-{
-    MemoryRegionIORange *mrio
-        = container_of(iorange, MemoryRegionIORange, iorange);
-    MemoryRegion *mr = mrio->mr;
-
-    offset += mrio->offset;
-    if (mr->ops->old_portio) {
-        const MemoryRegionPortio *mrp = find_portio(mr, offset - mrio->offset,
-                                                    width, false);
-
-        *data = ((uint64_t)1 << (width * 8)) - 1;
-        if (mrp) {
-            *data = mrp->read(mr->opaque, offset);
-        } else if (width == 2) {
-            mrp = find_portio(mr, offset - mrio->offset, 1, false);
-            assert(mrp);
-            *data = mrp->read(mr->opaque, offset) |
-                    (mrp->read(mr->opaque, offset + 1) << 8);
-        }
-        return;
-    }
-    *data = 0;
-    access_with_adjusted_size(offset, data, width,
-                              mr->ops->impl.min_access_size,
-                              mr->ops->impl.max_access_size,
-                              memory_region_read_accessor, mr);
-}
-
-static void memory_region_iorange_write(IORange *iorange,
-                                        uint64_t offset,
-                                        unsigned width,
-                                        uint64_t data)
-{
-    MemoryRegionIORange *mrio
-        = container_of(iorange, MemoryRegionIORange, iorange);
-    MemoryRegion *mr = mrio->mr;
-
-    offset += mrio->offset;
-    if (mr->ops->old_portio) {
-        const MemoryRegionPortio *mrp = find_portio(mr, offset - mrio->offset,
-                                                    width, true);
-
-        if (mrp) {
-            mrp->write(mr->opaque, offset, data);
-        } else if (width == 2) {
-            mrp = find_portio(mr, offset - mrio->offset, 1, true);
-            assert(mrp);
-            mrp->write(mr->opaque, offset, data & 0xff);
-            mrp->write(mr->opaque, offset + 1, data >> 8);
-        }
-        return;
-    }
-    access_with_adjusted_size(offset, &data, width,
-                              mr->ops->impl.min_access_size,
-                              mr->ops->impl.max_access_size,
-                              memory_region_write_accessor, mr);
-}
-
-static void memory_region_iorange_destructor(IORange *iorange)
-{
-    g_free(container_of(iorange, MemoryRegionIORange, iorange));
-}
-
-const IORangeOps memory_region_iorange_ops = {
-    .read = memory_region_iorange_read,
-    .write = memory_region_iorange_write,
-    .destructor = memory_region_iorange_destructor,
-};
-
 static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
 {
     AddressSpace *as;
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Hervé Poussineau]

Jan Kiszka a écrit :
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The current ioport dispatcher is a complex beast, mostly due to the
> need to deal with old portio interface users. But we can overcome it
> without converting all portio users by embedding the required base
> address of a MemoryRegionPortio access into that data structure. That
> removes the need to have the additional MemoryRegionIORange structure
> in the loop on every access.
> 
> To handle old portio memory ops, we simply install dispatching handlers
> for portio memory regions when registering them with the memory core.
> This removes the need for the old_portio field.
> 
> We can drop the additional aliasing of ioport regions and also the
> special address space listener. cpu_in and cpu_out now simply call
> address_space_read/write. And we can concentrate portio handling in a
> single source file.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---

...

> +
> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
> +                         unsigned size)
> +{
> +    MemoryRegionPortioList *mrpio = opaque;
> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
> +
> +    if (mrp) {
> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
> +    } else if (size == 2) {
> +        mrp = find_portio(mrpio, addr, 1, true);
> +        assert(mrp);
> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
> +    }
> +}
> +
> +static const MemoryRegionOps portio_ops = {
> +    .read = portio_read,
> +    .write = portio_write,
> +    .valid.unaligned = true,
> +    .impl.unaligned = true,
> +};
> +

You need to mark these operations as DEVICE_LITTLE_ENDIAN.
In portio_write above, you clearly assume that data is in LE format.

This fixes PPC PReP emulation, which would otherwise be broken with this 
patchset.

Hervé

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On 2013-06-23 22:50, Hervé Poussineau wrote:
> Jan Kiszka a écrit :
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The current ioport dispatcher is a complex beast, mostly due to the
>> need to deal with old portio interface users. But we can overcome it
>> without converting all portio users by embedding the required base
>> address of a MemoryRegionPortio access into that data structure. That
>> removes the need to have the additional MemoryRegionIORange structure
>> in the loop on every access.
>>
>> To handle old portio memory ops, we simply install dispatching handlers
>> for portio memory regions when registering them with the memory core.
>> This removes the need for the old_portio field.
>>
>> We can drop the additional aliasing of ioport regions and also the
>> special address space listener. cpu_in and cpu_out now simply call
>> address_space_read/write. And we can concentrate portio handling in a
>> single source file.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
> 
> ...
> 
>> +
>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>> +                         unsigned size)
>> +{
>> +    MemoryRegionPortioList *mrpio = opaque;
>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>> true);
>> +
>> +    if (mrp) {
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>> +    } else if (size == 2) {
>> +        mrp = find_portio(mrpio, addr, 1, true);
>> +        assert(mrp);
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>> >> 8);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps portio_ops = {
>> +    .read = portio_read,
>> +    .write = portio_write,
>> +    .valid.unaligned = true,
>> +    .impl.unaligned = true,
>> +};
>> +
> 
> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
> In portio_write above, you clearly assume that data is in LE format.

Anything behind PIO is little endian, of course. Will add this.

> 
> This fixes PPC PReP emulation, which would otherwise be broken with this
> patchset.

Thanks,
Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

On 24.06.2013, at 08:07, Jan Kiszka wrote:

> On 2013-06-23 22:50, Hervé Poussineau wrote:
>> Jan Kiszka a écrit :
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>> 
>>> The current ioport dispatcher is a complex beast, mostly due to the
>>> need to deal with old portio interface users. But we can overcome it
>>> without converting all portio users by embedding the required base
>>> address of a MemoryRegionPortio access into that data structure. That
>>> removes the need to have the additional MemoryRegionIORange structure
>>> in the loop on every access.
>>> 
>>> To handle old portio memory ops, we simply install dispatching handlers
>>> for portio memory regions when registering them with the memory core.
>>> This removes the need for the old_portio field.
>>> 
>>> We can drop the additional aliasing of ioport regions and also the
>>> special address space listener. cpu_in and cpu_out now simply call
>>> address_space_read/write. And we can concentrate portio handling in a
>>> single source file.
>>> 
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>> 
>> ...
>> 
>>> +
>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>> +                         unsigned size)
>>> +{
>>> +    MemoryRegionPortioList *mrpio = opaque;
>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>> true);
>>> +
>>> +    if (mrp) {
>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>> +    } else if (size == 2) {
>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>> +        assert(mrp);
>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>> 8);
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps portio_ops = {
>>> +    .read = portio_read,
>>> +    .write = portio_write,
>>> +    .valid.unaligned = true,
>>> +    .impl.unaligned = true,
>>> +};
>>> +
>> 
>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>> In portio_write above, you clearly assume that data is in LE format.
> 
> Anything behind PIO is little endian, of course. Will add this.

This patch breaks VGA on PPC as it is in master today.


Alex

> 
>> 
>> This fixes PPC PReP emulation, which would otherwise be broken with this
>> patchset.
> 
> Thanks,
> Jan
> 
> 

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

On 11.07.2013, at 14:29, Alexander Graf wrote:

> 
> On 24.06.2013, at 08:07, Jan Kiszka wrote:
> 
>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>> Jan Kiszka a écrit :
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>> 
>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>> need to deal with old portio interface users. But we can overcome it
>>>> without converting all portio users by embedding the required base
>>>> address of a MemoryRegionPortio access into that data structure. That
>>>> removes the need to have the additional MemoryRegionIORange structure
>>>> in the loop on every access.
>>>> 
>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>> for portio memory regions when registering them with the memory core.
>>>> This removes the need for the old_portio field.
>>>> 
>>>> We can drop the additional aliasing of ioport regions and also the
>>>> special address space listener. cpu_in and cpu_out now simply call
>>>> address_space_read/write. And we can concentrate portio handling in a
>>>> single source file.
>>>> 
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>> 
>>> ...
>>> 
>>>> +
>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>> +                         unsigned size)
>>>> +{
>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>> true);
>>>> +
>>>> +    if (mrp) {
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>> +    } else if (size == 2) {
>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>> +        assert(mrp);
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>> 8);
>>>> +    }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps portio_ops = {
>>>> +    .read = portio_read,
>>>> +    .write = portio_write,
>>>> +    .valid.unaligned = true,
>>>> +    .impl.unaligned = true,
>>>> +};
>>>> +
>>> 
>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>> In portio_write above, you clearly assume that data is in LE format.
>> 
>> Anything behind PIO is little endian, of course. Will add this.
> 
> This patch breaks VGA on PPC as it is in master today.

If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.


Alex

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Andreas Färber]

Am 11.07.2013 14:34, schrieb Alexander Graf:
> 
> On 11.07.2013, at 14:29, Alexander Graf wrote:
> 
>>
>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>
>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>> Jan Kiszka a écrit :
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>> need to deal with old portio interface users. But we can overcome it
>>>>> without converting all portio users by embedding the required base
>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>> in the loop on every access.
>>>>>
>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>> for portio memory regions when registering them with the memory core.
>>>>> This removes the need for the old_portio field.
>>>>>
>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>> single source file.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>> +                         unsigned size)
>>>>> +{
>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>> true);
>>>>> +
>>>>> +    if (mrp) {
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>> +    } else if (size == 2) {
>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>> +        assert(mrp);
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>> 8);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static const MemoryRegionOps portio_ops = {
>>>>> +    .read = portio_read,
>>>>> +    .write = portio_write,
>>>>> +    .valid.unaligned = true,
>>>>> +    .impl.unaligned = true,
>>>>> +};
>>>>> +
>>>>
>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>> In portio_write above, you clearly assume that data is in LE format.
>>>
>>> Anything behind PIO is little endian, of course. Will add this.
>>
>> This patch breaks VGA on PPC as it is in master today.
> 
> If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.

sPAPR has its MemoryRegion marked Little Endian:

http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b

Possibly we can now apply Hervé's patches on top to remove that hack again?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

On 11.07.2013, at 14:46, Andreas Färber wrote:

> Am 11.07.2013 14:34, schrieb Alexander Graf:
>> 
>> On 11.07.2013, at 14:29, Alexander Graf wrote:
>> 
>>> 
>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>> 
>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>> Jan Kiszka a écrit :
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> 
>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>> without converting all portio users by embedding the required base
>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>> in the loop on every access.
>>>>>> 
>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>> for portio memory regions when registering them with the memory core.
>>>>>> This removes the need for the old_portio field.
>>>>>> 
>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>> single source file.
>>>>>> 
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>> 
>>>>> ...
>>>>> 
>>>>>> +
>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>> +                         unsigned size)
>>>>>> +{
>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>> true);
>>>>>> +
>>>>>> +    if (mrp) {
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>> +    } else if (size == 2) {
>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>> +        assert(mrp);
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>> 8);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>> +    .read = portio_read,
>>>>>> +    .write = portio_write,
>>>>>> +    .valid.unaligned = true,
>>>>>> +    .impl.unaligned = true,
>>>>>> +};
>>>>>> +
>>>>> 
>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>> 
>>>> Anything behind PIO is little endian, of course. Will add this.
>>> 
>>> This patch breaks VGA on PPC as it is in master today.
>> 
>> If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.
> 
> sPAPR has its MemoryRegion marked Little Endian:

sPAPR works, it's only the Mac machines that break.


Apex

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

On 11.07.2013, at 14:48, Alexander Graf wrote:

> 
> On 11.07.2013, at 14:46, Andreas Färber wrote:
> 
>> Am 11.07.2013 14:34, schrieb Alexander Graf:
>>> 
>>> On 11.07.2013, at 14:29, Alexander Graf wrote:
>>> 
>>>> 
>>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>> 
>>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>>> Jan Kiszka a écrit :
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> 
>>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>>> without converting all portio users by embedding the required base
>>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>>> in the loop on every access.
>>>>>>> 
>>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>>> for portio memory regions when registering them with the memory core.
>>>>>>> This removes the need for the old_portio field.
>>>>>>> 
>>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>>> single source file.
>>>>>>> 
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>> 
>>>>>> ...
>>>>>> 
>>>>>>> +
>>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>> +                         unsigned size)
>>>>>>> +{
>>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>>> true);
>>>>>>> +
>>>>>>> +    if (mrp) {
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>>> +    } else if (size == 2) {
>>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>>> +        assert(mrp);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>>> 8);
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>>> +    .read = portio_read,
>>>>>>> +    .write = portio_write,
>>>>>>> +    .valid.unaligned = true,
>>>>>>> +    .impl.unaligned = true,
>>>>>>> +};
>>>>>>> +
>>>>>> 
>>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>> 
>>>>> Anything behind PIO is little endian, of course. Will add this.
>>>> 
>>>> This patch breaks VGA on PPC as it is in master today.
>>> 
>>> If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.
>> 
>> sPAPR has its MemoryRegion marked Little Endian:
> 
> sPAPR works, it's only the Mac machines that break.

So IIUC before cpu_inw and inl were doing portio accesses in host native endianness. Now with this change they do little endian accesses. All sensible callers of cpu_inX assume that data is passed in native endianness though and already do the little endian conversion themselves.

Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.


Alex

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

On 11.07.2013, at 15:28, Alexander Graf wrote:

> 
> On 11.07.2013, at 14:48, Alexander Graf wrote:
> 
>> 
>> On 11.07.2013, at 14:46, Andreas Färber wrote:
>> 
>>> Am 11.07.2013 14:34, schrieb Alexander Graf:
>>>> 
>>>> On 11.07.2013, at 14:29, Alexander Graf wrote:
>>>> 
>>>>> 
>>>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>>> 
>>>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>>>> Jan Kiszka a écrit :
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> 
>>>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>>>> without converting all portio users by embedding the required base
>>>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>>>> in the loop on every access.
>>>>>>>> 
>>>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>>>> for portio memory regions when registering them with the memory core.
>>>>>>>> This removes the need for the old_portio field.
>>>>>>>> 
>>>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>>>> single source file.
>>>>>>>> 
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> ---
>>>>>>> 
>>>>>>> ...
>>>>>>> 
>>>>>>>> +
>>>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>>> +                         unsigned size)
>>>>>>>> +{
>>>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>>>> true);
>>>>>>>> +
>>>>>>>> +    if (mrp) {
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>>>> +    } else if (size == 2) {
>>>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>>>> +        assert(mrp);
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>>>> 8);
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>>>> +    .read = portio_read,
>>>>>>>> +    .write = portio_write,
>>>>>>>> +    .valid.unaligned = true,
>>>>>>>> +    .impl.unaligned = true,
>>>>>>>> +};
>>>>>>>> +
>>>>>>> 
>>>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>>> 
>>>>>> Anything behind PIO is little endian, of course. Will add this.
>>>>> 
>>>>> This patch breaks VGA on PPC as it is in master today.
>>>> 
>>>> If I don't mark portio as little endian it works as expected. There's probably someone swapping things twice.
>>> 
>>> sPAPR has its MemoryRegion marked Little Endian:
>> 
>> sPAPR works, it's only the Mac machines that break.
> 
> So IIUC before cpu_inw and inl were doing portio accesses in host native endianness. Now with this change they do little endian accesses. All sensible callers of cpu_inX assume that data is passed in native endianness though and already do the little endian conversion themselves.
> 
> Semantically having your PCI host bridge do the endianness conversion is the correct way of handling it, as that's where the conversion happens. If it makes life easier to do it in the isa bridging code, that's fine for me too though. But then we'll have to get rid of all endianness swaps that already happen in PCI bridges.

Looking at a few more cpu_inX users I just stumbled over this MIPS code:

static uint64_t rtc_read(void *opaque, hwaddr addr, unsigned size)
{
    return cpu_inw(0x71);
}

static void rtc_write(void *opaque, hwaddr addr,
                      uint64_t val, unsigned size)
{
    cpu_outw(0x71, val & 0xff);
}

static const MemoryRegionOps rtc_ops = {
    .read = rtc_read,
    .write = rtc_write,
    .endianness = DEVICE_NATIVE_ENDIAN,
};


That one returned the ioport data in native endianness before (big endian) and should also be broken now. In this case there's no PCI bus to swizzle or not swizzle things around though.

Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Thu, 2013-07-11 at 15:28 +0200, Alexander Graf wrote:
> So IIUC before cpu_inw and inl were doing portio accesses in host
> native endianness. Now with this change they do little endian
> accesses. All sensible callers of cpu_inX assume that data is passed
> in native endianness though and already do the little endian
> conversion themselves.
> 
> Semantically having your PCI host bridge do the endianness conversion
> is the correct way of handling it, as that's where the conversion
> happens. If it makes life easier to do it in the isa bridging code,
> that's fine for me too though. But then we'll have to get rid of all
> endianness swaps that already happen in PCI bridges.

Or stop being utterly insane and remove all that endianness crap in
bridges etc... :-) The whole thing is completely ass backward to begin
with.

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Thu, 2013-07-11 at 15:28 +0200, Alexander Graf wrote:
> 
> Semantically having your PCI host bridge do the endianness conversion
> is the correct way of handling it, as that's where the conversion
> happens. If it makes life easier to do it in the isa bridging code,
> that's fine for me too though. But then we'll have to get rid of all
> endianness swaps that already happen in PCI bridges.

BTW. Why would IOs to a PCI VGA device go through an "ISA bridge" of any
kind ? PCI IO space is PCI IO space... whether there is an ISA bridge or
not is a separate thing and PCI VGA cards certainly don't sit behind
one.

Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

Am 12.07.2013 um 00:32 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> On Thu, 2013-07-11 at 15:28 +0200, Alexander Graf wrote:
>> 
>> Semantically having your PCI host bridge do the endianness conversion
>> is the correct way of handling it, as that's where the conversion
>> happens. If it makes life easier to do it in the isa bridging code,
>> that's fine for me too though. But then we'll have to get rid of all
>> endianness swaps that already happen in PCI bridges.
> 
> BTW. Why would IOs to a PCI VGA device go through an "ISA bridge" of any
> kind ? PCI IO space is PCI IO space... whether there is an ISA bridge or
> not is a separate thing and PCI VGA cards certainly don't sit behind
> one.

We model a single system wide io space today and access to that one happens through you pci host controller. I just messed up the terminology here.

Alex

> 
> Ben.
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 05:18 +0200, Alexander Graf wrote:
> We model a single system wide io space today and access to that one
> happens through you pci host controller. I just messed up the
> terminology here.

Single system wide IO space is broken. We have separate IO space per
PHB. That was working afaik.

In any case, I completely object to all that business with conversion in
bridges.

That's fundamentally WRONG.

The whole business of endianness in qemu is a mess. In the end what
matters and the only thing that does is:

 * The endianness of a given memory access by the guest (which may or
may not be the endianness of the guest -> MSR:LE, byteswap load/store
instsructions, etc..)

vs.

 * The endianness of the target device register (and I say register ...
a framebuffer does NOT have endianness per-se and thus accesses to BAR
mapping a "memory" range (framebuffer, ROM, ...) should go such that the
*byte order* of individual bytes is preserved, which typically means
untranslated).

Unless they are completely broken (and those exist, don't get me wrong,
though mostly they are a thing of a past long gone), bridges and busses
have no effect on endianness.

So I'm not sure what you guys are up to, but from what I read, it's
wrong, and the fact at this stage is that your broke IO space (and thus
virtio and VGA) on powerpc (including pseries).

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Hervé Poussineau]

Benjamin Herrenschmidt a écrit :
> On Fri, 2013-07-12 at 05:18 +0200, Alexander Graf wrote:
>> We model a single system wide io space today and access to that one
>> happens through you pci host controller. I just messed up the
>> terminology here.
> 
> Single system wide IO space is broken. We have separate IO space per
> PHB. That was working afaik.
> 
> In any case, I completely object to all that business with conversion in
> bridges.
> 
> That's fundamentally WRONG.

Indeed.

> 
> The whole business of endianness in qemu is a mess. In the end what
> matters and the only thing that does is:
> 
>  * The endianness of a given memory access by the guest (which may or
> may not be the endianness of the guest -> MSR:LE, byteswap load/store
> instsructions, etc..)
> 

On PowerPC PReP, you have the MSR:LE bit in the CPU, and the system 
endianness (port 0x92 on PReP system I/O board). Of course, both can be 
changed independently, but they are usually changed within a few 
instructions.

When only one of them is changed, this means some "interesting" things 
can happen. For example, in [1]:
"When PowerPC little-endian is in effect but before system's 
little-endian is in effect, the address munge becomes effective. If we 
need to access system endian port (byte) address, say 0x80000092, the 
program needs to issue 0x80000095 instead, to compensate (unmunge) for 
the effect of PowerPC address modification."

Those will be required for QEMU to be able to run BeOS, AIX, or Windows 
NT or PowerPC PReP.

In the light of this, I think there should only be one endianness for 
all memory accesses (which could be changed at runtime), and all 
bridges/devices should ask for "same endianness as parent" or "reverse 
endianness as parent", but not for big, little, or native endianness.

Regards,

Hervé

[1] ftp://ftp.software.ibm.com/rs6000/technology/spec/endian.ps)

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Hervé Poussineau <hpoussin@reactos.org> writes:

> When only one of them is changed, this means some "interesting" things 
> can happen. For example, in [1]:
> "When PowerPC little-endian is in effect but before system's 
> little-endian is in effect, the address munge becomes effective. If we 
> need to access system endian port (byte) address, say 0x80000092, the 
> program needs to issue 0x80000095 instead, to compensate (unmunge) for 
> the effect of PowerPC address modification."
>
> Those will be required for QEMU to be able to run BeOS, AIX, or Windows 
> NT or PowerPC PReP.
>
> In the light of this, I think there should only be one endianness for 
> all memory accesses (which could be changed at runtime),

We already do this, it's "host native endian".

> and all 
> bridges/devices should ask for "same endianness as parent" or "reverse 
> endianness as parent", but not for big, little, or native endianness.

I/O doesn't propagate so there's no way to say something like this.
That's the fundamental problem IMHO.

Regards,

Anthony Liguori

>
> Regards,
>
> Hervé
>
> [1] ftp://ftp.software.ibm.com/rs6000/technology/spec/endian.ps)

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 14:06 -0500, Anthony Liguori wrote:
> > In the light of this, I think there should only be one endianness
> for 
> > all memory accesses (which could be changed at runtime),
> 
> We already do this, it's "host native endian".

The simple fact that you used the word "endian" makes it broken here :-)

If the guest has done a byte reverse access for example, what you have
is not "host native endian" anymore.

The simple fact that you use "endian" to describe what you carry around
is a source of wrongness and errors. It's misleading and people always
get that wrong (I did a bloody talk on the matter at two conferences now
and it seems people really don't get what endianness really is about).

> > and all 
> > bridges/devices should ask for "same endianness as parent" or
> "reverse 
> > endianness as parent", but not for big, little, or native
> endianness.
> 
> I/O doesn't propagate so there's no way to say something like this.
> That's the fundamental problem IMHO.

There is no such thing as "endianness of parent" again. This is all
wrong. Wrong terminology, wrong concepts.

Do not try to use the "endian" attribute on a bus or bridge, it will
only confuse things further.

Stick to what is the byte order of your transport (much more precise
term, ie. which byte is the lowest address within the host variable that
carries the access).

Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 19:04 +0200, Hervé Poussineau wrote:
> "When PowerPC little-endian is in effect but before system's 
> little-endian is in effect, the address munge becomes effective. If we 
> need to access system endian port (byte) address, say 0x80000092, the 
> program needs to issue 0x80000095 instead, to compensate (unmunge) for 
> the effect of PowerPC address modification."
> 
> Those will be required for QEMU to be able to run BeOS, AIX, or Windows 
> NT or PowerPC PReP.

Funnily enough we've dropped support for Linux on PReP a long time
ago :-)

> In the light of this, I think there should only be one endianness for 
> all memory accesses (which could be changed at runtime), and all 
> bridges/devices should ask for "same endianness as parent" or "reverse 
> endianness as parent", but not for big, little, or native endianness.

Right. I would in fact avoid the term "endianness" that tends to confuse
people and basically stick to this:

 - For a given access, know (attribute) which byte is first in ascending
address order

 - For a given target *register* (and again, this ONLY applies to registers
and in fact, some devices might change their register "endianness" and also
have several banks with a different one), which byte is first in ascending
address order.

This is at that point that a conversion might need to occur, and only at that
point if there is a mismatch.

Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Fri, 2013-07-12 at 05:18 +0200, Alexander Graf wrote:
>> We model a single system wide io space today and access to that one
>> happens through you pci host controller. I just messed up the
>> terminology here.
>
> Single system wide IO space is broken. We have separate IO space per
> PHB. That was working afaik.

Hrm, probably not.  We don't propagate I/O spaces very well today.

> In any case, I completely object to all that business with conversion in
> bridges.
>
> That's fundamentally WRONG.
>
> The whole business of endianness in qemu is a mess. In the end what
> matters and the only thing that does is:

It's not as bad as you think I suspect.

>  * The endianness of a given memory access by the guest (which may or
> may not be the endianness of the guest -> MSR:LE, byteswap load/store
> instsructions, etc..)

Correct.

> vs.
>
>  * The endianness of the target device register (and I say register ...
> a framebuffer does NOT have endianness per-se and thus accesses to BAR
> mapping a "memory" range (framebuffer, ROM, ...) should go such that the
> *byte order* of individual bytes is preserved, which typically means
> untranslated).

Yes.  To put it another way, an MMIO write is a store and depending on
the VCPU, that will result in a write with a certain byte order.  That
byte order should be preserved.

However, what we don't model today, and why we have the silly
endianness in MemoryRegionOps, is the fact that I/O may pass through
multiple layers and those layers may change byte ordering.

We jump through great hoops to have a flat dispatch table.  I've never
liked it but that's what we do.  That means that in cases where a host
bridge may do byte swapping, we cannot easily support that.

That's really what endianness is for but it's abused.

> Unless they are completely broken (and those exist, don't get me wrong,
> though mostly they are a thing of a past long gone), bridges and busses
> have no effect on endianness.

That's simply not true.  There are programmable PCI host bridges that
support byte swapping.  Some allow this to be done on a per-device basis
too.

> So I'm not sure what you guys are up to, but from what I read, it's
> wrong, and the fact at this stage is that your broke IO space (and thus
> virtio and VGA) on powerpc (including pseries).

I'm not sure what this patch was trying to do but it was certainly
wrong.

Regards,

Anthony Liguori

>
> Cheers,
> Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Peter Maydell]

On 12 July 2013 18:49, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> On Fri, 2013-07-12 at 05:18 +0200, Alexander Graf wrote:
>>> We model a single system wide io space today and access to that one
>>> happens through you pci host controller. I just messed up the
>>> terminology here.
>>
>> Single system wide IO space is broken. We have separate IO space per
>> PHB. That was working afaik.
>
> Hrm, probably not.  We don't propagate I/O spaces very well today.
>
>> In any case, I completely object to all that business with conversion in
>> bridges.
>>
>> That's fundamentally WRONG.

It's not wrong when the hardware actually does a byteswap at
some level in the memory hierarchy. You can see this for instance
on ARMv7M systems, where byteswapping for bigendian happens at
an intermediate level that not all accesses go through:

 [CPU] ---->  [byteswap here] --> [memory and ext. devices]
         |
         -->  [internal memory mapped devices]

so some things see always little endian regardless.

>> The whole business of endianness in qemu is a mess. In the end what
>> matters and the only thing that does is:
>
> It's not as bad as you think I suspect.
>
>>  * The endianness of a given memory access by the guest (which may or
>> may not be the endianness of the guest -> MSR:LE, byteswap load/store
>> instsructions, etc..)
>
> Correct.
>
>> vs.
>>
>>  * The endianness of the target device register (and I say register ...
>> a framebuffer does NOT have endianness per-se and thus accesses to BAR
>> mapping a "memory" range (framebuffer, ROM, ...) should go such that the
>> *byte order* of individual bytes is preserved, which typically means
>> untranslated).
>
> Yes.  To put it another way, an MMIO write is a store and depending on
> the VCPU, that will result in a write with a certain byte order.  That
> byte order should be preserved.
>
> However, what we don't model today, and why we have the silly
> endianness in MemoryRegionOps, is the fact that I/O may pass through
> multiple layers and those layers may change byte ordering.
>
> We jump through great hoops to have a flat dispatch table.  I've never
> liked it but that's what we do.  That means that in cases where a host
> bridge may do byte swapping, we cannot easily support that.

We could support that if we cared to -- you just have to have a
container MemoryRegion type which is a byte-swapping container
(or just have a flag on existing containers, I suppose).
Then as you flatten the regions into the flat table you keep
track of how many levels of byteswapping each region goes through,
and you end up with a single 'byteswap or not?' flag for each
section of your flat dispatch table.

(Our other serious endianness problem is that we don't really
do very well at supporting a TCG CPU arbitrarily flipping
endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting
and ideally it should not be.)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 19:26 +0100, Peter Maydell wrote:

> It's not wrong when the hardware actually does a byteswap at
> some level in the memory hierarchy. You can see this for instance
> on ARMv7M systems, where byteswapping for bigendian happens at
> an intermediate level that not all accesses go through:
> 
>  [CPU] ---->  [byteswap here] --> [memory and ext. devices]
>          |
>          -->  [internal memory mapped devices]
> 
> so some things see always little endian regardless.

Ugh ? That's so completely fucked up, if that's indeed what the HW is
doing this is a piece of trash and the designers are in urgent need of
being turned into fertilizer.

Unless again you are talking about "lane swapping" which allows to
preserve the byte address invariance when the CPU decides to flip its
bus around, but I would have thought that modern CPUs do not do that
sort of shit anymore.

In any case, it cannot be represented with an "endian" attribute at the
bridge level, that doesn't mean anything. Again, the only endian
attribute that exists are the byte order of the original access (which
byte has the lowest address, regardless of significance of those bytes
in the target, ie, purely from a qemu standpoint, in the variable that
carries the access around inside qemu, which byte has the lowest
address), and the same on the target device (at which point a concept of
significance does apply, but it's a guest driver business to get it
right, qemu just need to make sure byte 0 goes to byte 0).

If a bridge flips things around in a way that breaks the model, then add
some property describing the flipping properties but don't call it "big
endian" or "little endian" at the bridge level, that has no meaning,
confuses things and introduces breakage like we have seen.

> >> The whole business of endianness in qemu is a mess. In the end what
> >> matters and the only thing that does is:
> >
> > It's not as bad as you think I suspect.
> >
> >>  * The endianness of a given memory access by the guest (which may or
> >> may not be the endianness of the guest -> MSR:LE, byteswap load/store
> >> instsructions, etc..)
> >
> > Correct.
> >
> >> vs.
> >>
> >>  * The endianness of the target device register (and I say register ...
> >> a framebuffer does NOT have endianness per-se and thus accesses to BAR
> >> mapping a "memory" range (framebuffer, ROM, ...) should go such that the
> >> *byte order* of individual bytes is preserved, which typically means
> >> untranslated).
> >
> > Yes.  To put it another way, an MMIO write is a store and depending on
> > the VCPU, that will result in a write with a certain byte order.  That
> > byte order should be preserved.
> >
> > However, what we don't model today, and why we have the silly
> > endianness in MemoryRegionOps, is the fact that I/O may pass through
> > multiple layers and those layers may change byte ordering.
> >
> > We jump through great hoops to have a flat dispatch table.  I've never
> > liked it but that's what we do.  That means that in cases where a host
> > bridge may do byte swapping, we cannot easily support that.
> 
> We could support that if we cared to -- you just have to have a
> container MemoryRegion type which is a byte-swapping container
> (or just have a flag on existing containers, I suppose).
> Then as you flatten the regions into the flat table you keep
> track of how many levels of byteswapping each region goes through,
> and you end up with a single 'byteswap or not?' flag for each
> section of your flat dispatch table.
> 
> (Our other serious endianness problem is that we don't really
> do very well at supporting a TCG CPU arbitrarily flipping
> endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting
> and ideally it should not be.)

Our experience is that it actually works fine for almost everything
except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and
should be).

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Peter Maydell]

On 12 July 2013 23:50, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Fri, 2013-07-12 at 19:26 +0100, Peter Maydell wrote:
>> It's not wrong when the hardware actually does a byteswap at
>> some level in the memory hierarchy. You can see this for instance
>> on ARMv7M systems, where byteswapping for bigendian happens at
>> an intermediate level that not all accesses go through:
>>
>>  [CPU] ---->  [byteswap here] --> [memory and ext. devices]
>>          |
>>          -->  [internal memory mapped devices]
>>
>> so some things see always little endian regardless.
>
> Ugh ? That's so completely fucked up, if that's indeed what the HW is
> doing this is a piece of trash and the designers are in urgent need of
> being turned into fertilizer.
>
> Unless again you are talking about "lane swapping" which allows to
> preserve the byte address invariance when the CPU decides to flip its
> bus around, but I would have thought that modern CPUs do not do that
> sort of shit anymore.

The block marked "byteswap here" does "byte invariant bigendian",
so byte accesses are unchanged, 16 bit accesses have the two words
of data flipped, and 32 bit accesses have the four bytes flipped;
this happens as the data passes through; addresses are unchanged.
It only happens if the CPU is configured by the guest to operate
in big-endian mode, obviously.
(Contrast 'word invariant bigendian', which is what ARM used to do,
where the addresses are changed but the data is not. That would be
pretty painful to implement in the memory region API though it is
of course trivial in hardware since it is just XORing of the low
address bits according to the access size...)

> Again, the only endian
> attribute that exists are the byte order of the original access (which
> byte has the lowest address, regardless of significance of those bytes
> in the target, ie, purely from a qemu standpoint, in the variable that
> carries the access around inside qemu, which byte has the lowest
> address)

What does this even mean? At the point where a memory access leaves
the CPU (emulation or real hardware) it has (a) an address and
(b) a width -- a 16 bit access is neither big nor little endian,
it's just a request for 16 bits of data (on real hardware it's
typically a bus transaction on a bunch of data lines with some
control lines indicating transaction width). Now the CPU emulation
may internally be intending to put that data into its emulated
register one way round or the other, but that's an internal detail
of the CPU emulation. (Similarly for stores.)

>, and the same on the target device (at which point a concept of
> significance does apply, but it's a guest driver business to get it
> right, qemu just need to make sure byte 0 goes to byte 0).

Similarly, at the target device end there is no concept
of a "big endian access" -- we make a request for 16
bits of data at a particular address (via the MemoryRegion
API) and the device returns 16 bits of data. It's entirely
possible to design hardware so that byte access to address
X, halfword access to address X and word access to address
X all return entirely different data (though it would be
a bit perverse.) (As an implementation convenience we may
choose to provide helper infrastructure so you don't have
to actually implement all of byte/halfword/word access by hand.)

> If a bridge flips things around in a way that breaks the
> model,

That breaks what model?

> then add some property describing the flipping
> properties but don't call it "big
> endian" or "little endian" at the bridge level, that has no meaning,
> confuses things and introduces breakage like we have seen.

I'm happy to call the property "byteswap", yes, because
that's what it does. If you did two of these in a row you'd
get a no-op.

>> (Our other serious endianness problem is that we don't really
>> do very well at supporting a TCG CPU arbitrarily flipping
>> endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting
>> and ideally it should not be.)
>
> Our experience is that it actually works fine for almost everything
> except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and
> should be).

I agree that TARGET_WORDS_BIGENDIAN *should* go away, but
it exists currently. Do you actually implement a CPU which
does dynamic endianness flipping? Is it at all efficient
in the config which is the opposite of whatever
TARGET_WORDS_BIGENDIAN says?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Sat, 2013-07-13 at 00:10 +0100, Peter Maydell wrote:

> The block marked "byteswap here" does "byte invariant bigendian",
> so byte accesses are unchanged, 16 bit accesses have the two words
> of data flipped, and 32 bit accesses have the four bytes flipped;
> this happens as the data passes through; addresses are unchanged.
> It only happens if the CPU is configured by the guest to operate
> in big-endian mode, obviously.
> (Contrast 'word invariant bigendian', which is what ARM used to do,
> where the addresses are changed but the data is not. That would be
> pretty painful to implement in the memory region API though it is
> of course trivial in hardware since it is just XORing of the low
> address bits according to the access size...)

Which means that ARM switches the byte order of its bus when
switching endian which is very very wrong... oh well.

PowerPCs operate in both endians and nowadays do not require any
adjustement at the bus level. It's simple, the definition that matter
and the only definition that matter for data is which byte is at what
address and that shouldn't change. For addresses, they should be in a
fixed significance order, not change order based on the "endianness" of
the processor.

Or to say things differently, byte order of data should be fixed, and
the "endianness" of the bus (which is purely in that case the byte order
of addresses) as well. ARM seem to have chosen to go from fixing the
byte order of data & changing the bus endianness, to changing the byte
order of data to preserve the bus endianness, or something along those
lines. Both approaches are WRONG.

Anyway, whatever, what is done is done, point is, what you have is a
byte lane swapper which is similar to what older ppc did which indeed
is needed if your bus flips around. I would still not model that using
the term "endianness" of either the bus or bridge. It's again purely a
statement of what byte lane coming out corresponds to what *address*,
regardless of byte significance.

> > Again, the only endian
> > attribute that exists are the byte order of the original access (which
> > byte has the lowest address, regardless of significance of those bytes
> > in the target, ie, purely from a qemu standpoint, in the variable that
> > carries the access around inside qemu, which byte has the lowest
> > address)
> 
> What does this even mean? At the point where a memory access leaves
> the CPU (emulation or real hardware) it has (a) an address and
> (b) a width -- a 16 bit access is neither big nor little endian,

Not from the point of view of the bus indeed. The value inside might or
might not have an endianness but that's purely somebody else business.

> it's just a request for 16 bits of data (on real hardware it's
> typically a bus transaction on a bunch of data lines with some
> control lines indicating transaction width). Now the CPU emulation
> may internally be intending to put that data into its emulated
> register one way round or the other, but that's an internal detail
> of the CPU emulation. (Similarly for stores.)

My statement above meant (sorry if it wasn't clear) that what matters is
that when qemu carries that request around (thus carries those 16-bits
in a u16 variable inside qemu itself, ie, the argument of some
load/store callback), the only attribute of interest is which of the
bytes in that u16 variable is the first in ascending address order.

IE. This is a property of the processor bus, it is *defined* which of
those 2 bytes that form that 16 bit wide "access" is supposed to go at
what address as part of the bus definition (though ARM seems to flip it
around).

Obviously, it should be done according to the host endianness, and thus
one would expect that qemu just "storing" that in memory results in the
two bytes being laid out in the right order.

My point here is that if any conversion at that level is needed, it's
purely somewhere in TCG to ensure that what's in the variable carried
out of TCG is represented according to the intended byte order of the
original access.

That's why the word "endianness" is so confusing. At this point, if
anything, the only endianness that exists is the one of the host CPU :-)

Now when we cross your byte-lanes adjusting bridge in qemu, two things
can happen.

 - The bridge is configured properly and it's a nop
 - The bridge is *not* configured properly, and you basically need to
perform a lane swap on anything crossing that bridge.

I wouldn't call that "endian". I wouldn't model that by saying that
some range of addresses is "LE" or "BE" or "Host Endian" or whatever ...

The best way to represent that in qemu would be to have some kind of
"lane swap" attribute which is set based on the (mis)match of the
CPU and bridge configuration.

> >, and the same on the target device (at which point a concept of
> > significance does apply, but it's a guest driver business to get it
> > right, qemu just need to make sure byte 0 goes to byte 0).
> 
> Similarly, at the target device end there is no concept
> of a "big endian access" -- we make a request for 16
> bits of data at a particular address (via the MemoryRegion
> API) and the device returns 16 bits of data.

>  It's entirely
> possible to design hardware so that byte access to address
> X, halfword access to address X and word access to address
> X all return entirely different data (though it would be
> a bit perverse.) (As an implementation convenience we may
> choose to provide helper infrastructure so you don't have
> to actually implement all of byte/halfword/word access by hand.)

In fact, I think some x86 IO devices did that perverse thing in the
past. It's not actually always possible. On modern busses it tends not
to be actually. Often, busses do *not* have low address bits below their
width but use byte enables instead. Now  of course the device could be
perverse enough to try to use those to "deduce" the address and still
return different values but I doubt anybody does that :-)

However this is academic. The point is that the bus the device is on has
a definition of what byte is expected first in ascending address order,
which should match the way qemu carry the data.

Endianness at the device level thus is purely about the interpretation
of those data at a register level, because qemu is no HW.

> > If a bridge flips things around in a way that breaks the
> > model,
> 
> That breaks what model?

Byte order invariance which is what we care about. If the bridge
preserves that, then it's a nop for qemu.

> > then add some property describing the flipping
> > properties but don't call it "big
> > endian" or "little endian" at the bridge level, that has no meaning,
> > confuses things and introduces breakage like we have seen.
> 
> I'm happy to call the property "byteswap", yes, because
> that's what it does. If you did two of these in a row you'd
> get a no-op.

Right but the bridge you mentioned "byteswap" configuration is based on
the configuration of the processor bus byte order, and if those are in
sync, the bridge should essentially be a nop for all intend and purpose.

If byte order is preserved, then the "value" qemu carries around can go
unmodified, all the way.

> >> (Our other serious endianness problem is that we don't really
> >> do very well at supporting a TCG CPU arbitrarily flipping
> >> endianness -- TARGET_WORDS_BIGENDIAN is a compile time setting
> >> and ideally it should not be.)
> >
> > Our experience is that it actually works fine for almost everything
> > except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and
> > should be).
> 
> I agree that TARGET_WORDS_BIGENDIAN *should* go away, but
> it exists currently. Do you actually implement a CPU which
> does dynamic endianness flipping? Is it at all efficient
> in the config which is the opposite of whatever
> TARGET_WORDS_BIGENDIAN says?

Yes. I haven't measured the speed (in fact I haven't looked at the code
either, others have, but I know it works).

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 July 2013 23:50, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>> Our experience is that it actually works fine for almost everything
>> except virtio :-) ie mostly TARGET_WORDS_BIGENDIAN is irrelevant (and
>> should be).
>
> I agree that TARGET_WORDS_BIGENDIAN *should* go away, but
> it exists currently. Do you actually implement a CPU which
> does dynamic endianness flipping?

TCG already supports bi-endianness for PPC.  Grep for 'le_mode' in
target-ppc/translate.c to see how the TCG instruction stream is affected
by it.

On PPC, le_mode only really affects load/stores and instruction
decoding.  Any shared data structures between the CPU and OS remain big
endian.

So TARGET_WORDS_BIGENDIAN is still accurate even when le_mode=1.  It's
not the semantic equivalent of changing TARGET_WORDS.

Regards,

Anthony Liguori

> Is it at all efficient
> in the config which is the opposite of whatever
> TARGET_WORDS_BIGENDIAN says?
>
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Peter Maydell]

On 15 July 2013 15:01, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 12 July 2013 23:50, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>> I agree that TARGET_WORDS_BIGENDIAN *should* go away, but
>> it exists currently. Do you actually implement a CPU which
>> does dynamic endianness flipping?
>
> TCG already supports bi-endianness for PPC.  Grep for 'le_mode' in
> target-ppc/translate.c to see how the TCG instruction stream is affected
> by it.

Right, that's what I thought. This means that if you're on a
little-endian host and using a PPC chip in le-mode then the
QEMU infrastructure will byteswap the data, and then those
generated bswap* ops will byteswap it again. It would be more
efficient if we supported dynamically having the emulated
CPU specify whether it wanted a BE or LE memory access, and
then byteswapping the correct number of times based on
device/host endianness/CPU setting. (This would also avoid
ugliness like the current ARM BE8 code which should be doing
"BE read for data, LE read for instructions" and instead has
to do "BE read of everything via TARGET_WORDS_BIGENDIAN
and then swap the instruction words again to get back to LE".)
If we ever want to support multiple different CPUs in one
model we'll need this anyway, because one might be BE and
another LE.

> On PPC, le_mode only really affects load/stores and instruction
> decoding.  Any shared data structures between the CPU and OS remain big
> endian.
>
> So TARGET_WORDS_BIGENDIAN is still accurate even when
> le_mode=1.  It's not the semantic equivalent of
> changing TARGET_WORDS.

I'm not sure what you mean by "accurate" here. It's a compile
time switch, when the hardware we're actually emulating does
runtime (dynamic) switching, and when the current runtime
mode doesn't match the compiled-in setting we end up doing
unnecessary double-swap operations.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Mon, 2013-07-15 at 09:01 -0500, Anthony Liguori wrote:
> On PPC, le_mode only really affects load/stores and instruction
> decoding.  Any shared data structures between the CPU and OS remain big
> endian.

Translation:

On ppc with the specific machine type "pseries" which emulates a
paravirtualized environment, the data structures between the guest and
the hypervisor remain big endian :-)

But this is essentially irrelevant to the basic discussion about endian
handling (ie. PCI devices remain LE, etc...).

Point is, TCG can deal with dual endian reasonably easily. The main
issues are around things like virtio with its stupid concept of "guest
endian" (wtf does guest endian means on a dual endian processor ?) but I
think we've whacked Rusty's head plenty enough on that one... and it's
not that hard to work around.

> So TARGET_WORDS_BIGENDIAN is still accurate even when le_mode=1.  It's
> not the semantic equivalent of changing TARGET_WORDS.

The point is that it's unnecessary :-) IE. We should aim to get rid of
the concept of target endianness altogether :-)

Cheers,
Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Benjamin Herrenschmidt]

On Fri, 2013-07-12 at 12:49 -0500, Anthony Liguori wrote:
> 
> > Unless they are completely broken (and those exist, don't get me wrong,
> > though mostly they are a thing of a past long gone), bridges and busses
> > have no effect on endianness.
> 
> That's simply not true.  There are programmable PCI host bridges that
> support byte swapping.  Some allow this to be done on a per-device basis
> too.

Those bridges are utterly broken and the result of very sick HW design
mind. I've encountered that sort of thing before and the only sane way
to use them is to disable that shit.

If a bridge swaps, it breaks byte order of non-register accesses (FIFO
transfers, DMAs, ...) and hell breaks lose.

Unless you confuse with the byte lane swapping that must occur when
bridging a processor to a bus when the processor changes its bus byte
order.

IE. Some processors (generally older, people have grown clues since
then) that can support dual endian operations had the bad habit of
reversing the location of the low-address and high-address bytes on
their bus (or rather didn't properly reverse LSB/MSB and thus requires
bridges to do it). Such processors do require the bridge to swap the
byte lanes when changing endianness. However I've been told that even
ARM doesn't do that any more.

In any case, even if you want to model a piece of crap like that, you
shouldn't do so by calling the bus "big endian" or "little endian" but
something like a byte lane swap attribute, which more precisely
describes what the bridge is doing. This is not endianness.

> > So I'm not sure what you guys are up to, but from what I read, it's
> > wrong, and the fact at this stage is that your broke IO space (and thus
> > virtio and VGA) on powerpc (including pseries).
> 
> I'm not sure what this patch was trying to do but it was certainly
> wrong.

Right :-)

Ben.

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 11/07/2013 14:46, Andreas Färber ha scritto:
> sPAPR has its MemoryRegion marked Little Endian:
> 
> http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b
> 
> Possibly we can now apply Hervé's patches on top to remove that hack again?

I can post a pull request with Herve's patch, if we agree that it's the
right thing.

Paolo

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

On Sat, Jul 13, 2013 at 9:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/07/2013 14:46, Andreas Färber ha scritto:
>> sPAPR has its MemoryRegion marked Little Endian:
>>
>> http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b
>>
>> Possibly we can now apply Hervé's patches on top to remove that hack again?
>
> I can post a pull request with Herve's patch, if we agree that it's the
> right thing.

http://permalink.gmane.org/gmane.comp.emulators.qemu/221950

Here's what's happening:

1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by
the time the handler is called, the value is in host byte order.

2) sPAPR (incorrectly) byte swaps by marking the region as little
endian (data is now garbage)

3) The portio layer (incorrectly) byte swaps because it is marked as
little endian (data is now good)

4) Dispatch happens to VGA device which (incorrectly) byte swaps
because it is marked as little endian (data is now bad)

(2), (3), and (4) are all wrong.  By removing either (2) or (3) we can
"fix" the regression but that's just because two wrongs make a right
in this situation.

We should remove *all* of the LE markings from ISA devices, remove the
portio mark, and the sPAPR mark.  That's the right fix.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Hervé Poussineau]

Anthony Liguori a écrit :
> On Sat, Jul 13, 2013 at 9:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 11/07/2013 14:46, Andreas Färber ha scritto:
>>> sPAPR has its MemoryRegion marked Little Endian:
>>>
>>> http://git.qemu.org/?p=qemu.git;a=blobdiff;f=hw/spapr_pci.c;h=a08ed11166595bdc493065beb64d4ce5b7b0dded;hp=c2c3079d21d5be2647faf85a8c608ac995d2ca62;hb=a3cfa18eb075c7ef78358ca1956fe7b01caa1724;hpb=286d52ebfc0d0d53c2a878e454292fea14bad41b
>>>
>>> Possibly we can now apply Hervé's patches on top to remove that hack again?
>> I can post a pull request with Herve's patch, if we agree that it's the
>> right thing.
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/221950
> 
> Here's what's happening:
> 
> 1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by
> the time the handler is called, the value is in host byte order.
> 
> 2) sPAPR (incorrectly) byte swaps by marking the region as little
> endian (data is now garbage)
> 
> 3) The portio layer (incorrectly) byte swaps because it is marked as
> little endian (data is now good)
> 
> 4) Dispatch happens to VGA device which (incorrectly) byte swaps
> because it is marked as little endian (data is now bad)
> 
> (2), (3), and (4) are all wrong.  By removing either (2) or (3) we can
> "fix" the regression but that's just because two wrongs make a right
> in this situation.
> 
> We should remove *all* of the LE markings from ISA devices, remove the
> portio mark, and the sPAPR mark.  That's the right fix.

OK for that if that fixes sPAPR.

Hervé

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 13/07/2013 17:22, Anthony Liguori ha scritto:
> 1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by
> the time the handler is called, the value is in host byte order.
> 
> 2) sPAPR (incorrectly) byte swaps by marking the region as little
> endian (data is now garbage)
> 
> 3) The portio layer (incorrectly) byte swaps because it is marked as
> little endian (data is now good)
> 
> 4) Dispatch happens to VGA device which (incorrectly) byte swaps
> because it is marked as little endian (data is now bad)
> 
> (2), (3), and (4) are all wrong.  By removing either (2) or (3) we can
> "fix" the regression but that's just because two wrongs make a right
> in this situation.
> 
> We should remove *all* of the LE markings from ISA devices, remove the
> portio mark, and the sPAPR mark.  That's the right fix.

So the bug here is that we have multiple levels of dispatch.  Byte
swapping in the dispatch level only works as long as every dispatch is
merged, which is not the case.

However, I do suspect that you have broken PREP again, because PREP has
1/3/4 but not 2.  Removing (2) IIUC amounts to re-applying commit
a178274efabcbbc5d44805b51def874e47051325, and I think that's a better fix.

Also, what devices exactly would have a non-native byte order?!?  I'm
confused...

Paolo

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/07/2013 17:22, Anthony Liguori ha scritto:
>> 1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by
>> the time the handler is called, the value is in host byte order.
>> 
>> 2) sPAPR (incorrectly) byte swaps by marking the region as little
>> endian (data is now garbage)
>> 
>> 3) The portio layer (incorrectly) byte swaps because it is marked as
>> little endian (data is now good)
>> 
>> 4) Dispatch happens to VGA device which (incorrectly) byte swaps
>> because it is marked as little endian (data is now bad)
>> 
>> (2), (3), and (4) are all wrong.  By removing either (2) or (3) we can
>> "fix" the regression but that's just because two wrongs make a right
>> in this situation.
>> 
>> We should remove *all* of the LE markings from ISA devices, remove the
>> portio mark, and the sPAPR mark.  That's the right fix.
>
> So the bug here is that we have multiple levels of dispatch.  Byte
> swapping in the dispatch level only works as long as every dispatch is
> merged, which is not the case.

It's not clear to me if "endianness" makes sense as a concept in the
memory API.  If a bus wants to byte swap, it would have to redispatch
anyway.

> However, I do suspect that you have broken PREP again, because PREP has
> 1/3/4 but not 2.

"Broken" is a relative term...  Not all ISA devices do (4)--see the
various audio devices.

Jan's original patch did code motion and added the LE flag to portio.
My patch simply reverted the added logic to the code motion so if it
broke PREP, PREP has been broken for quite some time.

> Removing (2) IIUC amounts to re-applying commit
> a178274efabcbbc5d44805b51def874e47051325, and I think that's a better
> fix.

It's a bigger change, but I think we should just remove
MemoryRegionOps::endianness altogether.

> Also, what devices exactly would have a non-native byte order?!?  I'm
> confused...

MMIO/PIO requests don't have a byte order.  It's literally 64 or 32 data
pins that are numbered D0..D31 whereas D0 is the LSB.  It doesn't matter
how the pins are arranged.

It's possible for busses to "byte lane swap" the data lines such that
D0..D7 is rerouted to D23..D31, etc. in order to deal with poorly
written drivers but as benh so colorfully put, this introduces more
problems than it solves.

Unfortunately, the existance of MemoryRegionOps::endianness makes this
difficult to untangle because we have a careful combination of "two
wrongs making a right".  In all fairness, this problem predates the
memory API.  The memory API just carried forward the brokenness.

Device originated DMA is a different story.  This has to happen with a
specific endianness but that is orthogonal to the memory API.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Peter Maydell]

On 14 July 2013 14:05, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> Also, what devices exactly would have a non-native byte order?!?  I'm
>> confused...
>
> MMIO/PIO requests don't have a byte order.  It's literally 64 or 32 data
> pins that are numbered D0..D31 whereas D0 is the LSB.  It doesn't matter
> how the pins are arranged.

Devices themselves do have a byte order, though, right? Specifically,
if you do a 32 bit read of address 0 on a device and an 8 bit read,
then you can distinguish a BE device from an LE one.
(Most notably, RAM in QEMU is always host-endian...)
Devices which only allow 32 bit reads and abort any others wouldn't
have an endianness though.

(I need to sit down and think about this all and draw diagrams
and look at what we currently do, though. BE guests on LE hosts
with and without KVM look particularly thorny.)

-- PMM

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

On Sun, Jul 14, 2013 at 9:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 July 2013 14:05, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>> Also, what devices exactly would have a non-native byte order?!?  I'm
>>> confused...
>>
>> MMIO/PIO requests don't have a byte order.  It's literally 64 or 32 data
>> pins that are numbered D0..D31 whereas D0 is the LSB.  It doesn't matter
>> how the pins are arranged.
>
> Devices themselves do have a byte order, though, right? Specifically,
> if you do a 32 bit read of address 0 on a device and an 8 bit read,

It depends on the bus and device.  Busses don't necessary pass the I/O
size down to the device like that.  If it does, the device may do any
number of things (including ignoring the request entirely.

What's most common AFAIK is that the access is treated as a word
access and then truncated.  IOW, the device sees the 32-bit word read
but somewhere along the way, the top 24 bits are discarded.

The real interesting question is what happens when you do a byte
access at address 1.  I think most devices simply don't allow that.

> then you can distinguish a BE device from an LE one.
> (Most notably, RAM in QEMU is always host-endian...)
> Devices which only allow 32 bit reads and abort any others wouldn't
> have an endianness though.

My guess is that if you do this with a PCI device we have marked as
LE, you'll get a truncated 32-bit read which will make it appear LE.
But that doesn't mean it's LE.

> (I need to sit down and think about this all and draw diagrams
> and look at what we currently do, though. BE guests on LE hosts
> with and without KVM look particularly thorny.)

I took a first pass at cleaning this up and broke PPC so I'm
investigating it further.  There may be another layer of silliness
hidden somewhere too.

Regards,

Anthony Liguori

> -- PMM

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Peter Maydell]

On 14 July 2013 16:18, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On Sun, Jul 14, 2013 at 9:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Devices themselves do have a byte order, though, right? Specifically,
>> if you do a 32 bit read of address 0 on a device and an 8 bit read,
>
> It depends on the bus and device.  Busses don't necessary pass the I/O
> size down to the device like that.  If it does, the device may do any
> number of things (including ignoring the request entirely.

Agreed.

> What's most common AFAIK is that the access is treated as a word
> access and then truncated.  IOW, the device sees the 32-bit word read
> but somewhere along the way, the top 24 bits are discarded.
>
> The real interesting question is what happens when you do a byte
> access at address 1.  I think most devices simply don't allow that.

Mmm, and where it is permitted it's going to be device dependent
(for instance the ARM GIC has most registers be 32-bit access only,
with a few which allow 8-bit access; and the GIC spec states that
those byte accesses use a little endian memory order model).

It looks like most of the ARM PLxxx devices on the APB bus simply
don't wire up address lines 0 and 1, so an access to address 1
will behave like one to address 0. (Of course we don't model this.)

-- PMM

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On 2013-07-14 17:18, Anthony Liguori wrote:
> On Sun, Jul 14, 2013 at 9:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 14 July 2013 14:05, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>>> Also, what devices exactly would have a non-native byte order?!?  I'm
>>>> confused...
>>>
>>> MMIO/PIO requests don't have a byte order.  It's literally 64 or 32 data
>>> pins that are numbered D0..D31 whereas D0 is the LSB.  It doesn't matter
>>> how the pins are arranged.
>>
>> Devices themselves do have a byte order, though, right? Specifically,
>> if you do a 32 bit read of address 0 on a device and an 8 bit read,
> 
> It depends on the bus and device.  Busses don't necessary pass the I/O
> size down to the device like that.  If it does, the device may do any
> number of things (including ignoring the request entirely.
> 
> What's most common AFAIK is that the access is treated as a word
> access and then truncated.  IOW, the device sees the 32-bit word read
> but somewhere along the way, the top 24 bits are discarded.
> 
> The real interesting question is what happens when you do a byte
> access at address 1.  I think most devices simply don't allow that.
> 
>> then you can distinguish a BE device from an LE one.
>> (Most notably, RAM in QEMU is always host-endian...)
>> Devices which only allow 32 bit reads and abort any others wouldn't
>> have an endianness though.
> 
> My guess is that if you do this with a PCI device we have marked as
> LE, you'll get a truncated 32-bit read which will make it appear LE.
> But that doesn't mean it's LE.
> 
>> (I need to sit down and think about this all and draw diagrams
>> and look at what we currently do, though. BE guests on LE hosts
>> with and without KVM look particularly thorny.)
> 
> I took a first pass at cleaning this up and broke PPC so I'm
> investigating it further.  There may be another layer of silliness
> hidden somewhere too.

Sorry for sending out invitations and then being late to this party -
vacation. What is the status now? Do we have a short-term plan to avoid
the regression or is this better solved by cleaning up the whole
endianess thing? Is anyone actively on it, or should I take a drink, sit
down and join the discussion?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 16/07/2013 09:18, Jan Kiszka ha scritto:
> Sorry for sending out invitations and then being late to this party
> - vacation. What is the status now? Do we have a short-term plan to
> avoid the regression or is this better solved by cleaning up the
> whole endianess thing? Is anyone actively on it, or should I take a
> drink, sit down and join the discussion?

Basically, we need testing.  The current state of the tree is before
Herve's patch, which means PREP is (should be) broken.

Alexey posted a patch that reintroduces the DEVICE_LITTLE_ENDIAN and
removes the cpu_{in,out}{b,w,l} indirection.

http://permalink.gmane.org/gmane.comp.emulators.qemu/222345

We need to test platforms that used a cpu_{in,out}{b,w,l} indirection
(MIPS, PPC, SPARC) with and without Alexey's patch.

The other occurrences of indirections are:

- - hw/isa/i82378.c: This is PREP.  Again, removing the indirection
should be tested by Herve or Andreas on top of Alexey's patch.

- - hw/isa/isa_mmio: bamboo, g3beige and mac99 could be tested by Alex.
 I don't know about MIPS.  If anything is broken, the solution is to
replace isa_mmio_{setup,init} with an alias to get_system_io().  This
stops using isa_mmio altogether, so it can be done only on those
platforms where it's needed.

- - hw/pci-host/apb.c: this is SPARC.  Perhaps Mark Cave-Ayland can test
it and see if it is broken---again with and without Alexey's patch.
There is a small difference.  This file uses DEVICE_NATIVE_ENDIAN and
does the byte swap itself in pci_apb_io{read,write}{b,w,l}.

There is also Alpha.  It doesn't matter because it's little endian,
but anyway rth is removing the indirection.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR5PcwAAoJEBvWZb6bTYbyAOMP/1y19WnUTxcA3+kIhQE1HU2R
ildgJ2VV3kRaY4n1HeBuXOXEQzNxwppC0c0Fq/fQpYWhUxB7Y7UTtDVq9S997EUm
SoU02UlD2+xnmgOSSTTrYNY20WBRt5/s3xkMjiQSPKItBGw1og7+72m/ydzjSPAh
DSBG/mPs12zI3WcTzaydirGVX874pV2N76TSyP+T4DMkipGlgGs1JSOP+Ib/fAWw
Hk/mIu+in7qQjv7DZ/0fJuk8cvbgafCGsKppQ5yYu2YKqkmsLhR6xPNeduWK2GMv
eRLOoobSOlQxUL4konlvI74Gw/+bFtpjWcjybDfAKcVBkm8H/zpg8SIgbmlbJzoR
Gx2bQJvj1PN9pJ/TZRo+1bcw4W7JXKN94s8CTWCL8eGDELB7BgNZFXrxnyfPrY5f
8ega3uZVtZ6SmtPMO7g5arQDfqdvplH/oQozK0muxXgfBzWwvF0Y/qDNRHY18yXC
ZdPdEI2FmslOp8ZzhFW+VApfJXDADlC9xOV9rIJ0jqOewBdJlKrEkqJ8oedYGsPM
am3fmRL3N/KJ1y59hkNEpGhF6yHS7eHJBVNYp0YakthKOKhZTcBfBryFs1Yo5zwt
zvozbSEM0wXnL2ajSwWmU5drH59GdtmYdsIyOm52Etobiu002SZ1VeuCEQOORTTL
qtqyWS4kFP/PTTm8KwV4
=RIcU
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Hervé Poussineau]

Paolo Bonzini a écrit :
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Il 16/07/2013 09:18, Jan Kiszka ha scritto:
>> Sorry for sending out invitations and then being late to this party
>> - vacation. What is the status now? Do we have a short-term plan to
>> avoid the regression or is this better solved by cleaning up the
>> whole endianess thing? Is anyone actively on it, or should I take a
>> drink, sit down and join the discussion?
> 
> Basically, we need testing.  The current state of the tree is before
> Herve's patch, which means PREP is (should be) broken.
> 
> Alexey posted a patch that reintroduces the DEVICE_LITTLE_ENDIAN and
> removes the cpu_{in,out}{b,w,l} indirection.
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/222345
> 
> We need to test platforms that used a cpu_{in,out}{b,w,l} indirection
> (MIPS, PPC, SPARC) with and without Alexey's patch.
> 
> The other occurrences of indirections are:
> 
> - - hw/isa/i82378.c: This is PREP.  Again, removing the indirection
> should be tested by Herve or Andreas on top of Alexey's patch.

For i82378, I have a big patch for it, which rewrites large parts of the 
emulation. Moreover, as i82378 is only used in PReP machine, this 
indirection can be ignored for now.

> - - hw/isa/isa_mmio: bamboo, g3beige and mac99 could be tested by Alex.
>  I don't know about MIPS.  If anything is broken, the solution is to
> replace isa_mmio_{setup,init} with an alias to get_system_io().  This
> stops using isa_mmio altogether, so it can be done only on those
> platforms where it's needed.

For MIPS Jazz, it is currently broken in SCSI emulation when installing 
Windows NT 4.0/MIPS. However, it seems quite a general problem on 
Windows NT 4, as Neozeed also reported the same freeze when running 
NT4/x86 on http://virtuallyfun.superglobalmegacorp.com/?p=3065 .

> - - hw/pci-host/apb.c: this is SPARC.  Perhaps Mark Cave-Ayland can test
> it and see if it is broken---again with and without Alexey's patch.
> There is a small difference.  This file uses DEVICE_NATIVE_ENDIAN and
> does the byte swap itself in pci_apb_io{read,write}{b,w,l}.
> 
> There is also Alpha.  It doesn't matter because it's little endian,
> but anyway rth is removing the indirection.

Hervé

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 16/07/2013 18:59, Hervé Poussineau ha scritto:
> Paolo Bonzini a écrit :
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 16/07/2013 09:18, Jan Kiszka ha scritto:
>>> Sorry for sending out invitations and then being late to this party
>>> - vacation. What is the status now? Do we have a short-term plan to
>>> avoid the regression or is this better solved by cleaning up the
>>> whole endianess thing? Is anyone actively on it, or should I take a
>>> drink, sit down and join the discussion?
>>
>> Basically, we need testing.  The current state of the tree is before
>> Herve's patch, which means PREP is (should be) broken.
>>
>> Alexey posted a patch that reintroduces the DEVICE_LITTLE_ENDIAN and
>> removes the cpu_{in,out}{b,w,l} indirection.
>>
>> http://permalink.gmane.org/gmane.comp.emulators.qemu/222345
>>
>> We need to test platforms that used a cpu_{in,out}{b,w,l} indirection
>> (MIPS, PPC, SPARC) with and without Alexey's patch.
>>
>> The other occurrences of indirections are:
>>
>> - - hw/isa/i82378.c: This is PREP.  Again, removing the indirection
>> should be tested by Herve or Andreas on top of Alexey's patch.
> 
> For i82378, I have a big patch for it, which rewrites large parts of the
> emulation. Moreover, as i82378 is only used in PReP machine, this
> indirection can be ignored for now.

As you prefer.

>> - - hw/isa/isa_mmio: bamboo, g3beige and mac99 could be tested by Alex.
>>  I don't know about MIPS.  If anything is broken, the solution is to
>> replace isa_mmio_{setup,init} with an alias to get_system_io().  This
>> stops using isa_mmio altogether, so it can be done only on those
>> platforms where it's needed.
> 
> For MIPS Jazz, it is currently broken in SCSI emulation when installing
> Windows NT 4.0/MIPS. However, it seems quite a general problem on
> Windows NT 4, as Neozeed also reported the same freeze when running
> NT4/x86 on http://virtuallyfun.superglobalmegacorp.com/?p=3065 .

But this bug could even prevent VGA initialization, so it should happen
much earlier than that.  So the SCSI breakage would not block testing.

Paolo

>> - - hw/pci-host/apb.c: this is SPARC.  Perhaps Mark Cave-Ayland can test
>> it and see if it is broken---again with and without Alexey's patch.
>> There is a small difference.  This file uses DEVICE_NATIVE_ENDIAN and
>> does the byte swap itself in pci_apb_io{read,write}{b,w,l}.
>>
>> There is also Alpha.  It doesn't matter because it's little endian,
>> but anyway rth is removing the indirection.
> 
> Hervé
> 
> 

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Alexander Graf <agraf@suse.de> writes:

> On 11.07.2013, at 14:29, Alexander Graf wrote:
>
>> This patch breaks VGA on PPC as it is in master today.
>
> If I don't mark portio as little endian it works as expected. There's
> probably someone swapping things twice.

This is the correct fix.  Can you please send a patch?

I/O dispatch functions take a native endian argument and when
cpu_{in,out][bwl] is called it's passed a native endian argument.  There
is no need to ever byte swap.

The ISA bus is going to have data pins numbered D0..D15.  There is no
concept of endianness.  D0 is the LSB.  How the CPU stores bytes in
memory is orthogonal to what is happening on the bus.

Regards,

Anthony Liguori

>
>
> Alex

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexander Graf]

Am 12.07.2013 um 14:56 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 11.07.2013, at 14:29, Alexander Graf wrote:
>> 
>>> This patch breaks VGA on PPC as it is in master today.
>> 
>> If I don't mark portio as little endian it works as expected. There's
>> probably someone swapping things twice.
> 
> This is the correct fix.  Can you please send a patch?

I only have my phone with me right now. The problem is that we need to

  1) revert the ioport access ops back to native
  2) fix the le assumption for the w-on-b emulation case

Alex

> 
> I/O dispatch functions take a native endian argument and when
> cpu_{in,out][bwl] is called it's passed a native endian argument.  There
> is no need to ever byte swap.
> 
> The ISA bus is going to have data pins numbered D0..D15.  There is no
> concept of endianness.  D0 is the LSB.  How the CPU stores bytes in
> memory is orthogonal to what is happening on the bus.
> 
> Regards,
> 
> Anthony Liguori
> 
>> 
>> 
>> Alex

[Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

Hi!

This patch also breaks virtio on powerpc. I thought it was fixed
(reverted?) in the master branch from qemu.org but it is still there. What
did I miss?

The problem comes from "-drive file=virtimg/fc18guest,if=virtio".

My full command line is:
./qemu-system-ppc64 -L "qemu-ppc64-bios/" \
 -trace "events=qemu_trace_events" \
 -nodefaults \
 -chardev "stdio,id=char1,signal=off,mux=on" \
 -device "spapr-vty,id=ser1,chardev=char1" \
 -mon "chardev=char1,mode=readline,id=mon1" \
 -drive file=virtimg/fc18guest,if=virtio \
 -m "1024" \
 -machine "pseries" \
 -nographic \
 -vga "none" \
 -enable-kvm


This is what happens:

SLOF **********************************************************************
QEMU Starting
 Build Date = Apr 30 2013 14:04:00
 FW Version = git-8cfdfc43f4c4c8c8
 Press "s" to enter Open Firmware.

Populating /vdevice methods
Populating /vdevice/nvram@71000000

NVRAM: size=65536, fetch=200E, store=200F
Populating /vdevice/vty@71000001
Populating /pci@800000020000000
 Adapters on 0800000020000000
                     00 0000 (D) : 1af4 1001    virtio [ block ]
No NVRAM common partition, re-initializing...
claim failed!
Using default console: /vdevice/vty@71000001

  Welcome to Open Firmware

  Copyright (c) 2004, 2011 IBM Corporation All rights reserved.
  This program and the accompanying materials are made available
  under the terms of the BSD License available at
  http://www.opensource.org/licenses/bsd-license.php


Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
virtioblk_read: Access beyond end of device!
[many of those]



On 07/11/2013 10:29 PM, Alexander Graf wrote:
> 
> On 24.06.2013, at 08:07, Jan Kiszka wrote:
> 
>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>> Jan Kiszka a écrit :
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>> need to deal with old portio interface users. But we can overcome it
>>>> without converting all portio users by embedding the required base
>>>> address of a MemoryRegionPortio access into that data structure. That
>>>> removes the need to have the additional MemoryRegionIORange structure
>>>> in the loop on every access.
>>>>
>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>> for portio memory regions when registering them with the memory core.
>>>> This removes the need for the old_portio field.
>>>>
>>>> We can drop the additional aliasing of ioport regions and also the
>>>> special address space listener. cpu_in and cpu_out now simply call
>>>> address_space_read/write. And we can concentrate portio handling in a
>>>> single source file.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>
>>> ...
>>>
>>>> +
>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>> +                         unsigned size)
>>>> +{
>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>> true);
>>>> +
>>>> +    if (mrp) {
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>> +    } else if (size == 2) {
>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>> +        assert(mrp);
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>> 8);
>>>> +    }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps portio_ops = {
>>>> +    .read = portio_read,
>>>> +    .write = portio_write,
>>>> +    .valid.unaligned = true,
>>>> +    .impl.unaligned = true,
>>>> +};
>>>> +
>>>
>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>> In portio_write above, you clearly assume that data is in LE format.
>>
>> Anything behind PIO is little endian, of course. Will add this.
> 
> This patch breaks VGA on PPC as it is in master today.
> 
> 
> Alex
> 
>>
>>>
>>> This fixes PPC PReP emulation, which would otherwise be broken with this
>>> patchset.
>>
>> Thanks,
>> Jan
>>
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:
> Hi!
> 
> This patch also breaks virtio on powerpc. I thought it was fixed
> (reverted?) in the master branch from qemu.org but it is still there. What
> did I miss?

It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.

Let me check if I can reproduce this, it looks like a endianness
problems reading virtio-blk config space.

Paolo

> Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> virtioblk_read: Access beyond end of device!
> [many of those]
> 
> 
> 
> On 07/11/2013 10:29 PM, Alexander Graf wrote:
>>
>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>
>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>> Jan Kiszka a écrit :
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>> need to deal with old portio interface users. But we can overcome it
>>>>> without converting all portio users by embedding the required base
>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>> in the loop on every access.
>>>>>
>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>> for portio memory regions when registering them with the memory core.
>>>>> This removes the need for the old_portio field.
>>>>>
>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>> single source file.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>> +                         unsigned size)
>>>>> +{
>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>> true);
>>>>> +
>>>>> +    if (mrp) {
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>> +    } else if (size == 2) {
>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>> +        assert(mrp);
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>> 8);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static const MemoryRegionOps portio_ops = {
>>>>> +    .read = portio_read,
>>>>> +    .write = portio_write,
>>>>> +    .valid.unaligned = true,
>>>>> +    .impl.unaligned = true,
>>>>> +};
>>>>> +
>>>>
>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>> In portio_write above, you clearly assume that data is in LE format.
>>>
>>> Anything behind PIO is little endian, of course. Will add this.
>>
>> This patch breaks VGA on PPC as it is in master today.
>>
>>
>> Alex
>>
>>>
>>>>
>>>> This fixes PPC PReP emulation, which would otherwise be broken with this
>>>> patchset.
>>>
>>> Thanks,
>>> Jan
>>>
>>>
>>
>>
> 
> 

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

Ok. So.

What broke is...
I could try explaining but backtraces are lot better :)

Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
we had a workaround in spapr_io_ops), now it works so double swap happens
and everything gets broken.

If we talk about VGA (in powerpc, it is all about powerpc), I guess
memory_region_iorange_write() will go through mr->ops->old_portio branch
and won't do any byte swapping (so spapr_io_ops will do the job), so we are
fine here. I do not understand yet why it works on mac99 though, too late
here :)

h_logical_store is a hypercall for system firmware to do cache inhibited
read/write.


This is with the patch applied (git checkout  b40acf9):


#0  virtqueue_init (vq=0x11014ac0) at
/home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1  0x0000000010371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
addr=0xd0fb0000000)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
val=0xd0fb0000)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
addr=0x8, val=0xd0fb0000, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4  0x000000001037e220 in memory_region_write_accessor (opaque=0x11019c78,
addr=0x8, value=0x1fffff0edc00,
    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037e36c in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edc00, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#6  0x0000000010380b5c in memory_region_dispatch_write (mr=0x11019c78,
addr=0x8, data=0xd0fb0000, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#7  0x0000000010383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
val=0xfbd0, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#8  0x00000000102e2fdc in address_space_rw (as=0x10ef4350
<address_space_io>, addr=0x48,
    buf=0x1fffff0edde0 "", len=0x4, is_write=0x1) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1918
#9  0x00000000102e33c8 in address_space_write (as=0x10ef4350
<address_space_io>, addr=0x48,
    buf=0x1fffff0edde0 "", len=0x4) at
/home/alexey/pcipassthru/qemu-impreza/exec.c:1969
#10 0x0000000010375754 in cpu_outl (addr=0x48, val=0xfbd0)
    at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
#11 0x0000000010358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
data=0xfbd0, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#12 0x000000001037e220 in memory_region_write_accessor (opaque=0x110191f8,
addr=0x48, value=0x1fffff0ee060,
    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#13 0x000000001037e36c in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#14 0x0000000010380b5c in memory_region_dispatch_write (mr=0x110191f8,
addr=0x48, data=0xfbd0, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
#15 0x0000000010383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
val=0xd0fb0000, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
#16 0x00000000102e47ac in stl_phys_internal (addr=0x10080000048,
val=0xd0fb0000, endian=
    DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
#17 0x00000000102e48a8 in stl_phys (addr=0x10080000048, val=0xd0fb0000)
    at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
#18 0x0000000010354f1c in h_logical_store (cpu=0x1fffff0f0010,
spapr=0x10fe9510, opcode=0x40,
    args=0x1ffffffd0030) at
/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570



This is without this patch (i.e. git checkout  b40acf9^ ):

#0  virtqueue_init (vq=0x11014ac0) at
/home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
#1  0x00000000103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
addr=0xffe2000)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
#2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
val=0xffe2)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
#3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
addr=0x8, val=0xffe2, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
#4  0x000000001037dca8 in memory_region_write_accessor (opaque=0x11019c78,
addr=0x8, value=0x1fffff0edca8,
    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#5  0x000000001037ddf4 in access_with_adjusted_size (addr=0x8,
value=0x1fffff0edca8, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x11019c78)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#6  0x000000001037e474 in memory_region_iorange_write
(iorange=0x1ffff0005430, offset=0x8, width=0x4,
    data=0xffe2) at /home/alexey/pcipassthru/qemu-impreza/memory.c:475
#7  0x00000000103750d4 in ioport_writel_thunk (opaque=0x1ffff0005430,
addr=0x48, data=0xffe2)
    at /home/alexey/pcipassthru/qemu-impreza/ioport.c:226
#8  0x0000000010374728 in ioport_write (index=0x2, address=0x48, data=0xffe2)
    at /home/alexey/pcipassthru/qemu-impreza/ioport.c:83
#9  0x0000000010375688 in cpu_outl (addr=0x48, val=0xffe2)
    at /home/alexey/pcipassthru/qemu-impreza/ioport.c:296
#10 0x00000000103583fc in spapr_io_write (opaque=0x11016a00, addr=0x48,
data=0xffe2, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
#11 0x000000001037dca8 in memory_region_write_accessor (opaque=0x110191f8,
addr=0x48, value=0x1fffff0ee060,
    size=0x4, shift=0x0, mask=0xffffffff) at
/home/alexey/pcipassthru/qemu-impreza/memory.c:364
#12 0x000000001037ddf4 in access_with_adjusted_size (addr=0x48,
value=0x1fffff0ee060, size=0x4,
    access_size_min=0x1, access_size_max=0x4, access=
    @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x110191f8)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
#13 0x0000000010380c90 in memory_region_dispatch_write (mr=0x110191f8,
addr=0x48, data=0xffe2, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:993
#14 0x00000000103840d8 in io_mem_write (mr=0x110191f8, addr=0x48,
val=0xe2ff0000, size=0x4)
    at /home/alexey/pcipassthru/qemu-impreza/memory.c:1696
#15 0x00000000102e4968 in stl_phys_internal (addr=0x10080000048,
val=0xe2ff0000, endian=
    DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2447
#16 0x00000000102e4a64 in stl_phys (addr=0x10080000048, val=0xe2ff0000)
    at /home/alexey/pcipassthru/qemu-impreza/exec.c:2469
#17 0x00000000103550d8 in h_logical_store (cpu=0x1fffff0f0010,
spapr=0x10fe9510, opcode=0x40,
    args=0x1ffffffd0030) at
/home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
#18 0x0000000010355698 in spapr_hypercall (cpu=0x1fffff0f0010, opcode=0x40,
args=0x1ffffffd0030)
    at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:689






On 07/19/2013 10:49 PM, Paolo Bonzini wrote:
> Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:
>> Hi!
>>
>> This patch also breaks virtio on powerpc. I thought it was fixed
>> (reverted?) in the master branch from qemu.org but it is still there. What
>> did I miss?
> 
> It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.
> 
> Let me check if I can reproduce this, it looks like a endianness
> problems reading virtio-blk config space.
> 
> Paolo
> 
>> Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> virtioblk_read: Access beyond end of device!
>> [many of those]
>>
>>
>>
>> On 07/11/2013 10:29 PM, Alexander Graf wrote:
>>>
>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>
>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>> Jan Kiszka a écrit :
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>> without converting all portio users by embedding the required base
>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>> in the loop on every access.
>>>>>>
>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>> for portio memory regions when registering them with the memory core.
>>>>>> This removes the need for the old_portio field.
>>>>>>
>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>> single source file.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>
>>>>> ...
>>>>>
>>>>>> +
>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>> +                         unsigned size)
>>>>>> +{
>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>> true);
>>>>>> +
>>>>>> +    if (mrp) {
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>> +    } else if (size == 2) {
>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>> +        assert(mrp);
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>> 8);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>> +    .read = portio_read,
>>>>>> +    .write = portio_write,
>>>>>> +    .valid.unaligned = true,
>>>>>> +    .impl.unaligned = true,
>>>>>> +};
>>>>>> +
>>>>>
>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>
>>>> Anything behind PIO is little endian, of course. Will add this.
>>>
>>> This patch breaks VGA on PPC as it is in master today.
>>>
>>>
>>> Alex
>>>
>>>>
>>>>>
>>>>> This fixes PPC PReP emulation, which would otherwise be broken with this
>>>>> patchset.
>>>>
>>>> Thanks,
>>>> Jan
>>>>
>>>>
>>>
>>>
>>
>>
> 


-- 
Alexey

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
> Ok. So.
> 
> What broke is...
> I could try explaining but backtraces are lot better :)
> 
> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
> we had a workaround in spapr_io_ops), now it works so double swap happens
> and everything gets broken.
> 
> If we talk about VGA (in powerpc, it is all about powerpc), I guess
> memory_region_iorange_write() will go through mr->ops->old_portio branch
> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
> fine here. I do not understand yet why it works on mac99 though, too late
> here :)


I understood. VGA does not work for mac99 either with this command line:
./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
So it works for pseries only because of parity bug in spapr_io_ops.

So the right fix is to get rid of spapr_io_ops and every other hack like
that and to add byte swapping to every "if (mr->ops->old_portio)" branch
(should fix VGA and all other old_portio users). Current byte swapping in
memory regions seems to be right.

I would try fixing it but since all my patches were terrible shit so far, I
won't risk :)



> h_logical_store is a hypercall for system firmware to do cache inhibited
> read/write.
> 
> 
> This is with the patch applied (git checkout  b40acf9):
> 
> 
> #0  virtqueue_init (vq=0x11014ac0) at
> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
> #1  0x0000000010371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
> addr=0xd0fb0000000)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
> #2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
> val=0xd0fb0000)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
> #3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
> addr=0x8, val=0xd0fb0000, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
> #4  0x000000001037e220 in memory_region_write_accessor (opaque=0x11019c78,
> addr=0x8, value=0x1fffff0edc00,
>     size=0x4, shift=0x0, mask=0xffffffff) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #5  0x000000001037e36c in access_with_adjusted_size (addr=0x8,
> value=0x1fffff0edc00, size=0x4,
>     access_size_min=0x1, access_size_max=0x4, access=
>     @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x11019c78)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #6  0x0000000010380b5c in memory_region_dispatch_write (mr=0x11019c78,
> addr=0x8, data=0xd0fb0000, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
> #7  0x0000000010383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
> val=0xfbd0, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
> #8  0x00000000102e2fdc in address_space_rw (as=0x10ef4350
> <address_space_io>, addr=0x48,
>     buf=0x1fffff0edde0 "", len=0x4, is_write=0x1) at
> /home/alexey/pcipassthru/qemu-impreza/exec.c:1918
> #9  0x00000000102e33c8 in address_space_write (as=0x10ef4350
> <address_space_io>, addr=0x48,
>     buf=0x1fffff0edde0 "", len=0x4) at
> /home/alexey/pcipassthru/qemu-impreza/exec.c:1969
> #10 0x0000000010375754 in cpu_outl (addr=0x48, val=0xfbd0)
>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
> #11 0x0000000010358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
> data=0xfbd0, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
> #12 0x000000001037e220 in memory_region_write_accessor (opaque=0x110191f8,
> addr=0x48, value=0x1fffff0ee060,
>     size=0x4, shift=0x0, mask=0xffffffff) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #13 0x000000001037e36c in access_with_adjusted_size (addr=0x48,
> value=0x1fffff0ee060, size=0x4,
>     access_size_min=0x1, access_size_max=0x4, access=
>     @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x110191f8)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #14 0x0000000010380b5c in memory_region_dispatch_write (mr=0x110191f8,
> addr=0x48, data=0xfbd0, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
> #15 0x0000000010383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
> val=0xd0fb0000, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
> #16 0x00000000102e47ac in stl_phys_internal (addr=0x10080000048,
> val=0xd0fb0000, endian=
>     DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
> #17 0x00000000102e48a8 in stl_phys (addr=0x10080000048, val=0xd0fb0000)
>     at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
> #18 0x0000000010354f1c in h_logical_store (cpu=0x1fffff0f0010,
> spapr=0x10fe9510, opcode=0x40,
>     args=0x1ffffffd0030) at
> /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
> 
> 
> 
> This is without this patch (i.e. git checkout  b40acf9^ ):
> 
> #0  virtqueue_init (vq=0x11014ac0) at
> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
> #1  0x00000000103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
> addr=0xffe2000)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
> #2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
> val=0xffe2)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
> #3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
> addr=0x8, val=0xffe2, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
> #4  0x000000001037dca8 in memory_region_write_accessor (opaque=0x11019c78,
> addr=0x8, value=0x1fffff0edca8,
>     size=0x4, shift=0x0, mask=0xffffffff) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #5  0x000000001037ddf4 in access_with_adjusted_size (addr=0x8,
> value=0x1fffff0edca8, size=0x4,
>     access_size_min=0x1, access_size_max=0x4, access=
>     @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x11019c78)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #6  0x000000001037e474 in memory_region_iorange_write
> (iorange=0x1ffff0005430, offset=0x8, width=0x4,
>     data=0xffe2) at /home/alexey/pcipassthru/qemu-impreza/memory.c:475
> #7  0x00000000103750d4 in ioport_writel_thunk (opaque=0x1ffff0005430,
> addr=0x48, data=0xffe2)
>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:226
> #8  0x0000000010374728 in ioport_write (index=0x2, address=0x48, data=0xffe2)
>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:83
> #9  0x0000000010375688 in cpu_outl (addr=0x48, val=0xffe2)
>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:296
> #10 0x00000000103583fc in spapr_io_write (opaque=0x11016a00, addr=0x48,
> data=0xffe2, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
> #11 0x000000001037dca8 in memory_region_write_accessor (opaque=0x110191f8,
> addr=0x48, value=0x1fffff0ee060,
>     size=0x4, shift=0x0, mask=0xffffffff) at
> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
> #12 0x000000001037ddf4 in access_with_adjusted_size (addr=0x48,
> value=0x1fffff0ee060, size=0x4,
>     access_size_min=0x1, access_size_max=0x4, access=
>     @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x110191f8)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
> #13 0x0000000010380c90 in memory_region_dispatch_write (mr=0x110191f8,
> addr=0x48, data=0xffe2, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:993
> #14 0x00000000103840d8 in io_mem_write (mr=0x110191f8, addr=0x48,
> val=0xe2ff0000, size=0x4)
>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1696
> #15 0x00000000102e4968 in stl_phys_internal (addr=0x10080000048,
> val=0xe2ff0000, endian=
>     DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2447
> #16 0x00000000102e4a64 in stl_phys (addr=0x10080000048, val=0xe2ff0000)
>     at /home/alexey/pcipassthru/qemu-impreza/exec.c:2469
> #17 0x00000000103550d8 in h_logical_store (cpu=0x1fffff0f0010,
> spapr=0x10fe9510, opcode=0x40,
>     args=0x1ffffffd0030) at
> /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
> #18 0x0000000010355698 in spapr_hypercall (cpu=0x1fffff0f0010, opcode=0x40,
> args=0x1ffffffd0030)
>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:689
> 
> 
> 
> 
> 
> 
> On 07/19/2013 10:49 PM, Paolo Bonzini wrote:
>> Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:
>>> Hi!
>>>
>>> This patch also breaks virtio on powerpc. I thought it was fixed
>>> (reverted?) in the master branch from qemu.org but it is still there. What
>>> did I miss?
>>
>> It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.
>>
>> Let me check if I can reproduce this, it looks like a endianness
>> problems reading virtio-blk config space.
>>
>> Paolo
>>
>>> Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> virtioblk_read: Access beyond end of device!
>>> [many of those]
>>>
>>>
>>>
>>> On 07/11/2013 10:29 PM, Alexander Graf wrote:
>>>>
>>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>>
>>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>>> Jan Kiszka a écrit :
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>>> without converting all portio users by embedding the required base
>>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>>> in the loop on every access.
>>>>>>>
>>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>>> for portio memory regions when registering them with the memory core.
>>>>>>> This removes the need for the old_portio field.
>>>>>>>
>>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>>> single source file.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>> +
>>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>> +                         unsigned size)
>>>>>>> +{
>>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>>> true);
>>>>>>> +
>>>>>>> +    if (mrp) {
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>>> +    } else if (size == 2) {
>>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>>> +        assert(mrp);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>>> 8);
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>>> +    .read = portio_read,
>>>>>>> +    .write = portio_write,
>>>>>>> +    .valid.unaligned = true,
>>>>>>> +    .impl.unaligned = true,
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>>
>>>>> Anything behind PIO is little endian, of course. Will add this.
>>>>
>>>> This patch breaks VGA on PPC as it is in master today.
>>>>
>>>>
>>>> Alex
>>>>
>>>>>
>>>>>>
>>>>>> This fixes PPC PReP emulation, which would otherwise be broken with this
>>>>>> patchset.
>>>>>
>>>>> Thanks,
>>>>> Jan
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Alexey Kardashevskiy]

On 07/20/2013 10:55 AM, Alexey Kardashevskiy wrote:
> On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
>> Ok. So.
>>
>> What broke is...
>> I could try explaining but backtraces are lot better :)
>>
>> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
>> we had a workaround in spapr_io_ops), now it works so double swap happens
>> and everything gets broken.
>>
>> If we talk about VGA (in powerpc, it is all about powerpc), I guess
>> memory_region_iorange_write() will go through mr->ops->old_portio branch
>> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
>> fine here. I do not understand yet why it works on mac99 though, too late
>> here :)
> 
> 
> I understood. VGA does not work for mac99 either with this command line:
> ./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
> So it works for pseries only because of parity bug in spapr_io_ops.


oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops
does the swapping in this case and portio_ops does not swap (in upstream).

Oh. Ah. Uh.

Adding cc:Benh...


> So the right fix is to get rid of spapr_io_ops and every other hack like
> that and to add byte swapping to every "if (mr->ops->old_portio)" branch
> (should fix VGA and all other old_portio users). Current byte swapping in
> memory regions seems to be right.
> 
> I would try fixing it but since all my patches were terrible shit so far, I
> won't risk :)
> 
> 
> 
>> h_logical_store is a hypercall for system firmware to do cache inhibited
>> read/write.
>>
>>
>> This is with the patch applied (git checkout  b40acf9):
>>
>>
>> #0  virtqueue_init (vq=0x11014ac0) at
>> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
>> #1  0x0000000010371f28 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
>> addr=0xd0fb0000000)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
>> #2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
>> val=0xd0fb0000)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
>> #3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
>> addr=0x8, val=0xd0fb0000, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
>> #4  0x000000001037e220 in memory_region_write_accessor (opaque=0x11019c78,
>> addr=0x8, value=0x1fffff0edc00,
>>     size=0x4, shift=0x0, mask=0xffffffff) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #5  0x000000001037e36c in access_with_adjusted_size (addr=0x8,
>> value=0x1fffff0edc00, size=0x4,
>>     access_size_min=0x1, access_size_max=0x4, access=
>>     @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x11019c78)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #6  0x0000000010380b5c in memory_region_dispatch_write (mr=0x11019c78,
>> addr=0x8, data=0xd0fb0000, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
>> #7  0x0000000010383fa4 in io_mem_write (mr=0x11019c78, addr=0x8,
>> val=0xfbd0, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
>> #8  0x00000000102e2fdc in address_space_rw (as=0x10ef4350
>> <address_space_io>, addr=0x48,
>>     buf=0x1fffff0edde0 "", len=0x4, is_write=0x1) at
>> /home/alexey/pcipassthru/qemu-impreza/exec.c:1918
>> #9  0x00000000102e33c8 in address_space_write (as=0x10ef4350
>> <address_space_io>, addr=0x48,
>>     buf=0x1fffff0edde0 "", len=0x4) at
>> /home/alexey/pcipassthru/qemu-impreza/exec.c:1969
>> #10 0x0000000010375754 in cpu_outl (addr=0x48, val=0xfbd0)
>>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:309
>> #11 0x0000000010358240 in spapr_io_write (opaque=0x11016a00, addr=0x48,
>> data=0xfbd0, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
>> #12 0x000000001037e220 in memory_region_write_accessor (opaque=0x110191f8,
>> addr=0x48, value=0x1fffff0ee060,
>>     size=0x4, shift=0x0, mask=0xffffffff) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #13 0x000000001037e36c in access_with_adjusted_size (addr=0x48,
>> value=0x1fffff0ee060, size=0x4,
>>     access_size_min=0x1, access_size_max=0x4, access=
>>     @0x1069df40: 0x1037e164 <memory_region_write_accessor>, opaque=0x110191f8)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #14 0x0000000010380b5c in memory_region_dispatch_write (mr=0x110191f8,
>> addr=0x48, data=0xfbd0, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:905
>> #15 0x0000000010383fa4 in io_mem_write (mr=0x110191f8, addr=0x48,
>> val=0xd0fb0000, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1608
>> #16 0x00000000102e47ac in stl_phys_internal (addr=0x10080000048,
>> val=0xd0fb0000, endian=
>>     DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2420
>> #17 0x00000000102e48a8 in stl_phys (addr=0x10080000048, val=0xd0fb0000)
>>     at /home/alexey/pcipassthru/qemu-impreza/exec.c:2442
>> #18 0x0000000010354f1c in h_logical_store (cpu=0x1fffff0f0010,
>> spapr=0x10fe9510, opcode=0x40,
>>     args=0x1ffffffd0030) at
>> /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
>>
>>
>>
>> This is without this patch (i.e. git checkout  b40acf9^ ):
>>
>> #0  virtqueue_init (vq=0x11014ac0) at
>> /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:90
>> #1  0x00000000103720e4 in virtio_queue_set_addr (vdev=0x11019dd0, n=0x0,
>> addr=0xffe2000)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio.c:662
>> #2  0x00000000102027f0 in virtio_ioport_write (opaque=0x11019580, addr=0x8,
>> val=0xffe2)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:278
>> #3  0x0000000010202f08 in virtio_pci_config_write (opaque=0x11019580,
>> addr=0x8, val=0xffe2, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/virtio/virtio-pci.c:416
>> #4  0x000000001037dca8 in memory_region_write_accessor (opaque=0x11019c78,
>> addr=0x8, value=0x1fffff0edca8,
>>     size=0x4, shift=0x0, mask=0xffffffff) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #5  0x000000001037ddf4 in access_with_adjusted_size (addr=0x8,
>> value=0x1fffff0edca8, size=0x4,
>>     access_size_min=0x1, access_size_max=0x4, access=
>>     @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x11019c78)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #6  0x000000001037e474 in memory_region_iorange_write
>> (iorange=0x1ffff0005430, offset=0x8, width=0x4,
>>     data=0xffe2) at /home/alexey/pcipassthru/qemu-impreza/memory.c:475
>> #7  0x00000000103750d4 in ioport_writel_thunk (opaque=0x1ffff0005430,
>> addr=0x48, data=0xffe2)
>>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:226
>> #8  0x0000000010374728 in ioport_write (index=0x2, address=0x48, data=0xffe2)
>>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:83
>> #9  0x0000000010375688 in cpu_outl (addr=0x48, val=0xffe2)
>>     at /home/alexey/pcipassthru/qemu-impreza/ioport.c:296
>> #10 0x00000000103583fc in spapr_io_write (opaque=0x11016a00, addr=0x48,
>> data=0xffe2, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_pci.c:468
>> #11 0x000000001037dca8 in memory_region_write_accessor (opaque=0x110191f8,
>> addr=0x48, value=0x1fffff0ee060,
>>     size=0x4, shift=0x0, mask=0xffffffff) at
>> /home/alexey/pcipassthru/qemu-impreza/memory.c:364
>> #12 0x000000001037ddf4 in access_with_adjusted_size (addr=0x48,
>> value=0x1fffff0ee060, size=0x4,
>>     access_size_min=0x1, access_size_max=0x4, access=
>>     @0x1069def8: 0x1037dbec <memory_region_write_accessor>, opaque=0x110191f8)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:396
>> #13 0x0000000010380c90 in memory_region_dispatch_write (mr=0x110191f8,
>> addr=0x48, data=0xffe2, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:993
>> #14 0x00000000103840d8 in io_mem_write (mr=0x110191f8, addr=0x48,
>> val=0xe2ff0000, size=0x4)
>>     at /home/alexey/pcipassthru/qemu-impreza/memory.c:1696
>> #15 0x00000000102e4968 in stl_phys_internal (addr=0x10080000048,
>> val=0xe2ff0000, endian=
>>     DEVICE_NATIVE_ENDIAN) at /home/alexey/pcipassthru/qemu-impreza/exec.c:2447
>> #16 0x00000000102e4a64 in stl_phys (addr=0x10080000048, val=0xe2ff0000)
>>     at /home/alexey/pcipassthru/qemu-impreza/exec.c:2469
>> #17 0x00000000103550d8 in h_logical_store (cpu=0x1fffff0f0010,
>> spapr=0x10fe9510, opcode=0x40,
>>     args=0x1ffffffd0030) at
>> /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:570
>> #18 0x0000000010355698 in spapr_hypercall (cpu=0x1fffff0f0010, opcode=0x40,
>> args=0x1ffffffd0030)
>>     at /home/alexey/pcipassthru/qemu-impreza/hw/ppc/spapr_hcall.c:689
>>
>>
>>
>>
>>
>>
>> On 07/19/2013 10:49 PM, Paolo Bonzini wrote:
>>> Il 19/07/2013 13:09, Alexey Kardashevskiy ha scritto:
>>>> Hi!
>>>>
>>>> This patch also breaks virtio on powerpc. I thought it was fixed
>>>> (reverted?) in the master branch from qemu.org but it is still there. What
>>>> did I miss?
>>>
>>> It was not reverted, only the "DEVICE_LITTLE_ENDIAN" marking was.
>>>
>>> Let me check if I can reproduce this, it looks like a endianness
>>> problems reading virtio-blk config space.
>>>
>>> Paolo
>>>
>>>> Trying to load:  from: disk ... virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> virtioblk_read: Access beyond end of device!
>>>> [many of those]
>>>>
>>>>
>>>>
>>>> On 07/11/2013 10:29 PM, Alexander Graf wrote:
>>>>>
>>>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>>>
>>>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>>>> Jan Kiszka a écrit :
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>>>> without converting all portio users by embedding the required base
>>>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>>>> in the loop on every access.
>>>>>>>>
>>>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>>>> for portio memory regions when registering them with the memory core.
>>>>>>>> This removes the need for the old_portio field.
>>>>>>>>
>>>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>>>> single source file.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>> +
>>>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>>> +                         unsigned size)
>>>>>>>> +{
>>>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>>>> true);
>>>>>>>> +
>>>>>>>> +    if (mrp) {
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>>>> +    } else if (size == 2) {
>>>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>>>> +        assert(mrp);
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>>>> 8);
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>>>> +    .read = portio_read,
>>>>>>>> +    .write = portio_write,
>>>>>>>> +    .valid.unaligned = true,
>>>>>>>> +    .impl.unaligned = true,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>>>
>>>>>> Anything behind PIO is little endian, of course. Will add this.
>>>>>
>>>>> This patch breaks VGA on PPC as it is in master today.
>>>>>
>>>>>
>>>>> Alex
>>>>>
>>>>>>
>>>>>>>
>>>>>>> This fixes PPC PReP emulation, which would otherwise be broken with this
>>>>>>> patchset.
>>>>>>
>>>>>> Thanks,
>>>>>> Jan
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 20/07/2013 03:11, Alexey Kardashevskiy ha scritto:
> On 07/20/2013 10:55 AM, Alexey Kardashevskiy wrote:
>> On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
>>> Ok. So.
>>>
>>> What broke is...
>>> I could try explaining but backtraces are lot better :)
>>>
>>> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
>>> we had a workaround in spapr_io_ops), now it works so double swap happens
>>> and everything gets broken.
>>>
>>> If we talk about VGA (in powerpc, it is all about powerpc), I guess
>>> memory_region_iorange_write() will go through mr->ops->old_portio branch
>>> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
>>> fine here. I do not understand yet why it works on mac99 though, too late
>>> here :)
>>
>> I understood. VGA does not work for mac99 either with this command line:
>> ./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
>> So it works for pseries only because of parity bug in spapr_io_ops.

Yes, this is what Herve was fixing for PREP too.

> oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops
> does the swapping in this case and portio_ops does not swap (in upstream).

Uff... I guess we have to look at all cases for big-endian machines, and
make sure there is an odd number of exchanges.  Aurelien/Alex/Herve,
where can I find images for mips malta, mips jazz, ppc4xx and prep?
Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

Also, is all the firmware included in pc-bios/?

For g3beige and mac99 I can use mintppc, I think, that's what I
installed on my old G4 PowerBook.

Paolo

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Edgar E. Iglesias]

On Sat, Jul 20, 2013 at 12:11:34PM +0200, Paolo Bonzini wrote:
> Il 20/07/2013 03:11, Alexey Kardashevskiy ha scritto:
> > On 07/20/2013 10:55 AM, Alexey Kardashevskiy wrote:
> >> On 07/20/2013 01:48 AM, Alexey Kardashevskiy wrote:
> >>> Ok. So.
> >>>
> >>> What broke is...
> >>> I could try explaining but backtraces are lot better :)
> >>>
> >>> Shortly - virtio_pci_config_ops.endianness was ignored before (was bad but
> >>> we had a workaround in spapr_io_ops), now it works so double swap happens
> >>> and everything gets broken.
> >>>
> >>> If we talk about VGA (in powerpc, it is all about powerpc), I guess
> >>> memory_region_iorange_write() will go through mr->ops->old_portio branch
> >>> and won't do any byte swapping (so spapr_io_ops will do the job), so we are
> >>> fine here. I do not understand yet why it works on mac99 though, too late
> >>> here :)
> >>
> >> I understood. VGA does not work for mac99 either with this command line:
> >> ./qemu-system-ppc64 -m "1024" -M "mac99" -vga "std"
> >> So it works for pseries only because of parity bug in spapr_io_ops.
> 
> Yes, this is what Herve was fixing for PREP too.
> 
> > oops. I am wrong and VGA works on mac99 in upstream because isa_mmio_ops
> > does the swapping in this case and portio_ops does not swap (in upstream).
> 
> Uff... I guess we have to look at all cases for big-endian machines, and
> make sure there is an odd number of exchanges.  Aurelien/Alex/Herve,
> where can I find images for mips malta, mips jazz, ppc4xx and prep?
> Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

Sorry, I don't know (I haven't done much SH).

Best regards,
Edgar

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Hervé Poussineau]

Paolo Bonzini a écrit :
  >> oops. I am wrong and VGA works on mac99 in upstream because 
isa_mmio_ops
>> does the swapping in this case and portio_ops does not swap (in upstream).
> 
> Uff... I guess we have to look at all cases for big-endian machines, and
> make sure there is an odd number of exchanges.  Aurelien/Alex/Herve,
> where can I find images for mips malta, mips jazz, ppc4xx and prep?
> Edgar, what about sh4eb (IIRC one of the two machine types supported PCI)?

For PReP, you can find a kernel at 
http://www.juneau-lug.org/zImage.initrd.sandalfoot
Run it with qemu-system-ppc -M prep -kernel zImage.initrd.sandalfoot

Hervé

Re: [Qemu-devel] BUG: Re: [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Paolo Bonzini]

Il 21/07/2013 17:13, Hervé Poussineau ha scritto:
> Paolo Bonzini a écrit :
>  >> oops. I am wrong and VGA works on mac99 in upstream because
> isa_mmio_ops
>>> does the swapping in this case and portio_ops does not swap (in
>>> upstream).
>>
>> Uff... I guess we have to look at all cases for big-endian machines, and
>> make sure there is an odd number of exchanges.  Aurelien/Alex/Herve,
>> where can I find images for mips malta, mips jazz, ppc4xx and prep?
>> Edgar, what about sh4eb (IIRC one of the two machine types supported
>> PCI)?
> 
> For PReP, you can find a kernel at
> http://www.juneau-lug.org/zImage.initrd.sandalfoot
> Run it with qemu-system-ppc -M prep -kernel zImage.initrd.sandalfoot

Thanks.  I can also use qtest to fix bugs, but this will help making a
more complete test.

Paolo

[Qemu-devel] [PATCH v4 11/14] ioport: Switch dispatching to memory core layer

[Jan Kiszka]

The current ioport dispatcher is a complex beast, mostly due to the
need to deal with old portio interface users. But we can overcome it
without converting all portio users by embedding the required base
address of a MemoryRegionPortio access into that data structure. That
removes the need to have the additional MemoryRegionIORange structure
in the loop on every access.

To handle old portio memory ops, we simply install dispatching handlers
for portio memory regions when registering them with the memory core.
This removes the need for the old_portio field.

We can drop the additional aliasing of ioport regions and also the
special address space listener. cpu_in and cpu_out now simply call
address_space_read/write. And we can concentrate portio handling in a
single source file.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v4:
 - mark portio dispatching ops as little endian

 exec.c                         |   27 --------
 include/exec/ioport.h          |    1 -
 include/exec/memory-internal.h |    2 -
 include/exec/memory.h          |    5 +-
 ioport.c                       |  138 +++++++++++++++++++++++++++++-----------
 memory.c                       |   88 -------------------------
 6 files changed, 102 insertions(+), 159 deletions(-)

diff --git a/exec.c b/exec.c
index 0b0118b..87c7ef3 100644
--- a/exec.c
+++ b/exec.c
@@ -1760,26 +1760,6 @@ static void core_log_global_stop(MemoryListener *listener)
     cpu_physical_memory_set_dirty_tracking(0);
 }
 
-static void io_region_add(MemoryListener *listener,
-                          MemoryRegionSection *section)
-{
-    MemoryRegionIORange *mrio = g_new(MemoryRegionIORange, 1);
-
-    mrio->mr = section->mr;
-    mrio->offset = section->offset_within_region;
-    iorange_init(&mrio->iorange, &memory_region_iorange_ops,
-                 section->offset_within_address_space,
-                 int128_get64(section->size));
-    ioport_register(&mrio->iorange);
-}
-
-static void io_region_del(MemoryListener *listener,
-                          MemoryRegionSection *section)
-{
-    isa_unassign_ioport(section->offset_within_address_space,
-                        int128_get64(section->size));
-}
-
 static MemoryListener core_memory_listener = {
     .begin = core_begin,
     .log_global_start = core_log_global_start,
@@ -1787,12 +1767,6 @@ static MemoryListener core_memory_listener = {
     .priority = 1,
 };
 
-static MemoryListener io_memory_listener = {
-    .region_add = io_region_add,
-    .region_del = io_region_del,
-    .priority = 0,
-};
-
 static MemoryListener tcg_memory_listener = {
     .commit = tcg_commit,
 };
@@ -1834,7 +1808,6 @@ static void memory_map_init(void)
     address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
-    memory_listener_register(&io_memory_listener, &address_space_io);
     memory_listener_register(&tcg_memory_listener, &address_space_memory);
 }
 
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index eb99ffe..b476857 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -56,7 +56,6 @@ typedef struct PortioList {
     struct MemoryRegion *address_space;
     unsigned nr;
     struct MemoryRegion **regions;
-    struct MemoryRegion **aliases;
     void *opaque;
     const char *name;
 } PortioList;
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 26689fe..d0e0633 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -119,8 +119,6 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
                                      int dirty_flags);
 
-extern const IORangeOps memory_region_iorange_ops;
-
 #endif
 
 #endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3598c4f..7c1e6da 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -124,10 +124,6 @@ struct MemoryRegionOps {
          bool unaligned;
     } impl;
 
-    /* If .read and .write are not present, old_portio may be used for
-     * backwards compatibility with old portio registration
-     */
-    const MemoryRegionPortio *old_portio;
     /* If .read and .write are not present, old_mmio may be used for
      * backwards compatibility with old mmio registration
      */
@@ -183,6 +179,7 @@ struct MemoryRegionPortio {
     unsigned size;
     IOPortReadFunc *read;
     IOPortWriteFunc *write;
+    uint32_t base; /* private field */
 };
 
 #define PORTIO_END_OF_LIST() { }
diff --git a/ioport.c b/ioport.c
index 56470c5..347da2c 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
 #include "exec/ioport.h"
 #include "trace.h"
 #include "exec/memory.h"
+#include "exec/address-spaces.h"
 
 /***********************************************************/
 /* IO Port */
@@ -47,6 +48,12 @@
 #  define LOG_IOPORT(...) do { } while (0)
 #endif
 
+typedef struct MemoryRegionPortioList {
+    MemoryRegion mr;
+    void *portio_opaque;
+    MemoryRegionPortio ports[];
+} MemoryRegionPortioList;
+
 /* XXX: use a two level table to limit memory usage */
 
 static void *ioport_opaque[MAX_IOPORTS];
@@ -279,27 +286,34 @@ void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(0, addr, val);
+    address_space_write(&address_space_io, addr, &val, 1);
 }
 
 void cpu_outw(pio_addr_t addr, uint16_t val)
 {
+    uint8_t buf[2];
+
     LOG_IOPORT("outw: %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(1, addr, val);
+    stw_p(buf, val);
+    address_space_write(&address_space_io, addr, buf, 2);
 }
 
 void cpu_outl(pio_addr_t addr, uint32_t val)
 {
+    uint8_t buf[4];
+
     LOG_IOPORT("outl: %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
     trace_cpu_out(addr, val);
-    ioport_write(2, addr, val);
+    stl_p(buf, val);
+    address_space_write(&address_space_io, addr, buf, 4);
 }
 
 uint8_t cpu_inb(pio_addr_t addr)
 {
     uint8_t val;
-    val = ioport_read(0, addr);
+
+    address_space_read(&address_space_io, addr, &val, 1);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inb : %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
     return val;
@@ -307,8 +321,11 @@ uint8_t cpu_inb(pio_addr_t addr)
 
 uint16_t cpu_inw(pio_addr_t addr)
 {
+    uint8_t buf[2];
     uint16_t val;
-    val = ioport_read(1, addr);
+
+    address_space_read(&address_space_io, addr, buf, 2);
+    val = lduw_p(buf);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inw : %04"FMT_pioaddr" %04"PRIx16"\n", addr, val);
     return val;
@@ -316,8 +333,11 @@ uint16_t cpu_inw(pio_addr_t addr)
 
 uint32_t cpu_inl(pio_addr_t addr)
 {
+    uint8_t buf[4];
     uint32_t val;
-    val = ioport_read(2, addr);
+
+    address_space_read(&address_space_io, addr, buf, 4);
+    val = ldl_p(buf);
     trace_cpu_in(addr, val);
     LOG_IOPORT("inl : %04"FMT_pioaddr" %08"PRIx32"\n", addr, val);
     return val;
@@ -336,7 +356,6 @@ void portio_list_init(PortioList *piolist,
     piolist->ports = callbacks;
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
-    piolist->aliases = g_new0(MemoryRegion *, n);
     piolist->address_space = NULL;
     piolist->opaque = opaque;
     piolist->name = name;
@@ -345,46 +364,96 @@ void portio_list_init(PortioList *piolist,
 void portio_list_destroy(PortioList *piolist)
 {
     g_free(piolist->regions);
-    g_free(piolist->aliases);
 }
 
+static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
+                                             uint64_t offset, unsigned size,
+                                             bool write)
+{
+    const MemoryRegionPortio *mrp;
+
+    for (mrp = mrpio->ports; mrp->size; ++mrp) {
+        if (offset >= mrp->offset && offset < mrp->offset + mrp->len &&
+            size == mrp->size &&
+            (write ? (bool)mrp->write : (bool)mrp->read)) {
+            return mrp;
+        }
+    }
+    return NULL;
+}
+
+static uint64_t portio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, false);
+    uint64_t data;
+
+    data = ((uint64_t)1 << (size * 8)) - 1;
+    if (mrp) {
+        data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, false);
+        assert(mrp);
+        data = mrp->read(mrpio->portio_opaque, mrp->base + addr) |
+                (mrp->read(mrpio->portio_opaque, mrp->base + addr + 1) << 8);
+    }
+    return data;
+}
+
+static void portio_write(void *opaque, hwaddr addr, uint64_t data,
+                         unsigned size)
+{
+    MemoryRegionPortioList *mrpio = opaque;
+    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
+
+    if (mrp) {
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+    } else if (size == 2) {
+        mrp = find_portio(mrpio, addr, 1, true);
+        assert(mrp);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
+    }
+}
+
+static const MemoryRegionOps portio_ops = {
+    .read = portio_read,
+    .write = portio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.unaligned = true,
+    .impl.unaligned = true,
+};
+
 static void portio_list_add_1(PortioList *piolist,
                               const MemoryRegionPortio *pio_init,
                               unsigned count, unsigned start,
                               unsigned off_low, unsigned off_high)
 {
-    MemoryRegionPortio *pio;
-    MemoryRegionOps *ops;
-    MemoryRegion *region, *alias;
+    MemoryRegionPortioList *mrpio;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    pio = g_new(MemoryRegionPortio, count + 1);
-    memcpy(pio, pio_init, sizeof(MemoryRegionPortio) * count);
-    memset(pio + count, 0, sizeof(MemoryRegionPortio));
+    mrpio = g_malloc0(sizeof(MemoryRegionPortioList) +
+                      sizeof(MemoryRegionPortio) * (count + 1));
+    mrpio->portio_opaque = piolist->opaque;
+    memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
+    memset(mrpio->ports + count, 0, sizeof(MemoryRegionPortio));
 
     /* Adjust the offsets to all be zero-based for the region.  */
     for (i = 0; i < count; ++i) {
-        pio[i].offset -= off_low;
+        mrpio->ports[i].offset -= off_low;
+        mrpio->ports[i].base = start + off_low;
     }
 
-    ops = g_new0(MemoryRegionOps, 1);
-    ops->old_portio = pio;
-
-    region = g_new(MemoryRegion, 1);
-    alias = g_new(MemoryRegion, 1);
     /*
      * Use an alias so that the callback is called with an absolute address,
      * rather than an offset relative to to start + off_low.
      */
-    memory_region_init_io(region, ops, piolist->opaque, piolist->name,
-                          INT64_MAX);
-    memory_region_init_alias(alias, piolist->name,
-                             region, start + off_low, off_high - off_low);
+    memory_region_init_io(&mrpio->mr, &portio_ops, mrpio, piolist->name,
+                          off_high - off_low);
     memory_region_add_subregion(piolist->address_space,
-                                start + off_low, alias);
-    piolist->regions[piolist->nr] = region;
-    piolist->aliases[piolist->nr] = alias;
+                                start + off_low, &mrpio->mr);
+    piolist->regions[piolist->nr] = &mrpio->mr;
     ++piolist->nr;
 }
 
@@ -427,19 +496,14 @@ void portio_list_add(PortioList *piolist,
 
 void portio_list_del(PortioList *piolist)
 {
-    MemoryRegion *mr, *alias;
+    MemoryRegionPortioList *mrpio;
     unsigned i;
 
     for (i = 0; i < piolist->nr; ++i) {
-        mr = piolist->regions[i];
-        alias = piolist->aliases[i];
-        memory_region_del_subregion(piolist->address_space, alias);
-        memory_region_destroy(alias);
-        memory_region_destroy(mr);
-        g_free((MemoryRegionOps *)mr->ops);
-        g_free(mr);
-        g_free(alias);
+        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
+        memory_region_del_subregion(piolist->address_space, &mrpio->mr);
+        memory_region_destroy(&mrpio->mr);
+        g_free(mrpio);
         piolist->regions[i] = NULL;
-        piolist->aliases[i] = NULL;
     }
 }
diff --git a/memory.c b/memory.c
index 47b005a..df07b24 100644
--- a/memory.c
+++ b/memory.c
@@ -401,94 +401,6 @@ static void access_with_adjusted_size(hwaddr addr,
     }
 }
 
-static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset,
-                                             unsigned width, bool write)
-{
-    const MemoryRegionPortio *mrp;
-
-    for (mrp = mr->ops->old_portio; mrp->size; ++mrp) {
-        if (offset >= mrp->offset && offset < mrp->offset + mrp->len
-            && width == mrp->size
-            && (write ? (bool)mrp->write : (bool)mrp->read)) {
-            return mrp;
-        }
-    }
-    return NULL;
-}
-
-static void memory_region_iorange_read(IORange *iorange,
-                                       uint64_t offset,
-                                       unsigned width,
-                                       uint64_t *data)
-{
-    MemoryRegionIORange *mrio
-        = container_of(iorange, MemoryRegionIORange, iorange);
-    MemoryRegion *mr = mrio->mr;
-
-    offset += mrio->offset;
-    if (mr->ops->old_portio) {
-        const MemoryRegionPortio *mrp = find_portio(mr, offset - mrio->offset,
-                                                    width, false);
-
-        *data = ((uint64_t)1 << (width * 8)) - 1;
-        if (mrp) {
-            *data = mrp->read(mr->opaque, offset);
-        } else if (width == 2) {
-            mrp = find_portio(mr, offset - mrio->offset, 1, false);
-            assert(mrp);
-            *data = mrp->read(mr->opaque, offset) |
-                    (mrp->read(mr->opaque, offset + 1) << 8);
-        }
-        return;
-    }
-    *data = 0;
-    access_with_adjusted_size(offset, data, width,
-                              mr->ops->impl.min_access_size,
-                              mr->ops->impl.max_access_size,
-                              memory_region_read_accessor, mr);
-}
-
-static void memory_region_iorange_write(IORange *iorange,
-                                        uint64_t offset,
-                                        unsigned width,
-                                        uint64_t data)
-{
-    MemoryRegionIORange *mrio
-        = container_of(iorange, MemoryRegionIORange, iorange);
-    MemoryRegion *mr = mrio->mr;
-
-    offset += mrio->offset;
-    if (mr->ops->old_portio) {
-        const MemoryRegionPortio *mrp = find_portio(mr, offset - mrio->offset,
-                                                    width, true);
-
-        if (mrp) {
-            mrp->write(mr->opaque, offset, data);
-        } else if (width == 2) {
-            mrp = find_portio(mr, offset - mrio->offset, 1, true);
-            assert(mrp);
-            mrp->write(mr->opaque, offset, data & 0xff);
-            mrp->write(mr->opaque, offset + 1, data >> 8);
-        }
-        return;
-    }
-    access_with_adjusted_size(offset, &data, width,
-                              mr->ops->impl.min_access_size,
-                              mr->ops->impl.max_access_size,
-                              memory_region_write_accessor, mr);
-}
-
-static void memory_region_iorange_destructor(IORange *iorange)
-{
-    g_free(container_of(iorange, MemoryRegionIORange, iorange));
-}
-
-const IORangeOps memory_region_iorange_ops = {
-    .read = memory_region_iorange_read,
-    .write = memory_region_iorange_write,
-    .destructor = memory_region_iorange_destructor,
-};
-
 static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
 {
     AddressSpace *as;
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

[Anthony Liguori]

Hervé Poussineau <hpoussin@reactos.org> writes:

> Jan Kiszka a écrit :
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> The current ioport dispatcher is a complex beast, mostly due to the
>> need to deal with old portio interface users. But we can overcome it
>> without converting all portio users by embedding the required base
>> address of a MemoryRegionPortio access into that data structure. That
>> removes the need to have the additional MemoryRegionIORange structure
>> in the loop on every access.
>> 
>> To handle old portio memory ops, we simply install dispatching handlers
>> for portio memory regions when registering them with the memory core.
>> This removes the need for the old_portio field.
>> 
>> We can drop the additional aliasing of ioport regions and also the
>> special address space listener. cpu_in and cpu_out now simply call
>> address_space_read/write. And we can concentrate portio handling in a
>> single source file.
>> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>
> ...
>
>> +
>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>> +                         unsigned size)
>> +{
>> +    MemoryRegionPortioList *mrpio = opaque;
>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
>> +
>> +    if (mrp) {
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>> +    } else if (size == 2) {
>> +        mrp = find_portio(mrpio, addr, 1, true);
>> +        assert(mrp);
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps portio_ops = {
>> +    .read = portio_read,
>> +    .write = portio_write,
>> +    .valid.unaligned = true,
>> +    .impl.unaligned = true,
>> +};
>> +
>
> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
> In portio_write above, you clearly assume that data is in LE format.
>
> This fixes PPC PReP emulation, which would otherwise be broken with this 
> patchset.

Did you actually test that as this is how the code behaved previously?

Regards,

Anthony Liguori

>
> Hervé

[Qemu-devel] [PATCH v3 12/14] ioport: Remove unused old dispatching services

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Remove unused ioport_register and isa_unassign_ioport along with
everything that only those services used.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/exec/ioport.h  |    5 -
 include/exec/iorange.h |   31 ------
 include/exec/memory.h  |    9 --
 ioport.c               |  238 ------------------------------------------------
 4 files changed, 0 insertions(+), 283 deletions(-)
 delete mode 100644 include/exec/iorange.h

diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index b476857..ba3ebb8 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -25,7 +25,6 @@
 #define IOPORT_H
 
 #include "qemu-common.h"
-#include "exec/iorange.h"
 
 typedef uint32_t pio_addr_t;
 #define FMT_pioaddr     PRIx32
@@ -36,10 +35,6 @@ typedef uint32_t pio_addr_t;
 /* These should really be in isa.h, but are here to make pc.h happy.  */
 typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data);
 typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
-typedef void (IOPortDestructor)(void *opaque);
-
-void ioport_register(IORange *iorange);
-void isa_unassign_ioport(pio_addr_t start, int length);
 
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
diff --git a/include/exec/iorange.h b/include/exec/iorange.h
deleted file mode 100644
index cd980a8..0000000
--- a/include/exec/iorange.h
+++ /dev/null
@@ -1,31 +0,0 @@
-#ifndef IORANGE_H
-#define IORANGE_H
-
-#include <stdint.h>
-
-typedef struct IORange IORange;
-typedef struct IORangeOps IORangeOps;
-
-struct IORangeOps {
-    void (*read)(IORange *iorange, uint64_t offset, unsigned width,
-                 uint64_t *data);
-    void (*write)(IORange *iorange, uint64_t offset, unsigned width,
-                  uint64_t data);
-    void (*destructor)(IORange *iorange);
-};
-
-struct IORange {
-    const IORangeOps *ops;
-    uint64_t base;
-    uint64_t len;
-};
-
-static inline void iorange_init(IORange *iorange, const IORangeOps *ops,
-                                uint64_t base, uint64_t len)
-{
-    iorange->ops = ops;
-    iorange->base = base;
-    iorange->len = len;
-}
-
-#endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7c1e6da..8c1373e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -22,7 +22,6 @@
 #include "exec/cpu-common.h"
 #include "exec/hwaddr.h"
 #include "qemu/queue.h"
-#include "exec/iorange.h"
 #include "exec/ioport.h"
 #include "qemu/int128.h"
 #include "qemu/notify.h"
@@ -46,14 +45,6 @@ struct MemoryRegionMmio {
     CPUWriteMemoryFunc *write[3];
 };
 
-/* Internal use; thunks between old-style IORange and MemoryRegions. */
-typedef struct MemoryRegionIORange MemoryRegionIORange;
-struct MemoryRegionIORange {
-    IORange iorange;
-    MemoryRegion *mr;
-    hwaddr offset;
-};
-
 typedef struct IOMMUTLBEntry IOMMUTLBEntry;
 
 /* See address_space_translate: bit 0 is read, bit 1 is write.  */
diff --git a/ioport.c b/ioport.c
index e34f6b2..126430d 100644
--- a/ioport.c
+++ b/ioport.c
@@ -30,18 +30,8 @@
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 
-/***********************************************************/
-/* IO Port */
-
-//#define DEBUG_UNUSED_IOPORT
 //#define DEBUG_IOPORT
 
-#ifdef DEBUG_UNUSED_IOPORT
-#  define LOG_UNUSED_IOPORT(fmt, ...) fprintf(stderr, fmt, ## __VA_ARGS__)
-#else
-#  define LOG_UNUSED_IOPORT(fmt, ...) do{ } while (0)
-#endif
-
 #ifdef DEBUG_IOPORT
 #  define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__)
 #else
@@ -54,234 +44,6 @@ typedef struct MemoryRegionPortioList {
     MemoryRegionPortio ports[];
 } MemoryRegionPortioList;
 
-/* XXX: use a two level table to limit memory usage */
-
-static void *ioport_opaque[MAX_IOPORTS];
-static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS];
-static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS];
-static IOPortDestructor *ioport_destructor_table[MAX_IOPORTS];
-
-static IOPortReadFunc default_ioport_readb, default_ioport_readw, default_ioport_readl;
-static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, default_ioport_writel;
-
-static uint32_t ioport_read(int index, uint32_t address)
-{
-    static IOPortReadFunc * const default_func[3] = {
-        default_ioport_readb,
-        default_ioport_readw,
-        default_ioport_readl
-    };
-    IOPortReadFunc *func = ioport_read_table[index][address];
-    if (!func)
-        func = default_func[index];
-    return func(ioport_opaque[address], address);
-}
-
-static void ioport_write(int index, uint32_t address, uint32_t data)
-{
-    static IOPortWriteFunc * const default_func[3] = {
-        default_ioport_writeb,
-        default_ioport_writew,
-        default_ioport_writel
-    };
-    IOPortWriteFunc *func = ioport_write_table[index][address];
-    if (!func)
-        func = default_func[index];
-    func(ioport_opaque[address], address, data);
-}
-
-static uint32_t default_ioport_readb(void *opaque, uint32_t address)
-{
-    LOG_UNUSED_IOPORT("unused inb: port=0x%04"PRIx32"\n", address);
-    return 0xff;
-}
-
-static void default_ioport_writeb(void *opaque, uint32_t address, uint32_t data)
-{
-    LOG_UNUSED_IOPORT("unused outb: port=0x%04"PRIx32" data=0x%02"PRIx32"\n",
-                      address, data);
-}
-
-/* default is to make two byte accesses */
-static uint32_t default_ioport_readw(void *opaque, uint32_t address)
-{
-    uint32_t data;
-    data = ioport_read(0, address);
-    address = (address + 1) & IOPORTS_MASK;
-    data |= ioport_read(0, address) << 8;
-    return data;
-}
-
-static void default_ioport_writew(void *opaque, uint32_t address, uint32_t data)
-{
-    ioport_write(0, address, data & 0xff);
-    address = (address + 1) & IOPORTS_MASK;
-    ioport_write(0, address, (data >> 8) & 0xff);
-}
-
-static uint32_t default_ioport_readl(void *opaque, uint32_t address)
-{
-    LOG_UNUSED_IOPORT("unused inl: port=0x%04"PRIx32"\n", address);
-    return 0xffffffff;
-}
-
-static void default_ioport_writel(void *opaque, uint32_t address, uint32_t data)
-{
-    LOG_UNUSED_IOPORT("unused outl: port=0x%04"PRIx32" data=0x%02"PRIx32"\n",
-                      address, data);
-}
-
-static int ioport_bsize(int size, int *bsize)
-{
-    if (size == 1) {
-        *bsize = 0;
-    } else if (size == 2) {
-        *bsize = 1;
-    } else if (size == 4) {
-        *bsize = 2;
-    } else {
-        return -1;
-    }
-    return 0;
-}
-
-/* size is the word size in byte */
-static int register_ioport_read(pio_addr_t start, int length, int size,
-                                IOPortReadFunc *func, void *opaque)
-{
-    int i, bsize;
-
-    if (ioport_bsize(size, &bsize)) {
-        hw_error("register_ioport_read: invalid size");
-        return -1;
-    }
-    for(i = start; i < start + length; ++i) {
-        ioport_read_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
-            hw_error("register_ioport_read: invalid opaque for address 0x%x",
-                     i);
-        ioport_opaque[i] = opaque;
-    }
-    return 0;
-}
-
-/* size is the word size in byte */
-static int register_ioport_write(pio_addr_t start, int length, int size,
-                                 IOPortWriteFunc *func, void *opaque)
-{
-    int i, bsize;
-
-    if (ioport_bsize(size, &bsize)) {
-        hw_error("register_ioport_write: invalid size");
-        return -1;
-    }
-    for(i = start; i < start + length; ++i) {
-        ioport_write_table[bsize][i] = func;
-        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
-            hw_error("register_ioport_write: invalid opaque for address 0x%x",
-                     i);
-        ioport_opaque[i] = opaque;
-    }
-    return 0;
-}
-
-static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
-{
-    IORange *ioport = opaque;
-    uint64_t data;
-
-    ioport->ops->read(ioport, addr - ioport->base, 1, &data);
-    return data;
-}
-
-static uint32_t ioport_readw_thunk(void *opaque, uint32_t addr)
-{
-    IORange *ioport = opaque;
-    uint64_t data;
-
-    ioport->ops->read(ioport, addr - ioport->base, 2, &data);
-    return data;
-}
-
-static uint32_t ioport_readl_thunk(void *opaque, uint32_t addr)
-{
-    IORange *ioport = opaque;
-    uint64_t data;
-
-    ioport->ops->read(ioport, addr - ioport->base, 4, &data);
-    return data;
-}
-
-static void ioport_writeb_thunk(void *opaque, uint32_t addr, uint32_t data)
-{
-    IORange *ioport = opaque;
-
-    ioport->ops->write(ioport, addr - ioport->base, 1, data);
-}
-
-static void ioport_writew_thunk(void *opaque, uint32_t addr, uint32_t data)
-{
-    IORange *ioport = opaque;
-
-    ioport->ops->write(ioport, addr - ioport->base, 2, data);
-}
-
-static void ioport_writel_thunk(void *opaque, uint32_t addr, uint32_t data)
-{
-    IORange *ioport = opaque;
-
-    ioport->ops->write(ioport, addr - ioport->base, 4, data);
-}
-
-static void iorange_destructor_thunk(void *opaque)
-{
-    IORange *iorange = opaque;
-
-    if (iorange->ops->destructor) {
-        iorange->ops->destructor(iorange);
-    }
-}
-
-void ioport_register(IORange *ioport)
-{
-    register_ioport_read(ioport->base, ioport->len, 1,
-                         ioport_readb_thunk, ioport);
-    register_ioport_read(ioport->base, ioport->len, 2,
-                         ioport_readw_thunk, ioport);
-    register_ioport_read(ioport->base, ioport->len, 4,
-                         ioport_readl_thunk, ioport);
-    register_ioport_write(ioport->base, ioport->len, 1,
-                          ioport_writeb_thunk, ioport);
-    register_ioport_write(ioport->base, ioport->len, 2,
-                          ioport_writew_thunk, ioport);
-    register_ioport_write(ioport->base, ioport->len, 4,
-                          ioport_writel_thunk, ioport);
-    ioport_destructor_table[ioport->base] = iorange_destructor_thunk;
-}
-
-void isa_unassign_ioport(pio_addr_t start, int length)
-{
-    int i;
-
-    if (ioport_destructor_table[start]) {
-        ioport_destructor_table[start](ioport_opaque[start]);
-        ioport_destructor_table[start] = NULL;
-    }
-    for(i = start; i < start + length; i++) {
-        ioport_read_table[0][i] = NULL;
-        ioport_read_table[1][i] = NULL;
-        ioport_read_table[2][i] = NULL;
-
-        ioport_write_table[0][i] = NULL;
-        ioport_write_table[1][i] = NULL;
-        ioport_write_table[2][i] = NULL;
-
-        ioport_opaque[i] = NULL;
-    }
-}
-
-/***********************************************************/
-
 void cpu_outb(pio_addr_t addr, uint8_t val)
 {
     LOG_IOPORT("outb: %04"FMT_pioaddr" %02"PRIx8"\n", addr, val);
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 13/14] vmport: Disentangle read handler type from portio

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

In case the latter may vanish one day, make sure the vmport read handler
type will remain unaffected. This is also conceptually cleaner.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/misc/vmport.c     |    4 ++--
 include/hw/i386/pc.h |    6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 57b71f5..eb9059e 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -43,13 +43,13 @@ typedef struct VMPortState
     ISADevice parent_obj;
 
     MemoryRegion io;
-    IOPortReadFunc *func[VMPORT_ENTRIES];
+    VMPortReadFunc *func[VMPORT_ENTRIES];
     void *opaque[VMPORT_ENTRIES];
 } VMPortState;
 
 static VMPortState *port_state;
 
-void vmport_register(unsigned char command, IOPortReadFunc *func, void *opaque)
+void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
 {
     if (command >= VMPORT_ENTRIES)
         return;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7f04967..3a89ce3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -3,7 +3,6 @@
 
 #include "qemu-common.h"
 #include "exec/memory.h"
-#include "exec/ioport.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "net/net.h"
@@ -56,11 +55,14 @@ typedef struct GSIState {
 void gsi_handler(void *opaque, int n, int level);
 
 /* vmport.c */
+typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
+
 static inline void vmport_init(ISABus *bus)
 {
     isa_create_simple(bus, "vmport");
 }
-void vmport_register(unsigned char command, IOPortReadFunc *func, void *opaque);
+
+void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
 void vmmouse_get_data(uint32_t *data);
 void vmmouse_set_data(const uint32_t *data);
 
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 14/14] ioport: Move portio types to ioport.h

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This decouples memory.h from ioport.h, concentrating all portio related
types in a single header.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/exec/ioport.h |   17 +++++++++++------
 include/exec/memory.h |   13 -------------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index ba3ebb8..6fb237d 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -25,6 +25,7 @@
 #define IOPORT_H
 
 #include "qemu-common.h"
+#include "exec/memory.h"
 
 typedef uint32_t pio_addr_t;
 #define FMT_pioaddr     PRIx32
@@ -32,9 +33,16 @@ typedef uint32_t pio_addr_t;
 #define MAX_IOPORTS     (64 * 1024)
 #define IOPORTS_MASK    (MAX_IOPORTS - 1)
 
-/* These should really be in isa.h, but are here to make pc.h happy.  */
-typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data);
-typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
+typedef struct MemoryRegionPortio {
+    uint32_t offset;
+    uint32_t len;
+    unsigned size;
+    uint32_t (*read)(void *opaque, uint32_t address);
+    void (*write)(void *opaque, uint32_t address, uint32_t data);
+    uint32_t base; /* private field */
+} MemoryRegionPortio;
+
+#define PORTIO_END_OF_LIST() { }
 
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
@@ -43,9 +51,6 @@ uint8_t cpu_inb(pio_addr_t addr);
 uint16_t cpu_inw(pio_addr_t addr);
 uint32_t cpu_inl(pio_addr_t addr);
 
-struct MemoryRegion;
-struct MemoryRegionPortio;
-
 typedef struct PortioList {
     const struct MemoryRegionPortio *ports;
     struct MemoryRegion *address_space;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8c1373e..84b3eef 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -22,7 +22,6 @@
 #include "exec/cpu-common.h"
 #include "exec/hwaddr.h"
 #include "qemu/queue.h"
-#include "exec/ioport.h"
 #include "qemu/int128.h"
 #include "qemu/notify.h"
 
@@ -30,7 +29,6 @@
 #define MAX_PHYS_ADDR            (((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 1)
 
 typedef struct MemoryRegionOps MemoryRegionOps;
-typedef struct MemoryRegionPortio MemoryRegionPortio;
 typedef struct MemoryRegionMmio MemoryRegionMmio;
 
 /* Must match *_DIRTY_FLAGS in cpu-all.h.  To be replaced with dynamic
@@ -164,17 +162,6 @@ struct MemoryRegion {
     NotifierList iommu_notify;
 };
 
-struct MemoryRegionPortio {
-    uint32_t offset;
-    uint32_t len;
-    unsigned size;
-    IOPortReadFunc *read;
-    IOPortWriteFunc *write;
-    uint32_t base; /* private field */
-};
-
-#define PORTIO_END_OF_LIST() { }
-
 /**
  * AddressSpace: describes a mapping of addresses to #MemoryRegion objects
  */
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH v3 00/14] Refactor portio dispatching

[Hervé Poussineau]

Jan Kiszka a écrit :
> Changes in v3:
>  - decouple vmport from portio types
>  - removed portio traces from memory.h, consolidating it in ioport.h
> 
> CC: Jan Kiszka <jan.kiszka@siemens.com>
> CC: malc <av1474@comtv.ru>
> 
> Jan Kiszka (14):
>   adlib: replace register_ioport*
>   applesmc: replace register_ioport*
>   wdt_ib700: replace register_ioport*
>   i82374: replace register_ioport*
>   prep: replace register_ioport*
>   vt82c686: replace register_ioport*
>   Privatize register_ioport_read/write
>   isa: implement isa_is_ioport_assigned via memory_region_find
>   vmware-vga: Accept unaligned I/O accesses
>   xen: Mark fixed platform I/O as unaligned
>   ioport: Switch dispatching to memory core layer
>   ioport: Remove unused old dispatching services
>   vmport: Disentangle read handler type from portio
>   ioport: Move portio types to ioport.h

The whole series:
Tested-by: Hervé Poussineau <hpoussin@reactos.org>