64-bit io paths

From what I know about PCI this isn't possible with that bus, so this

Step 1 to implementing alpha-softmmu is to properly handle 64-bit
I/O operations. Tristan Gingold managed a hack where he buffered half of the I/O operation in the host bridge; I think that's not something we want to encourage.

I'm a bit confused about IO_MEM_SUBWIDTH and subpage_register, and why things are done the way that they are. The loop in subpage_register appears to be written to support different devices at the same I/O address so long as they use different access widths. Is this really a consideration to how this code is written? Are there really systems that need to support this?

From what I know about PCI this isn't possible with that bus, so this isn't something that's going to appear on PCs. So if true, it's got to be some strange embedded weirdness.

I've reviewed all of the devices in hw/. Almost all of them have non-zero values for all entries. The ones that do have zeros are:

axis_dev88.c; gpio
eccmemctl.c; ecc, ecc_diag
escc.c; escc
esp.c; esp
etraxfs_eth.c; eth
etraxfs_pic.c; pic
etraxfs_ser.c; ser
etraxfs_timer.c; timer
fdc.c; fdctrl_mem_read_strict
fw_cfg.c; fw_cfg_ctl_mem, fw_cfg_data_mem
hpet.c; hpet_ram
lance.c; lance_mem
mac_dbdma.c; dbdma
mips_jazz.c; dma_dummy_read
r2d.c; r2d_fpga
sbi.c; sbi_mem
sh_pci.c; sh_pci
slavio_intctl.c; slavio_intctl, slavio_intctlm
slabio_misc.c; slavio_cfg_mem, etc.
sm501.c; sm501_system_config, sm501_disp_ctrl
sparc32_dma.c; dma_mem
sun4c_intctl.c; sun4c_intctl
sun4m_iommu.c; iommu_mem
tcx.c; tcx_dac, tcx_dummy
xilinx_ethlite.c; eth, pic
xilinx_timer.c; timer

Some of the ones that are obviously to be used together (e.g. etraxfs* and xilinx*) exclusively use one access method (e.g. readb or readl), which indicates that they cannot be using overlapping addresses.

Some of the others (e.g. hpet.c) it is obvious via surrounding context that the driver author assumed that the "functions may be NULL" comment above cpu_register_io_memory meant that accesses that are undefined need not be implemented. (This is what I assumed when I first read that comment as well.)

So... What's supposed to be going on here?

For what it's worth, this patch series has been tested with arm-test, sparc-test, coldfire-test from the qemu downloads page, as well as with a full Fedora and WinNT system boots. The second patch has not been properly tested beyond compile, obviously, since there are not yet any drivers that implement the hook.

r~

Richard Henderson (2):
io: Add CPUIOMemoryOps and use it.
io: Add readq/writeq hooks and use them.

  cpu-common.h       |   19 ++-
  exec-all.h         |    3 +-
  exec.c             |  425 +++++++++++++++++++++++++++++++++-------------------
  softmmu_template.h |   40 ++++--
  4 files changed, 320 insertions(+), 167 deletions(-)

Subpages are used when there are several devices on the same page. It's needed for at least Sparc32.

Subwidth (with NULL) is used mainly to indicate that the device does not accept accesses in some access widths. Sparc32 and Sparc64 need this (or some other way to signal bus errors for bad access widths).

In fact, many system devices on Sparc64 should only accept 64 bit accesses, but currently we can't enforce this.

Quoted 2 lines · Blue Swirl

Subpages are used when there are several devices on the same page. It's needed for at least Sparc32.

That's fine.

Quoted 3 lines

Subwidth (with NULL) is used mainly to indicate that the device does not accept accesses in some access widths. Sparc32 and Sparc64 need this (or some other way to signal bus errors for bad access widths).

This is also fine. Although by using NULL all you'd get is a qemu
null pointer dereference; I suppose this might get caught and translated to an cpu exception, but I think it would be preferable long-term to be more explicit about this and fill in the entries with a function that would explicitly raise the exception.

What this *does* confirm for me is that we don't need to support multiple devices at the same address, differentiated by the size of the reference. Which is something that the current subpage implementation actually supports.

I'll submit a patch to clean this up.

Quoted 2 lines

In fact, many system devices on Sparc64 should only accept 64 bit accesses, but currently we can't enforce this.

Cool. This is something that should be fixed by the end result
of the driver interface re-design that pbrook and I have been discussing on IRC.

Quoted 5 lines

