[Qemu-devel] [RFC][PATCH 00/15] Refactor portio dispatching

[Qemu-devel] [RFC][PATCH 00/15] Refactor portio dispatching

[Jan Kiszka]

This series converts the remaining users of register_ioport* to portio
lists, simplifies the handling of subpages and adds support for unaligned
memory region accesses. Then it replaces the current portio dispatcher
with the existing one for MMIO and removes several lines of code. This
also allows to build BQL-free portio on top once we enhance the memory
layer accordingly.

Seems to work fine so far but surely requires thorough review. And I
would welcome early comments on the direction.

Jan


CC: malc <av1474@comtv.ru>

Jan Kiszka (15):
  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
  memory: Introduce address_space_lookup_region
  memory: Rework sub-page handling
  memory: Allow unaligned address_space_rw
  vmware-vga: Accept unaligned I/O accesses
  ioport: Switch dispatching to memory core layer
  ioport: Remove unused old dispatching services
  ioport: Move IOPortRead/WriteFunc typedefs to memory.h

 cputlb.c                       |    2 +-
 exec.c                         |  273 +++++++++++++------------------------
 hw/acpi/piix4.c                |    6 +-
 hw/audio/adlib.c               |   20 ++-
 hw/display/vmware_vga.c        |    4 +
 hw/dma/i82374.c                |   17 ++-
 hw/isa/isa-bus.c               |   11 ++
 hw/isa/lpc_ich9.c              |    8 +-
 hw/isa/vt82c686.c              |   40 ++++--
 hw/misc/applesmc.c             |   48 +++++--
 hw/ppc/prep.c                  |   23 ++-
 hw/watchdog/wdt_ib700.c        |   12 ++-
 include/exec/cputlb.h          |    2 -
 include/exec/ioport.h          |   19 +---
 include/exec/iorange.h         |   31 ----
 include/exec/memory-internal.h |    2 -
 include/exec/memory.h          |   26 ++--
 include/hw/isa/isa.h           |    2 +
 ioport.c                       |  294 ++++------------------------------------
 memory.c                       |  102 +++++---------
 translate-all.c                |    3 +-
 21 files changed, 311 insertions(+), 634 deletions(-)
 delete mode 100644 include/exec/iorange.h

-- 
1.7.3.4

[Qemu-devel] [RFC][PATCH 01/15] adlib: replace register_ioport*

[Jan Kiszka]

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 fc20857..9a27c01 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 int Adlib_initfn (ISADevice *dev)
 {
     AdlibState *s = ADLIB(dev);
+    PortioList *port_list = g_new(PortioList, 1);
     struct audsettings as;
 
     if (glob_adlib) {
@@ -338,14 +346,10 @@ static int Adlib_initfn (ISADevice *dev)
     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(dev), 0);
 
     return 0;
 }
-- 
1.7.3.4

[Qemu-devel] [RFC][PATCH 02/15] applesmc: replace register_ioport*

[Jan Kiszka]

Convert over to memory regions to obsolete register_ioport*.

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

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 78904a8..af24be1 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,37 @@ 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 int applesmc_isa_init(ISADevice *dev)
 {
     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(dev, &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(dev, &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] [RFC][PATCH 03/15] wdt_ib700: replace register_ioport*

[Jan Kiszka]

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 6b8e33a..8b0fb4b 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 int wdt_ib700_init(ISADevice *dev)
 {
     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(dev), 0);
 
     return 0;
 }
-- 
1.7.3.4

[Qemu-devel] [RFC][PATCH 04/15] i82374: replace register_ioport*

[Jan Kiszka]

Convert over to memory regions to obsolete register_ioport*.

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

diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index f3d1924..3cc9aab 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -124,16 +124,23 @@ 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 int i82374_isa_init(ISADevice *dev)
 {
     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(dev), isa->iobase);
 
     i82374_init(s);
 
-- 
1.7.3.4

[Qemu-devel] [RFC][PATCH 05/15] prep: replace register_ioport*

[Jan Kiszka]

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 59c7da3..671dc26 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -429,6 +429,16 @@ static void ppc_prep_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
+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)
 {
@@ -445,6 +455,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
@@ -624,11 +635,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);
@@ -655,9 +665,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

Re: [Qemu-devel] [RFC][PATCH 05/15] prep: replace register_ioport*

[Andreas Färber]

Am 06.05.2013 16:26, schrieb Jan Kiszka:
> 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(-)

As a heads-up, for PReP we've been preparing to move this System I/O to
a qdev/QOM device, so this would likely need rebasing at some point.

Andreas

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

[Qemu-devel] [RFC][PATCH 06/15] vt82c686: replace register_ioport*

[Jan Kiszka]

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 5261927..c0d9919 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] [RFC][PATCH 07/15] Privatize register_ioport_read/write

[Jan Kiszka]

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] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Jan Kiszka]

Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
 hw/isa/isa-bus.c      |   11 +++++++++++
 hw/isa/lpc_ich9.c     |    8 ++++----
 include/exec/ioport.h |    1 -
 include/hw/isa/isa.h  |    2 ++
 ioport.c              |    7 -------
 6 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index c4af1cc..5955217 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
     uint8_t *pci_conf;
 
     pci_conf = s->dev.config;
-    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
+    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
     pci_conf[0x63] = 0x60;
-    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
-	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
+    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
+        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
 
 }
 
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 7860b17..598dd86 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -279,4 +279,15 @@ MemoryRegion *isa_address_space_io(ISADevice *dev)
     return isabus->address_space_io;
 }
 
+bool isa_is_ioport_assigned(ISABus *bus, pio_addr_t start)
+{
+    if (!bus) {
+        bus = isabus;
+    }
+    if (!bus) {
+        hw_error("No isa bus present.");
+    }
+    return memory_region_find(bus->address_space_io, start, 1).mr != NULL;
+}
+
 type_init(isabus_register_types)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 667e882..641227a 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -480,19 +480,19 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
     uint8_t *pci_conf;
 
     pci_conf = s->d.config;
-    if (isa_is_ioport_assigned(0x3f8)) {
+    if (isa_is_ioport_assigned(s->isa_bus, 0x3f8)) {
         /* com1 */
         pci_conf[0x82] |= 0x01;
     }
-    if (isa_is_ioport_assigned(0x2f8)) {
+    if (isa_is_ioport_assigned(s->isa_bus, 0x2f8)) {
         /* com2 */
         pci_conf[0x82] |= 0x02;
     }
-    if (isa_is_ioport_assigned(0x378)) {
+    if (isa_is_ioport_assigned(s->isa_bus, 0x378)) {
         /* lpt */
         pci_conf[0x82] |= 0x04;
     }
-    if (isa_is_ioport_assigned(0x3f0)) {
+    if (isa_is_ioport_assigned(s->isa_bus, 0x3f0)) {
         /* 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/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 82da37c..27fb51e 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -80,6 +80,8 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,
                               const MemoryRegionPortio *portio,
                               void *opaque, const char *name);
 
+bool isa_is_ioport_assigned(ISABus *bus, pio_addr_t start);
+
 static inline ISABus *isa_bus_from_device(ISADevice *d)
 {
     return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
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

Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Andreas Färber]

Am 06.05.2013 16:26, schrieb Jan Kiszka:
> Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
>  hw/isa/isa-bus.c      |   11 +++++++++++
>  hw/isa/lpc_ich9.c     |    8 ++++----
>  include/exec/ioport.h |    1 -
>  include/hw/isa/isa.h  |    2 ++
>  ioport.c              |    7 -------
>  6 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index c4af1cc..5955217 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>      uint8_t *pci_conf;
>  
>      pci_conf = s->dev.config;
> -    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
> +    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
>      pci_conf[0x63] = 0x60;
> -    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
> -	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
> +    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
> +        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
>  
>  }
>  

Is there really no way to access the ISABus from this device? Would be
nice to get rid of global ISA variables and not introduce more
dependencies. :)

Andreas

[...]
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 667e882..641227a 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -480,19 +480,19 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>      uint8_t *pci_conf;
>  
>      pci_conf = s->d.config;
> -    if (isa_is_ioport_assigned(0x3f8)) {
> +    if (isa_is_ioport_assigned(s->isa_bus, 0x3f8)) {
>          /* com1 */
>          pci_conf[0x82] |= 0x01;
>      }
> -    if (isa_is_ioport_assigned(0x2f8)) {
> +    if (isa_is_ioport_assigned(s->isa_bus, 0x2f8)) {
>          /* com2 */
>          pci_conf[0x82] |= 0x02;
>      }
> -    if (isa_is_ioport_assigned(0x378)) {
> +    if (isa_is_ioport_assigned(s->isa_bus, 0x378)) {
>          /* lpt */
>          pci_conf[0x82] |= 0x04;
>      }
> -    if (isa_is_ioport_assigned(0x3f0)) {
> +    if (isa_is_ioport_assigned(s->isa_bus, 0x3f0)) {
>          /* floppy */
>          pci_conf[0x82] |= 0x08;
>      }
[snip]

-- 
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] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Paolo Bonzini]

Il 06/05/2013 16:55, Andreas Färber ha scritto:
> Am 06.05.2013 16:26, schrieb Jan Kiszka:
>> Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
>>  hw/isa/isa-bus.c      |   11 +++++++++++
>>  hw/isa/lpc_ich9.c     |    8 ++++----
>>  include/exec/ioport.h |    1 -
>>  include/hw/isa/isa.h  |    2 ++
>>  ioport.c              |    7 -------
>>  6 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index c4af1cc..5955217 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>      uint8_t *pci_conf;
>>  
>>      pci_conf = s->dev.config;
>> -    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
>> +    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
>>      pci_conf[0x63] = 0x60;
>> -    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
>> -	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
>> +    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
>> +        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
>>  
>>  }
>>  
> 
> Is there really no way to access the ISABus from this device? Would be
> nice to get rid of global ISA variables and not introduce more
> dependencies. :)

There's always a way to find the ISABus via QOM:

    ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);

Paolo

Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Jan Kiszka]

On 2013-05-06 16:59, Paolo Bonzini wrote:
> Il 06/05/2013 16:55, Andreas Färber ha scritto:
>> Am 06.05.2013 16:26, schrieb Jan Kiszka:
>>> Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
>>>  hw/isa/isa-bus.c      |   11 +++++++++++
>>>  hw/isa/lpc_ich9.c     |    8 ++++----
>>>  include/exec/ioport.h |    1 -
>>>  include/hw/isa/isa.h  |    2 ++
>>>  ioport.c              |    7 -------
>>>  6 files changed, 20 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>> index c4af1cc..5955217 100644
>>> --- a/hw/acpi/piix4.c
>>> +++ b/hw/acpi/piix4.c
>>> @@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>>      uint8_t *pci_conf;
>>>  
>>>      pci_conf = s->dev.config;
>>> -    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
>>> +    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
>>>      pci_conf[0x63] = 0x60;
>>> -    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
>>> -	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
>>> +    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
>>> +        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
>>>  
>>>  }
>>>  
>>
>> Is there really no way to access the ISABus from this device? Would be
>> nice to get rid of global ISA variables and not introduce more
>> dependencies. :)
> 
> There's always a way to find the ISABus via QOM:
> 
>     ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);

