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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/28/2011 01:03 PM, Michael S. Tsirkin wrote:
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?

Bug; thanks.

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

No callers either - I build with a full configuration. I prefer the #ifdef here rather than all call sites.

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


I agree.  Anyone objects to making the memory API 64-bit?

It will reduce performance slightly for 32-on-32, but these configurations are getting rarer, and the performance loss is quite small.

Maybe we should make t_p_a_t 64-bit unconditionally. Note that sizes have to be 64-bit in any case, otherwise you can't express a 4G range without tricks.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux