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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/03/2016 08:12 PM, Julien Grall wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxx>
>> + *
>> + * 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.
>

This is true. I did not want to free the entire domain got every time
the hostp2m got changed, while altp2m was active (see
altp2m_propagate_change). But it would not introduce much more overhead
when it does. Thank you.

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void altp2m_teardown(struct domain *d)
>> +{
>> +    unsigned int i;
>> +    struct p2m_domain *p2m;
>> +
>> +    altp2m_lock(d);
>
> The lock is not necessary here.
>

Fair, the domain gets destroyed anyway. Thank you.

>> +
>> +    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.
>

Makes sense.

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

Thanks.

>> +
>> +    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.
>

Ok.

>>  {
>>      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.
>

I will respond to this answer, after I have made sure this is the case.
You are right, the need for this call depends on which point in the
domain initialization process libxl sets the parameter HVM_PARAM_ALTP2M.
Thank you.

>> +        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.
>

Stalled comment, thank you.

>> +    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
>>   */
>>
>

Best regards,
~Sergej


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