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

[Julien Grall <julien.grall@linaro.org>]

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.

-- 
Julien