Re: [PATCH v2 10/40] xen/arm: split MMU and MPU config files from config.h

[Penny Zheng <penny.zheng@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

Hi,


Sorry for the late reply. Got sidetracked by other tasks...


On 2023/1/19 22:20, Julien Grall wrote:
> Hi,
>
> On 13/01/2023 05:28, Penny Zheng wrote:
>> From: Wei Chen <wei.chen@arm.com>
>>
>> Xen defines some global configuration macros for Arm in
>> config.h. We still want to use it for Armv8-R systems, but
>> there are some address related macros that are defined for
>> MMU systems. These macros will not be used by MPU systems,
>> Adding ifdefery with CONFIG_HAS_MPU to gate these macros
>> will result in a messy and hard-to-read/maintain code.
>>
>> So we keep some common definitions still in config.h, but
>> move virtual address related definitions to a new file -
>> config_mmu.h. And use a new file config_mpu.h to store
>> definitions for MPU systems. To avoid spreading #ifdef
>> everywhere, we keep the same definition names for MPU
>> systems, like XEN_VIRT_START and HYPERVISOR_VIRT_START,
>> but the definition contents are MPU specific.
>>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> v1 -> v2:
>> 1. Remove duplicated FIXMAP definitions from config_mmu.h
>> ---
>>   xen/arch/arm/include/asm/config.h     | 103 +++--------------------
>>   xen/arch/arm/include/asm/config_mmu.h | 112 ++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/config_mpu.h |  25 ++++++
>
> I think this patch wants to be split in two. So we keep code movement 
> separate from the introduction of new feature (e.g. MPU).
>
> Furthermore, I think it would be better to name the new header 
> layout_* (or similar).
>
> Lastly, you are going to introduce several file with _mmu or _mpu. I 
> would rather prefer if we create directory instead.
>
>
>>   3 files changed, 147 insertions(+), 93 deletions(-)
>>   create mode 100644 xen/arch/arm/include/asm/config_mmu.h
>>   create mode 100644 xen/arch/arm/include/asm/config_mpu.h
>>
>> diff --git a/xen/arch/arm/include/asm/config.h 
>> b/xen/arch/arm/include/asm/config.h
>> index 25a625ff08..86d8142959 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -48,6 +48,12 @@
>>     #define INVALID_VCPU_ID MAX_VIRT_CPUS
>>   +/* Used for calculating PDX */
>
> I am not entirely sure to understand the purpose of this comment.
>
>> +#ifdef CONFIG_ARM_64
>> +#define FfaRAMETABLE_SIZE        GB(32)
>> +#define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>> +#endif
>> +
>
> Why do you only keep the 64-bit version in config.h?
>
> However... the frametable size is limited by the space we reserve in 
> the virtual address space. This would not be the case for the MPU.
>

Yes, but when calculating variable /pdx_group_valid/, which is defined 
as '''

/unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(/
                       (/FRAMETABLE_NR + PDX_GROUP_COUNT - 1) / 
PDX_GROUP_COUNT)] = { [0] = 1 }/ ''',

it relies on FRAMETABLE_NR to limit array length. If we are trying to 
get rid of the limit for the MPU, hmmm,

it may bring a lot of changes in pdx common codes, for example, maybe 
variable /pdx_group_valid/ needs to

be allocated in runtime, according actual frametable size, at least for 
MPU case.

So, here, I intend to keep the same limit as MMU has for MPU too, or any 
suggestion from you?


> So having the limit in common seems a bit odd. In fact, I think we 
> should look at getting rid of the limit for the MPU.
>
> [...]

[...]

> Cheers,
>
Cheers,

Penny Zheng

[-- Attachment #2: Type: text/html, Size: 5694 bytes --]