Re: [Xen-devel] [PATCH v3 14/38] arm/p2m: Add altp2m init/teardown routines

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej

On 16/08/16 23:16, Sergej Proskurin wrote:
> 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..66a373a
> --- /dev/null
> +++ b/xen/arch/arm/altp2m.c
> @@ -0,0 +1,61 @@
> +/*
> + * 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;
The structure domain is already initialized to 0, so this loop is pointless.

> +
> +    d->arch.altp2m_active = false;
> +
> +    return 0;
> +}
> +
[...]

> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index 0711796..a156109 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)
> +
>  /* Alternate p2m on/off per domain */
>  static inline bool_t altp2m_active(const struct domain *d)
>  {
> @@ -36,4 +39,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..a4e4762 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -127,8 +127,14 @@ struct arch_domain
>      paddr_t efi_acpi_len;
>  #endif
>
> +    /*
> +     * Lock that protects access to altp2m related fields in both struct
> +     * arch_domain and struct p2m_domain.
This comment looks wrong. struct p2m_domain is protected by its own 
lock. altp2m lock should not protect it.
> +     */
> +    spinlock_t altp2m_lock;
>      /* altp2m: allow multiple copies of host p2m */
>      bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
>  }  __cacheline_aligned;
>
>  struct arch_vcpu
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 1a004ed..de0c90a 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. */
This should belong to altp2m.h and not p2m.h

>  #define paddr_bits PADDR_BITS
>
>  #define INVALID_VTTBR (0UL)
Regards,

--
Julien Grall

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