Re: [PATCH] expand x86 arch_shared_info to support >3 level p2m tree

[Juergen Gross <jgross@suse.com>]

On 09/08/2014 03:59 PM, Andrew Cooper wrote:
> On 08/09/14 14:48, Juergen Gross wrote:
>> The x86 struct arch_shared_info field pfn_to_mfn_frame_list_list
>> currently contains the mfn of the top level page frame of the 3 level
>> p2m tree, which is used by the Xen tools during saving and restoring
>> (and live migration) of pv domains. With three levels of the p2m tree
>> it is possible to support up to 512 GB of RAM for a pv domain.
>
> Specifically only 64bit PV domains have the 512GB limit.
>
> 32bit PV domains have a far larger supported RAM as they can fit twice
> as many mfns in each p2m page.
>
>>   To be
>> able to support more RAM an additional level is to be added.
>>
>> This patch expands struct arch_shared_info with a new p2m tree root
>> and the number of levels of the p2m tree. The new information is
>> indicated by the domain to be valid by storing ~0UL into
>> pfn_to_mfn_frame_list_list (this should be done only if more than
>> three levels are needed, of course).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/include/public/arch-x86/xen.h | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
>> index f35804b..b7fa2b6 100644
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -224,7 +224,13 @@ struct arch_shared_info {
>>       /* Frame containing list of mfns containing list of mfns containing p2m. */
>>       xen_pfn_t     pfn_to_mfn_frame_list_list;
>>       unsigned long nmi_reason;
>> -    uint64_t pad[32];
>> +    /*
>> +     * Following two fields are valid if pfn_to_mfn_frame_list_list contains
>> +     * ~0UL.
>> +     */
>> +    unsigned long p2m_levels;   /* number of levels of p2m tree */
>> +    xen_pfn_t     p2m_root;     /* p2m tree top level mfn */
>> +    uint64_t pad[30];
>
> This padding is now wrong, as unsigned long is only 4 bytes in 32bit,
> and it is adjacent to another unsigned long.

The padding in this case is a nightmare.

For 32 bits arch_shared_info_t is 64 bit aligned due to uint64_t pad[].
This enforces a 4 byte hole before the structure (there are 3 4-byte
fields before it in shared_info_t). And before pad[] there is another
4-byte hole.

And don't forget: on 32 bits xen_pfn_t is 4 bytes, too. I could either
align each new variable explicitly to 8 bytes or I could use a union.

As arch_shared_info_t is at the end of shared_info_t I could just ignore
the alignment/padding ...

What is your preference?


Juergen