This is also fine. Although by using NULL all you'd get is a qemu
null pointer dereference; I suppose this might get caught and translated to an cpu exception, but I think it would be preferable long-term to be more explicit about this and fill in the entries with a function that would explicitly raise the exception.

Perhaps also the bus layer could do something here.

Interesting. Could you make a summary of the design for the benefit of the list?

Quoted 7 lines · Blue Swirl

Quoted 5 lines

This is also fine. Although by using NULL all you'd get is a qemu
null pointer dereference; I suppose this might get caught and translated to an cpu exception, but I think it would be preferable long-term to be more explicit about this and fill in the entries with a function that would explicitly raise the exception.

Perhaps also the bus layer could do something here.

What do you mean?

Quoted 1 line

Interesting. Could you make a summary of the design for the benefit of the list?

The basic device interface looks like

+/* Object based memory region registration. */
+
+typedef struct MemoryRegion MemoryRegion;
+
+typedef uint32_t MemoryCallbackRead(MemoryRegion *, target_phys_addr_t ofs); +typedef uint64_t MemoryCallbackRead64(MemoryRegion *, target_phys_addr_t ofs); +typedef void MemoryCallbackWrite(MemoryRegion *, target_phys_addr_t ofs,
+ uint32_t val);
+typedef void MemoryCallbackWrite64(MemoryRegion *, target_phys_addr_t ofs,
+ uint64_t val);
+
+typedef struct MemoryCallbackInfo {

 +    MemoryCallbackRead *read8;
 +    MemoryCallbackRead *read16;
 +    MemoryCallbackRead *read32;
 +    MemoryCallbackRead64 *read64;
 +
 +    MemoryCallbackWrite *write8;
 +    MemoryCallbackWrite *write16;
 +    MemoryCallbackWrite *write32;
 +    MemoryCallbackWrite64 *write64;
 +
 +    /* This describes RAM.  The callbacks above need not be used, and
 +       the host memory backing the RAM begins at qemu_get_ram_ptr_r.  */
 +    _Bool ram;
 +
 +    /* Legacy shim: propagate the IO_MEM_ROMD bit.  */
 +    _Bool romd;
 +} MemoryCallbackInfo;
 +
 +/* This structure describes the region.  It is logically opaque -- drivers
 +   should not be peeking inside.  But in the interest of efficiency we want
 +   to directly embed this structure in the driver's private strucure.  In
 +   this way we can avoid having to have an extra table of opaque pointers
 +   for consumption by the driver.  The intended use is
 +
 +    struct FooDeviceState {
 +        DeviceState qdev;
 +        MemoryRegion mem_region;
 +        ...
 +    };
 +
 +    static uint32_t foo_read8(MemoryRegion *region, target_phys_addr_t ofs)
 +    {
 +        FooDeviceState *s = container_of(region, FooDeviceState, mem_region);
 +        ...
 +    }
 +*/
 +
 +struct MemoryRegion {
 +    const MemoryCallbackInfo *cb;
 +    target_phys_addr_t start;
 +    target_phys_addr_t size;
 +
 +    /* If this value is not -1, this memory region is registred in
 +       the io_mem_region array, used by softmmu_header.h to resolve
 +       memory-mapped operations in the guest address space.  */
 +    int io_index;
 +};
 +
 +/* Register a memory region at START_ADDR/SIZE.  The REGION structure will
 +   be initialized appropriately for DEV using CB as the operation set.  */
 +extern void cpu_register_memory_region(MemoryRegion *region,
 +                                       const MemoryCallbackInfo *cb,
 +                                       target_phys_addr_t start_addr,
 +                                       target_phys_addr_t size);

+
+/* Unregister a memory region. */
+extern void cpu_unregister_memory_region(MemoryRegion *); +
+/* Allocate ram for use with cpu_register_memory_region. */ +extern const MemoryCallbackInfo *qemu_ram_alloc_r(ram_addr_t); +extern void qemu_ram_free_r(const MemoryCallbackInfo *);

The Basic Idea is that we have a MemoryRegion object that describes a contiguous mapping within the guest address space. This object needs to handle RAM, ROM and devices. The desire to handle memory and devices the same comes from the wish to have PCI device BARs show up as plain memory in the TLB as plain memory, and to be able to handle all PCI device regions identically within sysbus.

There are a number of restrictions on the interface above what we currently support:

* Objects may not be mapped over the top of existing objects. This is most abused by pflash devices with their IO_MEM_ROMD thing.

* Objects must be unmapped explicitly, rather than mapping IO_MEM_UNASSIGNED over the top of an object. This is most abused by the PCI subsystem.

The use of the MemoryRegion pointer doubling as the device's opaque pointer means that it's easy to implement generic helpers such as

uint64_t composite_read64(MemoryRegion *region, target_phys_addr_t ofs)

 {
   uint32_t v1 = region->cb->read32(region, ofs);
   uint32_t v2 = region->cb->read32(region, ofs + 4);

   return combine_for_target_endian(v1, v2);

}

or even

uin32_t invalid_read8(MemoryRegion *region, target_phys_addr_t ofs)

 {
    do_unassigned_access(region->start + ofs, 0, 0, 0, 1);
    return 0;

}

without having to have supplementary opaque pointer caches, as we currently do for e.g. subpage_t.

These can be entered into the device's MemoryCallbackInfo structure so that we're sure that there's defined semantics for every access, and the device need not invent its own invalid_read functions as quite a few do now.

I will admit that I do not yet have a good replacement for IO_MEM_ROMD, or toggling the read-only bit on a RAM region. Or even for the way that pc.c allocates RAM around the 640k-1M hole.

Greatly simplify the subpage implementation by not supporting
multiple devices at the same address at different widths. We don't need full copies of mem_read/mem_write/opaque for each address, only a single index back into the main io_mem_* arrays.

Signed-off-by Richard Henderson <[email protected]>

---

  cpu-common.h |    1 -
  exec.c       |  113 ++++++++++++++++++---------------------------------------
  2 files changed, 36 insertions(+), 78 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index b730ca0..b24cecc 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -125,7 +125,6 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
/* Acts like a ROM when read and like a device when written. */

  #define IO_MEM_ROMD        (1)
  #define IO_MEM_SUBPAGE     (2)
 -#define IO_MEM_SUBWIDTH    (4)

  #endif

diff --git a/exec.c b/exec.c
index 43366ac..14d1fd7 100644
--- a/exec.c
+++ b/exec.c
@@ -2549,16 +2549,15 @@ static inline void tlb_set_dirty(CPUState *env,
#define SUBPAGE_IDX(addr) ((addr) & ~TARGET_PAGE_MASK)

  typedef struct subpage_t {
     target_phys_addr_t base;
 -    CPUReadMemoryFunc * const *mem_read[TARGET_PAGE_SIZE][4];
 -    CPUWriteMemoryFunc * const *mem_write[TARGET_PAGE_SIZE][4];
 -    void *opaque[TARGET_PAGE_SIZE][2][4];
 -    ram_addr_t region_offset[TARGET_PAGE_SIZE][2][4];
 +    ram_addr_t sub_io_index[TARGET_PAGE_SIZE];
 +    ram_addr_t region_offset[TARGET_PAGE_SIZE];

} subpage_t;

static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,

                              ram_addr_t memory, ram_addr_t region_offset);
 -static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 -                           ram_addr_t orig_memory, ram_addr_t region_offset);
 +static subpage_t *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 +                                ram_addr_t orig_memory,
 +                                ram_addr_t region_offset);
  #define CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2, \
                       need_subpage)                                     \
     do {                                                                \
 @@ -2596,7 +2595,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
     PhysPageDesc *p;
     CPUState *env;
     ram_addr_t orig_size = size;

- void *subpage;
+ subpage_t *subpage;

     cpu_notify_set_memory(start_addr, size, phys_offset);

@@ -2615,7 +2614,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,

CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2,
need_subpage);
- if (need_subpage || phys_offset & IO_MEM_SUBWIDTH) {

 +            if (need_subpage) {
                 if (!(orig_memory & IO_MEM_SUBPAGE)) {
                     subpage = subpage_init((addr & TARGET_PAGE_MASK),
                                            &p->phys_offset, orig_memory,

@@ -2647,7 +2646,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr,
end_addr2, need_subpage);

- if (need_subpage || phys_offset & IO_MEM_SUBWIDTH) {

 +                if (need_subpage) {
                     subpage = subpage_init((addr & TARGET_PAGE_MASK),
                                            &p->phys_offset, IO_MEM_UNASSIGNED,
                                            addr & TARGET_PAGE_MASK);

@@ -3145,89 +3144,65 @@ static CPUWriteMemoryFunc * const watch_mem_write[3] = {
watch_mem_writel,
};

-static inline uint32_t subpage_readlen (subpage_t *mmio, target_phys_addr_t addr,
- unsigned int len)
+static inline uint32_t subpage_readlen (subpage_t *mmio,
+ target_phys_addr_t addr,
+ unsigned int len)
{
- uint32_t ret;
- unsigned int idx;
-
- idx = SUBPAGE_IDX(addr);
+ unsigned int idx = SUBPAGE_IDX(addr);

  #if defined(DEBUG_SUBPAGE)
     printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", __func__,
            mmio, len, addr, idx);

#endif
- ret = (**mmio->mem_read[idx][len])(mmio->opaque[idx][0][len],
- addr + mmio->region_offset[idx][0][len]);

 -    return ret;
 +    addr += mmio->region_offset[idx];
 +    idx = mmio->sub_io_index[idx];
 +    return io_mem_read[idx][len](io_mem_opaque[idx], addr);

}

static inline void subpage_writelen (subpage_t *mmio, target_phys_addr_t addr,
- uint32_t value, unsigned int len)
+ uint32_t value, unsigned int len)
{
- unsigned int idx;
-
- idx = SUBPAGE_IDX(addr);
+ unsigned int idx = SUBPAGE_IDX(addr);
#if defined(DEBUG_SUBPAGE)
- printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value %08x\n", __func__,
- mmio, len, addr, idx, value);
+ printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value %08x\n",
+ __func__, mmio, len, addr, idx, value);
#endif
- (**mmio->mem_write[idx][len])(mmio->opaque[idx][1][len],
- addr + mmio->region_offset[idx][1][len],
- value);
+

 +    addr += mmio->region_offset[idx];
 +    idx = mmio->sub_io_index[idx];
 +    io_mem_write[idx][len](io_mem_opaque[idx], addr, value);

}

static uint32_t subpage_readb (void *opaque, target_phys_addr_t addr) {
-#if defined(DEBUG_SUBPAGE)
- printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-

     return subpage_readlen(opaque, addr, 0);

}

static void subpage_writeb (void *opaque, target_phys_addr_t addr,

                             uint32_t value)
  {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
 -#endif
     subpage_writelen(opaque, addr, value, 0);

}

static uint32_t subpage_readw (void *opaque, target_phys_addr_t addr) {
-#if defined(DEBUG_SUBPAGE)
- printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-

     return subpage_readlen(opaque, addr, 1);

}

static void subpage_writew (void *opaque, target_phys_addr_t addr,

                             uint32_t value)
  {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
 -#endif
     subpage_writelen(opaque, addr, value, 1);

}

static uint32_t subpage_readl (void *opaque, target_phys_addr_t addr) {
-#if defined(DEBUG_SUBPAGE)
- printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
-#endif
-

     return subpage_readlen(opaque, addr, 2);

}

-static void subpage_writel (void *opaque,
- target_phys_addr_t addr, uint32_t value)
+static void subpage_writel (void *opaque, target_phys_addr_t addr,
+ uint32_t value)
{
-#if defined(DEBUG_SUBPAGE)
- printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
-#endif

     subpage_writelen(opaque, addr, value, 2);

}

@@ -3247,7 +3222,6 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
ram_addr_t memory, ram_addr_t region_offset)

  {
     int idx, eidx;

- unsigned int i;

     if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
         return -1;
 @@ -3257,27 +3231,18 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
     printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__,
            mmio, start, end, idx, eidx, memory);
  #endif
 -    memory >>= IO_MEM_SHIFT;
 +    memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
     for (; idx <= eidx; idx++) {
 -        for (i = 0; i < 4; i++) {
 -            if (io_mem_read[memory][i]) {
 -                mmio->mem_read[idx][i] = &io_mem_read[memory][i];
 -                mmio->opaque[idx][0][i] = io_mem_opaque[memory];
 -                mmio->region_offset[idx][0][i] = region_offset;
 -            }
 -            if (io_mem_write[memory][i]) {
 -                mmio->mem_write[idx][i] = &io_mem_write[memory][i];
 -                mmio->opaque[idx][1][i] = io_mem_opaque[memory];
 -                mmio->region_offset[idx][1][i] = region_offset;
 -            }
 -        }
 +        mmio->sub_io_index[idx] = memory;
 +        mmio->region_offset[idx] = region_offset;
     }

     return 0;

}

-static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
- ram_addr_t orig_memory, ram_addr_t region_offset)
+static subpage_t *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
+ ram_addr_t orig_memory,
+ ram_addr_t region_offset)

  {
     subpage_t *mmio;
     int subpage_memory;
 @@ -3291,8 +3256,7 @@ static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
            mmio, base, TARGET_PAGE_SIZE, subpage_memory);
  #endif
     *phys = subpage_memory | IO_MEM_SUBPAGE;

