| Message ID | 1481782394-14285-2-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi Bhupinder, On 15/12/16 06:13, Bhupinder Thakur wrote: > 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> > --- > xen/arch/arm/p2m.c | 45 +++++++++++++++++++++++++++++++++++------ > xen/include/asm-arm/p2m.h | 2 +- > xen/include/asm-arm/processor.h | 18 ++++++++++++++++- > 3 files changed, 57 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 2327509..b166122 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -7,6 +7,7 @@ > #include <xen/vm_event.h> > #include <xen/monitor.h> > #include <xen/iocap.h> > +#include <xen/xmalloc.h> > #include <public/vm_event.h> > #include <asm/flushtlb.h> > #include <asm/gic.h> > @@ -14,15 +15,23 @@ > #include <asm/hardirq.h> > #include <asm/page.h> > > +#define MAX_VMID_8_BIT (1UL << 8) > +#define MAX_VMID_16_BIT (1UL << 16) > + > +#define INVALID_VMID 0 /* VMID 0 is reserved */ > + > #ifdef CONFIG_ARM_64 > 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 = MAX_VMID_8_BIT; > +#define MAX_VMID max_vmid > #else > /* First level P2M is alway 2 consecutive pages */ > #define P2M_ROOT_LEVEL 1 > #define P2M_ROOT_ORDER 1 > +#define MAX_VMID MAX_VMID_8_BIT > #endif > > #define P2M_ROOT_PAGES (1<<P2M_ROOT_ORDER) > @@ -1219,7 +1228,7 @@ static int p2m_alloc_table(struct domain *d) > > p2m->root = page; > > - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; > + p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid << 48); > > /* > * Make sure that all TLBs corresponding to the new VMID are flushed > @@ -1230,19 +1239,27 @@ static int p2m_alloc_table(struct domain *d) > return 0; > } > > -#define MAX_VMID 256 > -#define INVALID_VMID 0 /* VMID 0 is reserved */ > > static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED; > > /* > - * 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. s/Aarch64/AArch64/ Also I would say "may support" because this is not true for all AArch64 platform. > + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent s/Aarch64/AArch64/ > + * domains. The bitmap space will be allocated dynamically based on > + * whether 8 or 16 bit VMIDs are supported. I am wondering if it would be better to comment #define MAX_VMID instead. E.g /* VMID is by default 8 bit width on AArch64 */ static unsigned int max_vmid = ....; #define MAX_VMID max_vmid /* VMID is always 8 bit width on AArch32 */ #define MAX_VMID MAX_VMID_8_BIT What do you think? > */ > -static DECLARE_BITMAP(vmid_mask, MAX_VMID); > +static unsigned long *vmid_mask; > > static void p2m_vmid_allocator_init(void) > { > + /* > + * allocate space for vmid_mask based on MAX_VMID > + */ > + vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID)); > + > + if ( !vmid_mask ) > + panic("Could not allocate VMID bitmap space"); > + > set_bit(INVALID_VMID, vmid_mask); > } > > @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void) > > unsigned int cpu; > unsigned int pa_range = 0x10; /* Larger than any possible value */ > + bool vmid_8_bit = false; Only one space between bool and vmid_8_bit please. > > for_each_online_cpu ( cpu ) > { > const struct cpuinfo_arm *info = &cpu_data[cpu]; > if ( info->mm64.pa_range < pa_range ) > pa_range = info->mm64.pa_range; > + > + /* set a flag if the current cpu does not suppot 16 bit VMIDs */ s/set/Set/ s/suppot/support/ Please also add a full stop at the end of the sentence. > + if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) > + vmid_8_bit = true; > } > > + /* > + * if the flag is not set then it means all CPUs support 16-bit s/if/If/ > + * VMIDs. > + */ > + if ( !vmid_8_bit ) > + max_vmid = MAX_VMID_16_BIT; > + > /* pa_range is 4 bits, but the defined encodings are only 3 bits */ > if ( pa_range&0x8 || !pa_range_info[pa_range].pabits ) > panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range); This code has changed in xen upstream and the platform will unlikely apply. Please rebase your code on staging. > > val |= VTCR_PS(pa_range); > val |= VTCR_TG0_4K; > + > + /* set the VS bit only if 16 bit VMID is supported */ s/set/Set/ + full stop > + if ( MAX_VMID == MAX_VMID_16_BIT ) > + val |= VTCR_VS; Sorry, I have only spotted until now. Can you print a message to advertise the width of VMID? See the printk("P2M: %d-bit levels..."); > val |= VTCR_SL0(pa_range_info[pa_range].sl0); > val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz); Cheers,
Hi Bhupinder, On 15/12/16 06:13, Bhupinder Thakur wrote: > + > + /* set a flag if the current cpu does not suppot 16 bit VMIDs */ > + if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) > + vmid_8_bit = true; > } > > + /* I just spot a trailing white space here while trying to apply the patch and test it. > + * if the flag is not set then it means all CPUs support 16-bit > + * VMIDs. > + */ Cheers,
Hi Julien, >> -#define MAX_VMID 256 >> -#define INVALID_VMID 0 /* VMID 0 is reserved */ >> >> static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED; >> >> /* >> - * 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. > > > s/Aarch64/AArch64/ ok. > > Also I would say "may support" because this is not true for all AArch64 > platform. > >> + * Using a bitmap here limits us to 256 or 65536 (for Aarch64) concurrent > > > s/Aarch64/AArch64/ > ok. >> + * domains. The bitmap space will be allocated dynamically based on >> + * whether 8 or 16 bit VMIDs are supported. > > > I am wondering if it would be better to comment #define MAX_VMID instead. > E.g > > /* VMID is by default 8 bit width on AArch64 */ > static unsigned int max_vmid = ....; > #define MAX_VMID max_vmid > > /* VMID is always 8 bit width on AArch32 */ > #define MAX_VMID MAX_VMID_8_BIT > > What do you think? Looks fine. I will modify. > >> */ >> -static DECLARE_BITMAP(vmid_mask, MAX_VMID); >> +static unsigned long *vmid_mask; >> >> static void p2m_vmid_allocator_init(void) >> { >> + /* >> + * allocate space for vmid_mask based on MAX_VMID >> + */ >> + vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID)); >> + >> + if ( !vmid_mask ) >> + panic("Could not allocate VMID bitmap space"); >> + >> set_bit(INVALID_VMID, vmid_mask); >> } >> >> @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void) >> >> unsigned int cpu; >> unsigned int pa_range = 0x10; /* Larger than any possible value */ >> + bool vmid_8_bit = false; > > > Only one space between bool and vmid_8_bit please. > ok. >> >> for_each_online_cpu ( cpu ) >> { >> const struct cpuinfo_arm *info = &cpu_data[cpu]; >> if ( info->mm64.pa_range < pa_range ) >> pa_range = info->mm64.pa_range; >> + >> + /* set a flag if the current cpu does not suppot 16 bit VMIDs */ > > > s/set/Set/ > s/suppot/support/ > > Please also add a full stop at the end of the sentence. > ok. > >> + if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) >> + vmid_8_bit = true; >> } >> >> + /* >> + * if the flag is not set then it means all CPUs support 16-bit > > > s/if/If/ > ok. >> + * VMIDs. >> + */ >> + if ( !vmid_8_bit ) >> + max_vmid = MAX_VMID_16_BIT; >> + >> /* pa_range is 4 bits, but the defined encodings are only 3 bits */ >> if ( pa_range&0x8 || !pa_range_info[pa_range].pabits ) >> panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", >> pa_range); > > > This code has changed in xen upstream and the platform will unlikely apply. > Please rebase your code on staging. > I checked with the staging branch. This code is same. I re-based my code on staging branch but there is no change in this code fragment. >> >> val |= VTCR_PS(pa_range); >> val |= VTCR_TG0_4K; >> + >> + /* set the VS bit only if 16 bit VMID is supported */ > > > s/set/Set/ + full stop ok. > >> + if ( MAX_VMID == MAX_VMID_16_BIT ) >> + val |= VTCR_VS; > > > Sorry, I have only spotted until now. Can you print a message to advertise > the width of VMID? > > See the printk("P2M: %d-bit levels..."); > ok. I will add the VMID size in this printk. >> val |= VTCR_SL0(pa_range_info[pa_range].sl0); >> val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz); > > > Cheers, > > -- > Julien Grall
Hi Julien, On 15 December 2016 at 16:00, Julien Grall <julien.grall@arm.com> wrote: > Hi Bhupinder, > > On 15/12/16 06:13, Bhupinder Thakur wrote: >> >> + >> + /* set a flag if the current cpu does not suppot 16 bit VMIDs */ >> + if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) >> + vmid_8_bit = true; >> } >> >> + /* > > > I just spot a trailing white space here while trying to apply the patch and > test it. > >> + * if the flag is not set then it means all CPUs support 16-bit >> + * VMIDs. >> + */ > I will remove the trailing white space. > > Cheers, > > -- > Julien Grall
Hi Bhupinder, On 15/12/16 11:23, Bhupinder Thakur wrote:. > >>> + * VMIDs. >>> + */ >>> + if ( !vmid_8_bit ) >>> + max_vmid = MAX_VMID_16_BIT; >>> + >>> /* pa_range is 4 bits, but the defined encodings are only 3 bits */ >>> if ( pa_range&0x8 || !pa_range_info[pa_range].pabits ) >>> panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", >>> pa_range); >> >> >> This code has changed in xen upstream and the platform will unlikely apply. >> Please rebase your code on staging. >> > I checked with the staging branch. This code is same. I re-based my > code on staging branch but there is no change in this code fragment. The code was changed by commit 7190f2d "fix potential pa_range_info out of bound access" that is present in both staging and master. I've tried to apply the patch on staging and it failed. So you need to rebase it. Cheers,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 2327509..b166122 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -7,6 +7,7 @@ #include <xen/vm_event.h> #include <xen/monitor.h> #include <xen/iocap.h> +#include <xen/xmalloc.h> #include <public/vm_event.h> #include <asm/flushtlb.h> #include <asm/gic.h> @@ -14,15 +15,23 @@ #include <asm/hardirq.h> #include <asm/page.h> +#define MAX_VMID_8_BIT (1UL << 8) +#define MAX_VMID_16_BIT (1UL << 16) + +#define INVALID_VMID 0 /* VMID 0 is reserved */ + #ifdef CONFIG_ARM_64 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 = MAX_VMID_8_BIT; +#define MAX_VMID max_vmid #else /* First level P2M is alway 2 consecutive pages */ #define P2M_ROOT_LEVEL 1 #define P2M_ROOT_ORDER 1 +#define MAX_VMID MAX_VMID_8_BIT #endif #define P2M_ROOT_PAGES (1<<P2M_ROOT_ORDER) @@ -1219,7 +1228,7 @@ static int p2m_alloc_table(struct domain *d) p2m->root = page; - p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid & 0xff) << 48; + p2m->vttbr = page_to_maddr(p2m->root) | ((uint64_t)p2m->vmid << 48); /* * Make sure that all TLBs corresponding to the new VMID are flushed @@ -1230,19 +1239,27 @@ static int p2m_alloc_table(struct domain *d) return 0; } -#define MAX_VMID 256 -#define INVALID_VMID 0 /* VMID 0 is reserved */ static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED; /* - * 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; static void p2m_vmid_allocator_init(void) { + /* + * allocate space for vmid_mask based on MAX_VMID + */ + vmid_mask = xzalloc_array(unsigned long, BITS_TO_LONGS(MAX_VMID)); + + if ( !vmid_mask ) + panic("Could not allocate VMID bitmap space"); + set_bit(INVALID_VMID, vmid_mask); } @@ -1632,20 +1649,36 @@ void __init setup_virt_paging(void) unsigned int cpu; unsigned int pa_range = 0x10; /* Larger than any possible value */ + bool vmid_8_bit = false; for_each_online_cpu ( cpu ) { const struct cpuinfo_arm *info = &cpu_data[cpu]; if ( info->mm64.pa_range < pa_range ) pa_range = info->mm64.pa_range; + + /* set a flag if the current cpu does not suppot 16 bit VMIDs */ + if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) + vmid_8_bit = true; } + /* + * if the flag is not set then it means all CPUs support 16-bit + * VMIDs. + */ + if ( !vmid_8_bit ) + max_vmid = MAX_VMID_16_BIT; + /* pa_range is 4 bits, but the defined encodings are only 3 bits */ if ( pa_range&0x8 || !pa_range_info[pa_range].pabits ) panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range); val |= VTCR_PS(pa_range); val |= VTCR_TG0_4K; + + /* set the VS bit only if 16 bit VMID is supported */ + if ( MAX_VMID == MAX_VMID_16_BIT ) + val |= VTCR_VS; val |= VTCR_SL0(pa_range_info[pa_range].sl0); val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz); diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 0987be2..9de55fc 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -30,7 +30,7 @@ struct p2m_domain { struct page_info *root; /* Current VMID in use */ - uint8_t vmid; + uint16_t vmid; /* Current Translation Table Base Register for the p2m */ uint64_t vttbr; diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 15bf890..48ce59b 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -215,6 +215,8 @@ #define VTCR_PS(x) ((x)<<16) +#define VTCR_VS (_AC(0x1,UL)<<19) + #endif #define VTCR_RES1 (_AC(1,UL)<<31) @@ -269,6 +271,11 @@ /* FSR long format */ #define FSRL_STATUS_DEBUG (_AC(0x22,UL)<<0) +#ifdef CONFIG_ARM_64 +#define MM64_VMID_8_BITS_SUPPORT 0x0 +#define MM64_VMID_16_BITS_SUPPORT 0x2 +#endif + #ifndef __ASSEMBLY__ struct cpuinfo_arm { @@ -337,7 +344,16 @@ struct cpuinfo_arm { unsigned long tgranule_64K:4; unsigned long tgranule_4K:4; unsigned long __res0:32; - }; + + unsigned long hafdbs:4; + unsigned long vmid_bits:4; + unsigned long vh:4; + unsigned long hpds:4; + unsigned long lo:4; + unsigned long pan:4; + unsigned long __res1:8; + unsigned long __res2:32; + }; } mm64; struct {
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> --- xen/arch/arm/p2m.c | 45 +++++++++++++++++++++++++++++++++++------ xen/include/asm-arm/p2m.h | 2 +- xen/include/asm-arm/processor.h | 18 ++++++++++++++++- 3 files changed, 57 insertions(+), 8 deletions(-)