Re: [PATCH v2] xen/arm: Add support for 16 bit VMIDs

From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
To: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v2] xen/arm: Add support for 16 bit VMIDs
Date: Wed, 30 Nov 2016 19:01:07 +0530	[thread overview]
Message-ID: <CACtJ1JT2RDKE9c3n4QTyuGuJXfiuikYPA=SeqmGg4AGpiy89nQ@mail.gmail.com> (raw)
In-Reply-To: <5ef6c4da-9e15-0967-e257-9fa578e4a17b@arm.com>

Hi Julien,

>>
>> VMID space is increased to 16-bits from 8-bits in ARMv8 8.1 revision.
>> This allows more than 256 VMs to be supported by Xen.
>>
>> This change adds support for 16-bit VMIDs in Xen based on whether the
>> architecture supports it.
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
>
> The tag reviewed-by has a strong meaning, it means that the person has fully reviewed the code and he agrees with the modifications (i.e the code is good to be pushed). For more details see a good description in in Linux doc (see [1]).
>
> In this case, neither Stefano nor I gave it. So please drop them until one of us formally gave him. (e.g Reviewed-by: Name <email>).
>

I will remove the reviewed-by tags. I will check the reference
mentioned by you for the guidelines.

>> @@ -19,6 +20,7 @@ static unsigned int __read_mostly p2m_root_order;
>>  static unsigned int __read_mostly p2m_root_level;
>>  #define P2M_ROOT_ORDER    p2m_root_order
>>  #define P2M_ROOT_LEVEL p2m_root_level
>> +static unsigned int __read_mostly max_vmid;
>
>
> I would much prefer if max_vmid and MAX_VMID are defined together.
>

will move the definitions together.

>
>> - * VTTBR_EL2 VMID field is 8 bits. Using a bitmap here limits us to
>> - * 256 concurrent domains.
>> + * VTTBR_EL2 VMID field is 8 or 16 bits. Aarch64 supports 16-bit VMID.
>> + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent
>> + * domains. The bitmap space will be allocated dynamically based on
>> + * whether 8 or 16 bit VMIDs are supported.
>>   */
>> -static DECLARE_BITMAP(vmid_mask, MAX_VMID);
>> +static unsigned long *vmid_mask=NULL;
>
>
> Coding style here.
>
> vmid_mask = NULL;
>
> However, the NULL is not necessary here as global variable are initialized to 0 by default.
>
ok.

>>
>> -void p2m_vmid_allocator_init(void)
>> +int p2m_vmid_allocator_init(void)
>>  {
>> -    set_bit(INVALID_VMID, vmid_mask);
>> +    int ret=0;
>
>
> Coding style;
>
> int ret = 0;

ok.

>
>> +
>> +#ifdef CONFIG_ARM_64
>> +
>> +    unsigned int cpu;
>> +
>> +    /*
>> +     * initialize max_vmid to 16 bits by default
>> +     */
>> +    max_vmid = MAX_VMID_16_BIT;
>
>
> This could be done at the declaration of max_vmid.
>

ok.

>> +
>> +    /*
>> +     * if any cpu does not support 16-bit VMID then restrict the
>> +     * max VMIDs which can be allocated to 256
>> +     */
>> +    for_each_online_cpu ( cpu )
>> +    {
>> +        const struct cpuinfo_arm *info = &cpu_data[cpu];
>> +
>> +        if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT )
>> +        {
>> +            max_vmid = MAX_VMID_8_BIT;
>> +            break;
>> +        }
>> +    }
>
>
> I still think this is very confusing to consider 16-bit VMID by default as this is an enhancement in a newer architecture.
>
> I would prefer to see the loop inverted, i.e checking whether all the CPUs support 16-bit and set max_vmid to 16 bit if supported.
>
> If you disagree please explain why.
>
The reason for doing it this way was to avoid using another variable
which would tell whether the FOR loop ran to completion (only then the
max_vmid can be set to MAX_VMID_16_BIT). By inverting the check I
avoided that extra variable.

>
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -789,7 +789,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>      gic_init();
>>
>> -    p2m_vmid_allocator_init();
>> +    if ( p2m_vmid_allocator_init() != 0 )
>> +        panic("Could not allocate VMID bitmap space");
>
>
> I am not sure why we have to initialize the VMID allocator far before setting up the stage-2 translation (see call setup_virt_paging).
>
> Overall, VMID are part of stage-2 subsystem. So I think it would be better to move this call in setup_virt_paging.
>
> With that you could take advantage of the for_each_online loop in setup_virt_paging and avoid to have go through again all CPUs.
>
> So what I would like to see is:
>    - Patch #1: move p2m_vmid_allocator_init in setup_virt_paging
>    - Patch #2: Add support for 16 bit VMIDs

The VMIDs are allocated from arch_init_memory () also (via
domain_create ()), which happens before setup_virt_paging ().

Regards,
Bhupinder

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