Re: [Qemu-arm] [RFC PATCH v1 3/4] translate-all.c: Compute L1 page table properties at runtime

[Peter Maydell]

On 13 June 2016 at 10:25, Paolo Bonzini <address@hidden> wrote:
>
>
> On 13/06/2016 11:08, address@hidden wrote:
>> From: Vijaya Kumar K <address@hidden>
>>
>> Remove L1 page mapping table properties computing
>> statically using macros which is dependent on
>> TARGET_PAGE_BITS. Drop macros V_L1_SIZE, V_L1_SHIFT,
>> V_L1_BITS macros and replace with variables which are
>> computed at early stage of VM boot.
>>
>> Removing dependency can help to make TARGET_PAGE_BITS
>> dynamic.
>>
>> Signed-off-by: Vijaya Kumar K <address@hidden>
>> ---
>>  include/qemu-common.h |    1 +
>>  translate-all.c       |   57 
>> ++++++++++++++++++++++++++++++-------------------
>>  vl.c                  |    3 +++
>>  3 files changed, 39 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index 1f2cb94..d5f0450 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -129,6 +129,7 @@ int parse_debug_env(const char *name, int max, int 
>> initial);
>>
>>  const char *qemu_ether_ntoa(const MACAddr *mac);
>>  void page_size_init(void);
>> +void init_l1_page_table_param(void);
>>
>>  /* returns non-zero if dump is in progress, otherwise zero is
>>   * returned. */
>> diff --git a/translate-all.c b/translate-all.c
>> index 118e7d3..a580ca9 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -57,6 +57,7 @@
>>  #include "qemu/bitmap.h"
>>  #include "qemu/timer.h"
>>  #include "exec/log.h"
>> +#include "qemu/error-report.h"
>>
>>  //#define DEBUG_TB_INVALIDATE
>>  //#define DEBUG_FLUSH
>> @@ -99,25 +100,18 @@ typedef struct PageDesc {
>>  #define V_L2_BITS 10
>>  #define V_L2_SIZE (1 << V_L2_BITS)
>>
>> -/* The bits remaining after N lower levels of page tables.  */
>> -#define V_L1_BITS_REM \
>> -    ((L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS) % V_L2_BITS)
>> -
>> -#if V_L1_BITS_REM < 4
>> -#define V_L1_BITS  (V_L1_BITS_REM + V_L2_BITS)
>> -#else
>> -#define V_L1_BITS  V_L1_BITS_REM
>> -#endif
>> -
>> -#define V_L1_SIZE  ((target_ulong)1 << V_L1_BITS)
>> -
>> -#define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
>> -
>>  uintptr_t qemu_host_page_size;
>>  intptr_t qemu_host_page_mask;
>>
>> +/*
>> + * L1 Mapping properties
>> + */
>> +static unsigned long v_l1_bits;
>> +static unsigned long v_l1_size;
>> +static unsigned long v_l1_shift;
>
> Please make these uint8_t.
>
>>  /* The bottom level has pointers to PageDesc */
>> -static void *l1_map[V_L1_SIZE];
>> +static void *l1_map;
>
> You can make this array have a static V_L2_SIZE * 16 size too.  Peter,
> what do you think?

I don't know this code well enough to have an informed view,
but we only allocate this once at startup, right? I'm not sure
why making it a static array would be better?

thanks
-- PMM