From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbWpN-0004TW-5x for qemu-devel@nongnu.org; Tue, 28 Jun 2011 07:51:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QbWpK-0002Mb-4l for qemu-devel@nongnu.org; Tue, 28 Jun 2011 07:51:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44383) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbWpJ-0002MD-I8 for qemu-devel@nongnu.org; Tue, 28 Jun 2011 07:51:33 -0400 Message-ID: <4E09C03F.4070205@redhat.com> Date: Tue, 28 Jun 2011 14:51:27 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1309180927-19003-1-git-send-email-avi@redhat.com> <1309180927-19003-2-git-send-email-avi@redhat.com> <20110628100343.GA21866@redhat.com> In-Reply-To: <20110628100343.GA21866@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org 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