Re: [PATCH RESEND v9 01/14] arch/arm: add consistency check to REMOVE p2m changes

[Julien Grall <julien.grall@linaro.org>]

Hi Arianna,

On 14/07/14 01:50, Arianna Avanzini wrote:
>   xen/arch/arm/p2m.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c0c011a..5a77189 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -596,12 +596,30 @@ static int apply_one_level(struct domain *d,
>
>           break;
>
> -    case RELINQUISH:
>       case REMOVE:
> +        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) )
> +        {
> +            unsigned long mfn = orig_pte.p2m.base;

I think the check is misplaced, it should go after:

if ( level < 3 && p2m_table(orig_pte) )
   return P2M_ONE_DESCEND;

It's because this code path can now be called with either a P2M Table or 
a P2M entry. You will get wrong error message and may log many times the 
same issue.

Also I've reported an issue to Ian about this code path today. If the 
guest balloning driver or your MMIO unmap a single 4K entry, it may fail 
or remove a  superpage by mistake.

> +            /*
> +             * 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 ( p2m_valid(orig_pte) && *maddr != pfn_to_paddr(mfn) )
> +                 printk("p2m_remove: mapping at %"PRIpaddr" is of maddr %"PRIpaddr" not %"PRIpaddr" as expected\n",
> +                        *addr, pfn_to_paddr(mfn), *maddr);

You have to print the domain id, otherwise we don't know where the issue 
comes from.

As said on the previous version, you can't use gdprintk to print the 
domain id. So you will have to do it yourself.

Regards,

-- 
Julien Grall