Re: [PATCH v3 2/5] arch, arm: add consistency checks to REMOVE p2m changes

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

Hello Arianna,

Thanks for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
> ---
>   xen/arch/arm/p2m.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..47bf154 100644
> --- a/xen/arcah/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -243,7 +243,8 @@ static int apply_p2m_changes(struct domain *d,
>       int rc;
>       struct p2m_domain *p2m = &d->arch.p2m;
>       lpae_t *first = NULL, *second = NULL, *third = NULL;
> -    paddr_t addr;
> +    p2m_type_t _t;
> +    paddr_t addr, _maddr = INVALID_PADDR;
>       unsigned long cur_first_page = ~0,
>                     cur_first_offset = ~0,
>                     cur_second_offset = ~0;
> @@ -252,6 +253,20 @@ static int apply_p2m_changes(struct domain *d,
>       bool_t populate = (op == INSERT || op == ALLOCATE);
>       lpae_t pte;
>
> +    /*
> +     * As of now, the lookup is needed only in in case
> +     * of REMOVE operation, as a consistency check on
> +     * the existence of a mapping between the machine
> +     * address and the start guest address given as
> +     * parameters.
> +     */
> +    if (op == REMOVE)
> +        /*
> +         * Be sure to lookup before grabbing the p2m_lock,
> +         * as the p2m_lookup() function holds it too.
> +         */
> +        _maddr = p2m_lookup(d, start_gpaddr, &_t);
> +

Did you try remove path? apply_p2m_changes is taking p2m->lock which is 
also taken by p2m_lookup. With this solution it will end up to a deadlock.

Anyway, you don't need to use p2m_lookup because you already have all 
the data in pte (if pte.p2m.valid == 1):
    - pte.p2m.type  = p2m type
    - pte.p2m.base  = MFN

>       spin_lock(&p2m->lock);
>
>       if ( d != current->domain )
> @@ -367,9 +382,23 @@ static int apply_p2m_changes(struct domain *d,
>                       maddr += PAGE_SIZE;
>                   }
>                   break;
> -            case RELINQUISH:
>               case REMOVE:
>                   {
> +                    /*
> +                     * Ensure that, if we are trying to unmap I/O memory
> +                     * ranges, the given gfn is p2m_mmio_direct.
> +                     */

> +                    if ( t == p2m_mmio_direct ? _t != p2m_mmio_direct : 0 ||
> +                         paddr_to_pfn(_maddr) == INVALID_MFN ||

Testing pte.p2m.valid instead of paddr_to(_maddr)... is right answer.

Moreover, why do you need to check t? Every call to 
guest_physmap_remove_page is done with a valid mfn (I guess it can be 
enhanced by a BUG_ON(mfn != INVALID_MFN) in this function).


> +                         maddr != _maddr )

maddr is not incremented during where the page is removed. The next 
iteration will likely fail. You need to increment it in various place.

> +                    {
> +                        count++;
> +                        break;

IHMO, skipping the page is totally wrong. You should return an error here.

Regards,

-- 
Julien Grall