Re: [PATCH v2 07/25] arm/altp2m: Add altp2m init/teardown routines.

[Julien Grall <julien.grall@arm.com>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
> The p2m initialization now invokes initialization routines responsible
> for the allocation and initialization of altp2m structures. The same
> applies to teardown routines. The functionality has been adopted from
> the x86 altp2m implementation.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v2: Shared code between host/altp2m init/teardown functions.
>     Added conditional init/teardown of altp2m.
>     Altp2m related functions are moved to altp2m.c
> ---
>  xen/arch/arm/Makefile        |  1 +
>  xen/arch/arm/altp2m.c        | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c           | 28 +++++++++++++----
>  xen/include/asm-arm/altp2m.h |  6 ++++
>  xen/include/asm-arm/domain.h |  4 +++
>  xen/include/asm-arm/p2m.h    |  5 ++++
>  6 files changed, 110 insertions(+), 5 deletions(-)
>  create mode 100644 xen/arch/arm/altp2m.c
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 23aaf52..4a7f660 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -5,6 +5,7 @@ subdir-$(CONFIG_ARM_64) += efi
>  subdir-$(CONFIG_ACPI) += acpi
>
>  obj-$(CONFIG_ALTERNATIVE) += alternative.o
> +obj-y += altp2m.o
>  obj-y += bootfdt.o
>  obj-y += cpu.o
>  obj-y += cpuerrata.o
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> new file mode 100644
> index 0000000..abbd39a
> --- /dev/null
> +++ b/xen/arch/arm/altp2m.c
> @@ -0,0 +1,71 @@
> +/*
> + * arch/arm/altp2m.c
> + *
> + * Alternate p2m
> + * Copyright (c) 2016 Sergej Proskurin <proskurin@sec.in.tum.de>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License, version 2,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + * FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <asm/p2m.h>
> +#include <asm/altp2m.h>
> +
> +int altp2m_init(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    spin_lock_init(&d->arch.altp2m_lock);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        d->arch.altp2m_p2m[i] = NULL;
> +        d->arch.altp2m_vttbr[i] = INVALID_VTTBR;

I don't think altp2m_vttbr is useful. There is no real performance 
impact to free the whole altp2m if the altp2m is destroyed (see 
altp2m_destroy_by_id) and re-allocated afterwards.

The code will actually much simpler. With this solution you will be able 
to detect if an altp2m is available by testin altp2m_p2m[i] is NULL.

> +    }
> +
> +    return 0;
> +}
> +
> +void altp2m_teardown(struct domain *d)
> +{
> +    unsigned int i;
> +    struct p2m_domain *p2m;
> +
> +    altp2m_lock(d);

The lock is not necessary here.

> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( !d->arch.altp2m_p2m[i] )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +        p2m_free_one(p2m);
> +        xfree(p2m);
> +
> +        d->arch.altp2m_vttbr[i] = INVALID_VTTBR;
> +        d->arch.altp2m_p2m[i] = NULL;

The domain will never be used afterward, so there is no point to set 
altp2m_vttbr and altp2m_p2m.

> +    }
> +
> +    d->arch.altp2m_active = false;

Ditto.

> +
> +    altp2m_unlock(d);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ff9c0d1..29ec5e5 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -14,6 +14,8 @@
>  #include <asm/hardirq.h>
>  #include <asm/page.h>
>
> +#include <asm/altp2m.h>
> +
>  #ifdef CONFIG_ARM_64
>  static unsigned int __read_mostly p2m_root_order;
>  static unsigned int __read_mostly p2m_root_level;
> @@ -1392,7 +1394,7 @@ void p2m_flush_table(struct p2m_domain *p2m)
>          free_domheap_page(pg);
>  }
>
> -static inline void p2m_free_one(struct p2m_domain *p2m)
> +void p2m_free_one(struct p2m_domain *p2m)

Please expose p2m_free_one in patch #4.

>  {
>      p2m_flush_table(p2m);
>
> @@ -1415,9 +1417,13 @@ int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
>      rwlock_init(&p2m->lock);
>      INIT_PAGE_LIST_HEAD(&p2m->pages);
>
> -    rc = p2m_alloc_vmid(p2m);
> -    if ( rc != 0 )
> -        return rc;
> +    /* Reused altp2m views keep their VMID. */
> +    if ( p2m->vmid == INVALID_VMID )
> +    {
> +        rc = p2m_alloc_vmid(p2m);
> +        if ( rc != 0 )
> +            return rc;
> +    }

My suggestion above will avoid this kind of hack.

>
>      p2m->domain = d;
>      p2m->access_required = false;
> @@ -1441,6 +1447,9 @@ static void p2m_teardown_hostp2m(struct domain *d)
>
>  void p2m_teardown(struct domain *d)
>  {
> +    if ( altp2m_enabled(d) )
> +        altp2m_teardown(d);
> +
>      p2m_teardown_hostp2m(d);
>  }
>
> @@ -1460,7 +1469,16 @@ static int p2m_init_hostp2m(struct domain *d)
>
>  int p2m_init(struct domain *d)
>  {
> -    return p2m_init_hostp2m(d);
> +    int rc;
> +
> +    rc = p2m_init_hostp2m(d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( altp2m_enabled(d) )

I am a bit skeptical that you can fully use altp2m with this check. 
p2m_init is created at the very beginning when the domain is allocated. 
So the HVM_PARAM_ALTP2M will not be set.

> +        rc = altp2m_init(d);
> +
> +    return rc;
>  }
>
>  int relinquish_p2m_mapping(struct domain *d)
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index d47b249..79ea66b 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -22,6 +22,9 @@
>
>  #include <xen/sched.h>
>
> +#define altp2m_lock(d)    spin_lock(&(d)->arch.altp2m_lock)
> +#define altp2m_unlock(d)  spin_unlock(&(d)->arch.altp2m_lock)
> +
>  #define altp2m_enabled(d) ((d)->arch.hvm_domain.params[HVM_PARAM_ALTP2M])
>
>  /* Alternate p2m on/off per domain */
> @@ -38,4 +41,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>      return 0;
>  }
>
> +int altp2m_init(struct domain *d);
> +void altp2m_teardown(struct domain *d);
> +
>  #endif /* __ASM_ARM_ALTP2M_H */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index cc4bda0..3c25ea5 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -129,6 +129,10 @@ struct arch_domain
>
>      /* altp2m: allow multiple copies of host p2m */
>      bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +    uint64_t altp2m_vttbr[MAX_ALTP2M];
> +    /* Covers access to members of the struct altp2m. */

I cannot find any "struct altp2m" in the code.

> +    spinlock_t altp2m_lock;
>  }  __cacheline_aligned;
>
>  struct arch_vcpu
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 1f9c370..24a1f61 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -9,6 +9,8 @@
>  #include <xen/p2m-common.h>
>  #include <public/memory.h>
>
> +#define MAX_ALTP2M 10           /* ARM might contain an arbitrary number of
> +                                   altp2m views. */
>  #define paddr_bits PADDR_BITS
>
>  /* Holds the bit size of IPAs in p2m tables.  */
> @@ -212,6 +214,9 @@ void guest_physmap_remove_page(struct domain *d,
>
>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>
> +/* Release resources held by the p2m structure. */
> +void p2m_free_one(struct p2m_domain *p2m);
> +
>  /*
>   * Populate-on-demand
>   */
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel