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

[Blue Swirl]

On 4/23/10, Richard Henderson <address@hidden> 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.