Re: [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.

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

Hello Sergej,

On 04/07/16 12:45, Sergej Proskurin wrote:
> The p2m intialization now invokes intialization routines responsible for

s/intialization/initialization/

> the allocation and intitialization of altp2m structures. The same

Ditto

> applies to teardown routines. The functionality has been adopted from
> the x86 altp2m implementation.

This patch would benefit to be split in 2:
    1) Moving p2m init/teardown in a separate function
    2) Introducing altp2m init/teardown

It will ease the review.

> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/p2m.c            | 166 ++++++++++++++++++++++++++++++++++++++++--
>   xen/include/asm-arm/domain.h  |   6 ++
>   xen/include/asm-arm/hvm/hvm.h |  12 +++
>   xen/include/asm-arm/p2m.h     |  20 +++++
>   4 files changed, 198 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index aa4e774..e72ca7a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1400,19 +1400,103 @@ static void p2m_free_vmid(struct domain *d)
>       spin_unlock(&vmid_alloc_lock);
>   }
>
> -void p2m_teardown(struct domain *d)
> +static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)

AFAICT, this function is only used by p2m_initialise_one. So I would 
prefer if you fold the code in the latter.

>   {
> -    struct p2m_domain *p2m = &d->arch.p2m;
> +    int ret = 0;
> +
> +    spin_lock_init(&p2m->lock);
> +    INIT_PAGE_LIST_HEAD(&p2m->pages);
> +
> +    spin_lock(&p2m->lock);
> +
> +    p2m->domain = d;
> +    p2m->access_required = false;
> +    p2m->mem_access_enabled = false;
> +    p2m->default_access = p2m_access_rwx;
> +    p2m->p2m_class = p2m_host;
> +    p2m->root = NULL;
> +
> +    /* Adopt VMID of the associated domain */
> +    p2m->vmid = d->arch.p2m.vmid;

It looks like to me that re-using the same VMID will require more TLB 
flush (such as when a VCPU is migrated to another physical CPU). So 
could you explain why you decided to re-use the same VMID?

> +    p2m->vttbr.vttbr = 0;
> +    p2m->vttbr.vttbr_vmid = p2m->vmid;
> +
> +    p2m->max_mapped_gfn = 0;
> +    p2m->lowest_mapped_gfn = ULONG_MAX;
> +    radix_tree_init(&p2m->mem_access_settings);
> +
> +    spin_unlock(&p2m->lock);
> +
> +    return ret;
> +}
> +
> +static void p2m_free_one(struct p2m_domain *p2m)
> +{
> +    mfn_t mfn;
> +    unsigned int i;
>       struct page_info *pg;
>
>       spin_lock(&p2m->lock);
>
>       while ( (pg = page_list_remove_head(&p2m->pages)) )
> -        free_domheap_page(pg);
> +        if ( pg != p2m->root )
> +            free_domheap_page(pg);
> +
> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> +    {
> +        mfn = _mfn(page_to_mfn(p2m->root) + i);
> +        clear_domain_page(mfn);
> +    }
> +    free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
> +    p2m->root = NULL;
> +
> +    radix_tree_destroy(&p2m->mem_access_settings, NULL);
> +
> +    spin_unlock(&p2m->lock);
> +
> +    xfree(p2m);
> +}
> +
> +static struct p2m_domain *p2m_init_one(struct domain *d)
> +{
> +    struct p2m_domain *p2m = xzalloc(struct p2m_domain);
> +
> +    if ( !p2m )
> +        return NULL;
> +
> +    if ( p2m_initialise(d, p2m) )
> +        goto free_p2m;
> +
> +    return p2m;
> +
> +free_p2m:
> +    xfree(p2m);
> +    return NULL;
> +}
> +
> +static void p2m_teardown_hostp2m(struct domain *d)

Why does p2m_teardown_hostp2m not use p2m_teardown_one to teardown the 
p2m? Assuming xfree(p2m) is move out of the function.

> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct page_info *pg = NULL;
> +    mfn_t mfn;
> +    unsigned int i;
> +
> +    spin_lock(&p2m->lock);
>
> -    if ( p2m->root )

Why did you remove this check? The p2m->root could be NULL if the an 
error occurred before create the root page table.

> -        free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
> +    while ( (pg = page_list_remove_head(&p2m->pages)) )
> +        if ( pg != p2m->root )

Why this check? p2m->root will never be part of p2m->pages.

> +        {
> +            mfn = _mfn(page_to_mfn(pg));
> +            clear_domain_page(mfn);
> +            free_domheap_page(pg);
> +        }
>
> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> +    {
> +        mfn = _mfn(page_to_mfn(p2m->root) + i);
> +        clear_domain_page(mfn);
> +    }
> +    free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
>       p2m->root = NULL;
>
>       p2m_free_vmid(d);
> @@ -1422,7 +1506,7 @@ void p2m_teardown(struct domain *d)
>       spin_unlock(&p2m->lock);
>   }
>
> -int p2m_init(struct domain *d)
> +static int p2m_init_hostp2m(struct domain *d)

Why does p2m_init_hostp2m not use p2m_init_one to initialize the p2m?

>   {
>       struct p2m_domain *p2m = &d->arch.p2m;
>       int rc = 0;
> @@ -1437,6 +1521,8 @@ int p2m_init(struct domain *d)
>       if ( rc != 0 )
>           goto err;
>
> +    p2m->vttbr.vttbr_vmid = p2m->vmid;
> +
>       d->arch.vttbr = 0;
>
>       p2m->root = NULL;
> @@ -1454,6 +1540,74 @@ err:
>       return rc;
>   }
>
> +static void p2m_teardown_altp2m(struct domain *d)
> +{
> +    unsigned int i;
> +    struct p2m_domain *p2m;
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( !d->arch.altp2m_p2m[i] )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +        p2m_free_one(p2m);
> +        d->arch.altp2m_vttbr[i] = INVALID_MFN;
> +        d->arch.altp2m_p2m[i] = NULL;

These 2 lines are not necessary because the domain is destroyed and the 
memory associated will be free very soon.

> +    }
> +
> +    d->arch.altp2m_active = false;
> +}
> +
> +static int p2m_init_altp2m(struct domain *d)
> +{
> +    unsigned int i;
> +    struct p2m_domain *p2m;
> +
> +    spin_lock_init(&d->arch.altp2m_lock);
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        d->arch.altp2m_vttbr[i] = INVALID_MFN;

The VTTBR will be stored in altp2m_p2m[i].vttbr. So why do you need to 
store in a different way?

Also, please don't mix value that have different meaning. MFN_INVALID 
indicates that a MFN is invalid not the VTTBR.

> +        d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> +        if ( p2m == NULL )
> +        {
> +            p2m_teardown_altp2m(d);

This call is not necessary. p2m_teardown_altp2m will be called by 
p2m_teardown as part of arch_domain_destroy.

> +            return -ENOMEM;
> +        }
> +        p2m->p2m_class = p2m_alternate;
> +        p2m->access_required = 1;
> +        _atomic_set(&p2m->active_vcpus, 0);
> +    }
> +
> +    return 0;
> +}
> +
> +void p2m_teardown(struct domain *d)
> +{
> +    /*
> +     * We must teardown altp2m unconditionally because
> +     * we initialise it unconditionally.

Why do we need to initialize altp2m unconditionally? When altp2m is not 
used we will allocate memory that is never used.

I would prefer to see the allocation of the memory only if the domain 
will make use of altp2m.

> +     */
> +    p2m_teardown_altp2m(d);
> +
> +    p2m_teardown_hostp2m(d);
> +}
> +
> +int p2m_init(struct domain *d)
> +{
> +    int rc = 0;
> +
> +    rc = p2m_init_hostp2m(d);
> +    if ( rc )
> +        return rc;
> +
> +    rc = p2m_init_altp2m(d);
> +    if ( rc )
> +        p2m_teardown_hostp2m(d);

This call is not necessary.

> +
> +    return rc;
> +}
> +
>   int relinquish_p2m_mapping(struct domain *d)
>   {
>       struct p2m_domain *p2m = &d->arch.p2m;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2039f16..6b9770f 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -29,6 +29,9 @@ enum domain_type {
>   #define is_64bit_domain(d) (0)
>   #endif
>
> +#define MAX_ALTP2M      10 /* arbitrary */
> +#define INVALID_ALTP2M  0xffff

IMHO, this should either be part of p2m.h or altp2m.h

> +
>   extern int dom0_11_mapping;
>   #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
>
> @@ -130,6 +133,9 @@ struct arch_domain
>
>       /* altp2m: allow multiple copies of host p2m */
>       bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +    spinlock_t altp2m_lock;

Please document was the lock is protecting.

> +    uint64_t altp2m_vttbr[MAX_ALTP2M];
>   }  __cacheline_aligned;
>
>   struct arch_vcpu
> diff --git a/xen/include/asm-arm/hvm/hvm.h b/xen/include/asm-arm/hvm/hvm.h
> index 96c455c..28d5298 100644
> --- a/xen/include/asm-arm/hvm/hvm.h
> +++ b/xen/include/asm-arm/hvm/hvm.h
> @@ -19,6 +19,18 @@
>   #ifndef __ASM_ARM_HVM_HVM_H__
>   #define __ASM_ARM_HVM_HVM_H__
>
> +struct vttbr_data {

This structure should not be part of hvm.h but processor.h. Also, I 
would rename it to simply vttbr.

> +    union {
> +        struct {
> +            u64 vttbr_baddr :40, /* variable res0: from 0-(x-1) bit */

Please drop vttbr_. Also, this field is 48 bits for ARMv8 (see ARM 
D7.2.102 in DDI 0487A.j).

> +                res1        :8,
> +                vttbr_vmid  :8,

Please drop vttbr_.

> +                res2        :8;
> +        };
> +        u64 vttbr;
> +    };
> +};
> +
>   struct hvm_function_table {
>       char *name;
>
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 0d1e61e..a78d547 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -8,6 +8,9 @@
>   #include <xen/p2m-common.h>
>   #include <public/memory.h>
>
> +#include <asm/atomic.h>
> +#include <asm/hvm/hvm.h>

ARM has not concept of HVM nor PV. So I would prefer to see a very 
limited usage of hvm.h

> +
>   #define paddr_bits PADDR_BITS
>
>   /* Holds the bit size of IPAs in p2m tables.  */
> @@ -17,6 +20,11 @@ struct domain;
>
>   extern void memory_type_changed(struct domain *);
>
> +typedef enum {
> +    p2m_host,
> +    p2m_alternate,
> +} p2m_class_t;
> +
>   /* Per-p2m-table state */
>   struct p2m_domain {
>       /* Lock that protects updates to the p2m */
> @@ -66,6 +74,18 @@ struct p2m_domain {
>       /* Radix tree to store the p2m_access_t settings as the pte's don't have
>        * enough available bits to store this information. */
>       struct radix_tree_root mem_access_settings;
> +
> +    /* Alternate p2m: count of vcpu's currently using this p2m. */
> +    atomic_t active_vcpus;
> +
> +    /* Choose between: host/alternate */
> +    p2m_class_t p2m_class;

Is there any reason to have this field? It is set but never used.

> +
> +    /* Back pointer to domain */
> +    struct domain *domain;

AFAICT, the only usage of this field is in p2m_altp2m_lazy_copy where 
you have direct access to the domain. So this could be dropped.

> +
> +    /* VTTBR information */
> +    struct vttbr_data vttbr;
>   };
>
>   /* List of possible type for each page in the p2m entry.
>

Regards,

-- 
Julien Grall

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