From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44646)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1YVl92-0000x9-2V
	for qemu-devel@nongnu.org; Wed, 11 Mar 2015 14:14:12 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1YVl8x-0007jC-1D
	for qemu-devel@nongnu.org; Wed, 11 Mar 2015 14:14:12 -0400
Received: from mail-lb0-f174.google.com ([209.85.217.174]:46071)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1YVl8w-0007j6-Pu
	for qemu-devel@nongnu.org; Wed, 11 Mar 2015 14:14:06 -0400
Received: by lbiz12 with SMTP id z12so10720163lbi.12
	for <qemu-devel@nongnu.org>; Wed, 11 Mar 2015 11:14:06 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <1426097167-13080-4-git-send-email-drjones@redhat.com>
References: <1426097167-13080-1-git-send-email-drjones@redhat.com>
	<1426097167-13080-4-git-send-email-drjones@redhat.com>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Wed, 11 Mar 2015 18:13:44 +0000
Message-ID: <CAFEAcA_qGPdhVz8Dy=PCwL2ijKmSdwp-1hZMSGC_5R8RcQAPcg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH v3 3/3] target-arm: get_phys_addr_lpae:
	more xn control
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: Andrew Jones <drjones@redhat.com>
Cc: QEMU Developers <qemu-devel@nongnu.org>

On 11 March 2015 at 18:06, Andrew Jones <drjones@redhat.com> wrote:
> +    if (is_aa64) {
> +        switch (regime_el(env, mmu_idx)) {
> +        case 1:
> +            if (is_user && !(user_rw & PAGE_READ)) {
> +                wxn = 0;

I still can't figure out the point of this.
This conditional will only be true if this is a
user access and user_rw is zero (indicating a page which
is neither readable nor writeable)...

> +            } else if (!is_user) {
> +                xn = pxn || (user_rw & PAGE_WRITE);
> +            }
> +            break;
> +        case 2:
> +        case 3:
> +            break;
> +        }
> +    } else if (arm_feature(env, ARM_FEATURE_V7)) {
> +        switch (regime_el(env, mmu_idx)) {
> +        case 1:
> +        case 3:
> +            if (is_user) {
> +                xn = xn || !(user_rw & PAGE_READ);
> +            } else {
> +                int uwxn = 0;
> +                if (have_wxn) {
> +                    uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN;
> +                }
> +                xn = xn || !(prot_rw & PAGE_READ) || pxn ||
> +                     (uwxn && (user_rw & PAGE_WRITE));
> +            }
> +            break;
> +        case 2:
> +            break;
> +        }
> +    } else {
> +        xn = wxn = 0;
> +    }

...and in that code path the only place wxn is used
later is in this check:

> +    if (xn || (wxn && (prot_rw & PAGE_WRITE))) {

...but in that case, we know that user_rw == prot_rw,
and (prot_rw & PAGE_WRITE) must be zero. So the
"(wxn && (prot_rw & PAGE_WRITE))" part of the condition
must be false, regardless of whether wxn is true or
false.

So why did we bother changing wxn in the code above?

> +        return prot_rw;
> +    }
> +    return prot_rw | PAGE_EXEC;
> +}

I don't see what I'm missing...

thanks
-- PMM