Err, in what way is this better? It also assumes that there is only one.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Paolo Bonzini]

Il 06/05/2013 17:02, Jan Kiszka ha scritto:
> On 2013-05-06 16:59, Paolo Bonzini wrote:
>> Il 06/05/2013 16:55, Andreas Färber ha scritto:
>>> Am 06.05.2013 16:26, schrieb Jan Kiszka:
>>>> Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
>>>>  hw/isa/isa-bus.c      |   11 +++++++++++
>>>>  hw/isa/lpc_ich9.c     |    8 ++++----
>>>>  include/exec/ioport.h |    1 -
>>>>  include/hw/isa/isa.h  |    2 ++
>>>>  ioport.c              |    7 -------
>>>>  6 files changed, 20 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>> index c4af1cc..5955217 100644
>>>> --- a/hw/acpi/piix4.c
>>>> +++ b/hw/acpi/piix4.c
>>>> @@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>>>      uint8_t *pci_conf;
>>>>  
>>>>      pci_conf = s->dev.config;
>>>> -    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
>>>> +    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
>>>>      pci_conf[0x63] = 0x60;
>>>> -    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
>>>> -	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
>>>> +    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
>>>> +        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
>>>>  
>>>>  }
>>>>  
>>>
>>> Is there really no way to access the ISABus from this device? Would be
>>> nice to get rid of global ISA variables and not introduce more
>>> dependencies. :)
>>
>> There's always a way to find the ISABus via QOM:
>>
>>     ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);
> 
> Err, in what way is this better? It also assumes that there is only one.

I didn't say it is better. :)  Unfortunately, the PIIX4 has these
register on the "wrong" function, ICH9 fixed it.

You could make this take an AddressSpace instead of an ISABus, and use
pci_address_space_io.  The assumption then becomes that the ISA and PM
devices have the same address spaces, which is somewhat hackish but
reasonable.

In fact, with the other new API I have added (address_space_valid), this
would become address_space_valid(pci_address_space_io(dev), 0x2f8, 1)
and similar.  I need to check, but you could remove
isa_is_ioport_assigned completely.

Paolo

Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find

[Jan Kiszka]

On 2013-05-06 16:55, Andreas Färber wrote:
> Am 06.05.2013 16:26, schrieb Jan Kiszka:
>> Move isa_is_ioport_assigned to the ISA core and implement it 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       |    6 +++---
>>  hw/isa/isa-bus.c      |   11 +++++++++++
>>  hw/isa/lpc_ich9.c     |    8 ++++----
>>  include/exec/ioport.h |    1 -
>>  include/hw/isa/isa.h  |    2 ++
>>  ioport.c              |    7 -------
>>  6 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index c4af1cc..5955217 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>      uint8_t *pci_conf;
>>  
>>      pci_conf = s->dev.config;
>> -    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
>> +    pci_conf[0x5f] = (isa_is_ioport_assigned(NULL, 0x378) ? 0x80 : 0) | 0x10;
>>      pci_conf[0x63] = 0x60;
>> -    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
>> -	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
>> +    pci_conf[0x67] = (isa_is_ioport_assigned(NULL, 0x3f8) ? 0x08 : 0) |
>> +        (isa_is_ioport_assigned(NULL, 0x2f8) ? 0x90 : 0);
>>  
>>  }
>>  
> 
> Is there really no way to access the ISABus from this device? Would be
> nice to get rid of global ISA variables and not introduce more
> dependencies. :)

There is likely a way, just didn't find a direct one. So I prefer to
limit the scope and leave such cleanups for later. Keep in mind that the
existing API had an implicit NULL device, so this is no regression!

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region

[Jan Kiszka]

This new service so far only replaces phys_page_find as public API. In a
follow-up step, it will return the effective memory region for the
specified address, i.e. after resolving what are currently sub-pages.
Moreover, it will also once encapsulate locking and reference counting
when we introduce BQL-free dispatching.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cputlb.c              |    2 +-
 exec.c                |   46 +++++++++++++++++++++-------------------------
 include/exec/cputlb.h |    2 --
 include/exec/memory.h |    9 +++++++++
 translate-all.c       |    3 +--
 5 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index aba7e44..e2c95c1 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
     if (size != TARGET_PAGE_SIZE) {
         tlb_add_large_page(env, vaddr, size);
     }
-    section = phys_page_find(address_space_memory.dispatch, paddr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, paddr);
 #if defined(DEBUG_TLB)
     printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
            " prot=%x idx=%d pd=0x%08lx\n",
diff --git a/exec.c b/exec.c
index 19725db..53c2778 100644
--- a/exec.c
+++ b/exec.c
@@ -182,7 +182,8 @@ static void phys_page_set(AddressSpaceDispatch *d,
     phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
 }
 
-MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
+static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d,
+                                           hwaddr index)
 {
     PhysPageEntry lp = d->phys_map;
     PhysPageEntry *p;
@@ -1894,19 +1895,16 @@ static void invalidate_and_set_dirty(hwaddr addr,
 void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                       int len, bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
     int l;
     uint8_t *ptr;
     uint32_t val;
-    hwaddr page;
     MemoryRegionSection *section;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
+        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        section = address_space_lookup_region(as, addr);
 
         if (is_write) {
             if (!memory_region_is_ram(section->mr)) {
@@ -2006,18 +2004,15 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
 void cpu_physical_memory_write_rom(hwaddr addr,
                                    const uint8_t *buf, int len)
 {
-    AddressSpaceDispatch *d = address_space_memory.dispatch;
     int l;
     uint8_t *ptr;
-    hwaddr page;
     MemoryRegionSection *section;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
+        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        section = address_space_lookup_region(&address_space_memory, addr);
 
         if (!(memory_region_is_ram(section->mr) ||
               memory_region_is_romd(section->mr))) {
@@ -2096,22 +2091,19 @@ void *address_space_map(AddressSpace *as,
                         hwaddr *plen,
                         bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
     hwaddr len = *plen;
     hwaddr todo = 0;
     int l;
-    hwaddr page;
     MemoryRegionSection *section;
     ram_addr_t raddr = RAM_ADDR_MAX;
     ram_addr_t rlen;
     void *ret;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
+        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        section = address_space_lookup_region(as, addr);
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
             if (todo || bounce.buffer) {
@@ -2188,6 +2180,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
     return address_space_unmap(&address_space_memory, buffer, len, is_write, access_len);
 }
 
+MemoryRegionSection *address_space_lookup_region(AddressSpace *as, hwaddr addr)
+{
+    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
+}
+
 /* warning: addr must be aligned */
 static inline uint32_t ldl_phys_internal(hwaddr addr,
                                          enum device_endian endian)
@@ -2196,7 +2193,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
     uint32_t val;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
@@ -2255,7 +2252,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
     uint64_t val;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
@@ -2322,7 +2319,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
     uint64_t val;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
@@ -2381,7 +2378,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
     uint8_t *ptr;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
@@ -2413,7 +2410,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
     uint8_t *ptr;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
@@ -2442,7 +2439,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
     uint8_t *ptr;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
@@ -2509,7 +2506,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
     uint8_t *ptr;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
@@ -2634,8 +2631,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch,
-                             phys_addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, phys_addr);
 
     return !(memory_region_is_ram(section->mr) ||
              memory_region_is_romd(section->mr));
diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
index 733c885..f144e6e 100644
--- a/include/exec/cputlb.h
+++ b/include/exec/cputlb.h
@@ -26,8 +26,6 @@ void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr,
                              target_ulong vaddr);
 void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
                            uintptr_t length);
-MemoryRegionSection *phys_page_find(struct AddressSpaceDispatch *d,
-                                    hwaddr index);
 void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length);
 void tlb_set_dirty(CPUArchState *env, target_ulong vaddr);
 extern int tlb_flush_count;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9e88320..11ca4e2 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -887,6 +887,15 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
 void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len);
 
+/**
+ * address_space_lookup_region: Looks up memory region corresponding to given
+ *                              access address
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ */
+MemoryRegionSection *address_space_lookup_region(AddressSpace *as,
+                                                 hwaddr addr);
 
 #endif
 
diff --git a/translate-all.c b/translate-all.c
index da93608..078a657 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1356,8 +1356,7 @@ void tb_invalidate_phys_addr(hwaddr addr)
     ram_addr_t ram_addr;
     MemoryRegionSection *section;
 
-    section = phys_page_find(address_space_memory.dispatch,
-                             addr >> TARGET_PAGE_BITS);
+    section = address_space_lookup_region(&address_space_memory, addr);
     if (!(memory_region_is_ram(section->mr)
           || (section->mr->rom_device && section->mr->readable))) {
         return;
-- 
1.7.3.4

Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region

[Paolo Bonzini]

Il 06/05/2013 16:26, Jan Kiszka ha scritto:
> This new service so far only replaces phys_page_find as public API. In a
> follow-up step, it will return the effective memory region for the
> specified address, i.e. after resolving what are currently sub-pages.
> Moreover, it will also once encapsulate locking and reference counting
> when we introduce BQL-free dispatching.

In my IOMMU rebase I have a similar function:

/* address_space_translate: translate an address range into an address space
 * into a MemoryRegionSection and an address range into that section.
 *
 * @as: #AddressSpace to be accessed
 * @addr: address within that address space
 * @xlat: pointer to address within the returned memory region section's
 * #MemoryRegion.
 * @len: pointer to length
 * @is_write: indicates the transfer direction
 */
MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
                                             hwaddr *xlat, hwaddr *len,
                                             bool is_write);

It wraps (actually, replaces) both phys_page_find and
memory_region_section_addr.

Paolo

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  cputlb.c              |    2 +-
>  exec.c                |   46 +++++++++++++++++++++-------------------------
>  include/exec/cputlb.h |    2 --
>  include/exec/memory.h |    9 +++++++++
>  translate-all.c       |    3 +--
>  5 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index aba7e44..e2c95c1 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>      if (size != TARGET_PAGE_SIZE) {
>          tlb_add_large_page(env, vaddr, size);
>      }
> -    section = phys_page_find(address_space_memory.dispatch, paddr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, paddr);
>  #if defined(DEBUG_TLB)
>      printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
>             " prot=%x idx=%d pd=0x%08lx\n",
> diff --git a/exec.c b/exec.c
> index 19725db..53c2778 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -182,7 +182,8 @@ static void phys_page_set(AddressSpaceDispatch *d,
>      phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
>  }
>  
> -MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d,
> +                                           hwaddr index)
>  {
>      PhysPageEntry lp = d->phys_map;
>      PhysPageEntry *p;
> @@ -1894,19 +1895,16 @@ static void invalidate_and_set_dirty(hwaddr addr,
>  void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                        int len, bool is_write)
>  {
> -    AddressSpaceDispatch *d = as->dispatch;
>      int l;
>      uint8_t *ptr;
>      uint32_t val;
> -    hwaddr page;
>      MemoryRegionSection *section;
>  
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(as, addr);
>  
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
> @@ -2006,18 +2004,15 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>  void cpu_physical_memory_write_rom(hwaddr addr,
>                                     const uint8_t *buf, int len)
>  {
> -    AddressSpaceDispatch *d = address_space_memory.dispatch;
>      int l;
>      uint8_t *ptr;
> -    hwaddr page;
>      MemoryRegionSection *section;
>  
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(&address_space_memory, addr);
>  
>          if (!(memory_region_is_ram(section->mr) ||
>                memory_region_is_romd(section->mr))) {
> @@ -2096,22 +2091,19 @@ void *address_space_map(AddressSpace *as,
>                          hwaddr *plen,
>                          bool is_write)
>  {
> -    AddressSpaceDispatch *d = as->dispatch;
>      hwaddr len = *plen;
>      hwaddr todo = 0;
>      int l;
> -    hwaddr page;
>      MemoryRegionSection *section;
>      ram_addr_t raddr = RAM_ADDR_MAX;
>      ram_addr_t rlen;
>      void *ret;
>  
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(as, addr);
>  
>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
>              if (todo || bounce.buffer) {
> @@ -2188,6 +2180,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
>      return address_space_unmap(&address_space_memory, buffer, len, is_write, access_len);
>  }
>  
> +MemoryRegionSection *address_space_lookup_region(AddressSpace *as, hwaddr addr)
> +{
> +    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
> +}
> +
>  /* warning: addr must be aligned */
>  static inline uint32_t ldl_phys_internal(hwaddr addr,
>                                           enum device_endian endian)
> @@ -2196,7 +2193,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>      uint32_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2255,7 +2252,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>      uint64_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2322,7 +2319,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>      uint64_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2381,7 +2378,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2413,7 +2410,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2442,7 +2439,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2509,7 +2506,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2634,8 +2631,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
>  {
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             phys_addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, phys_addr);
>  
>      return !(memory_region_is_ram(section->mr) ||
>               memory_region_is_romd(section->mr));
> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
> index 733c885..f144e6e 100644
> --- a/include/exec/cputlb.h
> +++ b/include/exec/cputlb.h
> @@ -26,8 +26,6 @@ void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr,
>                               target_ulong vaddr);
>  void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
>                             uintptr_t length);
> -MemoryRegionSection *phys_page_find(struct AddressSpaceDispatch *d,
> -                                    hwaddr index);
>  void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length);
>  void tlb_set_dirty(CPUArchState *env, target_ulong vaddr);
>  extern int tlb_flush_count;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 9e88320..11ca4e2 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -887,6 +887,15 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len);
>  
> +/**
> + * address_space_lookup_region: Looks up memory region corresponding to given
> + *                              access address
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + */
> +MemoryRegionSection *address_space_lookup_region(AddressSpace *as,
> +                                                 hwaddr addr);
>  
>  #endif
>  
> diff --git a/translate-all.c b/translate-all.c
> index da93608..078a657 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1356,8 +1356,7 @@ void tb_invalidate_phys_addr(hwaddr addr)
>      ram_addr_t ram_addr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>      if (!(memory_region_is_ram(section->mr)
>            || (section->mr->rom_device && section->mr->readable))) {
>          return;
> 

Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region

[Jan Kiszka]

On 2013-05-06 16:39, Paolo Bonzini wrote:
> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>> This new service so far only replaces phys_page_find as public API. In a
>> follow-up step, it will return the effective memory region for the
>> specified address, i.e. after resolving what are currently sub-pages.
>> Moreover, it will also once encapsulate locking and reference counting
>> when we introduce BQL-free dispatching.
> 
> In my IOMMU rebase I have a similar function:
> 
> /* address_space_translate: translate an address range into an address space
>  * into a MemoryRegionSection and an address range into that section.
>  *
>  * @as: #AddressSpace to be accessed
>  * @addr: address within that address space
>  * @xlat: pointer to address within the returned memory region section's
>  * #MemoryRegion.
>  * @len: pointer to length
>  * @is_write: indicates the transfer direction
>  */
> MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
>                                              hwaddr *xlat, hwaddr *len,
>                                              bool is_write);
> 
> It wraps (actually, replaces) both phys_page_find and
> memory_region_section_addr.

Good, looks like we are heading in similar directions. What is the
purpose of len? When does is_write matter?

In a later step, this should become something like
address_space_get_region_ref (to be paired with memory_region_unref,
once done). So this one also takes care of incrementing the reference
counter or acquiring the BQL, as necessary. Currently, it asks the
caller to specify if the BQL is already held, but that will change.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region

[Paolo Bonzini]

Il 06/05/2013 16:51, Jan Kiszka ha scritto:
> On 2013-05-06 16:39, Paolo Bonzini wrote:
>> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>>> This new service so far only replaces phys_page_find as public API. In a
>>> follow-up step, it will return the effective memory region for the
>>> specified address, i.e. after resolving what are currently sub-pages.
>>> Moreover, it will also once encapsulate locking and reference counting
>>> when we introduce BQL-free dispatching.
>>
>> In my IOMMU rebase I have a similar function:
>>
>> /* address_space_translate: translate an address range into an address space
>>  * into a MemoryRegionSection and an address range into that section.
>>  *
>>  * @as: #AddressSpace to be accessed
>>  * @addr: address within that address space
>>  * @xlat: pointer to address within the returned memory region section's
>>  * #MemoryRegion.
>>  * @len: pointer to length
>>  * @is_write: indicates the transfer direction
>>  */
>> MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
>>                                              hwaddr *xlat, hwaddr *len,
>>                                              bool is_write);
>>
>> It wraps (actually, replaces) both phys_page_find and
>> memory_region_section_addr.
> 
> Good, looks like we are heading in similar directions. What is the
> purpose of len? When does is_write matter?

Both matter when adding the IOMMU.  is_write is needed to check for
permissions, and len because the translation will be valid for one page
only (not for the full size of the MemoryRegion.

I can implement address_space_translate on top of
address_space_lookup_region and include your next patch too, so we're
fine here.

Paolo

> In a later step, this should become something like
> address_space_get_region_ref (to be paired with memory_region_unref,
> once done). So this one also takes care of incrementing the reference
> counter or acquiring the BQL, as necessary. Currently, it asks the
> caller to specify if the BQL is already held, but that will change.
> 
> Jan
> 

[Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Jan Kiszka]

Simplify the sub-page handling by implementing it directly in the
dispatcher instead of using a redirection memory region. We extend the
phys_sections entries to optionally hold a pointer to the sub-section
table that used to reside in the subpage_t structure. IOW, we add one
optional dispatch level below the existing radix tree.

address_space_lookup_region is extended to take this additional level
into account. This direct dispatching to that target memory region will
also be helpful when we want to add per-region locking control.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                |  175 +++++++++++++++++--------------------------------
 include/exec/memory.h |    1 -
 memory.c              |    1 -
 3 files changed, 59 insertions(+), 118 deletions(-)

diff --git a/exec.c b/exec.c
index 53c2778..3ee1f3f 100644
--- a/exec.c
+++ b/exec.c
@@ -51,7 +51,6 @@
 #include "exec/memory-internal.h"
 
 //#define DEBUG_UNASSIGNED
-//#define DEBUG_SUBPAGE
 
 #if !defined(CONFIG_USER_ONLY)
 int phys_ram_fd;
@@ -82,7 +81,14 @@ int use_icount;
 
 #if !defined(CONFIG_USER_ONLY)
 
-static MemoryRegionSection *phys_sections;
+#define SUBSECTION_IDX(addr) ((addr) & ~TARGET_PAGE_MASK)
+
+typedef struct PhysSection {
+    MemoryRegionSection section;
+    uint16_t *sub_section;
+} PhysSection;
+
+static PhysSection *phys_sections;
 static unsigned phys_sections_nb, phys_sections_nb_alloc;
 static uint16_t phys_section_unassigned;
 static uint16_t phys_section_notdirty;
@@ -182,8 +188,8 @@ static void phys_page_set(AddressSpaceDispatch *d,
     phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
 }
 
-static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d,
-                                           hwaddr index)
+static PhysSection *phys_section_find(AddressSpaceDispatch *d,
+                                      hwaddr index)
 {
     PhysPageEntry lp = d->phys_map;
     PhysPageEntry *p;
@@ -646,7 +652,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
            and avoid full address decoding in every device.
            We can't use the high bits of pd for this because
            IO_MEM_ROMD uses these as a ram address.  */
-        iotlb = section - phys_sections;
+        iotlb = container_of(section, PhysSection, section) - phys_sections;
         iotlb += memory_region_section_addr(section, paddr);
     }
 
@@ -668,27 +674,13 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
 #endif /* defined(CONFIG_USER_ONLY) */
 
 #if !defined(CONFIG_USER_ONLY)
+static int subsection_register(PhysSection *psection, uint32_t start,
+                               uint32_t end, uint16_t section);
+static void subsections_init(PhysSection *psection);
 
-#define SUBPAGE_IDX(addr) ((addr) & ~TARGET_PAGE_MASK)
-typedef struct subpage_t {
-    MemoryRegion iomem;
-    hwaddr base;
-    uint16_t sub_section[TARGET_PAGE_SIZE];
-} subpage_t;
-
-static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
-                             uint16_t section);
-static subpage_t *subpage_init(hwaddr base);
 static void destroy_page_desc(uint16_t section_index)
 {
-    MemoryRegionSection *section = &phys_sections[section_index];
-    MemoryRegion *mr = section->mr;
-
-    if (mr->subpage) {
-        subpage_t *subpage = container_of(mr, subpage_t, iomem);
-        memory_region_destroy(&subpage->iomem);
-        g_free(subpage);
-    }
+    g_free(phys_sections[section_index].sub_section);
 }
 
 static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
