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: Fri, 9 Dec 2016 16:19:01 +0530	[thread overview]
Message-ID: <CACtJ1JQ7nCiN2jDvDdmYkk1GvX=Q6a0A9PSPz317EfOUQr04RA@mail.gmail.com> (raw)
In-Reply-To: <fc3c54d1-93d7-2de5-448d-63641741caa0@arm.com>

Hi Julien,

On 6 December 2016 at 21:14, Julien Grall <julien.grall@arm.com> wrote:
>>>> +
>>>> +    /*
>>>> +     * 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.
>
>
> I tend to prefer a more readable code even if it means to have a bit more
> code. This is more maintainable in long-term. In this specific,
> I don't think avoiding an extra variable could justify this.
>
I will modify the code accordingly.

>>
>>>
>>>> --- 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
>>
I believe the 2nd patch should be based on the first patch. So they
should be applied in that order only. Should I send these two patches
as a series like [patch 1/2] and [patch 2/2]?
>>
>> The VMIDs are allocated from arch_init_memory () also (via
>> domain_create ()), which happens before setup_virt_paging ().
>
>
> That's not correct. The domains allocated in arch_init_memory does not
> require to have stage-2 page table (see the DOMCRF_dummy flags). The first
> created domain requiring a VMID will be DOM0 that is initialized after
> setup_virt_paging is called.

Yes. I will move p2m_vmid_allocator_init() inside setup_virt_paging().

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