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

[Julien Grall <julien.grall@xxxxxxx>]

On 30/11/16 13:31, Bhupinder Thakur wrote:
> Hi Julien,
Hi Bhupinder,

Apologies for the late answer.

> > > +
> > > +#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.
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.

> > > --- 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 ().
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.
Regards,

--
Julien Grall

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