@@ -722,10 +714,11 @@ static uint16_t phys_section_add(MemoryRegionSection *section)
 {
     if (phys_sections_nb == phys_sections_nb_alloc) {
         phys_sections_nb_alloc = MAX(phys_sections_nb_alloc * 2, 16);
-        phys_sections = g_renew(MemoryRegionSection, phys_sections,
+        phys_sections = g_renew(PhysSection, phys_sections,
                                 phys_sections_nb_alloc);
     }
-    phys_sections[phys_sections_nb] = *section;
+    phys_sections[phys_sections_nb].section = *section;
+    phys_sections[phys_sections_nb].sub_section = NULL;
     return phys_sections_nb++;
 }
 
@@ -734,31 +727,31 @@ static void phys_sections_clear(void)
     phys_sections_nb = 0;
 }
 
-static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
+static void register_subsection(AddressSpaceDispatch *d,
+                                MemoryRegionSection *section)
 {
-    subpage_t *subpage;
     hwaddr base = section->offset_within_address_space
         & TARGET_PAGE_MASK;
-    MemoryRegionSection *existing = phys_page_find(d, base >> TARGET_PAGE_BITS);
+    PhysSection *psection = phys_section_find(d, base >> TARGET_PAGE_BITS);
     MemoryRegionSection subsection = {
         .offset_within_address_space = base,
         .size = TARGET_PAGE_SIZE,
     };
+    uint16_t new_section;
     hwaddr start, end;
 
-    assert(existing->mr->subpage || existing->mr == &io_mem_unassigned);
+    assert(psection->sub_section ||
+           psection->section.mr == &io_mem_unassigned);
 
-    if (!(existing->mr->subpage)) {
-        subpage = subpage_init(base);
-        subsection.mr = &subpage->iomem;
-        phys_page_set(d, base >> TARGET_PAGE_BITS, 1,
-                      phys_section_add(&subsection));
-    } else {
-        subpage = container_of(existing->mr, subpage_t, iomem);
+    if (!psection->sub_section) {
+        new_section = phys_section_add(&subsection);
+        psection = &phys_sections[new_section];
+        subsections_init(psection);
+        phys_page_set(d, base >> TARGET_PAGE_BITS, 1, new_section);
     }
     start = section->offset_within_address_space & ~TARGET_PAGE_MASK;
     end = start + section->size - 1;
-    subpage_register(subpage, start, end, phys_section_add(section));
+    subsection_register(psection, start, end, phys_section_add(section));
 }
 
 
@@ -786,7 +779,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
         now.size = MIN(TARGET_PAGE_ALIGN(now.offset_within_address_space)
                        - now.offset_within_address_space,
                        now.size);
-        register_subpage(d, &now);
+        register_subsection(d, &now);
         remain.size -= now.size;
         remain.offset_within_address_space += now.size;
         remain.offset_within_region += now.size;
@@ -795,7 +788,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
         now = remain;
         if (remain.offset_within_region & ~TARGET_PAGE_MASK) {
             now.size = TARGET_PAGE_SIZE;
-            register_subpage(d, &now);
+            register_subsection(d, &now);
         } else {
             now.size &= TARGET_PAGE_MASK;
             register_multipage(d, &now);
@@ -806,7 +799,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
     }
     now = remain;
     if (now.size) {
-        register_subpage(d, &now);
+        register_subsection(d, &now);
     }
 }
 
@@ -1556,49 +1549,6 @@ static const MemoryRegionOps watch_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static uint64_t subpage_read(void *opaque, hwaddr addr,
-                             unsigned len)
-{
-    subpage_t *mmio = opaque;
-    unsigned int idx = SUBPAGE_IDX(addr);
-    MemoryRegionSection *section;
-#if defined(DEBUG_SUBPAGE)
-    printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", __func__,
-           mmio, len, addr, idx);
-#endif
-
-    section = &phys_sections[mmio->sub_section[idx]];
-    addr += mmio->base;
-    addr -= section->offset_within_address_space;
-    addr += section->offset_within_region;
-    return io_mem_read(section->mr, addr, len);
-}
-
-static void subpage_write(void *opaque, hwaddr addr,
-                          uint64_t value, unsigned len)
-{
-    subpage_t *mmio = opaque;
-    unsigned int idx = SUBPAGE_IDX(addr);
-    MemoryRegionSection *section;
-#if defined(DEBUG_SUBPAGE)
-    printf("%s: subpage %p len %d addr " TARGET_FMT_plx
-           " idx %d value %"PRIx64"\n",
-           __func__, mmio, len, addr, idx, value);
-#endif
-
-    section = &phys_sections[mmio->sub_section[idx]];
-    addr += mmio->base;
-    addr -= section->offset_within_address_space;
-    addr += section->offset_within_region;
-    io_mem_write(section->mr, addr, value, len);
-}
-
-static const MemoryRegionOps subpage_ops = {
-    .read = subpage_read,
-    .write = subpage_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 static uint64_t subpage_ram_read(void *opaque, hwaddr addr,
                                  unsigned size)
 {
@@ -1631,48 +1581,32 @@ static const MemoryRegionOps subpage_ram_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
-                             uint16_t section)
+static int subsection_register(PhysSection *psection, uint32_t start,
+                               uint32_t end, uint16_t section)
 {
     int idx, eidx;
 
     if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
         return -1;
-    idx = SUBPAGE_IDX(start);
-    eidx = SUBPAGE_IDX(end);
-#if defined(DEBUG_SUBPAGE)
-    printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__,
-           mmio, start, end, idx, eidx, memory);
-#endif
-    if (memory_region_is_ram(phys_sections[section].mr)) {
-        MemoryRegionSection new_section = phys_sections[section];
+    idx = SUBSECTION_IDX(start);
+    eidx = SUBSECTION_IDX(end);
+    if (memory_region_is_ram(phys_sections[section].section.mr)) {
+        MemoryRegionSection new_section = phys_sections[section].section;
         new_section.mr = &io_mem_subpage_ram;
         section = phys_section_add(&new_section);
     }
     for (; idx <= eidx; idx++) {
-        mmio->sub_section[idx] = section;
+        psection->sub_section[idx] = section;
     }
 
     return 0;
 }
 
-static subpage_t *subpage_init(hwaddr base)
+static void subsections_init(PhysSection *psection)
 {
-    subpage_t *mmio;
-
-    mmio = g_malloc0(sizeof(subpage_t));
-
-    mmio->base = base;
-    memory_region_init_io(&mmio->iomem, &subpage_ops, mmio,
-                          "subpage", TARGET_PAGE_SIZE);
-    mmio->iomem.subpage = true;
-#if defined(DEBUG_SUBPAGE)
-    printf("%s: %p base " TARGET_FMT_plx " len %08x %d\n", __func__,
-           mmio, base, TARGET_PAGE_SIZE, subpage_memory);
-#endif
-    subpage_register(mmio, 0, TARGET_PAGE_SIZE-1, phys_section_unassigned);
-
-    return mmio;
+    psection->sub_section = g_malloc0(sizeof(uint16_t) * TARGET_PAGE_SIZE);
+    subsection_register(psection, 0, TARGET_PAGE_SIZE-1,
+                        phys_section_unassigned);
 }
 
 static uint16_t dummy_section(MemoryRegion *mr)
@@ -1689,7 +1623,7 @@ static uint16_t dummy_section(MemoryRegion *mr)
 
 MemoryRegion *iotlb_to_region(hwaddr index)
 {
-    return phys_sections[index & ~TARGET_PAGE_MASK].mr;
+    return phys_sections[index & ~TARGET_PAGE_MASK].section.mr;
 }
 
 static void io_mem_init(void)
@@ -2182,7 +2116,16 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
 
 MemoryRegionSection *address_space_lookup_region(AddressSpace *as, hwaddr addr)
 {
-    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
+    PhysSection *psection;
+    uint16_t idx;
+
+    psection = phys_section_find(as->dispatch, addr >> TARGET_PAGE_BITS);
+    if (psection->sub_section) {
+        idx = psection->sub_section[SUBSECTION_IDX(addr)];
+        return &phys_sections[idx].section;
+    } else {
+        return &psection->section;
+    }
 }
 
 /* warning: addr must be aligned */
@@ -2383,7 +2326,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
-            section = &phys_sections[phys_section_rom];
+            section = &phys_sections[phys_section_rom].section;
         }
         io_mem_write(section->mr, addr, val, 4);
     } else {
@@ -2415,7 +2358,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
-            section = &phys_sections[phys_section_rom];
+            section = &phys_sections[phys_section_rom].section;
         }
 #ifdef TARGET_WORDS_BIGENDIAN
         io_mem_write(section->mr, addr, val >> 32, 4);
@@ -2444,7 +2387,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
-            section = &phys_sections[phys_section_rom];
+            section = &phys_sections[phys_section_rom].section;
         }
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
@@ -2511,7 +2454,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
     if (!memory_region_is_ram(section->mr) || section->readonly) {
         addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
-            section = &phys_sections[phys_section_rom];
+            section = &phys_sections[phys_section_rom].section;
         }
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 11ca4e2..0087555 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -124,7 +124,6 @@ struct MemoryRegion {
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);
     ram_addr_t ram_addr;
-    bool subpage;
     bool terminates;
     bool readable;
     bool ram;
diff --git a/memory.c b/memory.c
index 75ca281..fca0370 100644
--- a/memory.c
+++ b/memory.c
@@ -797,7 +797,6 @@ void memory_region_init(MemoryRegion *mr,
         mr->size = int128_2_64();
     }
     mr->addr = 0;
-    mr->subpage = false;
     mr->enabled = true;
     mr->terminates = false;
     mr->ram = false;
-- 
1.7.3.4

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Paolo Bonzini]

Il 06/05/2013 16:26, Jan Kiszka ha scritto:
> Simplify the sub-page handling by implementing it directly in the
> dispatcher instead of using a redirection memory region. We extend the
> phys_sections entries to optionally hold a pointer to the sub-section
> table that used to reside in the subpage_t structure. IOW, we add one
> optional dispatch level below the existing radix tree.
> 
> address_space_lookup_region is extended to take this additional level
> into account. This direct dispatching to that target memory region will
> also be helpful when we want to add per-region locking control.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I wonder if subpage_ram is needed at all now.  Should be a separate
patch anyway, so

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Peter Maydell]

On 6 May 2013 15:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Simplify the sub-page handling by implementing it directly in the
> dispatcher instead of using a redirection memory region. We extend the
> phys_sections entries to optionally hold a pointer to the sub-section
> table that used to reside in the subpage_t structure. IOW, we add one
> optional dispatch level below the existing radix tree.
>
> address_space_lookup_region is extended to take this additional level
> into account. This direct dispatching to that target memory region will
> also be helpful when we want to add per-region locking control.

This patch seems to break vexpress-a9. Test case if you want it:
http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
(125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb
and then run it; before this patch it boots, afterwards it doesn't
even manage to start the kernel.

My guess is you've broken subregion-sized mmio regions somehow
(and/or regions which are larger than a page in size but start
or finish at a non-page-aligned address), and probably in particular
the arm_gic regions that a9mpcore maps...

thanks
-- PMM

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Paolo Bonzini]

Il 06/05/2013 22:46, Peter Maydell ha scritto:
> On 6 May 2013 15:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Simplify the sub-page handling by implementing it directly in the
>> dispatcher instead of using a redirection memory region. We extend the
>> phys_sections entries to optionally hold a pointer to the sub-section
>> table that used to reside in the subpage_t structure. IOW, we add one
>> optional dispatch level below the existing radix tree.
>>
>> address_space_lookup_region is extended to take this additional level
>> into account. This direct dispatching to that target memory region will
>> also be helpful when we want to add per-region locking control.
> 
> This patch seems to break vexpress-a9. Test case if you want it:
> http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
> (125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb
> and then run it; before this patch it boots, afterwards it doesn't
> even manage to start the kernel.

valgrind is not happy with this patch either:

static int subsection_register(PhysSection *psection, uint32_t start,
                               uint32_t end, uint16_t section)
{
    int idx, eidx;

    if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
        return -1;
    idx = SUBSECTION_IDX(start);
    eidx = SUBSECTION_IDX(end);
    if (memory_region_is_ram(phys_sections[section].section.mr)) {
        MemoryRegionSection new_section = phys_sections[section].section;
        new_section.mr = &io_mem_subpage_ram;
        section = phys_section_add(&new_section);
    }
    for (; idx <= eidx; idx++) {
        psection->sub_section[idx] = section;
    }

    return 0;
}

The phys_section_add might invalidate psection.  If we can drop subpage
RAM, that would fix it.  But similarly here:

    subsection_register(psection, start, end, phys_section_add(section));

The phys_section_add might invalidate psection and the fix is a bit
more involved.

Paolo

> My guess is you've broken subregion-sized mmio regions somehow
> (and/or regions which are larger than a page in size but start
> or finish at a non-page-aligned address), and probably in particular
> the arm_gic regions that a9mpcore maps...
> 
> thanks
> -- PMM
> 
> 

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Paolo Bonzini]

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

Il 06/05/2013 22:46, Peter Maydell ha scritto:
> On 6 May 2013 15:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Simplify the sub-page handling by implementing it directly in the
>> dispatcher instead of using a redirection memory region. We extend the
>> phys_sections entries to optionally hold a pointer to the sub-section
>> table that used to reside in the subpage_t structure. IOW, we add one
>> optional dispatch level below the existing radix tree.
>>
>> address_space_lookup_region is extended to take this additional level
>> into account. This direct dispatching to that target memory region will
>> also be helpful when we want to add per-region locking control.
> 
> This patch seems to break vexpress-a9. Test case if you want it:
> http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
> (125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb
> and then run it; before this patch it boots, afterwards it doesn't
> even manage to start the kernel.
> 
> My guess is you've broken subregion-sized mmio regions somehow
> (and/or regions which are larger than a page in size but start
> or finish at a non-page-aligned address), and probably in particular
> the arm_gic regions that a9mpcore maps...

I think we just found out what all the subpage stuff is for. :)

When I added address_space_translate(), the trickiest conversion to the
new API was in tlb_set_page.  Hence I added a "you never know"-style assert:

    assert(size >= TARGET_PAGE_SIZE);
    if (size != TARGET_PAGE_SIZE) {
        tlb_add_large_page(env, vaddr, size);
     }
-    section = phys_page_find(address_space_memory.dispatch,
-			     paddr >> TARGET_PAGE_BITS);
+    sz = size;
+    section = address_space_translate(&address_space_memory, paddr,
+				      &xlat, &sz, false);
+    assert(sz >= TARGET_PAGE_SIZE);


Now, remember that address_space_translate clamps sz on output to the
size of the section.  And here's what happens:

(gdb) p sz
$4 = 256
(gdb) p *(section->mr)
$5 = {ops = 0x7ffff82d33c0, iommu_ops = 0x0, opaque = 0x7ffff91e6b50,
parent = 0x7ffff9069400, owner = 0x0, size = {lo = 256,
    hi = 0}, addr = 0, destructor = 0x7ffff7e824d0
<memory_region_destructor_none>, ram_addr = 18446744073709551615,
terminates =
    true, romd_mode = true, ram = false, readonly = false, enabled =
true, rom_device = false, warning_printed = false,
  flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, priority
= 0, may_overlap = false, subregions = {tqh_first = 0x0,
    tqh_last = 0x7ffff91e8ee8}, subregions_link = {tqe_next = 0x0,
tqe_prev = 0x7ffff91e64f8}, coalesced = {tqh_first = 0x0,
    tqh_last = 0x7ffff91e8f08}, name = 0x7ffff906bb60 "a9-scu",
dirty_log_mask = 0 '\000', ioeventfd_nb = 0, ioeventfds = 0x0,
  iommu_target_as = 0x0}

The TLB would only store this region instead of the whole page, and
subsequent access past the first 256 bytes would be emulated incorrectly.

For the record, I attach the patch I was using to fix Jan's.

Paolo

[-- Attachment #2: 0001-subpage-fix.patch --]
[-- Type: text/x-patch, Size: 4619 bytes --]

>From 796abe4e7114d18e74cc869922cc5eb0813396c8 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 7 May 2013 11:30:23 +0200
Subject: [PATCH] subpage fix

Note: this temporarily breaks RAM regions in the I/O address space, but
there is none.  It will be fixed later when the special address space
listener is dropped.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c |   56 ++++++++++++--------------------------------------------
 1 files changed, 12 insertions(+), 44 deletions(-)

diff --git a/exec.c b/exec.c
index 7098632..21bd08d 100644
--- a/exec.c
+++ b/exec.c
@@ -65,7 +65,6 @@ AddressSpace address_space_io;
 AddressSpace address_space_memory;
 
 MemoryRegion io_mem_ram, io_mem_rom, io_mem_unassigned, io_mem_notdirty;
-static MemoryRegion io_mem_subpage_ram;
 
 #endif
 
@@ -80,7 +79,8 @@ int use_icount;
 
 #if !defined(CONFIG_USER_ONLY)
 
-#define SUBSECTION_IDX(addr) ((addr) & ~TARGET_PAGE_MASK)
+#define SUBSECTION_IDX(addr)      ((addr) & ~TARGET_PAGE_MASK)
+#define PHYS_SECTION_ID(psection) ((psection) - phys_sections)
 
 typedef struct PhysSection {
     MemoryRegionSection section;
@@ -695,7 +695,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
             iotlb |= phys_section_rom;
         }
     } else {
-        iotlb = container_of(section, PhysSection, section) - phys_sections;
+        iotlb = PHYS_SECTION_ID(container_of(section, PhysSection, section));
         iotlb += xlat;
     }
 
@@ -782,7 +782,7 @@ static void register_subsection(AddressSpaceDispatch *d,
         .offset_within_address_space = base,
         .size = TARGET_PAGE_SIZE,
     };
-    uint16_t new_section;
+    uint16_t new_section, new_subsection;
     hwaddr start, end;
 
     assert(psection->sub_section ||
@@ -793,10 +793,17 @@ static void register_subsection(AddressSpaceDispatch *d,
         psection = &phys_sections[new_section];
         subsections_init(psection);
         phys_page_set(d, base >> TARGET_PAGE_BITS, 1, new_section);
+    } else {
+        new_section = PHYS_SECTION_ID(psection);
     }
+
+    new_subsection = phys_section_add(section);
+
+    /* phys_section_add invalidates psection, reload it  */
+    psection = &phys_sections[new_section];
     start = section->offset_within_address_space & ~TARGET_PAGE_MASK;
     end = start + section->size - 1;
-    subsection_register(psection, start, end, phys_section_add(section));
+    subsection_register(psection, start, end, new_subsection);
 }
 
 
