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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/07/14 12:55, Julien Grall wrote:
> 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,
>

XENLOG_G_WARNING or XENLOG_G_ERR

Guest printk()s are rate limited differently to core hypervisor
printk()s, and have a separate controllable loglevel.

~Andrew