- subpage_register(mmio, 0, TARGET_PAGE_SIZE - 1, orig_memory,
- region_offset);
+ subpage_register(mmio, 0, TARGET_PAGE_SIZE-1, orig_memory, region_offset);

     return mmio;
  }
 @@ -3322,8 +3286,6 @@ static int cpu_register_io_memory_fixed(int io_index,
                                         CPUWriteMemoryFunc * const *mem_write,
                                         void *opaque)
  {
 -    int i, subwidth = 0;
 -
     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)
 @@ -3334,14 +3296,11 @@ static int cpu_register_io_memory_fixed(int io_index,
             return -1;
     }

- for(i = 0;i < 3; i++) {
- if (!mem_read[i] || !mem_write[i])
- subwidth = IO_MEM_SUBWIDTH;
- io_mem_read[io_index][i] = mem_read[i];
- io_mem_write[io_index][i] = mem_write[i];

 -    }
 +    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
 +    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*));
     io_mem_opaque[io_index] = opaque;

- return (io_index << IO_MEM_SHIFT) | subwidth;
+
+ return (io_index << IO_MEM_SHIFT);
}

int cpu_register_io_memory(CPUReadMemoryFunc * const *mem_read,

--
1.6.6.1

Thanks, applied.

Quoted 268 lines · Richard Henderson

Greatly simplify the subpage implementation by not supporting multiple devices at the same address at different widths.  We don't need full copies of mem_read/mem_write/opaque for each address, only a single index back into the main io_mem_* arrays.

Signed-off-by Richard Henderson <[email protected]>

 ---
  cpu-common.h |    1 -
  exec.c       |  113 ++++++++++++++++++---------------------------------------

  2 files changed, 36 insertions(+), 78 deletions(-)

 diff --git a/cpu-common.h b/cpu-common.h
 index b730ca0..b24cecc 100644
 --- a/cpu-common.h
 +++ b/cpu-common.h
 @@ -125,7 +125,6 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr, /* Acts like a ROM when read and like a device when written.  */ #define IO_MEM_ROMD        (1)
  #define IO_MEM_SUBPAGE     (2)
 -#define IO_MEM_SUBWIDTH    (4)

  #endif

 diff --git a/exec.c b/exec.c
 index 43366ac..14d1fd7 100644
 --- a/exec.c
 +++ b/exec.c
 @@ -2549,16 +2549,15 @@ static inline void tlb_set_dirty(CPUState *env, #define SUBPAGE_IDX(addr) ((addr) & ~TARGET_PAGE_MASK) typedef struct subpage_t {
     target_phys_addr_t base;
 -    CPUReadMemoryFunc * const *mem_read[TARGET_PAGE_SIZE][4];
 -    CPUWriteMemoryFunc * const *mem_write[TARGET_PAGE_SIZE][4];
 -    void *opaque[TARGET_PAGE_SIZE][2][4];
 -    ram_addr_t region_offset[TARGET_PAGE_SIZE][2][4];
 +    ram_addr_t sub_io_index[TARGET_PAGE_SIZE];
 +    ram_addr_t region_offset[TARGET_PAGE_SIZE];
  } subpage_t;

  static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, ram_addr_t memory, ram_addr_t region_offset); -static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 -                           ram_addr_t orig_memory, ram_addr_t region_offset); +static subpage_t *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 +                                ram_addr_t orig_memory,
 +                                ram_addr_t region_offset);
  #define CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2, \
                       need_subpage)                                     \
     do {                                                                \
 @@ -2596,7 +2595,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr, PhysPageDesc *p;
     CPUState *env;
     ram_addr_t orig_size = size;
 -    void *subpage;
 +    subpage_t *subpage;

     cpu_notify_set_memory(start_addr, size, phys_offset);

 @@ -2615,7 +2614,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,

             CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2, need_subpage);
 -            if (need_subpage || phys_offset & IO_MEM_SUBWIDTH) {
 +            if (need_subpage) {
                 if (!(orig_memory & IO_MEM_SUBPAGE)) {
                     subpage = subpage_init((addr & TARGET_PAGE_MASK), &p->phys_offset, orig_memory,
 @@ -2647,7 +2646,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr, CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2, need_subpage);

 -                if (need_subpage || phys_offset & IO_MEM_SUBWIDTH) {
 +                if (need_subpage) {
                     subpage = subpage_init((addr & TARGET_PAGE_MASK), &p->phys_offset, IO_MEM_UNASSIGNED, addr & TARGET_PAGE_MASK); @@ -3145,89 +3144,65 @@ static CPUWriteMemoryFunc * const watch_mem_write[3] = { watch_mem_writel,
  };

 -static inline uint32_t subpage_readlen (subpage_t *mmio, target_phys_addr_t addr,
 -                                 unsigned int len)
 +static inline uint32_t subpage_readlen (subpage_t *mmio,
 +                                        target_phys_addr_t addr,
 +                                        unsigned int len)
  {
 -    uint32_t ret;
 -    unsigned int idx;
 -
 -    idx = SUBPAGE_IDX(addr);
 +    unsigned int idx = SUBPAGE_IDX(addr);
  #if defined(DEBUG_SUBPAGE)
     printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", __func__, mmio, len, addr, idx);
  #endif
 -    ret = (**mmio->mem_read[idx][len])(mmio->opaque[idx][0][len],
 -                                       addr + mmio->region_offset[idx][0][len]);

 -    return ret;
 +    addr += mmio->region_offset[idx];
 +    idx = mmio->sub_io_index[idx];
 +    return io_mem_read[idx][len](io_mem_opaque[idx], addr); }

  static inline void subpage_writelen (subpage_t *mmio, target_phys_addr_t addr,
 -                              uint32_t value, unsigned int len)
 +                                     uint32_t value, unsigned int len)
  {
 -    unsigned int idx;
 -
 -    idx = SUBPAGE_IDX(addr);
 +    unsigned int idx = SUBPAGE_IDX(addr);
  #if defined(DEBUG_SUBPAGE)
 -    printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value %08x\n", __func__,
 -           mmio, len, addr, idx, value);
 +    printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value %08x\n",
 +           __func__, mmio, len, addr, idx, value);
  #endif
 -    (**mmio->mem_write[idx][len])(mmio->opaque[idx][1][len],
 -                                  addr + mmio->region_offset[idx][1][len],
 -                                  value);
 +
 +    addr += mmio->region_offset[idx];
 +    idx = mmio->sub_io_index[idx];
 +    io_mem_write[idx][len](io_mem_opaque[idx], addr, value);
  }

  static uint32_t subpage_readb (void *opaque, target_phys_addr_t addr) {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr); -#endif
 -
     return subpage_readlen(opaque, addr, 0);
  }

  static void subpage_writeb (void *opaque, target_phys_addr_t addr, uint32_t value)
  {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value); -#endif
     subpage_writelen(opaque, addr, value, 0);
  }

  static uint32_t subpage_readw (void *opaque, target_phys_addr_t addr) {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr); -#endif
 -
     return subpage_readlen(opaque, addr, 1);
  }

  static void subpage_writew (void *opaque, target_phys_addr_t addr, uint32_t value)
  {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value); -#endif
     subpage_writelen(opaque, addr, value, 1);
  }

  static uint32_t subpage_readl (void *opaque, target_phys_addr_t addr) {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr); -#endif
 -
     return subpage_readlen(opaque, addr, 2);
  }

 -static void subpage_writel (void *opaque,
 -                         target_phys_addr_t addr, uint32_t value) +static void subpage_writel (void *opaque, target_phys_addr_t addr,
 +                            uint32_t value)
  {
 -#if defined(DEBUG_SUBPAGE)
 -    printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value); -#endif
     subpage_writelen(opaque, addr, value, 2);
  }

 @@ -3247,7 +3222,6 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, ram_addr_t memory, ram_addr_t region_offset)
  {
     int idx, eidx;
 -    unsigned int i;

     if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
         return -1;
 @@ -3257,27 +3231,18 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n", __func__, mmio, start, end, idx, eidx, memory);
  #endif
 -    memory >>= IO_MEM_SHIFT;
 +    memory = (memory >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
     for (; idx <= eidx; idx++) {
 -        for (i = 0; i < 4; i++) {
 -            if (io_mem_read[memory][i]) {
 -                mmio->mem_read[idx][i] = &io_mem_read[memory][i];
 -                mmio->opaque[idx][0][i] = io_mem_opaque[memory];
 -                mmio->region_offset[idx][0][i] = region_offset;
 -            }
 -            if (io_mem_write[memory][i]) {
 -                mmio->mem_write[idx][i] = &io_mem_write[memory][i];
 -                mmio->opaque[idx][1][i] = io_mem_opaque[memory];
 -                mmio->region_offset[idx][1][i] = region_offset;
 -            }
 -        }
 +        mmio->sub_io_index[idx] = memory;
 +        mmio->region_offset[idx] = region_offset;
     }

     return 0;
  }

 -static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 -                           ram_addr_t orig_memory, ram_addr_t region_offset) +static subpage_t *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
 +                                ram_addr_t orig_memory,
 +                                ram_addr_t region_offset)
  {
     subpage_t *mmio;
     int subpage_memory;
 @@ -3291,8 +3256,7 @@ static void *subpage_init (target_phys_addr_t base, ram_addr_t *phys, mmio, base, TARGET_PAGE_SIZE, subpage_memory);
  #endif
     *phys = subpage_memory | IO_MEM_SUBPAGE;
 -    subpage_register(mmio, 0, TARGET_PAGE_SIZE - 1, orig_memory,
 -                         region_offset);
 +    subpage_register(mmio, 0, TARGET_PAGE_SIZE-1, orig_memory, region_offset);

     return mmio;
  }
 @@ -3322,8 +3286,6 @@ static int cpu_register_io_memory_fixed(int io_index, CPUWriteMemoryFunc * const *mem_write, void *opaque)
  {
 -    int i, subwidth = 0;
 -
     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)
 @@ -3334,14 +3296,11 @@ static int cpu_register_io_memory_fixed(int io_index, return -1;
     }

 -    for(i = 0;i < 3; i++) {
 -        if (!mem_read[i] || !mem_write[i])
 -            subwidth = IO_MEM_SUBWIDTH;
 -        io_mem_read[io_index][i] = mem_read[i];
 -        io_mem_write[io_index][i] = mem_write[i];
 -    }
 +    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
 +    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*)); io_mem_opaque[io_index] = opaque;
 -    return (io_index << IO_MEM_SHIFT) | subwidth;
 +
 +    return (io_index << IO_MEM_SHIFT);
  }

  int cpu_register_io_memory(CPUReadMemoryFunc * const *mem_read,

--
 1.6.6.1

This patch introduces a regression. qemu crashes on lance test:

Quoted 1 line

This patch introduces a regression. qemu crashes on lance test:

I'm not sure how to get to this, since the sparc-test images don't include ifconfig, and I havn't been able to find a sparc install image that works (doesn't support sparc32 or sparc64 fails to load).