@@ -1607,38 +1614,6 @@ static const MemoryRegionOps watch_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static uint64_t subpage_ram_read(void *opaque, hwaddr addr,
-                                 unsigned size)
-{
-    ram_addr_t raddr = addr;
-    void *ptr = qemu_get_ram_ptr(raddr);
-    switch (size) {
-    case 1: return ldub_p(ptr);
-    case 2: return lduw_p(ptr);
-    case 4: return ldl_p(ptr);
-    default: abort();
-    }
-}
-
-static void subpage_ram_write(void *opaque, hwaddr addr,
-                              uint64_t value, unsigned size)
-{
-    ram_addr_t raddr = addr;
-    void *ptr = qemu_get_ram_ptr(raddr);
-    switch (size) {
-    case 1: return stb_p(ptr, value);
-    case 2: return stw_p(ptr, value);
-    case 4: return stl_p(ptr, value);
-    default: abort();
-    }
-}
-
-static const MemoryRegionOps subpage_ram_ops = {
-    .read = subpage_ram_read,
-    .write = subpage_ram_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 static int subsection_register(PhysSection *psection, uint32_t start,
                                uint32_t end, uint16_t section)
 {
@@ -1648,11 +1623,6 @@ static int subsection_register(PhysSection *psection, uint32_t start,
         return -1;
     idx = SUBSECTION_IDX(start);
     eidx = SUBSECTION_IDX(end);
-    if (memory_region_is_ram(phys_sections[section].section.mr)) {
-        MemoryRegionSection new_section = phys_sections[section].section;
-        new_section.mr = &io_mem_subpage_ram;
-        section = phys_section_add(&new_section);
-    }
     for (; idx <= eidx; idx++) {
         psection->sub_section[idx] = section;
     }
@@ -1692,8 +1662,6 @@ static void io_mem_init(void)
                           "unassigned", UINT64_MAX);
     memory_region_init_io(&io_mem_notdirty, &notdirty_mem_ops, NULL,
                           "notdirty", UINT64_MAX);
-    memory_region_init_io(&io_mem_subpage_ram, &subpage_ram_ops, NULL,
-                          "subpage-ram", UINT64_MAX);
     memory_region_init_io(&io_mem_watch, &watch_mem_ops, NULL,
                           "watch", UINT64_MAX);
 }
-- 
1.7.1

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Jan Kiszka]

On 2013-05-07 14:35, Paolo Bonzini wrote:
> Il 06/05/2013 22:46, Peter Maydell ha scritto:
>> On 6 May 2013 15:26, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> Simplify the sub-page handling by implementing it directly in the
>>> dispatcher instead of using a redirection memory region. We extend the
>>> phys_sections entries to optionally hold a pointer to the sub-section
>>> table that used to reside in the subpage_t structure. IOW, we add one
>>> optional dispatch level below the existing radix tree.
>>>
>>> address_space_lookup_region is extended to take this additional level
>>> into account. This direct dispatching to that target memory region will
>>> also be helpful when we want to add per-region locking control.
>>
>> This patch seems to break vexpress-a9. Test case if you want it:
>> http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
>> (125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb
>> and then run it; before this patch it boots, afterwards it doesn't
>> even manage to start the kernel.
>>
>> My guess is you've broken subregion-sized mmio regions somehow
>> (and/or regions which are larger than a page in size but start
>> or finish at a non-page-aligned address), and probably in particular
>> the arm_gic regions that a9mpcore maps...
> 
> I think we just found out what all the subpage stuff is for. :)
> 
> When I added address_space_translate(), the trickiest conversion to the
> new API was in tlb_set_page.  Hence I added a "you never know"-style assert:
> 
>     assert(size >= TARGET_PAGE_SIZE);
>     if (size != TARGET_PAGE_SIZE) {
>         tlb_add_large_page(env, vaddr, size);
>      }
> -    section = phys_page_find(address_space_memory.dispatch,
> -			     paddr >> TARGET_PAGE_BITS);
> +    sz = size;
> +    section = address_space_translate(&address_space_memory, paddr,
> +				      &xlat, &sz, false);
> +    assert(sz >= TARGET_PAGE_SIZE);
> 
> 
> Now, remember that address_space_translate clamps sz on output to the
> size of the section.  And here's what happens:
> 
> (gdb) p sz
> $4 = 256
> (gdb) p *(section->mr)
> $5 = {ops = 0x7ffff82d33c0, iommu_ops = 0x0, opaque = 0x7ffff91e6b50,
> parent = 0x7ffff9069400, owner = 0x0, size = {lo = 256,
>     hi = 0}, addr = 0, destructor = 0x7ffff7e824d0
> <memory_region_destructor_none>, ram_addr = 18446744073709551615,
> terminates =
>     true, romd_mode = true, ram = false, readonly = false, enabled =
> true, rom_device = false, warning_printed = false,
>   flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, priority
> = 0, may_overlap = false, subregions = {tqh_first = 0x0,
>     tqh_last = 0x7ffff91e8ee8}, subregions_link = {tqe_next = 0x0,
> tqe_prev = 0x7ffff91e64f8}, coalesced = {tqh_first = 0x0,
>     tqh_last = 0x7ffff91e8f08}, name = 0x7ffff906bb60 "a9-scu",
> dirty_log_mask = 0 '\000', ioeventfd_nb = 0, ioeventfds = 0x0,
>   iommu_target_as = 0x0}
> 
> The TLB would only store this region instead of the whole page, and
> subsequent access past the first 256 bytes would be emulated incorrectly.

Good catch. Hmm, and a tricky issue. So we need to preserve the
dispatching read/write handler of sub-pages, and a reference to the
sub-page would continue to end up in the TCG TLBs. But reference
counting of the memory regions that page may point to becomes hairy.

Conceptually, we would have to increment the counter for all regions a
sub-page refers to. Even worse, regions may be added to the sub-page
while it is referenced by some user. Doesn't work.

Well, the alternative is to handle a sub-page dispatch (ie. calling into
subpage_[ram_]read/write just like address_space_rw: take the necessary
lock that protect mapping changes, look into the sub-page and pick up
the target region, invoke memory_region_ref on it, perform the access
and unref the region again. Slow, but that's how sub-pages are. And it
only affects TCG. Hmm, or does your IOMMU core cache translations on a
per-page base as well?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Jan Kiszka]

