From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:45178)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfZtB-0004Je-OP
	for qemu-devel@nongnu.org; Thu, 23 May 2013 14:05:27 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfZt7-000694-4J
	for qemu-devel@nongnu.org; Thu, 23 May 2013 14:05:21 -0400
Received: from mail-lb0-f170.google.com ([209.85.217.170]:48823)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1UfZt6-00068w-U7
	for qemu-devel@nongnu.org; Thu, 23 May 2013 14:05:17 -0400
Received: by mail-lb0-f170.google.com with SMTP id t13so3764921lbd.29
	for <qemu-devel@nongnu.org>; Thu, 23 May 2013 11:05:16 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <1369133851-1894-16-git-send-email-pbonzini@redhat.com>
References: <1369133851-1894-1-git-send-email-pbonzini@redhat.com>
	<1369133851-1894-16-git-send-email-pbonzini@redhat.com>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Thu, 23 May 2013 19:04:55 +0100
Message-ID: <CAFEAcA-LTgN_W_2kSKUoBPtgyuEV0h=GDABAh+gqo6FHHQTshw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH 15/30] memory: add address_space_valid
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: jan.kiszka@gmail.com, qemu-devel@nongnu.org, David Gibson <david@gibson.dropbear.id.au>

On 21 May 2013 11:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
> +bool address_space_valid(AddressSpace *as, hwaddr addr, int len, bool is_write)
> +{
> +    AddressSpaceDispatch *d = as->dispatch;
> +    MemoryRegionSection *section;
> +    int l;
> +    hwaddr page;
> +
> +    while (len > 0) {
> +        page = addr & TARGET_PAGE_MASK;
> +        l = (page + TARGET_PAGE_SIZE) - addr;
> +        if (l > len) {
> +            l = len;
> +        }
> +        section = phys_page_find(d, addr >> TARGET_PAGE_BITS);
> +        if (section->mr == &io_mem_unassigned ||
> +            (is_write && section->mr->readonly)) {

It's kind of bogus that io_mem_unassigned is the only MemoryRegion
that can be unreadable. Why is 'readonly' a MemoryRegion attribute
and 'writeonly' not? Shouldn't we be calling the MemoryRegionOps
accepts() callback here? What about access alignment constraints
and access size restrictions? What if the validity of the range
changes between the time you asked and when you actually do the
access?

The whole API is kind of unconvincing really, especially since
the only thing we seem to use it for is some parameter checking
in spapr_llan.c (via a huge pile of intermediate wrappers).

thanks
-- PMM