That said, try this and see if it works.

r~

---

exec.c +3 -2

index 14d1fd7..572d3fd 100644
@@ -3286,6 +3286,8 @@ static int cpu_register_io_memory_fixed(int io_index,
                                         CPUWriteMemoryFunc * const *mem_write,
                                         void *opaque)
  {
+    int i;
+
     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)

@@ -3296,8 +3298,14 @@ static int cpu_register_io_memory_fixed(int io_index,
             return -1;
     }
 
-    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
-    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*));
+    for (i = 0; i < 3; ++i) {

2010/4/27 Richard Henderson <[email protected]>:

Quoted 42 lines · Artyom Tarasenko

Quoted 1 line

This patch introduces a regression. qemu crashes on lance test:

I'm not sure how to get to this, since the sparc-test images don't include ifconfig, and I havn't been able to find a sparc install image that works (doesn't support sparc32 or sparc64 fails to load).

That said, try this and see if it works.

r~

---

exec.c +10 -2

index 14d1fd7..572d3fd 100644
@@ -3286,6 +3286,8 @@ static int cpu_register_io_memory_fixed(int io_index,
                                         CPUWriteMemoryFunc * const *mem_write,
                                         void *opaque)
  {
+    int i;
+
     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)

@@ -3296,8 +3298,14 @@ static int cpu_register_io_memory_fixed(int io_index,
             return -1;
     }
 
