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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

On 09/09/2016 06:56 PM, Julien Grall wrote:
> 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.
> 

I will remove the loop, thank you.

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

In the next patch, I will provide a comment stating that the altp2m_lock
protects critical altp2m operation that must not be performed concurrently.

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

Unfortunately, this won't work. We cannot move this define into altp2m.h
as it would result in an header conflict: The header asm/altp2m.h
includes xen/domain.h, which in turn includes asm/domain.h. So by
including asm/altp2m.h in asm/domain.h (as MAX_ALTP2M is used there), we
would create a loop of header dependencies.

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

Cheers,
~Sergej

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