Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 27, 2011 at 04:21:48PM +0300, Avi Kivity wrote:

...

> +static bool memory_region_access_valid(MemoryRegion *mr,
> +                                       target_phys_addr_t addr,
> +                                       unsigned size)
> +{
> +    if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> +        return false;
> +    }
> +
> +    /* Treat zero as compatibility all valid */
> +    if (!mr->ops->valid.max_access_size) {
> +        return true;
> +    }
> +
> +    if (size > mr->ops->valid.max_access_size
> +        || size < mr->ops->valid.min_access_size) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static uint32_t memory_region_read_thunk_n(void *_mr,
> +                                           target_phys_addr_t addr,
> +                                           unsigned size)
> +{
> +    MemoryRegion *mr = _mr;
> +    unsigned access_size, access_size_min, access_size_max;
> +    uint64_t access_mask;
> +    uint32_t data = 0, tmp;
> +    unsigned i;
> +
> +    if (!memory_region_access_valid(mr, addr, size)) {
> +        return -1U; /* FIXME: better signalling */
> +    }
> +
> +    /* FIXME: support unaligned access */
> +
> +    access_size_min = mr->ops->impl.max_access_size;

min = max: Intentional? Cut&paste error?

> +    if (!access_size_min) {
> +        access_size_min = 1;
> +    }
> +    access_size_max = mr->ops->impl.max_access_size;
> +    if (!access_size_max) {
> +        access_size_max = 4;
> +    }

...

> diff --git a/memory.h b/memory.h
> new file mode 100644
> index 0000000..a67ff94
> --- /dev/null
> +++ b/memory.h
> @@ -0,0 +1,201 @@
> +#ifndef MEMORY_H
> +#define MEMORY_H
> +
> +#ifndef CONFIG_USER_ONLY

What's the story with this ifdef?
There are no stubs provided ...

> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include "qemu-common.h"
> +#include "cpu-common.h"
> +#include "targphys.h"
> +#include "qemu-queue.h"
> +
> +typedef struct MemoryRegionOps MemoryRegionOps;
> +typedef struct MemoryRegion MemoryRegion;
> +
> +/* Must match *_DIRTY_FLAGS in cpu-all.h.  To be replaced with dynamic
> + * registration.
> + */
> +#define DIRTY_MEMORY_VGA       0
> +#define DIRTY_MEMORY_CODE      1
> +#define DIRTY_MEMORY_MIGRATION 3
> +
> +/*
> + * Memory region callbacks
> + */
> +struct MemoryRegionOps {
> +    /* Read from the memory region. @addr is relative to @mr; @size is
> +     * in bytes. */
> +    uint64_t (*read)(MemoryRegion *mr,
> +                     target_phys_addr_t addr,
> +                     unsigned size);
> +    /* Write to the memory region. @addr is relative to @mr; @size is
> +     * in bytes. */
> +    void (*write)(MemoryRegion *mr,
> +                  target_phys_addr_t addr,
> +                  uint64_t data,
> +                  unsigned size);
> +
> +    enum device_endian endianness;
> +    /* Guest-visible constraints: */
> +    struct {
> +        /* If nonzero, specify bounds on access sizes beyond which a machine
> +         * check is thrown.
> +         */
> +        unsigned min_access_size;
> +        unsigned max_access_size;
> +        /* If true, unaligned accesses are supported.  Otherwise unaligned
> +         * accesses throw machine checks.
> +         */
> +         bool unaligned;
> +    } valid;
> +    /* Internal implementation constraints: */
> +    struct {
> +        /* If nonzero, specifies the minimum size implemented.  Smaller sizes
> +         * will be rounded upwards and a partial result will be returned.
> +         */
> +        unsigned min_access_size;
> +        /* If nonzero, specifies the maximum size implemented.  Larger sizes
> +         * will be done as a series of accesses with smaller sizes.
> +         */
> +        unsigned max_access_size;
> +        /* If true, unaligned accesses are supported.  Otherwise all accesses
> +         * are converted to (possibly multiple) naturally aligned accesses.
> +         */
> +         bool unaligned;
> +    } impl;
> +};
> +
> +typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> +
> +struct MemoryRegion {
> +    /* All fields are private - violators will be prosecuted */
> +    const MemoryRegionOps *ops;
> +    MemoryRegion *parent;
> +    uint64_t size;
> +    target_phys_addr_t addr;
> +    target_phys_addr_t offset;
> +    ram_addr_t ram_addr;
> +    bool has_ram_addr;
> +    MemoryRegion *alias;
> +    target_phys_addr_t alias_offset;
> +    unsigned priority;
> +    bool may_overlap;
> +    QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> +    QTAILQ_ENTRY(MemoryRegion) subregions_link;
> +    QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> +    const char *name;

I'm never completely sure whether these should be target addresses
or bus addresses or just uint64_t.
With pci on a 32 bit system you can stick a 64 bit address
in a BAR and the result will be that it is never accessed
from the CPU.

-- 
MST