Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes

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

Hi Arianna,

On 25/05/14 11:51, Arianna Avanzini wrote:
> +                    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) )
> +                    {
> +                        printk("p2m_remove: nonexistent mapping: "
> +                               "%"PRIx64" and %"PRIx64"\n",
> +                               pfn_to_paddr(mfn), maddr);
> +                        /*
> +                         * If we have successfully removed other mappings,
> +                         * overload flush local variable to store if we need
> +                         * to flush TLBs.
> +                         */
> +                        if (count) flush = 1; else flush = 0;

Hrrm, why do you need this line? Flush is already correctly set 
previously (see flush |= ... few lines above).

> +                        rc = -EINVAL;
> +                        goto out_flush;

As I said on a previous mail, you should continue to unmap even if we 
fail to remove one mapping. Otherwise, we may let the guest access to 
some MMIO region by mistake.

> +                    }
> +                }
> +                /* fall through */
> +            case RELINQUISH:
> +                {
>                       if ( !pte.p2m.valid )
>                       {
>                           count++;
> @@ -462,28 +490,30 @@ static int apply_p2m_changes(struct domain *d,
>
>           /* Got the next page */
>           addr += PAGE_SIZE;
> +        maddr += PAGE_SIZE;
>       }
>
> -    if ( flush )
> +    if ( op == ALLOCATE || op == INSERT )
>       {
>           unsigned long sgfn = paddr_to_pfn(start_gpaddr);
>           unsigned long egfn = paddr_to_pfn(end_gpaddr);
>
> -        flush_tlb_domain(d);
> -        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
> +        p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
>       }
>
> -    if ( op == ALLOCATE || op == INSERT )
> +    rc = 0;
> +
> +out_flush:

There is no need to create a new label. You can move the label "out" 
here and avoid to invert the position of the 2 if. It doesn't harm to 
update p2m->max_mapped_gfn and p2m->lowest_mapped_gfn and/or flush TLBs 
if we fail.

It could also be safeguard for later :).

Regards,

-- 
Julien Grall