Re: [PATCH v10 01/12] arch/arm: add consistency check to REMOVE p2m changes

[Julien Grall <julien.grall@linaro.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