From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v10 01/12] arch/arm: add consistency check
 to REMOVE p2m changes
Date: Tue, 29 Jul 2014 12:55:43 +0100
Message-ID: <53D78BBF.7080108@linaro.org>
References: <1406585529-32193-1-git-send-email-avanzini.arianna@gmail.com>
	<1406585529-32193-2-git-send-email-avanzini.arianna@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1406585529-32193-2-git-send-email-avanzini.arianna@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Arianna Avanzini <avanzini.arianna@gmail.com>, xen-devel@lists.xen.org
Cc: julien.grall@citrix.com, paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, JBeulich@suse.com, andrew.cooper3@citrix.com, viktor.kleinik@globallogic.com
List-Id: xen-devel@lists.xenproject.org

Hi Arianna,

On 07/28/2014 11:11 PM, Arianna Avanzini wrote:
> @@ -609,26 +610,42 @@ static int apply_one_level(struct domain *d,
>              if ( p2m_table(orig_pte) )
>                  return P2M_ONE_DESCEND;
>  
> -            if ( op == REMOVE &&
> -                 !is_mapping_aligned(*addr, end_gpaddr,
> -                                     0, /* maddr doesn't matter for remove */
> -                                     level_size) )
> +            if ( op == REMOVE )
>              {
> -                /*
> -                 * Removing a mapping from the middle of a superpage. Shatter
> -                 * and descend.
> -                 */
> -                *flush = true;
> -                rc = p2m_create_table(d, entry,
> -                                      level_shift - PAGE_SHIFT, flush_cache);
> -                if ( rc < 0 )
> -                    return rc;
> -
> -                p2m->stats.shattered[level]++;
> -                p2m->stats.mappings[level]--;
> -                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
> -
> -                return P2M_ONE_DESCEND;
> +                 if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) )
> +                 {

You don't cover every case with this solution. This code is only called
when (level < 3). So for single 4k (level 3), Xen will not print a
message when the MFN differ.

I still think that the best place to check it, is after the if ( level < 3 )

The code would look like:

if ( level < 3 )
{
  ....
}

if ( op == REMOVE && pfn_to_paddr(orig_pte.p2m.base) != *maddr )
	printk(XENLOG_ERR "p2m_remove"....);

> +                      unsigned long mfn = orig_pte.p2m.base;
> +                      /*
> +                       * Ensure that the guest address addr currently being
> +                       * handled (that is in the range given as argument to
> +                       * this function) is actually mapped to the corresponding
> +                       * machine address in the specified range. maddr here is
> +                       * the machine address given to the function, while mfn
> +                       * is the machine frame number actually mapped to the
> +                       * guest address: check if the two correspond.
> +                       */
> +                       if ( *maddr != pfn_to_paddr(mfn) )
> +                           printk("p2m_remove dom%d: mapping at %"PRIpaddr" is of maddr %"PRIpaddr" not %"PRIpaddr" as expected\n",

You have to prefix it at least by XENLOG_WARNING or XENLOG_ERR

Regards,

-- 
Julien Grall