Re: [PATCH v6 02/11] arch, arm: add consistency check to REMOVE p2m changes

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

Hi Arianna,

On 21/04/14 14:44, Arianna Avanzini wrote:
> ---
>   xen/arch/arm/p2m.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 403fd89..7f03aa7 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -319,6 +319,7 @@ static int apply_p2m_changes(struct domain *d,
>               if ( !populate )
>               {
>                   addr = (addr + FIRST_SIZE) & FIRST_MASK;
> +                maddr = (maddr + FIRST_SIZE) & FIRST_MASK;

We differ a bit from x86 on this point. We will silently ignore that the 
GFN is not mapped when op == REMOVE.

I'm wondering if we should return an error in this case. I don't find 
any place where it's valid to remove non-present mapping.

[..]

> @@ -406,12 +408,26 @@ static int apply_p2m_changes(struct domain *d,
>                   {
>                       pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
>                       write_pte(&third[third_table_offset(addr)], pte);
> -                    maddr += PAGE_SIZE;
>                   }
>                   break;
> -            case RELINQUISH:
>               case REMOVE:
>                   {
> +                    unsigned long mfn = pte.p2m.base;
> +
> +                    /*
> +                     * Ensure that the guest address given as argument to
> +                     * this function is actually mapped to the specified
> +                     * machine address. 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 ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> +                        return -EINVAL;
> +                }
> +                /* fall through */
> +            case RELINQUISH:
> +                {
>                       if ( !pte.p2m.valid )
>                       {
>                           count++;
> @@ -425,7 +441,6 @@ static int apply_p2m_changes(struct domain *d,
>                       count++;
>                   }
>                   break;
> -

Spurious change here.

Regards

-- 
Julien Grall