On 2013-05-07 19:26, Jan Kiszka wrote:
> Well, the alternative is to handle a sub-page dispatch (ie. calling into
> subpage_[ram_]read/write just like address_space_rw: take the necessary
> lock that protect mapping changes, look into the sub-page and pick up
> the target region, invoke memory_region_ref on it, perform the access
> and unref the region again. Slow, but that's how sub-pages are. And it
> only affects TCG. Hmm, or does your IOMMU core cache translations on a
> per-page base as well?

OK, there is no translation caching in the memory core. So I will
preserve the dispatching functions of sub-pages, just like the term
"sub-page" - along with a comment why we depend on page granularity.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling

[Paolo Bonzini]

> On 2013-05-07 19:26, Jan Kiszka wrote:
> > Well, the alternative is to handle a sub-page dispatch (ie. calling into
> > subpage_[ram_]read/write just like address_space_rw: take the necessary
> > lock that protect mapping changes, look into the sub-page and pick up
> > the target region, invoke memory_region_ref on it, perform the access
> > and unref the region again. Slow, but that's how sub-pages are. And it
> > only affects TCG. Hmm, or does your IOMMU core cache translations on a
> > per-page base as well?
> 
> OK, there is no translation caching in the memory core. So I will
> preserve the dispatching functions of sub-pages, just like the term
> "sub-page" - along with a comment why we depend on page granularity.

Note that TCG will cache translations because the TLB entry is filled
with the page after translation has taken place. In fact, this is the
main change from Avi's series to mine, and a side-effect of centralizing
the translation in address_space_translate.

It shouldn't be a problem though, the TLB entry will point to the subpage
and the compiled code will dispatch to it.

Paolo

[Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw

[Jan Kiszka]

This will be needed for some corner cases with para-virtual the I/O
ports.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c |   33 ++++++++++++++++++---------------
 1 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/exec.c b/exec.c
index 3ee1f3f..9c582b1 100644
--- a/exec.c
+++ b/exec.c
@@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
     uint8_t *ptr;
     uint32_t val;
     MemoryRegionSection *section;
+    MemoryRegion *mr;
 
     while (len > 0) {
         l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
         section = address_space_lookup_region(as, addr);
+        mr = section->mr;
 
         if (is_write) {
-            if (!memory_region_is_ram(section->mr)) {
+            if (!memory_region_is_ram(mr)) {
                 hwaddr addr1;
                 addr1 = memory_region_section_addr(section, addr);
                 /* XXX: could force cpu_single_env to NULL to avoid
                    potential bugs */
-                if (l >= 4 && ((addr1 & 3) == 0)) {
+                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {
                     /* 32 bit write access */
                     val = ldl_p(buf);
-                    io_mem_write(section->mr, addr1, val, 4);
+                    io_mem_write(mr, addr1, val, 4);
                     l = 4;
-                } else if (l >= 2 && ((addr1 & 1) == 0)) {
+                } else if (l >= 2 &&
+                           ((addr1 & 1) == 0 || mr->ops->impl.unaligned)) {
                     /* 16 bit write access */
                     val = lduw_p(buf);
-                    io_mem_write(section->mr, addr1, val, 2);
+                    io_mem_write(mr, addr1, val, 2);
                     l = 2;
                 } else {
                     /* 8 bit write access */
                     val = ldub_p(buf);
-                    io_mem_write(section->mr, addr1, val, 1);
+                    io_mem_write(mr, addr1, val, 1);
                     l = 1;
                 }
             } else if (!section->readonly) {
                 ram_addr_t addr1;
-                addr1 = memory_region_get_ram_addr(section->mr)
+                addr1 = memory_region_get_ram_addr(mr)
                     + memory_region_section_addr(section, addr);
                 /* RAM case */
                 ptr = qemu_get_ram_ptr(addr1);
@@ -1873,30 +1876,30 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 qemu_put_ram_ptr(ptr);
             }
         } else {
-            if (!(memory_region_is_ram(section->mr) ||
-                  memory_region_is_romd(section->mr))) {
+            if (!(memory_region_is_ram(mr) || memory_region_is_romd(mr))) {
                 hwaddr addr1;
                 /* I/O case */
                 addr1 = memory_region_section_addr(section, addr);
-                if (l >= 4 && ((addr1 & 3) == 0)) {
+                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {
                     /* 32 bit read access */
-                    val = io_mem_read(section->mr, addr1, 4);
+                    val = io_mem_read(mr, addr1, 4);
                     stl_p(buf, val);
                     l = 4;
-                } else if (l >= 2 && ((addr1 & 1) == 0)) {
+                } else if (l >= 2 &&
+                           ((addr1 & 1) == 0 || mr->ops->impl.unaligned)) {
                     /* 16 bit read access */
-                    val = io_mem_read(section->mr, addr1, 2);
+                    val = io_mem_read(mr, addr1, 2);
                     stw_p(buf, val);
                     l = 2;
                 } else {
                     /* 8 bit read access */
-                    val = io_mem_read(section->mr, addr1, 1);
+                    val = io_mem_read(mr, addr1, 1);
                     stb_p(buf, val);
                     l = 1;
                 }
             } else {
                 /* RAM case */
-                ptr = qemu_get_ram_ptr(section->mr->ram_addr
+                ptr = qemu_get_ram_ptr(mr->ram_addr
                                        + memory_region_section_addr(section,
                                                                     addr));
                 memcpy(buf, ptr, l);
-- 
1.7.3.4

Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw

[Paolo Bonzini]

Il 06/05/2013 16:26, Jan Kiszka ha scritto:
> This will be needed for some corner cases with para-virtual the I/O
> ports.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  exec.c |   33 ++++++++++++++++++---------------
>  1 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 3ee1f3f..9c582b1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>      uint8_t *ptr;
>      uint32_t val;
>      MemoryRegionSection *section;
> +    MemoryRegion *mr;
>  
>      while (len > 0) {
>          l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
>          section = address_space_lookup_region(as, addr);
> +        mr = section->mr;
>  
>          if (is_write) {
> -            if (!memory_region_is_ram(section->mr)) {
> +            if (!memory_region_is_ram(mr)) {
>                  hwaddr addr1;
>                  addr1 = memory_region_section_addr(section, addr);
>                  /* XXX: could force cpu_single_env to NULL to avoid
>                     potential bugs */
> -                if (l >= 4 && ((addr1 & 3) == 0)) {
> +                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {

Does the length matter at all if unaligned accesses are allowed?  I
think it shouldn't...

Paolo

>                      /* 32 bit write access */
>                      val = ldl_p(buf);
> -                    io_mem_write(section->mr, addr1, val, 4);
> +                    io_mem_write(mr, addr1, val, 4);
>                      l = 4;
> -                } else if (l >= 2 && ((addr1 & 1) == 0)) {
> +                } else if (l >= 2 &&
> +                           ((addr1 & 1) == 0 || mr->ops->impl.unaligned)) {
>                      /* 16 bit write access */
>                      val = lduw_p(buf);
> -                    io_mem_write(section->mr, addr1, val, 2);
> +                    io_mem_write(mr, addr1, val, 2);
>                      l = 2;
>                  } else {
>                      /* 8 bit write access */
>                      val = ldub_p(buf);
> -                    io_mem_write(section->mr, addr1, val, 1);
> +                    io_mem_write(mr, addr1, val, 1);
>                      l = 1;
>                  }
>              } else if (!section->readonly) {
>                  ram_addr_t addr1;
> -                addr1 = memory_region_get_ram_addr(section->mr)
> +                addr1 = memory_region_get_ram_addr(mr)
>                      + memory_region_section_addr(section, addr);
>                  /* RAM case */
>                  ptr = qemu_get_ram_ptr(addr1);
> @@ -1873,30 +1876,30 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  qemu_put_ram_ptr(ptr);
>              }
>          } else {
> -            if (!(memory_region_is_ram(section->mr) ||
> -                  memory_region_is_romd(section->mr))) {
> +            if (!(memory_region_is_ram(mr) || memory_region_is_romd(mr))) {
>                  hwaddr addr1;
>                  /* I/O case */
>                  addr1 = memory_region_section_addr(section, addr);
> -                if (l >= 4 && ((addr1 & 3) == 0)) {
> +                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {
>                      /* 32 bit read access */
> -                    val = io_mem_read(section->mr, addr1, 4);
> +                    val = io_mem_read(mr, addr1, 4);
>                      stl_p(buf, val);
>                      l = 4;
> -                } else if (l >= 2 && ((addr1 & 1) == 0)) {
> +                } else if (l >= 2 &&
> +                           ((addr1 & 1) == 0 || mr->ops->impl.unaligned)) {
>                      /* 16 bit read access */
> -                    val = io_mem_read(section->mr, addr1, 2);
> +                    val = io_mem_read(mr, addr1, 2);
>                      stw_p(buf, val);
>                      l = 2;
>                  } else {
>                      /* 8 bit read access */
> -                    val = io_mem_read(section->mr, addr1, 1);
> +                    val = io_mem_read(mr, addr1, 1);
>                      stb_p(buf, val);
>                      l = 1;
>                  }
>              } else {
>                  /* RAM case */
> -                ptr = qemu_get_ram_ptr(section->mr->ram_addr
> +                ptr = qemu_get_ram_ptr(mr->ram_addr
>                                         + memory_region_section_addr(section,
>                                                                      addr));
>                  memcpy(buf, ptr, l);
> 

Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw

[Jan Kiszka]

On 2013-05-06 16:55, Paolo Bonzini wrote:
> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>> This will be needed for some corner cases with para-virtual the I/O
>> ports.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  exec.c |   33 ++++++++++++++++++---------------
>>  1 files changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 3ee1f3f..9c582b1 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>>      uint8_t *ptr;
>>      uint32_t val;
>>      MemoryRegionSection *section;
>> +    MemoryRegion *mr;
>>  
>>      while (len > 0) {
>>          l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>>          if (l > len)
>>              l = len;
>>          section = address_space_lookup_region(as, addr);
>> +        mr = section->mr;
>>  
>>          if (is_write) {
>> -            if (!memory_region_is_ram(section->mr)) {
>> +            if (!memory_region_is_ram(mr)) {
>>                  hwaddr addr1;
>>                  addr1 = memory_region_section_addr(section, addr);
>>                  /* XXX: could force cpu_single_env to NULL to avoid
>>                     potential bugs */
>> -                if (l >= 4 && ((addr1 & 3) == 0)) {
>> +                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {
> 
> Does the length matter at all if unaligned accesses are allowed?  I
> think it shouldn't...

What do you mean? The length test here is not about alignment, it's
about proper split-up depending on the input size (we cannot use 32 bit
for all accesses and do not want to make them all byte accesses, do we?).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw

[Paolo Bonzini]

Il 06/05/2013 16:58, Jan Kiszka ha scritto:
> On 2013-05-06 16:55, Paolo Bonzini wrote:
>> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>>> This will be needed for some corner cases with para-virtual the I/O
>>> ports.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  exec.c |   33 ++++++++++++++++++---------------
>>>  1 files changed, 18 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 3ee1f3f..9c582b1 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>>>      uint8_t *ptr;
>>>      uint32_t val;
>>>      MemoryRegionSection *section;
>>> +    MemoryRegion *mr;
>>>  
>>>      while (len > 0) {
>>>          l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>>>          if (l > len)
>>>              l = len;
>>>          section = address_space_lookup_region(as, addr);
>>> +        mr = section->mr;
>>>  
>>>          if (is_write) {
>>> -            if (!memory_region_is_ram(section->mr)) {
>>> +            if (!memory_region_is_ram(mr)) {
>>>                  hwaddr addr1;
>>>                  addr1 = memory_region_section_addr(section, addr);
>>>                  /* XXX: could force cpu_single_env to NULL to avoid
>>>                     potential bugs */
>>> -                if (l >= 4 && ((addr1 & 3) == 0)) {
>>> +                if (l >= 4 && ((addr1 & 3) == 0 || mr->ops->impl.unaligned)) {
>>
>> Does the length matter at all if unaligned accesses are allowed?  I
>> think it shouldn't...
> 
> What do you mean? The length test here is not about alignment, it's
> about proper split-up depending on the input size (we cannot use 32 bit
> for all accesses and do not want to make them all byte accesses, do we?).

Oh right, I was thinking of my IOMMU tree where I have:

        l = len;
        section = address_space_translate(as, addr, &addr1, &l, is_write);

        if (is_write) {
            if (!memory_region_is_ram(section->mr)) {
                /* XXX: could force cpu_single_env to NULL to avoid
                   potential bugs */
                if (l >= 4 && ((addr1 & 3) == 0)) {

and address_space_translate does:

        *plen = MIN(section->size - addr, *plen);

It should not do this if section->mr->ops.unaligned.  Whoever rebases on top
of the other should keep this in mind.

Paolo

[Qemu-devel] [RFC][PATCH 12/15] vmware-vga: Accept unaligned I/O accesses

[Jan Kiszka]

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

Re: [Qemu-devel] [RFC][PATCH 12/15] vmware-vga: Accept unaligned I/O accesses

[Paolo Bonzini]

Il 06/05/2013 16:26, Jan Kiszka ha scritto:
> 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,
>      },
>  };
>  
> 

The Xen platform device needs the same.

Paolo

Re: [Qemu-devel] [RFC][PATCH 12/15] vmware-vga: Accept unaligned I/O accesses

[Jan Kiszka]

On 2013-05-06 16:40, Paolo Bonzini wrote:
> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>> 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,
>>      },
>>  };
>>  
>>
> 
> The Xen platform device needs the same.

OK, good to know. In theory, this should only affect weird PV, so I hope
that is all.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [RFC][PATCH 13/15] 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 hook
memory_region_iorange_read/write into the normal dispatch handler,
calling them when standard region handler is not set, but old_portio is.

We can drop the additional aliasing of ioport regions and can also
the special address space listener. cpu_in/out now simply call into
address_space_read/write.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                         |   25 ----------
 include/exec/ioport.h          |    1 -
 include/exec/memory-internal.h |    2 -
 include/exec/memory.h          |    3 +-
 ioport.c                       |   49 +++++++++++--------
 memory.c                       |  101 ++++++++++++++-------------------------
 6 files changed, 66 insertions(+), 115 deletions(-)

diff --git a/exec.c b/exec.c
index 9c582b1..992e16a 100644
--- a/exec.c
+++ b/exec.c
@@ -1679,24 +1679,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, section->size);
-    ioport_register(&mrio->iorange);
-}
-
-static void io_region_del(MemoryListener *listener,
-                          MemoryRegionSection *section)
-{
-    isa_unassign_ioport(section->offset_within_address_space, section->size);
-}
-
 static MemoryListener core_memory_listener = {
     .begin = core_begin,
     .log_global_start = core_log_global_start,
@@ -1704,12 +1686,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,
 };
@@ -1752,7 +1728,6 @@ static void memory_map_init(void)
     address_space_io.name = "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);
 
     dma_context_init(&dma_context_memory, &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 1b156fd..3134990 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -128,8 +128,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 0087555..5c9a958 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -151,6 +151,7 @@ struct MemoryRegionPortio {
     unsigned size;
     IOPortReadFunc *read;
     IOPortWriteFunc *write;
+    uint32_t base; /* private field */
 };
 
 #define PORTIO_END_OF_LIST() { }
@@ -810,7 +811,7 @@ void mtree_info(fprintf_function mon_printf, void *f);
  * address_space_init: initializes an address space
  *
  * @as: an uninitialized #AddressSpace
- * @root: a #MemoryRegion that routes addesses for the address space
+ * @root: a #MemoryRegion that routes addresses for the address space
  */
 void address_space_init(AddressSpace *as, MemoryRegion *root);
 
diff --git a/ioport.c b/ioport.c
index 56470c5..9f15567 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 */
@@ -279,27 +280,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 +315,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 +327,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 +350,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,7 +358,6 @@ void portio_list_init(PortioList *piolist,
 void portio_list_destroy(PortioList *piolist)
 {
     g_free(piolist->regions);
-    g_free(piolist->aliases);
 }
 
 static void portio_list_add_1(PortioList *piolist,
@@ -355,7 +367,7 @@ static void portio_list_add_1(PortioList *piolist,
 {
     MemoryRegionPortio *pio;
     MemoryRegionOps *ops;
-    MemoryRegion *region, *alias;
+    MemoryRegion *region;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
@@ -366,25 +378,24 @@ static void portio_list_add_1(PortioList *piolist,
     /* Adjust the offsets to all be zero-based for the region.  */
     for (i = 0; i < count; ++i) {
         pio[i].offset -= off_low;
+        pio[i].base = start + off_low;
     }
 
     ops = g_new0(MemoryRegionOps, 1);
     ops->old_portio = pio;
+    ops->valid.unaligned = true;
+    ops->impl.unaligned = true;
 
     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);
+                          off_high - off_low);
     memory_region_add_subregion(piolist->address_space,
-                                start + off_low, alias);
+                                start + off_low, region);
     piolist->regions[piolist->nr] = region;
-    piolist->aliases[piolist->nr] = alias;
     ++piolist->nr;
 }
 
@@ -427,19 +438,15 @@ void portio_list_add(PortioList *piolist,
 
 void portio_list_del(PortioList *piolist)
 {
-    MemoryRegion *mr, *alias;
+    MemoryRegion *mr;
     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_del_subregion(piolist->address_space, mr);
         memory_region_destroy(mr);
         g_free((MemoryRegionOps *)mr->ops);
         g_free(mr);
-        g_free(alias);
         piolist->regions[i] = NULL;
-        piolist->aliases[i] = NULL;
     }
 }
diff --git a/memory.c b/memory.c
index fca0370..9731dd0 100644
--- a/memory.c
+++ b/memory.c
@@ -380,79 +380,42 @@ static const MemoryRegionPortio *find_portio(MemoryRegion *mr, uint64_t offset,
     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;
+static uint64_t memory_region_iorange_read(MemoryRegion *mr,
+                                           uint64_t offset,
+                                           unsigned width)
+{
+    const MemoryRegionPortio *mrp = find_portio(mr, offset, width, false);
+    uint64_t data;
+
+    data = ((uint64_t)1 << (width * 8)) - 1;
+    if (mrp) {
+        data = mrp->read(mr->opaque, mrp->base + offset);
+    } else if (width == 2) {
+        mrp = find_portio(mr, offset, 1, false);
+        assert(mrp);
+        data = mrp->read(mr->opaque, mrp->base + offset) |
+                (mrp->read(mr->opaque, mrp->base + offset + 1) << 8);
     }
-    *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);
+    return data;
 }
 
-static void memory_region_iorange_write(IORange *iorange,
+static void memory_region_iorange_write(MemoryRegion *mr,
                                         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);
-}
+    const MemoryRegionPortio *mrp = find_portio(mr, offset, width, true);
 
-static void memory_region_iorange_destructor(IORange *iorange)
-{
-    g_free(container_of(iorange, MemoryRegionIORange, iorange));
+    if (mrp) {
+        mrp->write(mr->opaque, mrp->base + offset, data);
+    } else if (width == 2) {
+        mrp = find_portio(mr, offset, 1, true);
+        assert(mrp);
+        mrp->write(mr->opaque, mrp->base + offset, data & 0xff);
+        mrp->write(mr->opaque, mrp->base + offset + 1, data >> 8);
+    }
 }
 
-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;
@@ -854,7 +817,11 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
     }
 
     if (!mr->ops->read) {
-        return mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
+        if (mr->ops->old_portio) {
+            return memory_region_iorange_read(mr, addr, size);
+        } else {
+            return mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
+        }
     }
 
     /* FIXME: support unaligned access */
@@ -907,7 +874,11 @@ static void memory_region_dispatch_write(MemoryRegion *mr,
     adjust_endianness(mr, &data, size);
 
     if (!mr->ops->write) {
-        mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, data);
+        if (mr->ops->old_portio) {
+            memory_region_iorange_write(mr, addr, size, data);
+        } else {
+            mr->ops->old_mmio.write[ctz32(size)](mr->opaque, addr, data);
+        }
         return;
     }
 
-- 
1.7.3.4

[Qemu-devel] [RFC][PATCH 14/15] ioport: Remove unused old dispatching services

[Jan Kiszka]

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 5c9a958..cad73f5 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"
 
@@ -42,14 +41,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;
-};
-
 /*
  * Memory region callbacks
  */
diff --git a/ioport.c b/ioport.c
index 9f15567..000087e 100644
--- a/ioport.c
+++ b/ioport.c
@@ -30,252 +30,14 @@
 #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
 #  define LOG_IOPORT(...) do { } while (0)
 #endif
 
-/* 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] [RFC][PATCH 15/15] ioport: Move IOPortRead/WriteFunc typedefs to memory.h

[Jan Kiszka]

Move the function types required for MemoryRegionPortio to memory.h.
This allows to let ioport.h depend on memory.h, which is more consistent
instead than the other way around.

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

diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index ba3ebb8..c7da6d4 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,10 +33,6 @@ 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);
-
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);
 void cpu_outl(pio_addr_t addr, uint32_t val);
@@ -43,9 +40,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 cad73f5..7843076 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"
 
 typedef struct MemoryRegionOps MemoryRegionOps;
@@ -136,6 +135,9 @@ struct MemoryRegion {
     MemoryRegionIoeventfd *ioeventfds;
 };
 
+typedef void (IOPortWriteFunc)(void *opaque, uint32_t address, uint32_t data);
+typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);
+
 struct MemoryRegionPortio {
     uint32_t offset;
     uint32_t len;
-- 
1.7.3.4

Re: [Qemu-devel] [RFC][PATCH 00/15] Refactor portio dispatching

[Andreas Färber]

Am 06.05.2013 16:26, schrieb Jan Kiszka:
> This series converts the remaining users of register_ioport* to portio
> lists,

Why does it need to be lists? Is there anything wrong with using
isa_register_ioport() as done in Hervé's previous ioport cleanup series?
(Hope you were aware of those ppc-centered discussions?)

Andreas

> simplifies the handling of subpages and adds support for unaligned
> memory region accesses. Then it replaces the current portio dispatcher
> with the existing one for MMIO and removes several lines of code. This
> also allows to build BQL-free portio on top once we enhance the memory
> layer accordingly.
> 
> Seems to work fine so far but surely requires thorough review. And I
> would welcome early comments on the direction.
> 
> Jan
> 
> 
> CC: malc <av1474@comtv.ru>
> 
> Jan Kiszka (15):
>   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
>   memory: Introduce address_space_lookup_region
>   memory: Rework sub-page handling
>   memory: Allow unaligned address_space_rw
>   vmware-vga: Accept unaligned I/O accesses
>   ioport: Switch dispatching to memory core layer
>   ioport: Remove unused old dispatching services
>   ioport: Move IOPortRead/WriteFunc typedefs to memory.h
> 
>  cputlb.c                       |    2 +-
>  exec.c                         |  273 +++++++++++++------------------------
>  hw/acpi/piix4.c                |    6 +-
>  hw/audio/adlib.c               |   20 ++-
>  hw/display/vmware_vga.c        |    4 +
>  hw/dma/i82374.c                |   17 ++-
>  hw/isa/isa-bus.c               |   11 ++
>  hw/isa/lpc_ich9.c              |    8 +-
>  hw/isa/vt82c686.c              |   40 ++++--
>  hw/misc/applesmc.c             |   48 +++++--
>  hw/ppc/prep.c                  |   23 ++-
>  hw/watchdog/wdt_ib700.c        |   12 ++-
>  include/exec/cputlb.h          |    2 -
>  include/exec/ioport.h          |   19 +---
>  include/exec/iorange.h         |   31 ----
>  include/exec/memory-internal.h |    2 -
>  include/exec/memory.h          |   26 ++--
>  include/hw/isa/isa.h           |    2 +
>  ioport.c                       |  294 ++++------------------------------------
>  memory.c                       |  102 +++++---------
>  translate-all.c                |    3 +-
>  21 files changed, 311 insertions(+), 634 deletions(-)
>  delete mode 100644 include/exec/iorange.h
> 


-- 
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] [RFC][PATCH 00/15] Refactor portio dispatching

[Jan Kiszka]

On 2013-05-06 16:50, Andreas Färber wrote:
> Am 06.05.2013 16:26, schrieb Jan Kiszka:
>> This series converts the remaining users of register_ioport* to portio
>> lists,
> 
> Why does it need to be lists? Is there anything wrong with using
> isa_register_ioport() as done in Hervé's previous ioport cleanup series?
> (Hope you were aware of those ppc-centered discussions?)

Lists are the straight way of conversion, specifically for I/O port
assignments that are scattered. On some lazy day, someone can convert
them into multiple memory regions. That's beyond the scope if this
series and more risky.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux