Re: [Xen-devel] [PATCH] xen/arm: Implement MPIDR per VCPU

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> ---
> >>  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).
> 



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