| Message ID | 1370605083-15747-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: > Use different affinity for each VCPU and always expose an SMP systems to > the guest. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/domain.c | 11 +++++++++-- > xen/include/asm-arm/processor.h | 6 ++++++ > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index ff1410d..4654c9b 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) > isb(); > > WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); > - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); > + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), > + VMPIDR_EL2); Perhaps we should add v->arch.vmpidr and use that instead? > /* VGIC */ > gic_restore_state(n); > @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) > > /* Default the virtual ID to match the physical */ > d->arch.vpidr = boot_cpu_data.midr.bits; > - d->arch.vmpidr = boot_cpu_data.mpidr.bits; > + /* > + * Expose an SMP systems and remove the AFF0. It will be replace by > + * the VPCU ID I wonder if that instead of basing this on the underlying processor we should fabricate an entirely virtual one? Ian. > + * TODO: handle multi-threading processor > + */ > + d->arch.vmpidr = boot_cpu_data.mpidr.bits & MPIDR_AFF0_MASK; > + d->arch.vmpidr = (d->arch.vmpidr & ~MPIDR_UP) | MPIDR_SMP; > > /* TODO: retrieve the evtchn IRQ from the guest DTS */ > if ( d->domain_id ) > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 1c9d793..5181e7b 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -6,6 +6,12 @@ > /* MIDR Main ID Register */ > #define MIDR_MASK 0xff0ffff0 > > +/* MPIDR Multiprocessor Affinity Register */ > +#define MPIDR_UP (1 << 30) > +#define MPIDR_SMP (1 << 31) > +#define MPIDR_AFF0_SHIFT (0) > +#define MPIDR_AFF0_MASK (0xff << MPIDR_AFF0_SHIFT) > + > /* TTBCR Translation Table Base Control Register */ > #define TTBCR_EAE 0x80000000 > #define TTBCR_N_MASK 0x07
On 06/12/2013 04:11 PM, Ian Campbell wrote: > On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: >> Use different affinity for each VCPU and always expose an SMP systems to >> the guest. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/domain.c | 11 +++++++++-- >> xen/include/asm-arm/processor.h | 6 ++++++ >> 2 files changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index ff1410d..4654c9b 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) >> isb(); >> >> WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); >> - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); >> + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), >> + VMPIDR_EL2); > > Perhaps we should add v->arch.vmpidr and use that instead? As it's a read-only register, why can't we recreate it at each context switch? Adding a new field per cpu is a waste of space mainly when the vcpu structure must not be greater than a page. >> /* VGIC */ >> gic_restore_state(n); >> @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) >> >> /* Default the virtual ID to match the physical */ >> d->arch.vpidr = boot_cpu_data.midr.bits; >> - d->arch.vmpidr = boot_cpu_data.mpidr.bits; >> + /* >> + * Expose an SMP systems and remove the AFF0. It will be replace by >> + * the VPCU ID > > I wonder if that instead of basing this on the underlying processor we > should fabricate an entirely virtual one? Hum .. right, AFF1 could be different to 0. Is it okay if xen exposes all the vcpus in the same cluster? FYI, Linux uses the following approach to interpret MPIDR, in case of an SMP support without mutli-thread support): - AFF0: Processors - AFF1: Clusters - AFF2: Unused On the TC2 with big.LITTLE enabled, A7 and A5 are on different sockets, ie AFF1 fields are not the same. But it's only used for PMU (Performance monitor unit).
On Wed, 2013-06-12 at 23:23 +0100, Julien Grall wrote: > On 06/12/2013 04:11 PM, Ian Campbell wrote: > > > On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: > >> Use different affinity for each VCPU and always expose an SMP systems to > >> the guest. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/arch/arm/domain.c | 11 +++++++++-- > >> xen/include/asm-arm/processor.h | 6 ++++++ > >> 2 files changed, 15 insertions(+), 2 deletions(-) > >> > >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > >> index ff1410d..4654c9b 100644 > >> --- a/xen/arch/arm/domain.c > >> +++ b/xen/arch/arm/domain.c > >> @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) > >> isb(); > >> > >> WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); > >> - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); > >> + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), > >> + VMPIDR_EL2); > > > > Perhaps we should add v->arch.vmpidr and use that instead? > > As it's a read-only register, why can't we recreate it at each context > switch? Just to avoid unnecessary calculations on the context switch path. It's two memory accesses, a shift and an or rather than just one memory access. Maybe that's lost in the noise though. > Adding a new field per cpu is a waste of space mainly when the > vcpu structure must not be greater than a page. How close are we to this? > > >> /* VGIC */ > >> gic_restore_state(n); > >> @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) > >> > >> /* Default the virtual ID to match the physical */ > >> d->arch.vpidr = boot_cpu_data.midr.bits; > >> - d->arch.vmpidr = boot_cpu_data.mpidr.bits; > >> + /* > >> + * Expose an SMP systems and remove the AFF0. It will be replace by > >> + * the VPCU ID > > > > I wonder if that instead of basing this on the underlying processor we > > should fabricate an entirely virtual one? > > Hum .. right, AFF1 could be different to 0. > Is it okay if xen exposes all the vcpus in the same cluster? When Xen becomes multicluster aware then we will have to rethink but I think all vcpus in the same cluster is the correct thing to do for the time being. > FYI, Linux uses the following approach to interpret MPIDR, > in case of an SMP support without mutli-thread support): > - AFF0: Processors > - AFF1: Clusters > - AFF2: Unused This is implementation defined but this interpretation is consistent with example 2 of table B4-12 in the "Recommended use of the MPIDR" section of the ARM ARM. On first glance example 1 seems to fit our use case better but on second reading I think "virtual CPU" in that context is referring to what we would call hyper-threads on x86 and not VCPU in the Xen sense. So following example 2 seems like the way to go. > On the TC2 with big.LITTLE enabled, A7 and A5 are on different sockets, > ie AFF1 fields are not the same. But it's only used for PMU (Performance > monitor unit). >
On 06/13/2013 09:19 AM, Ian Campbell wrote: > On Wed, 2013-06-12 at 23:23 +0100, Julien Grall wrote: >> On 06/12/2013 04:11 PM, Ian Campbell wrote: >> >>> On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: >>>> Use different affinity for each VCPU and always expose an SMP systems to >>>> the guest. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> --- >>>> xen/arch/arm/domain.c | 11 +++++++++-- >>>> xen/include/asm-arm/processor.h | 6 ++++++ >>>> 2 files changed, 15 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>>> index ff1410d..4654c9b 100644 >>>> --- a/xen/arch/arm/domain.c >>>> +++ b/xen/arch/arm/domain.c >>>> @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) >>>> isb(); >>>> >>>> WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); >>>> - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); >>>> + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), >>>> + VMPIDR_EL2); >>> >>> Perhaps we should add v->arch.vmpidr and use that instead? >> >> As it's a read-only register, why can't we recreate it at each context >> switch? > > Just to avoid unnecessary calculations on the context switch path. It's > two memory accesses, a shift and an or rather than just one memory > access. Maybe that's lost in the noise though. > >> Adding a new field per cpu is a waste of space mainly when the >> vcpu structure must not be greater than a page. > > How close are we to this? The current size of each vcpu structure are: - arm32 : 2304 bytes - arm64 : 3328 bytes > >> >>>> /* VGIC */ >>>> gic_restore_state(n); >>>> @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) >>>> >>>> /* Default the virtual ID to match the physical */ >>>> d->arch.vpidr = boot_cpu_data.midr.bits; >>>> - d->arch.vmpidr = boot_cpu_data.mpidr.bits; >>>> + /* >>>> + * Expose an SMP systems and remove the AFF0. It will be replace by >>>> + * the VPCU ID >>> >>> I wonder if that instead of basing this on the underlying processor we >>> should fabricate an entirely virtual one? >> >> Hum .. right, AFF1 could be different to 0. >> Is it okay if xen exposes all the vcpus in the same cluster? > > When Xen becomes multicluster aware then we will have to rethink but I > think all vcpus in the same cluster is the correct thing to do for the > time being. I will rewrite the patch with a virtual MPIDR.
On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: > Use different affinity for each VCPU and always expose an SMP systems to > the guest. What is the bad effect of not doing this for 4.3? Does it fix an observable bug? > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/domain.c | 11 +++++++++-- > xen/include/asm-arm/processor.h | 6 ++++++ > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index ff1410d..4654c9b 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) > isb(); > > WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); > - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); > + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), > + VMPIDR_EL2); > > /* VGIC */ > gic_restore_state(n); > @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) > > /* Default the virtual ID to match the physical */ > d->arch.vpidr = boot_cpu_data.midr.bits; > - d->arch.vmpidr = boot_cpu_data.mpidr.bits; > + /* > + * Expose an SMP systems and remove the AFF0. It will be replace by > + * the VPCU ID > + * TODO: handle multi-threading processor > + */ > + d->arch.vmpidr = boot_cpu_data.mpidr.bits & MPIDR_AFF0_MASK; > + d->arch.vmpidr = (d->arch.vmpidr & ~MPIDR_UP) | MPIDR_SMP; > > /* TODO: retrieve the evtchn IRQ from the guest DTS */ > if ( d->domain_id ) > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 1c9d793..5181e7b 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -6,6 +6,12 @@ > /* MIDR Main ID Register */ > #define MIDR_MASK 0xff0ffff0 > > +/* MPIDR Multiprocessor Affinity Register */ > +#define MPIDR_UP (1 << 30) > +#define MPIDR_SMP (1 << 31) > +#define MPIDR_AFF0_SHIFT (0) > +#define MPIDR_AFF0_MASK (0xff << MPIDR_AFF0_SHIFT) > + > /* TTBCR Translation Table Base Control Register */ > #define TTBCR_EAE 0x80000000 > #define TTBCR_N_MASK 0x07
On 06/13/2013 03:59 PM, Ian Campbell wrote: > On Fri, 2013-06-07 at 12:38 +0100, Julien Grall wrote: >> Use different affinity for each VCPU and always expose an SMP systems to >> the guest. > > What is the bad effect of not doing this for 4.3? Does it fix an > observable bug? I didn't notice a specific bug, Linux uses the MPDIR mainly for optimizing scheduling. Unlike Xen, Linux doesn't use it to know which CPU is booting so we can defer it.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ff1410d..4654c9b 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -150,7 +150,8 @@ static void ctxt_switch_to(struct vcpu *n) isb(); WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2); - WRITE_SYSREG(n->domain->arch.vmpidr, VMPIDR_EL2); + WRITE_SYSREG(n->domain->arch.vmpidr | (n->vcpu_id << MPIDR_AFF0_SHIFT), + VMPIDR_EL2); /* VGIC */ gic_restore_state(n); @@ -495,7 +496,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) /* Default the virtual ID to match the physical */ d->arch.vpidr = boot_cpu_data.midr.bits; - d->arch.vmpidr = boot_cpu_data.mpidr.bits; + /* + * Expose an SMP systems and remove the AFF0. It will be replace by + * the VPCU ID + * TODO: handle multi-threading processor + */ + d->arch.vmpidr = boot_cpu_data.mpidr.bits & MPIDR_AFF0_MASK; + d->arch.vmpidr = (d->arch.vmpidr & ~MPIDR_UP) | MPIDR_SMP; /* TODO: retrieve the evtchn IRQ from the guest DTS */ if ( d->domain_id ) diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 1c9d793..5181e7b 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -6,6 +6,12 @@ /* MIDR Main ID Register */ #define MIDR_MASK 0xff0ffff0 +/* MPIDR Multiprocessor Affinity Register */ +#define MPIDR_UP (1 << 30) +#define MPIDR_SMP (1 << 31) +#define MPIDR_AFF0_SHIFT (0) +#define MPIDR_AFF0_MASK (0xff << MPIDR_AFF0_SHIFT) + /* TTBCR Translation Table Base Control Register */ #define TTBCR_EAE 0x80000000 #define TTBCR_N_MASK 0x07
Use different affinity for each VCPU and always expose an SMP systems to the guest. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/domain.c | 11 +++++++++-- xen/include/asm-arm/processor.h | 6 ++++++ 2 files changed, 15 insertions(+), 2 deletions(-)