-    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
-    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*));
+    for (i = 0; i < 3; ++i) {
+        io_mem_read[io_index][i]
+            = (mem_read[i] ? mem_read[i] : unassigned_mem_read[i]);
+    }
+    for (i = 0; i < 3; ++i) {
+        io_mem_write[io_index][i]
+            = (mem_write[i] ? mem_write[i] : unassigned_mem_write[i]);
+    }
     io_mem_opaque[io_index] = opaque;
 

exec.c static int cpu_register_io_memory_fixed(int io_index, · line 3311

Quoted 2 lines

exec.c +0 -0

     return (io_index << IO_MEM_SHIFT);

Looks good, thanks.

Acked-by Artyom Tarasenko <[email protected]>

2010/4/28 Artyom Tarasenko <[email protected]>:

Quoted 43 lines

2010/4/27 Richard Henderson <[email protected]>:

Quoted 42 lines · Artyom Tarasenko

Quoted 1 line

This patch introduces a regression. qemu crashes on lance test:

I'm not sure how to get to this, since the sparc-test images don't include ifconfig, and I havn't been able to find a sparc install image that works (doesn't support sparc32 or sparc64 fails to load).

That said, try this and see if it works.

r~

---

exec.c +10 -2

index 14d1fd7..572d3fd 100644
@@ -3286,6 +3286,8 @@ static int cpu_register_io_memory_fixed(int io_index,
                                        CPUWriteMemoryFunc * const *mem_write,
                                        void *opaque)
 {
+    int i;
+
    if (io_index <= 0) {
        io_index = get_free_io_mem_idx();
        if (io_index == -1)

@@ -3296,8 +3298,14 @@ static int cpu_register_io_memory_fixed(int io_index,
            return -1;
    }
 
-    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
-    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*));
+    for (i = 0; i < 3; ++i) {
+        io_mem_read[io_index][i]
+            = (mem_read[i] ? mem_read[i] : unassigned_mem_read[i]);
+    }
+    for (i = 0; i < 3; ++i) {
+        io_mem_write[io_index][i]
+            = (mem_write[i] ? mem_write[i] : unassigned_mem_write[i]);
+    }
    io_mem_opaque[io_index] = opaque;
 

