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

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

Hi Arianna,

On 05/05/2014 04:54 PM, Arianna Avanzini wrote:
> Currently, the REMOVE case of the switch in apply_p2m_changes()
> does not perform any consistency check on the mapping to be removed.
> More in detail, the code does not check if the guest address to be
> unmapped is actually mapped to the machine address given as a
> parameter.
> This commit attempts to add the above-described consistency check
> to the REMOVE path of apply_p2m_changes(). This is instrumental to
> one of the following commits which implements the possibility to
> trigger the removal of p2m ranges via the memory_mapping DOMCTL
> for ARM.
> 
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> 
> ---
> 
>     v7:
>         - Silently ignore the fact that, when removing a mapping, the specified
>           gfn is not mapped at all.

I think you misunderstood my previous comment. I didn't ask to remove
"maddr += ...". This code was right. Now the failure (i.e the MFN
doesn't match the GFN) is obscure.

On x86, Xen will continue to unmap even if we fail to remove one entry.
Of course, it will return an error at the end.

> ---
>  xen/arch/arm/p2m.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 403fd89..17b0635 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -406,12 +406,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;

I didn't catch it until now, this is wrong. In case of an error you have to:
	- unmap PT table mapping
        - unlock p2m lock
	- flush the TLBs (if we successfully removed other mapping)

Regards,

-- 
Julien Grall