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

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

Hi Arianna,

Thank you for the patch,

On 03/25/2014 02:02 AM, 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>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> 
> ---
> 
>     v4:
>         - Remove useless and slow lookup and use already-available
>           data from pte instead.
>         - Correctly increment the local variable used to keep the
>           machine address whose mapping is currently being removed.
>         - Return with an error upon finding a mismatch between the
>           actual machine address mapped to the guest address and
>           the machine address passed as parameter, instead of just
>           skipping the page.
> 
> ---
>  xen/arch/arm/p2m.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bb0db16 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -243,12 +243,13 @@ 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;
> +    paddr_t addr, _maddr;
>      unsigned long cur_first_page = ~0,
>                    cur_first_offset = ~0,
>                    cur_second_offset = ~0;
>      unsigned long count = 0;
>      unsigned int flush = 0;
> +    unsigned long mfn;
>      bool_t populate = (op == INSERT || op == ALLOCATE);
>      lpae_t pte;
>  
> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
>          p2m_load_VTTBR(d);
>  
>      addr = start_gpaddr;
> +    _maddr = maddr;

You don't need a temporary variable to hold maddr and increment it.
You can directly use maddr, but you will have to remove "maddr +=
PAGE_SIZE" in INSERT.

You also need to increment maddr when first/second level are not valid.

While reading the code, I'm wondering if we need to return an error if
we are trying to remove a non-existent page. Ian, Stefano, any thoughs?

>      while ( addr < end_gpaddr )
>      {
>          if ( cur_first_page != p2m_first_level_index(addr) )
> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
>  
>          flush |= pte.p2m.valid;
>  
> +        mfn = pte.p2m.base;

I would move mfn = ... in REMOVE part because it may not be valid on
some place and wrongly used in the future.

>          /* TODO: Handle other p2m type
>           *
>           * It's safe to do the put_page here because page_alloc will
> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
>           */
>          if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
>          {
> -            unsigned long mfn = pte.p2m.base;
> -
>              ASSERT(mfn_valid(mfn));
>              put_page(mfn_to_page(mfn));
>          }
> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
>                      maddr += PAGE_SIZE;
>                  }
>                  break;
> -            case RELINQUISH:
>              case REMOVE:
>                  {
> +                    ASSERT(pte.p2m.valid);

Why did you add an ASSERT here?

Regards,

-- 
Julien Grall