Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Paul Brook]

[Hit send too early on previous mail...]
> The basic device interface looks like
> ...
> +
> +/* 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.

Looks reasonable to me.
I'm tempted to add a DeviceState* argument to cpu_register_memory_region.
This might be informative for debugging, and allow future disjoint bus 
support. OTOH it might be more trouble than it's worth.
 
> 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.

I suggest adding a MemoryCallbackInfo* argument to qemu_ram_alloc_r.
If NULL this allocates a regular RAM block.  if non-null it allocates a hybrid 
ROM/IO block using the specified callbacks for IO accesses.  Then introduce a 
function that allows a device to switch a mapping between ROM and IO modes.  
Likewise to make a mapping of a RAM region readonly.



Paul

[Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Richard Henderson]

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

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Blue Swirl]

On 4/20/10, Richard Henderson <rth@twiddle.net> wrote:
> 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?

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.

>  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 would not call for example Enterprise M9000 an embedded system.

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

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Richard Henderson]

On 04/22/2010 12:38 PM, Blue Swirl wrote:
> Subpages are used when there are several devices on the same page.
> It's needed for at least Sparc32.

That's fine.

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

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


r~

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Blue Swirl]

On 4/22/10, Richard Henderson <rth@twiddle.net> wrote:
> On 04/22/2010 12:38 PM, Blue Swirl wrote:
>  > Subpages are used when there are several devices on the same page.
>  > It's needed for at least Sparc32.
>
>
> That's fine.
>
>
>  > 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.

Perhaps also the bus layer could do something here.

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

Yes, I also agree it's a bit overkill.

>  I'll submit a patch to clean this up.
>
>
>  > 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.

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

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Richard Henderson]

On 04/22/2010 01:01 PM, Blue Swirl wrote:
>> 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?

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


r~

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Blue Swirl]

On 4/23/10, Richard Henderson <rth@twiddle.net> wrote:
> On 04/22/2010 01:01 PM, Blue Swirl wrote:
>  >> 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?

If we had a bus layer that handled conversion from bus width to device
width (discussed earlier), it could as well handle the cases where
instead of conversion, a bus error should be reported.

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

Perhaps with small changes we could make this design stackable, so
that a device providing a bus inside another bus could use the same
registration functions, only the bus handle would change. Like:

upa = bus_register_memory_region(cpu_bus,,);
pci = bus_register_memory_region(upa,,);
ebus = bus_register_memory_region(pci,,):
ide = bus_register_memory_region(ebus,,):

Currently "cpu_bus" is always implicit and pci has separate
registration functions.

I'd suppose that ROMD, subpages and unassigned memory handlers could
be implemented by stacking instead of special casing them.

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

Read-only bit could be implemented in the stackable design by creating
a simple bus which can suppress writes when configured so, The device
behind would be just RAM.

Likewise for PC RAM mapping, the memory controller provides a bus
where the addresses may be twisted as needed before passing to
underlying RAM.

However, the outcome of the Generic DMA discussions (last year?) was
that instead of read/write functions, a mapping API would be better
because then we have the possibility to do zero copy DMA. Perhaps the
same applies here, too. This would be a bigger change obviously but if
there will be widespread changes to devices, it would be nice to get
this right.

Re: [Qemu-devel] [PATCH 0/2] [RFC] 64-bit io paths

[Paul Brook]

> The basic device interface looks like
> ...
> +
> +/* 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.

Looks reasonable to me.
I'm tempted to add a DeviceState* argument to cpu_register_memory_region.
This might be informative for debugging, and allow future disjoint bus 
support. OTOH it may be more trouble than it's worth.
 
> 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.