Quoted 2 lines

exec.c static int cpu_register_io_memory_fixed(int io_index, · line 3311

Quoted 2 lines

exec.c +0 -0

    return (io_index << IO_MEM_SHIFT);

Why the fix didn't make it into the git?
Does it introduce other problems?

Quoted 4 lines

Looks good, thanks.

Acked-by Artyom Tarasenko <[email protected]>

--
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

Implement the "functions may be omitted with NULL pointer"
interface mentioned in the function block comment by transforming NULL entries in the read/write arrays into calls to the unassigned_mem family of functions.

Signed-off-by Richard Henderson <[email protected]>

---

  exec.c |   12 ++++++++++--
  1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index e980788..3416aed 100644
--- a/exec.c
+++ b/exec.c
@@ -3262,6 +3262,8 @@ static int cpu_register_io_memory_fixed(int io_index,
CPUWriteMemoryFunc * const *mem_write, void *opaque)
{
+ int i;
+

     if (io_index <= 0) {
         io_index = get_free_io_mem_idx();
         if (io_index == -1)
 @@ -3272,8 +3274,14 @@ static int cpu_register_io_memory_fixed(int io_index,
             return -1;
     }

 -    memcpy(io_mem_read[io_index], mem_read, 3 * sizeof(CPUReadMemoryFunc*));
 -    memcpy(io_mem_write[io_index], mem_write, 3 * sizeof(CPUWriteMemoryFunc*));
 +    for (i = 0; i < 3; ++i) {
 +        io_mem_read[io_index][i]
 +            = (mem_read[i] ? mem_read[i] : unassigned_mem_read[i]);
 +    }
 +    for (i = 0; i < 3; ++i) {
 +        io_mem_write[io_index][i]
 +            = (mem_write[i] ? mem_write[i] : unassigned_mem_write[i]);
 +    }
     io_mem_opaque[io_index] = opaque;

     return (io_index << IO_MEM_SHIFT);

--
1.6.6.1

The basic device interface looks like
...

(quoted diff) +14 -0

+
+/* Register a memory region at START_ADDR/SIZE.  The REGION structure will
+   be initialized appropriately for DEV using CB as the operation set.  */
+extern void cpu_register_memory_region(MemoryRegion *region,
+                                       const MemoryCallbackInfo *cb,
+                                       target_phys_addr_t start_addr,
+                                       target_phys_addr_t size);
+
+/* Unregister a memory region.  */
+extern void cpu_unregister_memory_region(MemoryRegion *);
+
+/* Allocate ram for use with cpu_register_memory_region.  */
+extern const MemoryCallbackInfo *qemu_ram_alloc_r(ram_addr_t);
+extern void qemu_ram_free_r(const MemoryCallbackInfo *);

The Basic Idea is that we have a MemoryRegion object that describes a contiguous mapping within the guest address space. This object needs to handle RAM, ROM and devices. The desire to handle memory and devices the same comes from the wish to have PCI device BARs show up as plain memory in the TLB as plain memory, and to be able to handle all PCI device regions identically within sysbus.

I will admit that I do not yet have a good replacement for IO_MEM_ROMD,

or toggling the read-only bit on a RAM region.