[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] xen/arm: Add support for 16 bit VMIDs
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@xxxxxxxxxx> >> Reviewed-by: Julien Grall <julien.grall@xxxxxxx> >> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > > > 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |